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

Jump to bottom

I2C contract violation #825

Open
WBosonic opened this issue Aug 12, 2024 · 4 comments · May be fixed by #827
Open

I2C contract violation #825

WBosonic opened this issue Aug 12, 2024 · 4 comments · May be fixed by #827

Comments

@WBosonic
Copy link

The sensor I am writing a driver for requires I2C communication of the form
image
where INDEX is some internal 16 bit register and DATA can be any number of bytes.

The I2C transaction contract states:

  • Data from adjacent operations of the same type are sent after each other without an SP or SR.
  • Between adjacent operations of a different type an SR and SAD+R/W is sent.

Therefore I would expect this code to work

fn write(&mut self, mut reg: u16, data: &[u8]) -> Result<(), Error<I2C::Error>> {
    self.i2c.transaction(self.address, &mut [Operation::Write(&reg.to_be_bytes()), Operation::Write(data)])?;
}

Instead it appears an SP is sent between the two Write operations forcing unnecessary copies:

fn write(&mut self, mut reg: u16, data: &[u8]) -> Result<(), Error<I2C::Error>> {
    // self.i2c.transaction(self.address, &mut [Operation::Write(&reg.to_be_bytes()), Operation::Write(data)])?;

    let buf = &mut [0; 16];
    let data_len = buf.len() - 2;

    for chunk in data.chunks(data_len) {
        let chunk_buf = &mut buf[..chunk.len() + 2];

        let reg_bytes = reg.to_be_bytes();
        chunk_buf[0] = reg_bytes[0];
        chunk_buf[1] = reg_bytes[1];
        chunk_buf[2..].copy_from_slice(chunk);

        self.i2c.write(self.address, chunk_buf)?;

        reg += chunk.len() as u16;
    }

    Ok(())
}

Given that I need to send around 80kB of data this seems wasteful.
@jannic
Copy link
Member

jannic commented Aug 12, 2024

@WBosonic, thanks for the report. I agree this looks wrong. However, are you sure it's an SP (stop) you see between the write operations? I'd expect a SR (repeated start), triggered by https://github.com/rp-rs/rp-hal/blob/main/rp2040-hal/src/i2c/controller.rs#L294:

                if first_byte {
                    if !first_transaction {
                        w.restart().enable();
                    }
                    first_byte = false;
                }

@ithinuel, if I read the git history correctly, that was added in #764. Do you remember why?

@WBosonic
Copy link
Author

Ah yes it is an SR (I encountered this bug a while ago and misremembered).

From what I see the user facing I2C transaction does not actually check if the transaction type is the same as the one before.

@WBosonic WBosonic mentioned this issue Aug 13, 2024
Closed
@jannic
Copy link
Member

jannic commented Aug 15, 2024

Ah yes it is an SR (I encountered this bug a while ago and misremembered).

From what I see the user facing I2C transaction does not actually check if the transaction type is the same as the one before.

If I understand the datasheet correctly, it doesn't need to:

1 - If IC_RESTART_EN is 1, a RESTART is issued before the
data is sent/received (according to the value of CMD),
regardless of whether or not the transfer direction is
changing from the previous command; if IC_RESTART_EN
is 0, a STOP followed by a START is issued instead.
0 - If IC_RESTART_EN is 1, a RESTART is issued only if the
transfer direction is changing from the previous
command; if IC_RESTART_EN is 0, a STOP followed by a
START is issued instead.

So the fix could be as easy as removing w.restart().enable(); entirely?

@jannic jannic linked a pull request Aug 15, 2024 that will close this issue
Open
@ithinuel
Copy link
Member

Yes, provided that ic_restart_en is set (which it is).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove code setting IC_DATA_CMD.RESTART bit jannic/rp-hal
3 participants
@jannic @ithinuel @WBosonic