gutierrezps / ESP32_I2C_Slave

I2C slave library for ESP32
GNU Lesser General Public License v2.1
81 stars 20 forks source link

Thread-safe implementation on callback. #16

Closed sreekants closed 2 years ago

sreekants commented 2 years ago

I am looking at how the WireSlave.onRequest() method is implemented. The function accepts a callback and I was hoping to pass some context variable to the (say an opaque pointer) callback. As the callback has no argument, the only way to pass a variable to it is through a global variable. It can be done, but this would be bad design practice, particularly in a multi-threaded environment.

There are three ways to fix this:

  1. Pass a void* context pointer to WireSlave.onRequest() . This would however break compatibility with existing code that uses your library.
  2. Create an alternative function that accepts a void* pointer. This would unnecessarily clutter the library.
  3. Replace the low-level calls to i2c_XXX functions and wrap them into reusable functions that WireSlave.update() calls. This may be the best solution leaving the option to implement I/O strategies to the user of your library, but at the same time not losing the simplicity of the current design or the portability of the library. This can be done as inline class functions. Here's a suggested implementation.

` int readSlaveBuffer( uint8_t *data, size_t max_size, TickType_t ticks_to_wait=1 ) { return i2c_slave_read_buffer(portNum, data, max_size, ticks_to_wait ); }

int writeSlaveBuffer( const uint8_t *data, size_t size, TickType_t ticks_to_wait=0, bool reset_fifo=true ) { if( reset_fifo ) i2c_reset_tx_fifo(portNum); return i2c_slave_write_buffer(portNum, data, size, ticks_to_wait); } `

gutierrezps commented 2 years ago

Unfortunately, I couldn't address this issue in a timely manner because I haven't been working with the ESP32 ever since. In this meantime, I2C Slave support has been added to the official ESP32 framework as you can see in the updated README of this repo. Therefore this library is no longer needed and its development will be ceased.