Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unused static variable in ReceiveData() #12

Open
xennex22 opened this issue Dec 12, 2021 · 4 comments
Open

Unused static variable in ReceiveData() #12

xennex22 opened this issue Dec 12, 2021 · 4 comments
Assignees

Comments

@xennex22
Copy link

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

static uint32_t n_prev;

if (n == n_prev) {
/* No new bytes received */
n = 0U;
}

@VladimirUmek VladimirUmek self-assigned this Dec 13, 2021
@VladimirUmek
Copy link
Collaborator

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

@xennex22
Copy link
Author

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.

uint32_t rxc; /* Rx buffer count */
uint32_t rxi; /* Rx buffer index */

The COM driver uses DMA to receive serial data into the RxBuf

status = Com.drv->Receive (&RxBuf[0], SERIAL_RXBUF_SZ);

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.

Com.rxc += SERIAL_RXBUF_SZ;

In Serial_ReadBuf the calculation for available bytes is this:

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

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
Copy link
Author

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.

@VladimirUmek
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants