Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

multiplemonomials
Copy link
Contributor

@multiplemonomials multiplemonomials commented Mar 22, 2023

Summary of changes

I noticed that there's an issue in the new STM32 I2C HAL added by #15350 . Trying to do a repeated start using the transactional API:

    // Set read address to 1
    uint8_t const readAddr = 0x01;
    TEST_ASSERT_EQUAL(I2C::Result::ACK, i2c->write(EEPROM_I2C_ADDRESS, reinterpret_cast<const char *>(&readAddr), 1, true));

    // Read the byte back
    uint8_t readByte = 0;
    TEST_ASSERT_EQUAL(I2C::Result::ACK, i2c->read(EEPROM_I2C_ADDRESS | 1, reinterpret_cast<char *>(&readByte), 1));
    TEST_ASSERT_EQUAL_UINT8(0x3, readByte);

simply does not work:
image

This is because the code in get_hal_xfer_options() was returning I2C_LAST_FRAME for the case of stop=false, which is patently wrong -- it should be I2C_FIRST_FRAME, to indicate that we want a start condition but not a stop. I think I inherited this code from the original implementation, but I should have trusted my gut that this wasn't right...

However, that wasn't the only issue. Simply changing I2C_LAST_FRAME to I2C_FIRST_FRAME did fix the repeated starts, but created another issue where some of the test cases would time out trying to do I2C operations. It turns out there's some really wacky logic in the STM32 HAL code where, if you pass that constant and it detects that the previous transfer was the same type of transfer, it simply won't set the START flag. Which... causes it to hang forever and not send any data. Really not sure why it does this. Seriously, I'm scratching my head.

Luckily, the fix is pretty simple: use I2C_OTHER_FRAME instead of I2C_FIRST_FRAME, which activates additional logic which disables the other logic which cancels the start condition. So, we really just want to be using OTHER_FRAME everywhere.

With the new code, I can generate repeated start conditions properly:

image

Impact of changes

Migration actions required

Documentation

None (bugfix)


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 test suite here and it passed!


Reviewers

@0xc0170

@0xc0170 0xc0170 added needs: review release-type: patch Indentifies a PR as containing just a patch labels Mar 22, 2023
@0xc0170
Copy link
Contributor

0xc0170 commented Apr 3, 2023

CI started

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 3, 2023

CI failed, we are investigating, it's internal git error, currently showing in every CI job. We will fix it.

@mbed-ci
Copy link

mbed-ci commented Apr 3, 2023

Jenkins CI Test : ❌ FAILED

Build Number: 8 | 🔒 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_unittests ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test

@mbed-ci
Copy link

mbed-ci commented Apr 5, 2023

Jenkins CI Test : ✔️ SUCCESS

Build Number: 10 | 🔒 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-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 5, 2023

@jeromecoutant Happy with this fix?

@0xc0170 0xc0170 merged commit 65f45cd into ARMmbed:master Apr 11, 2023
@mergify mergify bot removed the ready for merge label Apr 11, 2023
@multiplemonomials multiplemonomials deleted the upstreamed/stm32-i2c-repeated-start-fix branch May 17, 2023 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-type: patch Indentifies a PR as containing just a patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants