WIP: more Descriptor work; refactor gattc/gatts read/write

15 files changed, 362 insertions, 172 deletions

diff --git a/ports/nrf/common-hal/bleio/Characteristic.c b/ports/nrf/common-hal/bleio/Characteristic.c
index 41fcdd1f1..e0ddf1e81 100644
--- a/ports/nrf/common-hal/bleio/Characteristic.c
+++ b/ports/nrf/common-hal/bleio/Characteristic.c
@@ -25,30 +25,23 @@
  * THE SOFTWARE.
  */
 
-#include <string.h>
-#include <stdio.h>
-
-#include "ble_drv.h"
-#include "ble_gatts.h"
-#include "nrf_soc.h"
-
 #include "py/runtime.h"
 
 #include "shared-bindings/bleio/__init__.h"
 #include "shared-bindings/bleio/Characteristic.h"
+#include "shared-bindings/bleio/Descriptor.h"
 #include "shared-bindings/bleio/Service.h"
 
-STATIC volatile bleio_characteristic_obj_t *m_read_characteristic;
+static volatile bleio_characteristic_obj_t *m_read_characteristic;
 
-STATIC uint16_t get_cccd(bleio_characteristic_obj_t *characteristic) {
-    const uint16_t conn_handle = common_hal_bleio_device_get_conn_handle(characteristic->service->device);
+STATIC uint16_t characteristic_get_cccd(uint16_t cccd_handle, uint16_t conn_handle) {
     uint16_t cccd;
     ble_gatts_value_t value = {
         .p_value = (uint8_t*) &cccd,
         .len = 2,
     };
 
-    const uint32_t err_code = sd_ble_gatts_value_get(conn_handle, characteristic->cccd_handle, &value);
+    const uint32_t err_code = sd_ble_gatts_value_get(conn_handle, cccd_handle, &value);
 
 
     if (err_code == BLE_ERROR_GATTS_SYS_ATTR_MISSING) {
@@ -61,64 +54,39 @@ STATIC uint16_t get_cccd(bleio_characteristic_obj_t *characteristic) {
     return cccd;
 }
 
-STATIC void gatts_read(bleio_characteristic_obj_t *characteristic) {
-    // This 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.
-    const uint16_t conn_handle = common_hal_bleio_device_get_conn_handle(characteristic->service->device);
-
-    mp_buffer_info_t bufinfo;
-    ble_gatts_value_t gatts_value = {
-        .p_value = NULL,
-        .len = 0,
-    };
-
-    // Read once to find out what size buffer we need, then read again to fill buffer.
-
-    uint32_t err_code = sd_ble_gatts_value_get(conn_handle, characteristic->handle, &gatts_value);
-    if (err_code == NRF_SUCCESS) {
-        characteristic->value_data = mp_obj_new_bytearray_of_zeros(gatts_value.len);
-        mp_get_buffer_raise(characteristic->value_data, &bufinfo, MP_BUFFER_WRITE);
-        gatts_value.p_value = bufinfo.buf;
-
-        // Read again, with the correct size of buffer.
-        err_code = sd_ble_gatts_value_get(conn_handle, characteristic->handle, &gatts_value);
-    }
-
-    if (err_code != NRF_SUCCESS) {
-        mp_raise_OSError_msg_varg(translate("Failed to read gatts value, err 0x%04x"), err_code);
-    }
-}
-
+STATIC void characteristic_on_gattc_read_rsp_evt(ble_evt_t *ble_evt, void *param) {
+    switch (ble_evt->header.evt_id) {
 
-STATIC void gatts_write(bleio_characteristic_obj_t *characteristic, mp_buffer_info_t *bufinfo) {
-    // This might be BLE_CONN_HANDLE_INVALID if we're not conected, but that's OK, because
-    // we can still read and write the local value.
-    const uint16_t conn_handle = common_hal_bleio_device_get_conn_handle(characteristic->service->device);
+        // More events may be handled later, so keep this as a switch.
 
-    ble_gatts_value_t gatts_value = {
-        .p_value = bufinfo->buf,
-        .len = bufinfo->len,
-    };
+        case BLE_GATTC_EVT_READ_RSP: {
+            ble_gattc_evt_read_rsp_t *response = &ble_evt->evt.gattc_evt.params.read_rsp;
+            if (m_read_characteristic) {
+                m_read_characteristic->value = mp_obj_new_bytearray(response->len, response->data);
+            }
+            // Indicate to busy-wait loop that we've read the attribute value.
+            m_read_characteristic = NULL;
+            break;
+        }
 
-    const uint32_t err_code = sd_ble_gatts_value_set(conn_handle, characteristic->handle, &gatts_value);
-    if (err_code != NRF_SUCCESS) {
-        mp_raise_OSError_msg_varg(translate("Failed to write gatts value, err 0x%04x"), err_code);
+        default:
+            // For debugging.
+            // mp_printf(&mp_plat_print, "Unhandled characteristic event: 0x%04x\n", ble_evt->header.evt_id);
+            break;
     }
 }
 
-STATIC void gatts_notify_indicate(bleio_characteristic_obj_t *characteristic, mp_buffer_info_t *bufinfo, uint16_t hvx_type) {
+STATIC void characteristic_gatts_notify_indicate(uint16_t handle, uint16_t conn_handle, mp_buffer_info_t *bufinfo, uint16_t hvx_type) {
     uint16_t hvx_len = bufinfo->len;
 
     ble_gatts_hvx_params_t hvx_params = {
-        .handle = characteristic->handle,
+        .handle = handle,
         .type = hvx_type,
         .offset = 0,
         .p_len = &hvx_len,
         .p_data = bufinfo->buf,
     };
 
-    const uint16_t conn_handle = common_hal_bleio_device_get_conn_handle(characteristic->service->device);
-
     while (1) {
         const uint32_t err_code = sd_ble_gatts_hvx(conn_handle, &hvx_params);
         if (err_code == NRF_SUCCESS) {
@@ -134,21 +102,17 @@ STATIC void gatts_notify_indicate(bleio_characteristic_obj_t *characteristic, mp
         // Some real error has occurred.
         mp_raise_OSError_msg_varg(translate("Failed to notify or indicate attribute value, err 0x%04x"), err_code);
     }
-
-}
-
-STATIC void check_connected(uint16_t conn_handle) {
-    if (conn_handle == BLE_CONN_HANDLE_INVALID) {
-        mp_raise_OSError_msg(translate("Not connected"));
-    }
 }
 
-STATIC void gattc_read(bleio_characteristic_obj_t *characteristic) {
+STATIC void characteristic_gattc_read(bleio_characteristic_obj_t *characteristic) {
     const uint16_t conn_handle = common_hal_bleio_device_get_conn_handle(characteristic->service->device);
-    check_connected(conn_handle);
+    common_hal_bleio_check_connected(conn_handle);
 
+    // Set to NULL in event loop after event.
     m_read_characteristic = characteristic;
 
+    ble_drv_add_event_handler(characteristic_on_gattc_read_rsp_evt, characteristic);
+
     const uint32_t err_code = sd_ble_gattc_read(conn_handle, characteristic->handle, 0);
     if (err_code != NRF_SUCCESS) {
         mp_raise_OSError_msg_varg(translate("Failed to read attribute value, err 0x%04x"), err_code);
@@ -157,91 +121,54 @@ STATIC void gattc_read(bleio_characteristic_obj_t *characteristic) {
     while (m_read_characteristic != NULL) {
         MICROPY_VM_HOOK_LOOP;
     }
-}
-
-STATIC void gattc_write(bleio_characteristic_obj_t *characteristic, mp_buffer_info_t *bufinfo) {
-    const uint16_t conn_handle = common_hal_bleio_device_get_conn_handle(characteristic->service->device);
-    check_connected(conn_handle);
-
-    ble_gattc_write_params_t write_params = {
-        .write_op = (characteristic->props & CHAR_PROP_WRITE_NO_RESPONSE)
-                    ? BLE_GATT_OP_WRITE_CMD: BLE_GATT_OP_WRITE_REQ,
-        .handle = characteristic->handle,
-        .p_value = bufinfo->buf,
-        .len = bufinfo->len,
-    };
-
-    while (1) {
-        uint32_t err_code = sd_ble_gattc_write(conn_handle, &write_params);
-        if (err_code == NRF_SUCCESS) {
-            break;
-        }
-
-        // Write with response will return NRF_ERROR_BUSY if the response has not been received.
-        // Write without reponse will return NRF_ERROR_RESOURCES if too many writes are pending.
-        if (err_code == NRF_ERROR_BUSY || err_code == NRF_ERROR_RESOURCES) {
-            // We could wait for an event indicating the write is complete, but just retrying is easier.
-            MICROPY_VM_HOOK_LOOP;
-            continue;
-        }
-
-        // Some real error occurred.
-        mp_raise_OSError_msg_varg(translate("Failed to write attribute value, err 0x%04x"), err_code);
-    }
 
+    ble_drv_remove_event_handler(characteristic_on_gattc_read_rsp_evt, characteristic);
 }
 
-STATIC void characteristic_on_ble_evt(ble_evt_t *ble_evt, void *param) {
-    switch (ble_evt->header.evt_id) {
-
-        // More events may be handled later, so keep this as a switch.
-
-        case BLE_GATTC_EVT_READ_RSP: {
-            ble_gattc_evt_read_rsp_t *response = &ble_evt->evt.gattc_evt.params.read_rsp;
-            m_read_characteristic->value_data = mp_obj_new_bytearray(response->len, response->data);
-            // Indicate to busy-wait loop that we've read the characteristic.
-            m_read_characteristic = NULL;
-            break;
-        }
-
-        default:
-            // For debugging.
-            // mp_printf(&mp_plat_print, "Unhandled characteristic event: 0x%04x\n", ble_evt->header.evt_id);
-            break;
-    }
-
-}
-
-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 security_mode, mp_obj_t descriptors) {
-    self->service = mp_const_none;
+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) {
+    self->service = MP_OBJ_NULL;
     self->uuid = uuid;
-    self->value_data = mp_const_none;
-    self->props = props;
-    self->security_mode = security_mode;
-    self->descriptor_list = mp_obj_new_list_from_iter(descriptors);
-
+    self->value = mp_const_none;
     self->handle = BLE_GATT_HANDLE_INVALID;
-
-    ble_drv_add_event_handler(characteristic_on_ble_evt, self);
+    self->props = props;
+    self->read_perm = read_perm;
+    self->write_perm = write_perm;
+    self->descriptor_list = descriptor_list;
+
+    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]);
+        descriptor->characteristic = self;
+    }
 }
 
 mp_obj_list_t *common_hal_bleio_characteristic_get_descriptor_list(bleio_characteristic_obj_t *self) {
     return self->descriptor_list;
 }
 
+bleio_service_obj_t *common_hal_bleio_characteristic_get_service(bleio_characteristic_obj_t *self) {
+    return self->service;
+}
+
 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)) {
-        gattc_read(self);
+        // self->value is set by evt handler.
+        characteristic_gattc_read(self);
     } else {
-        gatts_read(self);
+        self->value = common_hal_bleio_gatts_read(self->handle, conn_handle);
     }
 
-    return self->value_data;
+    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)) {
-        gattc_write(self, bufinfo);
+        // 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;
@@ -249,26 +176,25 @@ void common_hal_bleio_characteristic_set_value(bleio_characteristic_obj_t *self,
         const bool notify = self->props & CHAR_PROP_NOTIFY;
         const bool indicate = self->props & CHAR_PROP_INDICATE;
         if (notify | indicate) {
-            cccd = get_cccd(self);
+            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)) {
-            gatts_notify_indicate(self, bufinfo, 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)) {
-            gatts_notify_indicate(self, bufinfo, BLE_GATT_HVX_INDICATION);
+            characteristic_gatts_notify_indicate(self->handle, conn_handle, bufinfo, BLE_GATT_HVX_INDICATION);
             sent = true;
         }
 
         if (!sent) {
-            gatts_write(self, bufinfo);
+            common_hal_bleio_gatts_write(self->handle, conn_handle, bufinfo);
         }
     }
 }
 
-
 bleio_uuid_obj_t *common_hal_bleio_characteristic_get_uuid(bleio_characteristic_obj_t *self) {
     return self->uuid;
 }
@@ -286,14 +212,13 @@ void common_hal_bleio_characteristic_set_cccd(bleio_characteristic_obj_t *self,
         mp_raise_ValueError(translate("Can't set CCCD on local Characteristic"));
     }
 
+    const uint16_t conn_handle = common_hal_bleio_device_get_conn_handle(self->service->device);
+    common_hal_bleio_check_connected(conn_handle);
+
     uint16_t cccd_value =
         (notify ? BLE_GATT_HVX_NOTIFICATION : 0) |
         (indicate ? BLE_GATT_HVX_INDICATION : 0);
 
-    const uint16_t conn_handle = common_hal_bleio_device_get_conn_handle(self->service->device);
-    check_connected(conn_handle);
-
-
     ble_gattc_write_params_t write_params = {
         .write_op = BLE_GATT_OP_WRITE_REQ,
         .handle = self->cccd_handle,
diff --git a/ports/nrf/common-hal/bleio/Characteristic.h b/ports/nrf/common-hal/bleio/Characteristic.h
index 8c8309f09..46f46eee5 100644
--- a/ports/nrf/common-hal/bleio/Characteristic.h
+++ b/ports/nrf/common-hal/bleio/Characteristic.h
@@ -35,12 +35,14 @@
 
 typedef struct {
     mp_obj_base_t base;
+    // Will be MP_OBJ_NULL before being assigned to a Service.
     bleio_service_obj_t *service;
     bleio_uuid_obj_t *uuid;
-    mp_obj_t value_data;
+    mp_obj_t value;
     uint16_t handle;
     bleio_characteristic_properties_t props;
-    bleio_attribute_security_mode_t security_mode;
+    bleio_attribute_security_mode_t read_perm;
+    bleio_attribute_security_mode_t write_perm;
     mp_obj_list_t *descriptor_list;
     uint16_t user_desc_handle;
     uint16_t cccd_handle;
diff --git a/ports/nrf/common-hal/bleio/Descriptor.c b/ports/nrf/common-hal/bleio/Descriptor.c
index ad62b2535..d6473a64f 100644
--- a/ports/nrf/common-hal/bleio/Descriptor.c
+++ b/ports/nrf/common-hal/bleio/Descriptor.c
@@ -26,13 +26,93 @@
  * THE SOFTWARE.
  */
 
-#include "common-hal/bleio/Descriptor.h"
+#include "py/runtime.h"
+
+#include "shared-bindings/bleio/__init__.h"
+#include "shared-bindings/bleio/Descriptor.h"
+#include "shared-bindings/bleio/Service.h"
 #include "shared-bindings/bleio/UUID.h"
 
-void common_hal_bleio_descriptor_construct(bleio_descriptor_obj_t *self, bleio_uuid_obj_t *uuid) {
+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) {
+    self->characteristic = MP_OBJ_NULL;
     self->uuid = uuid;
+    self->value = mp_const_none;
+    self->handle = BLE_CONN_HANDLE_INVALID;
+    self->read_perm = read_perm;
+    self->write_perm = write_perm;
 }
 
 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) {
+    return self->characteristic;
+}
+
+STATIC void descriptor_on_gattc_read_rsp_evt(ble_evt_t *ble_evt, void *param) {
+    switch (ble_evt->header.evt_id) {
+
+        // More events may be handled later, so keep this as a switch.
+
+        case BLE_GATTC_EVT_READ_RSP: {
+            ble_gattc_evt_read_rsp_t *response = &ble_evt->evt.gattc_evt.params.read_rsp;
+            if (m_read_descriptor) {
+                m_read_descriptor->value = mp_obj_new_bytearray(response->len, response->data);
+            }
+            // Indicate to busy-wait loop that we've read the attribute value.
+            m_read_descriptor = NULL;
+            break;
+        }
+
+        default:
+            // For debugging.
+            // mp_printf(&mp_plat_print, "Unhandled descriptor event: 0x%04x\n", ble_evt->header.evt_id);
+            break;
+    }
+}
+
+STATIC void descriptor_gattc_read(bleio_descriptor_obj_t *descriptor) {
+    const uint16_t conn_handle =
+        common_hal_bleio_device_get_conn_handle(descriptor->characteristic->service->device);
+    common_hal_bleio_check_connected(conn_handle);
+
+    // Set to NULL in event loop after event.
+    m_read_descriptor = descriptor;
+
+    ble_drv_add_event_handler(descriptor_on_gattc_read_rsp_evt, descriptor);
+
+    const uint32_t err_code = sd_ble_gattc_read(conn_handle, descriptor->handle, 0);
+    if (err_code != NRF_SUCCESS) {
+        mp_raise_OSError_msg_varg(translate("Failed to read attribute value, err 0x%04x"), err_code);
+    }
+
+    while (m_read_descriptor != NULL) {
+        MICROPY_VM_HOOK_LOOP;
+    }
+
+    ble_drv_remove_event_handler(descriptor_on_gattc_read_rsp_evt, 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));
+    }
+
+    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);
+    }
+}
diff --git a/ports/nrf/common-hal/bleio/Descriptor.h b/ports/nrf/common-hal/bleio/Descriptor.h
index ad94a0bb9..2fae8cba6 100644
--- a/ports/nrf/common-hal/bleio/Descriptor.h
+++ b/ports/nrf/common-hal/bleio/Descriptor.h
@@ -36,11 +36,13 @@
 
 typedef struct {
     mp_obj_base_t base;
+    // Will be MP_OBJ_NULL before being assigned to a Characteristic.
     bleio_characteristic_obj_t *characteristic;
     bleio_uuid_obj_t *uuid;
-    mp_obj_t value_data;
+    mp_obj_t value;
     uint16_t handle;
-    bleio_attribute_security_mode_t security_mode;
+    bleio_attribute_security_mode_t read_perm;
+    bleio_attribute_security_mode_t write_perm;
 } bleio_descriptor_obj_t;
 
 #endif // MICROPY_INCLUDED_NRF_COMMON_HAL_BLEIO_DESCRIPTOR_H
