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

stm32_common/i2c_2: Fix repeated read condition #12571

Merged
merged 1 commit into from
Oct 25, 2019

Conversation

MrKevinWeiss
Copy link
Contributor

@MrKevinWeiss MrKevinWeiss commented Oct 25, 2019

Contribution description

Fix the condition to return -ENOPNOTSUPP when i2c repeated read attempted.
Currently the error occures even if a read after write is attempted.
This is the standard way to i2c_read_reg which should be supported.
The -EOPNOTSUPP requires the previous R/W state to be reading.
This means a I2C_SR2_TRA must be checked to be 0.

This bug was caught by @aabadie with and using saul in iotlabs testbed.

Testing procedure

To test you can use the iotlab-m3 and test the lps331ap driver:

BOARD=iotlab-m3 IOTLAB_NODE=<my_iotlab_node> make flash term -C tests/driver_lpsxxx/

To ensure the no support error is correctly working:
BOARD=iotlab-m3 IOTLAB_NODE=<my_iotlab_node> make flash term -C tests/periph_i2c/

> i2c_write_bytes 0 0x5C 4 0x0F
Success: i2c_0 wrote 1 bytes
> i2c_read_bytes 0 0X5C 1 0
Success: i2c_0 read 1 byte(s) : [0xbb]
# This means i2c_read_reg is now fixed

> i2c_read_bytes 0 0x5C 1 4
Success: i2c_0 read 1 byte(s) : [0xbb]
> i2c_read_bytes 0 0x5C 1 0
Error: EOPNOTSUPP [95]
# This means the desired error code is still returned

Also testing procedure is documented in #12564

Issues/PRs references

Fixes #12564
Request to add test for CI RIOT-OS/RobotFW-tests#39

Fix the condition to return -ENOPNOTSUPP when i2c repeated read attempted.
Currently the error occures even if a read after write is attempted.
This is the standard way to i2c_read_reg which should be supported.
The -EOPNOTSUPP requires the previous R/W state to be reading.
This means a `I2C_SR2_TRA` must be checked to be 0.
@MrKevinWeiss MrKevinWeiss requested review from aabadie and smlng October 25, 2019 08:44
@MrKevinWeiss MrKevinWeiss self-assigned this Oct 25, 2019
@MrKevinWeiss
Copy link
Contributor Author

@aabadie ya, I don't know what I was thinking... Sorry. Also weird that the CI didn't catch that. I will have to look into that.

@MrKevinWeiss MrKevinWeiss added Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Oct 25, 2019
@aabadie
Copy link
Contributor

aabadie commented Oct 25, 2019

I confirm #12564 is fixed by this PR. Thanks for tackling it so quick.

@MrKevinWeiss
Copy link
Contributor Author

Thanks for finding it, silly mistake on my part.

Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

Reasoning for the fix is OK and comment is explaining it.

Before approving, it would be great to verify other affected STMs are also working (F2, L1, F4). But maybe this is not mandatory ?

@MrKevinWeiss
Copy link
Contributor Author

They typically have the same base register map...
Checking the following reference manuals I can see that bit available:
[x] stm32f205
[x] stm32l152
[x] stm32f411

Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

Then let's go.

ACK

@aabadie aabadie merged commit 60748ae into RIOT-OS:master Oct 25, 2019
@MrKevinWeiss MrKevinWeiss deleted the pr/fix/stm32/i2c/rreadbytes branch October 25, 2019 10:38
@MrKevinWeiss
Copy link
Contributor Author

❤️

@fjmolinas fjmolinas added this to the Release 2020.01 milestone Dec 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid I2C sensors values on iotlab-m3
3 participants