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.

Sign up for GitHub

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

Jump to bottom

STM: Add separate flags for I2C slave transfer in progress #15499

Merged
merged 1 commit into from
May 3, 2024
Merged

STM: Add separate flags for I2C slave transfer in progress #15499

merged 1 commit into from
May 3, 2024

Conversation

agausmann
Copy link
Contributor

Summary of changes

Fixes #15498

Adds 2 boolean flags to the STM32 i2c_s object, to indicate whether a transfer is in progress, separate from the existing "transfer pending" flags.

i2c_slave_write, i2c_slave_read and their associated callbacks are modified to use these flags in addition to the pending flags.
The original behavior of the pending flags is preserved.

Impact of changes

Migration actions required

Documentation

None

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)
[] Tests / results supplied as part of this PR

Not sure what to do for regression tests for this patch.

Reviewers

@agausmann
Copy link
Contributor Author

The result of rebuilding https://github.com/agausmann/mbed-i2c-stall-repro with this patch:

https://asciinema.org/a/eVHMaz81ZXvxFTevgmtHLAaxY

@agausmann agausmann changed the title Add separate flags for I2C slave transfer in progress STM: Add separate flags for I2C slave transfer in progress Mar 12, 2024
@multiplemonomials
Copy link
Contributor

Nice catch on this, and this seems like a good fix! We'd love to have this PR over at mbed-ce/mbed-os!

By the way, as to regression tests, I actually have some ideas about that. I've been putting together a PCB which can test lots of different communications busses, and I also put together an I2C master test suite (which is what let me rewrite and fix all the STM32 I2C master mode code a year or two ago). Haven't really worked on slave mode yet I'm afraid...

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 14, 2024

@ARMmbed/team-st-mcd Please review

@0xc0170 0xc0170 added needs: review release-type: patch Indentifies a PR as containing just a patch labels Mar 14, 2024
0xc0170
0xc0170 previously requested changes Mar 28, 2024
View reviewed changes
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you extend the commit message ? Th details provided in this PR should be also part of the commit message.

@multiplemonomials
Copy link
Contributor

multiplemonomials commented Mar 28, 2024 •
edited
Loading

Do you want like a 1 paragraph summary, or the entire post?

EDIT: oops I thought this notification was for my other PR

@agausmann
Add separate flags for I2C slave transfer in progress
dc49c2b 
Fixes ARMmbed#15498

Adds 2 boolean flags to the STM32 `i2c_s` object
to indicate whether a transfer is in progress,
separate from the existing "transfer pending" flags.

`i2c_slave_write`, `i2c_slave_read` and their associated callbacks
are modified to use these flags in addition to the pending flags.
The original behavior of the pending flags is preserved.
@agausmann
Copy link
Contributor Author

I assume it's just the summary section. Updated

@mergify mergify bot dismissed 0xc0170’s stale review March 28, 2024 15:50

Pull request has been modified.

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 10, 2024

I am having issues now with Github (cant run the CI checks). I'll try tomorrow

@mergify mergify bot added needs: CI and removed needs: work labels Apr 10, 2024
@0xc0170
Copy link
Contributor

0xc0170 commented Apr 12, 2024

CI started

@mbed-ci
Copy link

mbed-ci commented Apr 12, 2024

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-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ❌

@mergify mergify bot added needs: work and removed needs: CI labels Apr 12, 2024
@0xc0170
Copy link
Contributor

0xc0170 commented Apr 24, 2024

I need to restart CI, lot of devices were not available

@mergify mergify bot added needs: work and removed needs: CI labels Apr 24, 2024
@mbed-ci
Copy link

mbed-ci commented Apr 29, 2024

Jenkins CI Test : ❌ FAILED

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_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 ❌

@0xc0170 0xc0170 merged commit 1060154 into ARMmbed:master May 3, 2024
18 of 20 checks passed
@mergify mergify bot removed the ready for merge label May 3, 2024
@agausmann agausmann deleted the i2c-slave-flags branch May 3, 2024 17:13
@multiplemonomials multiplemonomials mentioned this pull request Jul 20, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

Assignees
No one assigned
Labels
release-type: patch Indentifies a PR as containing just a patch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

STM32 I2CSlave race condition causing timeouts
4 participants
@agausmann @multiplemonomials @0xc0170 @mbed-ci