add PacketBuffer .incoming_ and .outgoing_packet_length

dhalbert · dhalbert · commit cb506f36a944 · 2020-05-04T15:42:10.000-04:00
diff --git a/ports/nrf/common-hal/_bleio/PacketBuffer.c b/ports/nrf/common-hal/_bleio/PacketBuffer.c
@@ -281,21 +281,33 @@ mp_int_t common_hal_bleio_packet_buffer_readinto(bleio_packet_buffer_obj_t *self
     return ret;
 }
 
-void 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_write(bleio_packet_buffer_obj_t *self, uint8_t *data, size_t len, uint8_t* header, size_t header_len) {
     if (self->outgoing[0] == NULL) {
         mp_raise_bleio_BluetoothError(translate("Writes not supported on Characteristic"));
     }
     if (self->conn_handle == BLE_CONN_HANDLE_INVALID) {
-        return;
+        return -1;
+    }
+    uint16_t outgoing_packet_length = common_hal_bleio_packet_buffer_get_outgoing_packet_length(self);
+
+    if (len + header_len > outgoing_packet_length) {
+        // Supplied data will not fit in a single BLE packet.
+        mp_raise_ValueError(translate("Total data to write is larger than outgoing_packet_length"));
     }
-    uint16_t packet_size = common_hal_bleio_packet_buffer_get_packet_size(self);
-    uint16_t max_size = packet_size - len;
-    while (max_size < self->pending_size && self->conn_handle != BLE_CONN_HANDLE_INVALID) {
-        RUN_BACKGROUND_TASKS;
+
+    if (len + self->pending_size > outgoing_packet_length) {
+        // No room to append len bytes to packet. Wait until we get a free buffer,
+        // and keep checking that we haven't been disconnected.
+        while (self->pending_size != 0 && self->conn_handle != BLE_CONN_HANDLE_INVALID) {
+            RUN_BACKGROUND_TASKS;
+        }
     }
     if (self->conn_handle == BLE_CONN_HANDLE_INVALID) {
-        return;
+        return -1;
     }
+
+    size_t num_bytes_written = 0;
+
     uint8_t is_nested_critical_region;
     sd_nvic_critical_region_enter(&is_nested_critical_region);
 
@@ -304,35 +316,44 @@ void common_hal_bleio_packet_buffer_write(bleio_packet_buffer_obj_t *self, uint8
     if (self->pending_size == 0) {
         memcpy(pending, header, header_len);
         self->pending_size += header_len;
+        num_bytes_written += header_len;
     }
     memcpy(pending + self->pending_size, data, len);
     self->pending_size += len;
+    num_bytes_written += len;
 
     sd_nvic_critical_region_exit(is_nested_critical_region);
 
     // If no writes are queued then sneak in this data.
     if (!self->packet_queued) {
         queue_next_write(self);
     }
+    return num_bytes_written;
 }
 
-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,
+mp_int_t common_hal_bleio_packet_buffer_get_incoming_packet_length(bleio_packet_buffer_obj_t *self) {
+    // If this PacketBuffer is coming from a remote service via 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 &&
+    if (self->characteristic == NULL) {
+        return -1;
+    }
+
+    if (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);
+         (CHAR_PROP_INDICATE | CHAR_PROP_NOTIFY))) {
+        // We are talking to a remote service, and data is arriving via NOTIFY or INDICATE.
+        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);
+            }
         }
         // 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.
@@ -341,6 +362,37 @@ mp_int_t common_hal_bleio_packet_buffer_get_packet_size(bleio_packet_buffer_obj_
     return self->characteristic->max_length;
 }
 
+mp_int_t common_hal_bleio_packet_buffer_get_outgoing_packet_length(bleio_packet_buffer_obj_t *self) {
+    // If we are sending data via 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) {
+        return -1;
+    }
+
+    if (self->characteristic->service != NULL &&
+        !self->characteristic->service->is_remote &&
+        (common_hal_bleio_characteristic_get_properties(self->characteristic) &
+         (CHAR_PROP_INDICATE | CHAR_PROP_NOTIFY))) {
+        // We are sending to a client, via NOTIFY or INDICATE.
+        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);
+            }
+        }
+        // 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;
+}
+
 bool common_hal_bleio_packet_buffer_deinited(bleio_packet_buffer_obj_t *self) {
     return self->characteristic == NULL;
 }
