I2C driver read() thread safety

[novirium]

Board

ESP32 Devkit C V4

Device Description

Plain Devkit C V4

Hardware Configuration

Testing was done with an SX1509 GPIO expander, but this is relevant to any I2C device.

Version

latest master (checkout manually)

IDE Name

PlatformIO

Operating System

Linux

Flash frequency

40Mhz

PSRAM enabled

yes

Upload speed

115200

Description

While a lot of the I2C driver is now largely thread/task safe with the use of the lock mutex, as far as I can tell there is still nothing stopping another task from resetting or writing over the top of rxBuffer once the transmission is finished and the lock is released with requestFrom(), but before the end user has finished calling read() to get the bytes of out the buffer. This has been mentioned directly here, but doesn't look like it was addressed when the I2C driver was refactored to be thread-safe.

There's not really any reliable way of the end user signalling that they're finished with the RX buffer built into the driver, so the only way I can see this working in a thread safe way is to allow a user-allocated buffer be optionally passed into requestFrom(), where received bytes get loaded into that instead.

Understandably, changing the Arduino Wire API probably isn't a popular idea, and allowing a new option wouldn't make existing libraries using I2C thread/task safe anyway, which negates a lot of the benefit.

While at the moment writes are thread safe, very few real-world uses of I2C don't also need to read from the slave device. Without it actually being thread safe, is the current Wire implementation potentially hazardous to users, suggesting it can be used between tasks without extra locks when this will very likely result in intermittent and hard-to-trace errors (chance timing occasionally resulting in RX buffer resets and overwrites)?

Note: I do get that the thread safety is not actually advertised anywhere in the documentation as a feature. This only comes up when people (like me) are looking around for how to implement thread/task safety when dealing with I2C on an ESP32, and come across the code in the Wire implementation here with all the locks scattered through it and the option CONFIG_DISABLE_HAL_LOCKS.

Sketch

_

Debug Message

_

Other Steps to Reproduce

No response

I have checked existing issues, online documentation and the Troubleshooting Guide

• I confirm I have checked existing issues, online documentation and Troubleshooting guide.

[me-no-dev]

If you want to do a thread-safe reading, I suggest you drop to esp32-hal-i2c API. https://github.com/espressif/arduino-esp32/blob/master/cores/esp32/esp32-hal-i2c.h#L33

The Wire class calls this directly to fill the RX buffer, so you would be all good. Yes, we long thought on options to make Wire.read thread-safe, but as you found yourself, it can not be safe by design and use (not all received data is always fully read by the code. sometimes only a few bytes are what is needed and people leave unread data in the buffers)

[novirium]

Yep, or put a lock higher up around library calls ourselves if we want to use pre-existing libraries in different tasks that already use Wire in the standard Arduino way.

This can probably be closed as "wontfix" - it was more to file an issue for people to find if they assume the whole driver is safe to use between tasks, and wind up wondering why it's having intermittent problems.

[VojtechBartoska]

Hello, closing this as wontfix.