Is uart_write_bytes and read thread safe?

[zliudr]

I have two tasks each handling a separate UART, 1 and 2. I had no issues when I had only one task reading a UART. Now CPU panicks and program is stuck.

[ESP_houwenxiang]

Hi, the read API is thread safe.
Is UART1 and UART2 running at a high baud rate at the same time? So, can you try to Initialize UART1 a UART2 on different core? So their ISR handle will be registered on different cores.

thanks !!

[username]

I too am monitoring 2 UARTS. Both of which are on the same core and I have not had that issue. Maybe it's the way your implementing you code that is causing the problem.

[zliudr]

Strange. I posted a reply and it didn't appear here.
I have two tasks, 9600 baud rate. The usb_host_task reads messages from UART1. It buffers it until it receives the end of message character \r\n then it sets a variable to true and waits for it to read false. Here it is:

Code: Select all

void usb_host_task()
{
    /* Configure parameters of an UART driver,
     * communication pins and install the driver */
    uart_config_t uart_config = {
        .baud_rate = USB_HOST_BAUD,
        .data_bits = UART_DATA_8_BITS,
        .parity    = UART_PARITY_DISABLE,
        .stop_bits = UART_STOP_BITS_1,
        .flow_ctrl = UART_HW_FLOWCTRL_DISABLE
    };
    uart_param_config(UART_NUM_1, &uart_config);
    uart_set_pin(UART_NUM_1, USB_HOST_TXD, USB_HOST_RXD, USB_HOST_RTS, USB_HOST_CTS);
    uart_driver_install(UART_NUM_1, UART_BUF_SIZE * 2, 0, 0, NULL, 0);
	uart_flush(UART_NUM_1); // Flush anything
    // Configure a temporary buffer for the incoming data
    uint8_t *data = usb_host_rx;
	uint16_t old_len=0;
    while (1) {
		if (barcode_received)
		{
			vTaskDelay(10 / portTICK_PERIOD_MS);
			continue;
		}
        // Read data from the UART
        int len = uart_read_bytes(UART_NUM_1, data+old_len, UART_BUF_SIZE, 1 / portTICK_RATE_MS);
		if (len==-1) continue; // error
		data[old_len+len]='\0'; // Terminate C-string
		old_len+=len;
		if ((data[old_len-2]=='\r')&&(data[old_len-1]=='\n'))
		{
			data[old_len-2]='\0'; // Discard \r\n
			barcode_received=1;
			old_len=0;
		}
		vTaskDelay(10 / portTICK_PERIOD_MS);
    }
}

The above code tests correctly with a main_app routine that reads the flag variable barcode_received and prints the buffered message to console. This feature has since been moved to the second task.

The second task is on UART2. It initially sends a message to UART2 to set mode of the receiver. It then reads the flag barcode_received and if true, takes the whole message and sends it to UART2, with the addition of an 'x' character.

Code: Select all