diff --git a/ports/nrf/common-hal/bleio/Service.c b/ports/nrf/common-hal/bleio/Service.c
index cdcbad7c4..6d2d9d74f 100644
--- a/ports/nrf/common-hal/bleio/Service.c
+++ b/ports/nrf/common-hal/bleio/Service.c
@@ -47,7 +47,6 @@ void common_hal_bleio_service_construct(bleio_service_obj_t *self, bleio_uuid_ob
             MP_OBJ_TO_PTR(characteristic_list->items[characteristic_idx]);
         characteristic->service = self;
     }
-
 }
 
 bleio_uuid_obj_t *common_hal_bleio_service_get_uuid(bleio_service_obj_t *self) {
@@ -146,7 +145,7 @@ void common_hal_bleio_service_add_all_characteristics(bleio_service_obj_t *self)
             BLE_GAP_CONN_SEC_MODE_SET_OPEN(&desc_attr_md.write_perm);
 
             mp_buffer_info_t bufinfo;
-            mp_get_buffer_raise(descriptor->value_data, &bufinfo, MP_BUFFER_READ);
+            mp_get_buffer_raise(descriptor->value, &bufinfo, MP_BUFFER_READ);
 
             ble_gatts_attr_t desc_attr = {
                 .p_uuid = &desc_uuid,
diff --git a/ports/nrf/common-hal/bleio/__init__.c b/ports/nrf/common-hal/bleio/__init__.c
index 303e70797..9879a9ff4 100644
--- a/ports/nrf/common-hal/bleio/__init__.c
+++ b/ports/nrf/common-hal/bleio/__init__.c
@@ -59,13 +59,19 @@ const super_adapter_obj_t common_hal_bleio_adapter_obj = {
     },
 };
 
+void common_hal_bleio_check_connected(uint16_t conn_handle) {
+    if (conn_handle == BLE_CONN_HANDLE_INVALID) {
+        mp_raise_OSError_msg(translate("Not connected"));
+    }
+}
+
 uint16_t common_hal_bleio_device_get_conn_handle(mp_obj_t device) {
     if (MP_OBJ_IS_TYPE(device, &bleio_peripheral_type)) {
         return ((bleio_peripheral_obj_t*) MP_OBJ_TO_PTR(device))->conn_handle;
     } else if (MP_OBJ_IS_TYPE(device, &bleio_central_type)) {
         return ((bleio_central_obj_t*) MP_OBJ_TO_PTR(device))->conn_handle;
     } else {
-        return 0;
+        return BLE_CONN_HANDLE_INVALID;
     }
 }
 
@@ -213,7 +219,7 @@ STATIC void on_char_discovery_rsp(ble_gattc_evt_char_disc_rsp_t *response, mp_ob
             (gattc_char->char_props.write_wo_resp ? CHAR_PROP_WRITE_NO_RESPONSE : 0);
 
         // 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, mp_const_empty_tuple);
+        common_hal_bleio_characteristic_construct(characteristic, uuid, props, SEC_MODE_OPEN, SEC_MODE_OPEN, mp_const_empty_tuple);
         characteristic->handle = gattc_char->handle_value;
         characteristic->service = m_char_discovery_service;
 
@@ -267,8 +273,7 @@ STATIC void on_desc_discovery_rsp(ble_gattc_evt_desc_disc_rsp_t *response, mp_ob
             // For now, just leave the UUID as NULL.
         }
 
-        // TODO: can we find out security mode?
-        common_hal_bleio_descriptor_construct(descriptor, uuid, SEC_MODE_OPEN);
+        common_hal_bleio_descriptor_construct(descriptor, uuid, SEC_MODE_OPEN, SEC_MODE_OPEN);
         descriptor->handle = gattc_desc->handle;
         descriptor->characteristic = m_desc_discovery_characteristic;
 
@@ -418,3 +423,79 @@ void common_hal_bleio_device_discover_remote_services(mp_obj_t device, mp_obj_t
     ble_drv_remove_event_handler(discovery_on_ble_evt, device);
 
 }
+
+// GATTS read of a Characteristic or Descriptor.
+mp_obj_t common_hal_bleio_gatts_read(uint16_t handle, uint16_t conn_handle) {
+    // 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.
+
+    mp_buffer_info_t bufinfo;
+    ble_gatts_value_t gatts_value = {
+        .p_value = NULL,
+        .len = 0,
+    };
+
+    // Read once to find out what size buffer we need, then read again to fill buffer.
+
+    mp_obj_t value = mp_const_none;
+    uint32_t err_code = sd_ble_gatts_value_get(conn_handle, handle, &gatts_value);
+    if (err_code == NRF_SUCCESS) {
+        value = mp_obj_new_bytearray_of_zeros(gatts_value.len);
+        mp_get_buffer_raise(value, &bufinfo, MP_BUFFER_WRITE);
+        gatts_value.p_value = bufinfo.buf;
+
+        // Read again, with the correct size of buffer.
+        err_code = sd_ble_gatts_value_get(conn_handle, handle, &gatts_value);
+    }
+
+    if (err_code != NRF_SUCCESS) {
+        mp_raise_OSError_msg_varg(translate("Failed to read gatts value, err 0x%04x"), err_code);
+    }
+
+    return value;
+}
+
+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.
+
+    ble_gatts_value_t gatts_value = {
+        .p_value = bufinfo->buf,
+        .len = bufinfo->len,
+    };
+
+    const uint32_t err_code = sd_ble_gatts_value_set(conn_handle, handle, &gatts_value);
+    if (err_code != NRF_SUCCESS) {
+        mp_raise_OSError_msg_varg(translate("Failed to write gatts value, err 0x%04x"), err_code);
+    }
+}
+
+void common_hal_bleio_gattc_write(uint16_t handle, uint16_t conn_handle, mp_buffer_info_t *bufinfo, bool write_no_response) {
+    common_hal_bleio_check_connected(conn_handle);
+
+    ble_gattc_write_params_t write_params = {
+        .write_op = write_no_response ? BLE_GATT_OP_WRITE_CMD: BLE_GATT_OP_WRITE_REQ,
+        .handle = handle,
+        .p_value = bufinfo->buf,
+        .len = bufinfo->len,
+    };
+
+    while (1) {
+        uint32_t err_code = sd_ble_gattc_write(conn_handle, &write_params);
+        if (err_code == NRF_SUCCESS) {
+            break;
+        }
+
+        // Write with response will return NRF_ERROR_BUSY if the response has not been received.
+        // Write without reponse will return NRF_ERROR_RESOURCES if too many writes are pending.
+        if (err_code == NRF_ERROR_BUSY || err_code == NRF_ERROR_RESOURCES) {
+            // We could wait for an event indicating the write is complete, but just retrying is easier.
+            MICROPY_VM_HOOK_LOOP;
+            continue;
+        }
+
+        // Some real error occurred.
+        mp_raise_OSError_msg_varg(translate("Failed to write attribute value, err 0x%04x"), err_code);
+    }
+
+}
diff --git a/shared-bindings/bleio/Address.c b/shared-bindings/bleio/Address.c
index d77cfa404..baeb6d7c0 100644
--- a/shared-bindings/bleio/Address.c
+++ b/shared-bindings/bleio/Address.c
@@ -83,7 +83,21 @@ STATIC mp_obj_t bleio_address_make_new(const mp_obj_type_t *type, size_t n_args,
 
 //|   .. attribute:: address_bytes
 //|
-//|     The bytes that make up the device address (read-only)
+//|     The bytes that make up the device address (read-only).
+//|
+//|     Note that the ``bytes`` object returned is in little-endian order:
+//|     The least significant byte is ``address_bytes[0]``. So the address will
+//|     appear to be reversed if you print the raw ``bytes`` object. If you print
+//|     or use `str()` on the :py:class:`~bleio.Attribute` object itself, the address will be printed
+//|     in the expected order. For example:
+//|
+//|     .. code-block:: pycon
+//|
+//|       >>> import bleio
+//|       >>> bleio.adapter.address
+//|       <Address c8:1d:f5:ed:a8:35>
+//|       >>> bleio.adapter.address.address_bytes
+//|       b'5\xa8\xed\xf5\x1d\xc8'
 //|
 STATIC mp_obj_t bleio_address_get_address_bytes(mp_obj_t self_in) {
     bleio_address_obj_t *self = MP_OBJ_TO_PTR(self_in);
diff --git a/shared-bindings/bleio/Attribute.c b/shared-bindings/bleio/Attribute.c
index f640cc05c..ef6b17253 100644
--- a/shared-bindings/bleio/Attribute.c
+++ b/shared-bindings/bleio/Attribute.c
@@ -37,12 +37,12 @@
 //| =========================================================
 //|
 //| Definitions associated with all BLE attributes: characteristics, descriptors, etc.
-//| `Attribute` is, notionally, a superclass of `Characteristic` and `Descriptor`,
+//| :py:class:`~bleio.Attribute` is, notionally, a superclass of `Characteristic` and `Descriptor`,
 //| but is not defined as a Python superclass of those classes.
 //|
 //| .. class:: Attribute()
 //|
-//|   You cannot create an instance of `Attribute`.
+//|   You cannot create an instance of :py:class:`~bleio.Attribute`.
 //|
 
 STATIC const mp_rom_map_elem_t bleio_attribute_locals_dict_table[] = {
diff --git a/shared-bindings/bleio/Characteristic.c b/shared-bindings/bleio/Characteristic.c
index 09ed48ef9..45bec6c28 100644
--- a/shared-bindings/bleio/Characteristic.c
+++ b/shared-bindings/bleio/Characteristic.c
@@ -30,6 +30,7 @@
 #include "py/runtime.h"
 #include "shared-bindings/bleio/Attribute.h"
 #include "shared-bindings/bleio/Characteristic.h"
+#include "shared-bindings/bleio/Descriptor.h"
 #include "shared-bindings/bleio/UUID.h"
 
 //| .. currentmodule:: bleio
@@ -41,24 +42,28 @@
 //| and writing of the characteristic's value.
 //|
 //|
-//| .. class:: Characteristic(uuid, *, properties=0, security_mode=`Attribute.OPEN`, descriptors=None)
+//| .. class:: Characteristic(uuid, *, properties=0, read_perm=`Attribute.OPEN`, write_perm=`Attribute.OPEN`, descriptors=None)
 //|
 //|   Create a new Characteristic object identified by the specified UUID.
 //|
 //|   :param bleio.UUID uuid: The uuid of the characteristic
 //|   :param int properties: bitmask of these values bitwise-or'd together: `BROADCAST`, `INDICATE`,
 //|     `NOTIFY`, `READ`, `WRITE`, `WRITE_NO_RESPONSE`
-//|   :param int security_mode: one of the integer values `Attribute.NO_ACCESS`, `Attribute.OPEN`,
+//|   :param int read_perm: Specifies whether the characteristic can be read by a client, and if so, which
+//|      security mode is required. Must be one of the integer values `Attribute.NO_ACCESS`, `Attribute.OPEN`,
 //|      `Attribute.ENCRYPT_NO_MITM`, `Attribute.ENCRYPT_WITH_MITM`, `Attribute.LESC_ENCRYPT_WITH_MITM`,
-//|      `Attribute.SIGNED_NO_MITM`, `Attribute.SIGNED_WITH_MITM`.
+//|      `Attribute.SIGNED_NO_MITM`, or `Attribute.SIGNED_WITH_MITM`.
+//|   :param int write_perm: Specifies whether the characteristic can be written by a client, and if so, which
+//|      security mode is required. Values allowed are the same as `read_perm`.
 //|   :param iterable descriptors: BLE descriptors for this characteristic.
 //|
 STATIC mp_obj_t bleio_characteristic_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_uuid, ARG_properties, ARG_security_mode, ARG_descriptors };
+    enum { ARG_uuid, ARG_properties, ARG_read_perm, ARG_write_perm, ARG_descriptors };
     static const mp_arg_t allowed_args[] = {
         { MP_QSTR_uuid,  MP_ARG_REQUIRED | MP_ARG_OBJ, {.u_obj = mp_const_none} },
         { MP_QSTR_properties, MP_ARG_KW_ONLY| MP_ARG_INT, {.u_int = 0 } },
-        { MP_QSTR_security_mode, MP_ARG_KW_ONLY| MP_ARG_INT, {.u_int = SEC_MODE_OPEN } },
+        { MP_QSTR_read_perm, MP_ARG_KW_ONLY| MP_ARG_INT, {.u_int = SEC_MODE_OPEN } },
+        { MP_QSTR_write_perm, MP_ARG_KW_ONLY| MP_ARG_INT, {.u_int = SEC_MODE_OPEN } },
         { MP_QSTR_descriptors, MP_ARG_KW_ONLY| MP_ARG_OBJ, {.u_obj = mp_const_none} },
     };
 
@@ -77,8 +82,11 @@ STATIC mp_obj_t bleio_characteristic_make_new(const mp_obj_type_t *type, size_t
         mp_raise_ValueError(translate("Invalid properties"));
     }
 
-    const bleio_attribute_security_mode_t security_mode = args[ARG_security_mode].u_int;
-    common_hal_bleio_attribute_security_mode_check_valid(security_mode);
+    const bleio_attribute_security_mode_t read_perm = args[ARG_read_perm].u_int;
+    common_hal_bleio_attribute_security_mode_check_valid(read_perm);
+
+    const bleio_attribute_security_mode_t write_perm = args[ARG_write_perm].u_int;
+    common_hal_bleio_attribute_security_mode_check_valid(write_perm);
 
     mp_obj_t descriptors = args[ARG_descriptors].u_obj;
     if (descriptors == mp_const_none) {
@@ -88,7 +96,27 @@ STATIC mp_obj_t bleio_characteristic_make_new(const mp_obj_type_t *type, size_t
     bleio_characteristic_obj_t *self = m_new_obj(bleio_characteristic_obj_t);
     self->base.type = &bleio_characteristic_type;
 
-    common_hal_bleio_characteristic_construct(self, uuid, properties, security_mode, descriptors);
+    // If descriptors is not an iterable, an exception will be thrown.
+    mp_obj_iter_buf_t iter_buf;
+    mp_obj_t iterable = mp_getiter(args[ARG_descriptors].u_obj, &iter_buf);
+
+// Copy the descriptors list and validate its items.
+    mp_obj_t desc_list_obj = mp_obj_new_list(0, NULL);
+    mp_obj_list_t *desc_list = MP_OBJ_TO_PTR(desc_list_obj);
+
+    mp_obj_t descriptor_obj;
+    while ((descriptor_obj = mp_iternext(iterable)) != MP_OBJ_STOP_ITERATION) {
+        if (!MP_OBJ_IS_TYPE(descriptor_obj, &bleio_descriptor_type)) {
+            mp_raise_ValueError(translate("descriptors includes an object that is not a Descriptors"));
+        }
+        bleio_descriptor_obj_t *descriptor = MP_OBJ_TO_PTR(descriptor_obj);
+        if (common_hal_bleio_descriptor_get_characteristic(descriptor) != mp_const_none) {
+            mp_raise_ValueError(translate("Descriptor is already attached to a Characteristic"));
+        }
+        mp_obj_list_append(desc_list_obj, descriptor_obj);
+    }
+
+    common_hal_bleio_characteristic_construct(self, uuid, properties, read_perm, write_perm, desc_list);
 
     return MP_OBJ_FROM_PTR(self);
 }
@@ -182,6 +210,24 @@ const mp_obj_property_t bleio_characteristic_descriptors_obj = {
                (mp_obj_t)&mp_const_none_obj },
 };
 
+//|   .. attribute:: service (read-only)
+//|
+//|     The Service this Characteristic is a part of. None if not yet assigned to a Service.
+//|
+STATIC mp_obj_t bleio_characteristic_get_service(mp_obj_t self_in) {
+    bleio_characteristic_obj_t *self = MP_OBJ_TO_PTR(self_in);
+
+    return common_hal_bleio_characteristic_get_service(self);
+}
+STATIC MP_DEFINE_CONST_FUN_OBJ_1(bleio_characteristic_get_service_obj, bleio_characteristic_get_service);
+
+const mp_obj_property_t bleio_characteristic_service_obj = {
+    .base.type = &mp_type_property,
+    .proxy = { (mp_obj_t)&bleio_characteristic_get_service_obj,
+               (mp_obj_t)&mp_const_none_obj,
+               (mp_obj_t)&mp_const_none_obj },
+};
+
 //|   .. method:: set_cccd(*, notify=False, indicate=False)
 //|
 //|     Set the remote characteristic's CCCD to enable or disable notification and indication.
diff --git a/shared-bindings/bleio/Characteristic.h b/shared-bindings/bleio/Characteristic.h
index 379f9b4bf..1d532d748 100644
--- a/shared-bindings/bleio/Characteristic.h
+++ b/shared-bindings/bleio/Characteristic.h
@@ -34,12 +34,13 @@
 
 extern const mp_obj_type_t bleio_characteristic_type;
 
-extern 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 security_mode, mp_obj_t descriptors);
+extern 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);
 extern mp_obj_t common_hal_bleio_characteristic_get_value(bleio_characteristic_obj_t *self);
 extern void common_hal_bleio_characteristic_set_value(bleio_characteristic_obj_t *self, mp_buffer_info_t *bufinfo);
 extern bleio_characteristic_properties_t common_hal_bleio_characteristic_get_properties(bleio_characteristic_obj_t *self);
 extern bleio_uuid_obj_t *common_hal_bleio_characteristic_get_uuid(bleio_characteristic_obj_t *self);
 extern mp_obj_list_t *common_hal_bleio_characteristic_get_descriptor_list(bleio_characteristic_obj_t *self);
+extern bleio_service_obj_t *common_hal_bleio_characteristic_get_service(bleio_characteristic_obj_t *self);
 extern void common_hal_bleio_characteristic_set_cccd(bleio_characteristic_obj_t *self, bool notify, bool indicate);
 
 #endif // MICROPY_INCLUDED_SHARED_BINDINGS_BLEIO_CHARACTERISTIC_H
diff --git a/shared-bindings/bleio/Descriptor.c b/shared-bindings/bleio/Descriptor.c
index b709f3aa7..1ab8c9ecd 100644
--- a/shared-bindings/bleio/Descriptor.c
+++ b/shared-bindings/bleio/Descriptor.c
@@ -42,15 +42,24 @@
 //| information about the characteristic.
 //|
 
-//| .. class:: Descriptor(uuid, security_mode=`Attribute.OPEN`)
+//| .. class:: Descriptor(uuid, *, read_perm=`Attribute.OPEN`, write_perm=`Attribute.OPEN`)
+//|
+//|   Create a new descriptor object with the UUID uuid
+//|
+//|   :param bleio.UUID uuid: The uuid of the descriptor
+//|   :param int read_perm: Specifies whether the descriptor can be read by a client, and if so, which
+//|      security mode is required. Must be one of the integer values `Attribute.NO_ACCESS`, `Attribute.OPEN`,
+//|      `Attribute.ENCRYPT_NO_MITM`, `Attribute.ENCRYPT_WITH_MITM`, `Attribute.LESC_ENCRYPT_WITH_MITM`,
+//|      `Attribute.SIGNED_NO_MITM`, or `Attribute.SIGNED_WITH_MITM`.
+//|   :param int write_perm: Specifies whether the descriptor can be written by a client, and if so, which
+//|      security mode is required. Values allowed are the same as `read_perm`.
 //|
-//|   Create a new descriptor object with the UUID uuid and the given value, if any.
-
 STATIC mp_obj_t bleio_descriptor_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_uuid, ARG_security_mode };
