Serial Ring Buffer Behaviour

[Edgar]

(Applies to MultiWii 2.3, Build 1676)

Hi!

I've found some issue in the Serial.cpp code that I want to share here:

As MultiWii uses its own serial functions, it also introduces some different (better suitable) behaviour when the serial RX ring buffers overflow: The original Arduino "HardwareSerial" library keeps the old data in the buffer and just skips the new data not fitting in anymore:

Code: Select all

inline void store_char(unsigned char c, ring_buffer *buffer)
{
  uint8_t i = (uint8_t)(buffer->head + 1) % SERIAL_BUFFER_SIZE;

  // if we should be storing the received character into the location
  // just before the tail (meaning that the head would advance to the
  // current location of the tail), we're about to overflow the buffer
  // and so we don't write the character or advance the head.
  if (i != buffer->tail) {
    buffer->buffer[buffer->head] = c;
    buffer->head = i;
  }
}

MultiWii throws out the oldest data so that always the freshest data is in the buffer:

Code: Select all

void store_uart_in_buf(uint8_t data, uint8_t portnum) {
.
.
.
  uint8_t h = serialHeadRX[portnum];
  serialBufferRX[h++][portnum] = data;
  if (h >= RX_BUFFER_SIZE) h = 0;
  serialHeadRX[portnum] = h;
}

But there is a design flaw: The code above doesn't adjust the serialTailRX-Pointer, when writing new data to a full buffer. This leads to (virtually) completely flushing the buffer by overwriting it with just one byte.

Proposal for solution (untested):

Code: Select all

void store_uart_in_buf(uint8_t data, uint8_t portnum) {
.
.
.
  uint8_t h = serialHeadRX[portnum];
  serialBufferRX[h++][portnum] = data;
  if (h >= RX_BUFFER_SIZE) h = 0;
  serialHeadRX[portnum] = h;
  if (serialTailRX[portnum] == h) 
    if (++serialTailRX[portnum] == RX_BUFFER_SIZE) serialTailRX[portnum] = 0;
}

Greets,
Edgar

[Edgar]

Hi!
Could some of the active developers do a quick review of the proposed fix and confirm (or not confirm) it, so that in case it's true it could be fixed in the public code?

Additionally I've found another flaw in the serial routines:

Code: Select all

  ISR (USART0_UDRE_vect) {
    uint8_t t = serialTailTX[0];
    if (serialHeadTX[0] != t) {
      if (++t >= TX_BUFFER_SIZE) t = 0;
      UDR0 = serialBufferTX[t][0];  // Transmit next byte in the ring
      serialTailTX[0] = t;
    }
    if (t == serialHeadTX[0]) UCSR0B &= ~(1<<UDRIE0); // Check if all data is transmitted . if yes disable transmitter UDRE interrupt
  }

The tail pointer is first inceremented and then the TX-byte is read out instead of first reading out and then incrementing. (also the same in ISRs for UART1,2,3)

I've found these flaws because I wanted to use the mw serial routines for a standalone sbus-receiver/telemetry module and during development I used the routines also for serial debuging. On the PC console then I noticed garbled output.

Hope, this is appreciated

[Alexinparis]

Hi,

(Applies to MultiWii 2.3, Build 1676)

As MultiWii uses its own serial functions, it also introduces some different (better suitable) behaviour when the serial RX ring buffers overflow: The original Arduino "HardwareSerial" library keeps the old data in the buffer and just skips the new data not fitting in anymore:

right

MultiWii throws out the oldest data so that always the freshest data is in the buffer:

right

But there is a design flaw: The code above doesn't adjust the serialTailRX-Pointer, when writing new data to a full buffer. This leads to (virtually) completely flushing the buffer by overwriting it with just one byte.

It's an assumed choice.
Avoiding this test allows a smaller and faster code.
I consider once there is a buffer overflow, there is no need to save anything in the buffer: the serial communication is spoiled anyway. This is something that should never happen. The only important think is to prevent any blocking state.


It was coded like this in MW2.2:

Code: Select all

...
  uint8_t h = serialHeadRX[portnum];
  if (++h >= RX_BUFFER_SIZE) h = 0;
  if (h == serialTailRX[portnum]) return; // we did not bite our own tail?
  serialBufferRX[serialHeadRX[portnum]][portnum] = data;  
  serialHeadRX[portnum] = h;
...

and now:

Code: Select all

...
  uint8_t h = serialHeadRX[portnum];
  serialBufferRX[h++][portnum] = data;
  if (h >= RX_BUFFER_SIZE) h = 0;
  serialHeadRX[portnum] = h;
...

with the following comment:
// we don't care about ring buffer overflow (head->tail) to avoid a test condition : data is lost anyway if it happens

[Alexinparis]

The tail pointer is first inceremented and then the TX-byte is read out instead of first reading out and then incrementing. (also the same in ISRs for UART1,2,3)

could you elaborate more ?
I don't see a flaw here: if this ISR is triggered, we are sure there is at least one data to send and this is the next one in the buffer.

[Edgar]

Hi!

Regarding the first point:
Oh, it was explained in the remark - I've missed that. Sorry.

But anyway, without the test condition, this

MultiWii throws out the oldest data so that always the freshest data is in the buffer:

right

isn't true anymore. Instead, a third behaviour is introduced: MultiWii doesn't throw out old data, as much as is needed to fit in the new data, but it completely empties the buffer everytime at once.

I'm not sure if it's good to change a solidly working algorithm into a "most of the time" working algorithm, just to save an instruction. Especially, if it's a function that many other functionality builds on. But you're right - maybe it wouldn't occour that often in the field


Regarding the second point:
I guess, in general it doesn't matter if pointers of a ring buffer are moved first, and then the data gets read or written from/to. As long as it's done equally, in the write-to-buffer functions as well as in the read-from-buffer functions.

In MultiWii the buffer is written to with this

Code: Select all

void store_uart_in_buf(uint8_t data, uint8_t portnum) {
...
  uint8_t h = serialHeadRX[portnum];
  serialBufferRX[h++][portnum] = data;
  if (h >= RX_BUFFER_SIZE) h = 0;
  serialHeadRX[portnum] = h;
}

So, the data is written first and then the pointer is moved

And, as stated above, in the read-from-buffer ISRs the pointer is moved first and then data is written to (to the new location):

Code: Select all

...
    uint8_t t = serialTailTX[0];
    if (serialHeadTX[0] != t) {
      if (++t >= TX_BUFFER_SIZE) t = 0;
      UDR0 = serialBufferTX[t][0];  // Transmit next byte in the ring
      serialTailTX[0] = t;
    }

I tried to reproduce the mentioned garbled output, but I've already made too many changes to my code, meanwhile. So I couldn't anymore.
But it's a fact, as I used the serial functions more or less unchanged out of the multiwii code, I got these scrambled console outputs repeatedly. (I had to use these functions for debug output also, because one can't use the arduino original serial functions and custom ones at the same time)

In case I'm wrong with my assumptions, I appologize. As I was not 100 percent sure, I wanted to ask here if one of the more skilled coders could do a quick but surely more qualified review.

Greets,
Edgar