specifying attribute length; fix up value setting

7 files changed, 126 insertions, 70 deletions

diff --git a/ports/nrf/common-hal/bleio/Characteristic.c b/ports/nrf/common-hal/bleio/Characteristic.c
index e0ddf1e81..f8b6ce06c 100644
--- a/ports/nrf/common-hal/bleio/Characteristic.c
+++ b/ports/nrf/common-hal/bleio/Characteristic.c
@@ -43,7 +43,6 @@ STATIC uint16_t characteristic_get_cccd(uint16_t cccd_handle, uint16_t conn_hand
 
     const uint32_t err_code = sd_ble_gatts_value_get(conn_handle, cccd_handle, &value);
 
-
     if (err_code == BLE_ERROR_GATTS_SYS_ATTR_MISSING) {
         // CCCD is not set, so say that neither Notify nor Indicate is enabled.
         cccd = 0;
@@ -125,16 +124,24 @@ STATIC void characteristic_gattc_read(bleio_characteristic_obj_t *characteristic
     ble_drv_remove_event_handler(characteristic_on_gattc_read_rsp_evt, characteristic);
 }
 
-void common_hal_bleio_characteristic_construct(bleio_characteristic_obj_t *self, 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_obj_list_t *descriptor_list) {
+void common_hal_bleio_characteristic_construct(bleio_characteristic_obj_t *self, 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_obj_list_t *descriptor_list) {
     self->service = MP_OBJ_NULL;
     self->uuid = uuid;
-    self->value = mp_const_none;
+    self->value = mp_const_empty_bytes;
     self->handle = BLE_GATT_HANDLE_INVALID;
     self->props = props;
     self->read_perm = read_perm;
     self->write_perm = write_perm;
     self->descriptor_list = descriptor_list;
 
+    const mp_int_t max_length_max = fixed_length ? BLE_GATTS_FIX_ATTR_LEN_MAX : BLE_GATTS_VAR_ATTR_LEN_MAX;
+    if (max_length < 0 || max_length > max_length_max) {
+        mp_raise_ValueError_varg(translate("max_length must be 0-%d when fixed_length is %s"),
+                                 max_length_max, fixed_length ? "True" : "False");
+    }
+    self->max_length = max_length;
+    self->fixed_length = fixed_length;
+
     for (size_t descriptor_idx = 0; descriptor_idx < descriptor_list->len; ++descriptor_idx) {
         bleio_descriptor_obj_t *descriptor =
             MP_OBJ_TO_PTR(descriptor_list->items[descriptor_idx]);
@@ -151,48 +158,63 @@ bleio_service_obj_t *common_hal_bleio_characteristic_get_service(bleio_character
 }
 
 mp_obj_t common_hal_bleio_characteristic_get_value(bleio_characteristic_obj_t *self) {
-    uint16_t conn_handle = common_hal_bleio_device_get_conn_handle(self->service->device);
-    if (common_hal_bleio_service_get_is_remote(self->service)) {
-        // self->value is set by evt handler.
-        characteristic_gattc_read(self);
-    } else {
-        self->value = common_hal_bleio_gatts_read(self->handle, conn_handle);
+    // Do GATT operations only if this characteristic has been added to a registered service.
+    if (self->handle != BLE_GATT_HANDLE_INVALID) {
+        uint16_t conn_handle = common_hal_bleio_device_get_conn_handle(self->service->device);
+        if (common_hal_bleio_service_get_is_remote(self->service)) {
+            // self->value is set by evt handler.
+            characteristic_gattc_read(self);
+        } else {
+            self->value = common_hal_bleio_gatts_read(self->handle, conn_handle);
+        }
     }
 
     return self->value;
 }
 
 void common_hal_bleio_characteristic_set_value(bleio_characteristic_obj_t *self, mp_buffer_info_t *bufinfo) {
-    uint16_t conn_handle = common_hal_bleio_device_get_conn_handle(self->service->device);
-
-    if (common_hal_bleio_service_get_is_remote(self->service)) {
-        // Last argument is true if write-no-reponse desired.
-        common_hal_bleio_gattc_write(self->handle, conn_handle, bufinfo,
-                                     (self->props & CHAR_PROP_WRITE_NO_RESPONSE));
-    } else {
-        bool sent = false;
-        uint16_t cccd = 0;
-
-        const bool notify = self->props & CHAR_PROP_NOTIFY;
-        const bool indicate = self->props & CHAR_PROP_INDICATE;
-        if (notify | indicate) {
-            cccd = characteristic_get_cccd(self->cccd_handle, conn_handle);
-        }
+    // Do GATT operations only if this characteristic has been added to a registered service.
+    if (self->handle != BLE_GATT_HANDLE_INVALID) {
+        uint16_t conn_handle = common_hal_bleio_device_get_conn_handle(self->service->device);
+
+        if (common_hal_bleio_service_get_is_remote(self->service)) {
+            // Last argument is true if write-no-reponse desired.
+            common_hal_bleio_gattc_write(self->handle, conn_handle, bufinfo,
+                                         (self->props & CHAR_PROP_WRITE_NO_RESPONSE));
+        } else {
+            if (self->fixed_length && bufinfo->len != self->max_length) {
+                mp_raise_ValueError(translate("Value length  required fixed length"));
+            }
+            if (bufinfo->len > self->max_length) {
+                mp_raise_ValueError(translate("Value length > max_length"));
+            }
 
-        // It's possible that both notify and indicate are set.
-        if (notify && (cccd & BLE_GATT_HVX_NOTIFICATION)) {
-            characteristic_gatts_notify_indicate(self->handle, conn_handle, bufinfo, BLE_GATT_HVX_NOTIFICATION);
-            sent = true;
-        }
-        if (indicate && (cccd & BLE_GATT_HVX_INDICATION)) {
-            characteristic_gatts_notify_indicate(self->handle, conn_handle, bufinfo, BLE_GATT_HVX_INDICATION);
-            sent = true;
-        }
+            bool sent = false;
+            uint16_t cccd = 0;
+
+            const bool notify = self->props & CHAR_PROP_NOTIFY;
+            const bool indicate = self->props & CHAR_PROP_INDICATE;
+            if (notify | indicate) {
+                cccd = characteristic_get_cccd(self->cccd_handle, conn_handle);
+            }
+
+            // It's possible that both notify and indicate are set.
+            if (notify && (cccd & BLE_GATT_HVX_NOTIFICATION)) {
+                characteristic_gatts_notify_indicate(self->handle, conn_handle, bufinfo, BLE_GATT_HVX_NOTIFICATION);
+                sent = true;
+            }
+            if (indicate && (cccd & BLE_GATT_HVX_INDICATION)) {
+                characteristic_gatts_notify_indicate(self->handle, conn_handle, bufinfo, BLE_GATT_HVX_INDICATION);
+                sent = true;
+            }
 
-        if (!sent) {
-            common_hal_bleio_gatts_write(self->handle, conn_handle, bufinfo);
+            if (!sent) {
+                common_hal_bleio_gatts_write(self->handle, conn_handle, bufinfo);
+            }
         }
     }
+
+    self->value = mp_obj_new_bytes(bufinfo->buf, bufinfo->len);
 }
 
 bleio_uuid_obj_t *common_hal_bleio_characteristic_get_uuid(bleio_characteristic_obj_t *self) {
diff --git a/ports/nrf/common-hal/bleio/Characteristic.h b/ports/nrf/common-hal/bleio/Characteristic.h
index 46f46eee5..fb6a4c6ce 100644
--- a/ports/nrf/common-hal/bleio/Characteristic.h
+++ b/ports/nrf/common-hal/bleio/Characteristic.h
@@ -39,6 +39,8 @@ typedef struct {
     bleio_service_obj_t *service;
     bleio_uuid_obj_t *uuid;
     mp_obj_t value;
+    uint16_t max_length;
+    bool fixed_length;
     uint16_t handle;
     bleio_characteristic_properties_t props;
     bleio_attribute_security_mode_t read_perm;
diff --git a/ports/nrf/common-hal/bleio/Descriptor.c b/ports/nrf/common-hal/bleio/Descriptor.c
index d6473a64f..2a2019277 100644
--- a/ports/nrf/common-hal/bleio/Descriptor.c
+++ b/ports/nrf/common-hal/bleio/Descriptor.c
@@ -35,20 +35,28 @@
 
 static volatile bleio_descriptor_obj_t *m_read_descriptor;
 
-void common_hal_bleio_descriptor_construct(bleio_descriptor_obj_t *self, bleio_uuid_obj_t *uuid, bleio_attribute_security_mode_t read_perm, bleio_attribute_security_mode_t write_perm) {
+void common_hal_bleio_descriptor_construct(bleio_descriptor_obj_t *self, bleio_uuid_obj_t *uuid, bleio_attribute_security_mode_t read_perm, bleio_attribute_security_mode_t write_perm, mp_int_t max_length, bool fixed_length) {
     self->characteristic = MP_OBJ_NULL;
     self->uuid = uuid;
-    self->value = mp_const_none;
-    self->handle = BLE_CONN_HANDLE_INVALID;
+    self->value = mp_const_empty_bytes;
+    self->handle = BLE_GATT_HANDLE_INVALID;
     self->read_perm = read_perm;
     self->write_perm = write_perm;
+
+    const mp_int_t max_length_max = fixed_length ? BLE_GATTS_FIX_ATTR_LEN_MAX : BLE_GATTS_VAR_ATTR_LEN_MAX;
+    if (max_length < 0 || max_length > max_length_max) {
+        mp_raise_ValueError_varg(translate("max_length must be 0-%d when fixed_length is %s"),
+                                 max_length_max, fixed_length ? "True" : "False");
+    }
+    self->max_length = max_length;
+    self->fixed_length = fixed_length;
 }
 
 bleio_uuid_obj_t *common_hal_bleio_descriptor_get_uuid(bleio_descriptor_obj_t *self) {
     return self->uuid;
 }
 
-mp_obj_t common_hal_bleio_descriptor_get_characteristic(bleio_descriptor_obj_t *self) {
+bleio_characteristic_obj_t *common_hal_bleio_descriptor_get_characteristic(bleio_descriptor_obj_t *self) {
     return self->characteristic;
 }
 
@@ -97,22 +105,37 @@ STATIC void descriptor_gattc_read(bleio_descriptor_obj_t *descriptor) {
 }
 
 mp_obj_t common_hal_bleio_descriptor_get_value(bleio_descriptor_obj_t *self) {
-    if (common_hal_bleio_service_get_is_remote(self->characteristic->service)) {
-        descriptor_gattc_read(self);
-    } else {
-        self->value = common_hal_bleio_gatts_read(
-            self->handle, common_hal_bleio_device_get_conn_handle(self->characteristic->service->device));
+    // Do GATT operations only if this descriptor has been registered
+    if (self->handle != BLE_GATT_HANDLE_INVALID) {
+        if (common_hal_bleio_service_get_is_remote(self->characteristic->service)) {
+            descriptor_gattc_read(self);
+        } else {
+            self->value = common_hal_bleio_gatts_read(
+                self->handle, common_hal_bleio_device_get_conn_handle(self->characteristic->service->device));
+        }
     }
 
     return self->value;
 }
 
 void common_hal_bleio_descriptor_set_value(bleio_descriptor_obj_t *self, mp_buffer_info_t *bufinfo) {
-    uint16_t conn_handle = common_hal_bleio_device_get_conn_handle(self->characteristic->service->device);
-    if (common_hal_bleio_service_get_is_remote(self->characteristic->service)) {
-        // false means WRITE_REQ, not write-no-response
-        common_hal_bleio_gattc_write(self->handle, conn_handle, bufinfo, false);
-    } else {
-        common_hal_bleio_gatts_write(self->handle, conn_handle, bufinfo);
+    // Do GATT operations only if this descriptor has been registered.
+    if (self->handle != BLE_GATT_HANDLE_INVALID) {
+        uint16_t conn_handle = common_hal_bleio_device_get_conn_handle(self->characteristic->service->device);
+        if (common_hal_bleio_service_get_is_remote(self->characteristic->service)) {
+            // false means WRITE_REQ, not write-no-response
+            common_hal_bleio_gattc_write(self->handle, conn_handle, bufinfo, false);
+        } else {
+            if (self->fixed_length && bufinfo->len != self->max_length) {
+                mp_raise_ValueError(translate("Value length != required fixed length"));
+            }
+            if (bufinfo->len > self->max_length) {
+                mp_raise_ValueError(translate("Value length > max_length"));
+            }
+
+            common_hal_bleio_gatts_write(self->handle, conn_handle, bufinfo);
+        }
     }
+
+    self->value = mp_obj_new_bytes(bufinfo->buf, bufinfo->len);
 }
diff --git a/ports/nrf/common-hal/bleio/Descriptor.h b/ports/nrf/common-hal/bleio/Descriptor.h
index 2fae8cba6..6db4d7be3 100644
--- a/ports/nrf/common-hal/bleio/Descriptor.h
+++ b/ports/nrf/common-hal/bleio/Descriptor.h
@@ -40,6 +40,8 @@ typedef struct {
     bleio_characteristic_obj_t *characteristic;
     bleio_uuid_obj_t *uuid;
     mp_obj_t value;
+    uint16_t max_length;
+    bool fixed_length;
     uint16_t handle;
     bleio_attribute_security_mode_t read_perm;
     bleio_attribute_security_mode_t write_perm;
diff --git a/ports/nrf/common-hal/bleio/Peripheral.c b/ports/nrf/common-hal/bleio/Peripheral.c
index 905b26835..640b93180 100644
--- a/ports/nrf/common-hal/bleio/Peripheral.c
+++ b/ports/nrf/common-hal/bleio/Peripheral.c
@@ -149,21 +149,18 @@ STATIC void peripheral_on_ble_evt(ble_evt_t *ble_evt, void *self_in) {
 
         case BLE_GAP_EVT_CONN_SEC_UPDATE: {
             ble_gap_conn_sec_t* conn_sec = &ble_evt->evt.gap_evt.params.conn_sec_update.conn_sec;
-            mp_printf(&mp_plat_print, "sm: %d, lv: %d\n", conn_sec->sec_mode.sm, conn_sec->sec_mode.lv);
             if (conn_sec->sec_mode.sm <= 1 && conn_sec->sec_mode.lv <= 1) {
                 // Security setup did not succeed:
                 // mode 0, level 0 means no access
                 // mode 1, level 1 means open link
                 // mode >=1 and/or level >=1 means encryption is set up
                 self->pair_status = PAIR_NOT_PAIRED;
-                mp_printf(&mp_plat_print, "PAIR_NOT_PAIRED\n");
             } else {
                 // TODO: see Bluefruit lib
                 // if ( !bond_load_cccd(_role, _conn_hdl, _ediv) ) {
                 //    sd_ble_gatts_sys_attr_set(_conn_hdl, NULL, 0, 0);
                 // }
                 self->pair_status = PAIR_PAIRED;
-                mp_printf(&mp_plat_print, "PAIR_PAIRED\n");
             }
             break;
         }
diff --git a/ports/nrf/common-hal/bleio/Service.c b/ports/nrf/common-hal/bleio/Service.c
index 0ac0107f5..a7a3a6db0 100644
--- a/ports/nrf/common-hal/bleio/Service.c
+++ b/ports/nrf/common-hal/bleio/Service.c
@@ -72,6 +72,10 @@ void common_hal_bleio_service_add_all_characteristics(bleio_service_obj_t *self)
         bleio_characteristic_obj_t *characteristic =
             MP_OBJ_TO_PTR(self->characteristic_list->items[characteristic_idx]);
 
+        if (characteristic->handle != BLE_GATT_HANDLE_INVALID) {
+            mp_raise_ValueError(translate("Characteristic already in use by another Service."));
+        }
+
         ble_gatts_char_md_t char_md = {
             .char_props.broadcast      = (characteristic->props & CHAR_PROP_BROADCAST) ? 1 : 0,
             .char_props.read           = (characteristic->props & CHAR_PROP_READ) ? 1 : 0,
@@ -97,17 +101,22 @@ void common_hal_bleio_service_add_all_characteristics(bleio_service_obj_t *self)
 
         ble_gatts_attr_md_t char_attr_md = {
             .vloc = BLE_GATTS_VLOC_STACK,
-            .vlen = 1,
+            .vlen = !characteristic->fixed_length,
         };
 
         BLE_GAP_CONN_SEC_MODE_SET_OPEN(&char_attr_md.read_perm);
         BLE_GAP_CONN_SEC_MODE_SET_OPEN(&char_attr_md.write_perm);
 
+        mp_buffer_info_t char_value_bufinfo;
+        mp_get_buffer_raise(characteristic->value, &char_value_bufinfo, MP_BUFFER_READ);
+
         ble_gatts_attr_t char_attr = {
             .p_uuid = &char_uuid,
             .p_attr_md = &char_attr_md,
-            .init_len = sizeof(uint8_t),
-            .max_len = GATT_MAX_DATA_LENGTH,
+            .init_len = char_value_bufinfo.len,
+            .p_value = char_value_bufinfo.buf,
+            .init_offs = 0,
+            .max_len = characteristic->max_length,
         };
 
         ble_gatts_char_handles_t char_handles;
@@ -118,10 +127,6 @@ void common_hal_bleio_service_add_all_characteristics(bleio_service_obj_t *self)
             mp_raise_OSError_msg_varg(translate("Failed to add characteristic, err 0x%04x"), err_code);
         }
 
-        if (characteristic->handle != BLE_GATT_HANDLE_INVALID) {
-            mp_raise_ValueError(translate("Characteristic already in use by another Service."));
-        }
-
         characteristic->user_desc_handle = char_handles.user_desc_handle;
         characteristic->cccd_handle = char_handles.cccd_handle;
         characteristic->sccd_handle = char_handles.sccd_handle;
@@ -138,25 +143,27 @@ void common_hal_bleio_service_add_all_characteristics(bleio_service_obj_t *self)
             ble_gatts_attr_md_t desc_attr_md = {
                 // Data passed is not in a permanent location and should be copied.
                 .vloc = BLE_GATTS_VLOC_STACK,
-                .vlen = 1,
+                .vlen = !descriptor->fixed_length,
             };
 
             BLE_GAP_CONN_SEC_MODE_SET_OPEN(&desc_attr_md.read_perm);
             BLE_GAP_CONN_SEC_MODE_SET_OPEN(&desc_attr_md.write_perm);
 
-            mp_buffer_info_t bufinfo;
-            mp_get_buffer_raise(descriptor->value, &bufinfo, MP_BUFFER_READ);
+            mp_buffer_info_t desc_value_bufinfo;
+            mp_get_buffer_raise(descriptor->value, &desc_value_bufinfo, MP_BUFFER_READ);
 
             ble_gatts_attr_t desc_attr = {
                 .p_uuid = &desc_uuid,
                 .p_attr_md = &desc_attr_md,
-                .init_len = bufinfo.len,
-                .p_value = bufinfo.buf,
+                .init_len = desc_value_bufinfo.len,
+                .p_value = desc_value_bufinfo.buf,
                 .init_offs = 0,
-                .max_len = GATT_MAX_DATA_LENGTH,
+                .max_len = descriptor->max_length,
             };
 
             err_code = sd_ble_gatts_descriptor_add(characteristic->handle, &desc_attr, &descriptor->handle);
-        }
-    }
+
+        } // loop over descriptors
+
+    } // loop over characteristics
 }
diff --git a/ports/nrf/common-hal/bleio/__init__.c b/ports/nrf/common-hal/bleio/__init__.c
index d09b6f5af..2d144f8b5 100644
--- a/ports/nrf/common-hal/bleio/__init__.c
+++ b/ports/nrf/common-hal/bleio/__init__.c
@@ -218,7 +218,9 @@ STATIC void on_char_discovery_rsp(ble_gattc_evt_char_disc_rsp_t *response, mp_ob
 
         // Call common_hal_bleio_characteristic_construct() to initalize some fields and set up evt handler.
         common_hal_bleio_characteristic_construct(
-            characteristic, uuid, props, SEC_MODE_OPEN, SEC_MODE_OPEN, mp_obj_new_list(0, NULL));
+            characteristic, uuid, props, SEC_MODE_OPEN, SEC_MODE_OPEN,
+            GATT_MAX_DATA_LENGTH, false,   // max_length, fixed_length: values may not matter for gattc
+            mp_obj_new_list(0, NULL));
         characteristic->handle = gattc_char->handle_value;
         characteristic->service = m_char_discovery_service;
 
@@ -272,7 +274,8 @@ STATIC void on_desc_discovery_rsp(ble_gattc_evt_desc_disc_rsp_t *response, mp_ob
             // For now, just leave the UUID as NULL.
         }
 
-        common_hal_bleio_descriptor_construct(descriptor, uuid, SEC_MODE_OPEN, SEC_MODE_OPEN);
+        common_hal_bleio_descriptor_construct(descriptor, uuid, SEC_MODE_OPEN, SEC_MODE_OPEN,
+                                              GATT_MAX_DATA_LENGTH, false);
         descriptor->handle = gattc_desc->handle;
         descriptor->characteristic = m_desc_discovery_characteristic;