I2C_slave hangs because of tx_buffer_ptr is never set back and *tx_buffer_ptr gets out of bounds..

[RoboDurden]

Sorry @maxgerhardt for causing this issue. Try to get a GD32F130C8 become an i2c slave (SimpleFOC..) but it hangs after several requestEvent() with

Wire.write(aiBuffer,30); // hanging after: 1 -> 41 calls , 2 -> 21 calls , 10 -> 17 calls , 20 -> 12 calls , 30 -> 7 calls , 40 -> 0 calls

The buffer pointer gets initialized in the TwoWire constructor Wire.cpp#L42
_i2c.tx_buffer_ptr = _tx_buffer.buffer;

but is never reset yet incremented when a byte is transmitted in twi.cpp#L650

        if (obj_s->tx_count > 0) {
            i2c_data_transmit(i2c, *obj_s->tx_buffer_ptr++);
            obj_s->tx_count--;
        }

I also do not understand the validation in i2c_slave_write_buffer(...) in twi.cpp#L550

    if (length > obj->tx_rx_buffer_size) {
        ret = I2C_DATA_TOO_LONG;
    } else {
        /* check the communication status */
        for (i = 0; i < length; i++) {
            *obj_s->tx_buffer_ptr++ = *(data + i);
        }
        obj_s->tx_count = length;
        obj_s->tx_buffer_ptr = obj_s->tx_buffer_ptr - length;
    }

This function can easily be called multiple times with Wire.write(..) within the requestEvent() event function.
So already without reseting tx_buffer_ptr, it can easily overflow with multiple calls per event.
That event function is called in Wire.cpp#L297 and

        // reset tx buffer iterator vars
        // !!! this will kill any pending pre-master sendTo() activity
        pWire->_tx_buffer.head = 0;
        pWire->_tx_buffer.tail = 0;

But i do not see this head and tail to have any effect on the tx_buffer_ptr

I fear, that the code has to be rewritten completely.
And as i guess the tx_buffer should be a cyclic buffer, using a pointer is aweful.
why not make it readable like aiTx_Buffer[iWritePos % buffSize] ???
And if head is the position that should be sent next:

 i2c_data_transmit(i2c, aiTx_Buffer[head % buffSize)]);
head = (head+1) % buffSize;

And the i2c_slave_write_buffer function adds new data to the tail:

        for (i = 0; i < length; i++) 
        {
            if (tail == head) return I2C_DATA_BUFFER_FULL
            aiTx_Buffer[tail % buffSize)] = *(data + i);
            head = (tail+1) % buffSize;
        }

If the i2c-slave code indeed has never been tested, i could make these changes. I have already spent the entire day on debugging this.
But first i would like to know if you would be happy to add to this repo :-)
And for sure i will be happy if everything with the code is fine and me simply being to stupid to use it porperly :-))

Greetings from Germany, Roland and :-)

[maxgerhardt]

A lot of this code has been taken as-is from Gigadevice without ever being written or looked at in detail by me -- it's more than plausible that we have a bug in the I2C slave implementation, or that calling the API in an unforseen manner causes these issues. I'll appreciate a PR on this matter if you can. If not, a small minimal sketch that reproduces the problem will also enable me to look into it.

[RoboDurden]

I have never made a pull request yet but would love to. I will fork this repo and try to get a working i2c_slave code.
The i2c_master_write code alreaedy implements head and tail.
I will try to be as close as possible to that code.
thanks for the quick response !

[RoboDurden]

Okay here my changes :-)
As the i2c_slave_tx is done via interrupt in twi.cpp, i could not use the cyclic head/tail method of wire.cpp :-(
So i assume (maybe i am wrong) that while the TwoWire::onRequestService is processed, the I2C interrupt handler i2c_irq(..) of twi.cpp DOES NOT get called (which also uses inrements the tx_buffer_ptr to output the buffer).

Then i can use the tx_buffer_ptr during the onRequestService to fill the buffer, reset it at its end and let the i2c_irq reuse tx_buffer_ptr to output the buffer.
Then only two changes need to be done:

Wire.cpp#L297 ->

void TwoWire::onRequestService(void* pWireObj)
{
    TwoWire* pWire = (TwoWire*) pWireObj;
    // don't bother if user hasn't registered a callback
    if (pWire->user_onRequest) {
        // reset master tx buffer iterator vars
        // !!! this will kill any pending pre-master sendTo() activity
        pWire->_tx_buffer.head = 0;
        pWire->_tx_buffer.tail = 0;

        // reset slave tx buffer iterator vars
        pWire->_i2c.tx_buffer_ptr = pWire->_tx_buffer.buffer;
        pWire->_i2c.tx_count = 0;

        // alert user program
        pWire->user_onRequest();

        // reset slave tx buffer iterator to let interrupt transmit the buffer
        pWire->_i2c.tx_buffer_ptr = pWire->_tx_buffer.buffer;
    }
}

and twi.cpp#L544 ->

ii2c_status_enum i2c_slave_write_buffer(i2c_t *obj, uint8_t *data, uint16_t length)
{
    struct i2c_s *obj_s = I2C_S(obj);

    obj_s->tx_count += length;
    if (obj_s->tx_count > obj->tx_rx_buffer_size) 
        return I2C_DATA_TOO_LONG;

    uint8_t i = 0;
    for (i; i < length; i++) 
        *obj_s->tx_buffer_ptr++ = *(data + i);
    return I2C_OK;
}

I can upload test code for ESP32_S2_Mini (master) <-> GD32F130C6/C8 (slave) to a github repo if requested.

ESP32_S2_Mini (master):

  // i2c: send data to slave
  oMaster2Slave.iValue++;   // to see something change :-)
  Wire.beginTransmission(I2C_SLAVE_ADDR);
  SerialWrite(Wire,oMaster2Slave);     // to send struct with crc: from openPanasonicBike
  byte error = Wire.endTransmission();
  if (error != 0) 
    OUT2N("master: i2c request failed, timeout error code",error)


  // i2c request data from slave
  Wire.requestFrom(I2C_SLAVE_ADDR, sizeof(oSlave2Master)+1);  // +1 because of one additional byte crc
  delay(1);
  unsigned int iAvail = Wire.available();
  if (SerialRead(Wire,oSlave2Master,iAvail))
  {
    OUT2T("slave.iValue",oSlave2Master.iValue);
    OUT2T("slave.fValue",oSlave2Master.fValue);
  }
  else  if (iAvail)
  {
    OUT2("\nmaster: CRC fail or too less avail: ",iAvail);
  }

GD32F130C6/C8 (slave) :

void requestEvent() 
{
  iRequests++;
  SerialWrite(Wire,oSlave2Master);     // to send struct with crc: from openPanasonicBike
}

void receiveEvent(int iAvail) 
{
  iReceived++;
  if (SerialRead(Wire,oMaster2Slave,iAvail))
  {
    oSlave2Master.iValue = oMaster2Slave.iValue;
    oSlave2Master.fValue += 0.01;
  }
  else  if (iAvail)
  {
    OUT2("\nCRC fail. avail: ",iAvail);
  }

  DEBUG( 
    OUT2T("slave",millis()) 
    OUT2T("iRequests",iRequests);
    OUT2(" iReceived",iReceived);
    OUTLN()
  )
}

SerialWrite and SerialRead are helper functions that add a 8bit checksum to the struct and send it likewise over i2c or uart.
DEBUG,OUT2,etc are serial logging macros and lead to:

slave.iValue: 242	 slave.fValue: 13.32	 slave: 50497	iRequests: 1018	 iReceived: 1017
slave.iValue: 243	 slave.fValue: 13.33	 slave: 50517	iRequests: 1019	 iReceived: 1018
slave.iValue: 244	 slave.fValue: 13.34	 slave: 50539	iRequests: 1020	 iReceived: 1019
slave.iValue: 245	 slave.fValue: 13.35	 slave: 50559	iRequests: 1021	 iReceived: 1020
slave.iValue: 246	 slave.fValue: 13.36	 slave: 50579	iRequests: 1022	 iReceived: 1021
slave.iValue: 247	 slave.fValue: 13.37	 slave: 50600	iRequests: 1023	 iReceived: 1022

That was a send+request at 20ms. 5ms also work.

The tx buffer is only 32 bytes long.
To send bigger structs one must #define WIRE_BUFFER_LENGTH 128 before #include <Wire.h>

@maxgerhardt , feel free to simply replace my new two functions with the old non-functional ones :-)

01:20 , a good night from Germany :-)

[RoboDurden]

Okay here my pull request: #100 :-)

[maxgerhardt]

Fixed per #100

[RoboDurden]

@maxgerhardt i have uploaded the i2c test code to: https://github.com/RoboDurden/GD32_I2C_Slave
Thanks again for your quick response here.
I will not have the happiness (=time) to join the community here. That is why i asked for a special "i2c label" notification. If then i would get notified i would be way more motivated to help..