Closed RoboDurden closed 1 year ago
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.
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 !
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:
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 :-)
Okay here my pull request: https://github.com/CommunityGD32Cores/ArduinoCore-GD32/pull/100 :-)
Fixed per #100
@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..
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
I also do not understand the validation in i2c_slave_write_buffer(...) in twi.cpp#L550
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
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 ifhead
is the position that should be sent next:And the i2c_slave_write_buffer function adds new data to the
tail
: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 :-)