stm32-extended I²C driver NACK handling broken

[chris-durand]

The STM32 extended I²C driver does not handle NACKs correctly. The transaction is detached immediately in the interrupt handler after a NACK has been received.
However, during that time the transfer is still active and transmitting a STOP condition. Any pending new transfer could start from that point on causing a race condition and data corruption.

Another problem could occur, depending on timing. After leaving the interrupt handler most interrupt flags are disabled. When the stop condition transfer finishes the STOPF flag is asserted, indicating that the transfer is completed. The interrupt will not be executed since the flag is disabled. It will remain pending.
When the next transfer starts with the STOPF flag set, it will immediately finish with a success condition and not be executed correctly. That problem was introduced with the attempted fixes in #305 and #311.
The original code tried to correctly detach the transaction after the STOPF flag has been asserted but due to other problems with the code the error handling often was not executed at all causing the program to hang.

The problem can be easily reproduced with a Nucleo with nothing connected to the I2C pins and the following program:

#include <modm/board.hpp>
#include <modm/architecture/interface/i2c_device.hpp>

int main()
{
	Board::initialize();

	MODM_LOG_INFO << "I2C NACK Test" << modm::endl;
	I2cMaster1::connect<GpioB7::Sda, GpioB6::Scl>(I2cMaster1::PullUps::Internal);
	I2cMaster1::initialize<Board::SystemClock, 100_kHz>();

	uint8_t buffer[10]{};
	modm::I2cWriteReadTransaction transaction{0x29};

	while(true) {
		// Issue does not occur if STOPF flag is correctly reset
                // Only for testing, logic error in driver has to be fixed
		// I2C1->ICR = I2C_ICR_STOPCF;

		MODM_LOG_INFO << "I2C_ISR_STOPF: " << (I2C1->ISR & I2C_ISR_STOPF) << "\n";

		const auto time = modm::PreciseClock::now();
		transaction.configureRead(buffer, 10);

		while (!I2cMaster1::start(&transaction, nullptr)) {}
		while (transaction.isBusy()) {}

		const bool success = (transaction.getState() != modm::I2c::TransactionState::Error);
		const auto timediff = modm::PreciseClock::now() - time;
		MODM_LOG_INFO << "duration: " << std::chrono::duration_cast<std::chrono::microseconds>(timediff) << "\n";
		MODM_LOG_INFO << "success: " << success << "\n" << modm::endl;

		Board::LedD13::set(success);
		modm::delay(500ms);
	}

	return 0;
}

Program output:

I2C NACK Test
I2C_ISR_STOPF: 0
duration: 115us us
success: false

I2C_ISR_STOPF: 32
duration: 22us us
success: true

I2C_ISR_STOPF: 32
duration: 22us us
success: true

The first read correctly fails, but on subsequent attempts the STOPF flag is pending before starting the transaction. They will complete with success although no device is connected and nothing has been transmitted.

If I comment in the line resetting the flag the behaviour is correct:

I2C NACK Test
I2C_ISR_STOPF: 0
duration: 115us
success: false

I2C_ISR_STOPF: 0
duration: 117us
success: false

I2C_ISR_STOPF: 0
duration: 117us
success: false

I'll prepare a pull request to fix this later.