Rethink STM32 I2C v2 HAL #15350

multiplemonomials · 2022-11-21T07:50:26Z

Summary of changes

Hi all. As you may have heard, I have been doing a lot of work on my mbed-ce fork to better test and fix up the I2C HAL for STM32 devices. I think that several serious bugs have been fixed, so I'd like to upstream these changes if possible.

In summary, I created a new test program that runs lots of different I2C scenarios against real hardware. As-is, the STM32 I2C v2 HAL failed many of these test cases, and all of the ones that involved single-byte functions (e.g I2C::write(int)). These functions are problematic because they are so low-level that the STM32 HAL cannot be used, and chip registers must be accessed directly to implement them. There were several places where the I2C peripheral could get into a bad state, freezing it and preventing subsequent transactions from going through.

To fix the issue, I rethought how a lot of the low-level I2C code works, implementing a new state enum to track just what the hardware is doing at any one time. After several weeks of nights on the logic analyzer working through test failures, the code is finally able to handle anything I can throw at it, and is much more compliant with how other manufacturers' HALs work.

Impact of changes

This PR is a major rethink of the STM32 I2C code for their v2 I2C peripheral. It tries to get it into a more API-compliant and less buggy state. It should fix several known bugs:

Single-byte API never returns NACK result code on error, only timeout (STM32 I2C HAL does not handle NACKs properly in single-byte mode mbed-ce/mbed-os#61)
Single-byte API returns errors one byte later than the byte that actually caused the error (STM32 I2C HAL does not handle NACKs properly in single-byte mode mbed-ce/mbed-os#61)
Using the single-byte API puts the I2C peripheral in a bad state where the next operation will fail (STM32 I2Cv2 HAL: i2c_stop() silently fails, preventing single-byte I2C from operating correctly mbed-ce/mbed-os#62)

Additionally, I found more issues while making this PR that will also be fixed:

Doing a single-byte operation that fails, then doing a transaction operation after that causes the transaction to return early and blindly report success, because the STOPF flag from earlier was not cleared
Doing an asynchronous read transaction then a synchronous write transaction could cause the asynchronous read transaction to be done a second time due to incorrect logic in the interrupt handler
The entire I2C peripheral was being reinitialized whenever a NACK was received, which is a fairly expensive operation, especially for an interrupt handler. Now NACKs are handled differently from other errors and don't cause a complete re-init.
Doing a repeated start with the single-byte API did not work, because the RELOAD and NBYTES fields have to be zeroed before the repeated start can be sent.

The biggest change this PR makes is the addition of a new state enum, obj_s->state. This provides a simple indicator of what the I2C code is doing at any given time, making certain parts of the logic a lot simpler and enabling some of the fixes to work. I also added a lot of new checks based on this state variable, so if you use the I2C object in an incorrect way, you have a fighting chance of getting a useful error code back instead of undefined behavior. (it's still tough though because certain functions do not have the ability to return an error code...)

Also, up until now, the single-byte API would return after enqueuing the byte into hardware, without waiting for the byte to actually be sent. I think this was done to provide a performance increase, but means that the HAL does not comply with standard Mbed behavior because it cannot return a NACK for a byte until later on, after it's been sent. (and the performance increase didn't mean a whole lot anyway, because it still spinlocks for byte N before sending byte N+1). I have switched things around so it's now compliant and returns the correct result for the byte it was received on.

Migration actions required

Documentation

None, this just brings it into compliance with the existing API.

Pull request type

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[X] Tests / results supplied as part of this PR

I ran my new comprehensive I2C test on both a Nucleo L452RE and a Nucleo H743ZI dev board, and all test cases passed. I also ran the EEPROM test case, and everything passed except the 4096 byte ("full chip") operations. This is due to another bug.

Reviewers

@0xc0170
@jeromecoutant

multiplemonomials · 2022-11-21T07:53:54Z

targets/TARGET_STM/TARGET_STM32F0/objects.h

@@ -77,39 +77,6 @@ struct serial_s {
 #endif
 };

-struct i2c_s {


It's a bit silly that this structure is copied verbatim into like 10 different objects.h headers, despite all of them being identical and being used by a single version of i2c_api.c. Since I had to modify the structure, I consolidated these into a single version in stm_i2c_api.h while I was at it.

ciarmcom · 2022-11-21T08:00:13Z

@multiplemonomials, thank you for your changes.
@0xc0170 @jeromecoutant @ARMmbed/mbed-os-maintainers please review.

0xc0170

There's list of changes in the commit message. Was this pull request squashed? It's not easy to review so much changes within i2c implementation, should this be split into smaller logical changes (my 2 below comments are related to the same remark).

hal/include/hal/i2c_api.h

0xc0170 · 2022-11-24T10:31:06Z

@jeromecoutant Would you find time to review ?

jeromecoutant · 2022-11-24T10:36:35Z

@jeromecoutant Would you find time to review ?

To be honest, for a complete review, I think not...
But I executed tests with CI shield with some families, it is OK.
I would like to start FPGA tests as well

multiplemonomials · 2022-11-25T07:34:36Z

To answer your question, yes this was squashed together from a bunch of commits, many of which were bugfixes. If you want, I can try to break a couple of the standalone fixes (e.g. adding docs, adding return codes) into separate MRs or separate commits. However, I'll warn you, at least 80% of the total changes need to be made at once and reviewed together, or they won't work and they won't make sense.

I definitely understand that this type of change is hard to review -- the STM I2C register level operation is complex and not very intuitive in how it maps to Mbed's I2C API. I can count at least 10 different times when my test broke and it turns out the hardware was working in a way completely different from how I'd expected. But please believe me that I've tested it extensively -- I'm pretty confident that if there were issues in the code, my hardware test cases would catch them (even if the FPGA board tests don't, because they only do one transaction before deleting the I2C object). And even if there are some small issues, please believe me that it's an order of magnitude less buggy than it was before.

I really don't want to let the perfect be the enemy of the good here, especially when people are constantly running into these I2C bugs. I can personally say that, in ~4 years of Mbed OS development, I have seen developers run into this bug on 4 separate occasions, each time causing major havoc (and one time requiring me to drive all the way to a lab at UC San Diego). I really would like to put it to bed.

0xc0170 · 2022-11-25T12:47:24Z

understood, so lets just split what can be (my 2 remarks above, possibly moving struct i2c_s into the header can also be separate logical unit, the rest within i2c implementation can stay to address the bugs there) - separate commits, no need to create another PR.

Pull request has been modified.

0xc0170 · 2022-11-28T15:24:44Z

Can you check the styling check failing (just one line).

I'll need to fix license check, it looks like something broke in the scancode dependencies

0xc0170

Thanks for adding 2 commits, it looks good to me (just styling fixes required).

0xc0170 · 2022-11-28T15:29:19Z

targets/TARGET_STM/i2c_api.c

+    } else {
+
+        if(obj_s->state == STM_I2C_SB_READ_IN_PROGRESS)
+        {


please keep the styling consistent, it uses { attached to the condition, and space after if, see line 1143, please fix all instances in this file

Is there a command I can run to autoformat everything?

Fixed, dang notepad++...

multiplemonomials · 2022-11-28T17:02:52Z

Regarding the license check, feel free to use my patch mbed-ce#104

pombredanne · 2022-11-28T20:47:28Z

I'll need to fix license check, it looks like something broke in the scancode dependencies

@0xc0170 I maintain scancode and this is now fixed. Sorry for the churn!

mbed-ci · 2022-11-29T13:03:03Z

Jenkins CI Test : ❌ FAILED

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	❌
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	❌
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	❌
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	❌
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	❌
jenkins-ci/mbed-os-ci_build-example-ARM	❌

multiplemonomials · 2022-11-29T16:14:18Z

Hmm, do you have any idea why the CI failed? I don't really know how to read the logs

0xc0170 · 2022-11-29T16:25:26Z

Compile [ 97.9%]: i2c_api.c
[Error] i2c_api.c@432,16: no member named 'sda_func' in 'struct i2c_s'
[Error] i2c_api.c@433,16: no member named 'scl_func' in 'struct i2c_s'
[Error] i2c_api.c@507,37: no member named 'sda_func' in 'struct i2c_s'
[Error] i2c_api.c@508,37: no member named 'scl_func' in 'struct i2c_s'
[ERROR] ./mbed-os/targets/TARGET_STM/i2c_api.c:432:16: error: no member named 'sda_func' in 'struct i2c_s'
        obj_s->sda_func = pinmap->sda_function;
        ~~~~~  ^
./mbed-os/targets/TARGET_STM/i2c_api.c:433:16: error: no member named 'scl_func' in 'struct i2c_s'
        obj_s->scl_func = pinmap->scl_function;
        ~~~~~  ^
./mbed-os/targets/TARGET_STM/i2c_api.c:507:37: error: no member named 'sda_func' in 'struct i2c_s'
    pin_function(obj_s->sda, obj_s->sda_func);
                             ~~~~~  ^
./mbed-os/targets/TARGET_STM/i2c_api.c:508:37: error: no member named 'scl_func' in 'struct i2c_s'
    pin_function(obj_s->scl, obj_s->scl_func);
                             ~~~~~  ^
4 errors generated.

This are the errors there for DISCO_L475VG_IOT01A board.

multiplemonomials · 2022-12-04T07:20:02Z

OK, looks like this was a mistake in my original Mbed CE MR. Fixed it there and here.

Pull request has been modified.

0xc0170 · 2022-12-05T12:57:07Z

CI started

mbed-ci · 2022-12-05T13:38:22Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️

multiplemonomials commented Nov 21, 2022

View reviewed changes

ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Nov 21, 2022

ciarmcom requested review from 0xc0170, jeromecoutant and a team November 21, 2022 08:00

ciarmcom added needs: review component: hal component: test devices: st labels Nov 21, 2022

0xc0170 previously requested changes Nov 23, 2022

View reviewed changes

hal/include/hal/i2c_api.h Show resolved Hide resolved

hal/include/hal/i2c_api.h Outdated Show resolved Hide resolved

mergify bot added needs: work and removed needs: review labels Nov 23, 2022

multiplemonomials and others added 2 commits November 27, 2022 21:14

Rethink STM32 I2C v2 HAL

0751932

Add documentation for I2C_EVENT macros

63fbaaf

multiplemonomials force-pushed the upstreamed/rethink-stm32-i2c-hal branch from a567b81 to a850734 Compare November 28, 2022 05:19

0xc0170 reviewed Nov 28, 2022

View reviewed changes

Add some additional I2C error codes

0c2ee4f

multiplemonomials force-pushed the upstreamed/rethink-stm32-i2c-hal branch from a850734 to 0c2ee4f Compare November 29, 2022 08:12

0xc0170 previously approved these changes Nov 29, 2022

View reviewed changes

mergify bot added needs: CI and removed needs: work labels Nov 29, 2022

mergify bot added needs: work needs: CI and removed needs: work needs: CI labels Nov 29, 2022

mergify bot added needs: work and removed needs: CI labels Nov 29, 2022

Fix compile error on static pinmap targets

d59c34b

0xc0170 approved these changes Dec 5, 2022

View reviewed changes

mergify bot added needs: CI and removed needs: work labels Dec 5, 2022

jeromecoutant approved these changes Dec 5, 2022

View reviewed changes

mergify bot added ready for merge and removed needs: CI labels Dec 5, 2022

0xc0170 merged commit d4df713 into ARMmbed:master Dec 5, 2022

mergify bot removed the ready for merge label Dec 5, 2022

saheerb added release-version: 6.17.0 Release-pending release-version: 6.18.0 labels Feb 23, 2023

mbedmain removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Feb 28, 2023

multiplemonomials mentioned this pull request Mar 22, 2023

Fix repeated start for transactional I2C API on STM32 devices with I2C v2 #15394

Merged

multiplemonomials deleted the upstreamed/rethink-stm32-i2c-hal branch May 17, 2023 06:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rethink STM32 I2C v2 HAL #15350

Rethink STM32 I2C v2 HAL #15350

multiplemonomials commented Nov 21, 2022

multiplemonomials Nov 21, 2022

ciarmcom commented Nov 21, 2022

0xc0170 left a comment •

edited

Loading

0xc0170 commented Nov 24, 2022

jeromecoutant commented Nov 24, 2022

multiplemonomials commented Nov 25, 2022 •

edited

Loading

0xc0170 commented Nov 25, 2022 •

edited

Loading

0xc0170 commented Nov 28, 2022

0xc0170 left a comment

0xc0170 Nov 28, 2022

multiplemonomials Nov 28, 2022

multiplemonomials Nov 29, 2022

multiplemonomials commented Nov 28, 2022

pombredanne commented Nov 28, 2022

mbed-ci commented Nov 29, 2022

multiplemonomials commented Nov 29, 2022

0xc0170 commented Nov 29, 2022 •

edited

Loading

multiplemonomials commented Dec 4, 2022

0xc0170 commented Dec 5, 2022

mbed-ci commented Dec 5, 2022

Rethink STM32 I2C v2 HAL #15350

Rethink STM32 I2C v2 HAL #15350

Conversation

multiplemonomials commented Nov 21, 2022

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

multiplemonomials Nov 21, 2022

Choose a reason for hiding this comment

ciarmcom commented Nov 21, 2022

0xc0170 left a comment • edited Loading

Choose a reason for hiding this comment

0xc0170 commented Nov 24, 2022

jeromecoutant commented Nov 24, 2022

multiplemonomials commented Nov 25, 2022 • edited Loading

0xc0170 commented Nov 25, 2022 • edited Loading

0xc0170 commented Nov 28, 2022

0xc0170 left a comment

Choose a reason for hiding this comment

0xc0170 Nov 28, 2022

Choose a reason for hiding this comment

multiplemonomials Nov 28, 2022

Choose a reason for hiding this comment

multiplemonomials Nov 29, 2022

Choose a reason for hiding this comment

multiplemonomials commented Nov 28, 2022

pombredanne commented Nov 28, 2022

mbed-ci commented Nov 29, 2022

Jenkins CI Test : ❌ FAILED

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

multiplemonomials commented Nov 29, 2022

0xc0170 commented Nov 29, 2022 • edited Loading

multiplemonomials commented Dec 4, 2022

0xc0170 commented Dec 5, 2022

mbed-ci commented Dec 5, 2022

Jenkins CI Test : ✔️ SUCCESS

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

0xc0170 left a comment •

edited

Loading

multiplemonomials commented Nov 25, 2022 •

edited

Loading

0xc0170 commented Nov 25, 2022 •

edited

Loading

0xc0170 commented Nov 29, 2022 •

edited

Loading