i2c: Add target implementation #45
Conversation
src/i2c.rs
Outdated
| /// This encapsulates a transaction event that occurs when listening in Target | ||
| /// operation. A TargetWrite event indicates that the Controller wants to read | ||
| /// data from the Target (I2C read operation). A TargetRead event indicates | ||
| /// that the Controller wants to write data to the Target (I2C write | ||
| /// operation). A Stop event indicates that a Stop condition was received and | ||
| /// the transaction has been completed. | ||
| #[derive(Copy, Clone, Debug, PartialEq, Eq)] | ||
| pub enum TargetEvent { |
There was a problem hiding this comment.
These definitions are pretty confusing. Wouldn't a TargetRead indicates that the controller wants to read data from the target?
There was a problem hiding this comment.
Ha. I actually had these the other way round, simply called TargetEvent::Read (controller wants to read, target must execute a write) and TargetEvent::Write (controller wants to write, target must execute a read), but I thought that was confusing, so I tried to disambiguate it by indicating that a TargetRead event is in response to controller write. Guess they're both a bit confusing, so I'll go back to the old iteration.
| length <= u8::MAX as usize, | ||
| "I2C max transfer size per reload = {}", | ||
| u8::MAX | ||
| ); |
There was a problem hiding this comment.
I added these bounds checks in the controller start logic because I realized that the controller implementation does not use NBYTES reloads to write buffers longer than 255 bytes, but also doesn't bounds check on repeat starts or reloads, leading to wrap around bugs on the value written to the NBYTES field and transactions that were unexpectedly short.
Created this issue to track: #46
|
@ryan-summers mind taking another look at this? I believe I've addressed everything |
src/i2c.rs
Outdated
| self.inner.disable_target_event(event) | ||
| } | ||
|
|
||
| fn configure_target(&mut self, config: impl Into<TargetConfig>) { |
There was a problem hiding this comment.
ah, in a previous revision I didn't include the bus frequency in the target config, so I had an implementation of u8 for Into<TargetConfig> so you could simply provide the address. Unfortunately, one needs to provide the frequency to set up timing parameters correctly, and providing a tuple was less ergonomic, so I just removed that implementation. And I will change to just use a TargetConfig here.
| if let Some(secondary_address) = config.secondary_address { | ||
| i2c.oar2().write(|w| { | ||
| w.oa2().set(secondary_address); | ||
| if let Some(mask) = config.secondary_address_mask_bits { |
There was a problem hiding this comment.
What happens if the user specifies an address but no mask?
(I have very limited experiance with i2c so keep that in mind :) )
There was a problem hiding this comment.
The mask allows for the target to respond to a wider range of addresses (ie. an address of 0x20 and mask of 0x1F will let the target watch for transmissions on any address from 0x20 to 0x3F). This is an STM32 specific feature, not a standard feature of I2C, by the way. If no mask is specified, the only secondary address that the target will respond to is the one specified (0x20 in the example)
| ControllerExpectedWrite, | ||
| /// Target operation only: | ||
| /// While waiting for a controller write event, a read event was received. | ||
| ControllerExpectedRead, |
There was a problem hiding this comment.
Just a (not well thought out) thought, would it make sense to split the errors into TargetError and ControllerError or similar? This so the user won't have to worry about the non relevant cases?
There was a problem hiding this comment.
Is TargetError a super set of ControllerError could ControllerError impl Into<TargetError>?
There was a problem hiding this comment.
My latest commit adds a TargetError enum and a TargetError variant for Error. Does that look better to you?
The example is currently only tested on NUCLEO-STM32H503, and uses pins that are not available on other H5 processors. This can be enabled on other processors by choosing different pins for the various evaluation boards.
- Deduplicate functions shared between AutoAck and ManualAck implementations for I2cTarget - Rename TargetEvent variants - Fix writing buffer larger than 255 bytes in ManualAck operation - Improved documentation
Co-authored-by: Albin Hedman <[email protected]>
This extends the I2C driver to support target operation. It adds an I2cTarget type to represent the I2C operation as the target.
Target operation can be operated in automatic ACKing mode, letting the hardware manage clock stretching and ACKing automatically, or it can be operated in manual ACK control mode, which lets the user of the driver determine when to ACK the end of a transaction. Manual ACK control allows for clock stretching while data is being processed or retrieved before ACK'ing the final byte of a transfer and initiating either the end of a transfer or a repeat start to read the processed/retrieved data. This implementation does not currently allow for ACKing individual or a subset of bytes manually, but extending the interface to do so can build upon what is implemented so far.
This also adds role switching to the driver, enabling both controller and target implementation through choosing the appropriate initialization functions.