How to do UART read right?

[dg9ngf]

Hello,

I've observed a wild behaviour from my application today. It's reading UART data to find complete messages and process them. When another device on the RS-485 bus went wild and sent tons of messages for a moment (just few seconds), my poor ESP32 got seriously confused. It read most of the messages first, then had several CRC errors in the read messages for a while, and then it turned strange.

It still did notice that new data was received regularly (every ~15 seconds, going on for minutes), but it was kind of like living in the past. uart_read_bytes() gave me data that was sent minutes ago. Apparently that data was still in some buffer but not reported or returned before. Also, each time only so much data was returned as was sent at the moment, but the data was old. I could see it by the logged message counters and the data wasn't aligned to the sent messages, there were always 3 or 4 bytes left of the following message (message size was around 11 bytes at the time).

So I'm asking, how can this happen? And how can I prevent that from happening again? If there's still more data to read, I want to have that immediately, not only when newer data is sent.

Here's parts of my code, please ask if you need to see more:

Code: Select all

#define BUF_SIZE (512)
#define STACK_SIZE (4096)

static QueueHandle_t uartQueue;
static uint8_t recvData[BUF_SIZE];
static QueueHandle_t recvQueue;

void init()
{
	uart_config_t uart_config = {
		.baud_rate = RS485_BAUDRATE,
		.data_bits = UART_DATA_8_BITS,
		.parity = UART_PARITY_DISABLE,
		.stop_bits = UART_STOP_BITS_1,
		.flow_ctrl = UART_HW_FLOWCTRL_DISABLE,
	};
	// Configure UART parameters
	int rxBufferSize = BUF_SIZE * 2;
	int txBufferSize = BUF_SIZE;
	int queueSize = 20;  // should I increase this?
	int intr_alloc_flags = 0;
	ESP_ERROR_CHECK(uart_driver_install(RS485_UART_NUM, rxBufferSize, txBufferSize, queueSize, &uartQueue, intr_alloc_flags));
	ESP_ERROR_CHECK(uart_param_config(RS485_UART_NUM, &uart_config));
	ESP_ERROR_CHECK(uart_set_pin(RS485_UART_NUM, RS485TX_GPIO, RS485RX_GPIO, RS485DE_GPIO, UART_PIN_NO_CHANGE));
	ESP_ERROR_CHECK(uart_set_mode(RS485_UART_NUM, UART_MODE_RS485_HALF_DUPLEX));

	xTaskCreate(recvTaskCode, "uart-recv", STACK_SIZE, NULL, 2, &recvTaskHandle);
	configASSERT(recvTaskHandle);
}

void recvTaskCode(void* unusedParameters)
{
	uart_event_t event;
	int usedData = 0;
	uint16_t msgLength = MIN_MSG_LENGTH;

	while (true)
	{
		// Read data from the UART to the end of the buffer
		if (xQueueReceive(uartQueue, (void*)&event, (TickType_t)portMAX_DELAY))
		{
			if (event.type == UART_DATA)
			{
				uint32_t readLength = event.size;
				uint32_t maxReadLength = BUF_SIZE - 1 - usedData;
				if (readLength > maxReadLength)
					readLength = maxReadLength;

				int len = uart_read_bytes(RS485_UART_NUM, recvData + usedData, readLength, pdMS_TO_TICKS(50));
				if (len == 0)
				{
					ESP_LOGW(TAG, "No bytes read although notified by event");
					// didn't observe this ever
				}
				if (len > 0)
				{
					usedData += len;
					ESP_LOGI(TAG, "Recv %d new bytes, total %d bytes", len, usedData);
					while (usedData >= msgLength)
					{
						// Enough data to determine the message frame length, also every message is at least that long
						msgLength = 6;   // actually read from received data here
						if (usedData >= msgLength)
						{
							// A complete message was received, check CRC
							// (...)
							if (recvCrc == compCrc)
							{
								// CRC matches, process the message
								// log message incl. its counter so that I can see what's going on
								// (...)

								// Remove the message from the buffer and keep what comes after it
								for (int j = msgLength; j < usedData; j++)
								{
									recvData[j - msgLength] = recvData[j];
								}
								usedData -= msgLength;
							}
							else
							{
								// CRC error, flush receive buffer and wait for idle
								// (...)
							}

							// Reset expected number of bytes to process new messages
							msgLength = MIN_MSG_LENGTH;
						}
					}
				}
			}
			// I noticed I could also handle these events, would that help?
			else if (event.type == UART_BUFFER_FULL ||
				event.type == UART_FIFO_OVF ||
				event.type == UART_FRAME_ERR ||
				event.type == UART_PARITY_ERR)
			{
				// This code is new, I added it after observing the problem: would it help?
				usedData = 0;
				uart_flush_input(RS485_UART_NUM);

				// Reset expected number of bytes to process new messages
				msgLength = MIN_MSG_LENGTH;
			}
		}
	}
}