void device_task()
{
	uint8_t emulator_response[32]="\0";
	uint64_t last_code_sent_us=0;  // Time last barcode was sent to emulator in us.
	const uint64_t delay_between_code_us = 100000L; // Delay in milliseconds between barcode sent to emulator.
	//emulator_setup(); // Set up uart2
    /* Configure parameters of an UART driver,
     * communication pins and install the driver */
    uart_config_t uart_config = {
        .baud_rate = EMULATOR_BAUD,
        .data_bits = UART_DATA_8_BITS,
        .parity    = UART_PARITY_DISABLE,
        .stop_bits = UART_STOP_BITS_1,
        .flow_ctrl = UART_HW_FLOWCTRL_DISABLE
    };
    uart_param_config(UART_NUM_2, &uart_config);
    uart_set_pin(UART_NUM_2, EMULATOR_TXD, EMULATOR_RXD, EMULATOR_RTS, EMULATOR_CTS);
    uart_driver_install(UART_NUM_2, UART_BUF_SIZE, UART_BUF_SIZE, 0, NULL, 0);
	uart_flush(UART_NUM_2); // Flush anything
	//uart_write_bytes(UART_NUM_2, "z00x", 4); // Set to mode 0
	uint64_t a_timer=esp_timer_get_time();
	while(esp_timer_get_time()-a_timer<200000L) 
	{
		vTaskDelay(2 / portTICK_PERIOD_MS);
	}
	
	int len;
	//len=uart_read_bytes(UART_NUM_2, emulator_response, 31, 1 / portTICK_RATE_MS);
	//emulator_response[len]='\0'; // Terminate the response
	//ESP_LOGI("Dev_task", "%s\r\n",emulator_response);
	while(1)
	{
		if(barcode_received)
		{
			printf("Barcode:%s\r\n",usb_host_rx);
			process_code(usb_host_rx);
			printf("%llu\r\n", esp_timer_get_time());
			while(esp_timer_get_time()-last_code_sent_us<delay_between_code_us)
			{
				vTaskDelay(2 / portTICK_PERIOD_MS);
			}
			uart_write_bytes(UART_NUM_2, (const char *) usb_host_rx, (int) strlen((const char*)usb_host_rx));
			uart_write_bytes(UART_NUM_2, "x",1);
			a_timer=esp_timer_get_time();
			while(esp_timer_get_time()-a_timer<200000L) 
			{
				vTaskDelay(2 / portTICK_PERIOD_MS);
			}
			len=uart_read_bytes(UART_NUM_2, emulator_response, 31, 1 / portTICK_RATE_MS);
			emulator_response[len]='\0'; // Terminate the response
			printf("%s\r\n",emulator_response);
			barcode_received=0;
			last_code_sent_us=esp_timer_get_time();
		}
		vTaskDelay(50 / portTICK_PERIOD_MS);
	}
	
}

Here are main_app calls to set up the tasks:

Code: Select all

	usb_host_rx=(uint8_t *) malloc(UART_BUF_SIZE);
	xTaskCreate(usb_host_task, "usb_host_task", 1024, NULL, 10, NULL);
	xTaskCreate(device_task, "device_task", 1024, NULL, 10, NULL);

My uart buf_size is 1024. Is my task stack too shallow? I thought without using a certain keyword (__task__ or something) I am just using the main stack but that could be a bad assumption. Did I set up my tasks improperly?

What I also consider a weak link is the variable barcode_received being read by both tasks. Is there a proper way to set/clear such flag between tasks? Thank you!

[username]

Personally, i would not touch anything serial with a stack of 1024. start with 2048 or 4096, you can always dial that in later and you get things working. There is a function that you can call to let you know to stack status.
did you declare you barcode_received as a volatile variable ?
I would suggest you not use barcode_received the way you are and instead look into xEventGroupWaitBits

[zliudr]

Thanks username! I'll increase my stack size. I've combined the two tasks into one. I think that tasks make me think lazy thoughts of only writing one ting that works in its lonesome. Here is my combined task:

Code: Select all

