async UART coming in broken packets

[Nathan45]

Hello,

This is probably just me not knowing what I'm doing, but I could really use some help. I am trying to set up two UARTs on the nRF52840. We have a custom board but I am starting with the DK. I have gotten both UART0 and UART1 to work simultaneously, while using RTT to verify that things were coming in. However, I need to manipulate the data coming into the RX buffers before sending it out. I'm having trouble:

-Getting UART data in large packets (the async callback seems to break up larger packets into multiple callbacks)

-Using a workqueue to effectively offload computing

-USING THE RINGBUFFER!!! I think a big part of my issue is that the head/tail is getting off or I'm not doing it right or something.

-When recieving larger test messages, such as "123456789012345678901234567890", I lose bytes or sometimes it gets outright truncated. After this, my program seems to be "off" or "shifted" as if the ringbuffer head or tail is off.

-using "uart_tx" with a string. I have converted the rx input (when it works) to a string (char array) but when I test out just sending it back with uart_tx it fails. I have tried typecasting into uint8_t* and lots of other things. I just can't get it to work.

I would also just love for someone to explain a little more about how to best receive UART data effectively. Things like best practices, etc. A code snippet would really help me.

Test setup:

nRF52840 DK plugged into my mac. There is also a uart/usb bridge plugged into P0.11 and P0.12 which I changed to UART0 in the device tree overlay. I use this to talk to the UART0 device. I am ignoring UART1 while I figure out how to accurately get messages from UART0. I am using RTT for my LOG messages, displayed on RTT-viewer or in VSC.

NCS 2.2.0

In my code I put some LOG_INF stuff. I included some screenshots of some basic tests. You can see that smaller tests from the serial terminal like "0123456789" are fine. But "01234567890123456789" will go freeze the program. This is with a uart_rx timeout of 1000 ms. I'm assuming I would want something more like 10-100ms. When I set uart_rx to 100ms it will freeze with just "0123456789" or even smaller.

main.c

/*
 * Copyright (c) 2012-2014 Wind River Systems, Inc.
 *
 * SPDX-License-Identifier: Apache-2.0
 */

#include <zephyr/kernel.h>
#include <zephyr/devicetree.h>
#include <zephyr/sys/ring_buffer.h>
#include <zephyr/drivers/gpio.h>
#include <zephyr/drivers/uart.h>
#include <zephyr/logging/log.h>
#include <string.h>

//setup the logging module
#define LOG_MODULE_NAME app 
LOG_MODULE_REGISTER(LOG_MODULE_NAME);

//THREAD SETUP
#define UART0_INTERPRETER_STACKSIZE 512
#define UART0_INTERPRETER_PRIORITY 4
#define WORKQ_PRIORITY 3

K_SEM_DEFINE(tx0_monitor, 0, 1);

K_MUTEX_DEFINE(ring_buf0);


//LED SETUP
static const struct gpio_dt_spec led0 = GPIO_DT_SPEC_GET(DT_ALIAS(led0), gpios);
static const struct gpio_dt_spec led1 = GPIO_DT_SPEC_GET(DT_ALIAS(led1), gpios);
static const struct gpio_dt_spec led2 = GPIO_DT_SPEC_GET(DT_ALIAS(led2), gpios);
//the leds can now be controlled using led0, led1, and led2.

//////

//UART SETUP
#define UART_TIMEOUT 1000
#define RECIEVE_BUFF_SIZE 64
#define MY_RING_BUF_BYTES 64

//ring buffer for moving data between the uart interrupt and the main thread
//RING_BUF_DECLARE is a macro that sets everything up automatically.
// https://docs.zephyrproject.org/3.2.0/kernel/data_structures/ring_buffers.html#api-reference
RING_BUF_DECLARE(rb0, MY_RING_BUF_BYTES);
RING_BUF_DECLARE(rb1, MY_RING_BUF_BYTES);

//device tree initialization of the uart device
const struct device *uart0 = DEVICE_DT_GET(DT_NODELABEL(uart0));
const struct device *uart1 = DEVICE_DT_GET(DT_NODELABEL(uart1));

static uint8_t rx_buf0[RECIEVE_BUFF_SIZE] = {0}; //buffer to store incoming UART data
static uint8_t rx_buf1[RECIEVE_BUFF_SIZE] = {0};

//just a normal tx buffer. Initialized with some welcome text.
static uint8_t tx_buf0[] = {"uart0\n\r\0"};
static uint8_t tx_buf1[] = {"uart1\n\r\0"};

