I2C contract violation

[WBosonic]

The sensor I am writing a driver for requires I2C communication of the form 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]

@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 https://github.com/rp-rs/rp-hal/pull/764. Do you remember why?

[WBosonic]

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.

[jannic]

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?

[ithinuel]

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