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

UART HW DMA ( UART Communication based on HW DMA ) - Buffer Overflow in STM32H743 Controller #76801

Open
adityamilinddesai-eaton opened this issue Aug 7, 2024 · 8 comments
Assignees
Labels
bug The issue is a bug, or the PR is fixing a bug platform: STM32 ST Micro STM32 priority: low Low impact/importance bug Stale Waiting for response Waiting for author's response

Comments

@adityamilinddesai-eaton
Copy link

We are using zephyr V3.6.0 and SDK version 0.16.5-1.
We are written code for Uart HW DMA for STM32H743 controller.
Assuming a buffer of 256 Bytes, A message sent with 257 bytes will cause an overflow event to be generated.
Example:
Sent Message: "Call me Ishmael. Some years ago never mind how long precisely having little or no money in my purse, and nothing particular to interest me on shore, I thought I would sail about a little and see the watery part of the world. It is a way I have of drivingEO"
Echoed Message: "Call me Ishmael. Some years ago never mind how long precisely having little or no money in my purse, and nothing particular to interest me on shore, I thought I would sail about a little and see the watery part of the world. It is a way I have of drivingE"

This event is serviced in our driver as well as the uart_stm32 driver. The next message sent by the client will be echoed with the overflow byte (O from #1 Sent Message) added to the beginning as seen here:

Sent Message: "Call me Ishmael. Some years ago never mind how long precisely having little or no money in my purse, and nothing particular to interest me on shore, I thought I would sail about a little and see the watery part of the world. It is a way I have of drivingE"

Echoed Message: "OCall me Ishmael. Some years ago never mind how long precisely having little or no money in my purse, and nothing particular to interest me on shore, I thought I would sail about a little and see the watery part of the world. It is a way I have of driving"

#Notice that the 'E' character has caused another overflow. If the user/client uses the entire buffer for every expected message, and has at least one overflow, this could cause some confusion.
How to avoid the carry over byte.

@adityamilinddesai-eaton adityamilinddesai-eaton added the bug The issue is a bug, or the PR is fixing a bug label Aug 7, 2024
Copy link

github-actions bot commented Aug 7, 2024

Hi @adityamilinddesai-eaton! We appreciate you submitting your first issue for our open-source project. 🌟

Even though I'm a bot, I can assure you that the whole community is genuinely grateful for your time and effort. 🤖💙

@bhardadinesh-eaton
Copy link

bhardadinesh-eaton commented Aug 7, 2024

See the below snippet for more understandings.
image

@manoj153
Copy link
Contributor

manoj153 commented Aug 7, 2024

hey sharing a code snippet will help a lot to comment.

@nordicjm nordicjm added the platform: STM32 ST Micro STM32 label Aug 8, 2024
@erwango erwango added the priority: low Low impact/importance bug label Aug 8, 2024
@erwango erwango assigned djiatsaf-st and unassigned erwango Aug 8, 2024
@adityamilinddesai-eaton
Copy link
Author

adityamilinddesai-eaton commented Aug 8, 2024

void Echo_UART_Test_Cback( cback_param_t param, cback_status_t
								  status_bits )
{
	if ( !Test_Bit( status_bits, RX_ERROR ) )
	{
		if ( Test_Bit( status_bits, RX_BYTE_TIMEOUT ) )
		{
			if ( usart_hw_dma_handle->Get_RX_Length() != 0 )
			{
				printk("%s", usart_buff);
			}
			else
			{
				printk("Zero Buffer\n\r");
			}
		}
		if ( Test_Bit( status_bits, RX_BUFF_FULL ) )
		{
			printk("Overflow Buffer\n\r" );
		}
		if ( Test_Bit( status_bits, TX_COMPLETE ) )
		{
			start_rx();
		}	
	}
	else
	{	
		stop_rx();
		printk("RX Error\n\r" );		
	}
}

void start_rx(void)
{
	uart_rx_disable( m_device );
	uart_rx_enable( m_device, data, length, timeout );
}	

void stop_rx(void)
{
	uart_rx_disable( m_device );
}
		
int main()
{
	const struct device* m_device = device_get_binding(UART_DEVICE_NAME);
	
	stop_rx();
	
	m_uart_cfg.baudrate = 115200;
	m_uart_cfg.flow_ctrl = UART_CFG_FLOW_CTRL_NONE;
	m_uart_cfg.parity = UART_CFG_PARITY_NONE;
	m_uart_cfg.data_bits = UART_CFG_DATA_BITS_8;
	m_uart_cfg.stop_bits = UART_CFG_STOP_BITS_1;
	uart_configure( m_device, &m_uart_cfg );
	
	start_rx();
	
	while(1);
}

@djiatsaf-st
Copy link
Collaborator

Hi @adityamilinddesai-eaton,
I looked on my side, I will suggest some ways to avoid the extra byte in the message.

First, when we have a buffer overflow, there are three events that occur after reception. First, the buffer is disabled (uart event type = UART_RX_BUF_RELEASED), then the communication is disabled (uart event type = UART_RX_DISABLED), and then there is a request made by the driver to have a second buffer (uart event type = UART_RX_BUF_REQUEST).

Then it is recommended in zephyr when we make a UART communication, to make a copy of the buffer to perform future processing there if necessary.

To avoid the carry over the byte, we can set up a flag that will notify us when receiving the next message that there was an overflow during the previous one. Then with the main buffer copy we could remove the extra byte in the message and display the result.

Here is an example : 

#include <zephyr/kernel.h>
#include <zephyr/device.h>
#include <zephyr/devicetree.h>
#include <zephyr/sys/printk.h>
#include <zephyr/drivers/uart.h>


/*Define the size of the receive buffer */
#define RECEIVE_BUFF_SIZE 10

/* Define the receiving timeout period */
#define RECEIVE_TIMEOUT 1000

/* Get uart/usart device */
const struct device *uart= DEVICE_DT_GET(DT_NODELABEL(usart3));


const struct uart_config uart_cfg = {
		.baudrate = 115200,
		.parity = UART_CFG_PARITY_NONE,
		.stop_bits = UART_CFG_STOP_BITS_1,
		.data_bits = UART_CFG_DATA_BITS_8,
		.flow_ctrl = UART_CFG_FLOW_CTRL_NONE
	};


/* Define the receive buffer */
static uint8_t rx_buf[RECEIVE_BUFF_SIZE] = {0};

/* Define the copy of receive buffer: the size of buffer can be more than */
uint8_t rx_copy_buffer[RECEIVE_BUFF_SIZE] = {0}; //initialization


/* copy value of an array into another */
void copyArray(uint8_t *src, uint8_t *dest, uint8_t offset, uint8_t size) {
    for (int i = offset; i < size; i++) {
        *(dest + i) = *(src + i); 
    }
}

/* check if buffer is empty or not */
int isArrayEmpty(uint8_t *array, int size) {
    for (int i = 0; i < size; i++) {
        if (array[i] != 0) {
            return 0; // Array is not empty
        }
    }
    return 1; // Array is empty
}

/* flag for buffer overflow */
bool overflow = false ; 

/* callback function */
static void uart_cb(const struct device *dev, struct uart_event *evt, void *user_data)
{
	
 
    switch (evt->type) {
    
            
	case UART_RX_RDY:

		    printk("### main rx_buffer ### : %s\r\n", evt->data.rx.buf);

		    int empty = isArrayEmpty(rx_copy_buffer, RECEIVE_BUFF_SIZE) ; 
		 
		    /* copy the main buffer */
		    copyArray(evt->data.rx.buf, rx_copy_buffer , evt->data.rx.offset, evt->data.rx.offset + evt->data.rx.len); 
		    
		    /* if we have overflow in the previous step and a buffer isn't empty */
		    if(overflow && !empty){

		        printk("<-- MSG WITHOUT CARRY BYTE --> : \t"); 
		        for (int j = evt->data.rx.offset + 1; j < evt->data.rx.offset + evt->data.rx.len; j++) {

		                printk("%c", rx_copy_buffer[j]); // print buffer without overflow character
		            }
		        printk("\n");

		        overflow = false ; // disable flag

		    }
		    else {
		            
		            for (int j = evt->data.rx.offset; j < evt->data.rx.offset + evt->data.rx.len; j++) {

		                printk("%c", rx_copy_buffer[j]); // 
		            }
		            printk("\n");
		    }


	     break;
        case UART_RX_DISABLED:
	
                printk("===== UART RX DISABLED: renabled ======\r\n");
                uart_rx_enable(dev ,rx_buf,sizeof rx_buf,RECEIVE_TIMEOUT); // continuous reception
       
             break;
        case UART_RX_BUF_RELEASED:
    
        	printk("===== Buffer is no longer used by UART driver. ======\r\n");

             break;		
       case UART_RX_BUF_REQUEST:
		printk(" /!\\ buffer overflow /!\\ \n");
		overflow = true ; //raise flag


	default:
	     break;
	}
}


int main(void)
{
		

    int ret;
	
	/* check if uart device is ready */
	if (!device_is_ready(uart)){
		return 1 ;
	}


    uart_rx_disable(uart);
    
    /* setup uart communication */
    int err = uart_configure(uart, &uart_cfg);

    if (err < 0) {

	   return 1 ;
	}

    /* Register the UART callback function */
    ret = uart_callback_set(uart, uart_cb, NULL);

    if (ret < 0) {

        return 1;
    }


    /* Start receiving by calling uart_rx_enable() and pass it the address of the receive  buffer */
    ret = uart_rx_enable(uart ,rx_buf,sizeof rx_buf,RECEIVE_TIMEOUT);


    if (ret) {
   
        return 1;
    }



        while (1);

}

Nota: This code only handles the error case mentioned when there is a buffer overflow

@djiatsaf-st
Copy link
Collaborator

djiatsaf-st commented Aug 16, 2024

I haven't looked much into this, but in zephyr there is a special mechanism to continuously receive data in case there is an overflow, that could be a solution to the problem too.

Here is the link :

https://docs.zephyrproject.org/latest/doxygen/html/group__uart__async.html#ggaf0c9513cbafa36d86b4c83f2b6a03dcda0d250f372702526f1bce6d4dfe166abe

we will have a configuration a bit like this:

case UART_RX_BUF_REQUEST:
uart_rx_buf_rsp(dev, rx_second_buffer,sizeof(rx_second_buffer));
break;

The global array rx_second_buffer will contain the data history for the defined size.

Finally, I saw that you use the device_get_binding() function, unless your application requires it,

Since version 3.2, zephyr suggests that we use the DEVICE_DT_GET() function instead as mentioned in the release note

https://docs.zephyrproject.org/latest/releases/release-notes-3.2.html#devicetree:~:text=As%20shown%20in,to%20do%20so

@FRASTM
Copy link
Collaborator

FRASTM commented Sep 18, 2024

@adityamilinddesai-eaton Does this proposal help ?

@FRASTM FRASTM added the Waiting for response Waiting for author's response label Oct 7, 2024
Copy link

github-actions bot commented Dec 7, 2024

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

@github-actions github-actions bot added the Stale label Dec 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The issue is a bug, or the PR is fixing a bug platform: STM32 ST Micro STM32 priority: low Low impact/importance bug Stale Waiting for response Waiting for author's response
Projects
None yet
Development

No branches or pull requests

7 participants