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

6 files changed, 238 insertions, 149 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);
+    }
+
+}