i2c slave tx now working, see #99

[RoboDurden]

@maxgerhardt and others :-)
From my issues/99:

Try to get a GD32F130C8 become an i2c slave (SimpleFOC..) but it hangs after several requestEvent()
...
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
...
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 :-)

Maybe add this to Wire/examples/slave_sender/slave_sender.pde:
// The tx buffer is only 32 bytes long. To send bigger structs one must #define WIRE_BUFFER_LENGTH 128 before #include <Wire.h>

Greetings from Germany, Roland and :-)

[maxgerhardt]

I've compared this to https://github.com/arduino/ArduinoCore-megaavr/tree/master/libraries/Wire/src and it looks more simliar now -- though some log in is utility/twi, some in Wire. But basically Wire::write() doing an "additive" slave buffer write is what they're doing too while we were doing an always set amount (obj_s->tx_count = length;).

It does feel a bit weird to be accessing sub-members of the pWire->_i2c object like pWire->_i2c.tx_buffer_ptr itself directly, since it's supposed to be an abstraction object that only gets passed around at the _i2c leve, but I think we can just add a function later with i2c_reset_slave_buf() in twi.cpp that'll take care of this. For now, I'll be taking "works" over "doesn't work".

[RoboDurden]

I may be wrong but the "_i2c level" seems to the old utility/twi.cpp non-objectoriented c code that bundled its runtime data in the _i2c struct.
To make use of that old c code, the _i2c was added to the TwoWire class in wire.cpp.
So i think it is okay to reset _i2c data inside a TwoWire method before calling twi function.
As the TwoWire::onRequestService is a static method it needs to call its own private TwoWire::_i2c via a pointer pWire->_i2c
I think that is okay.

[RoboDurden]

And i think i now have understood that the tx_buffer_ptr can not be used to transmit the data while being used to fill the buffer.
The transmitting is done directly in the i2c_irq function (which i think is directly call from the hardware) here: twi.cpp#L650
i2c_data_transmit(i2c, *obj_s->tx_buffer_ptr++);

And filling the buffer also starts in i2c_irq just one else if clause above:
twi.cpp#L644

        if (i2c_flag_get(i2c, GD32_I2C_FLAG_IS_TRANSMTR_OR_RECVR)) {
            obj_s->slave_transmit_callback(obj_s->pWireObj);
        }

slave_transmit_callback(..) -> TwoWire::onRequestService(..) -> TwoWire::user_onRequest() -> TwoWire::write(..) -> i2c_slave_write_buffer(..) -> *obj_s->tx_buffer_ptr++ = ..

I do not think that another i2c_irq(..) can be called while it has already been triggered.

[maxgerhardt]

I think the flow is like this, given an I2C transaction looks like

Considering only the case of a "slave transmitter" (I2C transaction from master with "READ" as intention, not WRITE): The address bits are sent by the master and determined by hardware to be the "our" (slave's) address. This fires the I2C_INT_FLAG_ADDSEND (address send) interrupt, which per your call chain above triggers user_onRequest(). This has the chance to fill the TX data buffer. IRQ execution ends. After that, when it's time to send the data, the TBE (transmit buffer empty) interrupt fires, in which the IRQ function will feed the data from the prepared TX buffer into I2C peripheral. That happens for every single byte then.

I'm taking that interpretation from the reference manual, see https://github.com/CommunityGD32Cores/gigadevice-firmware-and-docs/tree/main/GD32F1x0.

So yeah, it looks fine to me that the TX buffer is reset in TwoWire::onRequestService(). You also verified it to work on hardware?

[RoboDurden]

Yes i worked the entire day with my Gen2.0 hoverboard test setup = GD32F130C8 and did a lot of debugging to get an ESP32_S2_Mini to be the i2c master.

My code works with that hardware :-)

[maxgerhardt]

Great, could you look over the 2 review comments and fixup if needed? Then we can get this merged.

[maxgerhardt]

Per comments

[maxgerhardt]

I think incrementing tx_count should only happen if we know that we can hold the data, otherwise we get into an inconstistent state if we increment + return error. I.e., first check if (obj_s->tx_count + length <= obj->tx_rx_buffer_size), then increment.

[RoboDurden]

Great, could you look over the 2 review comments and fixup if needed? Then we can get this merged.

This is my first pull request. what 2 review comments ?

[maxgerhardt]

They should be visible in the conversation flow here or in-line in the changed files.

[RoboDurden]

Oh shame on me. Of course there is not type ii2c_status_enum this is simply a copy and paste mishap that left the first char 'i' in place.

Yes i tried to minimize the code and therefore added the length before testing because i the request from master will fail anyway as he will not receive the number bytes he asked for.
But you are correct. It is better to add the length after the tx_buffer_ptr has moved.

[RoboDurden]

i have updated the i2c_slave_write_buffer function in my fork. Do i need to make a second pull request ?

Yes, the hardware still works nicely with the update.

[maxgerhardt]

No, if you update the file in the same branch as this PR is being source from now ( main), then it'll automatically get added to the PR.

Please see review comment above for the final change.

[RoboDurden]

i am really stupid today. No longer focused :-( changed made to my fork

i2c_status_enum i2c_slave_write_buffer(i2c_t *obj, uint8_t *data, uint16_t length){
    struct i2c_s *obj_s = I2C_S(obj);
    if (    (obj_s->tx_count + length) > 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);
    obj_s->tx_count += length;
    return I2C_OK;
}

Sorry that you waste time with my stupid beginner mistakes

[maxgerhardt]

Thanks for the changes! Merged in succesfully.

[RoboDurden]

My first PR, that makes me happy :-)
Can i somehow register here an automatically be notified if someone else in the future will contribute to i2c ?
I might be able to help a bit.

[Candas1]

My first PR, that makes me happy :-)
Can i somehow register here an automatically be notified if someone else in the future will contribute to i2c ?
I might be able to help a bit.

Not specifically about i2c but you could set up the notifications to be informed about any activity on the arduino-gd32