UART RX missing bytes -event buffer length and offset mismatch-NRF Connect SDK

[Midhunjac]

Hi, greetings all !!

I am developing an application based on ble_uart_peripheral example on nrf52dk_nrf52832 using NRF Connect SDK2.1. In my application, I have to receive bytes from an external client via UART. The problem is, at high baud rate, some of my bytes get missed. For example, while debugging, the evt->data.rx.offset shows 4 but evt->data.rx.len shows 2. So, when i try to read the buffer, based on offset value, I miss bytes. HW flow control is not an option for me since that would involve changes to my client as well. Is there anything else I can do to get this working ? I thought it might be an overrun case, but that was not the error cause.

Any ideas ?

[Jørgen Holmefjord]

Hi,

Do you expect offset and len to always be the same? As described in uart_event_rx struct documentation: "The data represented by the event is stored in rx.buf[rx.offset] to rx.buf[rx.offset+rx.len]. That is, the length is relative to the offset." Is this how you read out data from the buffer in the event?

Best regards,
Jørgen

[Jørgen Holmefjord]

Do I read your code correctly that you will only assign one byte to data_t? If the CPU is blocked by other higher priority tasks to provide the UART RX event immediately when the data is received, the event can contain multiple bytes (given by the len struct member variable). You need to loop through the buffer from offset to offset+len to make sure you get all received bytes.

[Jørgen Holmefjord]

This is not the expected way that the async UART should work, but you can try to set CONFIG_BT_NUS_UART_BUFFER_SIZE=1, as this is what determines the buffer size used in the sample. Note that this may cause overrun errors if the CPU is blocked from providing a new buffer to the UARTE peripheral before ~4 bytes are received. I would be much more safe to have a larger buffer size and iterate through the received data when you get the events.

[Midhunjac]

I want to get data byte by byte for my purpose. what would be a better method to do that ? please excuse my ignorance.

[Jørgen Holmefjord]

Did you try setting the config I suggested in my last post?

[Midhunjac]

I am sorry I did not mention about that. Yes, I tried but that didnt work. When I added that in the config environment, the device stopped receiving at all. No callbacks at all for receiving.

[Jørgen Holmefjord]

Ah, I see. The config is used for both the RX and TX buffers, and the buffer will be too small for the TX strings used in the example, causing an error.

You can modify the example to use separate TX and RX buffer sizes, as shown in this patch: 

Fullscreen main_changes.diff Download

diff --git a/samples/bluetooth/peripheral_uart/src/main.c b/samples/bluetooth/peripheral_uart/src/main.c
index d8afb44ca..bb93b71f4 100644
--- a/samples/bluetooth/peripheral_uart/src/main.c
+++ b/samples/bluetooth/peripheral_uart/src/main.c
@@ -62,6 +62,12 @@ static struct bt_conn *auth_conn;
 static const struct device *uart = DEVICE_DT_GET(DT_CHOSEN(nordic_nus_uart));
 static struct k_work_delayable uart_work;
 