+    enum { ARG_uuid, ARG_read_perm, ARG_write_perm };
     static const mp_arg_t allowed_args[] = {
         { MP_QSTR_uuid, MP_ARG_REQUIRED | MP_ARG_OBJ },
-        { MP_QSTR_security_mode, MP_ARG_KW_ONLY| MP_ARG_INT, {.u_int = SEC_MODE_OPEN } },
+        { MP_QSTR_read_perm, MP_ARG_KW_ONLY| MP_ARG_INT, {.u_int = SEC_MODE_OPEN } },
+        { MP_QSTR_write_perm, MP_ARG_KW_ONLY| MP_ARG_INT, {.u_int = SEC_MODE_OPEN } },
     };
 
     mp_arg_val_t args[MP_ARRAY_SIZE(allowed_args)];
@@ -62,14 +71,17 @@ STATIC mp_obj_t bleio_descriptor_make_new(const mp_obj_type_t *type, size_t n_ar
         mp_raise_ValueError(translate("Expected a UUID"));
     }
 
-    const bleio_attribute_security_mode_t security_mode = args[ARG_security_mode].u_int;
-    common_hal_bleio_attribute_security_mode_check_valid(security_mode);
+    const bleio_attribute_security_mode_t read_perm = args[ARG_read_perm].u_int;
+    common_hal_bleio_attribute_security_mode_check_valid(read_perm);
+
+    const bleio_attribute_security_mode_t write_perm = args[ARG_write_perm].u_int;
+    common_hal_bleio_attribute_security_mode_check_valid(write_perm);
 
     bleio_descriptor_obj_t *self = m_new_obj(bleio_descriptor_obj_t);
     self->base.type = type;
     bleio_uuid_obj_t *uuid = MP_OBJ_TO_PTR(uuid_arg);
 
