I2C transaction breaks I2c trait contract

[KaSroka]

According to I2c trait transaction contract is as follows:

Transaction contract:
- Before executing the first operation an ST is sent automatically. This is followed by SAD+R/W as appropriate.
- 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.
- After executing the last operation an SP is sent automatically.
- If the last operation is a `Read` the master does not send an acknowledge for the last byte.
- `ST` = start condition
- `SAD+R/W` = slave address followed by bit 1 to indicate reading or 0 to indicate writing
- `SR` = repeated start condition
- `SP` = stop condition

Using following simple test code:

use embedded_hal::i2c::blocking::{I2c, Operation as I2cOperation};
use linux_embedded_hal::I2cdev;

fn main() {
    let mut i2c_dev = I2cdev::new("/dev/i2c-1").unwrap();

    let data = [0xaa; 1];
    let mut ops = [I2cOperation::Write(&[0x40]), I2cOperation::Write(&data)];
    i2c_dev.transaction(0x3d, &mut ops).unwrap();
}

I'm expecting no SR "repeated start condition" to appear as both operations are of the same type (Write). Unfortunately I can see SR "repeated start condition" on the bus as well as SAD+R/W "slave address followed by bit 1 to indicate reading or 0 to indicate writing" (positions 6 and 8). This was captured using logic analyzer on embedded Linux board for the sample code attached above:

[eldruin]

Interesting find! Following the code path you can see that two linux write messages are created and sent to the underlying rust-i2cdev library. There it is received and the message addresses filled and then sent to the kernel via FFI. The kernel documentation states:

The function will write or read data to or from that buffers depending on whether the I2C_M_RD flag is set in a particular message or not. The slave address and whether to use ten bit address mode has to be set in each message, overriding the values set with the above ioctl’s.

So the code path seems to be correct and this is just how the linux kernel works: it sends the messages without a stop in between, although it does send a restart with the same address for each message regardless of the type of the last one.

In order to uphold the transaction contract what we could do is to consolidate operations of the same type that follow each other into a single message for the kernel here. This needs to be done for transaction_iter as well, but the _iter part of it is internally already collected and not immediately sent so it should be doable.

This affects the 0.3.x version as well.

[SZenglein]

So according to https://docs.kernel.org/i2c/i2c-protocol.html, there exists a flag I2C_M_NOSTART that can be set for each message.

The description states:

This is often used to gather transmits from multiple data buffers in system memory into something that appears as a single transfer to the I2C device but may also be used between direction changes by some rare devices.

I think this might do exactly what the I2c trait contract requires.

[ohunter]

The I2C_M_NOSTART flag does fix the issue, but it assumes that the i2c device supports it. This can be checked using the I2C_FUNCS operation of ioctl. One potential issue with the fix is that it needs to be implemented in the rust-i2cdev repo as the i2c_mgs.flags field is private. If we can assume that the LinuxI2CDevice::transfer call should always fulfill the contract then this isn't an issue at all.

[eldruin]

I am not opposed to make rust-i2cdev fulfill the contract of embedded-hal::i2c::I2c. However, it is possible to set the NO_START message flag entirely from this crate by using with_flags(). i.e. LinuxI2CMessage::write(w).with_flags(I2CMessageFlags::NO_START). One would need to play a bit with the chain here, though, in order to set it only when appropriate.