Relying on event queue seems to be a really bad idea

idea--list commented 3 years ago

Description of defect

Based on the BLE_GattServer_AddService example i wrote a really basic signal generator that streams values to another BLE device after a connection is established. The intention of this was to see how many packets get lost and how reliable is BLE API in general. The nRF Connect (and similar) apps are simply not fit for such evaluations... so i designed an app for scoping the BLE stream with App Inventor.

Major conclusions:

Mbed BLE API can handle up to ca. 30ms connection intervals (~ 30Hz update rate).
If the connection interval is shorter, the BLE signal will begin to fall apart (above 40Hz it becomes extremely apparent).
If you disconnect and reconnect the devices then the data transmission rate doubles after each reconnection (i guess this is because of the nature of event_queue.call_every(). Until now i could not find any means to circumvent that...except for system_reset() which is not a viable option in real world applications)

The last 2 points simply render Mbed BLE API completely unusable for almost anything beyond unit tests on some developers workbench that should only run for some seconds. I mean in any real application chances are high that the BLE connection will get lost and the devices will reconnect...which will cause the BLE signal ripple apart.

I wonder what i might be missing, so any hint is welcome.

Target(s) affected by this defect ?

Artemis Thing Plus

Toolchain(s) (name and version) displaying this defect ?

ARMC 6.15

What version of Mbed-os are you using (tag or sha) ?

Mbed OS V6.9

What version(s) of tools are you using. List all that apply (E.g. mbed-cli)

Mbed Studio V1.4

How is this defect reproduced ?

Code for the BLE signal generator:

/* mbed Microcontroller Library
 * Copyright (c) 2006-2015 ARM Limited
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

#include "ble/BLE.h"
#include "ble/gap/Gap.h"
#include "ble/services/HeartRateService.h"
#include "pretty_printer.h"
#include <cstdint>
#include <events/mbed_events.h>

#define UPDATEINTV 100ms  //defines data update interval

using namespace std::literals::chrono_literals;

uint8_t signalType = 0; // 0 for saw   1 for Zig-Zag   2 for sine wave

// Sine wave values
uint8_t sine256[] = {
    127, 130, 133, 136, 139, 143, 146, 149, 152, 155, 158, 161, 164, 167, 170,
    173, 176, 178, 181, 184, 187, 190, 192, 195, 198, 200, 203, 205, 208, 210,
    212, 215, 217, 219, 221, 223, 225, 227, 229, 231, 233, 234, 236, 238, 239,
    240, 242, 243, 244, 245, 247, 248, 249, 249, 250, 251, 252, 252, 253, 253,
    253, 254, 254, 254, 254, 254, 254, 254, 253, 253, 253, 252, 252, 251, 250,
    249, 249, 248, 247, 245, 244, 243, 242, 240, 239, 238, 236, 234, 233, 231,
    229, 227, 225, 223, 221, 219, 217, 215, 212, 210, 208, 205, 203, 200, 198,
    195, 192, 190, 187, 184, 181, 178, 176, 173, 170, 167, 164, 161, 158, 155,
    152, 149, 146, 143, 139, 136, 133, 130, 127, 124, 121, 118, 115, 111, 108,
    105, 102, 99,  96,  93,  90,  87,  84,  81,  78,  76,  73,  70,  67,  64,
    62,  59,  56,  54,  51,  49,  46,  44,  42,  39,  37,  35,  33,  31,  29,
    27,  25,  23,  21,  20,  18,  16,  15,  14,  12,  11,  10,  9,   7,   6,
    5,   5,   4,   3,   2,   2,   1,   1,   1,   0,   0,   0,   0,   0,   0,
    0,   1,   1,   1,   2,   2,   3,   4,   5,   5,   6,   7,   9,   10,  11,
    12,  14,  15,  16,  18,  20,  21,  23,  25,  27,  29,  31,  33,  35,  37,
    39,  42,  44,  46,  49,  51,  54,  56,  59,  62,  64,  67,  70,  73,  76,
    78,  81,  84,  87,  90,  93,  96,  99,  102, 105, 108, 111, 115, 118, 121,
    124};

bool increase = 1;
int i = 0;

DigitalOut bLed(LED_BLUE, 0);

const static char DEVICE_NAME[] = "SignalStream";

static events::EventQueue event_queue(/* event count */ 16 * EVENTS_EVENT_SIZE);

class HeartrateDemo : ble::Gap::EventHandler {
public:
  HeartrateDemo(BLE &ble, events::EventQueue &event_queue)
      : _ble(ble), _event_queue(event_queue),
        _heartrate_uuid(GattService::UUID_HEART_RATE_SERVICE), _signal_value(0),
        _heartrate_service(ble, _signal_value,
                           HeartRateService::LOCATION_FINGER),
        _adv_data_builder(_adv_buffer) {}

  void start() {
    _ble.init(this, &HeartrateDemo::on_init_complete);

    _event_queue.dispatch_forever();
  }

private:
  /** Callback triggered when the ble initialization process has finished */
  void on_init_complete(BLE::InitializationCompleteCallbackContext *params) {
    if (params->error != BLE_ERROR_NONE) {
      printf("Ble initialization failed.");
      return;
    }

    print_mac_address();

    /* this allows us to receive events like onConnectionComplete() */
    _ble.gap().setEventHandler(this);

    start_advertising();
  }

