various i2c fixes #282
various i2c fixes #282
Conversation
clear the error flags after detecting them as the hardware does not autoclear on read. Signed-off-by: Sjoerd Simons <sjoerd@luon.net>
If the remote device fails to acknowledge send a stop. This allows the bus to go back to idle again. Signed-off-by: Sjoerd Simons <sjoerd@luon.net>
Enable the ack bit on multibyte (>2) reads otherwise bytes from the remote device are not acknowledged. Signed-off-by: Sjoerd Simons <sjoerd@luon.net>
sjoerdsimons
force-pushed
the
wip/sjoerd/i2c
branch
from
39456d2 to
765ac49
Compare
There was a problem hiding this comment.
Thanks for the PR, that sounds like it must have been an annoying bug to track down 😅
I added a thought about the random status register reads which I don't know the purpose of.
Would you mind adding an entry to the changelog about this? Other than that, it looks great!
| @@ -436,12 +436,17 @@ where | |||
| last_ret | |||
| } | |||
|
|
|||
| fn write_without_stop(&mut self, addr: u8, bytes: &[u8]) -> NbResult<(), Error> { | |||
| self.send_start_and_wait()?; | |||
| fn send_addr_and_wait(&mut self, addr: u8, read: bool) -> NbResult<(), Error> { | |||
| self.nb.i2c.sr1.read(); | |||
There was a problem hiding this comment.
Not directly related to the PR, but I'm now becomming unsure about what this is doing here. My best guess is that it tries to clear the status registers, but clearly that's not done since the first commit in the PR is needed...
There was a problem hiding this comment.
hmm; looking at the datasheet i'm not entirely sure if it's needed here either; Fwiw the relevant parts are:
Then the master waits for a read of the SR1 register followed by a write in the DR register
with the Slave address (see Figure 273 and Figure 274 Transfer sequencing EV5).
with EV5 being EV5: SB=1, cleared by reading SR1 register followed by writing DR register with Address.
But ofcourse the send_start_and_wait is already reading SR1 while polling for the flag; So it's probably not needed here.
Overall it seems some bits of sr1 are persistents (the error bits in particular) while others gets cleared on read
There was a problem hiding this comment.
Right. I think the best course of action here is to break this into its own issue and maybe fix it later since doing the wrong thing could break things subtly.
|
Thanks for this fix! I was beating my head against the wall trying to figure out why I couldn't use a library to read from a real time clock module. It was panicking when trying to get the date & time because it read 8 bytes through I2C. |
| $i2c.sr1.write(|w| w.berr().clear_bit()); | ||
| Err(Other(Error::Bus)) | ||
| } else if sr1.arlo().bit_is_set() { | ||
| $i2c.sr1.write(|w| w.arlo().clear_bit()); | ||
| Err(Other(Error::Arbitration)) | ||
| } else if sr1.af().bit_is_set() { | ||
| $i2c.sr1.write(|w| w.af().clear_bit()); | ||
| Err(Other(Error::Acknowledge)) | ||
| } else if sr1.ovr().bit_is_set() { | ||
| $i2c.sr1.write(|w| w.ovr().clear_bit()); |
There was a problem hiding this comment.
Hmm, is write correct here? These look like no-op to me.
There was a problem hiding this comment.
I could be wrong, but I believe these are there to reset the flags.
Edit: Reference manual says
Cleared by software writing 0, or by hardware when PE=0.
And I don't believe we have PE set
There was a problem hiding this comment.
Yes, but sr1.write(|w| w.any_field().clear_bit()); basically writes 0 to the whole register, regardless of the field name. That's why this looks like a mistake.
There was a problem hiding this comment.
Oh right, I guess that should be modify instead. Good catch, I'll go ahead and fix that
Fixes required to work with an AM2320 sensor; This sensor doesn't acknowledge the first i2c message sent to it, but it triggers it to wake up. This caused the i2c state to stay in an error state and the bus not to go idle. This is fixed in the first two patches
Later on a multi (>2) byte read is necessary to actually get the data; However the data didn't get properly acknowledged, fixed in the last patch.