Merge pull request adafruit#9374 from dhalbert/espressif-ble-fixes

Various Espressif BLE fixes

Author: tannewt

locale/circuitpython.pot

@@ -2007,6 +2007,10 @@ msgstr ""
 msgid "Too many channels in sample."
 msgstr ""
 
+#: ports/espressif/common-hal/_bleio/Characteristic.c
+msgid "Too many descriptors"
+msgstr ""
+
 #: shared-module/displayio/__init__.c
 msgid "Too many display busses; forgot displayio.release_displays() ?"
 msgstr ""
@@ -2215,8 +2219,9 @@ msgstr ""
 msgid "Unsupported hash algorithm"
 msgstr ""
 
+#: ports/espressif/common-hal/_bleio/Adapter.c
 #: ports/espressif/common-hal/dualbank/__init__.c
-msgid "Update Failed"
+msgid "Update failed"
 msgstr ""
 
 #: ports/espressif/common-hal/_bleio/Characteristic.c

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

@@ -24,6 +24,7 @@
 #include "shared-bindings/_bleio/Connection.h"
 #include "shared-bindings/_bleio/ScanEntry.h"
 #include "shared-bindings/time/__init__.h"
+#include "shared/runtime/interrupt_char.h"
 
 #include "controller/ble_ll_adv.h"
 #include "nimble/hci_common.h"
@@ -47,22 +48,23 @@
 #include "shared-module/os/__init__.h"
 #endif
 
-bleio_connection_internal_t bleio_connections[BLEIO_TOTAL_CONNECTION_COUNT];
+// Status variables used while busy-waiting for events.
+static volatile bool _nimble_sync;
+static volatile int _connection_status;
 
-bool ble_active = false;
+bleio_connection_internal_t bleio_connections[BLEIO_TOTAL_CONNECTION_COUNT];
 
 static void nimble_host_task(void *param) {
     nimble_port_run();
     nimble_port_freertos_deinit();
 }
 
-static TaskHandle_t cp_task = NULL;
 
 static void _on_sync(void) {
     int rc = ble_hs_util_ensure_addr(false);
     assert(rc == 0);
 
-    xTaskNotifyGive(cp_task);
+    _nimble_sync = true;
 }
 
 // All examples have this. It'd make sense in a header.