  void start_advertising() {
    /* Create advertising parameters and payload */

    ble::AdvertisingParameters adv_parameters(
        ble::advertising_type_t::CONNECTABLE_UNDIRECTED,
        ble::adv_interval_t(ble::millisecond_t(500)));

    _adv_data_builder.setFlags();
    _adv_data_builder.setAppearance(
        ble::adv_data_appearance_t::GENERIC_HEART_RATE_SENSOR);
    _adv_data_builder.setLocalServiceList({&_heartrate_uuid, 1});
    _adv_data_builder.setName(DEVICE_NAME);

    /* Setup advertising */
    ble_error_t error = _ble.gap().setAdvertisingParameters(
        ble::LEGACY_ADVERTISING_HANDLE, adv_parameters);

    if (error) {
      printf("_ble.gap().setAdvertisingParameters() failed\r\n");
      return;
    }

    error = _ble.gap().setAdvertisingPayload(
        ble::LEGACY_ADVERTISING_HANDLE, _adv_data_builder.getAdvertisingData());

    if (error) {
      printf("_ble.gap().setAdvertisingPayload() failed\r\n");
      return;
    }

    /* Start advertising */
    error = _ble.gap().startAdvertising(ble::LEGACY_ADVERTISING_HANDLE);

    if (error) {
      printf("_ble.gap().startAdvertising() failed\r\n");
      return;
    }

    printf("Heart rate sensor advertising, please connect\r\n");
  }

  void update_sensor_value() {
    switch (signalType) {
    case 0:
      _heartrate_service.updateHeartRate(_signal_value);
      _signal_value++;
      if (_signal_value > 255) {
        _signal_value = 0;
      }
      break;

    case 1: {
      /// int signalLevel = map(_signal_value, 0, 255, 10, 255);
      _heartrate_service.updateHeartRate(_signal_value);
      if (_signal_value >= 255) {
        increase = 0;
      } else if (_signal_value <= 0) {
        increase = 1;
      }

      if (increase) {
        _signal_value++;
      } else {
        _signal_value--;
      }

    } break;

    case 2:
      _signal_value = sine256[i];

      /// int signalLevel = map(_signal_value, 0, 255, 10, 255);
      _heartrate_service.updateHeartRate(_signal_value);
      i++;
      if (i == 256) {
        i = 0;
      }
      break;
    }
  }

  /* these implement ble::Gap::EventHandler */
private:
  /* when we connect we stop advertising, restart advertising so others can
   * connect */
  virtual void onConnectionComplete(const ble::ConnectionCompleteEvent &event) {
    bLed.write(1);
    if (event.getStatus() == ble_error_t::BLE_ERROR_NONE) {
      printf("Client connected, you may now subscribe to updates\r\n");
      /* now we are connected and can start sending high frequency data */
      _event_queue.call_every(UPDATEINTV, [this] { update_sensor_value(); });
    }
  }

  /* when we disconnect we stop advertising, restart advertising so others can
   * connect */
  virtual void
  onDisconnectionComplete(const ble::DisconnectionCompleteEvent &event) {
    bLed.write(0);
    printf("Client disconnected, restarting advertising\r\n");

    ble_error_t error =
        _ble.gap().startAdvertising(ble::LEGACY_ADVERTISING_HANDLE);

    if (error) {
      printf("_ble.gap().startAdvertising() failed\r\n");
      return;
    }
  }

private:
  BLE &_ble;
  events::EventQueue &_event_queue;

  UUID _heartrate_uuid;
  uint8_t _signal_value;
  HeartRateService _heartrate_service;

  uint8_t _adv_buffer[ble::LEGACY_ADVERTISING_MAX_SIZE];
  ble::AdvertisingDataBuilder _adv_data_builder;
};

/* Schedule processing of events from the BLE middleware in the event queue. */
void schedule_ble_events(BLE::OnEventsToProcessCallbackContext *context) {
  event_queue.call(Callback<void()>(&context->ble, &BLE::processEvents));
}

int main() {
  BLE &ble = BLE::Instance();
  ble.onEventsToProcess(schedule_ble_events);

  HeartrateDemo demo(ble, event_queue);
  demo.start();

  return 0;
}

Import this project into App Inventor and use your phone as a scope.

ciarmcom commented 3 years ago

Thank you for raising this detailed GitHub issue. I am now notifying our internal issue triagers. Internal Jira reference: https://jira.arm.com/browse/IOTOSM-3744

pan- commented 3 years ago

Thanks for sharing @idea--list . I don't think the issue has anything to do with the event queue which should be fast enough to handle any kind of transfer at the highest rate offered by BLE (every 6 ms). In the example posted above, the issue seems to lie in data queuing which is unconditional of the link configuration. By default Android connection interval is set to 50ms. If you push new data every 30ms of course it will saturate the transfer queue.

My recommendation is to split sampling rate from transfer rate: At a regular interval set locally the data you want to send. Then use GattServer::EventHandle::onDataSent to send new data through the BLE link: when it is called, it is safe to send new data. You can use a CircularBuffer If you want to store more than one sample.

paul-szczepanek-arm commented 3 years ago

Store samples and send them in a batch. The frequency of updates doubles after reconnection because you call _event_queue.call_every(UPDATEINTV, [this] { update_sensor_value(); }); a second time (the one from before is still running).

We're happy to help but this is not the place to ask these questions. This repository is for bugs with example code. If you have a general mbed-os bug to report please use the mbed-os repo. If you have programming questions then the forum is the better venue and will gather a wider audience.

ARMmbed / mbed-os-example-ble