From 155fa94fbf6afda9dbb3b8508c093648b5bffa09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20van=20de=20Giessen?= Date: Thu, 6 Mar 2025 18:52:54 +0100 Subject: [PATCH] esp32/machine_uart: Correctly manage UART queue and event task. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the driver was reinitialised while there was already an event task running the queue that task is trying to receive from would be deleted, causing it to try to take a lock that no longer existed and deadlocking the CPU. This change ensures the task is always shut down before recreating the queue and recreates the task afterwards. It also allows setting an IRQ handler before the UART is initialized (like other ports allow), removes the task when the UART is deinitialized (which was previously missing), adds a check that no event task can be started when no queue exists, and adds a check to prevent reinitialising the UART driver unnecessarily. Signed-off-by: Daniël van de Giessen --- ports/esp32/machine_uart.c | 39 ++++++++++++++++++++++++++++++-------- 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/ports/esp32/machine_uart.c b/ports/esp32/machine_uart.c index 73089ef46..e5857e894 100644 --- a/ports/esp32/machine_uart.c +++ b/ports/esp32/machine_uart.c @@ -59,6 +59,7 @@ #define UART_IRQ_BREAK (1 << UART_BREAK) #define MP_UART_ALLOWED_FLAGS (UART_IRQ_RX | UART_IRQ_RXIDLE | UART_IRQ_BREAK) #define RXIDLE_TIMER_MIN (5000) // 500 us +#define UART_QUEUE_SIZE (3) enum { RXIDLE_INACTIVE, @@ -174,6 +175,13 @@ static void uart_event_task(void *self_in) { } } +static inline void uart_event_task_create(machine_uart_obj_t *self) { + if (xTaskCreatePinnedToCore(uart_event_task, "uart_event_task", 2048, self, + ESP_TASKD_EVENT_PRIO, (TaskHandle_t *)&self->uart_event_task, MP_TASK_COREID) != pdPASS) { + mp_raise_msg(&mp_type_RuntimeError, MP_ERROR_TEXT("failed to create UART event task")); + } +} + static void mp_machine_uart_print(const mp_print_t *print, mp_obj_t self_in, mp_print_kind_t kind) { machine_uart_obj_t *self = MP_OBJ_TO_PTR(self_in); uint32_t baudrate; @@ -250,7 +258,7 @@ static void mp_machine_uart_init_helper(machine_uart_obj_t *self, size_t n_args, // wait for all data to be transmitted before changing settings uart_wait_tx_done(self->uart_num, pdMS_TO_TICKS(1000)); - if (args[ARG_txbuf].u_int >= 0 || args[ARG_rxbuf].u_int >= 0) { + if ((args[ARG_txbuf].u_int >= 0 && args[ARG_txbuf].u_int != self->txbuf) || (args[ARG_rxbuf].u_int >= 0 && args[ARG_rxbuf].u_int != self->rxbuf)) { // must reinitialise driver to change the tx/rx buffer size #if MICROPY_HW_ENABLE_UART_REPL if (self->uart_num == MICROPY_HW_UART_REPL) { @@ -275,9 +283,12 @@ static void mp_machine_uart_init_helper(machine_uart_obj_t *self, size_t n_args, check_esp_err(uart_get_word_length(self->uart_num, &uartcfg.data_bits)); check_esp_err(uart_get_parity(self->uart_num, &uartcfg.parity)); check_esp_err(uart_get_stop_bits(self->uart_num, &uartcfg.stop_bits)); - check_esp_err(uart_driver_delete(self->uart_num)); + mp_machine_uart_deinit(self); check_esp_err(uart_param_config(self->uart_num, &uartcfg)); - check_esp_err(uart_driver_install(self->uart_num, self->rxbuf, self->txbuf, 0, NULL, 0)); + check_esp_err(uart_driver_install(self->uart_num, self->rxbuf, self->txbuf, UART_QUEUE_SIZE, &self->uart_queue, 0)); + if (self->mp_irq_obj != NULL && self->mp_irq_obj->handler != mp_const_none) { + uart_event_task_create(self); + } } // set baudrate @@ -437,7 +448,8 @@ static mp_obj_t mp_machine_uart_make_new(const mp_obj_type_t *type, size_t n_arg self->timeout_char = 0; self->invert = 0; self->flowcontrol = 0; - self->uart_event_task = 0; + self->uart_event_task = NULL; + self->uart_queue = NULL; self->rxidle_state = RXIDLE_INACTIVE; switch (uart_num) { @@ -470,12 +482,13 @@ static mp_obj_t mp_machine_uart_make_new(const mp_obj_type_t *type, size_t n_arg { // Remove any existing configuration check_esp_err(uart_driver_delete(self->uart_num)); + self->uart_queue = NULL; // init the peripheral // Setup check_esp_err(uart_param_config(self->uart_num, &uartcfg)); - check_esp_err(uart_driver_install(uart_num, self->rxbuf, self->txbuf, 3, &self->uart_queue, 0)); + check_esp_err(uart_driver_install(uart_num, self->rxbuf, self->txbuf, UART_QUEUE_SIZE, &self->uart_queue, 0)); } mp_map_t kw_args; @@ -489,7 +502,12 @@ static mp_obj_t mp_machine_uart_make_new(const mp_obj_type_t *type, size_t n_arg } static void mp_machine_uart_deinit(machine_uart_obj_t *self) { + if (self->uart_event_task != NULL) { + vTaskDelete(self->uart_event_task); + self->uart_event_task = NULL; + } check_esp_err(uart_driver_delete(self->uart_num)); + self->uart_queue = NULL; } static mp_int_t mp_machine_uart_any(machine_uart_obj_t *self) { @@ -568,6 +586,12 @@ static const mp_irq_methods_t uart_irq_methods = { }; static mp_irq_obj_t *mp_machine_uart_irq(machine_uart_obj_t *self, bool any_args, mp_arg_val_t *args) { + #if MICROPY_HW_ENABLE_UART_REPL + if (self->uart_num == MICROPY_HW_UART_REPL) { + mp_raise_ValueError(MP_ERROR_TEXT("UART does not support IRQs")); + } + #endif + if (self->mp_irq_obj == NULL) { self->mp_irq_trigger = 0; self->mp_irq_obj = mp_irq_new(&uart_irq_methods, MP_OBJ_FROM_PTR(self)); @@ -597,9 +621,8 @@ static mp_irq_obj_t *mp_machine_uart_irq(machine_uart_obj_t *self, bool any_args uart_irq_configure_timer(self, trigger); // Start a task for handling events - if (handler != mp_const_none && self->uart_event_task == NULL) { - xTaskCreatePinnedToCore(uart_event_task, "uart_event_task", 2048, self, - ESP_TASKD_EVENT_PRIO, (TaskHandle_t *)&self->uart_event_task, MP_TASK_COREID); + if (handler != mp_const_none && self->uart_event_task == NULL && self->uart_queue != NULL) { + uart_event_task_create(self); } else if (handler == mp_const_none && self->uart_event_task != NULL) { vTaskDelete(self->uart_event_task); self->uart_event_task = NULL;