Fix a number of BLE workflow and bonding issues

devices/ble_hci/common-hal/_bleio/Adapter.c

@@ -643,7 +643,7 @@ uint32_t _common_hal_bleio_adapter_start_advertising(bleio_adapter_obj_t *self,
     bool connectable, bool anonymous, uint32_t timeout, float interval,
     const uint8_t *advertising_data, uint16_t advertising_data_len,
     const uint8_t *scan_response_data, uint16_t scan_response_data_len,
-    mp_int_t tx_power, const bleio_address_obj_t *directed_to) {
+    mp_int_t tx_power, const bleio_raw_address_t *directed_to) {
     check_enabled(self);
 
     if (self->now_advertising) {
@@ -662,11 +662,8 @@ uint32_t _common_hal_bleio_adapter_start_advertising(bleio_adapter_obj_t *self,
 
     // Copy peer address, if supplied.
     if (directed_to) {
-        mp_buffer_info_t bufinfo;
-        if (mp_get_buffer(directed_to->bytes, &bufinfo, MP_BUFFER_READ)) {
-            peer_addr.type = directed_to->type;
-            memcpy(&peer_addr.a.val, bufinfo.buf, sizeof(peer_addr.a.val));
-        }
+        peer_addr.type = directed_to->type;
+        memcpy(&peer_addr.a.val, directed_to->bytes, sizeof(peer_addr.a.val));
     }
 
     bool extended =
@@ -808,13 +805,20 @@ void common_hal_bleio_adapter_start_advertising(bleio_adapter_obj_t *self,
         mp_raise_NotImplementedError(MP_ERROR_TEXT("Only tx_power=0 supported"));
     }
 
+    // Convert here, where raising is allowed. The internal call must stay raise-free
+    // because supervisor/shared uses it too.
+    bleio_raw_address_t raw_directed_to;
+    if (directed_to != NULL) {
+        bleio_address_to_raw(directed_to, &raw_directed_to);
+    }
+
     const uint32_t result = _common_hal_bleio_adapter_start_advertising(
         self, connectable, anonymous, timeout, interval,
         advertising_data_bufinfo->buf,
         advertising_data_bufinfo->len,
         scan_response_data_bufinfo->buf,
         scan_response_data_bufinfo->len,
-        tx_power, directed_to);
+        tx_power, directed_to != NULL ? &raw_directed_to : NULL);
 
     if (result) {
         mp_raise_bleio_BluetoothError(MP_ERROR_TEXT("Already advertising"));

devices/ble_hci/common-hal/_bleio/PacketBuffer.c

@@ -245,6 +245,8 @@ 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);
+        // Mark as deinited, so common_hal_bleio_packet_buffer_deinited() reports it.
+        self->characteristic = NULL;
     }
 }
 

docs/workflows.md

@@ -95,13 +95,14 @@ to enable file system access.
 
 ### CircuitPython Service
 
-The base UUID for the CircuitPython service is `ADAFXXXX-4369-7263-7569-7450794686e`. The `XXXX` is
+The base UUID for the CircuitPython service is `ADAFXXXX-4369-7263-7569-74507974686e`. The `XXXX` is
 replaced by the four specific digits below. The service itself is `0001`.
 
-#### TX - `0002` / RX - `0003`
+#### RX - `0002` / TX - `0003`
 
 The TX and RX characteristics for the CircuitPython service work just like the Nordic Uart Service (NUS)
 but have different UUIDs to prevent conflicts with user-created NUS services.
+They are named from the NUS peripheral's point of view: a client writes to RX and subscribes to TX.
 
 #### Version - `0100`
 The Version characteristic is read-only and returns the UTF-8 encoded version string.

ports/espressif/boards/adafruit_metro_esp32s3/mpconfigboard.h

@@ -26,3 +26,6 @@
 #define DEFAULT_UART_BUS_TX (&pin_GPIO40)
 
 #define DOUBLE_TAP_PIN (&pin_GPIO38)
+
+// #define CIRCUITPY_CONSOLE_UART_TX DEFAULT_UART_BUS_TX
+// #define CIRCUITPY_CONSOLE_UART_RX DEFAULT_UART_BUS_RX

ports/espressif/common-hal/_bleio/Adapter.c

@@ -323,6 +323,13 @@ static void _convert_address(const bleio_address_obj_t *address, ble_addr_t *nim
     memcpy(nimble_address->val, (uint8_t *)address_buf_info.buf, NUM_BLEIO_ADDRESS_BYTES);
 }
 
