arduino / ArduinoCore-avr

The Official Arduino AVR core
https://www.arduino.cc
1.22k stars 1.04k forks source link

Wire.requestFrom() with zero bytes reads many bytes. #421

Open Koepel opened 3 years ago

Koepel commented 3 years ago

If zero number of bytes are requested from any existing device on the I2C bus with Wire.requestFrom( i2c_address, 0); then the acknowledge from that device makes the Wire library read 37 bytes. The return value of Wire.requestFrom( i2c_address, 0); is zero.

Request zero bytes:

Wire.requestFrom( 0x77, 0);  // 0x77 does exist

Result: afbeelding

As far as I can tell, the data is real data from the sensor. There is also a NACK after the last data byte just before the STOP condition. Everything is according to a standard I2C session, except that 37 bytes don't fit in a 32 byte buffer and there should be no reading of data at all.

If the device does not exist, then the NACK will stop the I2C session. afbeelding

Background

The "I2C Scanner" sketch uses a 'write' command with zero bytes to check the acknowledge. There are however some capacitive touch sensors that don't have the hardware for the 'write' command. They do acknowledge to the 'read' command. Therefor I was testing a "I2C Read Scanner" and was comparing the behavior of the AVR Wire library against the SoftwareWire library with requestFrom( i2c_address, 0); in the Wokwi simulator with a simulated logic analyzer. The Arduino Uno with DS1307 (in Wokwi) issued this bug. I confirmed this with real hardware with a Arduino Mega 2560 and a BMP085 and also a Arduino Uno as a Slave. The screendumps above are with real hardware and a LHT00SU1 logic analyzer with PulseView/sigrok.

Ragebone commented 2 years ago

@Koepel Can i please have a Trace-File to take a look? I'm not yet able to try reproducing the issue.

Koepel commented 2 years ago

This is a file from PulseView. Open the *.zip in PulseView. requestzero.zip

Same test as before: Arduino IDE 1.8.16, Arduino Mega 2560, BMP085 after a I2C level converter, PulseView 0.4.2, LHT00SU1 Logic Analyzer. Test sketch:

#include <Wire.h>

void setup() 
{
  Wire.begin();
  delay( 5000);
  Wire.requestFrom( 0x77, 0);
  while(true);
}

void loop() {}

May I counter your question ? Do you have a capture with zero bytes requested that stops after the I2C address ? Please show it and tell everything (hardware and software). Perhaps I can try to reproduce that.

Ragebone commented 2 years ago

I'm not yet able to try reproducing the issue.

Sorry, have phrased that badly. I am not able to do tests at the moment. Need to fix a few pro micros first and get them setup properly.

Thank you for the traces, i will take a look at them.

Ragebone commented 2 years ago

@Koepel Good news, i have observed similar behavior. Differences between the Trace you posted and my situation is that your Trace only contains 0 Bytes, looking like the Bus went dead for while. In my Situation, the first requested Byte is 0 and all following ones are 0xFF. I currently assume that might be due to the slave you used when tracing. I am using two pro micro.

Koepel commented 2 years ago

I noticed that as well. I should have captured the first I2C data after power up.

Ragebone commented 2 years ago

I don't see how that would have made a difference. Edit: All my traces so far have been more or less consistent.

I have to ask, does the i2c standard even allow for 0 bytes to be read in a transaction? Edit: Didn't have that thought before.

The way i understand how addressing works, there must at least be one byte transmitted. Making the TWI lib not follow spec by NACKing in line 556 fixes the observed behavior. Still in that case, one byte is read from the slave. Edit: and no more, never.

How is your "i2c read scanner" coming along? I am a bit annoyed from thinking about the related lack of "feedback" by "requestFrom()".

Koepel commented 2 years ago

The Slave should ACK to its I2C address and the Master should send a NACK after reading the last data byte. Then the Master can send a STOP.

That means that sending a STOP after the ACK of the Slave is not according to the I2C protocol. At least one data byte should be read. In the UM10204 document from NXP are hints that a STOP should be recognized by I2C devices to release the I2C bus. That document says that only a STOP immediately after a START is illegal.

In my opinion, it is not 100% clear if this situation is allowed.

When using a software I2C library, anything can be done, and it will work. I don't know if there is a I2C sensor that enters a erroneous state.

Ragebone commented 2 years ago

I see what you mean and i'm not sure how to go about this. The simple but also lazy and wonky fix is to just check for 0 requested bytes and set that to 1 or exit.

I have looked into it a bit and have no clue where the code goes wrong and starts reading 37 bytes. I'm also not sure how to easily debug that. My last session was me banging my head for hours and digitalWrite(0) -ing the Bus dead. Any suggestions and ideas?

xl-or commented 2 weeks ago

The error is in the twi_readFrom function. The test was carried out on the Arduino UNO board with Atmega328P.

https://github.com/arduino/ArduinoCore-avr/blob/321fca0bac806bdd36af8afbc13587f4b67eb5f1/libraries/Wire/src/utility/twi.c#L159

When passing 0 as the length of the received data, the unsigned twi_masterBufferLength variable, which is responsible for limiting the data written to the twi_masterBuffer buffer, turns into 255.

https://github.com/arduino/ArduinoCore-avr/blob/321fca0bac806bdd36af8afbc13587f4b67eb5f1/libraries/Wire/src/utility/twi.c#L183

In this case, we will go beyond the boundaries of twi_masterBuffer (the size of which by default is TWI_BUFFER_LENGTH == 32) and overwrite the data lying after it.

Let's look at the map file.

image

twi_masterBuffer is located at addresses 0x0080015c-0x008017b (32 bytes) Then the variables twi_inRepStart, twi_slarw are overwritten (+2 bytes) Then the variable twi_masterBufferLength is overwritten to 0xFF (+1 byte) The twi_masterBufferIndex is overwritten to 0xFF (+1 byte) After this, twi_masterBufferLength and twi_masterBufferIndex become equal and a request for the last byte is sent (with NACK) (+1 byte)

https://github.com/arduino/ArduinoCore-avr/blob/321fca0bac806bdd36af8afbc13587f4b67eb5f1/libraries/Wire/src/utility/twi.c#L555-L559

Total read 32 + 5 = 37 bytes (magic)

This is confirmed by the logic analyzer.

image

If we change the value of TWI_BUFFER_LENGTH to 8, we will get a similar situation 8 bytes of data are written to the twi_masterBuffer buffer and then 4 bytes are overwritten next, until twi_masterBufferLength and twi_masterBufferIndex again become equal + 1 last NACK byte. Total 8 + 5 = 13 bytes of data

image

image

I think it should be possible to "get" 0 bytes of data, i.e. a read request and then a stop signal. In any case, a zero length value causes an out-of-bounds buffer access, which is bad. It seems a magic that this error terminates itself and does not cause the program to crash.