Implement more ESP BLE Descriptor support

BLE HID was broken because the descriptor wasn't marked as
readable. This fixes #9430.

Also add read/write support for client. Servers can only set the
initial value. Setting it later will raise a NotImplementedError.

Setting a user_description will also raise a NotImplementedError.
This can help us find cases that use it.

Author: tannewt

ports/espressif/common-hal/_bleio/Adapter.c

@@ -85,7 +85,7 @@ void common_hal_bleio_adapter_set_enabled(bleio_adapter_obj_t *self, bool enable
 
         // ble_hs_cfg.reset_cb = blecent_on_reset;
         ble_hs_cfg.sync_cb = _on_sync;
-        // ble_hs_cfg.store_status_cb = ble_store_util_status_rr;
+        ble_hs_cfg.store_status_cb = ble_store_util_status_rr;
 
         ble_hs_cfg.sm_io_cap = BLE_SM_IO_CAP_NO_IO;
         ble_hs_cfg.sm_bonding = 1;

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

@@ -22,32 +22,6 @@
 
 static int characteristic_on_ble_gap_evt(struct ble_gap_event *event, void *param);
 
-static volatile int _completion_status;
-static uint64_t _timeout_start_time;
-
-static void _reset_completion_status(void) {
-    _completion_status = 0;
-}
-
-// Wait for a status change, recorded in a callback.
-// Try twice because sometimes we get a BLE_HS_EAGAIN.
-// Maybe we should try more than twice.
-static int _wait_for_completion(uint32_t timeout_msecs) {
-    for (int tries = 1; tries <= 2; tries++) {
-        _timeout_start_time = common_hal_time_monotonic_ms();
-        while ((_completion_status == 0) &&
-               (common_hal_time_monotonic_ms() < _timeout_start_time + timeout_msecs) &&
-               !mp_hal_is_interrupted()) {
-            RUN_BACKGROUND_TASKS;
-        }
-        if (_completion_status != BLE_HS_EAGAIN) {
-            // Quit, because either the status is either zero (OK) or it's an error.
-            break;
-        }
-    }
-    return _completion_status;
-}
-
 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,
@@ -151,35 +125,7 @@ bleio_service_obj_t *common_hal_bleio_characteristic_get_service(bleio_character
     return self->service;
 }
 
-typedef struct {
-    uint8_t *buf;
-    uint16_t len;
-} _read_info_t;
-
-static int _read_cb(uint16_t conn_handle,
-    const struct ble_gatt_error *error,
-    struct ble_gatt_attr *attr,
-    void *arg) {
-    _read_info_t *read_info = (_read_info_t *)arg;
-    switch (error->status) {
-        case 0: {
-            int len = MIN(read_info->len, OS_MBUF_PKTLEN(attr->om));
-            os_mbuf_copydata(attr->om, attr->offset, len, read_info->buf);
-            read_info->len = len;
-        }
-            MP_FALLTHROUGH;
-
-        default:
-            #if CIRCUITPY_VERBOSE_BLE
-            // For debugging.
-            mp_printf(&mp_plat_print, "Read status: %d\n", error->status);
-            #endif
-            break;
-    }
-    _completion_status = error->status;
 
-    return 0;
-}
 
 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.
@@ -188,14 +134,7 @@ size_t common_hal_bleio_characteristic_get_value(bleio_characteristic_obj_t *sel
     }
     uint16_t conn_handle = bleio_connection_get_conn_handle(self->service->connection);
     if (common_hal_bleio_service_get_is_remote(self->service)) {
-        _read_info_t read_info = {
-            .buf = buf,
-            .len = len
-        };
-        _reset_completion_status();
-        CHECK_NIMBLE_ERROR(ble_gattc_read(conn_handle, self->handle, _read_cb, &read_info));
-        CHECK_NIMBLE_ERROR(_wait_for_completion(2000));
-        return read_info.len;
+        return bleio_gattc_read(conn_handle, self->handle, buf, len);
     } else {
         len = MIN(self->current_value_len, len);
         memcpy(buf, self->current_value, len);
@@ -209,24 +148,13 @@ size_t common_hal_bleio_characteristic_get_max_length(bleio_characteristic_obj_t
     return self->max_length;
 }
 
-static int _write_cb(uint16_t conn_handle,
-    const struct ble_gatt_error *error,
-    struct ble_gatt_attr *attr,
-    void *arg) {
-    _completion_status = error->status;
-
-    return 0;
-}
-
 void common_hal_bleio_characteristic_set_value(bleio_characteristic_obj_t *self, mp_buffer_info_t *bufinfo) {
     if (common_hal_bleio_service_get_is_remote(self->service)) {
         uint16_t conn_handle = bleio_connection_get_conn_handle(self->service->connection);
         if ((self->props & CHAR_PROP_WRITE_NO_RESPONSE) != 0) {
             CHECK_NIMBLE_ERROR(ble_gattc_write_no_rsp_flat(conn_handle, self->handle, bufinfo->buf, bufinfo->len));
         } else {
-            _reset_completion_status();
-            CHECK_NIMBLE_ERROR(ble_gattc_write_flat(conn_handle, self->handle, bufinfo->buf, bufinfo->len, _write_cb, NULL));
-            CHECK_NIMBLE_ERROR(_wait_for_completion(2000));
+            bleio_gattc_write(conn_handle, self->handle, bufinfo->buf, bufinfo->len);
         }
     } else {
         // Validate data length for local characteristics only.
@@ -394,9 +322,7 @@ void common_hal_bleio_characteristic_set_cccd(bleio_characteristic_obj_t *self,
         (notify ? 1 << 0 : 0) |
         (indicate ? 1 << 1: 0);
 
-    _reset_completion_status();
-    CHECK_NIMBLE_ERROR(ble_gattc_write_flat(conn_handle, self->cccd_handle, &cccd_value, 2, _write_cb, NULL));
-    CHECK_NIMBLE_ERROR(_wait_for_completion(2000));
+    bleio_gattc_write(conn_handle, self->cccd_handle, (uint8_t *)&cccd_value, 2);
 }
 
 void bleio_characteristic_set_observer(bleio_characteristic_obj_t *self, mp_obj_t observer) {

ports/espressif/common-hal/_bleio/Descriptor.c

@@ -10,6 +10,7 @@
 #include "py/runtime.h"
 
 #include "shared-bindings/_bleio/__init__.h"
+#include "shared-bindings/_bleio/Attribute.h"
 #include "shared-bindings/_bleio/Descriptor.h"
 #include "shared-bindings/_bleio/Service.h"
 #include "shared-bindings/_bleio/UUID.h"
@@ -24,6 +25,31 @@ void common_hal_bleio_descriptor_construct(bleio_descriptor_obj_t *self, bleio_c
     self->write_perm = write_perm;
     self->initial_value = mp_obj_new_bytes(initial_value_bufinfo->buf, initial_value_bufinfo->len);
 
+    // Map CP's property values to Nimble's flag values.
+    self->flags = 0;
+    if (read_perm != SECURITY_MODE_NO_ACCESS) {
+        self->flags |= BLE_ATT_F_READ;
+    }
+    if (write_perm != SECURITY_MODE_NO_ACCESS) {
+        self->flags |= BLE_ATT_F_WRITE;
+    }
+    if (read_perm == SECURITY_MODE_ENC_WITH_MITM || write_perm == SECURITY_MODE_ENC_WITH_MITM ||
+        read_perm == SECURITY_MODE_SIGNED_WITH_MITM || write_perm == SECURITY_MODE_SIGNED_WITH_MITM) {
+        mp_raise_NotImplementedError(MP_ERROR_TEXT("MITM security not supported"));
+    }
+    if (read_perm == SECURITY_MODE_ENC_NO_MITM) {
+        self->flags |= BLE_ATT_F_READ_ENC;
+    }
+    if (read_perm == SECURITY_MODE_SIGNED_NO_MITM) {
+        self->flags |= BLE_ATT_F_READ_AUTHEN;
+    }
+    if (write_perm == SECURITY_MODE_ENC_NO_MITM) {
+        self->flags |= BLE_ATT_F_WRITE_ENC;
+    }
+    if (write_perm == SECURITY_MODE_SIGNED_NO_MITM) {
+        self->flags |= BLE_ATT_F_WRITE_AUTHEN;
+    }
+
     const mp_int_t max_length_max = BLE_ATT_ATTR_MAX_LEN;
     if (max_length < 0 || max_length > max_length_max) {
         mp_raise_ValueError_varg(MP_ERROR_TEXT("max_length must be 0-%d when fixed_length is %s"),
@@ -42,38 +68,35 @@ bleio_characteristic_obj_t *common_hal_bleio_descriptor_get_characteristic(bleio
 }
 
 size_t common_hal_bleio_descriptor_get_value(bleio_descriptor_obj_t *self, uint8_t *buf, size_t len) {
-    // Do GATT operations only if this descriptor has been registered
-    if (self->handle != BLEIO_HANDLE_INVALID) {
+    if (self->characteristic == NULL) {
+        return 0;
+    }
+    if (common_hal_bleio_service_get_is_remote(self->characteristic->service)) {
         uint16_t conn_handle = bleio_connection_get_conn_handle(self->characteristic->service->connection);
-        (void)conn_handle;
+        return bleio_gattc_read(conn_handle, self->handle, buf, len);
+    } else {
+        mp_buffer_info_t bufinfo;
+        mp_get_buffer(self->initial_value, &bufinfo, MP_BUFFER_READ);
+        len = MIN(bufinfo.len, len);
+        memcpy(buf, bufinfo.buf, len);
+        return len;
     }
 
-    // TODO: Implement this.
-
     return 0;
 }
 
 void common_hal_bleio_descriptor_set_value(bleio_descriptor_obj_t *self, mp_buffer_info_t *bufinfo) {
-    // TODO: Implement this.
-    // Do GATT operations only if this descriptor has been registered.
-    if (self->handle != BLEIO_HANDLE_INVALID) {
-        // uint16_t conn_handle = bleio_connection_get_conn_handle(self->characteristic->service->connection);
-        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 {
-            // Validate data length for local descriptors only.
-            if (self->fixed_length && bufinfo->len != self->max_length) {
-                mp_raise_ValueError(MP_ERROR_TEXT("Value length != required fixed length"));
-            }
-            if (bufinfo->len > self->max_length) {
-                mp_raise_ValueError(MP_ERROR_TEXT("Value length > max_length"));
-            }
-
-            // common_hal_bleio_gatts_write(self->handle, conn_handle, bufinfo);
-        }
+    if (self->characteristic == NULL) {
+        return;
     }
 
+    if (common_hal_bleio_service_get_is_remote(self->characteristic->service)) {
+        uint16_t conn_handle = bleio_connection_get_conn_handle(self->characteristic->service->connection);
+        bleio_gattc_write(conn_handle, self->handle, bufinfo->buf, bufinfo->len);
+    } else {
+        // Descriptor values should be set via the initial_value when used locally.
+        mp_raise_NotImplementedError(NULL);
+    }
 }
 
 int bleio_descriptor_access_cb(uint16_t conn_handle, uint16_t attr_handle,

ports/espressif/common-hal/_bleio/Service.c

@@ -86,6 +86,10 @@ void common_hal_bleio_service_add_characteristic(bleio_service_obj_t *self,
     const char *user_description) {
     mp_obj_list_append(self->characteristic_list, MP_OBJ_FROM_PTR(characteristic));
 
+    if (user_description != NULL) {
+        mp_raise_NotImplementedError_varg(MP_ERROR_TEXT("Invalid %q"), MP_QSTR_user_description);
+    }
+
     // Delete the old version of the service.
     if (self->characteristic_list->len > 1) {
         ble_gatts_delete_svc(&self->uuid->nimble_ble_uuid.u);

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

@@ -9,20 +9,25 @@
 #include <string.h>
 
 #include "py/runtime.h"
+#include "shared/runtime/interrupt_char.h"
+
 #include "shared-bindings/_bleio/__init__.h"
 #include "shared-bindings/_bleio/Adapter.h"
 #include "shared-bindings/_bleio/Characteristic.h"
 #include "shared-bindings/_bleio/Connection.h"
 #include "shared-bindings/_bleio/Descriptor.h"
 #include "shared-bindings/_bleio/Service.h"
 #include "shared-bindings/_bleio/UUID.h"
+#include "shared-bindings/time/__init__.h"
 #include "supervisor/shared/bluetooth/bluetooth.h"
 
 #include "common-hal/_bleio/__init__.h"
 #include "common-hal/_bleio/ble_events.h"
 
 #include "nvs_flash.h"
 
+static volatile int _completion_status;
+static uint64_t _timeout_start_time;
 
 background_callback_t bleio_background_callback;
 
@@ -152,3 +157,83 @@ void common_hal_bleio_check_connected(uint16_t conn_handle) {
         mp_raise_ConnectionError(MP_ERROR_TEXT("Not connected"));
     }
 }
+
+static void _reset_completion_status(void) {
+    _completion_status = 0;
+}
+
+// Wait for a status change, recorded in a callback.
+// Try twice because sometimes we get a BLE_HS_EAGAIN.
+// Maybe we should try more than twice.
+static int _wait_for_completion(uint32_t timeout_msecs) {
+    for (int tries = 1; tries <= 2; tries++) {
+        _timeout_start_time = common_hal_time_monotonic_ms();
+        while ((_completion_status == 0) &&
+               (common_hal_time_monotonic_ms() < _timeout_start_time + timeout_msecs) &&
+               !mp_hal_is_interrupted()) {
+            RUN_BACKGROUND_TASKS;
+        }
+        if (_completion_status != BLE_HS_EAGAIN) {
+            // Quit, because either the status is either zero (OK) or it's an error.
+            break;
+        }
+    }
+    return _completion_status;
+}
+
+typedef struct {
+    uint8_t *buf;
+    uint16_t len;
+} _read_info_t;
+
+static int _read_cb(uint16_t conn_handle,
+    const struct ble_gatt_error *error,
+    struct ble_gatt_attr *attr,
+    void *arg) {
+    _read_info_t *read_info = (_read_info_t *)arg;
+    switch (error->status) {
+        case 0: {
+            int len = MIN(read_info->len, OS_MBUF_PKTLEN(attr->om));
+            os_mbuf_copydata(attr->om, attr->offset, len, read_info->buf);
+            read_info->len = len;
+        }
+            MP_FALLTHROUGH;
+
+        default:
+            #if CIRCUITPY_VERBOSE_BLE
+            // For debugging.
+            mp_printf(&mp_plat_print, "Read status: %d\n", error->status);
+            #endif
+            break;
+    }
+    _completion_status = error->status;
+
+    return 0;
+}
+
+int bleio_gattc_read(uint16_t conn_handle, uint16_t value_handle, uint8_t *buf, size_t len) {
+    _read_info_t read_info = {
+        .buf = buf,
+        .len = len
+    };
+    _reset_completion_status();
+    CHECK_NIMBLE_ERROR(ble_gattc_read(conn_handle, value_handle, _read_cb, &read_info));
+    CHECK_NIMBLE_ERROR(_wait_for_completion(2000));
+    return read_info.len;
+}
+
+
+static int _write_cb(uint16_t conn_handle,
+    const struct ble_gatt_error *error,
+    struct ble_gatt_attr *attr,
+    void *arg) {
+    _completion_status = error->status;
+
+    return 0;
+}
+
+void bleio_gattc_write(uint16_t conn_handle, uint16_t value_handle, uint8_t *buf, size_t len) {
+    _reset_completion_status();
+    CHECK_NIMBLE_ERROR(ble_gattc_write_flat(conn_handle, value_handle, buf, len, _write_cb, NULL));
+    CHECK_NIMBLE_ERROR(_wait_for_completion(2000));
+}

ports/espressif/common-hal/_bleio/__init__.h

@@ -43,3 +43,6 @@ void check_notify(BaseType_t result);
 #define UNIT_1_MS  (1000)
 #define UNIT_1_25_MS  (1250)
 #define UNIT_10_MS    (10000)
+
+int bleio_gattc_read(uint16_t conn_handle, uint16_t value_handle, uint8_t *buf, size_t len);
+void bleio_gattc_write(uint16_t conn_handle, uint16_t value_handle, uint8_t *buf, size_t len);