From ea15a9118ad50dabc29934d7b08261ee2da51128 Mon Sep 17 00:00:00 2001
From: Dan Halbert <halbert@halwitz.org>
Date: Fri, 16 Sep 2022 18:46:02 -0400
Subject: [PATCH 1/2] ringbuf cleanup

---
 .../common-hal/_bleio/CharacteristicBuffer.c  |   1 +
 .../ble_hci/common-hal/_bleio/PacketBuffer.c  |   5 +-
 ports/broadcom/common-hal/busio/UART.c        |   5 +-
 ports/broadcom/common-hal/busio/UART.h        |   4 +-
 .../common-hal/_bleio/CharacteristicBuffer.c  |   7 +-
 .../common-hal/_bleio/PacketBuffer.c          |  13 +-
 .../common-hal/_bleio/CharacteristicBuffer.c  |   6 +-
 ports/nrf/common-hal/_bleio/PacketBuffer.c    |  11 +-
 ports/nrf/common-hal/busio/UART.c             |  22 ++--
 ports/nrf/common-hal/busio/UART.h             |   1 -
 ports/raspberrypi/common-hal/busio/UART.c     |  44 ++++---
 ports/stm/common-hal/busio/UART.c             |  11 +-
 ports/unix/coverage.c                         |  24 ++--
 py/ringbuf.c                                  | 120 +++++++-----------
 py/ringbuf.h                                  |  23 ++--
 15 files changed, 136 insertions(+), 161 deletions(-)

