From 52bc935fa7aa9e5c94f75fec8b2fa746c83f0be7 Mon Sep 17 00:00:00 2001 From: Scott Shawcroft Date: Thu, 25 Feb 2021 16:50:57 -0800 Subject: [PATCH] A few minor fixes for corner cases * Always clear the peripheral interrupt so we don't hang when full * Store the ringbuf in the object so it gets collected when we're alive * Make UART objects have a finaliser so they are deinit when their memory is freed * Copy bytes into the ringbuf from the FIFO after we read to ensure the interrupt is enabled ASAP * Copy bytes into the ringbuf from the FIFO before measuring our rx available because the interrupt is based on a threshold (not > 0). For example, a single byte won't trigger an interrupt. --- ports/raspberrypi/common-hal/busio/UART.c | 57 +++++++++++++++++------ ports/raspberrypi/common-hal/busio/UART.h | 1 + py/gc.c | 2 +- py/malloc.c | 6 +-- py/misc.h | 8 ++-- shared-bindings/busio/UART.c | 3 +- tests/manual/busio/uart_echo.py | 18 +++++++ 7 files changed, 72 insertions(+), 23 deletions(-) create mode 100644 tests/manual/busio/uart_echo.py diff --git a/ports/raspberrypi/common-hal/busio/UART.c b/ports/raspberrypi/common-hal/busio/UART.c index 1f0f4e2f6f..1bc1211777 100644 --- a/ports/raspberrypi/common-hal/busio/UART.c +++ b/ports/raspberrypi/common-hal/busio/UART.c @@ -73,18 +73,27 @@ static uint8_t pin_init(const uint8_t uart, const mcu_pin_obj_t * pin, const uin return pin->number; } -static ringbuf_t ringbuf[NUM_UARTS]; +static busio_uart_obj_t* active_uarts[NUM_UARTS]; -static void uart0_callback(void) { - while (uart_is_readable(uart0) && ringbuf_num_empty(&ringbuf[0]) > 0) { - ringbuf_put(&ringbuf[0], (uint8_t)uart_get_hw(uart0)->dr); +static void _copy_into_ringbuf(ringbuf_t* r, uart_inst_t* uart) { + while (uart_is_readable(uart) && ringbuf_num_empty(r) > 0) { + ringbuf_put(r, (uint8_t) uart_get_hw(uart)->dr); } } +static void shared_callback(busio_uart_obj_t *self) { + _copy_into_ringbuf(&self->ringbuf, self->uart); + // We always clear the interrupt so it doesn't continue to fire because we + // may not have read everything available. + uart_get_hw(self->uart)->icr = UART_UARTICR_RXIC_BITS; +} + +static void uart0_callback(void) { + shared_callback(active_uarts[0]); +} + static void uart1_callback(void) { - while (uart_is_readable(uart1) && ringbuf_num_empty(&ringbuf[1]) > 0) { - ringbuf_put(&ringbuf[1], (uint8_t)uart_get_hw(uart1)->dr); - } + shared_callback(active_uarts[1]); } void common_hal_busio_uart_construct(busio_uart_obj_t *self, @@ -138,9 +147,10 @@ void common_hal_busio_uart_construct(busio_uart_obj_t *self, // pointers like this are NOT moved, allocating the buffer // in the long-lived pool is not strictly necessary) // (This is a macro.) - if (!ringbuf_alloc(&ringbuf[uart_id], receiver_buffer_size, true)) { + if (!ringbuf_alloc(&self->ringbuf, receiver_buffer_size, true)) { mp_raise_msg(&mp_type_MemoryError, translate("Failed to allocate RX buffer")); } + active_uarts[uart_id] = self; if (uart_id == 1) { self->uart_irq_id = UART1_IRQ; irq_set_exclusive_handler(self->uart_irq_id, uart1_callback); @@ -149,7 +159,7 @@ void common_hal_busio_uart_construct(busio_uart_obj_t *self, irq_set_exclusive_handler(self->uart_irq_id, uart0_callback); } irq_set_enabled(self->uart_irq_id, true); - uart_set_irq_enables(self->uart, true, false); + uart_set_irq_enables(self->uart, true /* rx has data */, false /* tx needs data */); } } @@ -162,7 +172,8 @@ void common_hal_busio_uart_deinit(busio_uart_obj_t *self) { return; } uart_deinit(self->uart); - ringbuf_free(&ringbuf[self->uart_id]); + ringbuf_free(&self->ringbuf); + active_uarts[self->uart_id] = NULL; uart_status[self->uart_id] = STATUS_FREE; reset_pin_number(self->tx_pin); reset_pin_number(self->rx_pin); @@ -208,7 +219,7 @@ size_t common_hal_busio_uart_read(busio_uart_obj_t *self, uint8_t *data, size_t irq_set_enabled(self->uart_irq_id, false); // Copy as much received data as available, up to len bytes. - size_t total_read = ringbuf_get_n(&ringbuf[self->uart_id], data, len); + size_t total_read = ringbuf_get_n(&self->ringbuf, data, len); // Check if we still need to read more data. if (len > total_read) { @@ -218,7 +229,7 @@ size_t common_hal_busio_uart_read(busio_uart_obj_t *self, uint8_t *data, size_t while (len > 0 && (supervisor_ticks_ms64() - start_ticks < self->timeout_ms)) { if (uart_is_readable(self->uart)) { // Read and advance. - *data++ = uart_get_hw(self->uart)->dr; + data[total_read] = uart_get_hw(self->uart)->dr; // Adjust the counters. len--; @@ -230,11 +241,16 @@ size_t common_hal_busio_uart_read(busio_uart_obj_t *self, uint8_t *data, size_t RUN_BACKGROUND_TASKS; // Allow user to break out of a timeout with a KeyboardInterrupt. if (mp_hal_is_interrupted()) { - return 0; + break; } } } + // Now that we've emptied the ringbuf some, fill it up with anything in the + // FIFO. This ensures that we'll empty the FIFO as much as possible and + // reset the interrupt when we catch up. + _copy_into_ringbuf(&self->ringbuf, self->uart); + // Re-enable irq. irq_set_enabled(self->uart_irq_id, true); @@ -264,13 +280,24 @@ void common_hal_busio_uart_set_timeout(busio_uart_obj_t *self, mp_float_t timeou } uint32_t common_hal_busio_uart_rx_characters_available(busio_uart_obj_t *self) { - return ringbuf_num_filled(&ringbuf[self->uart_id]); + // Prevent conflict with uart irq. + irq_set_enabled(self->uart_irq_id, false); + // The UART only interrupts after a threshold so make sure to copy anything + // out of its FIFO before measuring how many bytes we've received. + _copy_into_ringbuf(&self->ringbuf, self->uart); + irq_set_enabled(self->uart_irq_id, false); + return ringbuf_num_filled(&self->ringbuf); } void common_hal_busio_uart_clear_rx_buffer(busio_uart_obj_t *self) { // Prevent conflict with uart irq. irq_set_enabled(self->uart_irq_id, false); - ringbuf_clear(&ringbuf[self->uart_id]); + ringbuf_clear(&self->ringbuf); + + // Throw away the FIFO contents too. + while (uart_is_readable(self->uart)) { + (void) uart_get_hw(self->uart)->dr; + } irq_set_enabled(self->uart_irq_id, true); } diff --git a/ports/raspberrypi/common-hal/busio/UART.h b/ports/raspberrypi/common-hal/busio/UART.h index da6170596b..4c07de240b 100644 --- a/ports/raspberrypi/common-hal/busio/UART.h +++ b/ports/raspberrypi/common-hal/busio/UART.h @@ -43,6 +43,7 @@ typedef struct { uint32_t baudrate; uint32_t timeout_ms; uart_inst_t * uart; + ringbuf_t ringbuf; } busio_uart_obj_t; extern void reset_uart(void); diff --git a/py/gc.c b/py/gc.c index 69327060f7..2aa2668dc4 100755 --- a/py/gc.c +++ b/py/gc.c @@ -192,7 +192,7 @@ void gc_init(void *start, void *end) { } void gc_deinit(void) { - // Run any finalizers before we stop using the heap. + // Run any finalisers before we stop using the heap. gc_sweep_all(); MP_STATE_MEM(gc_pool_start) = 0; diff --git a/py/malloc.c b/py/malloc.c index 8d5141ee04..c923e89a65 100644 --- a/py/malloc.c +++ b/py/malloc.c @@ -57,7 +57,7 @@ #undef free #undef realloc #define malloc_ll(b, ll) gc_alloc((b), false, (ll)) -#define malloc_with_finaliser(b) gc_alloc((b), true, false) +#define malloc_with_finaliser(b, ll) gc_alloc((b), true, (ll)) #define free gc_free #define realloc(ptr, n) gc_realloc(ptr, n, true) #define realloc_ext(ptr, n, mv) gc_realloc(ptr, n, mv) @@ -103,8 +103,8 @@ void *m_malloc_maybe(size_t num_bytes, bool long_lived) { } #if MICROPY_ENABLE_FINALISER -void *m_malloc_with_finaliser(size_t num_bytes) { - void *ptr = malloc_with_finaliser(num_bytes); +void *m_malloc_with_finaliser(size_t num_bytes, bool long_lived) { + void *ptr = malloc_with_finaliser(num_bytes, long_lived); if (ptr == NULL && num_bytes != 0) { m_malloc_fail(num_bytes); } diff --git a/py/misc.h b/py/misc.h index 6ed256e705..0fef7e249e 100644 --- a/py/misc.h +++ b/py/misc.h @@ -72,11 +72,13 @@ typedef unsigned int uint; #define m_new_obj_var_maybe(obj_type, var_type, var_num) ((obj_type*)m_malloc_maybe(sizeof(obj_type) + sizeof(var_type) * (var_num), false)) #define m_new_ll_obj_var_maybe(obj_type, var_type, var_num) ((obj_type*)m_malloc_maybe(sizeof(obj_type) + sizeof(var_type) * (var_num), true)) #if MICROPY_ENABLE_FINALISER -#define m_new_obj_with_finaliser(type) ((type*)(m_malloc_with_finaliser(sizeof(type)))) -#define m_new_obj_var_with_finaliser(type, var_type, var_num) ((type*)m_malloc_with_finaliser(sizeof(type) + sizeof(var_type) * (var_num))) +#define m_new_obj_with_finaliser(type) ((type*)(m_malloc_with_finaliser(sizeof(type), false))) +#define m_new_obj_var_with_finaliser(type, var_type, var_num) ((type*)m_malloc_with_finaliser(sizeof(type) + sizeof(var_type) * (var_num), false)) +#define m_new_ll_obj_with_finaliser(type) ((type*)(m_malloc_with_finaliser(sizeof(type), true))) #else #define m_new_obj_with_finaliser(type) m_new_obj(type) #define m_new_obj_var_with_finaliser(type, var_type, var_num) m_new_obj_var(type, var_type, var_num) +#define m_new_ll_obj_with_finaliser(type) m_new_ll_obj(type) #endif #if MICROPY_MALLOC_USES_ALLOCATED_SIZE #define m_renew(type, ptr, old_num, new_num) ((type*)(m_realloc((ptr), sizeof(type) * (old_num), sizeof(type) * (new_num)))) @@ -93,7 +95,7 @@ typedef unsigned int uint; void *m_malloc(size_t num_bytes, bool long_lived); void *m_malloc_maybe(size_t num_bytes, bool long_lived); -void *m_malloc_with_finaliser(size_t num_bytes); +void *m_malloc_with_finaliser(size_t num_bytes, bool long_lived); void *m_malloc0(size_t num_bytes, bool long_lived); #if MICROPY_MALLOC_USES_ALLOCATED_SIZE void *m_realloc(void *ptr, size_t old_num_bytes, size_t new_num_bytes); diff --git a/shared-bindings/busio/UART.c b/shared-bindings/busio/UART.c index 50d233f2b9..2f8fc9c322 100644 --- a/shared-bindings/busio/UART.c +++ b/shared-bindings/busio/UART.c @@ -82,7 +82,7 @@ STATIC mp_obj_t busio_uart_make_new(const mp_obj_type_t *type, size_t n_args, co // This is needed to avoid crashes with certain UART implementations which // cannot accomodate being moved after creation. (See // https://github.com/adafruit/circuitpython/issues/1056) - busio_uart_obj_t *self = m_new_ll_obj(busio_uart_obj_t); + busio_uart_obj_t *self = m_new_ll_obj_with_finaliser(busio_uart_obj_t); self->base.type = &busio_uart_type; enum { ARG_tx, ARG_rx, ARG_baudrate, ARG_bits, ARG_parity, ARG_stop, ARG_timeout, ARG_receiver_buffer_size, ARG_rts, ARG_cts, ARG_rs485_dir,ARG_rs485_invert}; @@ -387,6 +387,7 @@ const mp_obj_type_t busio_uart_parity_type = { }; STATIC const mp_rom_map_elem_t busio_uart_locals_dict_table[] = { + { MP_ROM_QSTR(MP_QSTR___del__), MP_ROM_PTR(&busio_uart_deinit_obj) }, { MP_ROM_QSTR(MP_QSTR_deinit), MP_ROM_PTR(&busio_uart_deinit_obj) }, { MP_ROM_QSTR(MP_QSTR___enter__), MP_ROM_PTR(&default___enter___obj) }, { MP_ROM_QSTR(MP_QSTR___exit__), MP_ROM_PTR(&busio_uart___exit___obj) }, diff --git a/tests/manual/busio/uart_echo.py b/tests/manual/busio/uart_echo.py new file mode 100644 index 0000000000..f55d4db9ec --- /dev/null +++ b/tests/manual/busio/uart_echo.py @@ -0,0 +1,18 @@ +import busio +import board +import time + +i = 0 + +u = busio.UART(tx=board.TX, rx=board.RX) + +while True: + u.write(str(i).encode("utf-8")) + time.sleep(0.1) + print(i, u.in_waiting) # should be the number of digits + time.sleep(0.1) + print(i, u.in_waiting) # should be the number of digits + r = u.read(64 + 10) + print(i, u.in_waiting) # should be 0 + print(len(r), r) + i += 1