+// Same, from a raw address. Unlike _convert_address() this cannot raise, so it is safe
+// on the path used by supervisor/shared.
+static void _convert_raw_address(const bleio_raw_address_t *address, ble_addr_t *nimble_address) {
+    nimble_address->type = address->type;
+    memcpy(nimble_address->val, address->bytes, NUM_BLEIO_ADDRESS_BYTES);
+}
+
 static int _mtu_reply(uint16_t conn_handle,
     const struct ble_gatt_error *error,
     uint16_t mtu, void *arg) {
@@ -535,7 +542,7 @@ uint32_t _common_hal_bleio_adapter_start_advertising(bleio_adapter_obj_t *self,
     bool connectable, bool anonymous, uint32_t timeout, float interval,
     const uint8_t *advertising_data, uint16_t advertising_data_len,
     const uint8_t *scan_response_data, uint16_t scan_response_data_len,
-    mp_int_t tx_power, const bleio_address_obj_t *directed_to) {
+    mp_int_t tx_power, const bleio_raw_address_t *directed_to) {
 
     if (ble_gap_adv_active() && !self->user_advertising) {
         return BLE_HS_EBUSY;
@@ -547,7 +554,7 @@ uint32_t _common_hal_bleio_adapter_start_advertising(bleio_adapter_obj_t *self,
 
     ble_addr_t peer;
     if (directed_to != NULL) {
-        _convert_address(directed_to, &peer);
+        _convert_raw_address(directed_to, &peer);
     }
 
     uint8_t own_addr_type;
@@ -558,7 +565,11 @@ uint32_t _common_hal_bleio_adapter_start_advertising(bleio_adapter_obj_t *self,
         return rc;
     }
 
-    bool high_duty_directed = directed_to != NULL && interval <= 3.5 && timeout <= 1; // Really 1.3, but it's an int
+    // High duty cycle directed advertising is capped at 1.28 seconds by the spec, so it
+    // only suits a short, finite window. An unlimited timeout is encoded as zero, which
+    // would otherwise satisfy "timeout <= 1" and pick a type that stops almost at once.
+    bool high_duty_directed = directed_to != NULL && interval <= 3.5 &&
+        timeout != 0 && timeout <= 1; // Really 1.3, but it's an int
 
     uint32_t timeout_ms = timeout * 1000;
 
@@ -706,13 +717,20 @@ void common_hal_bleio_adapter_start_advertising(bleio_adapter_obj_t *self, bool
             BLE_HS_FOREVER / 1000 - 1);
     }
 
+    // Convert here, where raising is allowed. The internal call must stay raise-free
+    // because supervisor/shared uses it too.
+    bleio_raw_address_t raw_directed_to;
+    if (directed_to != NULL) {
+        bleio_address_to_raw(directed_to, &raw_directed_to);
+    }
+
     CHECK_NIMBLE_ERROR(_common_hal_bleio_adapter_start_advertising(self, connectable, anonymous, timeout, interval,
         advertising_data_bufinfo->buf,
         advertising_data_bufinfo->len,
         scan_response_data_bufinfo->buf,
         scan_response_data_bufinfo->len,
         tx_power,
-        directed_to));
+        directed_to != NULL ? &raw_directed_to : NULL));
     self->user_advertising = true;
 }
 
@@ -837,13 +855,13 @@ void bleio_adapter_reset(bleio_adapter_obj_t *adapter) {
 
     // Wait up to 125 ms (128 ticks) for disconnect to complete. This should be
     // greater than most connection intervals.
-    bool any_connected = false;
+    bool any_connected;
     uint64_t start_ticks = supervisor_ticks_ms64();
-    while (any_connected && supervisor_ticks_ms64() - start_ticks < 128) {
+    do {
         any_connected = false;
         for (size_t i = 0; i < BLEIO_TOTAL_CONNECTION_COUNT; i++) {
             bleio_connection_internal_t *connection = &bleio_connections[i];
             any_connected |= connection->conn_handle != BLEIO_HANDLE_INVALID;
         }
-    }
+    } while (any_connected && supervisor_ticks_ms64() - start_ticks < 128);
 }

ports/espressif/common-hal/_bleio/__init__.c

