MaJerle / stm32-usart-uart-dma-rx-tx

STM32 examples for USART using DMA for efficient RX and TX transmission
MIT License
1.3k stars 322 forks source link

Keeping the old_po(sition) as static to function creates problems for resetting #25

Closed jnz86 closed 1 year ago

jnz86 commented 1 year ago

During the rx ring buffer write from dma source exchange, you are tracking old_pos as a static inside the rx_check().

If you want to reset the connection because of error, reinit, sleep/wake, whatever. This isn't great and there is no good way to do it. The obvious solution for me was to make this static outside of the function. Now at least I can reset it if I do something like change baud where I have to reset the DMA anyway.

I am aware this is not meant to be production ready code, but I figured it was worth discussing because the second you do want to use code like this, the issue pops up,

void
usart_rx_check(void) {
    static size_t old_pos;
    size_t pos;

    /* Calculate current position in buffer and check for new data available */
    pos = ARRAY_LEN(usart_rx_dma_buffer) - LL_DMA_GetDataLength(DMA1, LL_DMA_STREAM_0);
    if (pos != old_pos) {                       /* Check change in received data */
        if (pos > old_pos) {                    /* Current position is over previous one */
            /*
             * Processing is done in "linear" mode.

Also, I am aware that static variables are initialized by C standard, but I find when it's something like this that could cause difficult bugs if not set to zero at start, I typically set the equal to zero anyhow, a set your mind at ease thing. At least a comment in code or documentation would be helpful for those that didn't know https://stackoverflow.com/questions/3373108/why-are-static-variables-auto-initialized-to-zero

MaJerle commented 1 year ago

I do not disagree that static could be replaced by static global (module global) variant instead. Code is an example, indeed.

Concerning static vars being set to 0, even these may not be if linker has been modified by user.

jnz86 commented 1 year ago

Well, that seems like a good enough reason to explicitly set it = 0 then!

I had to add a little error handling (power off / unexpected reset of an attached module) but otherwise, I thought this code was very nice and am glad I found it.

MaJerle commented 1 year ago

Even = 0 at start is a work done by linker. So not even sure this will sustain, if you modify linker your own way.

The only correct way is to set the var = 0 in the init or some function, that only executes once.

jnz86 commented 1 year ago

Correction... and a change I just made. Executes once - per re-init of the DMA. That was the issue with it being static to the function, not file.

void rb_uart3_init(void)
{
    lwrb_init(&rb_uart3_rx, rb_data_uart3_rx, sizeof(rb_data_uart3_rx));
    lwrb_init(&rb_uart3_tx, rb_data_uart3_tx, sizeof(rb_data_uart3_tx));

    uart3_old_pos = 0;

    LL_USART_Disable(USART3);

    LL_DMA_SetPeriphAddress(DMA1, LL_DMA_STREAM_2,
                            LL_USART_DMA_GetRegAddr(USART3, LL_USART_DMA_REG_DATA_RECEIVE));
    LL_DMA_SetMemoryAddress(DMA1, LL_DMA_STREAM_2, (uint32_t) uart3_data_dma_rx);
    LL_DMA_SetDataLength(DMA1, LL_DMA_STREAM_2, ARRAY_LEN(uart3_data_dma_rx));
    LL_DMA_SetPeriphAddress(DMA1, LL_DMA_STREAM_3,
                            LL_USART_DMA_GetRegAddr(USART3, LL_USART_DMA_REG_DATA_TRANSMIT));
...