void device_task()
{
    uint8_t *data = usb_host_rx;
	uint16_t old_len=0;
	const uint64_t delay_between_code_ms = 100; // Delay in milliseconds between barcode sent to emulator.
	char emulator_response[emulator_response_max_len+1]="\0";
	//uint64_t last_code_sent_us=0;  // Time last barcode was sent to emulator in us.
	//uint64_t a_timer=esp_timer_get_time();
	int len=0; // Length of received UART bytes
	int ret_code=0; // Collect return code from function calls.
	// Set up phase
    // Configure parameters of an UART driver, communication pins and install the driver
    uart_config_t uart1_config = {
        .baud_rate = USB_HOST_BAUD,
        .data_bits = UART_DATA_8_BITS,
        .parity    = UART_PARITY_DISABLE,
        .stop_bits = UART_STOP_BITS_1,
        .flow_ctrl = UART_HW_FLOWCTRL_DISABLE
    };
    uart_param_config(UART_NUM_1, &uart1_config);
    uart_set_pin(UART_NUM_1, USB_HOST_TXD, USB_HOST_RXD, USB_HOST_RTS, USB_HOST_CTS);
    ret_code=uart_driver_install(UART_NUM_1, UART_BUF_SIZE, UART_BUF_SIZE, 0, NULL, 0);
	printf("UART1 install:%d", ret_code);
	uart_flush(UART_NUM_1); // Flush anything from rx ring buffer

    // Configure parameters of an UART driver, communication pins and install the driver
    uart_config_t uart2_config = {
        .baud_rate = EMULATOR_BAUD,
        .data_bits = UART_DATA_8_BITS,
        .parity    = UART_PARITY_DISABLE,
        .stop_bits = UART_STOP_BITS_1,
        .flow_ctrl = UART_HW_FLOWCTRL_DISABLE
    };
    uart_param_config(UART_NUM_2, &uart2_config);
    uart_set_pin(UART_NUM_2, EMULATOR_TXD, EMULATOR_RXD, EMULATOR_RTS, EMULATOR_CTS);
    ret_code=uart_driver_install(UART_NUM_2, UART_BUF_SIZE, UART_BUF_SIZE, 0, NULL, 0);
	printf("UART1 install:%d", ret_code);
	uart_flush(UART_NUM_2); // Flush anything
	vTaskDelay(pdMS_TO_TICKS(50)); // Heuristic wait
	uart_write_bytes(UART_NUM_2, "z00x", 4); // Set to mode 0

	// Task loop
    while (1) 
	{
		if (barcode_received) // Send barcode to emulator
		{
			printf("Barcode:%s\r\n",usb_host_rx);
			process_code(usb_host_rx);
			printf("%llu\r\n", esp_timer_get_time());
			vTaskDelay(pdMS_TO_TICKS(delay_between_code_ms)); // Delay regardless previous barcode send.
			uart_write_bytes(UART_NUM_2, (const char *) usb_host_rx, (int) strlen((const char*)usb_host_rx));
			uart_write_bytes(UART_NUM_2, "x",1);
			vTaskDelay(pdMS_TO_TICKS(delay_between_code_ms)); // Delay for response.
			len=uart_read_bytes(UART_NUM_2, (uint8_t *) emulator_response, emulator_response_max_len, 1 / portTICK_RATE_MS);
			if (len==-1) 
			{
				printf("Error receiving emulator response.\r\n");
			}
			else
			{
				emulator_response[len]='\0'; // Terminate the response
				printf("%s\r\n",emulator_response);
				//ESP_LOGI("Dev_task", "%s\r\n",emulator_response);
			}
			barcode_received=0;
			//last_code_sent_us=esp_timer_get_time();
		}
		
		else // Collect barcode from usb host mcu
		{
			// Read data from the UART
			len = uart_read_bytes(UART_NUM_1, data+old_len, UART_BUF_SIZE, 1 / portTICK_RATE_MS);
			if (len==-1) continue; // error, maybe nothing to read.
			data[old_len+len]='\0'; // Terminate C-string
			old_len+=len;
			if ((old_len>=2)&&(data[old_len-2]=='\r')&&(data[old_len-1]=='\n')) // Make sure there are at least 2 characters so old_len-2 in the following code won't write to places it shouldn't write to.
			{
				data[old_len-2]='\0'; // Discard \r\n
				barcode_received=1;
				old_len=0;
			}
		}
		vTaskDelay(pdMS_TO_TICKS(50));
    }
}

How I envisioned the program will work:

Two devices are connected to esp32 via uart1 and uart2 so let's just call the devices uart1 and uart2. Uart1 sends a barcode to esp32. Esp32 reads the complete barcode, do processing, and passes the barcode to uart2. Upon receiving barcode, uart2 responds with a message. Esp32 reads the messages and displays it to console.

The setup phase sets up both uarts and sends set mode command to uart2's device. Then depending on the value of barcode_received, the task either sends the barcode to uart2, or waits on uart1 to receive bits of the barcode. I think I can further encapsulate the code by defining barcode_received inside the task (no it wasn't declared as volatile, my bad) and also peek into uart1 receive buffer before performing the read.

[username]

I wish i had to time to help in more detail. I love tasks, in fact often find myself separating many things into their own things when i could have combined them.

For my application. I have 3 tasks. The first one is just for receiving serial data.

Code: Select all

/*************************************************
 *
 *
 * static void uart_event_task(void *pvParameters)
 *
 *
 *************************************************/
