STM32 I2C: Replace call to wait_us by a sw loop #6538

bcostm · 2018-04-04T13:42:40Z

Description

Replace the call to wait_us() with a sw loop using us_ticker_read.

This has been already done in PR #6069 but canceled due to another fix.

Tested OK with I2C ci-tests on different boards
report_CISHIELD_fix_i2c_wait.txt

cc @RobMeades Maybe verify it also on your platforms ?
cc @LMESTM

Pull request type

[X] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

RobMeades · 2018-04-04T13:45:13Z

@fahim-ublox can you make sure this is OK on C030/C027?
@andreaslarssonublox I guess we should check Odin also...?

0xc0170 · 2018-04-04T13:56:49Z

targets/TARGET_STM/i2c_api.c

 /* Timeout values for flags and events waiting loops. These timeouts are
   not based on accurate values, they just guarantee that the application will
   not remain stuck if the I2C communication is corrupted. 
 */
 #define FLAG_TIMEOUT ((int)0x1000)

+/* Instead of using wait_us() which can causes issues in some cases */
+#define WAIT_LOOP {uint32_t waitloop_timeout = BYTE_TIMEOUT_D2; while(--waitloop_timeout != 0);}


This wont be different with optimizations (different times) ? wont be optimized out?

Also, instead of macro , use a function. Should do the same trick, but more readable

bcostm · 2018-04-05T13:10:41Z

I have replaced the macro by a function and added some pragma to avoid compiler optimization. Tested with ARM, IAR and GCC. The loop time is between 3us to 10us depending of the board/compiler.

0xc0170 · 2018-04-05T13:42:13Z

In some case the call to wait_us() can cause issues. The idea is to not use the us_ticker anymore but instead a simple sw loop.

I believe a problem was wait() that can cause an issue if object is created during the boot and RTOs might not be initialized.

This can be solved as its here, but looking at the wait loop implementation - it shall fix it but having the optimization set in the code, and relying on it might not be the best approach. Why can't we use us ticker api. I recall some other HAL implementation uses it - what should fix this as well. Using us ticker read function and loop until the time passes?

bcostm · 2018-04-05T15:39:23Z

OK... I changed it by using us_ticker_read and loop timings are of course closer to the expected 1us...

0xc0170 · 2018-04-05T15:46:10Z

/morph build

mbed-ci · 2018-04-05T16:28:09Z

Build : SUCCESS

Build number : 1667
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6538/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-04-05T18:36:29Z

Exporter Build : SUCCESS

Build number : 1301
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6538/

mbed-ci · 2018-04-05T19:23:17Z

Test : FAILURE

Build number : 1462
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6538/1462

RobMeades · 2018-04-06T12:55:15Z

We seem to have one I2C test failure that occurs when we include the [original version] of this PR. We're just running the test again with the latest version, back with you soon...

LMESTM · 2018-04-06T13:07:27Z

@0xc0170 @bcostm
I think that it would be great to make wait_loop part of the HAL API so that other drivers can use it as well, what do you think ?
It just happens that I need about the same :-)

LMESTM · 2018-04-06T13:09:36Z

One comment: we're assuming here that us_ticker makes use of 1 microsecond - this is tru for all STM32 targets as far as I know, nevertheless this is not a requirement in mbed

bcostm · 2018-04-06T13:22:48Z

I think that it would be great to make wait_loop part of the HAL API so that other drivers can use it as well, what do you think ?

+1
I was thinking the same thing... Maybe place it in mbed_us_ticker_api.c ? and rename it to us_ticker_wait_loop ?

RobMeades · 2018-04-06T14:00:46Z

We've now tested with the latest version of this PR and all our tests are passing. We must have an I2C chip with slightly sensitive timing; it was a particular I2C command, RESET, that had been failing. Returning to an accurate wait was probably what we needed.

So we're OK with this PR.

0xc0170 · 2018-04-06T14:51:34Z

@RobMeades Thanks for testing and the feedback.

I was thinking the same thing... Maybe place it in mbed_us_ticker_api.c ? and rename it to us_ticker_wait_loop ?

@bulislaw @c1728p9

0xc0170 · 2018-04-06T14:51:53Z

/morph test

mbed-ci · 2018-04-06T18:47:40Z

Test : FAILURE

Build number : 1470
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6538/1470

0xc0170 · 2018-04-09T15:14:05Z

cc @studavekar - can you review the failures please?

/morph test

mbed-ci · 2018-04-09T23:33:50Z

Test : FAILURE

Build number : 1482
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6538/1482

0xc0170 · 2018-04-10T07:55:04Z

@bcostm Can you please review the test results? What commit is your branch based on?

bcostm · 2018-04-10T09:10:49Z

I am on master branch. I have rebased this PR.

0xc0170 · 2018-04-13T14:01:02Z

/morph build

mbed-ci · 2018-04-13T14:21:10Z

Build : SUCCESS

Build number : 1745
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6538/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

kjbracey · 2018-04-13T16:36:48Z

targets/TARGET_STM/i2c_api.c

