Merge branch 'bugfix/uart_no_int_after_flush_v4.4' into 'release/v4.4'

UART: RX interrupts are now properly restored after a flush (backport v4.4)

See merge request espressif/esp-idf!17122

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);
+}

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;