periph_i2c: Adapt i2c test to support read register

[MrKevinWeiss]

As mentioned in #53 the logic should reset when a start bit is sent. This means that proper repeated read registers should be supported.

This changes the test to expect the repeated read success instead of saying not supported.

Testing Procedure

Check the CI, investigate if any lockup occurs and if it does occur what is causing it.

[gschorcht]

The test seems to be correct now :smile:

However, the other test case where the master must not lock if a slave does not react correctly on a repeated start and clocks out a 0x00 byte that overwrites the address field might be still interesting.

Would it be possible to realize such a behavior with PHiLIP? If I understand it right, PHiLIP realizes the I2C slave using the hardware implementation. Theoretically, it should be possible to use a combination of I2C slave implementation and GPIO settings. This is, the first read operation without the STOP condition would be realized with I2C hardware implementation and after that a bit-banging implementation would be used with same GPIOs which waits for the repeated START and then pulls the SDA line to LOW during the address field :thinking:

[MrKevinWeiss]

I do want to try to reproduce this on a sensor. I remember seeing this I just need to verify what was going on.

Yes it could be possible. One limitation that I have is the speed of reaction, the more that I try to do in the i2c interrupt the longer it takes until the point where I cannot respond to the register in time (this may already occur if the clock is very high).

Soon I will work on prepping a new release of PHiLIP and this will be at the top of the list.

[gschorcht]

The Repeated Start Read Bytes tests work now for arduino-mega2560, esp32-wroom-32 and frdm-k22f but fail for all nucleo-* boards.

For the nucleo-* board, the tests have to fail now since repeated START reading is explicitly prevented in i2c_1.c and i2c_2.c in cpu/stm32_common/periph, for example in i2c_2.c:

    /* Do not support repeated start reading
     * The repeated start read requires the bus to be busy (I2C_SR2_BUSY == 1)
     * the previous R/W state to be a read (I2C_SR2_TRA == 0)
     * and for the command not to be split frame (I2C_NOSTART == 0)
    */
    if (((i2c->SR2 & (I2C_SR2_BUSY | I2C_SR2_TRA)) == I2C_SR2_BUSY) &&
        !(flags & I2C_NOSTART)) {
        return -EOPNOTSUPP;
    }

Having a look into the history, this check was introduced by you with https://github.com/RIOT-OS/RIOT/pull/10610 and https://github.com/RIOT-OS/RIOT/pull/12007, respectively.

Since this behavior is not documented in the API, it is not implemented in that way on other platforms. We have to decide what the behavior should be:

1. We don't allow repeated start reading. In this case, it should be documented in the API and the platform implementations should be checked.
2. We allow repeated start reading as the standard allows from my point of view. Then we have to change at least the STM32 implementation.

[MrKevinWeiss]

I took a closer look at the i2c while trying to lockup PHiLIP and I noticed that a read_bytes with I2C_NOSTOP flag a NACK is given for the data when it should be ACKed because we are assuming more data is coming for the ESP32.

Is there an easy way (or patch) to test what happens if it is ACKed?

I tried in cpu/esp32/periph/i2c_hw.c to add _i2c_hw[dev].regs->command[_i2c_bus[dev].cmd].ack_en = 1; when a nostop is used but I don't think the hw i2c is being used...

[MrKevinWeiss]

We don't allow repeated start reading. In this case, it should be documented in the API and the platform implementations should be checked. We allow repeated start reading as the standard allows from my point of view. Then we have to change at least the STM32 implementation.

I agree, there needs to be some clarification and improvements. I will be investigating over the next little while to make sure the API and functionality match.