CMSIS-Driver icon indicating copy to clipboard operation
CMSIS-Driver copied to clipboard

Unused static variable in ReceiveData()

Open xennex22 opened this issue 3 years ago • 4 comments

There is a static variable n_prev in the ESP32 function ReceiveData which is assigned but not used for anything.

https://github.com/ARM-software/CMSIS-Driver/blob/b2a6e81d3123b7532653c61edbbbda7c3e0d22ac/WiFi/ESP32/ESP32.c#L574

https://github.com/ARM-software/CMSIS-Driver/blob/b2a6e81d3123b7532653c61edbbbda7c3e0d22ac/WiFi/ESP32/ESP32.c#L588-L591

xennex22 avatar Dec 12 '21 23:12 xennex22

Hi, thanks for pointing this out! Looks like some leftover code - we'll take a look at this.

VladimirUmek avatar Dec 13 '21 08:12 VladimirUmek

I am closing in on the original problem I am investigating - there is a race condition in ESP32_Serial.c. This is on a STM32F777.

The SERIAL_COM struct has a buffer count and buffer index. https://github.com/ARM-software/CMSIS-Driver/blob/b2a6e81d3123b7532653c61edbbbda7c3e0d22ac/WiFi/ESP32/ESP32_Serial.c#L59-L60

The COM driver uses DMA to receive serial data into the RxBuf https://github.com/ARM-software/CMSIS-Driver/blob/b2a6e81d3123b7532653c61edbbbda7c3e0d22ac/WiFi/ESP32/ESP32_Serial.c#L269

Then the DMA fills the buffer, UART_Callback will start another DMA transfer. Com.rxc is incremented when this happens. In effect the rx buffer is circular. https://github.com/ARM-software/CMSIS-Driver/blob/b2a6e81d3123b7532653c61edbbbda7c3e0d22ac/WiFi/ESP32/ESP32_Serial.c#L407

In Serial_ReadBuf the calculation for available bytes is this: https://github.com/ARM-software/CMSIS-Driver/blob/b2a6e81d3123b7532653c61edbbbda7c3e0d22ac/WiFi/ESP32/ESP32_Serial.c#L344-L345

What I am seeing is that Serial_ReadBuf gets called where the DMA transfer has just completed but the variables have not been reset. So rxc + Com.drv->GetRxCount() is less than rxi. The calculated available bytes value n overflows, and then is limited to the buffer size len. Thus the serial buffer can be read past the available bytes.

I've confirmed that if rxc + GetRxCount() is less than rxi, if you immediately re-read the values then rxc changes. Here's the quick fix, but there must be something better. Also I don't know what happens when the 32 bit values finally roll over.

n  = Com.rxc + Com.drv->GetRxCount();
if(n<Com.rxi)
  n += SERIAL_RXBUF_SZ;
n -= Com.rxi;

xennex22 avatar Dec 15 '21 16:12 xennex22

I'm not sure where this code repository is, but found another bug. MCI_STM32F7xx.c V1.10

/* Data buffer must be 32 Byte aligned and size is n *32 */
    if ((((uint32_t)data) != 0) || (((block_count * block_size) & 0x1FU) != 0)) {

Always will fail.

if ((((uint32_t)data & 0x1FU) != 0) || (((block_count * block_size) & 0x1FU) != 0)) {

Is probably the intention.

xennex22 avatar Dec 19 '21 21:12 xennex22

Many thanks again for the analysis and further investigation! The serial interface code will apparently also need review.

Second one, MCI driver implementation: MCI_STM32F7xx.c and its code are not part of any public repository, so is also good that you reported it in this context. I checked the MCI driver code and the problem was already found but the STM32F7xx DFP was not yet released. We'll need to check the release schedule but till then you already have working solution.

VladimirUmek avatar Dec 20 '21 11:12 VladimirUmek