-    common_hal_bleio_descriptor_construct(self, uuid, security_mode);
+    common_hal_bleio_descriptor_construct(self, uuid, read_perm, write_perm);
 
     return MP_OBJ_FROM_PTR(self);
 }
@@ -93,6 +105,24 @@ const mp_obj_property_t bleio_descriptor_uuid_obj = {
               (mp_obj_t)&mp_const_none_obj},
 };
 
+//|   .. attribute:: characteristic (read-only)
+//|
+//|     The Characteristic this Descriptor is a part of. None if not yet assigned to a Characteristic.
+//|
+STATIC mp_obj_t bleio_descriptor_get_characteristic(mp_obj_t self_in) {
+    bleio_descriptor_obj_t *self = MP_OBJ_TO_PTR(self_in);
+
+    return common_hal_bleio_descriptor_get_characteristic(self);
+}
+STATIC MP_DEFINE_CONST_FUN_OBJ_1(bleio_descriptor_get_characteristic_obj, bleio_descriptor_get_characteristic);
+
+const mp_obj_property_t bleio_descriptor_characteristic_obj = {
+    .base.type = &mp_type_property,
+    .proxy = { (mp_obj_t)&bleio_descriptor_get_characteristic_obj,
+               (mp_obj_t)&mp_const_none_obj,
+               (mp_obj_t)&mp_const_none_obj },
+};
+
 STATIC const mp_rom_map_elem_t bleio_descriptor_locals_dict_table[] = {
     // Properties
     { MP_ROM_QSTR(MP_QSTR_uuid), MP_ROM_PTR(&bleio_descriptor_uuid_obj) },
diff --git a/shared-bindings/bleio/Descriptor.h b/shared-bindings/bleio/Descriptor.h
index a7daee574..eefce03d1 100644
--- a/shared-bindings/bleio/Descriptor.h
+++ b/shared-bindings/bleio/Descriptor.h
@@ -34,8 +34,8 @@
 
 extern const mp_obj_type_t bleio_descriptor_type;
 
-extern mp_int_t common_hal_bleio_descriptor_get_handle(bleio_descriptor_obj_t *self);
-extern mp_obj_t common_hal_bleio_descriptor_get_uuid(bleio_descriptor_obj_t *self);
-extern void common_hal_bleio_descriptor_construct(bleio_descriptor_obj_t *self, bleio_uuid_obj_t *uuid, bleio_attribute_security_mode_t security_mode);
+extern 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);
+extern bleio_uuid_obj_t *common_hal_bleio_descriptor_get_uuid(bleio_descriptor_obj_t *self);
+extern mp_obj_t common_hal_bleio_descriptor_get_characteristic(bleio_descriptor_obj_t *self);
 
 #endif // MICROPY_INCLUDED_SHARED_BINDINGS_BLEIO_DESCRIPTOR_H
