From 371db85c8e9c29ac66d772e051c7a167782e8407 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Wed, 6 Aug 2025 17:26:21 +1000 Subject: [PATCH] esp32/machine_uart: Convert machine.UART objects to static instances. Makes machine.UART objects static instances (similar to other ports), adds machine_uart_deinit_all() function for cleanup on soft reset. - Fixes the case where the OS-level uart_event_task is leaked (along with 2KB of heap) when a UART object is garbage collected (including after a soft reset). - Fixes hard fault if a previous UART irq() fires after the UART object is garbage collected (including after a soft reset), but before any new UART object is instantiated for the same UART number. - Constructing multiple UART objects for the same UART now returns the same instance multiple times, not different instances. - Was also able to streamline deinit/init to only install the driver once, rather than install-with-defaults/uninstall/reinstall. Alternative would be to add a finaliser, but this is more consistent with how most other UART objects are implemented. This work was funded through GitHub Sponsors. Signed-off-by: Angus Gratton --- ports/esp32/machine_uart.c | 224 ++++++++++++++++++++++--------------- ports/esp32/main.c | 3 + ports/esp32/modmachine.h | 1 + 3 files changed, 137 insertions(+), 91 deletions(-) diff --git a/ports/esp32/machine_uart.c b/ports/esp32/machine_uart.c index c4dab2cae5..55987c9b62 100644 --- a/ports/esp32/machine_uart.c +++ b/ports/esp32/machine_uart.c @@ -68,6 +68,9 @@ enum { RXIDLE_ALERT, }; +// RXIDLE irq feature uses this machine.Timer id +#define RXIDLE_TIMER_IDX 0 + typedef struct _machine_uart_obj_t { mp_obj_base_t base; uart_port_t uart_num; @@ -96,6 +99,14 @@ typedef struct _machine_uart_obj_t { static const char *_parity_name[] = {"None", "1", "0"}; +static bool uart_is_repl(uart_port_t uart_num) { + #if MICROPY_HW_ENABLE_UART_REPL + return uart_num == MICROPY_HW_UART_REPL; + #else + return false; + #endif +} + /******************************************************************************/ // MicroPython bindings for UART @@ -245,13 +256,67 @@ 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_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) { - mp_raise_ValueError(MP_ERROR_TEXT("UART buffer size is fixed")); + // If UART is being freshly initialised then restore object defaults + if (!uart_is_driver_installed(self->uart_num)) { + self->bits = 8; + self->parity = 0; + self->stop = 1; + self->rts = UART_PIN_NO_CHANGE; + self->cts = UART_PIN_NO_CHANGE; + self->txbuf = 256; + self->rxbuf = 256; // IDF minimum + self->timeout = 0; + self->timeout_char = 0; + self->invert = 0; + self->flowcontrol = 0; + self->uart_event_task = NULL; + self->uart_queue = NULL; + self->rxidle_state = RXIDLE_INACTIVE; + + // Set the MicroPython default UART pins. These may be overwritten with + // caller-provided pins, below + switch (self->uart_num) { + case UART_NUM_0: + self->rx = UART_PIN_NO_CHANGE; // GPIO 3 + self->tx = UART_PIN_NO_CHANGE; // GPIO 1 + break; + case UART_NUM_1: + self->rx = 9; + self->tx = 10; + break; + #if SOC_UART_HP_NUM > 2 + case UART_NUM_2: + self->rx = 16; + self->tx = 17; + break; + #endif + #if SOC_UART_LP_NUM >= 1 + case LP_UART_NUM_0: + self->rx = 4; + self->tx = 5; + break; + #endif + case UART_NUM_MAX: + assert(0); // Range is checked in mp_machine_uart_make_new, value should be unreachable + } + } + + // Default driver parameters, should correspond to values set above + uart_config_t uartcfg = { + .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, + .rx_flow_ctrl_thresh = 0, + .source_clk = UART_SOURCE_CLK, + }; + + + 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)) { + if (uart_is_repl(self->uart_num)) { + mp_raise_ValueError(MP_ERROR_TEXT("REPL UART buffer size is fixed")); } - #endif if (args[ARG_txbuf].u_int >= 0) { self->txbuf = args[ARG_txbuf].u_int; @@ -259,18 +324,26 @@ static void mp_machine_uart_init_helper(machine_uart_obj_t *self, size_t n_args, if (args[ARG_rxbuf].u_int >= 0) { self->rxbuf = args[ARG_rxbuf].u_int; } - uart_config_t uartcfg = { - .flow_ctrl = UART_HW_FLOWCTRL_DISABLE, - .rx_flow_ctrl_thresh = 0, - .source_clk = UART_SOURCE_CLK, - }; - uint32_t baudrate; - check_esp_err(uart_get_baudrate(self->uart_num, &baudrate)); - uartcfg.baud_rate = baudrate; - 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)); - mp_machine_uart_deinit(self); + + // must reinitialise driver to change the tx/rx buffer size + if (uart_is_driver_installed(self->uart_num)) { + // Update uartcfg with the current uart parameters + uint32_t baudrate; + check_esp_err(uart_get_baudrate(self->uart_num, &baudrate)); + uartcfg.baud_rate = baudrate; + 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)); + // De-initialise the driver + mp_machine_uart_deinit(self); + } + } + + if (!uart_is_repl(self->uart_num) && !uart_is_driver_installed(self->uart_num)) { + // install the driver if needed - could be uninstalled for multiple reasons: + // 1. First time initialising this UART + // 2. Has been temporarily de-initialised to change the buffer size + // 3. Python code called .deinit() and now .init() check_esp_err(uart_param_config(self->uart_num, &uartcfg)); 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) { @@ -399,6 +472,9 @@ static void mp_machine_uart_init_helper(machine_uart_obj_t *self, size_t n_args, } uint8_t uart_fifo_len = UART_HW_FIFO_LEN(self->uart_num); check_esp_err(uart_set_hw_flow_ctrl(self->uart_num, self->flowcontrol, uart_fifo_len - uart_fifo_len / 4)); + + // discard any input from previous configuration + check_esp_err(uart_flush_input(self->uart_num)); } static mp_obj_t mp_machine_uart_make_new(const mp_obj_type_t *type, size_t n_args, size_t n_kw, const mp_obj_t *args) { @@ -409,73 +485,14 @@ static mp_obj_t mp_machine_uart_make_new(const mp_obj_type_t *type, size_t n_arg if (uart_num < 0 || uart_num >= UART_NUM_MAX) { mp_raise_msg_varg(&mp_type_ValueError, MP_ERROR_TEXT("UART(%d) does not exist"), uart_num); } + _Static_assert(UART_NUM_MAX == SOC_UART_NUM, "SOC and driver constant should match"); - // Defaults - uart_config_t uartcfg = { - .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, - .rx_flow_ctrl_thresh = 0, - .source_clk = UART_SOURCE_CLK, - }; - - // create instance - machine_uart_obj_t *self = mp_obj_malloc(machine_uart_obj_t, &machine_uart_type); - self->uart_num = uart_num; - self->bits = 8; - self->parity = 0; - self->stop = 1; - self->rts = UART_PIN_NO_CHANGE; - self->cts = UART_PIN_NO_CHANGE; - self->txbuf = 256; - self->rxbuf = 256; // IDF minimum - self->timeout = 0; - self->timeout_char = 0; - self->invert = 0; - self->flowcontrol = 0; - self->uart_event_task = NULL; - self->uart_queue = NULL; - self->rxidle_state = RXIDLE_INACTIVE; - - switch (uart_num) { - case UART_NUM_0: - self->rx = UART_PIN_NO_CHANGE; // GPIO 3 - self->tx = UART_PIN_NO_CHANGE; // GPIO 1 - break; - case UART_NUM_1: - self->rx = 9; - self->tx = 10; - break; - #if SOC_UART_HP_NUM > 2 - case UART_NUM_2: - self->rx = 16; - self->tx = 17; - break; - #endif - #if SOC_UART_LP_NUM >= 1 - case LP_UART_NUM_0: - self->rx = 4; - self->tx = 5; - #endif - - } - - #if MICROPY_HW_ENABLE_UART_REPL - // Only reset the driver if it's not the REPL UART. - if (uart_num != MICROPY_HW_UART_REPL) - #endif - { - // 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, UART_QUEUE_SIZE, &self->uart_queue, 0)); + machine_uart_obj_t *self = MP_STATE_PORT(machine_uart_objs)[uart_num]; + if (self == NULL) { + // Allocate a new UART object + self = mp_obj_malloc(machine_uart_obj_t, &machine_uart_type); + self->uart_num = uart_num; + // remaining fields are set from inside mp_machine_uart_init_helper() } mp_map_t kw_args; @@ -485,6 +502,8 @@ static mp_obj_t mp_machine_uart_make_new(const mp_obj_type_t *type, size_t n_arg // Make sure pins are connected. check_esp_err(uart_set_pin(self->uart_num, self->tx, self->rx, self->rts, self->cts)); + MP_STATE_PORT(machine_uart_objs)[uart_num] = self; + return MP_OBJ_FROM_PTR(self); } @@ -493,8 +512,31 @@ static void mp_machine_uart_deinit(machine_uart_obj_t *self) { vTaskDelete(self->uart_event_task); self->uart_event_task = NULL; } - check_esp_err(uart_driver_delete(self->uart_num)); - self->uart_queue = NULL; + if (self->rxidle_timer != NULL) { + machine_timer_disable(self->rxidle_timer); + if (self->rxidle_state > RXIDLE_STANDBY) { + // Currently deinit(),init() sequence resumes any previously + // configured irqs, and we currently also rely on this when changing + // buffer size - so keep rxidle in standby if it was active + self->rxidle_state = RXIDLE_STANDBY; + } + } + // Only stop the ESP-IDF driver entirely if it's not the REPL, + // (if it's the REPL then it shouldn't have any ISR configured anyway.) + if (!uart_is_repl(self->uart_num)) { + check_esp_err(uart_driver_delete(self->uart_num)); + self->uart_queue = NULL; + } +} + +void machine_uart_deinit_all(void) { + for (int i = 0; i < UART_NUM_MAX; i++) { + machine_uart_obj_t *uart = MP_STATE_PORT(machine_uart_objs)[i]; + if (uart) { + mp_machine_uart_deinit(uart); + MP_STATE_PORT(machine_uart_objs)[i] = NULL; + } + } } static mp_int_t mp_machine_uart_any(machine_uart_obj_t *self) { @@ -574,11 +616,9 @@ 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")); + if (uart_is_repl(self->uart_num)) { + mp_raise_ValueError(MP_ERROR_TEXT("REPL UART does not support IRQs")); } - #endif if (self->mp_irq_obj == NULL) { self->mp_irq_trigger = 0; @@ -605,7 +645,7 @@ static mp_irq_obj_t *mp_machine_uart_irq(machine_uart_obj_t *self, bool any_args } self->mp_irq_obj->ishard = false; self->mp_irq_trigger = trigger; - self->rxidle_timer = machine_timer_create(0); + self->rxidle_timer = machine_timer_create(RXIDLE_TIMER_IDX); uart_irq_configure_timer(self, trigger); // Start a task for handling events @@ -700,3 +740,5 @@ static mp_uint_t mp_machine_uart_ioctl(mp_obj_t self_in, mp_uint_t request, uint } return ret; } + +MP_REGISTER_ROOT_POINTER(void *machine_uart_objs[SOC_UART_NUM]); diff --git a/ports/esp32/main.c b/ports/esp32/main.c index 1523e07f91..bd5775bc66 100644 --- a/ports/esp32/main.c +++ b/ports/esp32/main.c @@ -180,6 +180,9 @@ soft_reset_exit: MP_STATE_PORT(espnow_singleton) = NULL; #endif + // Deinit uart before timers, as esp32 uart + // depends on a timer instance + machine_uart_deinit_all(); machine_timer_deinit_all(); #if MICROPY_PY_ESP32_PCNT diff --git a/ports/esp32/modmachine.h b/ports/esp32/modmachine.h index 9a5e6a566e..12e0d68002 100644 --- a/ports/esp32/modmachine.h +++ b/ports/esp32/modmachine.h @@ -20,6 +20,7 @@ void machine_pins_deinit(void); void machine_pwm_deinit_all(void); // TODO: void machine_rmt_deinit_all(void); void machine_timer_deinit_all(void); +void machine_uart_deinit_all(void); void machine_i2s_init0(); #endif // MICROPY_INCLUDED_ESP32_MODMACHINE_H