void uart_event_task(void *pvParameters)
{
	uart_event_t event;
	size_t buffered_size;
	uint8_t dtmp[BUF_SIZE];
	
	
	for (;;) 
	{
		//Waiting for UART event.
		if(xQueueReceive(uart2_queue, (void *)&event, (portTickType)portMAX_DELAY)) 
		{
			//ESP_LOGI(TAG, "uart[%d] Timed Out", UART_K64);
			
			// Just incase (portTickType)portMAX_DELAY ever times out
			// Make sure we have received something
			if (event.size > 0)
			{
				bzero(dtmp, BUF_SIZE);
				//ESP_LOGI(TAG, "uart[%d] event:", UART_K64);

				switch (event.type) 
				{
					//Event of UART receving data
					/*We'd better handler data event fast, there would be much more data events than
					other types of events. If we take too much time on data event, the queue might
					be full.*/
					case UART_DATA :	uart_read_bytes(UART_K64, dtmp, event.size, portMAX_DELAY);
										ESP_LOGI(TAG, "[%d]=%s", event.size, dtmp);
										break;
				
					//Event of HW FIFO overflow detected
					case UART_FIFO_OVF :    ESP_LOGI(TAG, "hw fifo overflow");
											// If fifo overflow happened, you should consider adding flow control for your application.
											// The ISR has already reset the rx FIFO,
											// As an example, we directly flush the rx buffer here in order to read more data.
											uart_flush_input(UART_K64);
											xQueueReset(uart2_queue);
											break;
				
					//Event of UART ring buffer full
					case UART_BUFFER_FULL :	    ESP_LOGI(TAG, "ring buffer full");
											// If buffer full happened, you should consider encreasing your buffer size
											// As an example, we directly flush the rx buffer here in order to read more data.
											uart_flush_input(UART_K64);
											xQueueReset(uart2_queue);
											break;
					//Event of UART RX break detected
					case UART_BREAK :    ESP_LOGI(TAG, "uart rx break"); break;
				
					//Event of UART parity check error
					case UART_PARITY_ERR :    ESP_LOGI(TAG, "uart parity error"); break;
				
					//Event of UART frame error
					case UART_FRAME_ERR :    ESP_LOGI(TAG, "uart frame error"); break;
				
					//UART_PATTERN_DET
					case UART_PATTERN_DET :	uart_get_buffered_data_len(UART_K64, &buffered_size);
											int pos = uart_pattern_pop_pos(UART_K64);
					
											//ESP_LOGI(TAG, "[UART PATTERN DETECTED] pos: %d, buffered size: %d", pos, buffered_size);
					
											if (pos == -1) 
											{
												ESP_LOGI(TAG, "!!! Pattern position queue is full !!!");
												// There used to be a UART_PATTERN_DET event, but the pattern position queue is full so that it can not
												// record the position. We should set a larger queue size.
												// As an example, we directly flush the rx buffer here.
												uart_flush_input(UART_K64);
											}
											else 
											{
												uart_read_bytes(UART_K64, dtmp, buffered_size, 100 / portTICK_PERIOD_MS);
							
												ESP_LOGI(TAG, "[%u]: %s", buffered_size, dtmp);

												// Lets parse the K64 Received serial data 
												parseSerialCommands((char *)dtmp);			
											}
											break;
					
					//Others
					default :    ESP_LOGI(TAG, "uart event type: %d", event.type); break;
				}
				
			}	
		}
	}
	free(dtmp);
	vTaskDelete(NULL);
}

if you look at the end you will see that when it received all the serial data it calls parseSerialCommands() function.
In there it figures out what I received and what to do with it. Just a short clip of whats in mine.

Code: Select all

//**************************************************************************
//
//
//**************************************************************************
void parseSerialCommands(char *temp_string)
{
	// NT - Notification of DTMF Tone received
	if(strncmp(temp_string, "NT", 2) == 0)
	{
		// Tell DTMF Timer to start the timer or reset it
		// When its done it will sendout the DTMF
		xEventGroupSetBits(dtmf_rx_timer_event_group, DTMF_RX_TIMER_START_BIT);
        }
        
}

All i do in parseSerialCommands() is set flag bits for my other waiting task.

Code: Select all

const int DTMF_TX_TIMER_START_BIT = BIT0;
const int DTMF_TX_TIMER_CANCEL_BIT = BIT1;

const int DTMF_RX_TIMER_START_BIT = BIT0;
const int DTMF_RX_TIMER_CANCEL_BIT = BIT1;


/********************************************************************
 *
 *
 *  	dtmfTXTimerTask()
 *
 *
 ********************************************************************/
