From 2bae80af95390121dfcaa3482697fa2347ecc6ee Mon Sep 17 00:00:00 2001 From: Dan Halbert Date: Tue, 7 Jul 2026 23:00:00 -0400 Subject: [PATCH 1/4] espressif/_bleio: build discovered GATT objects on the VM task Remote service/characteristic/descriptor discovery callbacks run on the NimBLE host task, not the CircuitPython VM task, and were allocating CircuitPython objects (mp_obj_malloc, etc.) directly in that context. Because the espressif port has no GIL (MICROPY_PY_THREAD == 0), an allocation on the host task can trigger a gc_collect() there, which scans the host task's stack instead of the VM task's and frees the still-live half-built discovery objects. The resulting heap corruption showed up as an interrupt-watchdog reset (ESP_RST_INT_WDT) partway through discovery. The callbacks now only copy the plain NimBLE result struct into a fixed ring buffer (a non-allocating memcpy, guarded with interrupts disabled since ringbuf ops are not atomic and the host and VM tasks preempt each other). The VM task drains the ring and builds the Python objects, so all heap allocation happens on the VM task. The ring storage is allocated on first use from the port (IDF) heap, so it needs no GC root, survives soft reloads, and costs no RAM until a central actually discovers services. Co-Authored-By: Claude Opus 4.8 (1M context) Co-Authored-By: Claude Fable 5 --- .../espressif/common-hal/_bleio/Connection.c | 242 +++++++++++++----- 1 file changed, 172 insertions(+), 70 deletions(-) diff --git a/ports/espressif/common-hal/_bleio/Connection.c b/ports/espressif/common-hal/_bleio/Connection.c index 8df4dc26545..5236ee6e6e7 100644 --- a/ports/espressif/common-hal/_bleio/Connection.c +++ b/ports/espressif/common-hal/_bleio/Connection.c @@ -14,6 +14,7 @@ #include "py/objlist.h" #include "py/objstr.h" #include "py/qstr.h" +#include "py/ringbuf.h" #include "py/runtime.h" #include "shared/runtime/interrupt_char.h" @@ -24,8 +25,10 @@ #include "shared-bindings/_bleio/Characteristic.h" #include "shared-bindings/_bleio/Service.h" #include "shared-bindings/_bleio/UUID.h" +#include "shared-bindings/microcontroller/__init__.h" #include "shared-bindings/time/__init__.h" +#include "supervisor/port_heap.h" #include "supervisor/shared/tick.h" #include "common-hal/_bleio/ble_events.h" @@ -184,21 +187,6 @@ static volatile int _last_discovery_status; // Give 20 seconds for each step of discovery: services, characteristics, attributes. #define DISCOVERY_TIMEOUT_MS 20000 -static int _wait_for_discovery_step_done(void) { - const uint64_t timeout_time_ms = common_hal_time_monotonic_ms() + DISCOVERY_TIMEOUT_MS; - while ((_last_discovery_status == 0) && (common_hal_time_monotonic_ms() < timeout_time_ms)) { - RUN_BACKGROUND_TASKS; - if (mp_hal_is_interrupted()) { - // Return prematurely. Then the interrupt will be raised. - _last_discovery_status = BLE_HS_EDONE; - } - } - if (_last_discovery_status == 0) { - return BLE_HS_ETIMEOUT; - } - return _last_discovery_status; -} - // Record result of last discovery step: services, characteristics, descriptors. static void _set_discovery_step_status(int status) { _last_discovery_status = status; @@ -215,18 +203,123 @@ static void _check_discovery_status(int status) { CHECK_BLE_ERROR(status); } +// Raw discovery results are staged here by the NimBLE host-task callbacks. +// Those callbacks run on the "nimble_host" task, NOT the VM task, so they must +// not allocate from the MicroPython heap: an allocation there can trigger a +// gc_collect() that scans the wrong task's stack and frees the VM's live +// objects. Instead the callbacks copy the plain NimBLE result struct into this +// ring (a non-allocating memcpy), and the VM task drains it and builds the +// Python objects. See shared-module/_bleio/ScanResults.c for the same pattern. +// +// Discovery is serialized (one discover_remote_services() at a time), so a +// single file-static ring suffices. It must hold one ATT-PDU burst of records: +// within a response PDU the host task invokes the callback repeatedly without +// yielding, so the VM task cannot drain until the next PDU's round-trip. +// +// The storage is allocated on first use from the port (IDF) heap, not the GC +// heap, so it needs no GC root and survives soft reloads: a stale procedure's +// pushes always land in live memory. It is never freed. (Same pattern as the +// port_malloc-backed ringbuf in shared-module/keypad/EventQueue.c.) +#define DISCOVERY_RING_SIZE (4096) +static uint8_t *_discovery_ring_buffer; +static ringbuf_t _discovery_ring; + +// Stage one fixed-size record. Runs on the nimble_host task. +// ringbuf ops are not atomic and the two tasks preempt each other, so guard +// with interrupts disabled; the guarded region is a single small record copy. +static bool _push_record(const void *record, size_t size) { + common_hal_mcu_disable_interrupts(); + bool ok = ringbuf_num_empty(&_discovery_ring) >= size; + if (ok) { + ringbuf_put_n(&_discovery_ring, (const uint8_t *)record, size); + } + common_hal_mcu_enable_interrupts(); + return ok; +} + +// Retrieve one fixed-size record. Runs on the VM task. +static bool _pop_record(void *record, size_t size) { + common_hal_mcu_disable_interrupts(); + bool ok = ringbuf_num_filled(&_discovery_ring) >= size; + if (ok) { + ringbuf_get_n(&_discovery_ring, (uint8_t *)record, size); + } + common_hal_mcu_enable_interrupts(); + return ok; +} + +// Reset the ring before a discovery step. Runs on the VM task. Guarded because +// a stale procedure from a previous errored, timed-out, or interrupted +// discovery may still be pushing records on the nimble_host task. +static void _reset_discovery_ring(void) { + if (_discovery_ring_buffer == NULL) { + _discovery_ring_buffer = port_malloc(DISCOVERY_RING_SIZE, false); + if (_discovery_ring_buffer == NULL) { + m_malloc_fail(DISCOVERY_RING_SIZE); + } + } + common_hal_mcu_disable_interrupts(); + ringbuf_init(&_discovery_ring, _discovery_ring_buffer, DISCOVERY_RING_SIZE); + common_hal_mcu_enable_interrupts(); +} + static int _discovered_service_cb(uint16_t conn_handle, const struct ble_gatt_error *error, const struct ble_gatt_svc *svc, void *arg) { - bleio_connection_internal_t *self = (bleio_connection_internal_t *)arg; + if (error->status != 0) { + // BLE_HS_EDONE or some error has occurred. + _set_discovery_step_status(error->status); + return 0; + } + + // Runs on the nimble_host task: stage the raw result only, never allocate. + if (!_push_record(svc, sizeof(*svc))) { + _set_discovery_step_status(BLE_HS_ENOMEM); + } + return 0; +} +static int _discovered_characteristic_cb(uint16_t conn_handle, + const struct ble_gatt_error *error, + const struct ble_gatt_chr *chr, + void *arg) { + if (error->status != 0) { + // BLE_HS_EDONE or some error has occurred. + _set_discovery_step_status(error->status); + return 0; + } + + // Runs on the nimble_host task: stage the raw result only, never allocate. + if (!_push_record(chr, sizeof(*chr))) { + _set_discovery_step_status(BLE_HS_ENOMEM); + } + return 0; +} + +static int _discovered_descriptor_cb(uint16_t conn_handle, + const struct ble_gatt_error *error, + uint16_t chr_val_handle, + const struct ble_gatt_dsc *dsc, + void *arg) { if (error->status != 0) { // BLE_HS_EDONE or some error has occurred. _set_discovery_step_status(error->status); return 0; } + // Runs on the nimble_host task: stage the raw result only, never allocate. + if (!_push_record(dsc, sizeof(*dsc))) { + _set_discovery_step_status(BLE_HS_ENOMEM); + } + return 0; +} + +// Build a Service object from a staged raw record. Runs on the VM task. +static void _build_service(void *ctx, const void *record) { + bleio_connection_internal_t *self = ctx; + const struct ble_gatt_svc *svc = record; + bleio_service_obj_t *service = mp_obj_malloc(bleio_service_obj_t, &bleio_service_type); // Initialize several fields at once. @@ -244,27 +337,58 @@ static int _discovered_service_cb(uint16_t conn_handle, mp_obj_list_append(MP_OBJ_FROM_PTR(self->remote_service_list), MP_OBJ_FROM_PTR(service)); - return 0; } -static int _discovered_characteristic_cb(uint16_t conn_handle, - const struct ble_gatt_error *error, - const struct ble_gatt_chr *chr, - void *arg) { - bleio_service_obj_t *service = (bleio_service_obj_t *)arg; - - if (error->status != 0) { - // BLE_HS_EDONE or some error has occurred. - _set_discovery_step_status(error->status); - return 0; +// Drain and build all staged records on the VM task, until the step completes +// and the ring is empty. build() is invoked for each raw record with ctx passed +// through. Returns the discovery step status. +static int _drain_records(void *ctx, size_t record_size, + void (*build)(void *ctx, const void *record)) { + const uint64_t timeout_time_ms = common_hal_time_monotonic_ms() + DISCOVERY_TIMEOUT_MS; + // Sized to the largest record type so one buffer serves every step. + union { + struct ble_gatt_svc svc; + struct ble_gatt_chr chr; + struct ble_gatt_dsc dsc; + } record; + while (true) { + if (_pop_record(&record, record_size)) { + build(ctx, &record); + continue; + } + // Ring is empty. + if (_last_discovery_status != 0) { + // On EDONE or a NimBLE error, the terminal callback has already + // run, so no more records will arrive: drain any that raced in, + // then stop. On ENOMEM (our own push failure) the procedure may + // still be running, but we are raising anyway; any later + // discovery resets the ring before reuse. + while (_pop_record(&record, record_size)) { + build(ctx, &record); + } + return _last_discovery_status; + } + if (common_hal_time_monotonic_ms() >= timeout_time_ms) { + return BLE_HS_ETIMEOUT; + } + RUN_BACKGROUND_TASKS; + if (mp_hal_is_interrupted()) { + // Return prematurely. Then the interrupt will be raised. + _set_discovery_step_status(BLE_HS_EDONE); + } } +} + +// Build a Characteristic object from a staged raw record. Runs on the VM task. +static void _build_characteristic(void *ctx, const void *record) { + bleio_service_obj_t *service = ctx; + const struct ble_gatt_chr *chr = record; bleio_characteristic_obj_t *characteristic = mp_obj_malloc(bleio_characteristic_obj_t, &bleio_characteristic_type); // Known characteristic UUID. bleio_uuid_obj_t *uuid = mp_obj_malloc(bleio_uuid_obj_t, &bleio_uuid_type); - uuid->nimble_ble_uuid = chr->uuid; bleio_characteristic_properties_t props = @@ -288,33 +412,14 @@ static int _discovered_characteristic_cb(uint16_t conn_handle, // Set def_handle directly since it is only used in discovery. characteristic->def_handle = chr->def_handle; - #if CIRCUITPY_VERBOSE_BLE - mp_printf(&mp_plat_print, "_discovered_characteristic_cb: char handle: %d\n", characteristic->handle); - #endif - mp_obj_list_append(MP_OBJ_FROM_PTR(service->characteristic_list), MP_OBJ_FROM_PTR(characteristic)); - return 0; } -static int _discovered_descriptor_cb(uint16_t conn_handle, - const struct ble_gatt_error *error, - uint16_t chr_val_handle, - const struct ble_gatt_dsc *dsc, - void *arg) { - bleio_characteristic_obj_t *characteristic = (bleio_characteristic_obj_t *)arg; - - if (error->status != 0) { - - #if CIRCUITPY_VERBOSE_BLE - mp_printf(&mp_plat_print, "_discovered_descriptor_cb error->status: %d, handle: %d\n", - error->status, error->att_handle); - #endif - - // BLE_HS_EDONE or some error has occurred. - _set_discovery_step_status(error->status); - return 0; - } +// Build a Descriptor object from a staged raw record. Runs on the VM task. +static void _build_descriptor(void *ctx, const void *record) { + bleio_characteristic_obj_t *characteristic = ctx; + const struct ble_gatt_dsc *dsc = record; // Remember handles for certain well-known descriptors. switch (dsc->uuid.u16.value) { @@ -344,14 +449,8 @@ static int _discovered_descriptor_cb(uint16_t conn_handle, GATT_MAX_DATA_LENGTH, false, mp_const_empty_bytes); descriptor->handle = dsc->handle; - #if CIRCUITPY_VERBOSE_BLE - mp_printf(&mp_plat_print, "_discovered_descriptor_cb: char handle: %d, desc handle: %d, uuid type: %d, u16 value: 0x%x\n", - characteristic->handle, descriptor->handle, dsc->uuid.u.type, dsc->uuid.u16.value); - #endif - mp_obj_list_append(MP_OBJ_FROM_PTR(characteristic->descriptor_list), MP_OBJ_FROM_PTR(descriptor)); - return 0; } static void discover_remote_services(bleio_connection_internal_t *self, mp_obj_t service_uuids_whitelist) { @@ -359,13 +458,14 @@ static void discover_remote_services(bleio_connection_internal_t *self, mp_obj_t self->remote_service_list = mp_obj_new_list(0, NULL); if (service_uuids_whitelist == mp_const_none) { - // Reset discovery status before starting callbacks + // Reset discovery status and staging ring before starting callbacks. _set_discovery_step_status(0); + _reset_discovery_ring(); CHECK_NIMBLE_ERROR(ble_gattc_disc_all_svcs(self->conn_handle, _discovered_service_cb, self)); - // Wait for _discovered_service_cb() to be called multiple times until it's done. - int status = _wait_for_discovery_step_done(); + // Drain staged services and build them on the VM task until done. + int status = _drain_records(self, sizeof(struct ble_gatt_svc), _build_service); _check_discovery_status(status); } else { mp_obj_iter_buf_t iter_buf; @@ -377,25 +477,26 @@ static void discover_remote_services(bleio_connection_internal_t *self, mp_obj_t } bleio_uuid_obj_t *uuid = MP_OBJ_TO_PTR(uuid_obj); - // Reset discovery status before starting callbacks + // Reset discovery status and staging ring before starting callbacks. _set_discovery_step_status(0); + _reset_discovery_ring(); CHECK_NIMBLE_ERROR(ble_gattc_disc_svc_by_uuid(self->conn_handle, &uuid->nimble_ble_uuid.u, _discovered_service_cb, self)); - // Wait for _discovered_service_cb() to be called multiple times until it's done. - int status = _wait_for_discovery_step_done(); + // Drain staged services and build them on the VM task until done. + int status = _drain_records(self, sizeof(struct ble_gatt_svc), _build_service); _check_discovery_status(status); } } // Now discover characteristics for each discovered service. - for (size_t i = 0; i < self->remote_service_list->len; i++) { bleio_service_obj_t *service = MP_OBJ_TO_PTR(self->remote_service_list->items[i]); - // Reset discovery status before starting callbacks + // Reset discovery status and staging ring before starting callbacks. _set_discovery_step_status(0); + _reset_discovery_ring(); CHECK_NIMBLE_ERROR(ble_gattc_disc_all_chrs(self->conn_handle, service->start_handle, @@ -403,8 +504,8 @@ static void discover_remote_services(bleio_connection_internal_t *self, mp_obj_t _discovered_characteristic_cb, service)); - // Wait for _discovered_characteristic_cb() to be called multiple times until it's done. - int status = _wait_for_discovery_step_done(); + // Drain staged characteristics and build them on the VM task until done. + int status = _drain_records(service, sizeof(struct ble_gatt_chr), _build_characteristic); _check_discovery_status(status); // Got characteristics for this service. Now discover descriptors for each characteristic. @@ -429,8 +530,9 @@ static void discover_remote_services(bleio_connection_internal_t *self, mp_obj_t continue; } - // Reset discovery status before starting callbacks + // Reset discovery status and staging ring before starting callbacks. _set_discovery_step_status(0); + _reset_discovery_ring(); // The descriptor handle inclusive range is [characteristic->handle + 1, end_handle], // but ble_gattc_disc_all_dscs() requires starting with characteristic->handle. @@ -438,8 +540,8 @@ static void discover_remote_services(bleio_connection_internal_t *self, mp_obj_t end_handle, _discovered_descriptor_cb, characteristic)); - // Wait for _discovered_descriptor_cb to be called multiple times until it's done. - status = _wait_for_discovery_step_done(); + // Drain staged descriptors and build them on the VM task until done. + status = _drain_records(characteristic, sizeof(struct ble_gatt_dsc), _build_descriptor); _check_discovery_status(status); } } From 97cf5c585580abaebadc2cefa7da1dacdb0d7ce6 Mon Sep 17 00:00:00 2001 From: Dan Halbert Date: Wed, 8 Jul 2026 15:14:06 -0400 Subject: [PATCH 2/4] espressif/_bleio: guard buffer state shared with the NimBLE host task CharacteristicBuffer and PacketBuffer ringbufs are filled from the nimble_host task, which preempts the VM task at arbitrary points, but the non-atomic ringbuf operations were unguarded: the comment claiming the BLE host task "won't interrupt us" has been wrong since the code was written (the host task runs at a much higher priority on the same core). PacketBuffer's pending outgoing buffers have the same problem: write() on the VM task appends to them while queue_next_write() on the host task consumes them. Guard these operations with interrupts disabled, mirroring the nordic port's critical sections around the same code, in the same places. Co-Authored-By: Claude Fable 5 --- .../common-hal/_bleio/CharacteristicBuffer.c | 21 ++++++++++++++----- .../common-hal/_bleio/PacketBuffer.c | 20 ++++++++++++++++++ 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/ports/espressif/common-hal/_bleio/CharacteristicBuffer.c b/ports/espressif/common-hal/_bleio/CharacteristicBuffer.c index 3c0cbf323f3..2f34666420b 100644 --- a/ports/espressif/common-hal/_bleio/CharacteristicBuffer.c +++ b/ports/espressif/common-hal/_bleio/CharacteristicBuffer.c @@ -16,12 +16,19 @@ #include "shared-bindings/_bleio/__init__.h" #include "shared-bindings/_bleio/Connection.h" #include "shared-bindings/_bleio/CharacteristicBuffer.h" +#include "shared-bindings/microcontroller/__init__.h" #include "supervisor/shared/tick.h" #include "common-hal/_bleio/ble_events.h" +// The ringbuf is filled from the nimble_host task, which preempts the VM task +// at arbitrary points, and ringbuf operations are not atomic, so guard them +// with interrupts disabled. + +// Runs on the nimble_host task. void bleio_characteristic_buffer_extend(bleio_characteristic_buffer_obj_t *self, const uint8_t *data, size_t len) { + common_hal_mcu_disable_interrupts(); if (self->watch_for_interrupt_char) { for (uint16_t i = 0; i < len; i++) { if (data[i] == mp_interrupt_char) { @@ -34,6 +41,7 @@ void bleio_characteristic_buffer_extend(bleio_characteristic_buffer_obj_t *self, } else { ringbuf_put_n(&self->ringbuf, data, len); } + common_hal_mcu_enable_interrupts(); } void _common_hal_bleio_characteristic_buffer_construct(bleio_characteristic_buffer_obj_t *self, @@ -70,20 +78,23 @@ uint32_t common_hal_bleio_characteristic_buffer_read(bleio_characteristic_buffer } } + common_hal_mcu_disable_interrupts(); uint32_t num_bytes_read = ringbuf_get_n(&self->ringbuf, data, len); + common_hal_mcu_enable_interrupts(); return num_bytes_read; } -// NOTE: The nRF port has protection around these operations because the ringbuf -// is filled from an interrupt. On ESP the ringbuf is filled from the BLE host -// task that won't interrupt us. - uint32_t common_hal_bleio_characteristic_buffer_rx_characters_available(bleio_characteristic_buffer_obj_t *self) { - return ringbuf_num_filled(&self->ringbuf); + common_hal_mcu_disable_interrupts(); + uint32_t count = ringbuf_num_filled(&self->ringbuf); + common_hal_mcu_enable_interrupts(); + return count; } void common_hal_bleio_characteristic_buffer_clear_rx_buffer(bleio_characteristic_buffer_obj_t *self) { + common_hal_mcu_disable_interrupts(); ringbuf_clear(&self->ringbuf); + common_hal_mcu_enable_interrupts(); } bool common_hal_bleio_characteristic_buffer_deinited(bleio_characteristic_buffer_obj_t *self) { diff --git a/ports/espressif/common-hal/_bleio/PacketBuffer.c b/ports/espressif/common-hal/_bleio/PacketBuffer.c index db035157ceb..a14c627f110 100644 --- a/ports/espressif/common-hal/_bleio/PacketBuffer.c +++ b/ports/espressif/common-hal/_bleio/PacketBuffer.c @@ -15,6 +15,7 @@ #include "shared-bindings/_bleio/__init__.h" #include "shared-bindings/_bleio/Connection.h" #include "shared-bindings/_bleio/PacketBuffer.h" +#include "shared-bindings/microcontroller/__init__.h" #include "supervisor/shared/tick.h" #include "supervisor/shared/bluetooth/serial.h" @@ -23,6 +24,11 @@ #include "host/ble_att.h" +// The ringbuf and the pending outgoing buffers are shared with the nimble_host +// task, which preempts the VM task at arbitrary points, and ringbuf operations +// are not atomic, so guard the shared accesses with interrupts disabled. + +// Runs on the nimble_host task. void bleio_packet_buffer_extend(bleio_packet_buffer_obj_t *self, uint16_t conn_handle, const uint8_t *data, size_t len) { if (self->conn_handle != conn_handle) { return; @@ -34,6 +40,8 @@ void bleio_packet_buffer_extend(bleio_packet_buffer_obj_t *self, uint16_t conn_h return; } + common_hal_mcu_disable_interrupts(); + // Make room for the new value by dropping the oldest packets first. while (ringbuf_num_empty(&self->ringbuf) < len + sizeof(uint16_t)) { uint16_t packet_length; @@ -45,6 +53,8 @@ void bleio_packet_buffer_extend(bleio_packet_buffer_obj_t *self, uint16_t conn_h } ringbuf_put_n(&self->ringbuf, (uint8_t *)&len, sizeof(uint16_t)); ringbuf_put_n(&self->ringbuf, data, len); + + common_hal_mcu_enable_interrupts(); } static int packet_buffer_on_ble_client_evt(struct ble_gap_event *event, void *param); @@ -264,6 +274,8 @@ mp_int_t common_hal_bleio_packet_buffer_readinto(bleio_packet_buffer_obj_t *self return 0; } + common_hal_mcu_disable_interrupts(); + // Get packet length, which is in first two bytes of packet. uint16_t packet_length; ringbuf_get_n(&self->ringbuf, (uint8_t *)&packet_length, sizeof(uint16_t)); @@ -282,6 +294,8 @@ mp_int_t common_hal_bleio_packet_buffer_readinto(bleio_packet_buffer_obj_t *self ret = packet_length; } + common_hal_mcu_enable_interrupts(); + return ret; } @@ -324,6 +338,10 @@ mp_int_t common_hal_bleio_packet_buffer_write(bleio_packet_buffer_obj_t *self, c size_t num_bytes_written = 0; + // The nimble_host task may modify pending_size, pending_index, and + // packet_queued via queue_next_write(), so guard the append. + common_hal_mcu_disable_interrupts(); + uint32_t *pending = self->outgoing[self->pending_index]; if (self->pending_size == 0) { @@ -335,6 +353,8 @@ mp_int_t common_hal_bleio_packet_buffer_write(bleio_packet_buffer_obj_t *self, c self->pending_size += len; num_bytes_written += len; + common_hal_mcu_enable_interrupts(); + // If no writes are queued then sneak in this data. if (!self->packet_queued) { // This will queue up the packet even if it can't send immediately. From 8eb5e6f958f0688f99bb988a6f39e4d4496dc6da Mon Sep 17 00:00:00 2001 From: Dan Halbert Date: Wed, 8 Jul 2026 15:55:38 -0400 Subject: [PATCH 3/4] espressif/_bleio: restore CIRCUITPY_VERBOSE_BLE discovery prints Restore the verbose prints dropped when discovery object construction moved off the NimBLE host task, and add matching error prints to all three discovery callbacks for consistency (only the descriptor callback had one before). The object prints now live in _build_characteristic() and _build_descriptor(), which run on the VM task. Also print the step status in _check_discovery_status(), which covers the timeout and ring-full errors that never pass through a callback. Co-Authored-By: Claude Fable 5 --- .../espressif/common-hal/_bleio/Connection.c | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/ports/espressif/common-hal/_bleio/Connection.c b/ports/espressif/common-hal/_bleio/Connection.c index 5236ee6e6e7..131a89bc9ee 100644 --- a/ports/espressif/common-hal/_bleio/Connection.c +++ b/ports/espressif/common-hal/_bleio/Connection.c @@ -193,6 +193,10 @@ static void _set_discovery_step_status(int status) { } static void _check_discovery_status(int status) { + #if CIRCUITPY_VERBOSE_BLE + mp_printf(&mp_plat_print, "discovery step status: %d\n", status); + #endif + if (status == BLE_HS_EDONE) { return; } @@ -268,6 +272,11 @@ static int _discovered_service_cb(uint16_t conn_handle, const struct ble_gatt_svc *svc, void *arg) { if (error->status != 0) { + #if CIRCUITPY_VERBOSE_BLE + mp_printf(&mp_plat_print, "_discovered_service_cb error->status: %d, handle: %d\n", + error->status, error->att_handle); + #endif + // BLE_HS_EDONE or some error has occurred. _set_discovery_step_status(error->status); return 0; @@ -285,6 +294,11 @@ static int _discovered_characteristic_cb(uint16_t conn_handle, const struct ble_gatt_chr *chr, void *arg) { if (error->status != 0) { + #if CIRCUITPY_VERBOSE_BLE + mp_printf(&mp_plat_print, "_discovered_characteristic_cb error->status: %d, handle: %d\n", + error->status, error->att_handle); + #endif + // BLE_HS_EDONE or some error has occurred. _set_discovery_step_status(error->status); return 0; @@ -303,6 +317,11 @@ static int _discovered_descriptor_cb(uint16_t conn_handle, const struct ble_gatt_dsc *dsc, void *arg) { if (error->status != 0) { + #if CIRCUITPY_VERBOSE_BLE + mp_printf(&mp_plat_print, "_discovered_descriptor_cb error->status: %d, handle: %d\n", + error->status, error->att_handle); + #endif + // BLE_HS_EDONE or some error has occurred. _set_discovery_step_status(error->status); return 0; @@ -412,6 +431,10 @@ static void _build_characteristic(void *ctx, const void *record) { // Set def_handle directly since it is only used in discovery. characteristic->def_handle = chr->def_handle; + #if CIRCUITPY_VERBOSE_BLE + mp_printf(&mp_plat_print, "_build_characteristic: char handle: %d\n", characteristic->handle); + #endif + mp_obj_list_append(MP_OBJ_FROM_PTR(service->characteristic_list), MP_OBJ_FROM_PTR(characteristic)); } @@ -449,6 +472,11 @@ static void _build_descriptor(void *ctx, const void *record) { GATT_MAX_DATA_LENGTH, false, mp_const_empty_bytes); descriptor->handle = dsc->handle; + #if CIRCUITPY_VERBOSE_BLE + mp_printf(&mp_plat_print, "_build_descriptor: char handle: %d, desc handle: %d, uuid type: %d, u16 value: 0x%x\n", + characteristic->handle, descriptor->handle, dsc->uuid.u.type, dsc->uuid.u16.value); + #endif + mp_obj_list_append(MP_OBJ_FROM_PTR(characteristic->descriptor_list), MP_OBJ_FROM_PTR(descriptor)); } From c5c0603568169df1e51a4b12170679df5f7bba7e Mon Sep 17 00:00:00 2001 From: Dan Halbert Date: Fri, 10 Jul 2026 14:32:39 -0400 Subject: [PATCH 4/4] espressif/_bleio: use a FreeRTOS queue for discovery staging Per review, replace the interrupt-guarded ringbuf with a FreeRTOS queue, the RTOS-native primitive for task-to-task message passing. xQueueSend(), xQueueReceive(), and xQueueReset() are thread-safe by construction, so the hand-rolled critical sections go away, along with the port_malloc() storage management: the queue is created lazily from the FreeRTOS (IDF) heap on first use and never deleted. Queue-full still maps to BLE_HS_ENOMEM, and the drain loop and error handling are unchanged. Co-Authored-By: Claude Fable 5 --- .../espressif/common-hal/_bleio/Connection.c | 138 ++++++++---------- 1 file changed, 60 insertions(+), 78 deletions(-) diff --git a/ports/espressif/common-hal/_bleio/Connection.c b/ports/espressif/common-hal/_bleio/Connection.c index 131a89bc9ee..91d2336ee68 100644 --- a/ports/espressif/common-hal/_bleio/Connection.c +++ b/ports/espressif/common-hal/_bleio/Connection.c @@ -14,7 +14,6 @@ #include "py/objlist.h" #include "py/objstr.h" #include "py/qstr.h" -#include "py/ringbuf.h" #include "py/runtime.h" #include "shared/runtime/interrupt_char.h" @@ -25,14 +24,15 @@ #include "shared-bindings/_bleio/Characteristic.h" #include "shared-bindings/_bleio/Service.h" #include "shared-bindings/_bleio/UUID.h" -#include "shared-bindings/microcontroller/__init__.h" #include "shared-bindings/time/__init__.h" -#include "supervisor/port_heap.h" #include "supervisor/shared/tick.h" #include "common-hal/_bleio/ble_events.h" +#include "freertos/FreeRTOS.h" +#include "freertos/queue.h" + #include "host/ble_att.h" // Uncomment to turn on debug logging just in this file. @@ -212,59 +212,43 @@ static void _check_discovery_status(int status) { // not allocate from the MicroPython heap: an allocation there can trigger a // gc_collect() that scans the wrong task's stack and frees the VM's live // objects. Instead the callbacks copy the plain NimBLE result struct into this -// ring (a non-allocating memcpy), and the VM task drains it and builds the -// Python objects. See shared-module/_bleio/ScanResults.c for the same pattern. +// FreeRTOS queue, and the VM task drains it and builds the Python objects. // // Discovery is serialized (one discover_remote_services() at a time), so a -// single file-static ring suffices. It must hold one ATT-PDU burst of records: +// single file-static queue suffices. It must hold one ATT-PDU burst of records: // within a response PDU the host task invokes the callback repeatedly without -// yielding, so the VM task cannot drain until the next PDU's round-trip. +// yielding, so the VM task cannot drain until the next PDU's round-trip. The +// largest burst is a find-information response at the maximum ATT MTU we +// advertise (CONFIG_BT_NIMBLE_ATT_PREFERRED_MTU, 256): about 64 descriptor +// records. The depth is twice that. // -// The storage is allocated on first use from the port (IDF) heap, not the GC +// The queue is created on first use from the FreeRTOS (IDF) heap, not the GC // heap, so it needs no GC root and survives soft reloads: a stale procedure's -// pushes always land in live memory. It is never freed. (Same pattern as the -// port_malloc-backed ringbuf in shared-module/keypad/EventQueue.c.) -#define DISCOVERY_RING_SIZE (4096) -static uint8_t *_discovery_ring_buffer; -static ringbuf_t _discovery_ring; - -// Stage one fixed-size record. Runs on the nimble_host task. -// ringbuf ops are not atomic and the two tasks preempt each other, so guard -// with interrupts disabled; the guarded region is a single small record copy. -static bool _push_record(const void *record, size_t size) { - common_hal_mcu_disable_interrupts(); - bool ok = ringbuf_num_empty(&_discovery_ring) >= size; - if (ok) { - ringbuf_put_n(&_discovery_ring, (const uint8_t *)record, size); - } - common_hal_mcu_enable_interrupts(); - return ok; -} - -// Retrieve one fixed-size record. Runs on the VM task. -static bool _pop_record(void *record, size_t size) { - common_hal_mcu_disable_interrupts(); - bool ok = ringbuf_num_filled(&_discovery_ring) >= size; - if (ok) { - ringbuf_get_n(&_discovery_ring, (uint8_t *)record, size); - } - common_hal_mcu_enable_interrupts(); - return ok; -} - -// Reset the ring before a discovery step. Runs on the VM task. Guarded because -// a stale procedure from a previous errored, timed-out, or interrupted -// discovery may still be pushing records on the nimble_host task. -static void _reset_discovery_ring(void) { - if (_discovery_ring_buffer == NULL) { - _discovery_ring_buffer = port_malloc(DISCOVERY_RING_SIZE, false); - if (_discovery_ring_buffer == NULL) { - m_malloc_fail(DISCOVERY_RING_SIZE); +// sends always land in live memory. It is never deleted. + +// One queue item, sized to the largest record type, so one queue can serve every +// discovery step. +typedef union { + struct ble_gatt_svc svc; + struct ble_gatt_chr chr; + struct ble_gatt_dsc dsc; +} discovery_record_t; + +#define DISCOVERY_QUEUE_DEPTH (128) +static QueueHandle_t _discovery_queue; + +// Create the queue on first use, and empty it before a discovery step. Runs on +// the VM task. xQueueReset() is safe against a stale procedure from a previous +// errored, timed-out, or interrupted discovery that may still be sending +// records on the nimble_host task. +static void _reset_discovery_queue(void) { + if (_discovery_queue == NULL) { + _discovery_queue = xQueueCreate(DISCOVERY_QUEUE_DEPTH, sizeof(discovery_record_t)); + if (_discovery_queue == NULL) { + m_malloc_fail(DISCOVERY_QUEUE_DEPTH * sizeof(discovery_record_t)); } } - common_hal_mcu_disable_interrupts(); - ringbuf_init(&_discovery_ring, _discovery_ring_buffer, DISCOVERY_RING_SIZE); - common_hal_mcu_enable_interrupts(); + xQueueReset(_discovery_queue); } static int _discovered_service_cb(uint16_t conn_handle, @@ -283,7 +267,8 @@ static int _discovered_service_cb(uint16_t conn_handle, } // Runs on the nimble_host task: stage the raw result only, never allocate. - if (!_push_record(svc, sizeof(*svc))) { + discovery_record_t record = { .svc = *svc }; + if (xQueueSend(_discovery_queue, &record, 0) != pdTRUE) { _set_discovery_step_status(BLE_HS_ENOMEM); } return 0; @@ -305,7 +290,8 @@ static int _discovered_characteristic_cb(uint16_t conn_handle, } // Runs on the nimble_host task: stage the raw result only, never allocate. - if (!_push_record(chr, sizeof(*chr))) { + discovery_record_t record = { .chr = *chr }; + if (xQueueSend(_discovery_queue, &record, 0) != pdTRUE) { _set_discovery_step_status(BLE_HS_ENOMEM); } return 0; @@ -328,7 +314,8 @@ static int _discovered_descriptor_cb(uint16_t conn_handle, } // Runs on the nimble_host task: stage the raw result only, never allocate. - if (!_push_record(dsc, sizeof(*dsc))) { + discovery_record_t record = { .dsc = *dsc }; + if (xQueueSend(_discovery_queue, &record, 0) != pdTRUE) { _set_discovery_step_status(BLE_HS_ENOMEM); } return 0; @@ -359,30 +346,25 @@ static void _build_service(void *ctx, const void *record) { } // Drain and build all staged records on the VM task, until the step completes -// and the ring is empty. build() is invoked for each raw record with ctx passed -// through. Returns the discovery step status. -static int _drain_records(void *ctx, size_t record_size, +// and the queue is empty. build() is invoked for each raw record with ctx +// passed through. Returns the discovery step status. +static int _drain_records(void *ctx, void (*build)(void *ctx, const void *record)) { const uint64_t timeout_time_ms = common_hal_time_monotonic_ms() + DISCOVERY_TIMEOUT_MS; - // Sized to the largest record type so one buffer serves every step. - union { - struct ble_gatt_svc svc; - struct ble_gatt_chr chr; - struct ble_gatt_dsc dsc; - } record; + discovery_record_t record; while (true) { - if (_pop_record(&record, record_size)) { + if (xQueueReceive(_discovery_queue, &record, 0) == pdTRUE) { build(ctx, &record); continue; } - // Ring is empty. + // Queue is empty. if (_last_discovery_status != 0) { // On EDONE or a NimBLE error, the terminal callback has already // run, so no more records will arrive: drain any that raced in, - // then stop. On ENOMEM (our own push failure) the procedure may + // then stop. On ENOMEM (our own send failure) the procedure may // still be running, but we are raising anyway; any later - // discovery resets the ring before reuse. - while (_pop_record(&record, record_size)) { + // discovery resets the queue before reuse. + while (xQueueReceive(_discovery_queue, &record, 0) == pdTRUE) { build(ctx, &record); } return _last_discovery_status; @@ -486,14 +468,14 @@ static void discover_remote_services(bleio_connection_internal_t *self, mp_obj_t self->remote_service_list = mp_obj_new_list(0, NULL); if (service_uuids_whitelist == mp_const_none) { - // Reset discovery status and staging ring before starting callbacks. + // Reset discovery status and staging queue before starting callbacks. _set_discovery_step_status(0); - _reset_discovery_ring(); + _reset_discovery_queue(); CHECK_NIMBLE_ERROR(ble_gattc_disc_all_svcs(self->conn_handle, _discovered_service_cb, self)); // Drain staged services and build them on the VM task until done. - int status = _drain_records(self, sizeof(struct ble_gatt_svc), _build_service); + int status = _drain_records(self, _build_service); _check_discovery_status(status); } else { mp_obj_iter_buf_t iter_buf; @@ -505,15 +487,15 @@ static void discover_remote_services(bleio_connection_internal_t *self, mp_obj_t } bleio_uuid_obj_t *uuid = MP_OBJ_TO_PTR(uuid_obj); - // Reset discovery status and staging ring before starting callbacks. + // Reset discovery status and staging queue before starting callbacks. _set_discovery_step_status(0); - _reset_discovery_ring(); + _reset_discovery_queue(); CHECK_NIMBLE_ERROR(ble_gattc_disc_svc_by_uuid(self->conn_handle, &uuid->nimble_ble_uuid.u, _discovered_service_cb, self)); // Drain staged services and build them on the VM task until done. - int status = _drain_records(self, sizeof(struct ble_gatt_svc), _build_service); + int status = _drain_records(self, _build_service); _check_discovery_status(status); } } @@ -522,9 +504,9 @@ static void discover_remote_services(bleio_connection_internal_t *self, mp_obj_t for (size_t i = 0; i < self->remote_service_list->len; i++) { bleio_service_obj_t *service = MP_OBJ_TO_PTR(self->remote_service_list->items[i]); - // Reset discovery status and staging ring before starting callbacks. + // Reset discovery status and staging queue before starting callbacks. _set_discovery_step_status(0); - _reset_discovery_ring(); + _reset_discovery_queue(); CHECK_NIMBLE_ERROR(ble_gattc_disc_all_chrs(self->conn_handle, service->start_handle, @@ -533,7 +515,7 @@ static void discover_remote_services(bleio_connection_internal_t *self, mp_obj_t service)); // Drain staged characteristics and build them on the VM task until done. - int status = _drain_records(service, sizeof(struct ble_gatt_chr), _build_characteristic); + int status = _drain_records(service, _build_characteristic); _check_discovery_status(status); // Got characteristics for this service. Now discover descriptors for each characteristic. @@ -558,9 +540,9 @@ static void discover_remote_services(bleio_connection_internal_t *self, mp_obj_t continue; } - // Reset discovery status and staging ring before starting callbacks. + // Reset discovery status and staging queue before starting callbacks. _set_discovery_step_status(0); - _reset_discovery_ring(); + _reset_discovery_queue(); // The descriptor handle inclusive range is [characteristic->handle + 1, end_handle], // but ble_gattc_disc_all_dscs() requires starting with characteristic->handle. @@ -569,7 +551,7 @@ static void discover_remote_services(bleio_connection_internal_t *self, mp_obj_t _discovered_descriptor_cb, characteristic)); // Drain staged descriptors and build them on the VM task until done. - status = _drain_records(characteristic, sizeof(struct ble_gatt_dsc), _build_descriptor); + status = _drain_records(characteristic, _build_descriptor); _check_discovery_status(status); } }