i2c: Add I2C Controller implementation #27
Conversation
astapleton
force-pushed
the
as/i2c-controller
branch
from
fc92300 to
2cd9a63
Compare
There was a problem hiding this comment.
Have you tested these on the H5 as well? I'd be a bit suspicious around some of the timing calcs, so at least one functional test would give some confidence.
Main comments in this are around the use of nb. I didn't closely look into the register calcs or device datasheet
| @@ -50,6 +50,7 @@ embedded-hal = "1.0.0" | |||
| defmt = { version = "0.3.8", optional = true } | |||
| paste = "1.0.15" | |||
| log = { version = "0.4.20", optional = true} | |||
| nb = "1.1.0" | |||
There was a problem hiding this comment.
I really would avoid using the nb crate nowadays. There's been talk of entirely deprecating it. It was intended to support embedded async before async was possible on embedded, but there's more modern approaches now with async frameworks targeting embedded.
There was a problem hiding this comment.
Ah, thanks for this context. I'll look at embedded-hal-async
There was a problem hiding this comment.
@ryan-summers I looked at embedded-hal-async, but I'm not sure how it solves the same problem? It doesn't provide any tool to manage repeatedly trying a blocking call. Is the suggestion to remove non-blocking calls entirely? Wouldn't they be useful in interrupts, or for creating a Future implementation for async?
Is it embedded-hal-nb that might be deprecated rather than the nb crate? That wasn't actually at 1.0 when I originally created this driver, so I didn't implement those APIs.
| //! i2c.start(0x18, AddressMode::AddressMode7bit, write.len(), Stop::Automatic) | ||
| //! | ||
| //! for byte in write { | ||
| //! block!(i2c.write_nb(byte))?; |
There was a problem hiding this comment.
I'd remove the nb implementation and instead go for embedded-hal-async personally, but up to you
| Event::Reload => w.tcie().enabled(), | ||
| Event::TransferComplete => w.tcie().enabled(), |
There was a problem hiding this comment.
Are these (same as below) supposed to be looking at the same bit?
There was a problem hiding this comment.
The Reload and TransferComplete both looking at TCIE? I'll double check
There was a problem hiding this comment.
Page 1216 of RM0492 Rev 2 indicates that both of these events generate a TCIE interrupt.
src/i2c.rs
Outdated
| Event::AddressMatch => w.addrie().enabled(), | ||
| }); | ||
| let _ = self.i2c.cr1().read(); | ||
| let _ = self.i2c.cr1().read(); // Delay 2 peripheral clocks |
There was a problem hiding this comment.
Would be helpful to document why there's a delay here
src/i2c.rs
Outdated
| fn flush_txdr(&mut self) { | ||
| // If a pending TXIS flag is set, write dummy data to TXDR | ||
| if self.i2c.isr().read().txis().bit_is_set() { | ||
| self.i2c.txdr().write(|w| w.txdata().bits(0)); |
There was a problem hiding this comment.
Are you certain this won't enqueue data to be written over I2C?
There was a problem hiding this comment.
It only writes to it after the transaction has finished (after a NACK), so no data will be written out. It's derived from stm32h7xx-hal, which added it here: stm32-rs/stm32h7xx-hal#43. I've also added more calls to the flush_txdr method due to some controller mode edge cases I found, where unhandled state would cause the driver to lock up.
| // If TXDR is not flagged as empty, write 1 to flush it | ||
| if self.i2c.isr().read().txe().is_not_empty() { | ||
| self.i2c.isr().write(|w| w.txe().set_bit()); | ||
| } |
There was a problem hiding this comment.
Do you need to poll something until the flush completes?
There was a problem hiding this comment.
I believe what this does is clear the TXDR register such that when the next transaction is started, the TXIS bit gets set properly, which it wouldn't if the TXDR register was never flushed (and the transaction hangs waiting for TXIS).
The suspicion is warranted, I'll give you that. I honestly don't understand them properly and the reference manual doesn't explain them very well and just refers you to use STM32Cube to determine the timing values. However, I have tested these timing values quite extensively on the H503. I'm using them for a I2C target implementation (which I'll put up for review after I merge this) in pre-production hardware and they have worked well so far. I'll take another look a them though. |
astapleton
force-pushed
the
as/i2c-controller
branch
from
2cd9a63 to
9ac41de
Compare
|
I've looked at the TIMINGR calculations again, and I still don't know where some of those numbers come from, but between the tests, the fact that they've been working pretty well for me, and seem to do a good job for the H7 and the fact that embassy uses a very similar derivation (probably copied from stm32h7xx-hal, based on timing and similarity), I think I'll stick with them as is. |
This adds the I2C Controller driver implementation for the STM32H5. It implements a blocking, and non-blocking API, as well as the embedded-hal 1.0 traits.
It borrows a lot from the STM32H7 HAL (particularly the TIMINGR configuration, which is a bit inscrutable in the reference manual), but it borrows the
Instanceconcept from the STM32F4 project to minimize the need for macros.It uses the updated terminology of Controller/Target from the latest version of the I2C spec (Rev. 7.0)