From 5f27ec91570a2c098018f0d8de4014f8b5757a6a Mon Sep 17 00:00:00 2001 From: Omar Chebib Date: Wed, 29 Dec 2021 17:13:46 +0800 Subject: [PATCH 1/2] UART: fix a bug preventing the user from freeing a previously registered ISR * Closes https://github.com/espressif/esp-idf/issues/8150 --- components/driver/include/driver/uart.h | 4 +- components/driver/test/test_uart.c | 98 +++++++++++++++++++++++++ components/driver/uart.c | 6 ++ 3 files changed, 106 insertions(+), 2 deletions(-) diff --git a/components/driver/include/driver/uart.h b/components/driver/include/driver/uart.h index 7269b3fa14..970191ab4e 100644 --- a/components/driver/include/driver/uart.h +++ b/components/driver/include/driver/uart.h @@ -333,7 +333,7 @@ esp_err_t uart_enable_rx_intr(uart_port_t uart_num); esp_err_t uart_disable_rx_intr(uart_port_t uart_num); /** - * @brief Disable UART TX interrupt (TX_FULL & TX_TIMEOUT INTERRUPT) + * @brief Disable UART TX interrupt (TXFIFO_EMPTY INTERRUPT) * * @param uart_num UART port number * @@ -344,7 +344,7 @@ esp_err_t uart_disable_rx_intr(uart_port_t uart_num); esp_err_t uart_disable_tx_intr(uart_port_t uart_num); /** - * @brief Enable UART TX interrupt (TX_FULL & TX_TIMEOUT INTERRUPT) + * @brief Enable UART TX interrupt (TXFIFO_EMPTY INTERRUPT) * * @param uart_num UART port number, the max port number is (UART_NUM_MAX -1). * @param enable 1: enable; 0: disable diff --git a/components/driver/test/test_uart.c b/components/driver/test/test_uart.c index df190f4753..d93af6b1cd 100644 --- a/components/driver/test/test_uart.c +++ b/components/driver/test/test_uart.c @@ -7,6 +7,8 @@ #include "esp_system.h" // for uint32_t esp_random() #include "esp_rom_gpio.h" #include "soc/uart_periph.h" +#include "hal/uart_ll.h" +#include "hal/uart_hal.h" #define UART_TAG "Uart" #define UART_NUM1 (UART_NUM_1) @@ -427,3 +429,99 @@ TEST_CASE("uart int state restored after flush", "[uart]") TEST_ESP_OK(uart_driver_delete(uart_echo)); free(data); } + +/* Global variable shared between the ISR and the test function */ +volatile uint32_t uart_isr_happened = 0; + +static void uart_custom_isr(void* arg) { + (void) arg; + + /* Clear interrupt status and disable TX interrupt here in order to + * prevent an infinite call loop. Use the LL function to prevent + * entering a critical section from an interrupt. */ + uart_ll_disable_intr_mask(UART_LL_GET_HW(1), UART_INTR_TXFIFO_EMPTY); + uart_clear_intr_status(UART_NUM_1, UART_INTR_TXFIFO_EMPTY); + + /* Mark the interrupt as serviced */ + uart_isr_happened = 1; +} + + +/** + * This function shall always be executed by core 0. + * This is required by `uart_isr_free`. + */ +static void uart_test_custom_isr_core0(void* param) { + /** + * Setup the UART1 and make sure we can register and free a custom ISR + */ + 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 = 4; + const int uart_rx = 5; + const int buf_size = 256; + const int intr_alloc_flags = 0; + const char msg[] = "hello world\n"; + uart_isr_handle_t handle = NULL; + + 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)); + + /* Unregister the default ISR setup by the function call above */ + TEST_ESP_OK(uart_isr_free(uart_echo)); + TEST_ESP_OK(uart_isr_register(uart_echo, uart_custom_isr, NULL, intr_alloc_flags, &handle)); + /* Set the TX FIFO empty threshold to the size of the message we are sending, + * make sure it is never 0 in any case */ + TEST_ESP_OK(uart_enable_tx_intr(uart_echo, true, MAX(sizeof(msg), 1))); + uart_write_bytes(uart_echo, msg, sizeof(msg)); + + /* 10ms will be enough to receive the interrupt */ + vTaskDelay(10 / portTICK_PERIOD_MS); + + /* Make sure the ISR occured */ + TEST_ASSERT_EQUAL(uart_isr_happened, 1); + esp_rom_printf("ISR happened: %d\n", uart_isr_happened); + TEST_ESP_OK(uart_isr_free(uart_echo)); + TEST_ESP_OK(uart_driver_delete(uart_echo)); + +#if !CONFIG_FREERTOS_UNICORE + TaskHandle_t* parent_task = (TaskHandle_t*) param; + esp_rom_printf("Notifying caller\n"); + TEST_ASSERT(xTaskNotify(*parent_task, 0, eNoAction)); + vTaskDelete(NULL); +#else + (void) param; +#endif //!CONFIG_FREERTOS_UNICORE +} + + +TEST_CASE("uart can register and free custom ISRs", "[uart]") +{ +#if !CONFIG_FREERTOS_UNICORE + TaskHandle_t task_handle; + TaskHandle_t current_handler = xTaskGetCurrentTaskHandle(); + /* Run the test on a determianted core, do not allow the core to be changed + * as we will manipulate ISRs. */ + BaseType_t ret = xTaskCreatePinnedToCore(uart_test_custom_isr_core0, + "uart_test_custom_isr_core0_task", + 2048, + ¤t_handler, + 5, + &task_handle, + 0); + TEST_ASSERT(ret) + TEST_ASSERT(xTaskNotifyWait(0, 0, NULL, 1000 / portTICK_PERIOD_MS)); + (void) task_handle; +#else + uart_test_custom_isr_core0(NULL); +#endif //!CONFIG_FREERTOS_UNICORE +} diff --git a/components/driver/uart.c b/components/driver/uart.c index 21d2779ec6..6f2295d740 100644 --- a/components/driver/uart.c +++ b/components/driver/uart.c @@ -611,6 +611,9 @@ esp_err_t uart_disable_tx_intr(uart_port_t uart_num) esp_err_t uart_enable_tx_intr(uart_port_t uart_num, int enable, int thresh) { + if (enable == 0) { + return uart_disable_tx_intr(uart_num); + } UART_CHECK((uart_num < UART_NUM_MAX), "uart_num error", ESP_FAIL); UART_CHECK((thresh < SOC_UART_FIFO_LEN), "empty intr threshold error", ESP_FAIL); uart_hal_clr_intsts_mask(&(uart_context[uart_num].hal), UART_INTR_TXFIFO_EMPTY); @@ -627,6 +630,9 @@ esp_err_t uart_isr_register(uart_port_t uart_num, void (*fn)(void *), void *arg, UART_CHECK((uart_num < UART_NUM_MAX), "uart_num error", ESP_FAIL); UART_ENTER_CRITICAL(&(uart_context[uart_num].spinlock)); ret = esp_intr_alloc(uart_periph_signal[uart_num].irq, intr_alloc_flags, fn, arg, handle); + if (ret == ESP_OK) { + p_uart_obj[uart_num]->intr_handle = *handle; + } UART_EXIT_CRITICAL(&(uart_context[uart_num].spinlock)); return ret; } From 65cfc9e656ed3e333e7101cf5d8786ba4298b026 Mon Sep 17 00:00:00 2001 From: Omar Chebib Date: Tue, 22 Feb 2022 10:47:49 +0800 Subject: [PATCH 2/2] UART: Fix custom ISR unit test On ESP32, UART_INTR_BRK_DET may be triggered after setting the new ISR handler. Disable these interrrupts. --- components/driver/test/test_uart.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/components/driver/test/test_uart.c b/components/driver/test/test_uart.c index d93af6b1cd..8f4d3412a1 100644 --- a/components/driver/test/test_uart.c +++ b/components/driver/test/test_uart.c @@ -476,6 +476,10 @@ static void uart_test_custom_isr_core0(void* param) { 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)); + /* Prevent the custom ISR handler from being called if UART_INTR_BRK_DET interrupt occurs. + * It shall only be called for TX interrupts. */ + uart_disable_intr_mask(uart_echo, UART_INTR_BRK_DET); + /* Unregister the default ISR setup by the function call above */ TEST_ESP_OK(uart_isr_free(uart_echo)); TEST_ESP_OK(uart_isr_register(uart_echo, uart_custom_isr, NULL, intr_alloc_flags, &handle)); @@ -518,7 +522,7 @@ TEST_CASE("uart can register and free custom ISRs", "[uart]") 5, &task_handle, 0); - TEST_ASSERT(ret) + TEST_ASSERT(ret); TEST_ASSERT(xTaskNotifyWait(0, 0, NULL, 1000 / portTICK_PERIOD_MS)); (void) task_handle; #else