I2C Slave race condition leads to handler being out of sync

[arduino]

I2C Slave race condition leads to handler being out of sync

[agdl]

From @stickbreaker on August 19, 2015 1:47

I am not familiar with the DUE, but Shouldn't the I2C HW be stretching SCL, This should prohibit any new transaction before the software has completed handling the current transaction?

Chuck

[agdl]

From @shahokun on August 19, 2015 3:47

Clock stretching in general is not necessarily an issue. In this case, however, the HW is stretching the clock for a Master Read operation, but the software incorrectly thinks it's in a Slave Send/Master Write operation and does not respond correctly. The clock is stretched indefinitely and the transaction never completes.

[agdl]

From @stickbreaker on August 19, 2015 4:29

Ok, I think I see what you have explained. You getting an slave read interrupt while processing a slave write command.
It sounds reasonable to reconfigure before switching modes.
Chuck.

[agdl]

From @shahokun on August 20, 2015 20:36

Testing showed that the proposed solution above did not fix the issue. After more debugging, it seems that the problem is simpler: the Master continues onto the next I2C transaction before the Slave (Arduino) has a chance to respond to the end of the first I2C transaction. This may be because the Slave is busy in a disable interrupts block, or some other rare sequence of events. The race condition goes something like this:

1. The Master sends a Write operation. The Slave has status = SLAVE_RECV.
2. The I2C transaction completes. The Slave gets an interrupt for the I2C completion, but is busy elsewhere and does not service it for a bit.
3. The Master continues on to the next I2C transaction. This time, it is a Read operation. The Slave still has status = SLAVE_RECV (Master Write), but the Arduino TWI hardware thinks it is handling a Master Read operation, and the TWI status register reflects as much.
4. The Slave services the I2C interrupt. Now when it reads the TWI status register (uint32_t sr = TWI_GetStatus(twi);), it indicates the transaction is not complete, and is in Master Read mode.
5. The Arduino TWI hardware stretches the clock until the slave responds, i.e. calls TWI_WriteByte(). However, since the Slave is in status = SLAVE_RECV, this never happens. Meanwhile, the I2C transaction never completes, freeing the Slave to switch modes, because the clock is stretched.

The root cause of this issue is that the Wire library assumes that the status register follows some predictable sequence of events. However, because it is interrupt driven, the hardware status register can change state before the software has a chance to poll the status. Essentially, the Wire driver should not assume that just because it is currently in the SLAVE_RECV state that it will stay that way. I would propose changing the Slave driver in Wire.cpp to something like this:

void TwoWire::onService(void) {
// Retrieve interrupt status
uint32_t sr = TWI_GetStatus(twi);

if (status == SLAVE_IDLE && TWI_STATUS_SVACC(sr)) {
    TWI_DisableIt(twi, TWI_IDR_SVACC);
    TWI_EnableIt(twi, TWI_IER_RXRDY | TWI_IER_GACC | TWI_IER_NACK
            | TWI_IER_EOSACC | TWI_IER_SCL_WS | TWI_IER_TXCOMP);

    srvBufferLength = 0;
    srvBufferIndex = 0;

    // Detect if we should go into RECV or SEND status
    // SVREAD==1 means *master* reading -> SLAVE_SEND
    if (!TWI_STATUS_SVREAD(sr)) {
        status = SLAVE_RECV;
    } else {
        status = SLAVE_SEND;

        // Alert calling program to generate a response ASAP
        if (onRequestCallback)
            onRequestCallback();
        else
            // create a default 1-byte response
            write((uint8_t) 0);
    }
}

if (status != SLAVE_IDLE) {
    if (TWI_STATUS_TXCOMP(sr) && TWI_STATUS_EOSACC(sr)) {
        if (status == SLAVE_RECV && onReceiveCallback) {
            // Copy data into rxBuffer
            // (allows to receive another packet while the
            // user program reads actual data)
            for (uint8_t i = 0; i < srvBufferLength; ++i)
                rxBuffer[i] = srvBuffer[i];
            rxBufferIndex = 0;
            rxBufferLength = srvBufferLength;

            // Alert calling program
            onReceiveCallback( rxBufferLength);
        }


        // Transfer completed
        //TWI_DisableIt(twi, TWI_IDR_RXRDY | TWI_IDR_GACC | TWI_IDR_NACK
        //        | TWI_IDR_EOSACC | TWI_IDR_SCL_WS | TWI_IER_TXCOMP);
        //TWI_EnableIt(twi, TWI_SR_SVACC);
        //status = SLAVE_IDLE;

        // EDIT: Reversed order of operations to prevent nested
        // interrupt triggering.
        // This did not appear to fix the issue so may be unneeded.
        status = SLAVE_IDLE;
        // Transfer completed
        TWI_DisableIt(twi, TWI_IDR_RXRDY | TWI_IDR_GACC | TWI_IDR_NACK
                | TWI_IDR_EOSACC | TWI_IDR_SCL_WS | TWI_IER_TXCOMP);
        TWI_EnableIt(twi, TWI_SR_SVACC);

    }
}

if (status == SLAVE_RECV) {
    if (TWI_STATUS_RXRDY(sr)) {
        if (srvBufferLength < BUFFER_LENGTH)
            srvBuffer[srvBufferLength++] = TWI_ReadByte(twi);
    }
    // EDIT: Write a dummy byte if the TWI peripheral ends up in a
    // different mode than you expected, so it doesn't get stuck indefinitely.
    else if (TWI_STATUS_SVREAD(sr) == 1 && TWI_STATUS_TXRDY(sr) && !TWI_STATUS_NACK(sr)) {
        TWI_WriteByte(twi, 0);
    }
}

if (status == SLAVE_SEND) {
    if (TWI_STATUS_TXRDY(sr) && !TWI_STATUS_NACK(sr)) {
        uint8_t c = 'x';
        if (srvBufferIndex < srvBufferLength)
            c = srvBuffer[srvBufferIndex++];
        TWI_WriteByte(twi, c);
    }
    // EDIT: Read a dummy byte if the TWI peripheral ends up in a
    // different mode than you expected, so it doesn't get stuck indefinitely.
    else if (TWI_STATUS_SVREAD(sr) == 0 && TWI_STATUS_RXRDY(sr)) {
        TWI_ReadByte(twi);
    }
}
}

This may not be the best solution depending on how you want to handle I2C error conditions. However, I think that this issue should be at least be warned about in the Wire documentation. Yes, it results because of a combination of the Master sending I2C messages too often, and the Slave not handling I2C interrupts quickly enough, but the software driver should not be able to get stuck in this unsynchronized state.