rename and improve PacketBuffer packet length property

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;
             }
         }
@@ -202,7 +208,6 @@ void common_hal_bleio_packet_buffer_construct(
     }
 
     if (incoming) {
-        // This is a macro.
         ringbuf_alloc(&self->ringbuf, buffer_size * (sizeof(uint16_t) + characteristic->max_length), false);
 
         if (self->ringbuf.buf == NULL) {
@@ -249,7 +254,7 @@ 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_count(&self->ringbuf) < 2) {
         return 0;
     }
@@ -280,7 +285,7 @@ void common_hal_bleio_packet_buffer_write(bleio_packet_buffer_obj_t *self, uint8
     if (self->conn_handle == BLE_CONN_HANDLE_INVALID) {
         return;
     }
-    uint16_t packet_size = common_hal_bleio_packet_buffer_get_packet_size(self);
+    uint16_t packet_size = common_hal_bleio_packet_buffer_get_incoming_packet_length(self);
     uint16_t max_size = packet_size - len;
     while (max_size < self->pending_size && self->conn_handle != BLE_CONN_HANDLE_INVALID) {
         RUN_BACKGROUND_TASKS;
@@ -308,26 +313,30 @@ 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) {
-    uint16_t mtu;
-    if (self->conn_handle == BLE_CONN_HANDLE_INVALID) {
-        return 0;
-    }
-    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) {
-            break;
+mp_int_t common_hal_bleio_packet_buffer_get_incoming_packet_length(bleio_packet_buffer_obj_t *self) {
+    // If this PacketBuffer is being used for NOTIFY or INDICATE from
+    // a remote service, 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 a long as the characteristic
+    // will permit, whether or not we're connected.
+
+    if (self->characteristic != NULL &&
+        self->characteristic->service != NULL &&
+        self->characteristic->service->is_remote &&
+        (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;
     }
-    if (connection->mtu == 0) {
-        mtu = BLE_GATT_ATT_MTU_DEFAULT;
-    }
-    if (self->characteristic->max_length > mtu) {
-        mtu = self->characteristic->max_length;
-    }
-    uint16_t att_overhead = 3;
-    return mtu - att_overhead;
+    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,

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);

shared-bindings/_bleio/PacketBuffer.c

@@ -42,7 +42,7 @@
 //|
 //| Accumulates a Characteristic's incoming packets in a FIFO buffer and facilitates packet aware
 //| outgoing writes. A packet's size is either the characteristic length or the maximum transmission
-//| unit (MTU), whichever is smaller. The MTU can change so check `packet_size` before creating a
+//| unit (MTU), whichever is smaller. The MTU can change so check `incoming_packet_length` before creating a
 //| buffer to store data.
 //|
 //| When we're the server, we ignore all connections besides the first to subscribe to
@@ -71,7 +71,7 @@ STATIC mp_obj_t bleio_packet_buffer_make_new(const mp_obj_type_t *type, size_t n
 
     const mp_obj_t characteristic = args[ARG_characteristic].u_obj;
 
-    const int buffer_size = args[ARG_buffer_size].u_int;
+    const mp_int_t buffer_size = args[ARG_buffer_size].u_int;
     if (buffer_size < 1) {
         mp_raise_ValueError_varg(translate("%q must be >= 1"), MP_QSTR_buffer_size);
     }
@@ -97,7 +97,7 @@ STATIC void check_for_deinit(bleio_packet_buffer_obj_t *self) {
 //|   .. method:: readinto(buf)
 //|
 //|     Reads a single BLE packet into the ``buf``. Raises an exception if the next packet is longer
-//|     than the given buffer. Use `packet_size` to read the maximum length of a single packet.
+//|     than the given buffer. Use `incoming_packet_length` to read the maximum length of a single packet.
 //|
 //|     :return: number of bytes read and stored into ``buf``
 //|     :rtype: int
@@ -109,7 +109,7 @@ STATIC mp_obj_t bleio_packet_buffer_readinto(mp_obj_t self_in, mp_obj_t buffer_o
     mp_buffer_info_t bufinfo;
     mp_get_buffer_raise(buffer_obj, &bufinfo, MP_BUFFER_WRITE);
 
-    int size = common_hal_bleio_packet_buffer_readinto(self, bufinfo.buf, bufinfo.len);
+    mp_int_t size = common_hal_bleio_packet_buffer_readinto(self, bufinfo.buf, bufinfo.len);
     if (size < 0) {
         mp_raise_ValueError_varg(translate("Buffer too short by %d bytes"), size * -1);
     }
@@ -166,33 +166,39 @@ STATIC mp_obj_t bleio_packet_buffer_deinit(mp_obj_t self_in) {
 }
 STATIC MP_DEFINE_CONST_FUN_OBJ_1(bleio_packet_buffer_deinit_obj, bleio_packet_buffer_deinit);
 
-//|   .. attribute:: packet_size
+//|   .. attribute:: incoming_packet_length
 //|
-//|     Maximum size of each packet in bytes. This is the minimum of the Characteristic length and
-//|     the negotiated Maximum Transfer Unit (MTU).
+//|     Maximum size of a packet.
+//|     If the packet is arriving from a remote service via notify or indicate,
+//|     the maximum size is `Connection.max_packet_length`.
+//|     Otherwise it is the ``max_length`` of the `Characteristic`.
 //|
-STATIC mp_obj_t bleio_packet_buffer_get_packet_size(mp_obj_t self_in) {
+STATIC mp_obj_t bleio_packet_buffer_get_incoming_packet_length(mp_obj_t self_in) {
     bleio_packet_buffer_obj_t *self = MP_OBJ_TO_PTR(self_in);
 
-    return MP_OBJ_NEW_SMALL_INT(common_hal_bleio_packet_buffer_get_packet_size(self));
+    mp_int_t size = common_hal_bleio_packet_buffer_get_incoming_packet_length(self);
+    if (size < 0) {
+        mp_raise_ValueError(translate("No connection: size cannot be determined"));
+    }
+    return MP_OBJ_NEW_SMALL_INT(size);
 }
-STATIC MP_DEFINE_CONST_FUN_OBJ_1(bleio_packet_buffer_get_packet_size_obj, bleio_packet_buffer_get_packet_size);
+STATIC MP_DEFINE_CONST_FUN_OBJ_1(bleio_packet_buffer_get_incoming_packet_length_obj, bleio_packet_buffer_get_incoming_packet_length);
 
-const mp_obj_property_t bleio_packet_buffer_packet_size_obj = {
+const mp_obj_property_t bleio_packet_buffer_incoming_packet_length_obj = {
     .base.type = &mp_type_property,
-    .proxy = { (mp_obj_t)&bleio_packet_buffer_get_packet_size_obj,
+    .proxy = { (mp_obj_t)&bleio_packet_buffer_get_incoming_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_packet_buffer_locals_dict_table[] = {
-    { MP_ROM_QSTR(MP_QSTR_deinit),             MP_ROM_PTR(&bleio_packet_buffer_deinit_obj) },
+    { MP_ROM_QSTR(MP_QSTR_deinit),                   MP_ROM_PTR(&bleio_packet_buffer_deinit_obj) },
 
     // Standard stream methods.
-    { MP_OBJ_NEW_QSTR(MP_QSTR_readinto),       MP_ROM_PTR(&bleio_packet_buffer_readinto_obj) },
-    { MP_OBJ_NEW_QSTR(MP_QSTR_write),          MP_ROM_PTR(&bleio_packet_buffer_write_obj) },
+    { MP_OBJ_NEW_QSTR(MP_QSTR_readinto),             MP_ROM_PTR(&bleio_packet_buffer_readinto_obj) },
+    { MP_OBJ_NEW_QSTR(MP_QSTR_write),                MP_ROM_PTR(&bleio_packet_buffer_write_obj) },
 
-    { MP_OBJ_NEW_QSTR(MP_QSTR_packet_size),    MP_ROM_PTR(&bleio_packet_buffer_packet_size_obj) },
+    { MP_OBJ_NEW_QSTR(MP_QSTR_incoming_packet_length), MP_ROM_PTR(&bleio_packet_buffer_incoming_packet_length_obj) },
 };
 
 STATIC MP_DEFINE_CONST_DICT(bleio_packet_buffer_locals_dict, bleio_packet_buffer_locals_dict_table);