Quality RTOS & Embedded Software

 Real time embedded FreeRTOS RSS feed 
Real time embedded FreeRTOS mailing list 
Quick Start Supported MCUs PDF Books Trace Tools Ecosystem TCP & FAT Training




Loading

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

Posted by wlauer on June 6, 2014

Hello,

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

In particular there is a software architecture issue with management of the circular buffer for the USART demo. The function, freertosusartserialreadpacket(), 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, freertosusartserialreadpacket() (line 491), and the ISR Handler (line 658) in file freertosusartserial.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, freertoscopybytesfrompdccircularbuffer(), 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 prxbufferdetails->nextbytetoread. Then, if data was copied from the buffer and the DMA controller is stopped, it calls configurerxdma(usartindex, dataremoved) 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 rxpdcparameters.uladdr (the write ptr) to set up the next write into the dma buffer and calls configurerxdma(usartindex, 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 freertosusartserialreadpacket() is interrupted between the update of prxbufferdetails->nextbytetoread and the subsequent call to configurerxdma(usartindex, *dataremoved) by a dma transfer completion interrupt which updates rxbufferdefinition->rxpdcparameters.uladdr and calls configurerxdma(usartindex, *data_added). The logic in
configurerxdma 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 freertosusartserialreadpacket 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 freertosusartserialreadpacket reads one byte. Because the dma controller is still running configurerxdma(usartindex, dataremoved) is not called. The read pointer is 2 bytes ahead of the write pointer.

  4. Then another call to freertosusartserialreadpacket read one byte occurs. The read occurs but after line 256 in freertoscopybytesfrompdccircularbuffer() 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 configurerxdma(usartindex, dataadded) 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 configurerxdma(usartindex, dataadded). 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 configurerxdma(usartindex, dataremoved). 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 freertosreadpacket() function occurs because between updating the prxbufferdetails->nextbytetoread and the call to configurerxdma(usartindex, dataremoved) a previous dma transfer
completes. After the interrupt service routine completes the subsequent call to configurerxdma(usartindex, dataremoved) resumes execution it can misinterpret the state of the full buffer as empty. The update of prxbufferdetails->nextbytetoread and the call to configurerxdma(usartindex, dataremoved) cannot be interrupted and hence need to execute in the *same *critical section.

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

configASSERT((rxbufferdefinition->rxpdcparameters.ulsize + rxbufferdefinition->rxpdcparameters.ulsize) <= rxbufferdefinition->pastrxbufferendaddress);

should read

configASSERT((rxbufferdefinition->rxpdcparameters.uladdr + rxbufferdefinition->rxpdcparameters.ulsize) <= rxbufferdefinition->pastrxbufferendaddress);


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

Posted by rtel on June 6, 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.

Attachments

alternate (7672 bytes)

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

Posted by rtel on 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 freertosuartserialreadpacket() is like this:

~~~~ /* Copy as much data as is available, up to however much a maximum of the total number of requested bytes. */ bytesread += freertoscopybytesfrompdccircularbuffer( &(rxbufferdefinitions[usartindex]), allusartdefinitions[usartindex].pdcbaseaddress->PERIPHRPR, &(data[bytesread]), (len - bytesread));

/* 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 (bytesread > 0) { taskENTERCRITICAL(); { if(rxbufferdefinitions[usartindex].rxpdcparameters.ulsize == 0UL) { configurerxdma(usartindex, dataremoved); } } taskEXIT_CRITICAL(); } ~~~~
and the fix would change it to be:

~~~~ taskENTERCRITICAL(); { /* Copy as much data as is available, up to however much a maximum of the total number of requested bytes. */ bytesread += freertoscopybytesfrompdccircularbuffer( &(rxbufferdefinitions[usartindex]), allusartdefinitions[usartindex].pdcbaseaddress->PERIPHRPR, &(data[bytesread]), (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 (bytesread > 0) { if(rxbufferdefinitions[usartindex].rxpdcparameters.ulsize == 0UL) { configurerxdma(usartindex, dataremoved); } } } taskEXITCRITICAL(); ~~~~

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

Do you agree?

Regards.

Attachments

alternate (7672 bytes)

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

Posted by wlauer on 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 freertoscopybytesfrompdccircularbuffer and moved the critical section out of there and joined it with

if (bytesread > 0) { if(rxbufferdefinitions[usartindex].rxpdcparameters.ulsize == 0UL) { configurerxdma(usartindex, data_removed); } }

Attachments

alternate (7672 bytes)


[ Back to the top ]    [ About FreeRTOS ]    [ Sitemap ]    [ ]




Copyright (C) 2004-2010 Richard Barry. Copyright (C) 2010-2016 Real Time Engineers Ltd.
Any and all data, files, source code, html content and documentation included in the FreeRTOSTM distribution or available on this site are the exclusive property of Real Time Engineers Ltd.. See the files license.txt (included in the distribution) and this copyright notice for more information. FreeRTOSTM and FreeRTOS.orgTM are trade marks of Real Time Engineers Ltd.

Latest News:

FreeRTOS V9.0.0 is now available for download.


Free TCP/IP and file system demos for the RTOS


Sponsored Links

⇓ Now With No Code Size Limit! ⇓
⇑ Free Download Without Registering ⇑


FreeRTOS Partners

ARM Connected RTOS partner for all ARM microcontroller cores

Renesas Electronics Gold Alliance RTOS Partner.jpg

Microchip Premier RTOS Partner

RTOS partner of NXP for all NXP ARM microcontrollers

Atmel RTOS partner supporting ARM Cortex-M3 and AVR32 microcontrollers

STMicro RTOS partner supporting ARM7, ARM Cortex-M3, ARM Cortex-M4 and ARM Cortex-M0

Xilinx Microblaze and Zynq partner

Silicon Labs low power RTOS partner

Altera RTOS partner for Nios II and Cortex-A9 SoC

Freescale Alliance RTOS Member supporting ARM and ColdFire microcontrollers

Infineon ARM Cortex-M microcontrollers

Texas Instruments MCU Developer Network RTOS partner for ARM and MSP430 microcontrollers

Cypress RTOS partner supporting ARM Cortex-M3

Fujitsu RTOS partner supporting ARM Cortex-M3 and FM3

Microsemi (previously Actel) RTOS partner supporting ARM Cortex-M3

Atollic Partner

IAR Partner

Keil ARM Partner

Embedded Artists