diff --git a/components/driver/test/test_uart.c b/components/driver/test/test_uart.c index 21e3d05cc9..571d3d0972 100644 --- a/components/driver/test/test_uart.c +++ b/components/driver/test/test_uart.c @@ -461,3 +461,76 @@ TEST_CASE("uart can register and free custom ISRs", "[uart]") uart_test_custom_isr_core0(NULL); #endif //!CONFIG_FREERTOS_UNICORE } + +TEST_CASE("uart int state restored after flush", "[uart]") +{ + /** + * The first goal of this test is to make sure that when our RX FIFO is full, + * we can continue receiving back data after flushing + * For more details, check IDF-4374 + */ + uart_config_t uart_config = { + .baud_rate = 115200, + .data_bits = UART_DATA_8_BITS, + .parity = UART_PARITY_DISABLE, + .stop_bits = UART_STOP_BITS_1, + .flow_ctrl = UART_HW_FLOWCTRL_DISABLE, + .source_clk = UART_SCLK_APB, + }; + + const uart_port_t uart_echo = UART_NUM_1; + const int uart_tx_signal = U1TXD_OUT_IDX; + const int uart_tx = 4; + const int uart_rx = 5; + const int buf_size = 256; + const int intr_alloc_flags = 0; + + TEST_ESP_OK(uart_driver_install(uart_echo, buf_size * 2, 0, 0, NULL, intr_alloc_flags)); + TEST_ESP_OK(uart_param_config(uart_echo, &uart_config)); + TEST_ESP_OK(uart_set_pin(uart_echo, uart_tx, uart_rx, UART_PIN_NO_CHANGE, UART_PIN_NO_CHANGE)); + + /* Make sure UART2's RX signal is connected to TX pin + * This creates a loop that lets us receive anything we send on the UART */ + esp_rom_gpio_connect_out_signal(uart_rx, uart_tx_signal, false, false); + + uint8_t *data = (uint8_t *) malloc(buf_size); + TEST_ASSERT_NOT_NULL(data); + uart_write_bytes(uart_echo, (const char *) data, buf_size); + + /* As we set up a loopback, we can read them back on RX */ + int len = uart_read_bytes(uart_echo, data, buf_size, 1000 / portTICK_RATE_MS); + TEST_ASSERT_EQUAL(len, buf_size); + + /* Fill the RX buffer, this should disable the RX interrupts */ + int written = uart_write_bytes(uart_echo, (const char *) data, buf_size); + TEST_ASSERT_NOT_EQUAL(-1, written); + written = uart_write_bytes(uart_echo, (const char *) data, buf_size); + TEST_ASSERT_NOT_EQUAL(-1, written); + written = uart_write_bytes(uart_echo, (const char *) data, buf_size); + TEST_ASSERT_NOT_EQUAL(-1, written); + + /* Flush the input buffer, RX interrupts should be re-enabled */ + uart_flush_input(uart_echo); + written = uart_write_bytes(uart_echo, (const char *) data, buf_size); + TEST_ASSERT_NOT_EQUAL(-1, written); + len = uart_read_bytes(uart_echo, data, buf_size, 1000 / portTICK_RATE_MS); + /* len equals buf_size bytes if interrupts were indeed re-enabled */ + TEST_ASSERT_EQUAL(len, buf_size); + + /** + * Second test, make sure that if we explicitly disable the RX interrupts, + * they are NOT re-enabled after flushing + * To do so, start by cleaning the RX FIFO, disable the RX interrupts, + * flush again, send data to the UART and check that we haven't received + * any of the bytes */ + uart_flush_input(uart_echo); + uart_disable_rx_intr(uart_echo); + uart_flush_input(uart_echo); + written = uart_write_bytes(uart_echo, (const char *) data, buf_size); + TEST_ASSERT_NOT_EQUAL(-1, written); + len = uart_read_bytes(uart_echo, data, buf_size, 250 / portTICK_RATE_MS); + TEST_ASSERT_EQUAL(len, 0); + + TEST_ESP_OK(uart_driver_delete(uart_echo)); + free(data); +} diff --git a/components/driver/uart.c b/components/driver/uart.c index 9a21539b16..dda1f3b69a 100644 --- a/components/driver/uart.c +++ b/components/driver/uart.c @@ -114,6 +114,7 @@ typedef struct { uint8_t *rx_head_ptr; /*!< pointer to the head of RX item*/ uint8_t rx_data_buf[SOC_UART_FIFO_LEN]; /*!< Data buffer to stash FIFO data*/ uint8_t rx_stash_len; /*!< stashed data length.(When using flow control, after reading out FIFO data, if we fail to push to buffer, we can just stash them.) */ + uint32_t rx_int_usr_mask; /*!< RX interrupt status. Valid at any time, regardless of RX buffer status. */ uart_pat_rb_t rx_pattern_pos; int tx_buf_size; /*!< TX ring buffer size */ bool tx_waiting_fifo; /*!< this flag indicates that some task is waiting for FIFO empty interrupt, used to send all data without any data buffer*/ @@ -357,16 +358,45 @@ esp_err_t uart_enable_intr_mask(uart_port_t uart_num, uint32_t enable_mask) { ESP_RETURN_ON_FALSE((uart_num < UART_NUM_MAX), ESP_FAIL, UART_TAG, "uart_num error"); UART_ENTER_CRITICAL(&(uart_context[uart_num].spinlock)); + /* Keep track of the interrupt toggling. In fact, without such variable, + * once the RX buffer is full and the RX interrupts disabled, it is + * impossible what was the previous state (enabled/disabled) of these + * interrupt masks. Thus, this will be very particularly handy when + * emptying a filled RX buffer. */ + p_uart_obj[uart_num]->rx_int_usr_mask |= enable_mask; uart_hal_clr_intsts_mask(&(uart_context[uart_num].hal), enable_mask); uart_hal_ena_intr_mask(&(uart_context[uart_num].hal), enable_mask); UART_EXIT_CRITICAL(&(uart_context[uart_num].spinlock)); return ESP_OK; } +/** + * @brief Function re-enabling the given interrupts (mask) if and only if + * they have not been disabled by the user. + * + * @param uart_num UART number to perform the operation on + * @param enable_mask Interrupts (flags) to be re-enabled + * + * @return ESP_OK in success, ESP_FAIL if uart_num is incorrect + */ +static esp_err_t uart_reenable_intr_mask(uart_port_t uart_num, uint32_t enable_mask) +{ + ESP_RETURN_ON_FALSE((uart_num < UART_NUM_MAX), ESP_FAIL, UART_TAG, "uart_num error"); + UART_ENTER_CRITICAL(&(uart_context[uart_num].spinlock)); + /* Mask will only contain the interrupt flags that needs to be re-enabled + * AND which have NOT been explicitly disabled by the user. */ + uint32_t mask = p_uart_obj[uart_num]->rx_int_usr_mask & enable_mask; + uart_hal_clr_intsts_mask(&(uart_context[uart_num].hal), mask); + uart_hal_ena_intr_mask(&(uart_context[uart_num].hal), mask); + UART_EXIT_CRITICAL(&(uart_context[uart_num].spinlock)); + return ESP_OK; +} + esp_err_t uart_disable_intr_mask(uart_port_t uart_num, uint32_t disable_mask) { ESP_RETURN_ON_FALSE((uart_num < UART_NUM_MAX), ESP_FAIL, UART_TAG, "uart_num error"); UART_ENTER_CRITICAL(&(uart_context[uart_num].spinlock)); + p_uart_obj[uart_num]->rx_int_usr_mask &= ~disable_mask; uart_hal_disable_intr_mask(&(uart_context[uart_num].hal), disable_mask); UART_EXIT_CRITICAL(&(uart_context[uart_num].spinlock)); return ESP_OK; @@ -1235,7 +1265,9 @@ static bool uart_check_buf_full(uart_port_t uart_num) p_uart_obj[uart_num]->rx_buffered_len += p_uart_obj[uart_num]->rx_stash_len; p_uart_obj[uart_num]->rx_buffer_full_flg = false; UART_EXIT_CRITICAL(&(uart_context[uart_num].spinlock)); - uart_enable_rx_intr(p_uart_obj[uart_num]->uart_num); + /* Only re-activate UART_INTR_RXFIFO_TOUT or UART_INTR_RXFIFO_FULL + * interrupts if they were NOT explicitly disabled by the user. */ + uart_reenable_intr_mask(p_uart_obj[uart_num]->uart_num, UART_INTR_RXFIFO_TOUT | UART_INTR_RXFIFO_FULL); return true; } } @@ -1313,16 +1345,6 @@ esp_err_t uart_get_buffered_data_len(uart_port_t uart_num, size_t *size) esp_err_t uart_flush(uart_port_t uart_num) __attribute__((alias("uart_flush_input"))); -static esp_err_t uart_disable_intr_mask_and_return_prev(uart_port_t uart_num, uint32_t disable_mask, uint32_t *prev_mask) -{ - ESP_RETURN_ON_FALSE((uart_num < UART_NUM_MAX), ESP_FAIL, UART_TAG, "uart_num error"); - UART_ENTER_CRITICAL(&(uart_context[uart_num].spinlock)); - *prev_mask = uart_hal_get_intr_ena_status(&uart_context[uart_num].hal) & disable_mask; - uart_hal_disable_intr_mask(&(uart_context[uart_num].hal), disable_mask); - UART_EXIT_CRITICAL(&(uart_context[uart_num].spinlock)); - return ESP_OK; -} - esp_err_t uart_flush_input(uart_port_t uart_num) { ESP_RETURN_ON_FALSE((uart_num < UART_NUM_MAX), ESP_FAIL, UART_TAG, "uart_num error"); @@ -1330,11 +1352,12 @@ esp_err_t uart_flush_input(uart_port_t uart_num) uart_obj_t *p_uart = p_uart_obj[uart_num]; uint8_t *data; size_t size; - uint32_t prev_mask; //rx sem protect the ring buffer read related functions xSemaphoreTake(p_uart->rx_mux, (portTickType)portMAX_DELAY); - uart_disable_intr_mask_and_return_prev(uart_num, UART_INTR_RXFIFO_FULL | UART_INTR_RXFIFO_TOUT, &prev_mask); + UART_ENTER_CRITICAL(&(uart_context[uart_num].spinlock)); + uart_hal_disable_intr_mask(&(uart_context[uart_num].hal), UART_INTR_RXFIFO_FULL | UART_INTR_RXFIFO_TOUT); + UART_EXIT_CRITICAL(&(uart_context[uart_num].spinlock)); while (true) { if (p_uart->rx_head_ptr) { vRingbufferReturnItem(p_uart->rx_ring_buf, p_uart->rx_head_ptr); @@ -1382,7 +1405,9 @@ esp_err_t uart_flush_input(uart_port_t uart_num) p_uart->rx_cur_remain = 0; p_uart->rx_head_ptr = NULL; uart_hal_rxfifo_rst(&(uart_context[uart_num].hal)); - uart_enable_intr_mask(uart_num, prev_mask); + /* Only re-enable UART_INTR_RXFIFO_TOUT or UART_INTR_RXFIFO_FULL if they + * were explicitly enabled by the user. */ + uart_reenable_intr_mask(uart_num, UART_INTR_RXFIFO_TOUT | UART_INTR_RXFIFO_FULL); xSemaphoreGive(p_uart->rx_mux); return ESP_OK; } @@ -1561,6 +1586,7 @@ esp_err_t uart_driver_install(uart_port_t uart_num, int rx_buffer_size, int tx_b p_uart_obj[uart_num]->tx_waiting_fifo = false; p_uart_obj[uart_num]->rx_ptr = NULL; p_uart_obj[uart_num]->rx_cur_remain = 0; + p_uart_obj[uart_num]->rx_int_usr_mask = UART_INTR_RXFIFO_FULL | UART_INTR_RXFIFO_TOUT; p_uart_obj[uart_num]->rx_head_ptr = NULL; p_uart_obj[uart_num]->tx_buf_size = tx_buffer_size; p_uart_obj[uart_num]->uart_select_notif_callback = NULL;