+    t1 = us_ticker_read();
+    do {
+        t2 = us_ticker_read();
+        elapsed = (t2 > t1) ? (t2 - t1) : ((uint64_t)t2 + 0xFFFFFFFF - t1 + 1);


All the above isn't necessary - just elapsed = t2 - t1; will suffice even on wrap because of the way unsigned arithmetic works.

OK. Thanks for the review. I will update it.

are you sure ? we don't know if the ticker is 32 bits or 16 bits wide ...

I wasn't aware the ticker could be less than 32 bits wide, but seems it can.

But if it can be, that wrap logic wouldn't be correct. It's adding 2^32 when it sees it wrap. It should be adding 2^(ticker width), right?

I've seen similar wrap logic in other places in the tree, with both 2^16 and 2^32 increments, apparently with hard knowledge of their timer width. For this platform, it seems to be 32-bit.

You're right ! the bit width of timer needs to be used. In STM32 targets we have both 32-bits and 16-bits timers and the I2C driver is common for all ... I definitely think that this function should be implemented carefully and would better belong to common code in mbed hal common layer for instance.

If it can be 16-bit for you, I think you should be implementing us_ticker_get_info() to reveal that to the world, which I don't see in any STM32 target directory. That is relied on to get the wrap right in some existing code.

Double-checking, looks like you implement your own 16->32-bit expansion in us_ticker_16b.c. So your us_ticker_read is always 32-bit.

The implementation of us_ticker_get_info() is done in ticker feature branch. This should land on master very soon. So this needs to be taken into account from now on.

mbed-ci · 2018-04-14T00:13:54Z

Exporter Build : SUCCESS

Build number : 1380
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6538/

mbed-ci · 2018-04-14T05:16:59Z

Test : SUCCESS

Build number : 1548
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6538/1548

cmonr · 2018-04-17T21:00:13Z

Restarting CI since it appears that PRs have been rebased since the last run.

/morph build

mbed-ci · 2018-04-17T21:49:24Z

Build : SUCCESS

Build number : 1778
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6538/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-04-18T05:10:28Z

Exporter Build : SUCCESS

Build number : 1421
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6538/

kjbracey · 2018-04-18T08:45:23Z

targets/TARGET_STM/i2c_api.c

 /* Timeout values for flags and events waiting loops. These timeouts are
   not based on accurate values, they just guarantee that the application will
   not remain stuck if the I2C communication is corrupted. 
 */
 #define FLAG_TIMEOUT ((int)0x1000)

+/* This function assumes the tick is 1 us */


At a minimum should also comment that it assumes it's 32-bit - not sure if@LMESTM is happy with that assumption for this patch, given his comments.

bcostm · 2018-04-18T09:32:33Z

I close this PR and I have open Issue #6664 instead in order to have this function at mbed level and not in ST platform only.

mbed-ci · 2018-04-18T10:12:02Z

Test : SUCCESS

Build number : 1588
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6538/1588

0xc0170 added the needs: review label Apr 4, 2018

0xc0170 reviewed Apr 4, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Apr 5, 2018

0xc0170 previously approved these changes Apr 5, 2018

View reviewed changes

0xc0170 added needs: CI and removed needs: work labels Apr 5, 2018

LMESTM approved these changes Apr 6, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: CI labels Apr 10, 2018

bcostm dismissed 0xc0170’s stale review via 811a334 April 10, 2018 09:10

bcostm force-pushed the fix_i2c_wait branch from 1a137c2 to 811a334 Compare April 10, 2018 09:10

0xc0170 added needs: CI and removed needs: work labels Apr 13, 2018

kjbracey reviewed Apr 13, 2018

View reviewed changes

bcostm added 5 commits April 16, 2018 15:48

STM32 I2C: change the call to wait_us

83fe2ba

STM32 I2C: use a function instead of a macro

3ffc588

STM32 I2C: use us_ticker_read

4f966ed

STM32 I2C: rename the wait function

7e8a9a1

STM32 I2C: add timeout check and simplify wait loop

e698f89

bcostm force-pushed the fix_i2c_wait branch from 811a334 to e698f89 Compare April 16, 2018 14:06

cmonr added the release-version: 5.8.3 label Apr 17, 2018

kjbracey reviewed Apr 18, 2018

View reviewed changes

bcostm mentioned this pull request Apr 18, 2018

Wait function using us_ticker_read #6664

Closed

bcostm closed this Apr 18, 2018

sg- removed the needs: CI label Apr 18, 2018

bcostm mentioned this pull request May 4, 2018

Update the STM32 USB driver to the new API #6555

Merged

bcostm deleted the fix_i2c_wait branch July 3, 2018 13:05

STM32 I2C: Replace call to wait_us by a sw loop #6538

STM32 I2C: Replace call to wait_us by a sw loop #6538

Uh oh!

Conversation

bcostm commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Uh oh!

RobMeades commented Apr 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bcostm commented Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Apr 5, 2018

Uh oh!

bcostm commented Apr 5, 2018

Uh oh!

0xc0170 commented Apr 5, 2018

Uh oh!

mbed-ci commented Apr 5, 2018

Triggering tests

Uh oh!

mbed-ci commented Apr 5, 2018

Uh oh!

mbed-ci commented Apr 5, 2018

Uh oh!

RobMeades commented Apr 6, 2018

Uh oh!

LMESTM commented Apr 6, 2018

Uh oh!

LMESTM commented Apr 6, 2018

Uh oh!

bcostm commented Apr 6, 2018

Uh oh!

RobMeades commented Apr 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Apr 6, 2018

Uh oh!

0xc0170 commented Apr 6, 2018

Uh oh!

mbed-ci commented Apr 6, 2018

Uh oh!

0xc0170 commented Apr 9, 2018

Uh oh!

mbed-ci commented Apr 9, 2018

Uh oh!

0xc0170 commented Apr 10, 2018

Uh oh!

bcostm commented Apr 10, 2018

Uh oh!

0xc0170 commented Apr 13, 2018

Uh oh!

mbed-ci commented Apr 13, 2018

Triggering tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbed-ci commented Apr 14, 2018

Uh oh!

mbed-ci commented Apr 14, 2018

Uh oh!

cmonr commented Apr 17, 2018

bcostm commented Apr 4, 2018 •

edited

Loading

bcostm commented Apr 5, 2018 •

edited

Loading

RobMeades commented Apr 6, 2018 •

edited

Loading