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/stm_common: Refactor and cleanup i2c_1 #10610

Merged
merged 1 commit into from
Dec 21, 2018
Merged

cpu/stm_common: Refactor and cleanup i2c_1 #10610

merged 1 commit into from
Dec 21, 2018

Conversation

MrKevinWeiss
Copy link
Contributor

@MrKevinWeiss MrKevinWeiss commented Dec 13, 2018 •
edited
Loading

Contribution description

This PR fixes many bugs discovered by the HiL.

  • Refactor to use common read_regs.
  • Add error reporting and handling for unsupported low level commands.
  • Document hardware constraints.
  • Reduces code size around 20 to 50 bytes

Testing procedure

Test on stm32f0, f3, f7, 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.

  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

checks some boxes in #9518
helps with some stm32 based boards for #3366
depends on #10608

@MrKevinWeiss MrKevinWeiss added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Platform: ARM Platform: This PR/issue effects ARM-based platforms Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: cpu Area: CPU/MCU ports labels Dec 13, 2018
@MrKevinWeiss MrKevinWeiss self-assigned this Dec 13, 2018
@MrKevinWeiss MrKevinWeiss mentioned this pull request Dec 13, 2018
Closed
@MrKevinWeiss MrKevinWeiss added the State: waiting for other PR State: The PR requires another PR to be merged first label Dec 13, 2018
@MrKevinWeiss MrKevinWeiss mentioned this pull request Dec 19, 2018
Merged
@aabadie
Copy link
Contributor

aabadie commented Dec 20, 2018

This needs a rebase @MrKevinWeiss

@MrKevinWeiss
Copy link
Contributor Author

On it!

@MrKevinWeiss MrKevinWeiss removed the State: waiting for other PR State: The PR requires another PR to be merged first label Dec 20, 2018
@MrKevinWeiss MrKevinWeiss force-pushed the pr/fix/stmi2c1 branch 2 times, most recently from aaf64ec to d201a97 Compare December 20, 2018 13:52
aabadie
aabadie requested changes Dec 20, 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.

I tested this PR on L0, L4, F0, F3 and F7. It works like a charm with the sensor I have (hts221). I didn't push the testing further.

I have minor comments inline.

My general impression is that it is a very good job and I'd like to have it in the release :)

cpu/stm32_common/periph/i2c_1.c Outdated
}

/**
* Cannot support continuous writes or fram splitting at this level. If an
Copy link
Contributor

Choose a reason for hiding this comment

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

s/fram/frame

cpu/stm32_common/periph/i2c_1.c Outdated
size_t length, uint8_t flags, uint32_t cr2_flags)
{
assert(i2c != NULL);
assert(length < 256);
Copy link
Contributor

Choose a reason for hiding this comment

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

why ? Can we have a macro defined for this ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment for line https://github.com/RIOT-OS/RIOT/pull/10610/files#diff-0cd75c62c13ab35de384461ef1979484R186

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why the length limitation or why not combine both in a macro?
If length then it is because I would have to do a check to see if it is greater, if it is set the reload bit and keep track of everything sent. It adds extra steps and I don't know how valuable it would be (even for something like EEPROM, when you are talking about 256 bytes adding a start and stop frame isn't that much overhead).

cpu/stm32_common/periph/i2c_1.c Outdated
if ((flags & I2C_REG16) || (flags & I2C_ADDR10)) {
/* If reload was set, cannot send a repeated start */
if ((i2c->ISR & I2C_ISR_TCR) &&
!(flags & I2C_NOSTART)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be on the same line above

cpu/stm32_common/periph/i2c_1.c Outdated
}
/* If reload was NOT set, must either stop or start */
if ((i2c->ISR & I2C_ISR_TC) &&
(flags & I2C_NOSTART)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be on the same line above

cpu/stm32_common/periph/i2c_1.c Outdated
int ret = _start(i2c,
(address << 1) |
(length << I2C_CR2_NBYTES_Pos) |
/* I2C_FLAG_WRITE | */
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe remove ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I would prefer to have all parameters on the same line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK will do

@aabadie aabadie mentioned this pull request Dec 20, 2018
Merged
@MrKevinWeiss
Copy link
Contributor Author

MrKevinWeiss commented Dec 20, 2018 •
edited
Loading

@aabadie I think I did what you want.

@aabadie aabadie mentioned this pull request Dec 20, 2018
Merged
aabadie
aabadie reviewed Dec 20, 2018
View reviewed changes
cpu/stm32_common/periph/i2c_1.c Outdated
{
assert(dev < I2C_NUMOF);
assert(dev < I2C_NUMOF && length < 256);
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking of having a define for 256. To avoid hard coded values in the code and to have it defined once.

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, makes sense, on it.

aabadie
aabadie reviewed Dec 20, 2018
View reviewed changes
cpu/stm32_common/include/periph_cpu_common.h Outdated
#define PERIPH_I2C_NEED_WRITE_REGS
#endif

Copy link
Contributor

@aabadie aabadie Dec 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This blank line could be removed

@MrKevinWeiss
cpu/stm_common: Refactor and cleanup i2c_1
f3b2a62 
Refactor to use common read_regs.
Add error reporting and handling for unsupported low level commands.
Document hardware constraints.
@MrKevinWeiss
Copy link
Contributor Author

@aabadie All done!

@aabadie aabadie added this to the Release 2019.01 milestone Dec 21, 2018
aabadie
aabadie reviewed Dec 21, 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.

Thanks again for the cleanup and fixes in the I2C driver. It looks and works much better now.

I'm just wondering if we shouldn't also handle the case where more than 256 bytes are transferred on the bus. I was comparing the implementation with the reference manual, from page 713 to 718, and it seems it could be done. Maybe later though. What do you think ?

@MrKevinWeiss
Copy link
Contributor Author

It is possible but I am not sure it is worth the extra code size at the moment. If anyone requests it in the future I can add it.

aabadie
aabadie approved these changes Dec 21, 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.

I think we are good then.

ACK and go!

@aabadie aabadie merged commit 8449324 into RIOT-OS:master Dec 21, 2018
@MrKevinWeiss MrKevinWeiss deleted the pr/fix/stmi2c1 branch January 8, 2019 12:51
@gschorcht gschorcht mentioned this pull request Jan 12, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aabadie aabadie aabadie approved these changes

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@maribu maribu Awaiting requested review from maribu

@dylad dylad Awaiting requested review from dylad

Assignees

@MrKevinWeiss MrKevinWeiss

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 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
Release 2019.01
Development

Successfully merging this pull request may close these issues.
2 participants
@MrKevinWeiss @aabadie