From e41e67f2f16f7e2829a7a0ca070f4d775847bdcf Mon Sep 17 00:00:00 2001 From: Luca Burelli Date: Wed, 13 Jan 2021 18:42:04 +0100 Subject: [PATCH 1/2] uart: Add missing critical section wrappers around rx_buffered_len The missing barriers caused uart_get_buffered_data_len() to (very rarely) return a garbage value. When used in MicroPython, though, this caused select() to return and a subsequent read() to stall indefinitely until a char was actually available. Signed-off-by: Chen Yi Qun Closes https://github.com/espressif/esp-idf/issues/6397 Merges https://github.com/espressif/esp-idf/pull/6396 --- components/driver/uart.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/components/driver/uart.c b/components/driver/uart.c index f9e0948a62..ea79bd387a 100644 --- a/components/driver/uart.c +++ b/components/driver/uart.c @@ -1295,7 +1295,9 @@ esp_err_t uart_get_buffered_data_len(uart_port_t uart_num, size_t *size) { ESP_RETURN_ON_FALSE((uart_num < UART_NUM_MAX), ESP_FAIL, UART_TAG, "uart_num error"); ESP_RETURN_ON_FALSE((p_uart_obj[uart_num]), ESP_FAIL, UART_TAG, "uart driver error"); + UART_ENTER_CRITICAL(&(uart_context[uart_num].spinlock)); *size = p_uart_obj[uart_num]->rx_buffered_len; + UART_EXIT_CRITICAL(&(uart_context[uart_num].spinlock)); return ESP_OK; } @@ -1334,14 +1336,17 @@ esp_err_t uart_flush_input(uart_port_t uart_num) p_uart->rx_cur_remain = 0; p_uart->rx_head_ptr = NULL; } - data = (uint8_t *) xRingbufferReceive(p_uart->rx_ring_buf, &size, (portTickType) 0); - if (data == NULL) { - if ( p_uart_obj[uart_num]->rx_buffered_len != 0 ) { - ESP_LOGE(UART_TAG, "rx_buffered_len error"); + data = (uint8_t*) xRingbufferReceive(p_uart->rx_ring_buf, &size, (portTickType) 0); + if(data == NULL) { + UART_ENTER_CRITICAL(&(uart_context[uart_num].spinlock)); + if( p_uart_obj[uart_num]->rx_buffered_len != 0 ) { p_uart_obj[uart_num]->rx_buffered_len = 0; + UART_EXIT_CRITICAL(&(uart_context[uart_num].spinlock)); + // this must be called outside the critical section + ESP_LOGE(UART_TAG, "rx_buffered_len error"); + UART_ENTER_CRITICAL(&(uart_context[uart_num].spinlock)); } //We also need to clear the `rx_buffer_full_flg` here. - UART_ENTER_CRITICAL(&(uart_context[uart_num].spinlock)); p_uart_obj[uart_num]->rx_buffer_full_flg = false; UART_EXIT_CRITICAL(&(uart_context[uart_num].spinlock)); break; From 4e3f5573c4ba1f5b00dc3f4337bf6be119b1da09 Mon Sep 17 00:00:00 2001 From: Omar Chebib Date: Mon, 28 Jun 2021 14:46:41 +0800 Subject: [PATCH 2/2] uart: cleaner way of handling error in a critical section Some critical sections have also been added, making the code more symetric accross the similar functions. Closes https://github.com/espressif/esp-idf/pull/6396 --- components/driver/uart.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/components/driver/uart.c b/components/driver/uart.c index ea79bd387a..3f06bc82eb 100644 --- a/components/driver/uart.c +++ b/components/driver/uart.c @@ -258,7 +258,9 @@ esp_err_t uart_set_stop_bits(uart_port_t uart_num, uart_stop_bits_t stop_bit) esp_err_t uart_get_stop_bits(uart_port_t uart_num, uart_stop_bits_t *stop_bit) { ESP_RETURN_ON_FALSE((uart_num < UART_NUM_MAX), ESP_FAIL, UART_TAG, "uart_num error"); + UART_ENTER_CRITICAL(&(uart_context[uart_num].spinlock)); uart_hal_get_stop_bits(&(uart_context[uart_num].hal), stop_bit); + UART_EXIT_CRITICAL(&(uart_context[uart_num].spinlock)); return ESP_OK; } @@ -274,7 +276,9 @@ esp_err_t uart_set_parity(uart_port_t uart_num, uart_parity_t parity_mode) esp_err_t uart_get_parity(uart_port_t uart_num, uart_parity_t *parity_mode) { ESP_RETURN_ON_FALSE((uart_num < UART_NUM_MAX), ESP_FAIL, UART_TAG, "uart_num error"); + UART_ENTER_CRITICAL(&(uart_context[uart_num].spinlock)); uart_hal_get_parity(&(uart_context[uart_num].hal), parity_mode); + UART_EXIT_CRITICAL(&(uart_context[uart_num].spinlock)); return ESP_OK; } @@ -1338,17 +1342,19 @@ esp_err_t uart_flush_input(uart_port_t uart_num) } data = (uint8_t*) xRingbufferReceive(p_uart->rx_ring_buf, &size, (portTickType) 0); if(data == NULL) { + bool error = false; UART_ENTER_CRITICAL(&(uart_context[uart_num].spinlock)); if( p_uart_obj[uart_num]->rx_buffered_len != 0 ) { p_uart_obj[uart_num]->rx_buffered_len = 0; - UART_EXIT_CRITICAL(&(uart_context[uart_num].spinlock)); - // this must be called outside the critical section - ESP_LOGE(UART_TAG, "rx_buffered_len error"); - UART_ENTER_CRITICAL(&(uart_context[uart_num].spinlock)); + error = true; } //We also need to clear the `rx_buffer_full_flg` here. p_uart_obj[uart_num]->rx_buffer_full_flg = false; UART_EXIT_CRITICAL(&(uart_context[uart_num].spinlock)); + if (error) { + // this must be called outside the critical section + ESP_LOGE(UART_TAG, "rx_buffered_len error"); + } break; } UART_ENTER_CRITICAL(&(uart_context[uart_num].spinlock));