esp32/machine_uart: Correctly manage UART queue and event task.

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 <[email protected]>

Author: DvdGiessen

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;