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/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/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

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

@@ -248,6 +248,14 @@ static bool adapter_on_ble_evt(ble_evt_t *ble_evt, void *self_in) {
             connection->connection_obj = mp_const_none;
             connection->pair_status = PAIR_NOT_PAIRED;
             connection->mtu = 0;
+            // Remember where the peer connected from. A central that does not
+            // distribute an identity address during bonding still has to reach us
+            // somehow, and this is the only address we will ever learn for it.
+            connection->peer_addr = connected->peer_addr;
+            // Start from a clean keyset. The SoftDevice only fills in the keys the
+            // peer actually distributes, so a recycled connection slot would other-
+            // wise carry the previous peer's keys into this peer's stored bond.
+            bonding_clear_keys(&connection->bonding_keys);
 
             ble_drv_add_event_handler_entry(&connection->handler_entry, connection_on_ble_evt, connection);
             self->connection_objs = NULL;
@@ -615,6 +623,13 @@ static void _convert_address(const bleio_address_obj_t *address, ble_gap_addr_t
     memcpy(sd_address->addr, (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_gap_addr_t *sd_address) {
+    sd_address->addr_type = address->type;
+    memcpy(sd_address->addr, address->bytes, NUM_BLEIO_ADDRESS_BYTES);
+}
+
 mp_obj_t common_hal_bleio_adapter_connect(bleio_adapter_obj_t *self, bleio_address_obj_t *address, mp_float_t timeout) {
     ble_gap_addr_t addr;
     _convert_address(address, &addr);
@@ -735,7 +750,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 (self->current_advertising_data != NULL && self->current_advertising_data == self->advertising_data) {
         return NRF_ERROR_BUSY;
     }
@@ -753,6 +768,26 @@ uint32_t _common_hal_bleio_adapter_start_advertising(bleio_adapter_obj_t *self,
     if (timeout == 0) {
         timeout = BLE_GAP_ADV_TIMEOUT_GENERAL_UNLIMITED;
     }
+    // Anonymous advertising means the BLE workflow is trying to reconnect to a bond
+    // without being trackable by anyone else. That works for a central that uses privacy,
+    // because it holds our IRK and can resolve the private address we advertise under.
+    //
+    // A central that distributed no IRK does not use privacy. It cannot resolve us, so it
+    // will never recognize an undirected private advertisement as us -- but it does
+    // connect from a stable address, which we stored with the bond and can aim a directed
+    // advertisement at. That is the only thing such a host will act on, and it is much
+    // faster besides. Centrals that do use privacy keep the undirected path, which is
+    // also what Apple's accessory guidelines require of us.
+    ble_gap_addr_t reconnect_peer;
+    bool directed_reconnect = anonymous && directed_to == NULL &&
+        bonding_load_directed_reconnect_address(&reconnect_peer);
+    if (directed_reconnect) {
+        anonymous = false;
+        // ADV_DIRECT_IND carries no advertising data, so drop whatever we were given.
+        advertising_data_len = 0;
+        scan_response_data_len = 0;
+    }
+
     uint32_t err_code;
     bool extended = advertising_data_len > BLE_GAP_ADV_SET_DATA_SIZE_MAX ||
         scan_response_data_len > BLE_GAP_ADV_SET_DATA_SIZE_MAX;
@@ -771,13 +806,18 @@ uint32_t _common_hal_bleio_adapter_start_advertising(bleio_adapter_obj_t *self,
     } else if (connectable) {
         if (directed_to == NULL) {
             adv_type = BLE_GAP_ADV_TYPE_CONNECTABLE_SCANNABLE_UNDIRECTED;
-        } else if (interval <= 3.5 && timeout <= 1.3) {
+            // 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.3" and pick
+            // a type that stops almost immediately.
+        } else if (interval <= 3.5 &&
+                   timeout != BLE_GAP_ADV_TIMEOUT_GENERAL_UNLIMITED && timeout <= 1.3) {
             adv_type = BLE_GAP_ADV_TYPE_CONNECTABLE_NONSCANNABLE_DIRECTED_HIGH_DUTY_CYCLE;
-            _convert_address(directed_to, &peer_address);
+            _convert_raw_address(directed_to, &peer_address);
             peer = &peer_address;
         } else {
             adv_type = BLE_GAP_ADV_TYPE_CONNECTABLE_NONSCANNABLE_DIRECTED;
-            _convert_address(directed_to, &peer_address);
+            _convert_raw_address(directed_to, &peer_address);
             peer = &peer_address;
         }
     } else if (scan_response_data_len > 0) {
@@ -786,6 +826,12 @@ uint32_t _common_hal_bleio_adapter_start_advertising(bleio_adapter_obj_t *self,
         adv_type = BLE_GAP_ADV_TYPE_NONCONNECTABLE_NONSCANNABLE_UNDIRECTED;
     }
 
+    // Low duty cycle, so it can run for as long as the workflow keeps advertising.
+    if (directed_reconnect) {
+        adv_type = BLE_GAP_ADV_TYPE_CONNECTABLE_NONSCANNABLE_DIRECTED;
+        peer = &reconnect_peer;
+    }
+
     if (anonymous) {
         ble_gap_privacy_params_t privacy = {
             .privacy_mode = BLE_GAP_PRIVACY_MODE_DEVICE_PRIVACY,
@@ -794,7 +840,13 @@ uint32_t _common_hal_bleio_adapter_start_advertising(bleio_adapter_obj_t *self,
             // advertising. This prevents a potential race condition where we
             // fire off a beacon with the same advertising data but a new MAC
             // address just as we tear down the connection.
-            .private_addr_cycle_s = timeout + 1,
+            //
+            // Unlimited advertising has no such moment, and timeout + 1 would then
+            // rotate every second, too fast for a central to resolve an address and
+            // still connect to it before it changes. Zero asks the SoftDevice for its
+            // default of BLE_GAP_DEFAULT_PRIVATE_ADDR_CYCLE_INTERVAL_S (15 minutes).
+            .private_addr_cycle_s =
+                timeout == BLE_GAP_ADV_TIMEOUT_GENERAL_UNLIMITED ? 0 : timeout + 1,
             .p_device_irk = NULL,
         };
         err_code = sd_ble_gap_privacy_set(&privacy);
@@ -905,13 +957,20 @@ void common_hal_bleio_adapter_start_advertising(bleio_adapter_obj_t *self, bool
     memcpy(self->advertising_data, advertising_data_bufinfo->buf, advertising_data_bufinfo->len);
     memcpy(self->scan_response_data, scan_response_data_bufinfo->buf, scan_response_data_bufinfo->len);
 
+    // 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_nrf_error(_common_hal_bleio_adapter_start_advertising(self, connectable, anonymous, timeout, interval,
         self->advertising_data,
         advertising_data_bufinfo->len,
         self->scan_response_data,
         scan_response_data_bufinfo->len,
         tx_power,
-        directed_to));
+        directed_to != NULL ? &raw_directed_to : NULL));
     self->user_advertising = true;
 }
 
@@ -998,13 +1057,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 != BLE_CONN_HANDLE_INVALID;
         }
-    }
+    } while (any_connected && supervisor_ticks_ms64() - start_ticks < 128);
 }

