akospasztor / stm32-dma-uart

Efficient DMA timeout mechanism for peripheral DMA configured in circular mode demonstrated on a STM32 microcontroller.
MIT License
127 stars 46 forks source link

False IDLE check ignores real data #4

Closed diffstorm closed 6 years ago

diffstorm commented 6 years ago

Hello,

Define DMA_BUF_SIZE as 15 Then send 5 bytes 3 times. You will get the first 2 data, but the 3rd data will be ignored because of the match with the following condition;

    /* Ignore IDLE Timeout when the received characters exactly filled up the DMA buffer and DMA Rx Complete IT is generated, but there is no new character during timeout */
    if(dma_uart_rx.flag && currCNDTR == DMA_BUF_SIZE)
    { 
        dma_uart_rx.flag = 0;
        return;
    }

Additionally;

What if IDLE interrupt fires and then additional data comes? In case the transmitter cannot send data continuously, but takes some little break sometimes? The timer count down will not stop, maybe it will fire in the middle of receiving.

    /* DMA timer */
    if(dma_uart_rx.timer == 1)
    {
        /* DMA Timeout event: set Timeout Flag and call DMA Rx Complete Callback */
        dma_uart_rx.flag = 1;
        hdma_usart2_rx.XferCpltCallback(&hdma_usart2_rx);
    }
    if(dma_uart_rx.timer) { --dma_uart_rx.timer; }

Additionally 2; Your copy code is wrong, since you should copy using 2 different offsets when the circular buffer start and length are not linear.

    /* Copy and Process new data */
    for(i=0,pos=start; i<length; ++i,++pos)
    {
        data[i] = dma_rx_buf[pos];
    }

To be something like;

    if(Length > 0)
    {
        if(Start + Length >= DMA_RX_BUFFER_SIZE)
        {
            Put(&DMA_RX_Buffer[Start], DMA_RX_BUFFER_SIZE - Start);
            Put(&DMA_RX_Buffer[0], Length - (DMA_RX_BUFFER_SIZE - Start));
        }
        else
        {
            Put(&DMA_RX_Buffer[Start], Length);
        }
    }
akospasztor commented 6 years ago

First, I would seriously recommend to study how the DMA works.

Secondly, please read and try to understand how this code works.

Lastly, we are using this solution with systems working autonomously for years without any errors. So I suggest to try this demo out before stating that this implementation is wrong. Use a debugger and step through the code if necessary to understand and verify that it indeed really works as it should.

diffstorm commented 6 years ago

Hello, First of all I want to thank you for the useful code. I have fixed the problems on my implementation and just wanted to inform you about the bugs. They are real as far as I see the problems by testing. Please check my suggestions again and remove/close the issue if you think that it is really not necessary. Please don't recommend me to study the DMA :)

Set the buffer size to 15 and try my suggestion, also since your DMA works in circular mode, your buffer is not always linear, so you cannot copy the data from "start" with "length" in just single for loop. The bugs are obvious. The reason you don't see them is because you should use much bigger buffer and the bugs occur rarely.

akospasztor commented 6 years ago

Hi, first of all excuse me for being too harsh in my previous comment.

So let's see the scenarios. In general, the processing function can be called upon the timeout and when the dma circular buffer is "complete", in other words when the circular buffer's counter reaches zero.

Scenario 1: buffer size is 15, and let's suppose we have 3 chunks of data arriving. First two chunks are processed after the timeout, that is clear. Immediately after the third chunk arrives, the DMA will issue a "transfer complete" interrupt, because the DMA's counter will reach zero. In this case, the same function will process the last chunk of data, but in this case it's really important not to process the data again after the timeout. This is checked by your first quoted code:

/* Ignore IDLE Timeout when the received characters exactly filled up the DMA buffer and DMA Rx Complete IT is generated, but there is no new character during timeout */
if(dma_uart_rx.flag && currCNDTR == DMA_BUF_SIZE)
{ 
    dma_uart_rx.flag = 0;
    return;
}

Scenario 2: buffer size is 15, we have two chunks of data: first consists of 10 bytes and the second consists of another 10 bytes. The system receives the first chunk. Then the second chunk arrives. This is processed in two steps. The DMA's counter will reach zero after the 5th byte of the second chunk. Upon the transfer complete interrupt, the first part (first 5 bytes) is processed by the function. The second part (remaining 5 bytes) will be processed after the timeout. So in other words, when an "overflow" event occurs, the data is processed in two parts. That is why the issues you mentioned are not applicable, for example when the data is copied.

Scenario 3: new data arrives before the timeout of the previous packet. If the data is entirely received before the timeout, the new data will be processed together with the previous data. If an "overflow" happens in this case, please see scenario 2. If the new data is still being received when the timeout reaches zero, then the old and new data will be processed in two parts: 1) the olda data, together with the first part (until the timeout) of the new data, and 2) the remaining part of new data. This will be triggered by a second timeout, because the IDLE interrupt will set the timer again after receiving the second chunk.

Again, if you're still unsure, please download the code, flash it to a discovery board and verify these scenarios with a debugger before just saying the code does not work.