+struct uart_data_rx_t {
+	void *fifo_reserved;
+	uint8_t data[1];
+	uint16_t len;
+};
+
 struct uart_data_t {
 	void *fifo_reserved;
 	uint8_t data[UART_BUF_SIZE];
@@ -92,6 +98,7 @@ static void uart_cb(const struct device *dev, struct uart_event *evt, void *user
 
 	static size_t aborted_len;
 	struct uart_data_t *buf;
+	struct uart_data_rx_t *buf_rx;
 	static uint8_t *aborted_buf;
 	static bool disable_req;
 
@@ -128,15 +135,16 @@ static void uart_cb(const struct device *dev, struct uart_event *evt, void *user
 
 	case UART_RX_RDY:
 		LOG_DBG("UART_RX_RDY");
-		buf = CONTAINER_OF(evt->data.rx.buf, struct uart_data_t, data);
-		buf->len += evt->data.rx.len;
+		buf_rx = CONTAINER_OF(evt->data.rx.buf, struct uart_data_rx_t, data);
+		buf_rx->len += evt->data.rx.len;
+		LOG_INF("UART RX: offset: %d, len: %d", evt->data.rx.offset, evt->data.rx.len);
 
 		if (disable_req) {
 			return;
 		}
 
-		if ((evt->data.rx.buf[buf->len - 1] == '\n') ||
-		    (evt->data.rx.buf[buf->len - 1] == '\r')) {
+		if ((evt->data.rx.buf[buf_rx->len - 1] == '\n') ||
+		    (evt->data.rx.buf[buf_rx->len - 1] == '\r')) {
 			disable_req = true;
 			uart_rx_disable(uart);
 		}
@@ -147,26 +155,26 @@ static void uart_cb(const struct device *dev, struct uart_event *evt, void *user
 		LOG_DBG("UART_RX_DISABLED");
 		disable_req = false;
 
-		buf = k_malloc(sizeof(*buf));
-		if (buf) {
-			buf->len = 0;
+		buf_rx = k_malloc(sizeof(*buf));
+		if (buf_rx) {
+			buf_rx->len = 0;
 		} else {
 			LOG_WRN("Not able to allocate UART receive buffer");
 			k_work_reschedule(&uart_work, UART_WAIT_FOR_BUF_DELAY);
 			return;
 		}
 
-		uart_rx_enable(uart, buf->data, sizeof(buf->data),
+		uart_rx_enable(uart, buf_rx->data, sizeof(buf_rx->data),
 			       UART_WAIT_FOR_RX);
 
 		break;
 
 	case UART_RX_BUF_REQUEST:
 		LOG_DBG("UART_RX_BUF_REQUEST");
-		buf = k_malloc(sizeof(*buf));
-		if (buf) {
-			buf->len = 0;
-			uart_rx_buf_rsp(uart, buf->data, sizeof(buf->data));
+		buf_rx = k_malloc(sizeof(*buf_rx));
+		if (buf_rx) {
+			buf_rx->len = 0;
+			uart_rx_buf_rsp(uart, buf_rx->data, sizeof(buf_rx->data));
 		} else {
 			LOG_WRN("Not able to allocate UART receive buffer");
 		}
@@ -175,13 +183,13 @@ static void uart_cb(const struct device *dev, struct uart_event *evt, void *user
 
 	case UART_RX_BUF_RELEASED:
 		LOG_DBG("UART_RX_BUF_RELEASED");
-		buf = CONTAINER_OF(evt->data.rx_buf.buf, struct uart_data_t,
+		buf_rx = CONTAINER_OF(evt->data.rx_buf.buf, struct uart_data_rx_t,
 				   data);
 
-		if (buf->len > 0) {
-			k_fifo_put(&fifo_uart_rx_data, buf);
+		if (buf_rx->len > 0) {
+			k_fifo_put(&fifo_uart_rx_data, buf_rx);
 		} else {
-			k_free(buf);
+			k_free(buf_rx);
 		}
 
 		break;
@@ -208,18 +216,18 @@ static void uart_cb(const struct device *dev, struct uart_event *evt, void *user
 
 static void uart_work_handler(struct k_work *item)
 {
-	struct uart_data_t *buf;
+	struct uart_data_rx_t *buf_rx;
 
-	buf = k_malloc(sizeof(*buf));
-	if (buf) {
-		buf->len = 0;
+	buf_rx = k_malloc(sizeof(*buf_rx));
+	if (buf_rx) {
+		buf_rx->len = 0;
 	} else {
 		LOG_WRN("Not able to allocate UART receive buffer");
 		k_work_reschedule(&uart_work, UART_WAIT_FOR_BUF_DELAY);
 		return;
 	}
 
-	uart_rx_enable(uart, buf->data, sizeof(buf->data), UART_WAIT_FOR_RX);
+	uart_rx_enable(uart, buf_rx->data, sizeof(buf_rx->data), UART_WAIT_FOR_RX);
 }
 
 static bool uart_test_async_api(const struct device *dev)
@@ -234,7 +242,7 @@ static int uart_init(void)
 {
 	int err;
 	int pos;
-	struct uart_data_t *rx;
+	struct uart_data_rx_t *rx;
 	struct uart_data_t *tx;
 
 	if (!device_is_ready(uart)) {

Note that the peripheral_uart application gave lots of warnings about not being able to allocate UART receive buffer when receiving much data, so this approach may lead to data loss if you are not careful about the baudrate and frequency of incoming data. I would advise against using this approach. Processing the received bytes according to the offset and len parameters and/or pushing the received data into a queue for later processing would be a much safer approach.

[Midhunjac]

Yes. The issue was caused due to the shared buffer. So, i tried using seperate buffers. But buffer size of 1 wont be enough as I observed it caused kernel panic. I believe the first 2 locations are used for some OS level purposes. Anyway, I got it working with size 3. But as you mentioned, this might not be a good solution.

Jørgen Holmefjord said:

Processing the received bytes according to the offset and len parameters and/or pushing the received data into a queue for later processing would be a much safer approach.

That was what i tried to do initially, but without knowing the exact length and offset, that would be difficult.  Can you guide me with a code snippet ?

[Jørgen Holmefjord]

Midhunjac said:

That was what i tried to do initially, but without knowing the exact length and offset, that would be difficult.  Can you guide me with a code snippet ?

The offset and length are given in the event. Based on your code, you can process it directly like this:

case UART_RX_RDY:
	LOG_DBG("UART_RX_RDY");
	buf = CONTAINER_OF(evt->data.rx.buf, struct uart_data_t, data);
	buf->len += evt->data.rx.len;
	if (disable_req) {
		return;
	}
	uint8_t data_t;
	for (uint8_t i; i < evt->data.rx.len; i++)
	{
	    data_t = *(buf->data+evt->data.rx.offset+i);
        packetDecode(data_t,eSrc_UART);
	}
    if ((evt->data.rx.buf[buf->len - 1] == '\n') ||
        (evt->data.rx.buf[buf->len - 1] == '\r')) 
    {
    	disable_req = true;
    	uart_rx_disable(uart);
    }
	
	break;

You can also define a Queue or a FIFO where you push each received byte, to be processed later.