//This structures allows for on-the-fly setting changes
const struct uart_config uart0_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
};

const struct uart_config uart1_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
};

//////

struct work_info {
	struct k_work work;
	char name[25];
} uart0_work_info, uart1_work_info;

void uart0_offload_func(struct k_work *work_tem) {
	
	//LOG_INF("IN OFFLOAD");
	int ret;
	int i = 0;
	uint8_t ret_data[64] = {0}; //this one is for the uart data
	char recv_cmd[64] = {0}; //for the recieved command, stored in a string
	uint32_t read_size = 0;
	uint8_t *ret_worker = ret_data;

	k_mutex_lock(&ring_buf0, K_FOREVER);
	read_size = ring_buf_get(&rb0, &ret_data, sizeof ret_data - 1);
	k_mutex_unlock(&ring_buf0);

	if (!read_size == 0) {
		ret_data[read_size] = '\0';

		//optional echo
		k_sem_take(&tx0_monitor, K_MSEC(5)); // only let the next uart_tx start when the last call to uart_tx is finished.
		uart_tx(uart0, ret_worker, read_size, SYS_FOREVER_US);
		//

		LOG_INF("read_size = %d", read_size);
		LOG_INF("sizeof(ret_data)=%d", sizeof(ret_data));
		for (i = 0 ; i < read_size ; i++) {
			LOG_INF("recv_cmd[%d] = %c", i, ret_worker[i]);
			recv_cmd[i] = ret_worker[i];
		}
	}
	
	LOG_INF("recv_cmd: %s", recv_cmd);

	// * DO STRING OPERATIONS HERE *

	// * SEND RESULT OVER TX *

	//k_sem_take(&tx0_monitor, K_MSEC(10));
	//int err = uart_tx(uart0, tx_worker, strlen(tx_worker), SYS_FOREVER_US); // *THIS NEVER WORKS!! OUTPUTS GARBAGE ON TERMINAL
	//LOG_INF("tx res = %d", err);
	
}


#define WORKQ_THREAD_STACK_SIZE 512
static K_THREAD_STACK_DEFINE(uart0_workq_stack_area, WORKQ_THREAD_STACK_SIZE);
static struct k_work_q uart0_work_q = {0};

// This is the callback that is used in the "asynchronous" mode of the uart
// https://academy.nordicsemi.com/topic/uart-driver/
static void uart0_cb(const struct device *dev, struct uart_event *evt, void *user_data) {
	
	static uint16_t pos;

	//When an event happens, it enters this function. then you must check which type of
	// event it was and then act accordingly. All the possible events are listed under
	// this switch statement, but you don't have to write out the ones that you don't
	// need.
	switch (evt->type) {

		case UART_TX_DONE:
			//do something
			k_sem_give(&tx0_monitor);
			break;
		
		case UART_TX_ABORTED:
			//do something
			break;
		
		case UART_RX_RDY:

			k_mutex_lock(&ring_buf0, K_FOREVER);
			int len = ring_buf_put(&rb0, (uint8_t *)(evt->data.rx.buf + evt->data.rx.offset), evt->data.rx.len);
			k_mutex_unlock(&ring_buf0);

			int err = k_work_submit_to_queue(&uart0_work_q, &uart0_work_info.work);
			LOG_INF("%d, %d, %d",len, evt->data.rx.len, err);

			//k_sem_give(&rb0_monitor);
			/*
			// if the user pressed 1, 2, or 3, turn on the respective LED
			if((evt->data.rx.len) == 1) {
				if(evt->data.rx.buf[evt->data.rx.offset] == '1')
					gpio_pin_toggle_dt(&led0);
				else if (evt->data.rx.buf[evt->data.rx.offset] == '2')
					gpio_pin_toggle_dt(&led1);
				else if (evt->data.rx.buf[evt->data.rx.offset] == '3')
					gpio_pin_toggle_dt(&led2);
			}
			*/

			break;

		case UART_RX_BUF_REQUEST:
			//do something
			break;

		case UART_RX_BUF_RELEASED:
			//do something
			break;

		case UART_RX_DISABLED:
			//do something

			//RX gets disabled after each read, so to enable continuous reception you
			//must enable the rx within this switch-case.
			// https://academy.nordicsemi.com/topic/uart-driver/
			uart_rx_enable(dev, rx_buf0, sizeof(rx_buf0), UART_TIMEOUT);
			break;

		case UART_RX_STOPPED:
			//so something
			break;

		default:
			break;

	} //end switch statement
}