diff --git a/shared-bindings/bleio/Service.c b/shared-bindings/bleio/Service.c
index b4aeab2bd..6961b0b52 100644
--- a/shared-bindings/bleio/Service.c
+++ b/shared-bindings/bleio/Service.c
@@ -96,6 +96,9 @@ STATIC mp_obj_t bleio_service_make_new(const mp_obj_type_t *type, size_t n_args,
             // The descriptor base UUID doesn't match the characteristic base UUID.
             mp_raise_ValueError(translate("Characteristic UUID doesn't match Service UUID"));
         }
+        if (common_hal_bleio_characteristic_get_service(characteristic) != MP_OBJ_NULL) {
+            mp_raise_ValueError(translate("Characteristic is already attached to a Service"));
+        }
         mp_obj_list_append(char_list_obj, characteristic_obj);
     }
 
diff --git a/shared-bindings/bleio/__init__.c b/shared-bindings/bleio/__init__.c
index 7a6f96436..0009cf113 100644
--- a/shared-bindings/bleio/__init__.c
+++ b/shared-bindings/bleio/__init__.c
@@ -59,6 +59,7 @@
 //|
 //|     Address
 //|     Adapter
+//|     Attribute
 //|     Central
 //|     Characteristic
 //|     CharacteristicBuffer