Essentially, I'm waiting for a UART event, and if it's a data event, I read as many bytes as indicated, but not more than the buffer size (never had to read more than the buffer size, says the log). Then it's read and all the complete messages in it are processed. If something is left, it's kept for the next data event and more read data.

I guess that waiting for the data event and only reading as much data as indicated is a problem. The data is already in the read buffer, but I'm not asking for it yet. Maybe some events got lost? Can I just skip the events and always read all the data that is there? I want uart_read_bytes() to give me all from the buffer, but not wait for more. Only if there is nothing in the buffer, I want it to wait a moment until something was received. Is that possible?

Also, I have two timeouts here: the xQueueReceive() and uart_read_bytes() calls both want a timeout. What should I pass here?

As described, I basically want all received bytes immediately but wait if nothing is available at the moment. Pretty simple.

[MicroController]

Code: Select all

if (readLength > maxReadLength)
	readLength = maxReadLength;

If this condition ever becomes true, e.g. because data was temporarily received faster than could be consumed/processed, you will read too little data from the UART's buffer (maxReadLength instead of event.size). (event.size - maxReadLength) bytes remain in the buffer until you do the next read upon the next DATA event. Your reading got out of sync with the UART events.

You may want to make sure you read exactly event.size bytes upon every DATA event (can do multiple uart_read_bytes()...), or ignore event.size and always read+process the number of bytes uart_get_buffered_data_len(...) returns (possibly 0).

[dg9ngf]

Thanks for the heads up. I'm currently restructuring my code to loop over the event.size and read more if I haven't read all reported data yet. The other approach won't help me because I still need to wait if no data has been received, and I don't want to wait in a tight loop, so I need the events for that.

[MicroController]

The other approach won't help me because ... I don't want to wait in a tight loop, so I need the events for that.

Using uart_get_buffered_data_len() in the place of event.size has nothing to do with "wait[ing] in a tight loop"; and you actually only need the event-based UART interface when&if you want to handle any of the other, non-data UART events.

[dg9ngf]

Using uart_get_buffered_data_len() in the place of event.size has nothing to do with "wait[ing] in a tight loop"

So if I call that function, I get 0 most of the time. All I can do next is call the function again. This creates a lifelock and other tasks can't run anymore. How else will I know when data has been received, without any delay? As I understood it, only the event queue provides that.

[MicroController]

Using uart_get_buffered_data_len() in the place of event.size has nothing to do with "wait[ing] in a tight loop"

So if I call that function, I get 0 most of the time.

Not if you call it inside the event handler upon a DATA event, where you'd normally read event.size. Using uart_get_buffered_data_len() provides for a little more resilience in case one or more DATA events get 'lost'/skipped.

How else will I know when data has been received, without any delay? As I understood it, only the event queue provides that.

When you don't need the non-data UART events, you just call uart_read_bytes(...) from any regular task; it will block until data has been received or it timed out.

[dg9ngf]

Not if you call it inside the event handler upon a DATA event, where you'd normally read event.size. Using uart_get_buffered_data_len() provides for a little more resilience in case one or more DATA events get 'lost'/skipped.

Ah that you mean. Understood. It would recover from a missed event, but still sleep in once.

When you don't need the non-data UART events, you just call uart_read_bytes(...) from any regular task; it will block until data has been received or it timed out.

That's the part I don't understand. The API documentation doesn't help here. The behaviour of the function remains unclear. I give it a number of bytes and a timeout. But will it a) read up to that length and give me any fragments of it as soon as possible, or will it b) wait for exactly that length and only timeout if that couldn't be accomplished, returning whatever it has received? (a) would be good, (b) would be slow to start receiving short messages (for longer timeouts) or keep the CPU busy (for shorter timeouts).

Anyway, I have changed my code to still use the event, use event.size in full (read until I have it all), and uart_flush_input() and enter the recovery state on error events. The recovery is also entered on message CRC errors or incomplete messages, then I'll wait for the bus to be idle for a moment (using a timeout for receiving from the queue) and discard everything else I read until then. In any case, no data is left unconsumed anymore. (Testing of this isn't complete yet.)

[MicroController]

b) wait for exactly that length and only timeout if that couldn't be accomplished, returning whatever it has received?

Yes, b) is how it works.

You can work around that by
1. uart_read_bytes(...) with a buffer size of 1 (and whatever timeout you need) - this will block iff no data is available
2. if 1. did not time out, i.e. it read 1 byte, call uart_get_buffered_data_len(...) to find out if and how many more bytes are available
3. uart_read_bytes(...) with a buffer size of uart_get_buffered_data_len(...) (or less) to immediately get the remaining data (won't block since we know enough data is already in the buffer).