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.

Author: tannewt

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);
 }
 

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);

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;

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);
     }

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);

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) },

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