@@ -133,15 +135,26 @@ void common_hal_bleio_adapter_set_enabled(bleio_adapter_obj_t *self, bool enable
 
         ble_store_config_init();
 
-        cp_task = xTaskGetCurrentTaskHandle();
-
+        _nimble_sync = false;
         nimble_port_freertos_init(nimble_host_task);
-        // Wait for sync.
-        ulTaskNotifyTake(pdTRUE, pdMS_TO_TICKS(200));
+        // Wait for sync from nimble task.
+        const uint64_t timeout_time_ms = common_hal_time_monotonic_ms() + 200;
+        while (!_nimble_sync && (common_hal_time_monotonic_ms() < timeout_time_ms)) {
+            RUN_BACKGROUND_TASKS;
+            if (mp_hal_is_interrupted()) {
+                // Return prematurely. Then the interrupt will be raised.
+                return;
+            }
+        }
+
+        if (!_nimble_sync) {
+            mp_raise_RuntimeError(MP_ERROR_TEXT("Update failed"));
+        }
     } else {
         int ret = nimble_port_stop();
-        while (xTaskGetHandle("nimble_host") != NULL) {
-            vTaskDelay(pdMS_TO_TICKS(2));
+        while (xTaskGetHandle("nimble_host") != NULL && !mp_hal_is_interrupted()) {
+            RUN_BACKGROUND_TASKS;
+            common_hal_time_delay_ms(2);
         }
         if (ret == 0) {
             nimble_port_deinit();
@@ -277,8 +290,19 @@ mp_obj_t common_hal_bleio_adapter_start_scan(bleio_adapter_obj_t *self, uint8_t
         duration_ms = BLE_HS_FOREVER;
     }
 
-    CHECK_NIMBLE_ERROR(ble_gap_disc(own_addr_type, duration_ms, &disc_params,
-        _scan_event, self->scan_results));
+    int tries = 5;
+    int status;
+    // BLE_HS_EBUSY may occasionally occur, indicating something has not finished. Retry a few times if so.
+    do {
+        status = ble_gap_disc(own_addr_type, duration_ms, &disc_params,
+            _scan_event, self->scan_results);
+        if (status != BLE_HS_EBUSY) {
+            break;
+        }
+        common_hal_time_delay_ms(50);
+        RUN_BACKGROUND_TASKS;
+    } while (tries-- > 0);
+    CHECK_NIMBLE_ERROR(status);
 
     return MP_OBJ_FROM_PTR(self->scan_results);
 }
@@ -309,15 +333,16 @@ static int _mtu_reply(uint16_t conn_handle,
     if (error->status == 0) {
         connection->mtu = mtu;
     }
-    xTaskNotify(cp_task, conn_handle, eSetValueWithOverwrite);
+    // Set status var to connection handle to report that connection is now established.
+    // Another routine is waiting for this.
+    _connection_status = conn_handle;
     return 0;
 }
 
 static void _new_connection(uint16_t conn_handle) {
     // Set the tx_power for the connection higher than the advertisement.
     esp_ble_tx_power_set(conn_handle, ESP_PWR_LVL_N0);
 
-
     // Find an empty connection. One should always be available because the SD has the same
     // total connection limit.
     bleio_connection_internal_t *connection = NULL;
@@ -353,13 +378,14 @@ static int _connect_event(struct ble_gap_event *event, void *self_in) {
     switch (event->type) {
         case BLE_GAP_EVENT_CONNECT:
             if (event->connect.status == 0) {
-                // This triggers an MTU exchange. Its reply will unblock CP.
+                // This triggers an MTU exchange. Its reply will exit the loop waiting for a connection.
                 _new_connection(event->connect.conn_handle);
                 // Set connections objs back to NULL since we have a new
                 // connection and need a new tuple.
                 self->connection_objs = NULL;
             } else {
-                xTaskNotify(cp_task, -event->connect.status, eSetValueWithOverwrite);
+                // The loop waiting for the connection to be comnpleted will stop when _connection_status changes.
+                _connection_status = -event->connect.status;
             }
             break;
 
@@ -397,24 +423,31 @@ mp_obj_t common_hal_bleio_adapter_connect(bleio_adapter_obj_t *self, bleio_addre
     ble_addr_t addr;
     _convert_address(address, &addr);
 
-    cp_task = xTaskGetCurrentTaskHandle();
-    // Make sure we don't have a pending notification from a previous time. This
-    // can happen if a previous wait timed out before the notification was given.
-    xTaskNotifyStateClear(cp_task);
+    const int timeout_ms = SEC_TO_UNITS(timeout, UNIT_1_MS) + 0.5f;
     CHECK_NIMBLE_ERROR(
         ble_gap_connect(own_addr_type, &addr,
-            SEC_TO_UNITS(timeout, UNIT_1_MS) + 0.5f,
+            timeout_ms,
             &conn_params,
             _connect_event, self));
 
-    int error_code;
     // Wait an extra 50 ms to give the connect method the opportunity to time out.
-    CHECK_NOTIFY(xTaskNotifyWait(0, 0, (uint32_t *)&error_code, pdMS_TO_TICKS(timeout * 1000 + 50)));
+
+    const uint64_t timeout_time_ms = common_hal_time_monotonic_ms() + timeout_ms;
+    // _connection_status gets set to either a positive connection handle or a negative error code.
+    _connection_status = BLEIO_HANDLE_INVALID;
+    while (_connection_status == BLEIO_HANDLE_INVALID && (common_hal_time_monotonic_ms() < timeout_time_ms)) {
+        RUN_BACKGROUND_TASKS;
+        if (mp_hal_is_interrupted()) {
+            // Return prematurely. Then the interrupt exception  will be raised.
+            return mp_const_none;
+        }
+    }
+
     // Negative values are error codes, connection handle otherwise.
-    if (error_code < 0) {
-        CHECK_BLE_ERROR(-error_code);
+    if (_connection_status < 0) {
+        CHECK_BLE_ERROR(-_connection_status);
     }
-    uint16_t conn_handle = error_code;
+    const uint16_t conn_handle = _connection_status;
 
     // TODO: If we have keys, then try and encrypt the connection.
 

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

@@ -7,20 +7,47 @@
 #include <string.h>
 
 #include "py/runtime.h"
+#include "shared/runtime/interrupt_char.h"
 
 #include "shared-bindings/_bleio/__init__.h"
 #include "shared-bindings/_bleio/Characteristic.h"
 #include "shared-bindings/_bleio/CharacteristicBuffer.h"
 #include "shared-bindings/_bleio/Descriptor.h"
 #include "shared-bindings/_bleio/PacketBuffer.h"
 #include "shared-bindings/_bleio/Service.h"
+#include "shared-bindings/time/__init__.h"
 
 #include "common-hal/_bleio/Adapter.h"
 #include "common-hal/_bleio/Service.h"
 
-
 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,
@@ -34,7 +61,7 @@ void common_hal_bleio_characteristic_construct(bleio_characteristic_obj_t *self,
     self->props = props;
     self->read_perm = read_perm;
     self->write_perm = write_perm;
-    self->observer = NULL;
+    self->observer = mp_const_none;
 
     // Map CP's property values to Nimble's flag values.
     self->flags = 0;
@@ -125,7 +152,6 @@ bleio_service_obj_t *common_hal_bleio_characteristic_get_service(bleio_character
 }
 
 typedef struct {
-    TaskHandle_t task;
     uint8_t *buf;
     uint16_t len;
 } _read_info_t;
@@ -148,9 +174,9 @@ static int _read_cb(uint16_t conn_handle,
             // For debugging.
             mp_printf(&mp_plat_print, "Read status: %d\n", error->status);
             #endif
-            xTaskNotify(read_info->task, error->status, eSetValueWithOverwrite);
             break;
     }
+    _completion_status = error->status;
 
     return 0;
 }
@@ -163,14 +189,12 @@ 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 = {
-            .task = xTaskGetCurrentTaskHandle(),
             .buf = buf,
             .len = len
         };
+        _reset_completion_status();
         CHECK_NIMBLE_ERROR(ble_gattc_read(conn_handle, self->handle, _read_cb, &read_info));
-        int error_code;
-        xTaskNotifyWait(0, 0, (uint32_t *)&error_code, 200);
-        CHECK_BLE_ERROR(error_code);
+        CHECK_NIMBLE_ERROR(_wait_for_completion(2000));
         return read_info.len;
     } else {
         len = MIN(self->current_value_len, len);
@@ -189,8 +213,7 @@ static int _write_cb(uint16_t conn_handle,
     const struct ble_gatt_error *error,
     struct ble_gatt_attr *attr,
     void *arg) {
-    TaskHandle_t task = (TaskHandle_t)arg;
-    xTaskNotify(task, error->status, eSetValueWithOverwrite);
+    _completion_status = error->status;
 
     return 0;
 }
@@ -201,10 +224,9 @@ void common_hal_bleio_characteristic_set_value(bleio_characteristic_obj_t *self,
         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 {
-            CHECK_NIMBLE_ERROR(ble_gattc_write_flat(conn_handle, self->handle, bufinfo->buf, bufinfo->len, _write_cb, xTaskGetCurrentTaskHandle()));
-            int error_code;
-            xTaskNotifyWait(0, 0, (uint32_t *)&error_code, 200);
-            CHECK_BLE_ERROR(error_code);
+            _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));
         }
     } else {
         // Validate data length for local characteristics only.
@@ -338,6 +360,10 @@ bleio_characteristic_properties_t common_hal_bleio_characteristic_get_properties
 void common_hal_bleio_characteristic_add_descriptor(bleio_characteristic_obj_t *self,
     bleio_descriptor_obj_t *descriptor) {
     size_t i = self->descriptor_list->len;
+    if (i >= MAX_DESCRIPTORS) {
+        mp_raise_bleio_BluetoothError(MP_ERROR_TEXT("Too many descriptors"));
+    }
+
     mp_obj_list_append(MP_OBJ_FROM_PTR(self->descriptor_list),
         MP_OBJ_FROM_PTR(descriptor));
 
@@ -368,10 +394,9 @@ void common_hal_bleio_characteristic_set_cccd(bleio_characteristic_obj_t *self,
         (notify ? 1 << 0 : 0) |
         (indicate ? 1 << 1: 0);
 
-    CHECK_NIMBLE_ERROR(ble_gattc_write_flat(conn_handle, self->cccd_handle, &cccd_value, 2, _write_cb, xTaskGetCurrentTaskHandle()));
-    int error_code;
-    xTaskNotifyWait(0, 0, (uint32_t *)&error_code, 200);
-    CHECK_BLE_ERROR(error_code);
+    _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));
 }
 
 void bleio_characteristic_set_observer(bleio_characteristic_obj_t *self, mp_obj_t observer) {

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

@@ -70,7 +70,6 @@ uint32_t common_hal_bleio_characteristic_buffer_read(bleio_characteristic_buffer
     }
 
     uint32_t num_bytes_read = ringbuf_get_n(&self->ringbuf, data, len);
-
     return num_bytes_read;
 }