Merge pull request #4604 from tannewt/buffer_size

Assume max characteristic size when the client

Author: dhalbert

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

@@ -78,6 +78,10 @@ bleio_service_obj_t *common_hal_bleio_characteristic_get_service(bleio_character
     return self->service;
 }
 
+size_t common_hal_bleio_characteristic_get_max_length(bleio_characteristic_obj_t *self) {
+    return self->max_length;
+}
+
 size_t common_hal_bleio_characteristic_get_value(bleio_characteristic_obj_t *self, uint8_t *buf, size_t len) {
     // Do GATT operations only if this characteristic has been added to a registered service.
     if (self->handle != BLE_GATT_HANDLE_INVALID) {

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

@@ -81,7 +81,7 @@ void bleio_packet_buffer_update(bleio_packet_buffer_obj_t *self, mp_buffer_info_
 
 void common_hal_bleio_packet_buffer_construct(
     bleio_packet_buffer_obj_t *self, bleio_characteristic_obj_t *characteristic,
-    size_t buffer_size) {
+    size_t buffer_size, size_t max_packet_size) {
 
     self->characteristic = characteristic;
     self->client = self->characteristic->service->is_remote;
@@ -101,7 +101,7 @@ void common_hal_bleio_packet_buffer_construct(
     }
 
     if (incoming) {
-        if (!ringbuf_alloc(&self->ringbuf, buffer_size * (sizeof(uint16_t) + characteristic->max_length), false)) {
+        if (!ringbuf_alloc(&self->ringbuf, buffer_size * (sizeof(uint16_t) + max_packet_size), false)) {
             mp_raise_ValueError(translate("Buffer too large and unable to allocate"));
         }
     }
@@ -110,12 +110,13 @@ void common_hal_bleio_packet_buffer_construct(
         self->packet_queued = false;
         self->pending_index = 0;
         self->pending_size = 0;
-        self->outgoing[0] = m_malloc(characteristic->max_length, false);
-        self->outgoing[1] = m_malloc(characteristic->max_length, false);
+        self->outgoing[0] = m_malloc(max_packet_size, false);
+        self->outgoing[1] = m_malloc(max_packet_size, false);
     } else {
         self->outgoing[0] = NULL;
         self->outgoing[1] = NULL;
     }
+    self->max_packet_size = max_packet_size;
 
     bleio_characteristic_set_observer(self->characteristic, self);
 }
@@ -243,15 +244,16 @@ mp_int_t common_hal_bleio_packet_buffer_get_outgoing_packet_length(bleio_packet_
         if (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);
+                return MIN(MIN(common_hal_bleio_connection_get_max_packet_length(connection),
+                               self->max_packet_size),
+                           self->characteristic->max_length);
             }
         }
         // There's no current connection, so we don't know the MTU, and
         // we can't tell what the largest outgoing packet length would be.
         return -1;
     }
-    return self->characteristic->max_length;
+    return MIN(self->characteristic->max_length, self->max_packet_size);
 }
 
 bool common_hal_bleio_packet_buffer_deinited(bleio_packet_buffer_obj_t *self) {

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

@@ -42,6 +42,7 @@ typedef struct {
     // 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).
     volatile uint16_t conn_handle;
+    uint16_t max_packet_size;
     uint8_t pending_index;
     uint8_t write_type;
     bool client;

locale/circuitpython.pot

@@ -2132,7 +2132,7 @@ msgid "Too many displays"
 msgstr ""
 
 #: ports/nrf/common-hal/_bleio/PacketBuffer.c
-msgid "Total data to write is larger than outgoing_packet_length"
+msgid "Total data to write is larger than %q"
 msgstr ""
 
 #: py/obj.c

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

@@ -131,6 +131,10 @@ size_t common_hal_bleio_characteristic_get_value(bleio_characteristic_obj_t *sel
     return 0;
 }
 
+size_t common_hal_bleio_characteristic_get_max_length(bleio_characteristic_obj_t *self) {
+    return self->max_length;
+}
+
 void common_hal_bleio_characteristic_set_value(bleio_characteristic_obj_t *self, mp_buffer_info_t *bufinfo) {
     // Do GATT operations only if this characteristic has been added to a registered service.
     if (self->handle != BLE_GATT_HANDLE_INVALID) {

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

@@ -42,7 +42,8 @@
 
 STATIC void write_to_ringbuf(bleio_packet_buffer_obj_t *self, uint8_t *data, uint16_t len) {
     if (len + sizeof(uint16_t) > ringbuf_capacity(&self->ringbuf)) {
-        // This shouldn't happen.
+        // This shouldn't happen but can if our buffer size was much smaller than
+        // the writes the client actually makes.
         return;
     }
     // Push all the data onto the ring buffer.
@@ -189,7 +190,7 @@ STATIC bool packet_buffer_on_ble_server_evt(ble_evt_t *ble_evt, void *param) {
 
 void common_hal_bleio_packet_buffer_construct(
     bleio_packet_buffer_obj_t *self, bleio_characteristic_obj_t *characteristic,
-    size_t buffer_size) {
+    size_t buffer_size, size_t max_packet_size) {
 
     self->characteristic = characteristic;
     self->client = self->characteristic->service->is_remote;
@@ -206,8 +207,11 @@ void common_hal_bleio_packet_buffer_construct(
         self->conn_handle = BLE_CONN_HANDLE_INVALID;
     }
 
+    // Cap the packet size to our implementation limits.
+    self->max_packet_size = MIN(max_packet_size, BLE_GATTS_VAR_ATTR_LEN_MAX - 3);
+
     if (incoming) {
-        if (!ringbuf_alloc(&self->ringbuf, buffer_size * (sizeof(uint16_t) + characteristic->max_length), false)) {
+        if (!ringbuf_alloc(&self->ringbuf, buffer_size * (sizeof(uint16_t) + self->max_packet_size), false)) {
             mp_raise_ValueError(translate("Buffer too large and unable to allocate"));
         }
     }
@@ -216,8 +220,8 @@ void common_hal_bleio_packet_buffer_construct(
         self->packet_queued = false;
         self->pending_index = 0;
         self->pending_size = 0;
-        self->outgoing[0] = m_malloc(characteristic->max_length, false);
-        self->outgoing[1] = m_malloc(characteristic->max_length, false);
+        self->outgoing[0] = m_malloc(self->max_packet_size, false);
+        self->outgoing[1] = m_malloc(self->max_packet_size, false);
     } else {
         self->outgoing[0] = NULL;
         self->outgoing[1] = NULL;
@@ -296,10 +300,16 @@ mp_int_t common_hal_bleio_packet_buffer_write(bleio_packet_buffer_obj_t *self, u
     }
     uint16_t outgoing_packet_length = common_hal_bleio_packet_buffer_get_outgoing_packet_length(self);
 
-    if (len + header_len > outgoing_packet_length) {
+    uint16_t total_len = len + header_len;
+    if (total_len > outgoing_packet_length) {
+        // Supplied data will not fit in a single BLE packet.
+        mp_raise_ValueError_varg(translate("Total data to write is larger than %q"), MP_QSTR_outgoing_packet_length);
+    }
+    if (total_len > self->max_packet_size) {
         // Supplied data will not fit in a single BLE packet.
-        mp_raise_ValueError(translate("Total data to write is larger than outgoing_packet_length"));
+        mp_raise_ValueError_varg(translate("Total data to write is larger than %q"), MP_QSTR_max_packet_size);
     }
+    outgoing_packet_length = MIN(outgoing_packet_length, self->max_packet_size);
 
     if (len + self->pending_size > outgoing_packet_length) {
         // No room to append len bytes to packet. Wait until we get a free buffer,
@@ -390,8 +400,9 @@ mp_int_t common_hal_bleio_packet_buffer_get_outgoing_packet_length(bleio_packet_
         if (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);
+                return MIN(MIN(common_hal_bleio_connection_get_max_packet_length(connection),
+                               self->max_packet_size),
+                           self->characteristic->max_length);
             }
         }
         // There's no current connection, so we don't know the MTU, and
@@ -406,11 +417,12 @@ mp_int_t common_hal_bleio_packet_buffer_get_outgoing_packet_length(bleio_packet_
         if (self->conn_handle != BLE_CONN_HANDLE_INVALID) {
             bleio_connection_internal_t *connection = bleio_conn_handle_to_connection(self->conn_handle);
             if (connection) {
-                return common_hal_bleio_connection_get_max_packet_length(connection);
+                return MIN(common_hal_bleio_connection_get_max_packet_length(connection),
+                           self->max_packet_size);
             }
         }
     }
-    return self->characteristic->max_length;
+    return MIN(self->characteristic->max_length, self->max_packet_size);
 }
 
 bool common_hal_bleio_packet_buffer_deinited(bleio_packet_buffer_obj_t *self) {

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

@@ -44,6 +44,7 @@ typedef struct {
     // 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).
     volatile uint16_t conn_handle;
+    uint16_t max_packet_size;
     uint8_t pending_index;
     uint8_t write_type;
     bool client;

shared-bindings/_bleio/Characteristic.c

@@ -219,6 +219,23 @@ const mp_obj_property_t bleio_characteristic_value_obj = {
                (mp_obj_t)&mp_const_none_obj },
 };
 
+//|     max_length: int
+//|     """The max length of this characteristic."""
+//|
+STATIC mp_obj_t bleio_characteristic_get_max_length(mp_obj_t self_in) {
+    bleio_characteristic_obj_t *self = MP_OBJ_TO_PTR(self_in);
+
+    return MP_OBJ_NEW_SMALL_INT(common_hal_bleio_characteristic_get_max_length(self));
+}
+STATIC MP_DEFINE_CONST_FUN_OBJ_1(bleio_characteristic_get_max_length_obj, bleio_characteristic_get_max_length);
+
+const mp_obj_property_t bleio_characteristic_max_length_obj = {
+    .base.type = &mp_type_property,
+    .proxy = { (mp_obj_t)&bleio_characteristic_get_max_length_obj,
+               (mp_obj_t)&mp_const_none_obj,
+               (mp_obj_t)&mp_const_none_obj },
+};
+
 //|     descriptors: Descriptor
 //|     """A tuple of :py:class:`Descriptor` objects related to this characteristic. (read-only)"""
 //|

shared-bindings/_bleio/Characteristic.h

@@ -40,6 +40,7 @@ extern bleio_characteristic_properties_t common_hal_bleio_characteristic_get_pro
 extern mp_obj_tuple_t *common_hal_bleio_characteristic_get_descriptors(bleio_characteristic_obj_t *self);
 extern bleio_service_obj_t *common_hal_bleio_characteristic_get_service(bleio_characteristic_obj_t *self);
 extern bleio_uuid_obj_t *common_hal_bleio_characteristic_get_uuid(bleio_characteristic_obj_t *self);
+extern size_t common_hal_bleio_characteristic_get_max_length(bleio_characteristic_obj_t *self);
 extern size_t common_hal_bleio_characteristic_get_value(bleio_characteristic_obj_t *self, uint8_t *buf, size_t len);
 extern void common_hal_bleio_characteristic_add_descriptor(bleio_characteristic_obj_t *self, bleio_descriptor_obj_t *descriptor);
 extern void common_hal_bleio_characteristic_construct(bleio_characteristic_obj_t *self, bleio_service_obj_t *service, uint16_t handle, bleio_uuid_obj_t *uuid, bleio_characteristic_properties_t props, bleio_attribute_security_mode_t read_perm, bleio_attribute_security_mode_t write_perm, mp_int_t max_length, bool fixed_length, mp_buffer_info_t *initial_value_bufinfo);

shared-bindings/_bleio/PacketBuffer.c

@@ -44,7 +44,7 @@
 //|     When we're the server, we ignore all connections besides the first to subscribe to
 //|     notifications."""
 //|
-//|     def __init__(self, characteristic: Characteristic, *, buffer_size: int) -> None:
+//|     def __init__(self, characteristic: Characteristic, *, buffer_size: int, max_packet_size: Optional[int] = None) -> None:
 //|         """Monitor the given Characteristic. Each time a new value is written to the Characteristic
 //|         add the newly-written bytes to a FIFO buffer.
 //|
@@ -55,14 +55,17 @@
 //|           It may be a local Characteristic provided by a Peripheral Service, or a remote Characteristic
 //|           in a remote Service that a Central has connected to.
 //|         :param int buffer_size: Size of ring buffer (in packets of the Characteristic's maximum
-//|           length) that stores incoming packets coming from the peer."""
+//|           length) that stores incoming packets coming from the peer.
+//|         :param int max_packet_size: Maximum size of packets. Overrides value from the characteristic.
+//|           (Remote characteristics may not have the correct length.)"""
 //|         ...
 //|
 STATIC mp_obj_t bleio_packet_buffer_make_new(const mp_obj_type_t *type, size_t n_args, const mp_obj_t *pos_args, mp_map_t *kw_args) {
-    enum { ARG_characteristic, ARG_buffer_size };
+    enum { ARG_characteristic, ARG_buffer_size, ARG_max_packet_size };
     static const mp_arg_t allowed_args[] = {
         { MP_QSTR_characteristic,  MP_ARG_REQUIRED | MP_ARG_OBJ },
         { MP_QSTR_buffer_size, MP_ARG_KW_ONLY | MP_ARG_REQUIRED | MP_ARG_INT },
+        { MP_QSTR_max_packet_size, MP_ARG_KW_ONLY | MP_ARG_OBJ, {.u_obj = mp_const_none}},
     };
 
     mp_arg_val_t args[MP_ARRAY_SIZE(allowed_args)];
@@ -79,10 +82,15 @@ STATIC mp_obj_t bleio_packet_buffer_make_new(const mp_obj_type_t *type, size_t n
         mp_raise_TypeError(translate("Expected a Characteristic"));
     }
 
+    size_t max_packet_size = common_hal_bleio_characteristic_get_max_length(characteristic);
+    if (args[ARG_max_packet_size].u_obj != mp_const_none) {
+        max_packet_size = mp_obj_get_int(args[ARG_max_packet_size].u_obj);
+    }
+
     bleio_packet_buffer_obj_t *self = m_new_obj(bleio_packet_buffer_obj_t);
     self->base.type = &bleio_packet_buffer_type;
 
-    common_hal_bleio_packet_buffer_construct(self, MP_OBJ_TO_PTR(characteristic), buffer_size);
+    common_hal_bleio_packet_buffer_construct(self, MP_OBJ_TO_PTR(characteristic), buffer_size, max_packet_size);
 
     return MP_OBJ_FROM_PTR(self);
 }
@@ -133,7 +141,7 @@ STATIC mp_obj_t bleio_packet_buffer_write(mp_uint_t n_args, const mp_obj_t *pos_
     enum { ARG_data, ARG_header };
     static const mp_arg_t allowed_args[] = {
         { MP_QSTR_data,  MP_ARG_REQUIRED | MP_ARG_OBJ },
-        { MP_QSTR_header, MP_ARG_KW_ONLY | MP_ARG_OBJ, {.u_obj = MP_OBJ_NULL}},
+        { MP_QSTR_header, MP_ARG_KW_ONLY | MP_ARG_OBJ, {.u_obj = mp_const_none}},
     };
 
     mp_arg_val_t args[MP_ARRAY_SIZE(allowed_args)];
@@ -147,7 +155,7 @@ STATIC mp_obj_t bleio_packet_buffer_write(mp_uint_t n_args, const mp_obj_t *pos_
 
     mp_buffer_info_t header_bufinfo;
     header_bufinfo.len = 0;
-    if (args[ARG_header].u_obj != MP_OBJ_NULL) {
+    if (args[ARG_header].u_obj != mp_const_none) {
         mp_get_buffer_raise(args[ARG_header].u_obj, &header_bufinfo, MP_BUFFER_READ);
     }
 

shared-bindings/_bleio/PacketBuffer.h

@@ -33,7 +33,7 @@ extern const mp_obj_type_t bleio_packet_buffer_type;
 
 extern void common_hal_bleio_packet_buffer_construct(
     bleio_packet_buffer_obj_t *self, bleio_characteristic_obj_t *characteristic,
-    size_t buffer_size);
+    size_t buffer_size, size_t max_packet_size);
 mp_int_t common_hal_bleio_packet_buffer_write(bleio_packet_buffer_obj_t *self, uint8_t *data, size_t len, uint8_t *header, size_t header_len);
 mp_int_t 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_get_incoming_packet_length(bleio_packet_buffer_obj_t *self);