-
Notifications
You must be signed in to change notification settings - Fork 206
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
Allow reuse of I2C code without macros #37
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very clever. I like that approach. Need to play with it in detail later.
NB: For me a breaking change would be okay if that resulted in a really clean and nice interface. If that works out okay I'd actually recommend looking into doing that to other peripherals as well.
I2C1 works nicely, haven't tested the other ones, yet. A bit of cleanup and reordering and maybe we will need to fix the i2c3 SNAFU in the SVD files and this should be great. |
Applied both changes (sans space deletion). Will work on getting my I2C device working today. |
Finally was able to get I2C up and running on my send. Both I2C1 and I2C2 can see the CS43L22 on the F4 disco board. I'll do a bit more testing with actually commanding it, but looks good so far. EDIT - Was able to pull ID from the chip using write_read on both I2C1 and 2. It looks like the derivedFrom XML attribute in the SVD is causing the deduplication. The I2C3 thing comes from ST and it doesn't look like svd2rust can modify this at the moment. Funny thing, it looks like the F4 series of SVDs are the only ones messed up enough to change what the base peripheral is between devices. F410 and F413 use I2C2 as a base, the rest use 3. All other series except F2 use I2C1. Not sure if is worth the effort to modify these as a simple |
This HAL impl is using the
That'd be fine with me for now. |
b332bfa
to
9f31167
Compare
@psycowithespn Looks great and compiles nicely. Haven't been able to test it yet due to lack of time. Is there more you'd like to add or is this ready to go in? |
@therealprof Just did a little bit more diff minimalization. I'd call this ready to go now. There is one lurking warning that I'm getting, but it appears erroneous:
|
Also, the I2C3 issue is functionally complete in stm32-rs/stm32-rs#98, to be merged with the svd2rust 0.14 upgrade. |
@psycowithespn Would you mind rebasing it to fix the conflict? |
f9e4d25
to
4d8ab08
Compare
@therealprof Rebased. I can't do anything about the warning. If no warnings is a goal, I can just inline the 5 usages of the type alias. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind the warnings. Looks fantastic to me, thanks a lot!
This is a mutation (actually not bad) of my first thought on how to get code reuse of the I2C peripherals without resorting to macros. The bare minimum pins and peripherals were added to get the point across. Currently untested, I have some sort of wiring issue or bad chip to deal with.
The core of this is the fact that I2C1, 2, and 3 all implement
Deref<Target = i2c3::RegisterBlock>
, I'm assuming due to some deduplication (also assuming it lives somewhere insvd2rust
). If this deduplication will continue to exist in the future, I don't see any negatives associated with it.Two traits were needed so the
embedded_hal
traits could directly use theDeref
constraint instead of requiring a newimpl
for each I2C peripheral due to the enable bits and pclk.Correct me if I'm wrong, but I believe this is not a breaking change.
Will resolve issues on my end and get this tested.