Add lock in Wire.cpp to protect concurrent i2c transactions performed by different tasks

[mircopz]

Description of Change

This is the problem. In my application, two different tasks access the i2c.
Sometimes I get the following assert:

assert failed: xQueueGenericSend queue.c:832 (pxQueue->pcHead != ((void *)0) || pxQueue->u.xSemaphore.xMutexHolder == ((void *)0) || pxQueue->u.xSemaphore.xMutexHolder == xTaskGetCurrentTaskHandle())
...
0x40096eee: xQueueGenericSend at .../esp-idf/components/freertos/queue.c line 821 (discriminator 2)
0x400f05ed: TwoWire::beginTransmission(unsigned short) at .../libraries/Wire/src/Wire.cpp line 418
:  (inlined by) TwoWire::beginTransmission(unsigned short) at .../libraries/Wire/src/Wire.cpp line 408
0x400f076c: TwoWire::beginTransmission(unsigned char) at .../libraries/Wire/src/Wire.cpp line 637

This comes from this piece of code:

    void TwoWire::beginTransmission(uint16_t address)
   ...
    if(nonStop && nonStopTask == xTaskGetCurrentTaskHandle()){
        log_e("Unfinished Repeated Start transaction! Expected requestFrom, not beginTransmission! Clearing...");
        //release lock
        xSemaphoreGive(lock);
    }

The xSemaphoreGive is not always correct, because I can't release a mutex if it is taken by another thread.
The solution is to protect all i2c transaction, by taking the lock at the beginning of each transaction if the current task is different from the one already performing a i2c transaction.

Tests scenarios

I have tested my Pull Request on Arduino-esp32 core v2.0.8 with ESP32 Board with this scenario.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ me-no-dev
❌ mircopz
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[me-no-dev]

Can you please provide example code that can replicate the issue you are seeing? The code is meant to release the lock only if called from the same task that last called endTransaction(false). If your code calls that and does not immediately call requestFrom, then you should fix your code. In reality you should never end up in that if

[mircopz]

Sorry but now I don't have any example code... I should work on it to recreate a simple example.
I try to add an example, I don't know if this can help.
This is my application code:

int8_t readBytes(uint8_t regAddr, uint8_t length, uint8_t *data) 
{
  Wire.beginTransmission(devAddr);
  Wire.write(regAddr);
  Wire.endTransmission(false);
  return Wire.requestFrom(devAddr, length, data);
}

The readBytes function is called from two tasks (T1 and T2). Let's assume that T2 calls beginTransmission is done while T1 is marking the transaction as non-stop

uint8_t TwoWire::endTransmission(bool sendStop)
{
 ...
       nonStop = true;
       // HERE --> the other task call its beginTransmission
#if !CONFIG_DISABLE_HAL_LOCKS
       nonStopTask = xTaskGetCurrentTaskHandle();
#endif
 ...
}

In this case, T2 finds nonStop as true, but nonStopTask is not updated yet (it's still T2 and not T1). So T2 tries to release the lock because both conditions are true, but the last task which acquired the lock was T1 (hence the assert)

   if(nonStop && nonStopTask == xTaskGetCurrentTaskHandle()) {
       log_e("Unfinished Repeated Start transaction! Expected requestFrom, not beginTransmission! Clearing...");
       //release lock
       xSemaphoreGive(lock);
   }

With my commit, all i2c transactions are protected, not only the non-stop transactions.

[VojtechBartoska]

Adding to 3.0.0 milestone so we will resolve this.