static void uart1_cb(const struct device *dev, struct uart_event *evt, void *user_data) {
	const uint8_t rx[32] = {0};
	//When an event happens, it enters this function. then you must check which type of
	// event it was and then act accordingly. All the possible events are listed under
	// this switch statement, but you don't have to write out the ones that you don't
	// need.
	switch (evt->type) {

		case UART_TX_DONE:
			//do something
			// Since I'm not using this one, this is an example of one that I could leave out.
			break;
		
		case UART_TX_ABORTED:
			//do something
			break;
		
		//UART 1 data rx ready
		case UART_RX_RDY:

			//store the recieved data in the ring buffer
			if( ring_buf_put(&rb1, (evt->data.rx.buf + evt->data.rx.offset), evt->data.rx.len) < evt->data.rx.len )
    		{
        		LOG_WRN("RingBuf1 write failed");
      		}

			// if the user pressed 1, 2, or 3, turn on the respective LED
			if((evt->data.rx.len) == 1) {
				if(evt->data.rx.buf[evt->data.rx.offset] == '1')
					gpio_pin_toggle_dt(&led0);
				else if (evt->data.rx.buf[evt->data.rx.offset] == '2')
					gpio_pin_toggle_dt(&led1);
				else if (evt->data.rx.buf[evt->data.rx.offset] == '3')
					gpio_pin_toggle_dt(&led2);
			}

			// echo back the recieved data
			//uart_tx(uart, "RCVD:\r\n\0", sizeof "RCVD:\r\n\0", SYS_FOREVER_US);
			uart_tx(uart1, (const uint8_t *)(evt->data.rx.buf + evt->data.rx.offset), evt->data.rx.len, SYS_FOREVER_US);
			//strncpy(rx, (const uint8_t *)(evt->data.rx.buf + evt->data.rx.offset), sizeof(rx)-1);
			//LOG_INF("TX: %s", rx);

			break;

		case UART_RX_BUF_REQUEST:
			//do something
			break;

		case UART_RX_BUF_RELEASED:
			//do something
			break;

		case UART_RX_DISABLED:
			//do something

			//RX gets disabled after each read, so to enable continuous reception you
			//must enable the rx within this switch-case.
			// https://academy.nordicsemi.com/topic/uart-driver/
			uart_rx_enable(dev, rx_buf1, sizeof(rx_buf1), UART_TIMEOUT);
			break;

		case UART_RX_STOPPED:
			//so something
			break;

		default:
			break;

	} //end switch statement
}

///////////////



// This function sets up the LEDs mostly.
// It also makes sure the devices are ready
int setup(void) {
	if (!device_is_ready(uart0)) {
		printk("uart0 device not ready\r\n");
		return 1;
	}

	if (!device_is_ready(uart1)) {
		LOG_INF("uart1 device not ready");
		return 1;
	}

	if (!device_is_ready(led0.port)) {
		printk("An LED device is not ready");
		return 1;
	}

	int ret = gpio_pin_configure_dt(&led0, GPIO_OUTPUT_ACTIVE);
	if (ret < 0) {
		return 1;
	}

	ret = gpio_pin_configure_dt(&led1, GPIO_OUTPUT_ACTIVE);
	if (ret < 0) {
		return 1;
	}
	ret = gpio_pin_configure_dt(&led2, GPIO_OUTPUT_ACTIVE);
	if (ret < 0) {
		return 1;
	}

	return 0;
}

// This is me just trying to learn about ring buffers. Not related to anything.
void ring_buf_test(void) {
	uint8_t my_data[] = "Testing ring buffer\r\n\0";
	uint8_t ret_data[32] = {0};
	uint32_t ret;

	ret = ring_buf_put(&rb0, my_data, sizeof my_data);
	LOG_INF("ring_buf_put ret=%d", ret);
	ret = ring_buf_get(&rb0, ret_data, sizeof(my_data)-10);
	LOG_INF("mydata=%d, ret=%d", sizeof my_data, ret);
	LOG_INF("ret_data=%s", ret_data);
	ret = ring_buf_get(&rb0, ret_data[sizeof ret_data], 1);
	LOG_INF("mydata=%d, ret=%d", sizeof my_data, ret);
	LOG_INF("ret_data=%s", ret_data);
	ret = ring_buf_get(&rb0, ret_data, 1);
	LOG_INF("mydata=%d, ret=%d", sizeof my_data, ret);
	LOG_INF("ret_data=%s", ret_data);
	ret = ring_buf_get(&rb0, ret_data, 1);
	LOG_INF("mydata=%d, ret=%d", sizeof my_data, ret);
	LOG_INF("ret_data=%s", ret_data);

	ring_buf_reset(&rb0);
	ret = ring_buf_get(&rb0, ret_data, 1);
	LOG_INF("mydata=%d, ret=%d", sizeof my_data, ret);
	LOG_INF("ret_data=%s", ret_data);
}

//int test_ext = 5;






void t_uart0_interpreter() { //disabled in favor of workqueue. This was a thread that constantly looked at the ring buffer. Worked pretty well.
	int ret;
	uint8_t ret_data[4] = {0}; //this one is for the uart data
	/*
	while (1) {
		//LOG_INF("test_ext = %d", test_ext);
		ret = ring_buf_get(&rb0, &ret_data, 1);
		LOG_INF("t_ret=%s",ret_data);
		k_msleep(100);
		if (ret_data == "<") {
			LOG_INF("SOF DETECTED");
		}
	}
	*/
	while (1) {
	while (ring_buf_get(&rb0, &ret_data, 1) == 1) {
		LOG_INF("t_ret=%s",ret_data);
		//k_msleep(100);
		if (!strcmp((const char *)ret_data, "<")) {
			LOG_INF("SOF DETECTED");
		}
	}
	k_msleep(10);
	}
}

//START MAIN FUNCTION
//Connect a usb/uart bridge to the DK at the P0.11 and P0.12 and type commands
int main(void)
{
	// declare generic return values
	int err;
	int ret;
	uint8_t ret_data[64] = {0}; //this one is for the uart data
	err = setup(); //setup the GPIOs and uart device
	if (err == 1) {
		return 1;
	}

	//ENABLE/CONFIGURE UART0
	//assign the settings found in the settings structure for uart
	if (uart_configure(uart0, &uart0_cfg) == -ENOSYS) {
		return -ENOSYS;
	}

	//assign the callback function to the uart device
	err = uart_callback_set(uart0, uart0_cb, NULL); 
	if (err) {
		return err;
	}

	//enable rx, and assign a buffer
	uart_rx_enable(uart0, rx_buf0, sizeof rx_buf0, UART_TIMEOUT);


	//ENABLE/CONFIGURE UART1
	if (uart_configure(uart1, &uart1_cfg) == -ENOSYS) {
		return -ENOSYS;
	}
	err = uart_callback_set(uart1, uart1_cb, NULL); 
	if (err) {
		return err;
	}
	uart_rx_enable(uart1, rx_buf1, sizeof rx_buf1, UART_TIMEOUT);

	
	//ring_buf_test(); //just me messing around


	k_work_queue_start(&uart0_work_q, uart0_workq_stack_area,
                   K_THREAD_STACK_SIZEOF(uart0_workq_stack_area), WORKQ_PRIORITY,
                   NULL);
	strcpy(uart0_work_info.name, "Thread0 emulate_work()");
	k_work_init(&uart0_work_info.work, uart0_offload_func);

	//start the main loop.
	LOG_INF("STARTING MAIN LOOP");
	while (1) {
		LOG_INF("LOOPING"); //prints over RTT. Connect via usb and view with RTT viewer (there is one built into vscode)
		// you can use the built in RTT viewer by clicking the plug shaped icon next to "RTT" under "CONNECTED DEVICES"
		// on the side tab on the bottom left.
		err = uart_tx(uart0, tx_buf0, sizeof(tx_buf0), SYS_FOREVER_US); //prints on uart0. I set TX to P0.11 and RX to P0.12 using an overlay
		// I had to move the uart pins because P0.6 and P0.8 (the default uart0) are inherently stuck to the console on the DK???
		if (err) {
			return err;
		}
		err = uart_tx(uart1, tx_buf1, sizeof(tx_buf1), SYS_FOREVER_US); // uart1, TX RX set to P1.01 and P1.02
		if (err) {
			return err;
		}

		k_msleep(30000); //sleep for 30 second
	}

}

//K_THREAD_DEFINE(t_uart0_interpreter_id, UART0_INTERPRETER_STACKSIZE, t_uart0_interpreter, NULL, NULL, NULL, UART0_INTERPRETER_PRIORITY, 0, 0);

prj.conf

