address review comments; avoid calling common_hal_bleio_device... routines from shared-bindings

Author: dhalbert

ports/nrf/common-hal/bleio/Attribute.c

@@ -29,31 +29,31 @@
 // Convert a bleio security mode to a ble_gap_conn_sec_mode_t setting.
 void bleio_attribute_gatts_set_security_mode(ble_gap_conn_sec_mode_t *perm, bleio_attribute_security_mode_t security_mode) {
     switch (security_mode) {
-        case SEC_MODE_NO_ACCESS:
+        case SECURITY_MODE_NO_ACCESS:
             BLE_GAP_CONN_SEC_MODE_SET_NO_ACCESS(perm);
             break;
 
-        case SEC_MODE_OPEN:
+        case SECURITY_MODE_OPEN:
             BLE_GAP_CONN_SEC_MODE_SET_OPEN(perm);
             break;
 
-        case SEC_MODE_ENC_NO_MITM:
+        case SECURITY_MODE_ENC_NO_MITM:
             BLE_GAP_CONN_SEC_MODE_SET_ENC_NO_MITM(perm);
             break;
 
-        case SEC_MODE_ENC_WITH_MITM:
+        case SECURITY_MODE_ENC_WITH_MITM:
             BLE_GAP_CONN_SEC_MODE_SET_ENC_WITH_MITM(perm);
             break;
 
-        case SEC_MODE_LESC_ENC_WITH_MITM:
+        case SECURITY_MODE_LESC_ENC_WITH_MITM:
             BLE_GAP_CONN_SEC_MODE_SET_LESC_ENC_WITH_MITM(perm);
             break;
 
-        case SEC_MODE_SIGNED_NO_MITM:
+        case SECURITY_MODE_SIGNED_NO_MITM:
             BLE_GAP_CONN_SEC_MODE_SET_SIGNED_NO_MITM(perm);
             break;
 
-        case SEC_MODE_SIGNED_WITH_MITM:
+        case SECURITY_MODE_SIGNED_WITH_MITM:
             BLE_GAP_CONN_SEC_MODE_SET_SIGNED_WITH_MITM(perm);
             break;
     }

ports/nrf/common-hal/bleio/Central.c

@@ -34,9 +34,9 @@
 #include "nrf_soc.h"
 #include "py/objstr.h"
 #include "py/runtime.h"
+#include "shared-bindings/bleio/__init__.h"
 #include "shared-bindings/bleio/Adapter.h"
 #include "shared-bindings/bleio/Central.h"
-#include "common-hal/bleio/__init__.h"
 
 STATIC void central_on_ble_evt(ble_evt_t *ble_evt, void *central_in) {
     bleio_central_obj_t *central = (bleio_central_obj_t*)central_in;
@@ -131,6 +131,15 @@ bool common_hal_bleio_central_get_connected(bleio_central_obj_t *self) {
     return self->conn_handle != BLE_CONN_HANDLE_INVALID;
 }
 
+mp_obj_tuple_t *common_hal_bleio_central_discover_remote_services(bleio_central_obj_t *self, mp_obj_t service_uuids_whitelist) {
+    common_hal_bleio_device_discover_remote_services(MP_OBJ_FROM_PTR(self), service_uuids_whitelist);
+    // Convert to a tuple and then clear the list so the callee will take ownership.
+    mp_obj_tuple_t *services_tuple = mp_obj_new_tuple(self->remote_services_list->len,
+                                                      self->remote_services_list->items);
+    mp_obj_list_clear(self->remote_services_list);
+    return services_tuple;
+}
+
 mp_obj_list_t *common_hal_bleio_central_get_remote_services(bleio_central_obj_t *self) {
     return self->remote_services_list;
 }

ports/nrf/common-hal/bleio/Peripheral.c

@@ -36,12 +36,12 @@
 #include "py/objlist.h"
 #include "py/objstr.h"
 #include "py/runtime.h"
+#include "shared-bindings/bleio/__init__.h"
 #include "shared-bindings/bleio/Adapter.h"
 #include "shared-bindings/bleio/Characteristic.h"
 #include "shared-bindings/bleio/Peripheral.h"
 #include "shared-bindings/bleio/Service.h"
 #include "shared-bindings/bleio/UUID.h"
-#include "common-hal/bleio/Service.h"
 
 #define BLE_MIN_CONN_INTERVAL        MSEC_TO_UNITS(15, UNIT_0_625_MS)
 #define BLE_MAX_CONN_INTERVAL        MSEC_TO_UNITS(300, UNIT_0_625_MS)
@@ -303,8 +303,13 @@ void common_hal_bleio_peripheral_disconnect(bleio_peripheral_obj_t *self) {
     sd_ble_gap_disconnect(self->conn_handle, BLE_HCI_REMOTE_USER_TERMINATED_CONNECTION);
 }
 
-mp_obj_list_t *common_hal_bleio_peripheral_get_remote_services(bleio_peripheral_obj_t *self) {
-    return self->remote_services_list;
+mp_obj_tuple_t *common_hal_bleio_peripheral_discover_remote_services(bleio_peripheral_obj_t *self, mp_obj_t service_uuids_whitelist) {
+    common_hal_bleio_device_discover_remote_services(MP_OBJ_FROM_PTR(self), service_uuids_whitelist);
+    // Convert to a tuple and then clear the list so the callee will take ownership.
+    mp_obj_tuple_t *services_tuple = mp_obj_new_tuple(self->remote_services_list->len,
+                                                      self->remote_services_list->items);
+    mp_obj_list_clear(self->remote_services_list);
+    return services_tuple;
 }
 
 void common_hal_bleio_peripheral_pair(bleio_peripheral_obj_t *self) {

ports/nrf/common-hal/bleio/__init__.c

@@ -218,7 +218,7 @@ 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,
+            characteristic, uuid, props, SECURITY_MODE_OPEN, SECURITY_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;
@@ -274,7 +274,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.
         }
 
-        common_hal_bleio_descriptor_construct(descriptor, uuid, SEC_MODE_OPEN, SEC_MODE_OPEN,
+        common_hal_bleio_descriptor_construct(descriptor, uuid, SECURITY_MODE_OPEN, SECURITY_MODE_OPEN,
                                               GATT_MAX_DATA_LENGTH, false);
         descriptor->handle = gattc_desc->handle;
         descriptor->characteristic = m_desc_discovery_characteristic;

shared-bindings/bleio/Attribute.c

@@ -76,13 +76,13 @@ STATIC const mp_rom_map_elem_t bleio_attribute_locals_dict_table[] = {
 //|
 //|      security_mode: authenticated data signing, without man-in-the-middle protection
 //|
-    { MP_ROM_QSTR(MP_QSTR_NO_ACCESS),              MP_ROM_INT(SEC_MODE_NO_ACCESS) },
-    { MP_ROM_QSTR(MP_QSTR_OPEN),                   MP_ROM_INT(SEC_MODE_OPEN) },
-    { MP_ROM_QSTR(MP_QSTR_ENCRYPT_NO_MITM),        MP_ROM_INT(SEC_MODE_ENC_NO_MITM) },
-    { MP_ROM_QSTR(MP_QSTR_ENCRYPT_WITH_MITM),      MP_ROM_INT(SEC_MODE_ENC_WITH_MITM) },
-    { MP_ROM_QSTR(MP_QSTR_LESC_ENCRYPT_WITH_MITM), MP_ROM_INT(SEC_MODE_LESC_ENC_WITH_MITM) },
-    { MP_ROM_QSTR(MP_QSTR_SIGNED_NO_MITM),         MP_ROM_INT(SEC_MODE_SIGNED_NO_MITM) },
-    { MP_ROM_QSTR(MP_QSTR_SIGNED_WITH_MITM),       MP_ROM_INT(SEC_MODE_SIGNED_WITH_MITM) },
+    { MP_ROM_QSTR(MP_QSTR_NO_ACCESS),              MP_ROM_INT(SECURITY_MODE_NO_ACCESS) },
+    { MP_ROM_QSTR(MP_QSTR_OPEN),                   MP_ROM_INT(SECURITY_MODE_OPEN) },
+    { MP_ROM_QSTR(MP_QSTR_ENCRYPT_NO_MITM),        MP_ROM_INT(SECURITY_MODE_ENC_NO_MITM) },
+    { MP_ROM_QSTR(MP_QSTR_ENCRYPT_WITH_MITM),      MP_ROM_INT(SECURITY_MODE_ENC_WITH_MITM) },
+    { MP_ROM_QSTR(MP_QSTR_LESC_ENCRYPT_WITH_MITM), MP_ROM_INT(SECURITY_MODE_LESC_ENC_WITH_MITM) },
+    { MP_ROM_QSTR(MP_QSTR_SIGNED_NO_MITM),         MP_ROM_INT(SECURITY_MODE_SIGNED_NO_MITM) },
+    { MP_ROM_QSTR(MP_QSTR_SIGNED_WITH_MITM),       MP_ROM_INT(SECURITY_MODE_SIGNED_WITH_MITM) },
 
 };
 STATIC MP_DEFINE_CONST_DICT(bleio_attribute_locals_dict, bleio_attribute_locals_dict_table);

shared-bindings/bleio/Central.c

@@ -34,7 +34,6 @@
 #include "py/objproperty.h"
 #include "py/objstr.h"
 #include "py/runtime.h"
-#include "shared-bindings/bleio/__init__.h"
 #include "shared-bindings/bleio/Adapter.h"
 #include "shared-bindings/bleio/Address.h"
 #include "shared-bindings/bleio/Characteristic.h"
@@ -66,7 +65,7 @@
 //|
 //|    central = bleio.Central()
 //|    central.connect(my_entry.address, 10)  # timeout after 10 seconds
-//|    central.discover_remote_services()
+//|    remote_services = central.discover_remote_services()
 //|
 
 //| .. class:: Central()
@@ -132,15 +131,13 @@ STATIC MP_DEFINE_CONST_FUN_OBJ_1(bleio_central_disconnect_obj, bleio_central_dis
 
 //|   .. method:: discover_remote_services(service_uuids_whitelist=None)
 //|      Do BLE discovery for all services or for the given service UUIDS,
-//|      to find their handles and characteristics.
-//|      The attribute `remote_services` will contain a list of all discovered services.
-//|      `Central.connected` must be True.
+//|      to find their handles and characteristics, and return the discovered services.
+//|      `Peripheral.connected` must be True.
 //|
 //|   :param iterable service_uuids_whitelist: an iterable of :py:class:~`UUID` objects for the services
 //|      provided by the peripheral that you want to use.
-//|      The peripheral may provide more services, but services not listed are ignored.
-//|      If a service in service_uuids_whitelist is not found during discovery, it will not
-//|      appear in `remote_services`.
+//|      The peripheral may provide more services, but services not listed are ignored
+//|      and will not be returned.
 //|
 //|      If service_uuids_whitelist is None, then all services will undergo discovery, which can be slow.
 //|
@@ -149,6 +146,8 @@ STATIC MP_DEFINE_CONST_FUN_OBJ_1(bleio_central_disconnect_obj, bleio_central_dis
 //|      service or characteristic to be discovered. Creating the UUID causes the UUID to be registered
 //|      for use. (This restriction may be lifted in the future.)
 //|
+//|    :return: A tuple of services provided by the remote peripheral.
+//|
 STATIC mp_obj_t bleio_central_discover_remote_services(mp_uint_t n_args, const mp_obj_t *pos_args, mp_map_t *kw_args) {
     bleio_central_obj_t *self = MP_OBJ_TO_PTR(pos_args[0]);
 
@@ -164,10 +163,9 @@ STATIC mp_obj_t bleio_central_discover_remote_services(mp_uint_t n_args, const m
         mp_raise_ValueError(translate("Not connected"));
     }
 
-    common_hal_bleio_device_discover_remote_services(MP_OBJ_FROM_PTR(self),
-                                                     args[ARG_service_uuids_whitelist].u_obj);
-
-    return mp_const_none;
+    return MP_OBJ_FROM_PTR(common_hal_bleio_central_discover_remote_services(
+                               MP_OBJ_FROM_PTR(self),
+                               args[ARG_service_uuids_whitelist].u_obj));
 }
 STATIC MP_DEFINE_CONST_FUN_OBJ_KW(bleio_central_discover_remote_services_obj, 1, bleio_central_discover_remote_services);
 
@@ -189,28 +187,6 @@ const mp_obj_property_t bleio_central_connected_obj = {
                (mp_obj_t)&mp_const_none_obj },
 };
 
-
-//|   .. attribute:: remote_services (read-only)
-//|
-//|     A tuple of services provided by the remote peripheral.
-//|     If the Central is not connected, an empty tuple will be returned.
-//|
-STATIC mp_obj_t bleio_central_get_remote_services(mp_obj_t self_in) {
-    bleio_central_obj_t *self = MP_OBJ_TO_PTR(self_in);
-
-    // Return list as a tuple so user won't be able to change it.
-    mp_obj_list_t *service_list = common_hal_bleio_central_get_remote_services(self);
-    return mp_obj_new_tuple(service_list->len, service_list->items);
-}
-STATIC MP_DEFINE_CONST_FUN_OBJ_1(bleio_central_get_remote_services_obj, bleio_central_get_remote_services);
-
-const mp_obj_property_t bleio_central_remote_services_obj = {
-    .base.type = &mp_type_property,
-    .proxy = { (mp_obj_t)&bleio_central_get_remote_services_obj,
-               (mp_obj_t)&mp_const_none_obj,
-               (mp_obj_t)&mp_const_none_obj },
-};
-
 STATIC const mp_rom_map_elem_t bleio_central_locals_dict_table[] = {
     // Methods
     { MP_ROM_QSTR(MP_QSTR_connect),                  MP_ROM_PTR(&bleio_central_connect_obj) },
@@ -219,7 +195,6 @@ STATIC const mp_rom_map_elem_t bleio_central_locals_dict_table[] = {
 
     // Properties
     { MP_ROM_QSTR(MP_QSTR_connected),       MP_ROM_PTR(&bleio_central_connected_obj) },
-    { MP_ROM_QSTR(MP_QSTR_remote_services), MP_ROM_PTR(&bleio_central_remote_services_obj) },
 };
 
 STATIC MP_DEFINE_CONST_DICT(bleio_central_locals_dict, bleio_central_locals_dict_table);

shared-bindings/bleio/Central.h

@@ -28,6 +28,7 @@
 #ifndef MICROPY_INCLUDED_SHARED_BINDINGS_BLEIO_CENTRAL_H
 #define MICROPY_INCLUDED_SHARED_BINDINGS_BLEIO_CENTRAL_H
 
+#include "py/objtuple.h"
 #include "common-hal/bleio/Central.h"
 #include "common-hal/bleio/Service.h"
 
@@ -37,6 +38,6 @@ extern void common_hal_bleio_central_construct(bleio_central_obj_t *self);
 extern void common_hal_bleio_central_connect(bleio_central_obj_t *self, bleio_address_obj_t *address, mp_float_t timeout);
 extern void common_hal_bleio_central_disconnect(bleio_central_obj_t *self);
 extern bool common_hal_bleio_central_get_connected(bleio_central_obj_t *self);
-extern mp_obj_list_t *common_hal_bleio_central_get_remote_services(bleio_central_obj_t *self);
+extern mp_obj_tuple_t *common_hal_bleio_central_discover_remote_services(bleio_central_obj_t *self, mp_obj_t service_uuids_whitelist);
 
 #endif // MICROPY_INCLUDED_SHARED_BINDINGS_BLEIO_CENTRAL_H

shared-bindings/bleio/Characteristic.c

@@ -67,8 +67,8 @@ STATIC mp_obj_t bleio_characteristic_make_new(const mp_obj_type_t *type, size_t
     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_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_read_perm, MP_ARG_KW_ONLY| MP_ARG_INT, {.u_int = SECURITY_MODE_OPEN} },
+        { MP_QSTR_write_perm, MP_ARG_KW_ONLY| MP_ARG_INT, {.u_int = SECURITY_MODE_OPEN} },
         { MP_QSTR_max_length, MP_ARG_KW_ONLY| MP_ARG_INT, {.u_int = 20} },
         { MP_QSTR_fixed_length, MP_ARG_KW_ONLY| MP_ARG_BOOL, {.u_bool = false} },
         { MP_QSTR_descriptors, MP_ARG_KW_ONLY| MP_ARG_OBJ, {.u_obj = mp_const_none} },

shared-bindings/bleio/Descriptor.c

@@ -62,8 +62,8 @@ STATIC mp_obj_t bleio_descriptor_make_new(const mp_obj_type_t *type, size_t n_ar
     enum { ARG_uuid, ARG_read_perm, ARG_write_perm, ARG_max_length, ARG_fixed_length };
     static const mp_arg_t allowed_args[] = {
         { MP_QSTR_uuid, MP_ARG_REQUIRED | MP_ARG_OBJ },
-        { 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_read_perm, MP_ARG_KW_ONLY| MP_ARG_INT, {.u_int = SECURITY_MODE_OPEN } },
+        { MP_QSTR_write_perm, MP_ARG_KW_ONLY| MP_ARG_INT, {.u_int = SECURITY_MODE_OPEN } },
         { MP_QSTR_max_length, MP_ARG_KW_ONLY| MP_ARG_INT, {.u_int = 20} },
         { MP_QSTR_fixed_length, MP_ARG_KW_ONLY| MP_ARG_BOOL, {.u_bool = false} },
     };

shared-bindings/bleio/Peripheral.c

@@ -35,7 +35,6 @@
 #include "py/objstr.h"
 #include "py/runtime.h"
 
-#include "shared-bindings/bleio/__init__.h"
 #include "shared-bindings/bleio/Adapter.h"
 #include "shared-bindings/bleio/Characteristic.h"
 #include "shared-bindings/bleio/Peripheral.h"
@@ -263,15 +262,13 @@ STATIC MP_DEFINE_CONST_FUN_OBJ_1(bleio_peripheral_disconnect_obj, bleio_peripher
 
 //|   .. method:: discover_remote_services(service_uuids_whitelist=None)
 //|      Do BLE discovery for all services or for the given service UUIDS,
-//|      to find their handles and characteristics.
-//|      The attribute `remote_services` will contain a list of all discovered services.
+//|      to find their handles and characteristics, and return the discovered services.
 //|      `Peripheral.connected` must be True.
 //|
 //|   :param iterable service_uuids_whitelist: an iterable of :py:class:~`UUID` objects for the services
 //|      provided by the peripheral that you want to use.
-//|      The peripheral may provide more services, but services not listed are ignored.
-//|      If a service in service_uuids_whitelist is not found during discovery, it will not
-//|      appear in `remote_services`.
+//|      The peripheral may provide more services, but services not listed are ignored
+//|      and will not be returned.
 //|
 //|      If service_uuids_whitelist is None, then all services will undergo discovery, which can be slow.
 //|
@@ -285,6 +282,8 @@ STATIC MP_DEFINE_CONST_FUN_OBJ_1(bleio_peripheral_disconnect_obj, bleio_peripher
 //|      Examples include a peripheral accessing a central that provides Current Time Service,
 //|      Apple Notification Center Service, or Battery Service.
 //|
+//|    :return: A tuple of services provided by the remote central.
+//|
 STATIC mp_obj_t bleio_peripheral_discover_remote_services(mp_uint_t n_args, const mp_obj_t *pos_args, mp_map_t *kw_args) {
     bleio_peripheral_obj_t *self = MP_OBJ_TO_PTR(pos_args[0]);
 
@@ -300,27 +299,12 @@ STATIC mp_obj_t bleio_peripheral_discover_remote_services(mp_uint_t n_args, cons
         mp_raise_ValueError(translate("Not connected"));
     }
 
-    common_hal_bleio_device_discover_remote_services(MP_OBJ_FROM_PTR(self),
-                                                     args[ARG_service_uuids_whitelist].u_obj);
-
-    return mp_const_none;
+    return MP_OBJ_FROM_PTR(common_hal_bleio_peripheral_discover_remote_services(
+                               MP_OBJ_FROM_PTR(self),
+                               args[ARG_service_uuids_whitelist].u_obj));
 }
 STATIC MP_DEFINE_CONST_FUN_OBJ_KW(bleio_peripheral_discover_remote_services_obj, 1, bleio_peripheral_discover_remote_services);
 
-//|   .. attribute:: remote_services (read-only)
-//|
-//|     A tuple of services provided by the remote central.
-//|     If discovery did not occur, an empty tuple will be returned.
-//|
-STATIC mp_obj_t bleio_peripheral_get_remote_services(mp_obj_t self_in) {
-    bleio_peripheral_obj_t *self = MP_OBJ_TO_PTR(self_in);
-
-    // Return list as a tuple so user won't be able to change it.
-    mp_obj_list_t *service_list = common_hal_bleio_peripheral_get_remote_services(self);
-    return mp_obj_new_tuple(service_list->len, service_list->items);
-}
-STATIC MP_DEFINE_CONST_FUN_OBJ_1(bleio_peripheral_get_remote_services_obj, bleio_peripheral_get_remote_services);
-
 //|   .. method:: pair()
 //|
 //|     Request pairing with connected central.
@@ -333,14 +317,6 @@ STATIC mp_obj_t bleio_peripheral_pair(mp_obj_t self_in) {
 }
 STATIC MP_DEFINE_CONST_FUN_OBJ_1(bleio_peripheral_pair_obj, bleio_peripheral_pair);
 
-const mp_obj_property_t bleio_peripheral_remote_services_obj = {
-    .base.type = &mp_type_property,
-    .proxy = { (mp_obj_t)&bleio_peripheral_get_remote_services_obj,
-               (mp_obj_t)&mp_const_none_obj,
-               (mp_obj_t)&mp_const_none_obj },
-};
-
-
 STATIC const mp_rom_map_elem_t bleio_peripheral_locals_dict_table[] = {
     // Methods
     { MP_ROM_QSTR(MP_QSTR_start_advertising),        MP_ROM_PTR(&bleio_peripheral_start_advertising_obj) },
@@ -352,7 +328,6 @@ STATIC const mp_rom_map_elem_t bleio_peripheral_locals_dict_table[] = {
     // Properties
     { MP_ROM_QSTR(MP_QSTR_connected),       MP_ROM_PTR(&bleio_peripheral_connected_obj) },
     { MP_ROM_QSTR(MP_QSTR_name),            MP_ROM_PTR(&bleio_peripheral_name_obj) },
-    { MP_ROM_QSTR(MP_QSTR_remote_services), MP_ROM_PTR(&bleio_peripheral_remote_services_obj) },
     { MP_ROM_QSTR(MP_QSTR_services),        MP_ROM_PTR(&bleio_peripheral_services_obj) },
 };