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

various i2c fixes #282

Merged
merged 5 commits into from
Nov 6, 2020
Merged

various i2c fixes #282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c80c7da
i2c: Clear error flags
sjoerdsimons Nov 1, 2020
a1d8b24
i2c: Send stop after acknowledge error
sjoerdsimons Nov 1, 2020
765ac49
i2c: Set ack bit on multibyte reads
sjoerdsimons Nov 1, 2020
ea24f01
Update changelog
sjoerdsimons Nov 5, 2020
db95d14
Merge branch 'master' into wip/sjoerd/i2c
TheZoq2 Nov 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

- LSB/MSB bit format selection for `SPI`

### Fixed
- Fix > 2 byte i2c reads
- Send stop after acknowledge errors on i2c
- Fix i2c interactions after errors

## [v0.7.0]- 2020-10-17

### Breaking changes
Expand Down
33 changes: 26 additions & 7 deletions src/i2c.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,12 +220,16 @@ macro_rules! wait_for_flag {
let sr1 = $i2c.sr1.read();

if sr1.berr().bit_is_set() {
$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());
Comment on lines +223 to +232
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, is write correct here? These look like no-op to me.

Copy link
Member

@TheZoq2 TheZoq2 Dec 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, I guess that should be modify instead. Good catch, I'll go ahead and fix that

Err(Other(Error::Overrun))
} else if sr1.$flag().bit_is_set() {
Ok(())
Expand Down Expand Up @@ -432,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();
Copy link
Member

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

self.nb.send_addr(addr, false);
self.nb.send_addr(addr, read);
let ret = busy_wait_cycles!(wait_for_flag!(self.nb.i2c, addr), self.addr_timeout);
if ret == Err(Other(Error::Acknowledge)) {
self.nb.send_stop();
}
ret
}

busy_wait_cycles!(wait_for_flag!(self.nb.i2c, addr), self.addr_timeout)?;
fn write_bytes_and_wait(&mut self, bytes: &[u8]) -> NbResult<(), Error> {
self.nb.i2c.sr1.read();
self.nb.i2c.sr2.read();

Expand All @@ -451,6 +460,17 @@ where

Ok(())
}

fn write_without_stop(&mut self, addr: u8, bytes: &[u8]) -> NbResult<(), Error> {
self.send_start_and_wait()?;
self.send_addr_and_wait(addr, false)?;

let ret = self.write_bytes_and_wait(bytes);
if ret == Err(Other(Error::Acknowledge)) {
self.nb.send_stop();
}
ret
}
}

impl<I2C, PINS> Write for BlockingI2c<I2C, PINS>
Expand All @@ -476,9 +496,7 @@ where

fn read(&mut self, addr: u8, buffer: &mut [u8]) -> Result<(), Self::Error> {
self.send_start_and_wait()?;
self.nb.i2c.sr1.read();
self.nb.send_addr(addr, true);
busy_wait_cycles!(wait_for_flag!(self.nb.i2c, addr), self.addr_timeout)?;
self.send_addr_and_wait(addr, true)?;

match buffer.len() {
1 => {
Expand Down Expand Up @@ -515,6 +533,7 @@ where
self.nb.i2c.cr1.modify(|_, w| w.ack().set_bit());
}
buffer_len => {
self.nb.i2c.cr1.modify(|_, w| w.ack().set_bit());
self.nb.i2c.sr1.read();
self.nb.i2c.sr2.read();

Expand Down