search

RIOT-OS / RobotFW-tests

Includes tests for RIOT based on the Robot Framework
GNU Lesser General Public License v2.1
4 stars 13 forks source link

tests: Periph I2C Flags #53

Open gschorcht opened 4 years ago

gschorcht commented 4 years ago

While analyzing why the following tests in tests/periph_i2c/tests/04__periph_i2c_flags fail for esp32-wroom-32

I was wondering why the second i2c_read_bytes call should fail?

In section 3.1.10, the I2C standard only specifies:

"A data transfer is always terminated by a STOP condition (P) generated by the master. However, if a master still wishes to communicate on the bus, it can generate a repeated START condition (Sr) and address another slave without first generating a STOP condition. Various combinations of read/write formats are then possible within such a transfer."

From my point of view, "address another slave ..." doesn't mean necessarily that a second read with the same slave address should fail. It just says, that the master can continue the communication on the bus with another slave. In fact, the slave address in combined format is the same and only the direction is changed. Furthermore, "Various combinations ..." does not imply that the combined read after write transfer must be the only one.

Furthermore, according to Note 4 in section 3.1.10

"I2C-bus compatible devices must reset their bus logic on receipt of a START or repeated START condition such that they all anticipate the sending of a slave address"

That is, all slaves including the former one should await an address field after a repeated start. So why shouldn't it be the same address as before?

Although it might not make sense to have consecutive read transfers from the same slave, the standard does not forbid this.

I have tested it with a chip PCF8575 chip from NXP, the owner of the I2C standard. The chip accepts consecutive i2c_read_bytes calls with repeated START conditions. So IMHO, the ESP32 I2C behaves correctly.

MrKevinWeiss commented 4 years ago

That is, all slaves including the former one should await an address field after a repeated start. So why shouldn't it be the same address as before?

It is not about the address change it is the readable data in the i2c changing

From the slave perspective, it should still be clocking out bytes if a read occurred and there was no stop. If the master is trying to send a repeated start address and the slave cannot handle a repeated start then the SDA line could be held down by the slave causing a lockup problem.

This is the behavior of PHiLIP and I believe I tested it on a sensor (I was not able to fail it the first time as the data did not hold down the line but when the sensor data did hold it down, the device was locked up).

Although it might not make sense to have consecutive read transfers from the same slave, the standard does not forbid this.

I think since some slave devices may lock up we should forbid it... Some devices may work but if the hardware cannot support (or expect) a repeated start when reading then the bus could lock up.

On the other hand, it has been a while since I evaluated that and I cannot tell you the name of the sensor that I tested this on so maybe I will take a closer look.

The real test should be named "Repeated start lockup condition should recover actually"

Can we leave this issue open until I have time to dive a bit deeper? Or others can give their on should we implement to the standard or to devices...

I really do need to confirm if this is just implementation on my side though.

gschorcht commented 4 years ago

That is, all slaves including the former one should await an address field after a repeated start. So why shouldn't it be the same address as before?

It is not about the address change it is the readable data in the i2c changing

From the slave perspective, it should still be clocking out bytes if a read occurred and there was no stop. If the master is trying to send a repeated start address and the slave cannot handle a repeated start then the SDA line could be held down by the slave causing a lockup problem.

Hm, this situation should not happen according to note 4 in section 3.1.10 of the I2C standard.

"I2C-bus compatible devices MUST reset their bus logic on receipt of a START or repeated START condition such that they all anticipate the sending of a slave address"

Only misbehaving devices might continue to clock out bytes and might hold the SDA line LOW when the master tries to send the address field. So you wanna test that a misbehaving slave wouldn't lock up the master, right?

If I'm right, the test should be checked. The following screenshots are the two phases of the Repeated Start Read Bytes 0 test. I2C_Repeated_Start_Read_Bytes_0_1 I2C_Repeated_Start_Read_Bytes_0_2

PHiLIP isn't holding SDA line LOW during the address field after repeated START. Even more, it is reacting with an ACK on the address field before sending the 0x00 bytes. So everything seems fine from the master. That's why ESP32 reacts with Success even though the test result should be failed.

MrKevinWeiss commented 4 years ago

Ok, let's see what the HiL says.