r910 introduces race condition into serial code

[Tommie]

I think the last change introduces a race condition into the serial code.

Consider this instruction sequence:

Code: Select all

void serialize8(uint8_t a)  {
  headTX++;
  if (headTX == TX_BUFFER_SIZE) {
    headTX = 0;
  }
  bufTX[headTX] = a;
  checksum ^= a;
  #if !defined(PROMICRO)
    UCSR0B |= (1<<UDRIE0);
  #endif
}

For a brief moment, headTX will have the value of TX_BUFFER_SIZE; this would not pose a problem, but during this time, the serial ISR could fire:

Code: Select all

  ISR_UART {
    if (headTX != tailTX) {
      tailTX++;
      if (tailTX == TX_BUFFER_SIZE) {
        tailTX = 0;
      }

since headTX is outside of the allowed range [0, TX_BUFFER_SIZE-1], tailTX will "overtake" headTX and crush the entire buffer content.

In my opinion, there are two solutions to counter this issue:

a) restore the previous if/else construct, which will always atomically leave the variables in a well defined state
b) disable interrupts (cli()/sei()) while inside the serializer function (obviously not an option)

[kos]

57600 , help reduce the race condition.. or swicth to promicro

so..seriously

we have to keep the length of the serial burst low enough for that case to not happen .

we could also let the mcu time to reply , but this is optional .. it is the requester task to not over flood the mcu. it worked that way , it will work that way . no need to add thread or so and extra sync . the osd is smart enough to request at a safe rate.

you a trying to circumvent the inherent nature of an interrupt driven uart

[Tommie]

you a trying to circumvent the inherent nature of an interrupt driven uart

No, I am trying to circumvent the consequences of non-thread-safe programming. Which is easy if you change from one valid state to the next and ensure atomicity - which is not an issue if the state is represented by a single byte (headTX).

[kos]

that will increase cycletime

[kos]

that will increase cycletime

This not an encouragement ....

[Tommie]

Since the UART receive interrupt is disabled while inside evaluateCommand(), any bytes received wile inside the function are silently discarded and not buffered. This is not a clean way to handle the situation;
why not simply revert to the old style of incrementing headTX and tailTX, which is simple and safe?

Patch: https://github.com/wertarbyte/multiwii- ... 79ebe2a817

[Alexinparis]

don't make a confusion between RX interrupt and TX interrupt.
triggered via RXCIE0 and UDRIE0
only the TX interrupt is deactivated during the serialization process

[Tommie]

And how does this the race condition? The same occurs with headRX as well:

Code: Select all

uint8_t SerialRead(uint8_t port) {
  uint8_t c = serialBufferRX[serialTailRX[port]][port];
  if ((serialHeadRX[port] != serialTailRX[port])) {
    serialTailRX[port]++;
    if (serialTailRX[port] == RX_BUFFER_SIZE) {
      serialTailRX[port] = 0;
    }
  }
  return c;
}

and during the receive interrupt:

Code: Select all

static void inline store_uart_in_buf(uint8_t data, uint8_t portnum) {
  /* the received data byte */
  uint8_t d = data;
  uint8_t i = serialHeadRX[portnum];
  i++;
  if (i == RX_BUFFER_SIZE) {
    i = 0;
  }
  /* we did not bite our own tail? */
  if (i != serialTailRX[portnum]) {
   serialBufferRX[serialHeadRX[portnum]][portnum] = d;
   serialHeadRX[portnum] = i;
  } else {
    /* sorry, this byte is lost :-/ */
  }
}

The interrupt does not notice that the ring buffer has bitten his own tail and keeps on pumping data.

I still advocate for return to the safe and sound incrementation via if/else as indicate by my patch. Turning interrupts off and on at varous locations just doesn't cut it, at least not cleanly. And it even occupies two additional bytes compared to the initial solution in my patch.

[Alexinparis]

It's maybe true for RX interrupt, I need to check.

Turning interrupts off and on at varous locations just doesn't cut it, at least not cleanly.

For TX interrupt, it's only your subjective view.

[itain]

I see the code is now changed but not fixed, and also not very efficient.

A race may still occur, headTX is updated before data is stored in the buffer. and the ISR may just send the old (junk) value in that buffer location.

Note too that headTX and tailTX are volatile. The compiler must not optimize accesses to these variables. It is better to use a local variable for all the calculations and have only one load and one store to memory.
Something like:

Code: Select all

void serialize8(uint8_t a)  {
  uint8_t tempHeadTX = headTX;
  if(++tempHeadTX >= sizeof(bufTX))
    tempHeadTX = 0;
  bufTX[tempHeadTX] = a;  // Store to bufTX before updating headTX!
  headTX = tempHeadTX;
  checksum ^= a;
  #if !defined(PROMICRO)
    UCSR0B |= (1<<UDRIE0);
  #endif
}

[Tommie]

Oh, thanks for the hint. Your version looks completely valid, I did not think of that. And it also brings down the code size a fair bit: From 26714 bytes to 26702 bytes

Testing it right now...

[kos]

Since the UART receive interrupt is disabled while inside evaluateCommand(), any bytes received wile inside the function are silently discarded and not buffered. This is not a clean way to handle the situation;

https://www.google.fr/search?q=ftdi+buffer

... 256 Byte receive buffer and 128 Byte transmit buffer utilising buffer ..

http://www.ftdichip.com/Products/FT232R.htm