# nothing here
CONFIG_GPIO=y
CONFIG_SERIAL=y
CONFIG_RING_BUFFER=y


CONFIG_UART_CONSOLE=n
#CONFIG_CONSOLE=n
CONFIG_LOG=y
CONFIG_LOG_BACKEND_RTT=y
#CONFIG_RTT_CONSOLE=y

CONFIG_UART_ASYNC_API=y
CONFIG_BUILD_OUTPUT_EXE=y

nrf52840dk_nrf52840.overlay

/ {
    pin-controller {
        uart0_default {
            group1 {
                psels = <NRF_PSEL(UART_TX, 0, 11)>,
                        <NRF_PSEL(UART_RTS, 0, 5)>;
            };

            group2 {
                psels = <NRF_PSEL(UART_RX, 0, 12)>,
                        <NRF_PSEL(UART_CTS, 0, 7)>;
                bias-pull-up;
            };
        };
    };
};

[Nathan45]

For anyone with a similar issue, I found that working through the linked github example made UART work a LOT better and helped me to understand it better. In a nutshell:

- Double buffering is important! don't leave it out!

- You don't have to understand double buffering to implement it, as shown in the linked example it's just a single line of code in the UART callback, and a few lines of setting up the buffer at the top of the file.

- Message queues are important too! Those are worth understanding a bit more. I don't think they are required, but they work really well in tandem with async UART, and thats what the linked example uses.

- The linked example prints everything right away, but if you want the data in memory so you can perform operations on it you can start a thread that checks the message queue for new messages using the "k_msgq_get(...)" function. If this function returns true, it means that there is uart data ready to save to memory and you just save it and repeat basically. I added a basic snippet that I use (I modified the snippet from what we exactly use for intellectual property reasons, code may/maynot be broken but it should at least work as a starting point).

https://github.com/too1/ncs-uart-async-count-rx

#define UART0_INTERPRETER_STACKSIZE 512 //thread stuff
#define UART0_INTERPRETER_PRIORITY 7    //thread stuff

#define UART_BUF_SIZE 64

/ .
/ .
/ .

void uart_printer() {
	printk("STARTING UART GRABBER THREAD"); 
	app_uart_send("START\r\n", strlen("START\r\n"));
	int data_length;
	struct uart_msg_queue_item incoming_message;

	while (1) {
		//app_uart_send("C", 1);
		// This function will not return until a new message is ready
		if (k_msgq_get(&uart_rx_msgq, &incoming_message, K_NO_WAIT) == 0) {
			printk("message detected\n");
			uint8_t string_buffer[UART_BUF_SIZE] = "0";
			uint8_t read_value[UART_BUF_SIZE] = "0";
			uint8_t new_buffer[UART_BUF_SIZE] = "0";
			data_length = 0;

			memcpy(string_buffer, incoming_message.bytes, incoming_message.length);
			data_length += incoming_message.length;
			string_buffer[data_length] = '\0';
			// Read remaining bytes after initial read is started. append them to final string
			while (k_msgq_get(&uart_rx_msgq, &incoming_message, K_MSEC(50)) == 0) { //K_MSEC(x) time value = uart timeout sort of
				memcpy(new_buffer, incoming_message.bytes, incoming_message.length);
				//memcpy(new_buffer, "x", 1);
				data_length += incoming_message.length;
				strcat(string_buffer, new_buffer);
				string_buffer[data_length] = '\0';
			}
			printk("message grabbed\n");
			
			strncpy(read_value, string_buffer, strlen(string_buffer));
			//LOG_INF("message = %s", read_value);
			// HOORAY!! read value is now store in memory as read_value. Also as string_buffer.
			/////////////// PROCESS/HANDLE DATA BELOW //////////////////////
			
			/ .
			/ .
			/ .

			////////////////////////////////////////////////////////////////////////
		}
		k_msleep(10); //give other threads time
	}
}

/ .
/ .
/ .

K_THREAD_DEFINE(t_uart0_interpreter_id, UART0_INTERPRETER_STACKSIZE, uart_printer, NULL, NULL, NULL, UART0_INTERPRETER_PRIORITY, 0, 0);

[Mike Austin (LPI)]

Nathan45 - awesome! This is exactly what I've been trying to understand how to do, having stumbled across the same Github example.  You've saved me heaps of time/hassle.

Not sure I fully understand what I'm doing, but I've got something working and so now I can play around with it and make sure I do

Cheers,

Mike

[Nathan45]

Glad I could help! 

God bless,

Nathan