Serial(__UARTx__ ) and FSP code issue can lose data

[KurtE]

@facchinm @maidnl and others,

As I have mentioned in another issue: #44, there are some pieces missing in the current implementation of the SerialX classes as well as things that could improve performance. Toward this goal I have tried to implement code to flesh out some of the missing pieces, like availableForWrite, flush, etc. #59 marked Draft.

The main thing I am trying to do, is to change the implementation of the write method, to use the rather large FIFO buffer already built into your Uart class.

Roughly: when you call Write, the code would put the new data into your FIFO. If the UART was not active, have the class have a temporary buffer maybe 8 or 16 bytes or the like, that the code would extract up to that many bytes from the fifo and call off to: R_SCI_UART_Write with that buffer and length.

When the FSP method and corresponding ISR code completes the processing of this buffer, it will call back to your code class saying that it completed that buffer. I would expect at that point I should be able to extract the next set of bytes into my temporary buffer and call some FSP method, which might be R_SCI_UART_Write, or potentially something else to continue to output.

The problem:
In r_rsi_b_uart.c, the ISR function: void sci_b_uart_txi_isr (void)
Will see that it is more data to output, so suppose the tx_src_bytes has is equal to 1, i.e the last element.
In simplified explanation (disregarding the FIFO case), will place the last character into TDR register and decrement the count.

It will then see that the tx_src_bytes is now 0, and it will do the callback.

Now in the callback I calling R_SCI_UART_Write, would at times would blindly put the first character of the new buffer into the TDR register without checking the TDRE status, and would overwrite the last character that I output in the previous buffer.
The code looks different now, not sure if there as a new version of this stuff that my last sync of all sub-systems may have changed. I need to look closer, but almost like it still turns off the interrupts, and maybe now waits until everything transfers, before starting back up? Will experiment. But either way, this is not an ideal solution.

possibilities

1. Fix or extend the FSP code - For example if it is stuffing out a character regardless of TDRE (or FIFO full), add a check to not do so.
   or maybe better solution, either allow the callback method to call something to say continue using this buffer and length, or have method for either FIFO of buffers or double buffer of them. Or add the FIFO of actual bytes to the FSP code.

If this is the preferred approach, it brings up questions like:
a) How do you make changes to FSP? Does Arduino have to submit request to Renesas, and wait for hopefully new release.
b) How do you build a new FSP archive file to build with? So far it looks like you need to an IDE, but then it looks like it requires several other things as well.

2. Mostly use FSP, but for example add our own method, which might be something new or a fixed version of an existing api. Example updates the buffer/counter in their data structures.

Sort of messy but at least you can easily get up and running.

3. Replace FSP, with hopefully a lighter weight and hopefully faster implementation. This potentially could be only part of the code or the whole UART code. I do not know Arduino's goals for the usage of FSP. Is it the goal to have the Arduino code as a wrapper of the Renesas code, or the FSP code was simply the fastest way to the goal of being able to release the new board.

My WIP branch I mentioned, is a partial 3). Probably not in a clean way. I replaced their TX ISR, with my own. Not sure if Arduino has a prescribed way to do so, or if it is frowned upon. I did it by hacking up an equivalent Teensy code: attachInterruptVector, where I passed in the ISR number for TX and my own code, which uses the FIFO queue already built into the code.

Question:
What strategy does Arduino prefer to deal with, issues with FSP, with bugs, limitations, or bloat?

I know that none of these solutions are overly original, but having a good idea of your preferences, make it a lot easier to make progress and hopefully an acceptable solution.

Thanks

Kurt

[KurtE]

As it appears no interest, will go ahead and close.

My assumptions are now: Avoid changing FSP or the like. No interest, no instructions on how or if...

Work around, the original issue...

First attempt to wait for TDRE sort of worked:

          uint32_t start_time = micros();
          if (uart_ptr->uart_ctrl.fifo_depth) {
            // Hopefully they don't completely fill the FIFO before calling us.
          } else {
            // Uart does not have FIFO so check TDRE..
            while ((((uint32_t)micros() - start_time) < 250) && !uart_ptr->uart_ctrl.p_reg->SSR_b.TDRE) {
              DBGDigitalToggle(DEBUG_PIN_WRITE_TOGGLE);
            }
          }

          R_SCI_UART_Write(&(uart_ptr->uart_ctrl), uart_ptr->tx_fsi_buffer, cb);

Some test cases worked fine, however it would at times be in there long enough that we would miss an RX interrupt and end up with overflow and mis character on receive.

Instead, I manually re-setup the data and interrupts, to continue the transfer...

          uart_ptr->uart_ctrl.tx_src_bytes = cb;
          uart_ptr->uart_ctrl.p_tx_src     =  uart_ptr->tx_fsi_buffer;

          // and reenable the TIE and not TEIE
          uart_ptr->uart_ctrl.p_reg->SCR &= (uint8_t) ~(R_SCI0_SCR_TEIE_Msk); // don't wait on transer end interrupt.
          uart_ptr->uart_ctrl.p_reg->SCR |= (uint8_t)R_SCI0_SCR_TIE_Msk;

Note: as I mentioned in different issue, though I think there is still some risk, as the code to simply copy up to 16 bytes out of the txBuffer using the Safe stuff is real slow. And the unsafe code is truly unsafe, even for single producer and single consumer.

But that is a different issue.