ports/nordic/common-hal/_bleio/Connection.c

@@ -314,6 +314,7 @@ void bleio_connection_clear(bleio_connection_internal_t *self) {
     self->conn_handle = BLE_CONN_HANDLE_INVALID;
     self->pair_status = PAIR_NOT_PAIRED;
     self->is_central = false;
+    memset(&self->peer_addr, 0, sizeof(self->peer_addr));
     bonding_clear_keys(&self->bonding_keys);
 }
 

ports/nordic/common-hal/_bleio/Connection.h

@@ -31,6 +31,10 @@ typedef enum {
 typedef struct {
     uint16_t conn_handle;
     bool is_central;
+    // Address the peer used to establish this connection. Not necessarily an identity
+    // address: a peer using privacy connects from a resolvable private address, which
+    // is of no use once the connection is gone. Check the type before relying on it.
+    ble_gap_addr_t peer_addr;
     // Remote services discovered when this peripheral is acting as a client.
     mp_obj_list_t *remote_service_list;
     // The advertising data and scan response buffers are held by us, not by the SD, so we must

ports/nordic/common-hal/_bleio/PacketBuffer.c

@@ -481,8 +481,14 @@ 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);
+        if (self->client) {
+            ble_drv_remove_event_handler(packet_buffer_on_ble_client_evt, self);
+        } else {
+            ble_drv_remove_event_handler(packet_buffer_on_ble_server_evt, self);
+        }
         ringbuf_deinit(&self->ringbuf);
+        // Mark as deinited, so common_hal_bleio_packet_buffer_deinited() reports it.
+        self->characteristic = NULL;
     }
 }