@@ -35,9 +35,12 @@ void bleio_user_reset(void) {
     if (!common_hal_bleio_adapter_get_enabled(&common_hal_bleio_adapter_obj)) {
         return;
     }
-    // Stop any user scanning or advertising, and stop all connections.
-    // TODO: Don't stop BLE workflow connection.
-    bleio_adapter_reset(&common_hal_bleio_adapter_obj);
+    // Stop any user scanning or advertising. Deliberately not bleio_adapter_reset(),
+    // which also drops every connection, including the BLE workflow's. Connections
+    // that user code created are torn down by bleio_reset() instead, which runs only
+    // when user code imported _bleio. This matches the nordic port.
+    common_hal_bleio_adapter_stop_scan(&common_hal_bleio_adapter_obj);
+    common_hal_bleio_adapter_stop_advertising(&common_hal_bleio_adapter_obj);
 
     ble_event_remove_heap_handlers();
 
@@ -52,11 +55,21 @@ void bleio_reset(void) {
         return;
     }
 
+    // If user code never imported _bleio, then it cannot have added anything to the
+    // GATT attribute table or created any connections, so there is nothing to tear
+    // down. Skipping matters: the disable/enable cycle below drops any BLE workflow
+    // session that is in progress. Measured: without this return, the first keypress
+    // at the REPL prompt disconnects the workflow client.
+    if (!bleio_user_imported()) {
+        return;
+    }
+
     supervisor_stop_bluetooth();
     ble_event_reset();
     bleio_adapter_reset(&common_hal_bleio_adapter_obj);
     common_hal_bleio_adapter_set_enabled(&common_hal_bleio_adapter_obj, false);
     supervisor_start_bluetooth();
+    bleio_clear_user_imported();
 }
 
 // The singleton _bleio.Adapter object, bound to _bleio.adapter

ports/espressif/common-hal/_bleio/ble_events.c

@@ -28,11 +28,14 @@ void ble_event_reset(void) {
 void ble_event_remove_heap_handlers(void) {
     ble_event_handler_entry_t *it = MP_STATE_VM(ble_event_handler_entries);
     while (it != NULL) {
-        // If the param is on the heap, then delete the handler.
-        if (gc_ptr_on_heap(it->param)) {
+        // Capture next before removing, because removing clears the entry's next.
+        ble_event_handler_entry_t *next = it->next;
+        // If the entry or its param is on the heap, then delete the handler.
+        // Both are checked because the heap they live on is about to go away.
+        if (gc_ptr_on_heap(it) || gc_ptr_on_heap(it->param)) {
             ble_event_remove_handler(it->func, it->param);
         }
-        it = it->next;
+        it = next;
     }
 }
 

ports/nordic/bluetooth/ble_drv.c

@@ -141,11 +141,14 @@ void ble_drv_reset(void) {
 void ble_drv_remove_heap_handlers(void) {
     ble_drv_evt_handler_entry_t *it = MP_STATE_VM(ble_drv_evt_handler_entries);
     while (it != NULL) {
-        // If the param is on the heap, then delete the handler.
-        if (gc_ptr_on_heap(it->param)) {
+        // Capture next before removing, because removing clears the entry's next.
+        ble_drv_evt_handler_entry_t *next = it->next;
+        // If the entry or its param is on the heap, then delete the handler.
+        // Both are checked because the heap they live on is about to go away.
+        if (gc_ptr_on_heap(it) || gc_ptr_on_heap(it->param)) {
             ble_drv_remove_event_handler(it->func, it->param);
         }
-        it = it->next;
+        it = next;
     }
 }
 

ports/nordic/boards/feather_nrf52840_express/mpconfigboard.h

@@ -46,3 +46,7 @@
 
 #define DEFAULT_UART_BUS_RX         (&pin_P0_24)
 #define DEFAULT_UART_BUS_TX         (&pin_P0_25)
+
+// Uncomment to get a serial console on the TX and RX pins, in addition to USB.
+// #define CIRCUITPY_CONSOLE_UART_TX (DEFAULT_UART_BUS_TX)
+// #define CIRCUITPY_CONSOLE_UART_RX (DEFAULT_UART_BUS_RX)

ports/nordic/boards/pca10100/mpconfigboard.mk

@@ -9,4 +9,5 @@ INTERNAL_FLASH_FILESYSTEM = 1
 
 CIRCUITPY_ONEWIREIO = 0
 CIRCUITPY_AUDIOMIXER = 0
+CIRCUITPY_NVM = 0
 CIRCUITPY_RAINBOWIO = 0