diff --git a/shared-bindings/bleio/__init__.h b/shared-bindings/bleio/__init__.h
index 626b188a9..6a4c804f5 100644
--- a/shared-bindings/bleio/__init__.h
+++ b/shared-bindings/bleio/__init__.h
@@ -38,9 +38,15 @@
 
 extern const super_adapter_obj_t common_hal_bleio_adapter_obj;
 
+extern void common_hal_bleio_check_connected(uint16_t conn_handle);
+
 extern uint16_t common_hal_bleio_device_get_conn_handle(mp_obj_t device);
 extern mp_obj_list_t *common_hal_bleio_device_get_remote_services_list(mp_obj_t device);
 extern void common_hal_bleio_device_discover_remote_services(mp_obj_t device, mp_obj_t service_uuids_whitelist);
 
+extern mp_obj_t common_hal_bleio_gatts_read(uint16_t handle, uint16_t conn_handle);
+extern void common_hal_bleio_gatts_write(uint16_t handle, uint16_t conn_handle, mp_buffer_info_t *bufinfo);
+extern void common_hal_bleio_gattc_write(uint16_t handle, uint16_t conn_handle, mp_buffer_info_t *bufinfo, bool write_no_response);
+
 
 #endif // MICROPY_INCLUDED_SHARED_BINDINGS_BLEIO___INIT___H