Wire.endTransmission() does a read() instead of write() in case of zero packet length, which leads to I2C scanning problems

[ratio-x]

Hi all,
hi @facchinm,

I think there is a (well-known) issue with endTransmission() for zero bytes length packets (like the ones used during scanning) in mbed Wire.cpp, formerly discussed here: Issue 212. I think the #214 fix made the situation even worse.

The problematic code is here (current version 2.7.2):

uint8_t arduino::MbedI2C::endTransmission(bool stopBit) {
	#ifndef TARGET_PORTENTA_H7
	if (usedTxBuffer == 0) {
		// we are scanning, return 0 if the addresed device responds with an ACK
		char buf[1];
		int ret = master->read(_address, buf, 1, !stopBit);
		return ret;
	}
	#endif
	if (master->write(_address, (const char *) txBuffer, usedTxBuffer, !stopBit) == 0) return 0;
	return 2;
}

Wire.endTransmission() does a read() instead of a write() if the transmitBuffer is empty (usedTxBuffer == 0). Referring to discussions on Issue 212 and the placed comment in #214

// we are scanning, return 0 if the addresed device responds with an ACK

I guess this was intended as a feature in case of I2C bus scanning. But in fact this avoids the return of 0 (device detection), because read() sets the (8th) rw bit HIGH, which results in NAK in most cases.

We know the problem that some vendors incorrectly provide 8-bit addresses which include the read/write bit. Therefore for I2C bus scanning (zero packet length) the rw bit should always be LOW (write) to address the device on 0x00..0x7F (except some reserved addresses...).

I think that's the reason for older platforms (e.g. AVR, ESP) never do a read, but do a write only in endTransmission().

For example, Wire.cpp in AVR platform looks like this; no read() - just write():

uint8_t TwoWire::endTransmission(uint8_t sendStop)
{
  // transmit buffer (blocking)
  uint8_t ret = twi_writeTo(txAddress, txBuffer, txBufferLength, 1, sendStop);
  // reset tx buffer iterator vars
  txBufferIndex = 0;
  txBufferLength = 0;
  // indicate that we are done transmitting
  transmitting = 0;
  return ret;
}

I'm working with NANO 33 BLE (mbed). If you google around you'll find many forum threads with a title "I2C device not found" which are likely related to this code, rather than pullup or frequency issues.

I fixed the problem for many undetected devices with following modification, which also improves the return value 2/3 on NAK on transmit of address/data, conforming to protocol specifications. I didn't notice any negative impacts, especially no devices were no longer detected that were previously detected.

uint8_t arduino::MbedI2C::endTransmission(bool stopBit) {
      // never use read on I2C scanning, since many devices expect write
      // rw bit low (=write) corresponds to an address <= 0x7F, if interpreted as 8-bit address
      /*
      #ifndef TARGET_PORTENTA_H7
      if (usedTxBuffer == 0) {
           // we are scanning, return 0 if the addresed device responds with an ACK
           char buf[1];
           int ret = master->read(_address, buf, 1, !stopBit);
           return ret;
      }
      #endif
      */
      // we need a better handling of return value here
      // always sending 2 is inappropriate
      /*
      if (master->write(_address, (const char *) txBuffer, usedTxBuffer, !stopBit) == 0) return 0;
      return 2;
      */
      // this is still not perfect, because return value of master->write is not evaluated, but works for most cases
      if (master->write(_address, (const char *) txBuffer, usedTxBuffer, !stopBit) == 0) return 0;
      if (usedTxBuffer == 0) return 2;   // received NAK on transmit of address (without data)
      return 3;   // received NAK on transmit of data 
}

I'm not experienced in pull requests, so kindly asking you to chenge the code for next release.

Tank you

Links to discussions (with screenshots of Logic Analyzer):

https://forum.arduino.cc/t/nano-33-ble-i2c-problems-caused-in-mbed-wire-cpp-read-write-bit-on-i2c-scan/964676

RobTillaart/MS5611#31

[ratio-x]

Created PR #415