Loss of arbitration not checked during I2C master read - code can hang here forever

[lupalby]

In function readDataWIRE (here

ArduinoCore-samd/cores/arduino/SERCOM.cpp

Line 635 in d72f117

while( sercom->I2CM.INTFLAG.bit.SB == 0 )

) the while loop checks only the SB bit, that will be set to 1 if a byte is correctly received. In case of bus errors, like loss of arbitration, the MB bit will be set together with the ERROR bit, but the SB bit will remain 0 resulting in the code hanging there forever.
This happens often to me with high bus load and I was able to identify where the code was getting stuck in an infinite loop.
The code is implemented correctly with this additional check for example in function startTransmissionWIRE (

ArduinoCore-samd/cores/arduino/SERCOM.cpp

Lines 512 to 521 in d72f117

	while( !sercom->I2CM.INTFLAG.bit.SB )
	{
	// If the slave NACKS the address, the MB bit will be set.
	// In that case, send a stop condition and return false.
	if (sercom->I2CM.INTFLAG.bit.MB) {
	sercom->I2CM.CTRLB.bit.CMD = 3; // Stop condition
	return false;
	}
	// Wait transmission complete
	}

), but not in readDataWIRE. Unfortunately the readDataWIRE function returns a byte of data, so there is no easy way to inform the caller that the operation failed. The caller, in this case function requestFrom from Wire.cpp (

ArduinoCore-samd/libraries/Wire/Wire.cpp

Lines 65 to 98 in d72f117

	uint8_t TwoWire::requestFrom(uint8_t address, size_t quantity, bool stopBit)
	{
	if(quantity == 0)
	{
	return 0;
	}
	size_t byteRead = 0;
	rxBuffer.clear();
	if(sercom->startTransmissionWIRE(address, WIRE_READ_FLAG))
	{
	// Read first data
	rxBuffer.store_char(sercom->readDataWIRE());
	// Connected to slave
	for (byteRead = 1; byteRead < quantity; ++byteRead)
	{
	sercom->prepareAckBitWIRE(); // Prepare Acknowledge
	sercom->prepareCommandBitsWire(WIRE_MASTER_ACT_READ); // Prepare the ACK command for the slave
	rxBuffer.store_char(sercom->readDataWIRE()); // Read data and send the ACK
	}
	sercom->prepareNackBitWIRE(); // Prepare NACK to stop slave transmission
	//sercom->readDataWIRE(); // Clear data register to send NACK
	if (stopBit)
	{
	sercom->prepareCommandBitsWire(WIRE_MASTER_ACT_STOP); // Send Stop
	}
	}
	return byteRead;
	}

) would try to read following bytes, even if the communication already dropped.
I already implemented a fix that seems to be working. I will do some more testing and I will create a pull request, just to give you guys a starting point for a working solution.