There is a bug in the ATMEL ASF freertos peripheral control demo using freertos_usart_serial_read_packet()

wlauer wrote on Friday, June 06, 2014:

Hello,

There is a bug in the freertos peripheral control demo available on ASF.
http://asf.atmel.com/docs/latest/sam3x/html/group__freertos__service__group.html

In particular there is a software architecture issue with management of
the circular buffer for the USART demo.
The function, freertos_usart_serial_read_packet(), under certain
circumstances mishandles the read and write pointers and concludes the
circular buffer is empty when it is actually full.

The bug involves the function, freertos_usart_serial_read_packet() (line
491), and the ISR Handler (line 658) in file freertos_usart_serial.c.

freertos_usart_serial_read_packet()  calls:
     freertos_copy_bytes_from_pdc_circular_buffer() in file
freertos_peripheral_control.c line 181 which manipulates
p_rx_buffer_details->next_byte_to_read (the read ptr) @ lines 243
and 252
     configure_rx_dma(usart_index, data_removed)    which
manipulates rx_pdc_parameters.ul_size (the size for dma transfer) @
lines  609, 611, 618, and 623

and the ISR Handler which calls:
     configure_rx_dma(usart_index, data_added); which manipulates
rx_pdc_parameters.ul_addr (the write ptr) @ lines 704 and 711

The function, freertos_copy_bytes_from_pdc_circular_buffer(), determines
how much data is available in the dma buffer. If data is available, it
copies it from the dma buffer to the user buffer and then updates
p_rx_buffer_details->next_byte_to_read. Then, if data was copied from
the buffer and the DMA controller is stopped, it calls
configure_rx_dma(usart_index, data_removed) to restart the dma
controller to fill in the area in the dma buffer that was opened up by
the copy from the dma buffer (the read).

When the DMA transfer (started by the read above) is complete, the
Handler is called @ line 694. The Handler then manipulates
rx_pdc_parameters.ul_addr (the write ptr) to set up the next write into
the dma buffer and calls configure_rx_dma(usart_index, data_added) line
717 to restart the dma engine if there is space available to fill in the
dma buffer.

This works unless a call to freertos_usart_serial_read_packet() is
interrupted between the update of p_rx_buffer_details->next_byte_to_read
and the subsequent call to configure_rx_dma(usart_index, data_removed)
by a dma transfer completion interrupt which updates
rx_buffer_definition->rx_pdc_parameters.ul_addr and calls
configure_rx_dma(usart_index, data_added). The logic in
configure_rx_dma to decide whether the buffer is empty or full line 601
becomes confused and makes the wrong choice.

  1. By example if we start out with the buffer full (i.e. both the read
    and write pointers are equal and the DMA controller is stopped).

  2. Then a call to freertos_usart_serial_read_packet reads one byte which
    starts the dma engine with a size of 1 to fill in the byte we read.
    The read pointer is 1 byte ahead of the write pointer and the
    dma controller is running.

  3. Then another call to freertos_usart_serial_read_packet reads one
    byte. Because the dma controller is still running
    configure_rx_dma(usart_index, data_removed) is not called.
    The read pointer is 2 bytes ahead of the write pointer.

  4. Then another call to freertos_usart_serial_read_packet read one byte
    occurs. The read occurs but after line 256 in
    freertos_copy_bytes_from_pdc_circular_buffer() and before**executing
    line 561 the dma controller receives the byte started in step 2 and
    triggers an interrupt.

  5. The interrupt runs and immediately restarts the dma controller via a
    call to configure_rx_dma(usart_index, data_added) to fill in the two
    bytes from steps 3 and step 4.

  6. Before execution can resume at line 561 (other interrupt processing
    and such) the dma controller completes the transfer of the two bytes in
    step 5 and generates an interrupt.

  7. The interrupt handler advances the write ptr by 2 and the read and
    write ptr are now equal and calls configure_rx_dma(usart_index,
    data_added). This call correctly identifies the buffer as full and stops
    the dma controller by setting size to 0 line 609.
    The read and write pointer are equal and the dma controller is
    stopped because the buffer is full.

  8. Finally execution resumes at line 561 (from step 4) and subsequent
    call to configure_rx_dma(usart_index, data_removed). Seeing that the
    read and write pointers are equal and assuming the cause was the read in
    step 4 incorrectly determines the buffer was empty and at line 612
    starts the action to refill the entire buffer destroying the valid data
    in the dma buffer.

