calling write_read with an empty write buffer repeatedly retries I2C reads

[TheButlah]

With this code on an ESP32-C3:

i2c
  .write_read(ADDR_DOESNT_MATTER, &[], &mut [0; RECV_LEN_DOESNT_MATTER])
  .expect("Failed i2c");

I get this signal trace: repeated.zip

The code panics due to the NACK and then suspends execution in a loop inside my panic handler - I have triple checked this with both probe-rs and defmt logs. However, even though execution is suspended, the hardware I2C continues to send bogus I2C transmissions. So my only conclusion is that the registers that control the hardware I2C have been left in a state that repeatedly sends incorrect data.

Probably, it is due to this code in esp-hal:

The write command is issued, but the data to write is never actually set because bytes is empty.

[bjoernQ]

Thanks for opening this issue!

I did some tests on my side, too.

This is with a no bytes write and something on the bus that ACKs:

This is with a one bytes write and something that ACKs:

This is with a no bytes write and nothing that ACKs: (i.e. I think that is your case)

This is with a one byte write and nothing that ACKs:

So, what is repeated seems to be the attempt to read.

For the last case already the write operation fails and the transfer is stopped.

[bjoernQ]

We can get rid of those read attempts by adding self.reset(); to execute_transmission before returning return Err(Error::AckCheckFailed); (maybe we should do that in all error cases)

[bjoernQ]

Just out of curiosity: is this related to your problem using MPU6050? I ordered a MPU6050 break-out board but which should hopefully arrive by the end of the week

[TheButlah]

Yes, it was. I added this PR in which originally I was writing an empty, zero length buffer as an attempt to reset any internal state in the I2C peripheral (because IDK what I'm doing lmao)

I now write a single 0x00 instead, and that results in successful communication with the MPU6050 after one or two retries

[jessebraham]

Can this issue be closed now that #233 has been merged by any chance?

[bjoernQ]

this problem should be fixed now - will close it
@TheButlah feel free to re-open if you disagree