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

cpu/stm32_common: Bug fixes for stm i2c #10540

Closed
wants to merge 3 commits into from
Closed

cpu/stm32_common: Bug fixes for stm i2c #10540

wants to merge 3 commits into from

Conversation

MrKevinWeiss
Copy link
Contributor

@MrKevinWeiss MrKevinWeiss commented Dec 3, 2018 •
edited
Loading

Contribution description

This PR fixes many bugs discovered by the HiL.

  • Fixes remappable sda and scl pins on stm32f1xx boards
  • Refactors and provides accurate features for both i2c_1 and i2c_2
  • Uses common i2c functions when possible
  • Documents hardware constraints
  • Prevents or recovers from lockup states
  • Reduces code size
  • More accurately matches i2c API

Example of bytes saved with build sizes of tests/periph_i2c

BOARD with PR master (at the time) bytes saved
nucleo-l073rz DEVELHELP=1 16004 16188 184
nucleo-l073rz DEVELHELP=0 14952 15140 188
nucleo-f410rb DEVELHELP=1 15936 15992 56
nucleo-f410rb DEVELHELP=0 14976 15000 24

Testing procedure

Test on stm32f1, stm32f4 and (stm32f3, f0, l0 or l4)
One can run the robot-test from the HiL branch or run term in tests/periph_i2c/
Or run manually against your favorite i2c device. No everything will be supported but everything should at least give correct error messages and not lock up the bus.
*If nucleo-f103rb is used it needs external pullups.

  1. Basic read regs with 1,2,3 bytes
  2. Basic write regs with 1,2,3 bytes
  3. Test split frame (NOSTOP, NOSTOP|NOSTART, NOSTART) reads
  4. Test split frame writes
  5. Repeated start write (write_bytes+NOSTOP, write_bytes)
  6. Getting an address nack then read a proper register.

Issues/PRs references

fixes #10395
helps with stm32 based boards for #3366
checks some boxes in #9518

@MrKevinWeiss MrKevinWeiss added Platform: ARM Platform: This PR/issue effects ARM-based platforms State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports labels Dec 3, 2018
@MrKevinWeiss MrKevinWeiss self-assigned this Dec 3, 2018
@MrKevinWeiss
Copy link
Contributor Author

Still a WIP but I tried to really clean up the i2c_2.c since most things were not working. I think I may have to also do the same for i2c_1.c, at least get it into a state that it can use the standard read/write regs.

I can try to setup so the HiL tests it too.

@miri64 miri64 added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Dec 4, 2018
@MrKevinWeiss MrKevinWeiss changed the title cpu/stm32_common/i2c: Bug fixes for stm i2c cpu/stm32_common: Bug fixes for stm i2c Dec 7, 2018
@MrKevinWeiss MrKevinWeiss added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Dec 7, 2018
@MrKevinWeiss
Copy link
Contributor Author

@aabadie ready for review, it is easier if you go by each commit. They should all compile and be testable separately as suggested by @cladmi

@MrKevinWeiss
cpu/stm32f1: Add remap support for stm32f1 cpus and boards
154fabd 
This commit fixes configuration problems when trying to use i2c pins that need to be remapped.
All B8 and B9 pins for STM32F1 need to be remapped, so a check is done if the remappable pins are selected.
@MrKevinWeiss
cpu/stm_common: Refactor and fix implementation for i2c_2
d3d125d 
Refactors i2c_2 to match the structure of i2c_1 better.
Corrects functionality issues.
Allows the common implementation of read_regs and write_regs.
Documents contraints of hardware.
Matches error messages with API.
@MrKevinWeiss
cpu/stm_common: Refactor and cleanup i2c_1
e402359 
Refactor to use common read_regs.
Add error reporting and handling for unsupported low level commands.
Document hardware constraints.
aabadie
aabadie requested changes Dec 13, 2018
View reviewed changes
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.

Very nice work but there are quite some changes. Maybe the pin remapping could be part of a separate PR. Since it's small and it only affect a few boards (only f1 based), it should simpler and faster to review.

I also think the pm_layered change in the i2c1 driver variant could be part of its own PR.

cpu/stm32_common/periph/i2c_2.c
@@ -16,9 +17,9 @@
* @file
* @brief Low-level I2C driver implementation
*
* This driver supports the STM32 F4 families.
* This driver supports the STM32 F1, L1, and F4 families.
Copy link
Contributor

Choose a reason for hiding this comment

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

and F2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup thanks, I will split everything out.

@aabadie
Copy link
Contributor

aabadie commented Dec 13, 2018

Also it's quite unclear to me why this PR would fix #3366 since this issue is not only related to stm32. Can we confirm that it's already fixed for other families (nrf, etc) ? If it's not the case, I suggest to remove the mention in the initial comment if this PR.

@maribu maribu mentioned this pull request Dec 13, 2018
Merged
@MrKevinWeiss
Copy link
Contributor Author

@aabadie True, It only helps the stm, I have not check all the other boards.

@MrKevinWeiss
Copy link
Contributor Author

Closing in favor of #10608 #10610 #10607
The pm layer was left out as it was only added to make things look the same and was not initially tested. This may be added at a later date.

@MrKevinWeiss MrKevinWeiss deleted the pr/stmi2c2 branch January 8, 2019 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aabadie aabadie aabadie requested changes

@smlng smlng Awaiting requested review from smlng

@dylad dylad Awaiting requested review from dylad

Assignees

@MrKevinWeiss MrKevinWeiss

Labels
Area: boards Area: Board ports 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 Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

stm32f103xx: I2C has no internal pullups and af config
3 participants
@MrKevinWeiss @aabadie @miri64