The bug in the freertos_read_packet() function occurs because between
updating the p_rx_buffer_details->next_byte_to_read and the call to
configure_rx_dma(usart_index, data_removed) a previous dma transfer
completes. After the interrupt service routine completes the subsequent
call to configure_rx_dma(usart_index, data_removed) resumes execution it
can misinterpret the state of the full buffer as empty. The update of
p_rx_buffer_details->next_byte_to_read and the call to
configure_rx_dma(usart_index, data_removed) cannot be interrupted and
hence need to execute in the *same *critical section.

Additionally line 627 of configure_rx_dma() incorrectly performs a
sanity check to make sure the dma controller is not programmed to write
beyond the dma buffer length.

configASSERT((rx_buffer_definition->rx_pdc_parameters.ul_size +
rx_buffer_definition->rx_pdc_parameters.ul_size) <=
rx_buffer_definition->past_rx_buffer_end_address);

should read

configASSERT((rx_buffer_definition->rx_pdc_parameters.ul_addr +
rx_buffer_definition->rx_pdc_parameters.ul_size) <=
rx_buffer_definition->past_rx_buffer_end_address);

rtel wrote on Friday, June 06, 2014:

[I deleted the duplicate post]

Thank for taking the time to provide such detailed information. It will take me a while to digest what you have written - and will report report back to Atmel if appropriate.

Regards.

rtel wrote on Friday, June 20, 2014:

I want to ensure my report to Atmel is correct, so see if you agree with the following. The existing code in freertos_uart_serial_read_packet() is like this:

/* Copy as much data as is available, up to however much
a maximum of the total number of requested bytes. */
bytes_read += freertos_copy_bytes_from_pdc_circular_buffer(
    &(rx_buffer_definitions[usart_index]),
    all_usart_definitions[usart_index].pdc_base_address->PERIPH_RPR,
    &(data[bytes_read]),
    (len - bytes_read));

/* The Rx DMA will have stopped if the Rx buffer had become
full before this read operation.  If bytes were removed by
this read then there is guaranteed to be space in the Rx
buffer and the Rx DMA can be restarted. */
if (bytes_read > 0) {
  taskENTER_CRITICAL();
  {
    if(rx_buffer_definitions[usart_index].rx_pdc_parameters.ul_size == 0UL) {
      configure_rx_dma(usart_index, data_removed);
    }
  }
  taskEXIT_CRITICAL();
}

and the fix would change it to be:
taskENTER_CRITICAL();
{
  /* Copy as much data as is available, up to however much
  a maximum of the total number of requested bytes. */
  bytes_read += freertos_copy_bytes_from_pdc_circular_buffer(
      &(rx_buffer_definitions[usart_index]),
      all_usart_definitions[usart_index].pdc_base_address->PERIPH_RPR,
      &(data[bytes_read]),
      (len - bytes_read));

  /* The Rx DMA will have stopped if the Rx buffer had become
  full before this read operation.  If bytes were removed by
  this read then there is guaranteed to be space in the Rx
  buffer and the Rx DMA can be restarted. */
  if (bytes_read > 0) {
    if(rx_buffer_definitions[usart_index].rx_pdc_parameters.ul_size == 0UL) {
      configure_rx_dma(usart_index, data_removed);
    }
  }
}
taskEXIT_CRITICAL();

So the critical section has been removed from inside the if() to around the entire code section.

Do you agree?

Regards.

wlauer wrote on Friday, June 27, 2014:

Yes! I think that would work. But it seems a like a lot of code to put into a critical section. I rewrote freertos_copy_bytes_from_pdc_circular_buffer and moved the critical section out of there and joined it with

if (bytes_read > 0) {
if(rx_buffer_definitions[usart_index].rx_pdc_parameters.ul_size == 0UL) {
configure_rx_dma(usart_index, data_removed);
}
}