void dtmfTXTimerTask(void * parameter)
{
	EventBits_t uxBits;
	TickType_t dtmf_tx_timer;
	
	dtmf_tx_timer_event_group = xEventGroupCreate();
	
	xEventGroupClearBits(dtmf_tx_timer_event_group, DTMF_TX_TIMER_START_BIT | DTMF_TX_TIMER_CANCEL_BIT);

	printf("DTMF TX Timer Task Started\r\n");

	// Clear the Buffer
	clearTX_DTMF();

	
	for (;;)
	{
	        // wait here until a bit changes
		uxBits = xEventGroupWaitBits(dtmf_tx_timer_event_group, DTMF_TX_TIMER_START_BIT, true, false, portMAX_DELAY);

		
		// Was the bit set, or did we reach portMAX_DELAY ?
		if(uxBits & DTMF_TX_TIMER_START_BIT)
		{
			printf("DTMF TX Timer Triggered\r\n");
			
			// Clear This Bit, just play it safe
			xEventGroupClearBits(dtmf_tx_timer_event_group, DTMF_TX_TIMER_CANCEL_BIT);
		}
			
		vTaskDelay(10 / portTICK_PERIOD_MS);
	}
	
}

Hopefully, you can figure out what I am doing with this minimalist code snippets. What I try if i was you would be to have one task handle all incoming serial data. Thats its only job. Once you get that data, have another task wait for that bit to be set so you can then check it. Though doing it all in one task is ok too. AS the old saying goes, there are many ways to skin a cat.

[zliudr]

Great! Thank you so much for your snippets! Since I am new to this, I rely on the example code in the esp-idf repo to understand. I don't know where else other than the 1600 page doc to find literature on how to do simple things. It's like a space alien came by an English dictionary and is teaching themselves how to speak English without any other books in English
I'm somewhat familiar with bits from my wading into esp-azure and they use a bit to indicate wifi connectivity.

As of now, my single-task approach is finally working. Since C/C++ is so easy to shoot yourself in the foot, I won't mind just expanding on your example and my working task until I know what I did wrong. I did increase stack to 4K though. Compilation is slow. I have a decent machine and it takes too long

Reminds me the olden days with x386 assembly and no debugger that can take you into protected mode. "Boot and reboot are on a boat, boot fell off, who's left?"

[username]

I rely on the example code in the esp-idf repo to understand.

I agree. If they had not provided that extensive example folder with all the great examples i would not have been able to do the things i have done.

Compilation is slow. I have a decent machine and it takes too long

Really? A new project compilation takes about 10-12 seconds for me. Are you using the old "make" way or the new CMake ?
If using the older make, add the -jx option to your command line. I.E. make -j8 flash. The -j option will allow you to use more cores of your cpu when you compile. I would suggest if you are using make, to switch to CMake, you wont have to be concerned about it and it's very fast.

https://docs.espressif.com/projects/esp ... index.html

[zliudr]

Thanks for the option! With -j8 I cut down compile time by more than half for new projects. I'm using make. I'll read about the CMake way to see if I can do that to further cut down my compile time. I see occasional recompilation of lots of core/system stuff at the slightest change of sdkconfig, which I thought shouldn't require recompile. Maybe CMake avoids unnecessary recompilation. Clean also takes less time with the -j8 option. One step at a time

Thinking back how ESP platforms became so popular as they are now, they started with ESP8266 wifi modules for arduino and other MCU dev platforms. At the time an arduino wifi shield or module cost $30-40. ESP8266 was less than $10 I think. Before that an arduino official wifi shield used to cost over $80 and was not easy to use since Arduino developers assume best-case scenarios that are not the case for actual projects and their documentations/code lack details of how to handle less-than-optimal conditions. I got started with ESP8266 on an arduino firmware. They must have done a lot of work to bring this platform to the arduino world. MicroPython is also amazing on ESP32. For this project I just have to use the C/C++ IDF and Azure SDKs. It's hard, without the huge arduino forum crowd even with their excellent sample projects. Nothing is impossible although I am starting to consider that the esp toolchains are built to build other platforms such as MicroPython, Arduino, NodeJS, etc. Those who use esp toolchains directly must embrace some learning curves.