diff --git a/devices/ble_hci/common-hal/_bleio/CharacteristicBuffer.c b/devices/ble_hci/common-hal/_bleio/CharacteristicBuffer.c
index 0d87f03ae3..5c8c659e36 100644
--- a/devices/ble_hci/common-hal/_bleio/CharacteristicBuffer.c
+++ b/devices/ble_hci/common-hal/_bleio/CharacteristicBuffer.c
@@ -93,6 +93,7 @@ bool common_hal_bleio_characteristic_buffer_deinited(bleio_characteristic_buffer
 void common_hal_bleio_characteristic_buffer_deinit(bleio_characteristic_buffer_obj_t *self) {
     if (!common_hal_bleio_characteristic_buffer_deinited(self)) {
         bleio_characteristic_clear_observer(self->characteristic);
+        ringbuf_deinit(&self->ringbuf);
     }
 }
 
diff --git a/devices/ble_hci/common-hal/_bleio/PacketBuffer.c b/devices/ble_hci/common-hal/_bleio/PacketBuffer.c
index 7380d7ed4f..b690909029 100644
--- a/devices/ble_hci/common-hal/_bleio/PacketBuffer.c
+++ b/devices/ble_hci/common-hal/_bleio/PacketBuffer.c
@@ -37,13 +37,13 @@
 #include "supervisor/shared/tick.h"
 
 STATIC void write_to_ringbuf(bleio_packet_buffer_obj_t *self, uint8_t *data, uint16_t len) {
-    if (len + sizeof(uint16_t) > ringbuf_capacity(&self->ringbuf)) {
+    if (len + sizeof(uint16_t) > ringbuf_size(&self->ringbuf)) {
         // This shouldn't happen.
         return;
     }
     // Push all the data onto the ring buffer.
     // Make room for the new value by dropping the oldest packets first.
-    while (ringbuf_capacity(&self->ringbuf) - ringbuf_num_filled(&self->ringbuf) < len + sizeof(uint16_t)) {
+    while (ringbuf_size(&self->ringbuf) - ringbuf_num_filled(&self->ringbuf) < len + sizeof(uint16_t)) {
         uint16_t packet_length;
         ringbuf_get_n(&self->ringbuf, (uint8_t *)&packet_length, sizeof(uint16_t));
         for (uint16_t i = 0; i < packet_length; i++) {
@@ -264,5 +264,6 @@ bool common_hal_bleio_packet_buffer_deinited(bleio_packet_buffer_obj_t *self) {
 void common_hal_bleio_packet_buffer_deinit(bleio_packet_buffer_obj_t *self) {
     if (!common_hal_bleio_packet_buffer_deinited(self)) {
         bleio_characteristic_clear_observer(self->characteristic);
+        ringbuf_deinit(&self->ringbuf);
     }
 }
diff --git a/ports/broadcom/common-hal/busio/UART.c b/ports/broadcom/common-hal/busio/UART.c
index 52d1b7a418..6d97b42c76 100644
--- a/ports/broadcom/common-hal/busio/UART.c
+++ b/ports/broadcom/common-hal/busio/UART.c
@@ -208,7 +208,9 @@ void common_hal_busio_uart_construct(busio_uart_obj_t *self,
     self->sigint_enabled = sigint_enabled;
 
     if (rx != NULL) {
+        // Use the provided buffer when given.
         if (receiver_buffer != NULL) {
+            ringbuf_init(&self->ringbuf, receiver_buffer, receiver_buffer_size);
             self->ringbuf = (ringbuf_t) { receiver_buffer, receiver_buffer_size };
         } else {
             // Initially allocate the UART's buffer in the long-lived part of the
@@ -217,7 +219,6 @@ void common_hal_busio_uart_construct(busio_uart_obj_t *self,
             // self->buffer, so do it manually.  (However, as long as internal
             // 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(&self->ringbuf, receiver_buffer_size, true)) {
                 m_malloc_fail(receiver_buffer_size);
             }
@@ -337,7 +338,7 @@ void common_hal_busio_uart_deinit(busio_uart_obj_t *self) {
         pl011->CR = 0;
     }
     active_uart[self->uart_id] = NULL;
-    ringbuf_free(&self->ringbuf);
+    ringbuf_deinit(&self->ringbuf);
     uart_status[self->uart_id] = STATUS_FREE;
     common_hal_reset_pin(self->tx_pin);
     common_hal_reset_pin(self->rx_pin);
diff --git a/ports/broadcom/common-hal/busio/UART.h b/ports/broadcom/common-hal/busio/UART.h
index 0590bc28b6..57926ca58b 100644
--- a/ports/broadcom/common-hal/busio/UART.h
+++ b/ports/broadcom/common-hal/busio/UART.h
@@ -36,11 +36,11 @@ typedef struct {
     const mcu_pin_obj_t *rx_pin;
     const mcu_pin_obj_t *cts_pin;
     const mcu_pin_obj_t *rts_pin;
-    uint8_t uart_id;
     uint32_t baudrate;
     uint32_t timeout_ms;
-    bool sigint_enabled;
     ringbuf_t ringbuf;
+    bool sigint_enabled;
+    uint8_t uart_id;
 } busio_uart_obj_t;
 
 extern void reset_uart(void);
diff --git a/ports/espressif/common-hal/_bleio/CharacteristicBuffer.c b/ports/espressif/common-hal/_bleio/CharacteristicBuffer.c
index fc3d0bbf06..48be3abd67 100644
--- a/ports/espressif/common-hal/_bleio/CharacteristicBuffer.c
+++ b/ports/espressif/common-hal/_bleio/CharacteristicBuffer.c
@@ -72,11 +72,7 @@ void _common_hal_bleio_characteristic_buffer_construct(bleio_characteristic_buff
     void *static_handler_entry) {
     self->characteristic = characteristic;
     self->timeout_ms = timeout * 1000;
-
-    self->ringbuf.buf = (uint8_t *)buffer;
-    self->ringbuf.size = buffer_size;
-    self->ringbuf.iget = 0;
-    self->ringbuf.iput = 0;
+    ringbuf_init(&self->ringbuf, buffer, buffer_size);
 
     if (static_handler_entry != NULL) {
         ble_event_add_handler_entry((ble_event_handler_entry_t *)static_handler_entry, characteristic_buffer_on_ble_evt, self);
@@ -131,6 +127,7 @@ void common_hal_bleio_characteristic_buffer_deinit(bleio_characteristic_buffer_o
     if (!common_hal_bleio_characteristic_buffer_deinited(self)) {
         ble_event_remove_handler(characteristic_buffer_on_ble_evt, self);
         self->characteristic = NULL;
+        ringbuf_deinit(&self->ringbuf);
     }
 }
 
diff --git a/ports/espressif/common-hal/_bleio/PacketBuffer.c b/ports/espressif/common-hal/_bleio/PacketBuffer.c
index 4e65bf309c..3b3e51df61 100644
--- a/ports/espressif/common-hal/_bleio/PacketBuffer.c
+++ b/ports/espressif/common-hal/_bleio/PacketBuffer.c
@@ -42,13 +42,13 @@
 
 STATIC void write_to_ringbuf(bleio_packet_buffer_obj_t *self, const struct os_mbuf *mbuf) {
     size_t len = OS_MBUF_PKTLEN(mbuf);
-    if (len + sizeof(uint16_t) > ringbuf_capacity(&self->ringbuf)) {
+    if (len + sizeof(uint16_t) > ringbuf_size(&self->ringbuf)) {
         // This shouldn't happen but can if our buffer size was much smaller than
         // the writes the client actually makes.
         return;
     }
     // Make room for the new value by dropping the oldest packets first.
-    while (ringbuf_capacity(&self->ringbuf) - ringbuf_num_filled(&self->ringbuf) < len + sizeof(uint16_t)) {
+    while (ringbuf_size(&self->ringbuf) - ringbuf_num_filled(&self->ringbuf) < len + sizeof(uint16_t)) {
         uint16_t packet_length;
         ringbuf_get_n(&self->ringbuf, (uint8_t *)&packet_length, sizeof(uint16_t));
         for (uint16_t i = 0; i < packet_length; i++) {
@@ -164,10 +164,7 @@ void _common_hal_bleio_packet_buffer_construct(
     }
 
     if (incoming) {
-        self->ringbuf.buf = (uint8_t *)incoming_buffer;
-        self->ringbuf.size = incoming_buffer_size;
-        self->ringbuf.iget = 0;
-        self->ringbuf.iput = 0;
+        ringbuf_init(&self->ringbuf, (uint8_t *)incoming_buffer, incoming_buffer_size);
     }
 
     self->packet_queued = false;
@@ -219,8 +216,7 @@ void common_hal_bleio_packet_buffer_construct(
     size_t incoming_buffer_size = 0;
     uint32_t *incoming_buffer = NULL;
     if (incoming) {
-        incoming_buffer_size = buffer_size * (sizeof(uint16_t) + max_packet_size);
-        incoming_buffer = m_malloc(incoming_buffer_size, false);
+        ringbuf_init(&self->ringbuf, (uint8_t *)incoming_buffer, incoming_buffer_size);
     }
 
     uint32_t *outgoing1 = NULL;
@@ -414,5 +410,6 @@ bool common_hal_bleio_packet_buffer_deinited(bleio_packet_buffer_obj_t *self) {
 void common_hal_bleio_packet_buffer_deinit(bleio_packet_buffer_obj_t *self) {
     if (!common_hal_bleio_packet_buffer_deinited(self)) {
         ble_event_remove_handler(packet_buffer_on_ble_client_evt, self);
+        ringbuf_deinit(&self->ringbuf);
     }
 }
diff --git a/ports/nrf/common-hal/_bleio/CharacteristicBuffer.c b/ports/nrf/common-hal/_bleio/CharacteristicBuffer.c
index 5772616ee5..d13ffa1a94 100644
--- a/ports/nrf/common-hal/_bleio/CharacteristicBuffer.c
+++ b/ports/nrf/common-hal/_bleio/CharacteristicBuffer.c
@@ -90,10 +90,7 @@ void _common_hal_bleio_characteristic_buffer_construct(bleio_characteristic_buff
     self->characteristic = characteristic;
     self->timeout_ms = timeout * 1000;
 
-    self->ringbuf.buf = (uint8_t *)buffer;
-    self->ringbuf.size = buffer_size;
-    self->ringbuf.iget = 0;
-    self->ringbuf.iput = 0;
+    ringbuf_init(&self->ringbuf, buffer, buffer_size);
 
     if (static_handler_entry != NULL) {
         ble_drv_add_event_handler_entry((ble_drv_evt_handler_entry_t *)static_handler_entry, characteristic_buffer_on_ble_evt, self);
@@ -159,6 +156,7 @@ void common_hal_bleio_characteristic_buffer_deinit(bleio_characteristic_buffer_o
     if (!common_hal_bleio_characteristic_buffer_deinited(self)) {
         ble_drv_remove_event_handler(characteristic_buffer_on_ble_evt, self);
         self->characteristic = NULL;
+        ringbuf_deinit(&self->ringbuf);
     }
 }
 
diff --git a/ports/nrf/common-hal/_bleio/PacketBuffer.c b/ports/nrf/common-hal/_bleio/PacketBuffer.c
index 38dcdd9041..52402a93f1 100644
--- a/ports/nrf/common-hal/_bleio/PacketBuffer.c
+++ b/ports/nrf/common-hal/_bleio/PacketBuffer.c
@@ -43,7 +43,7 @@
 #include "supervisor/shared/bluetooth/serial.h"
 
 STATIC void write_to_ringbuf(bleio_packet_buffer_obj_t *self, uint8_t *data, uint16_t len) {
-    if (len + sizeof(uint16_t) > ringbuf_capacity(&self->ringbuf)) {
+    if (len + sizeof(uint16_t) > ringbuf_size(&self->ringbuf)) {
         // This shouldn't happen but can if our buffer size was much smaller than
         // the writes the client actually makes.
         return;
@@ -52,7 +52,7 @@ STATIC void write_to_ringbuf(bleio_packet_buffer_obj_t *self, uint8_t *data, uin
     uint8_t is_nested_critical_region;
     sd_nvic_critical_region_enter(&is_nested_critical_region);
     // Make room for the new value by dropping the oldest packets first.
-    while (ringbuf_capacity(&self->ringbuf) - ringbuf_num_filled(&self->ringbuf) < len + sizeof(uint16_t)) {
+    while (ringbuf_size(&self->ringbuf) - ringbuf_num_filled(&self->ringbuf) < len + sizeof(uint16_t)) {
         uint16_t packet_length;
         ringbuf_get_n(&self->ringbuf, (uint8_t *)&packet_length, sizeof(uint16_t));
         for (uint16_t i = 0; i < packet_length; i++) {
@@ -233,10 +233,7 @@ void _common_hal_bleio_packet_buffer_construct(
     }
 
     if (incoming) {
-        self->ringbuf.buf = (uint8_t *)incoming_buffer;
-        self->ringbuf.size = incoming_buffer_size;
-        self->ringbuf.iget = 0;
-        self->ringbuf.iput = 0;
+        ringbuf_init(&self->ringbuf, (uint8_t *)incoming_buffer, incoming_buffer_size);
     }
 
     self->packet_queued = false;
@@ -502,7 +499,9 @@ bool common_hal_bleio_packet_buffer_deinited(bleio_packet_buffer_obj_t *self) {
 }
 
 void common_hal_bleio_packet_buffer_deinit(bleio_packet_buffer_obj_t *self) {
+
     if (!common_hal_bleio_packet_buffer_deinited(self)) {
         ble_drv_remove_event_handler(packet_buffer_on_ble_client_evt, self);
+        ringbuf_deinit(&self->ringbuf);
     }
 }
diff --git a/ports/nrf/common-hal/busio/UART.c b/ports/nrf/common-hal/busio/UART.c
index ae8389fd9e..df4a485999 100644
--- a/ports/nrf/common-hal/busio/UART.c
+++ b/ports/nrf/common-hal/busio/UART.c
@@ -213,24 +213,20 @@ void common_hal_busio_uart_construct(busio_uart_obj_t *self,
 
     // Init buffer for rx
     if (rx != NULL) {
-        self->allocated_ringbuf = true;
         // Use the provided buffer when given.
         if (receiver_buffer != NULL) {
-            self->ringbuf.buf = receiver_buffer;
-            self->ringbuf.size = receiver_buffer_size - 1;
-            self->ringbuf.iput = 0;
-            self->ringbuf.iget = 0;
-            self->allocated_ringbuf = false;
+            ringbuf_init(&self->ringbuf, receiver_buffer, receiver_buffer_size);
+        } else {
             // Initially allocate the UART's buffer in the long-lived part of the
             // heap.  UARTs are generally long-lived objects, but the "make long-
             // lived" machinery is incapable of moving internal pointers like
             // self->buffer, so do it manually.  (However, as long as internal
             // pointers like this are NOT moved, allocating the buffer
             // in the long-lived pool is not strictly necessary)
-            // (This is a macro.)
-        } else if (!ringbuf_alloc(&self->ringbuf, receiver_buffer_size, true)) {
-            nrfx_uarte_uninit(self->uarte);
-            m_malloc_fail(receiver_buffer_size);
+            if (!ringbuf_alloc(&self->ringbuf, receiver_buffer_size, true)) {
+                nrfx_uarte_uninit(self->uarte);
+                m_malloc_fail(receiver_buffer_size);
+            }
         }
 
         self->rx_pin_number = rx->number;
@@ -282,9 +278,7 @@ void common_hal_busio_uart_deinit(busio_uart_obj_t *self) {
         self->rx_pin_number = NO_PIN;
         self->rts_pin_number = NO_PIN;
         self->cts_pin_number = NO_PIN;
-        if (self->allocated_ringbuf) {
-            ringbuf_free(&self->ringbuf);
-        }
+        ringbuf_deinit(&self->ringbuf);
 
         for (size_t i = 0; i < MP_ARRAY_SIZE(nrfx_uartes); i++) {
             if (self->uarte == &nrfx_uartes[i]) {
@@ -305,7 +299,7 @@ size_t common_hal_busio_uart_read(busio_uart_obj_t *self, uint8_t *data, size_t
 
     // check removed to reduce code size
     /*
-    if (len > ringbuf_capacity(&self->ringbuf)) {
+    if (len > ringbuf_size(&self->ringbuf)) {
         mp_raise_ValueError(translate("Reading >receiver_buffer_size bytes is not supported"));
     }
     */
diff --git a/ports/nrf/common-hal/busio/UART.h b/ports/nrf/common-hal/busio/UART.h
index 140f8d0c0a..2eaf584403 100644
--- a/ports/nrf/common-hal/busio/UART.h
+++ b/ports/nrf/common-hal/busio/UART.h
@@ -44,7 +44,6 @@ typedef struct {
     ringbuf_t ringbuf;
     uint8_t rx_char;    // EasyDMA buf
     bool rx_paused;     // set by irq if no space in rbuf
-    bool allocated_ringbuf;
 
     uint8_t tx_pin_number;
     uint8_t rx_pin_number;
diff --git a/ports/raspberrypi/common-hal/busio/UART.c b/ports/raspberrypi/common-hal/busio/UART.c
index c06bb21903..4212249ec9 100644
--- a/ports/raspberrypi/common-hal/busio/UART.c
+++ b/ports/raspberrypi/common-hal/busio/UART.c
@@ -155,27 +155,33 @@ void common_hal_busio_uart_construct(busio_uart_obj_t *self,
     uart_set_hw_flow(self->uart, (cts != NULL), (rts != NULL));
 
     if (rx != NULL) {
-        // Initially allocate the UART's buffer in the long-lived part of the
-        // heap. UARTs are generally long-lived objects, but the "make long-
-        // lived" machinery is incapable of moving internal pointers like
-        // self->buffer, so do it manually.  (However, as long as internal
-        // 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(&self->ringbuf, receiver_buffer_size, true)) {
-            m_malloc_fail(receiver_buffer_size);
-        }
-        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);
+        // Use the provided buffer when given.
+        if (receiver_buffer != NULL) {
+            ringbuf_init(&self->ringbuf, receiver_buffer, receiver_buffer_size);
         } else {
-            self->uart_irq_id = UART0_IRQ;
-            irq_set_exclusive_handler(self->uart_irq_id, uart0_callback);
+            // Initially allocate the UART's buffer in the long-lived part of the
+            // heap. UARTs are generally long-lived objects, but the "make long-
+            // lived" machinery is incapable of moving internal pointers like
+            // self->buffer, so do it manually.  (However, as long as internal
+            // pointers like this are NOT moved, allocating the buffer
+            // in the long-lived pool is not strictly necessary)
+            if (!ringbuf_alloc(&self->ringbuf, receiver_buffer_size, true)) {
+                uart_deinit(self->uart);
+                m_malloc_fail(receiver_buffer_size);
+            }
         }
-        irq_set_enabled(self->uart_irq_id, true);
-        uart_set_irq_enables(self->uart, true /* rx has data */, false /* tx needs data */);
     }
+
+    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);
+    } else {
+        self->uart_irq_id = UART0_IRQ;
+        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 /* rx has data */, false /* tx needs data */);
 }
 
 bool common_hal_busio_uart_deinited(busio_uart_obj_t *self) {
@@ -187,7 +193,7 @@ void common_hal_busio_uart_deinit(busio_uart_obj_t *self) {
         return;
     }
     uart_deinit(self->uart);
-    ringbuf_free(&self->ringbuf);
+    ringbuf_deinit(&self->ringbuf);
     active_uarts[self->uart_id] = NULL;
     uart_status[self->uart_id] = STATUS_FREE;
     reset_pin_number(self->tx_pin);
diff --git a/ports/stm/common-hal/busio/UART.c b/ports/stm/common-hal/busio/UART.c
index 171e915dd9..48894db5f9 100644
--- a/ports/stm/common-hal/busio/UART.c
+++ b/ports/stm/common-hal/busio/UART.c
@@ -214,9 +214,16 @@ void common_hal_busio_uart_construct(busio_uart_obj_t *self,
 
     // Init buffer for rx and claim pins
     if (self->rx != NULL) {
+        // Use the provided buffer when given.
         if (receiver_buffer != NULL) {
-            self->ringbuf = (ringbuf_t) { receiver_buffer, receiver_buffer_size };
+            ringbuf_init(&self->ringbuf, receiver_buffer, receiver_buffer_size);
         } else {
+            // Initially allocate the UART's buffer in the long-lived part of the
+            // heap. UARTs are generally long-lived objects, but the "make long-
+            // lived" machinery is incapable of moving internal pointers like
+            // self->buffer, so do it manually.  (However, as long as internal
+            // pointers like this are NOT moved, allocating the buffer
+            // in the long-lived pool is not strictly necessary)
             if (!ringbuf_alloc(&self->ringbuf, receiver_buffer_size, true)) {
                 m_malloc_fail(receiver_buffer_size);
             }
@@ -281,7 +288,7 @@ void common_hal_busio_uart_deinit(busio_uart_obj_t *self) {
         self->rx = NULL;
     }
 
-    ringbuf_free(&self->ringbuf);
+    ringbuf_deinit(&self->ringbuf);
 }
 
 size_t common_hal_busio_uart_read(busio_uart_obj_t *self, uint8_t *data, size_t len, int *errcode) {
diff --git a/ports/unix/coverage.c b/ports/unix/coverage.c
index 179181dc83..0e4c6dbd48 100644
--- a/ports/unix/coverage.c
+++ b/ports/unix/coverage.c
@@ -526,7 +526,9 @@ STATIC mp_obj_t extra_coverage(void) {
 
     // ringbuf
     {
-        byte buf[100];
+        #define RINGBUF_SIZE 99
+
+        byte buf[RINGBUF_SIZE];
         ringbuf_t ringbuf;
         ringbuf_init(&ringbuf, &buf[0], sizeof(buf));
 
@@ -546,7 +548,7 @@ STATIC mp_obj_t extra_coverage(void) {
         mp_printf(&mp_plat_print, "%d %d\n", ringbuf_num_empty(&ringbuf), ringbuf_num_filled(&ringbuf));
 
         // Two-byte put with full ringbuf.
-        for (int i = 0; i < 99; ++i) {
+        for (int i = 0; i < RINGBUF_SIZE; ++i) {
             ringbuf_put(&ringbuf, i);
         }
         mp_printf(&mp_plat_print, "%d %d\n", ringbuf_num_empty(&ringbuf), ringbuf_num_filled(&ringbuf));
@@ -558,16 +560,15 @@ STATIC mp_obj_t extra_coverage(void) {
         ringbuf_get(&ringbuf);
         mp_printf(&mp_plat_print, "%d %d\n", ringbuf_num_empty(&ringbuf), ringbuf_num_filled(&ringbuf));
         mp_printf(&mp_plat_print, "%d\n", ringbuf_put16(&ringbuf, 0xcc99));
-        for (int i = 0; i < 97; ++i) {
+        for (int i = 0; i < RINGBUF_SIZE - 2; ++i) {
             ringbuf_get(&ringbuf);
         }
         mp_printf(&mp_plat_print, "%04x\n", ringbuf_get16(&ringbuf));
         mp_printf(&mp_plat_print, "%d %d\n", ringbuf_num_empty(&ringbuf), ringbuf_num_filled(&ringbuf));
 
         // Two-byte put with wrap around on first byte:
-        ringbuf.iput = 0;
-        ringbuf.iget = 0;
-        for (int i = 0; i < 99; ++i) {
+        ringbuf_clear(&ringbuf);
+        for (int i = 0; i < RINGBUF_SIZE; ++i) {
             ringbuf_put(&ringbuf, i);
             ringbuf_get(&ringbuf);
         }
@@ -575,9 +576,8 @@ STATIC mp_obj_t extra_coverage(void) {
         mp_printf(&mp_plat_print, "%04x\n", ringbuf_get16(&ringbuf));
 
         // Two-byte put with wrap around on second byte:
-        ringbuf.iput = 0;
-        ringbuf.iget = 0;
-        for (int i = 0; i < 98; ++i) {
+        ringbuf_clear(&ringbuf);
+        for (int i = 0; i < RINGBUF_SIZE - 1; ++i) {
             ringbuf_put(&ringbuf, i);
             ringbuf_get(&ringbuf);
         }
@@ -585,13 +585,11 @@ STATIC mp_obj_t extra_coverage(void) {
         mp_printf(&mp_plat_print, "%04x\n", ringbuf_get16(&ringbuf));
 
         // Two-byte get from empty ringbuf.
-        ringbuf.iput = 0;
-        ringbuf.iget = 0;
+        ringbuf_clear(&ringbuf);
         mp_printf(&mp_plat_print, "%d\n", ringbuf_get16(&ringbuf));
 
         // Two-byte get from ringbuf with one byte available.
-        ringbuf.iput = 0;
-        ringbuf.iget = 0;
+        ringbuf_clear(&ringbuf);
         ringbuf_put(&ringbuf, 0xaa);
         mp_printf(&mp_plat_print, "%d\n", ringbuf_get16(&ringbuf));
     }
diff --git a/py/ringbuf.c b/py/ringbuf.c
index fe47b50068..8a4cb33cbc 100644
--- a/py/ringbuf.c
+++ b/py/ringbuf.c
@@ -27,86 +27,85 @@
 
 #include "ringbuf.h"
 
-bool ringbuf_init(ringbuf_t *r, uint8_t *buf, size_t capacity) {
+bool ringbuf_init(ringbuf_t *r, uint8_t *buf, size_t size) {
     r->buf = buf;
-    r->size = capacity;
-    r->iget = r->iput = 0;
+    r->size = size;
+    r->used = 0;
+    r->next_read = 0;
+    r->next_write = 0;
     return r->buf != NULL;
 }
 
-// Dynamic initialization. This should be accessible from a root pointer.
-// capacity is the number of bytes the ring buffer can hold. The actual
-// size of the buffer is one greater than that, due to how the buffer
-// handles empty and full statuses.
-bool ringbuf_alloc(ringbuf_t *r, size_t capacity, bool long_lived) {
-    r->buf = gc_alloc(capacity + 1, false, long_lived);
-    r->size = capacity + 1;
-    r->iget = r->iput = 0;
-    return r->buf != NULL;
+// Dynamic initialization. This should be accessible from a root pointer..
+bool ringbuf_alloc(ringbuf_t *r, size_t size, bool long_lived) {
+    bool result = ringbuf_init(r, gc_alloc(size, false, long_lived), size);
+    return result;
 }
 
-void ringbuf_free(ringbuf_t *r) {
-    // Free buf by letting gc take care of it. If the VM has finished already,
+void ringbuf_deinit(ringbuf_t *r) {
+    // Free buf by doing nothing and letting gc take care of it. If the VM has finished already,
     // this will be safe.
     r->buf = (uint8_t *)NULL;
     r->size = 0;
     ringbuf_clear(r);
 }
 
-size_t ringbuf_capacity(ringbuf_t *r) {
-    return r->size - 1;
+size_t ringbuf_size(ringbuf_t *r) {
+    return r->size;
 }
 
-// Returns -1 if buffer is empty, else returns byte fetched.
+// Return -1 if buffer is empty, else return byte fetched.
 int ringbuf_get(ringbuf_t *r) {
-    if (r->iget == r->iput) {
+    if (r->used < 1) {
         return -1;
     }
-    uint8_t v = r->buf[r->iget++];
-    if (r->iget >= r->size) {
-        r->iget = 0;
+    uint8_t v = r->buf[r->next_read];
+    r->next_read++;
+    if (r->next_read >= r->size) {
+        r->next_read = 0;
     }
+    r->used--;
     return v;
 }
 
 int ringbuf_get16(ringbuf_t *r) {
-    int v = ringbuf_peek16(r);
-    if (v == -1) {
-        return v;
-    }
-    r->iget += 2;
-    if (r->iget >= r->size) {
-        r->iget -= r->size;
-    }
-    return v;
-}
-
-// Returns -1 if no room in buffer, else returns 0.
-int ringbuf_put(ringbuf_t *r, uint8_t v) {
-    uint32_t iput_new = r->iput + 1;
-    if (iput_new >= r->size) {
-        iput_new = 0;
-    }
-    if (iput_new == r->iget) {
+    if (r->used < 2) {
         return -1;
     }
-    r->buf[r->iput] = v;
-    r->iput = iput_new;
+
+    int high_byte = ringbuf_get(r);
+    int low_byte = ringbuf_get(r);
+    return (high_byte << 8) | low_byte;
+}
+
+// Return -1 if no room in buffer, else return 0.
+int ringbuf_put(ringbuf_t *r, uint8_t v) {
+    if (r->used >= r->size) {
+        return -1;
+    }
+    r->buf[r->next_write] = v;
+    r->next_write++;
+    if (r->next_write >= r->size) {
+        r->next_write = 0;
+    }
+    r->used++;
     return 0;
 }
 
 void ringbuf_clear(ringbuf_t *r) {
-    r->iput = r->iget = 0;
+    r->next_write = 0;
+    r->next_read = 0;
+    r->used = 0;
 }
 
 // Number of free slots that can be written.
 size_t ringbuf_num_empty(ringbuf_t *r) {
-    return (r->size + r->iget - r->iput - 1) % r->size;
+    return r->size - r->used;
 }
 
 // Number of bytes available to read.
 size_t ringbuf_num_filled(ringbuf_t *r) {
-    return (r->size + r->iput - r->iget) % r->size;
+    return r->used;
 }
 
 // If the ring buffer fills up, not all bytes will be written.
@@ -134,37 +133,12 @@ size_t ringbuf_get_n(ringbuf_t *r, uint8_t *buf, size_t bufsize) {
     return bufsize;
 }
 
-int ringbuf_peek16(ringbuf_t *r) {
-    if (r->iget == r->iput) {
-        return -1;
-    }
-    uint32_t iget_a = r->iget + 1;
-    if (iget_a == r->size) {
-        iget_a = 0;
-    }
-    if (iget_a == r->iput) {
-        return -1;
-    }
-    return (r->buf[r->iget] << 8) | (r->buf[iget_a]);
-}
-
 int ringbuf_put16(ringbuf_t *r, uint16_t v) {
-    uint32_t iput_a = r->iput + 1;
-    if (iput_a == r->size) {
-        iput_a = 0;
-    }
-    if (iput_a == r->iget) {
+    if (r->size - r->used < 2) {
         return -1;
     }
-    uint32_t iput_b = iput_a + 1;
-    if (iput_b == r->size) {
-        iput_b = 0;
-    }
-    if (iput_b == r->iget) {
-        return -1;
-    }
-    r->buf[r->iput] = (v >> 8) & 0xff;
-    r->buf[iput_a] = v & 0xff;
-    r->iput = iput_b;
+
+    ringbuf_put(r, (v >> 8) & 0xff);
+    ringbuf_put(r, v & 0xff);
     return 0;
 }
diff --git a/py/ringbuf.h b/py/ringbuf.h
index d868eff1e4..2725bedcca 100644
--- a/py/ringbuf.h
+++ b/py/ringbuf.h
@@ -33,19 +33,23 @@
 
 typedef struct _ringbuf_t {
     uint8_t *buf;
-    // Allocated size; capacity is one less. Don't reference this directly.
     uint32_t size;
-    uint32_t iget;
-    uint32_t iput;
+    uint32_t used;
+    uint32_t next_read;
+    uint32_t next_write;
 } ringbuf_t;
 
-// Note that the capacity of the buffer is N-1!
-
-// For static initialization use ringbuf_init()
+// For static initialization with an existing buffer, use ringbuf_init().
 bool ringbuf_init(ringbuf_t *r, uint8_t *buf, size_t capacity);
+
+// For allocation of a buffer on the heap, use ringbuf_alloc().
 bool ringbuf_alloc(ringbuf_t *r, size_t capacity, bool long_lived);
-void ringbuf_free(ringbuf_t *r);
-size_t ringbuf_capacity(ringbuf_t *r);
+
+// Mark ringbuf as no longer in use, and allow any heap storage to be freed by gc.
+void ringbuf_deinit(ringbuf_t *r);
+
+// Note: Ringbuf operations are not atomic.
+size_t ringbuf_size(ringbuf_t *r);
 int ringbuf_get(ringbuf_t *r);
 int ringbuf_put(ringbuf_t *r, uint8_t v);
 void ringbuf_clear(ringbuf_t *r);
@@ -54,9 +58,8 @@ size_t ringbuf_num_filled(ringbuf_t *r);
 size_t ringbuf_put_n(ringbuf_t *r, const uint8_t *buf, size_t bufsize);
 size_t ringbuf_get_n(ringbuf_t *r, uint8_t *buf, size_t bufsize);
 
-// Note: big-endian. No-op if not enough room available for both bytes.
+// Note: big-endian. Return -1 if can't read or write two bytes.
 int ringbuf_get16(ringbuf_t *r);
-int ringbuf_peek16(ringbuf_t *r);
 int ringbuf_put16(ringbuf_t *r, uint16_t v);
 
 #endif // MICROPY_INCLUDED_PY_RINGBUF_H

From e25c195b789c5273adf028980fc1d371557f5e8f Mon Sep 17 00:00:00 2001
From: Dan Halbert <halbert@halwitz.org>
Date: Sun, 25 Sep 2022 09:12:23 -0400
Subject: [PATCH 2/2] fix broadcom UART ringbuf init

---
 ports/broadcom/common-hal/busio/UART.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/ports/broadcom/common-hal/busio/UART.c b/ports/broadcom/common-hal/busio/UART.c
index 6d97b42c76..9f51f15acb 100644
--- a/ports/broadcom/common-hal/busio/UART.c
+++ b/ports/broadcom/common-hal/busio/UART.c
@@ -211,7 +211,6 @@ void common_hal_busio_uart_construct(busio_uart_obj_t *self,
         // Use the provided buffer when given.
         if (receiver_buffer != NULL) {
             ringbuf_init(&self->ringbuf, receiver_buffer, receiver_buffer_size);
-            self->ringbuf = (ringbuf_t) { receiver_buffer, receiver_buffer_size };
         } else {
             // Initially allocate the UART's buffer in the long-lived part of the
             // heap. UARTs are generally long-lived objects, but the "make long-