further cleanup and bug fixing

Author: dhalbert

ports/nrf/common-hal/_bleio/Characteristic.c

@@ -127,6 +127,7 @@ size_t common_hal_bleio_characteristic_get_value(bleio_characteristic_obj_t *sel
             // self->value is set by evt handler.
             return common_hal_bleio_gattc_read(self->handle, conn_handle, buf, len);
         } else {
+            // conn_handle is ignored for non-system attributes.
             return common_hal_bleio_gatts_read(self->handle, conn_handle, buf, len);
         }
     }
@@ -152,6 +153,7 @@ void common_hal_bleio_characteristic_set_value(bleio_characteristic_obj_t *self,
                                          (self->props & CHAR_PROP_WRITE_NO_RESPONSE));
         } else {
             // Always write the value locally even if no connections are active.
+            // conn_handle is ignored for non-system attributes, so we use BLE_CONN_HANDLE_INVALID.
             common_hal_bleio_gatts_write(self->handle, BLE_CONN_HANDLE_INVALID, bufinfo);
             // Check to see if we need to notify or indicate any active connections.
             for (size_t i = 0; i < BLEIO_TOTAL_CONNECTION_COUNT; i++) {

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

@@ -371,6 +371,11 @@ mp_float_t common_hal_bleio_connection_get_connection_interval(bleio_connection_
     return 1.25f * self->conn_params.min_conn_interval;
 }
 
+// Return the current negotiated MTU length, minus overhead.
+mp_int_t common_hal_bleio_connection_get_max_packet_length(bleio_connection_internal_t *self) {
+    return (self->mtu == 0 ? BLE_GATT_ATT_MTU_DEFAULT : self->mtu) - 3;
+}
+
 void common_hal_bleio_connection_set_connection_interval(bleio_connection_internal_t *self, mp_float_t new_interval) {
     self->conn_params_updating = true;
     uint16_t interval = new_interval / 1.25f;
@@ -752,3 +757,16 @@ mp_obj_t bleio_connection_new_from_internal(bleio_connection_internal_t* interna
 
     return MP_OBJ_FROM_PTR(connection);
 }
+
+// Find the connection that uses the given conn_handle. Return NULL if not found.
+bleio_connection_internal_t *bleio_conn_handle_to_connection(uint16_t conn_handle) {
+    bleio_connection_internal_t *connection;
+    for (size_t i = 0; i < BLEIO_TOTAL_CONNECTION_COUNT; i++) {
+        connection = &bleio_connections[i];
+        if (connection->conn_handle == conn_handle) {
+            return connection;
+        }
+    }
+
+    return NULL;
+}

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

@@ -87,5 +87,6 @@ bool connection_on_ble_evt(ble_evt_t *ble_evt, void *self_in);
 
 uint16_t bleio_connection_get_conn_handle(bleio_connection_obj_t *self);
 mp_obj_t bleio_connection_new_from_internal(bleio_connection_internal_t* connection);
+bleio_connection_internal_t *bleio_conn_handle_to_connection(uint16_t conn_handle);
 
 #endif // MICROPY_INCLUDED_NRF_COMMON_HAL_BLEIO_CONNECTION_H

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

@@ -107,12 +107,18 @@ STATIC uint32_t queue_next_write(bleio_packet_buffer_obj_t *self) {
 }
 
 STATIC bool packet_buffer_on_ble_client_evt(ble_evt_t *ble_evt, void *param) {
-    bleio_packet_buffer_obj_t *self = (bleio_packet_buffer_obj_t *) param;
+    const uint16_t evt_id = ble_evt->header.evt_id;
+    // Check if this is a GATTC event so we can make sure the conn_handle is valid.
+    if (evt_id < BLE_GATTC_EVT_BASE || evt_id > BLE_GATTC_EVT_LAST) {
+        return false;
+    }
+
     uint16_t conn_handle = ble_evt->evt.gattc_evt.conn_handle;
+    bleio_packet_buffer_obj_t *self = (bleio_packet_buffer_obj_t *) param;
     if (conn_handle != self->conn_handle) {
         return false;
     }
-    switch (ble_evt->header.evt_id) {
+    switch (evt_id) {
         case BLE_GATTC_EVT_HVX: {
             // A remote service wrote to this characteristic.
             ble_gattc_evt_hvx_t* evt_hvx = &ble_evt->evt.gattc_evt.params.hvx;
@@ -142,9 +148,9 @@ STATIC bool packet_buffer_on_ble_client_evt(ble_evt_t *ble_evt, void *param) {
 
 STATIC bool packet_buffer_on_ble_server_evt(ble_evt_t *ble_evt, void *param) {
     bleio_packet_buffer_obj_t *self = (bleio_packet_buffer_obj_t *) param;
-    uint16_t conn_handle = ble_evt->evt.gatts_evt.conn_handle;
     switch (ble_evt->header.evt_id) {
         case BLE_GATTS_EVT_WRITE: {
+            uint16_t conn_handle = ble_evt->evt.gatts_evt.conn_handle;
             // A client wrote to this server characteristic.
 
             ble_gatts_evt_write_t *evt_write = &ble_evt->evt.gatts_evt.params.write;
@@ -168,7 +174,7 @@ STATIC bool packet_buffer_on_ble_server_evt(ble_evt_t *ble_evt, void *param) {
             break;
         }
         case BLE_GAP_EVT_DISCONNECTED: {
-            if (self->conn_handle == conn_handle) {
+            if (self->conn_handle == ble_evt->evt.gap_evt.conn_handle) {
                 self->conn_handle = BLE_CONN_HANDLE_INVALID;
             }
         }
@@ -246,21 +252,20 @@ void common_hal_bleio_packet_buffer_construct(
     }
 }
 
-int common_hal_bleio_packet_buffer_readinto(bleio_packet_buffer_obj_t *self, uint8_t *data, size_t len) {
+mp_int_t common_hal_bleio_packet_buffer_readinto(bleio_packet_buffer_obj_t *self, uint8_t *data, size_t len) {
     if (ringbuf_num_filled(&self->ringbuf) < 2) {
         return 0;
     }
 
-    uint16_t packet_length;
-    int ret;
-
     // Copy received data. Lock out write interrupt handler while copying.
     uint8_t is_nested_critical_region;
     sd_nvic_critical_region_enter(&is_nested_critical_region);
 
-    // Get packet length first.
+    // 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));
 
+    mp_int_t ret;
     if (packet_length > len) {
         // Packet is longer than requested. Return negative of overrun value.
         ret = len - packet_length;
@@ -311,26 +316,29 @@ void common_hal_bleio_packet_buffer_write(bleio_packet_buffer_obj_t *self, uint8
     }
 }
 
-uint16_t common_hal_bleio_packet_buffer_get_packet_size(bleio_packet_buffer_obj_t *self) {
-    // First, assume default MTU size.
-    uint16_t mtu = BLE_GATT_ATT_MTU_DEFAULT;
-
-    // If there's a connection, get its actual MTU.
-    if (self->conn_handle != BLE_CONN_HANDLE_INVALID) {
-        bleio_connection_internal_t *connection;
-        for (size_t i = 0; i < BLEIO_TOTAL_CONNECTION_COUNT; i++) {
-            connection = &bleio_connections[i];
-            if (connection->conn_handle == self->conn_handle) {
-                if (connection->mtu != 0) {
-                    mtu = connection->mtu;
-                }
-                break;
-            }
+mp_int_t common_hal_bleio_packet_buffer_get_packet_size(bleio_packet_buffer_obj_t *self) {
+    // If this PacketBuffer is being used for NOTIFY or INDICATE,
+    // the maximum size is what can be sent in one
+    // BLE packet. But we must be connected to know that value.
+    //
+    // Otherwise it can be as long as the characteristic
+    // will permit, whether or not we're connected.
+
+    if (self->characteristic != NULL &&
+        self->characteristic->service != NULL &&
+        (common_hal_bleio_characteristic_get_properties(self->characteristic) &
+         (CHAR_PROP_INDICATE | CHAR_PROP_NOTIFY)) &&
+        self->conn_handle != BLE_CONN_HANDLE_INVALID) {
+        bleio_connection_internal_t *connection = bleio_conn_handle_to_connection(self->conn_handle);
+        if (connection) {
+            return MIN(common_hal_bleio_connection_get_max_packet_length(connection),
+                       self->characteristic->max_length);
         }
+        // There's no current connection, so we don't know the MTU, and
+        // we can't tell what the largest incoming packet length would be.
+        return -1;
     }
-
-    // 3 is bytes of ATT overhead.
-    return MIN(mtu - 3, self->characteristic->max_length);
+    return self->characteristic->max_length;
 }
 
 bool common_hal_bleio_packet_buffer_deinited(bleio_packet_buffer_obj_t *self) {

ports/nrf/common-hal/_bleio/PacketBuffer.h

@@ -41,6 +41,8 @@ typedef struct {
     // the other is waiting to be queued and can be extended.
     uint8_t* outgoing[2];
     uint16_t pending_size;
+    // We remember the conn_handle so we can do a NOTIFY/INDICATE to a client.
+    // We can find out the conn_handle on a Characteristic write or a CCCD write (but not a read).
     uint16_t conn_handle;
     uint8_t pending_index;
     uint8_t write_type;

ports/nrf/common-hal/_bleio/Service.h

@@ -39,6 +39,8 @@ typedef struct bleio_service_obj {
     bool is_remote;
     bool is_secondary;
     bleio_uuid_obj_t *uuid;
+    // The connection object is set only when this is a remote service.
+    // A local service doesn't know the connection.
     mp_obj_t connection;
     mp_obj_list_t *characteristic_list;
     // Range of attribute handles of this remote service.

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

@@ -118,8 +118,8 @@ void common_hal_bleio_check_connected(uint16_t conn_handle) {
 
 // GATTS read of a Characteristic or Descriptor.
 size_t common_hal_bleio_gatts_read(uint16_t handle, uint16_t conn_handle, uint8_t* buf, size_t len) {
-    // conn_handle might be BLE_CONN_HANDLE_INVALID if we're not connected, but that's OK, because
-    // we can still read and write the local value.
+    // conn_handle is ignored unless this is a system attribute.
+    // If we're not connected, but that's OK, because we can still read and write the local value.
 
     ble_gatts_value_t gatts_value = {
         .p_value = buf,
@@ -132,8 +132,8 @@ size_t common_hal_bleio_gatts_read(uint16_t handle, uint16_t conn_handle, uint8_
 }
 
 void common_hal_bleio_gatts_write(uint16_t handle, uint16_t conn_handle, mp_buffer_info_t *bufinfo) {
-    // conn_handle might be BLE_CONN_HANDLE_INVALID if we're not connected, but that's OK, because
-    // we can still read and write the local value.
+    // conn_handle is ignored unless this is a system attribute.
+    // If we're not connected, but that's OK, because we can still read and write the local value.
 
     ble_gatts_value_t gatts_value = {
         .p_value = bufinfo->buf,

ports/nrf/mpconfigport.mk

@@ -51,14 +51,8 @@ endif
 # frequencyio not yet implemented
 CIRCUITPY_FREQUENCYIO = 0
 
-ifndef CIRCUITPY_PROTOMATTER
-CIRCUITPY_RGBMATRIX = 1
-CIRCUITPY_FRAMEBUFFERIO = 1
-endif
-
-ifndef CIRCUITPY_ULAB
-CIRCUITPY_ULAB = 1
-endif
+CIRCUITPY_RGBMATRIX ?= 1
+CIRCUITPY_FRAMEBUFFERIO ?= 1
 
 # nRF52840-specific
 

shared-bindings/_bleio/Connection.c

@@ -214,6 +214,25 @@ STATIC mp_obj_t bleio_connection_get_connection_interval(mp_obj_t self_in) {
 }
 STATIC MP_DEFINE_CONST_FUN_OBJ_1(bleio_connection_get_connection_interval_obj, bleio_connection_get_connection_interval);
 
+//|   .. attribute:: max_packet_length
+//|
+//|     The maximum number of data bytes that can be sent in a single transmission,
+//|     not including overhead bytes.
+//|
+//|     This is the maximum number of bytes that can be sent in a notification,
+//|     which must be sent in a single packet.
+//|     But for a regular characteristic read or write, may be sent in multiple packets,
+//|     so this limit does not apply.
+//|
+STATIC mp_obj_t bleio_connection_get_max_packet_length(mp_obj_t self_in) {
+    bleio_connection_obj_t *self = MP_OBJ_TO_PTR(self_in);
+
+    bleio_connection_ensure_connected(self);
+    return mp_obj_new_int(common_hal_bleio_connection_get_max_packet_length(self->connection));
+}
+STATIC MP_DEFINE_CONST_FUN_OBJ_1(bleio_connection_get_max_packet_length_obj, bleio_connection_get_max_packet_length);
+
+
 STATIC mp_obj_t bleio_connection_set_connection_interval(mp_obj_t self_in, mp_obj_t interval_in) {
     bleio_connection_obj_t *self = MP_OBJ_TO_PTR(self_in);
 
@@ -233,6 +252,13 @@ const mp_obj_property_t bleio_connection_connection_interval_obj = {
                (mp_obj_t)&mp_const_none_obj },
 };
 
+const mp_obj_property_t bleio_connection_max_packet_length_obj = {
+    .base.type = &mp_type_property,
+    .proxy = { (mp_obj_t)&bleio_connection_get_max_packet_length_obj,
+               (mp_obj_t)&mp_const_none_obj,
+               (mp_obj_t)&mp_const_none_obj },
+};
+
 STATIC const mp_rom_map_elem_t bleio_connection_locals_dict_table[] = {
     // Methods
     { MP_ROM_QSTR(MP_QSTR_pair),                     MP_ROM_PTR(&bleio_connection_pair_obj) },
@@ -243,7 +269,7 @@ STATIC const mp_rom_map_elem_t bleio_connection_locals_dict_table[] = {
     { MP_ROM_QSTR(MP_QSTR_connected),           MP_ROM_PTR(&bleio_connection_connected_obj) },
     { MP_ROM_QSTR(MP_QSTR_paired),              MP_ROM_PTR(&bleio_connection_paired_obj) },
     { MP_ROM_QSTR(MP_QSTR_connection_interval), MP_ROM_PTR(&bleio_connection_connection_interval_obj) },
-
+    { MP_ROM_QSTR(MP_QSTR_max_packet_length),   MP_ROM_PTR(&bleio_connection_max_packet_length_obj) },
 };
 
 STATIC MP_DEFINE_CONST_DICT(bleio_connection_locals_dict, bleio_connection_locals_dict_table);

shared-bindings/_bleio/Connection.h

@@ -34,11 +34,12 @@
 
 extern const mp_obj_type_t bleio_connection_type;
 
-extern void common_hal_bleio_connection_pair(bleio_connection_internal_t *self, bool bond);
-extern void common_hal_bleio_connection_disconnect(bleio_connection_internal_t *self);
-extern bool common_hal_bleio_connection_get_connected(bleio_connection_obj_t *self);
-extern bool common_hal_bleio_connection_get_paired(bleio_connection_obj_t *self);
-extern mp_obj_tuple_t *common_hal_bleio_connection_discover_remote_services(bleio_connection_obj_t *self, mp_obj_t service_uuids_whitelist);
+void common_hal_bleio_connection_pair(bleio_connection_internal_t *self, bool bond);
+void common_hal_bleio_connection_disconnect(bleio_connection_internal_t *self);
+bool common_hal_bleio_connection_get_connected(bleio_connection_obj_t *self);
+mp_int_t common_hal_bleio_connection_get_max_packet_length(bleio_connection_internal_t *self);
+bool common_hal_bleio_connection_get_paired(bleio_connection_obj_t *self);
+mp_obj_tuple_t *common_hal_bleio_connection_discover_remote_services(bleio_connection_obj_t *self, mp_obj_t service_uuids_whitelist);
 
 mp_float_t common_hal_bleio_connection_get_connection_interval(bleio_connection_internal_t *self);
 void common_hal_bleio_connection_set_connection_interval(bleio_connection_internal_t *self, mp_float_t new_interval);