diff --git a/ports/nrf/common-hal/_bleio/PacketBuffer.h b/ports/nrf/common-hal/_bleio/PacketBuffer.h
@@ -40,10 +40,10 @@ typedef struct {
     // Two outgoing buffers to alternate between. One will be queued for transmission by the SD and
     // the other is waiting to be queued and can be extended.
     uint8_t* outgoing[2];
-    uint16_t pending_size;
+    volatile 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;
+    volatile uint16_t conn_handle;
     uint8_t pending_index;
     uint8_t write_type;
     bool client;
diff --git a/ports/nrf/common-hal/_bleio/__init__.c b/ports/nrf/common-hal/_bleio/__init__.c
@@ -119,7 +119,7 @@ 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 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.
+    // If we're not connected, that's OK, because we can still read and write the local value.
 
     ble_gatts_value_t gatts_value = {
         .p_value = buf,
@@ -133,7 +133,7 @@ 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 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.
+    // If we're not connected, that's OK, because we can still read and write the local value.
 
     ble_gatts_value_t gatts_value = {
         .p_value = bufinfo->buf,
diff --git a/shared-bindings/_bleio/PacketBuffer.c b/shared-bindings/_bleio/PacketBuffer.c
@@ -42,16 +42,16 @@
 //|
 //| 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
-//| buffer to store data.
+//| unit (MTU) minus overhead, whichever is smaller. The MTU can change so check `incoming_packet_length`
+//| and `outgoing_packet_length` before creating a buffer to store data.
 //|
 //| When we're the server, we ignore all connections besides the first to subscribe to
 //| notifications.
 //|
 //| .. class:: PacketBuffer(characteristic, *, buffer_size)
 //|
 //|   Monitor the given Characteristic. Each time a new value is written to the Characteristic
-//|   add the newly-written bytes to a FIFO buffer.
+//|   add the newly-written packet of bytes to a FIFO buffer.
 //|
 //|   :param Characteristic characteristic: The Characteristic to monitor.
 //|     It may be a local Characteristic provided by a Peripheral Service, or a remote Characteristic
@@ -125,6 +125,9 @@ STATIC MP_DEFINE_CONST_FUN_OBJ_2(bleio_packet_buffer_readinto_obj, bleio_packet_
 //|
 //|     This does not block until the data is sent. It only blocks until the data is pending.
 //|
+//|     :return: number of bytes written. May include header bytes when packet is empty.
+//|     :rtype: int
+//|
 // TODO: Add a kwarg `merge=False` to dictate whether subsequent writes are merged into a pending
 // one.
 STATIC mp_obj_t bleio_packet_buffer_write(mp_uint_t n_args, const mp_obj_t *pos_args, mp_map_t *kw_args) {
@@ -149,9 +152,21 @@ STATIC mp_obj_t bleio_packet_buffer_write(mp_uint_t n_args, const mp_obj_t *pos_
         mp_get_buffer_raise(args[ARG_header].u_obj, &header_bufinfo, MP_BUFFER_READ);
     }
 
-    common_hal_bleio_packet_buffer_write(self, data_bufinfo.buf, data_bufinfo.len,
-                                         header_bufinfo.buf, header_bufinfo.len);
-    return mp_const_none;
+    mp_int_t num_bytes_written = common_hal_bleio_packet_buffer_write(
+        self, data_bufinfo.buf, data_bufinfo.len, header_bufinfo.buf, header_bufinfo.len);
+    if (num_bytes_written < 0) {
+        // TODO: Raise an error if not connected. Right now the not-connected error
+        // is unreliable, because common_hal_bleio_packet_buffer_write()
+        // checks for conn_handle being set, but setting that
+        // can be delayed because conn_handle is discovered by spying on
+        // gatts write events, which may not have been sent yet.
+        //
+        // IDEAL:
+        // mp_raise_bleio_ConnectionError(translate("Not connected"));
+        // TEMPORARY:
+        num_bytes_written = 0;
+    }
+    return MP_OBJ_NEW_SMALL_INT(num_bytes_written);
 }
 STATIC MP_DEFINE_CONST_FUN_OBJ_KW(bleio_packet_buffer_write_obj, 1, bleio_packet_buffer_write);
 
@@ -168,37 +183,72 @@ STATIC MP_DEFINE_CONST_FUN_OBJ_1(bleio_packet_buffer_deinit_obj, bleio_packet_bu
 
 //|   .. attribute:: packet_size
 //|
-//|     Maximum size of a packet.
+//|     `packet_size` is the same as `incoming_packet_length`.
+//|     The name `packet_size` is deprecated and
+//|     will be removed in CircuitPython 6.0.0.
+//|
+//|   .. attribute:: incoming_packet_length
+//|
+//|     Maximum length in bytes of a packet we are reading.
 //|     If the packet is arriving from a remote service via notify or indicate,
-//|     the maximum size is `Connection.max_packet_length`.
+//|     the maximum length is `Connection.max_packet_length`.
+//|     Otherwise it is the ``max_length`` of the :py:class:`~_bleio.Characteristic`.
+//|
+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);
+
+    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_incoming_packet_length_obj, bleio_packet_buffer_get_incoming_packet_length);
+
+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_incoming_packet_length_obj,
+               (mp_obj_t)&mp_const_none_obj,
+               (mp_obj_t)&mp_const_none_obj },
+};
+
+//|   .. attribute:: outgoing_packet_length
+//|
+//|     Maximum length in bytes of a packet we are writing.
+//|     If the packet is being sent via notify or indicate,
+//|     the maximum length is `Connection.max_packet_length`.
 //|     Otherwise it is the ``max_length`` of the :py:class:`~_bleio.Characteristic`.
 //|
-STATIC mp_obj_t bleio_packet_buffer_get_packet_size(mp_obj_t self_in) {
+STATIC mp_obj_t bleio_packet_buffer_get_outgoing_packet_length(mp_obj_t self_in) {
     bleio_packet_buffer_obj_t *self = MP_OBJ_TO_PTR(self_in);
 
-    mp_int_t size = common_hal_bleio_packet_buffer_get_packet_size(self);
+    mp_int_t size = common_hal_bleio_packet_buffer_get_outgoing_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_outgoing_packet_length_obj, bleio_packet_buffer_get_outgoing_packet_length);
 
-const mp_obj_property_t bleio_packet_buffer_packet_size_obj = {
+const mp_obj_property_t bleio_packet_buffer_outgoing_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_outgoing_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) },
+    // .packet_size is now an alias for .incoming_packet_length
+    // TODO: It will be removed in 6.0.0.
+    { MP_OBJ_NEW_QSTR(MP_QSTR_packet_size),            MP_ROM_PTR(&bleio_packet_buffer_incoming_packet_length_obj) },
+    { MP_OBJ_NEW_QSTR(MP_QSTR_incoming_packet_length), MP_ROM_PTR(&bleio_packet_buffer_incoming_packet_length_obj) },
+    { MP_OBJ_NEW_QSTR(MP_QSTR_outgoing_packet_length), MP_ROM_PTR(&bleio_packet_buffer_outgoing_packet_length_obj) },
 };
 
 STATIC MP_DEFINE_CONST_DICT(bleio_packet_buffer_locals_dict, bleio_packet_buffer_locals_dict_table);
diff --git a/shared-bindings/_bleio/PacketBuffer.h b/shared-bindings/_bleio/PacketBuffer.h
@@ -34,9 +34,10 @@ 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);
-void 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_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_packet_size(bleio_packet_buffer_obj_t *self);
+mp_int_t common_hal_bleio_packet_buffer_get_incoming_packet_length(bleio_packet_buffer_obj_t *self);
+mp_int_t common_hal_bleio_packet_buffer_get_outgoing_packet_length(bleio_packet_buffer_obj_t *self);
 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);
 
