Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BLE: alternately broadcast user's and Particle-specific advertising d… #1882

Merged
merged 8 commits into from Aug 26, 2019

Conversation

@XuGuohui
Copy link
Contributor

commented Aug 9, 2019

Problem

The advertising data and scan response data set by user application will not be broadcasting when device in the Listening mode. What that means is, for custom mobile App it would not be able to filter their device by the advertising data set in user application when device in the Listening mode.

Solution

Alternately broadcast user's and Particle-specific advertising data when device in the Listening mode. By this way, both of the advertising data will be broadcasting and observed by Particle mobile App and custom mobile App. The only defect is that when device in the listening mode, the advertising and connection parameters set in user application are adjusted to appropriate values to coordinate the Particle mobile setup. But once device exits the Listening mode, these parameters are restored for user application.

Effective advertising parameters in the Listening mode:

|<--------------->|<--------------->|<--------------->|<--------------->|
User ADV          Particle ADV      User ADV          Particle ADV 

+----------+---------+
| Interval | Timeout |
+----------+---------+
| 20ms     | 1000ms  |
+----------+---------+

Effective connection parameters in the Listening mode:

+--------------+--------------+---------+---------+
| MIN Interval | MAX Interval | Latency | Timeout |
+--------------+--------------+---------+---------+
| 30ms         | 50ms         | 0       | 5000ms  |
+--------------+--------------+---------+---------+

Steps to Test

Build and flash the monolithic firmware with log enabled:
$ make MODULAR=n DEBUG_BUILD=y PLATFORM=xenon TEST=<path_to_the_example>
$ dfu-util -d 2b04:d00e -a 0 -s 0x30000:leave -D <path_to_the_built_binary>

  1. Device is neither broadcasting nor connected before entering the Listening mode:

    • Check the log message when device in the Listening mode, it should be only broadcasting the Particle-specific advertising data.
    • It should be able to be connected by the Particle mobile App.
    • Make the device leave the listening mode and disconnect from mobile App, device shouldn't be broadcasting.
  2. Device is broadcasting before entering the Listening mode:

    • Check the log message when device in the Listening mode, it should be alternately broadcasting user's and Particle-specific advertising data in every 500ms interval.
    • It should be able to be connected by the Particle mobile App.
    • Make the device leave the listening mode and disconnect from mobile App, device should be only broadcasting the user's advertising data with the advertising parameters set in user application.
  3. Device is connected before entering the Listening mode:

    • Check the log message when device in the Listening mode, it shouldn't be broadcasting.
    • Disconnect device when device in the listening mode, it should be alternately broadcasting user's and Particle-specific advertising data in every 500ms interval.
    • It should be able to be connected by the Particle mobile App.
    • Make the device leave the listening mode and disconnect from mobile App, device should be only broadcasting the user's advertising data with the advertising parameters set in user application.

Example App

#include "Particle.h"

SYSTEM_MODE(MANUAL);

Serial1LogHandler log(115200, LOG_LEVEL_ALL);

const char* serviceUuid = "6E400001-B5A3-F393-E0A9-E50E24DCCA9E";

void setup() {
    BLE.setAdvertisingTimeout(1000); // 1000 * 10 ms

    BleAdvertisingData data;
    data.appendServiceUUID(serviceUuid);
    BLE.advertise(&data);
}

void loop() {
}
// TODO: Would it be better to call the method of BLE Control Request Channel to isolate the logic?
// Cache the application specific advertising data, advertising parameters and connection parameters.
int BleListeningModeHandler::constructControlRequestAdvData() {
ctrlReqAdvData_.reset(new(std::nothrow) uint8_t[BLE_MAX_ADV_DATA_LEN]);

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Aug 12, 2019

Member

Let's use the following pattern:

{
    auto ctrlReqAdvData = std::make_unique<uint8_t[]>(BLE_MAX_ADV_DATA_LEN);
    CHECK_TRUE(ctrlReqAdvData);
    // ...
    auto ctrlReqSrData = std::make_unique<uint8_t[]>(BLE_MAX_ADV_DATA_LEN);
    CHECK_TRUE(ctrlReqSrData);
    // ...
    // A bunch of other stuff that may return from the function prematurely e.g. due to an error
    // ...
    ctrlReqAdvData_.swap(ctrlReqAdvData);
    ctrlReqSrData_.swap(ctrlReqSrData);
    return SYSTEM_ERROR_NONE;
}

Same thing in cacheUserConfigurations().

This comment has been minimized.

Copy link
@XuGuohui

XuGuohui Aug 13, 2019

Author Contributor

If we're going to use Vector instead, this should be different.

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Aug 13, 2019

Member

Same pattern applies to the Vector. The following can also be used:

ctrlReqAdvData_ = std::move(ctrlReqAdvData);

What I'm trying to suggest here is to fill in the member variables only at the end of the function.

This comment has been minimized.

Copy link
@XuGuohui

XuGuohui Aug 13, 2019

Author Contributor

I see. In that case memory allocated in this function will be freed automatically if error occurs before copying the constructed data or cached data and the vector will be empty then. 👍

int BleListeningModeHandler::cacheUserConfigurations() {
LOG_DEBUG(TRACE, "Cache user's BLE configurations.");

// Advertising data set by user application

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Aug 12, 2019

Member

We should acquire the BLE HAL lock for the whole duration of this function, in order to get a snapshot of the application BLE configuration without giving it a chance to change it in-between.

Also, how do we guard the application from changing the configuration while its running concurrently with the listening mode?

This comment has been minimized.

Copy link
@XuGuohui

XuGuohui Aug 13, 2019

Author Contributor

How about introducing HAL API hal_ble_notify_enter_listening_mode() and hal_ble_notify_exit_listening_mode()? And if so, these two APIs will not be exported as part of dynalib.

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Aug 13, 2019

Member

What would they do?

This comment has been minimized.

Copy link
@XuGuohui

XuGuohui Aug 13, 2019

Author Contributor

They will set/clear a flag in BLE HAL to indicate whether the advertising data, advertising parameters, connection parameters and other configurations are allowed to be changed during the mobile setup. Besides, we can also make use of this flag to handle some conflicts between the Listening mode and user application, such like BLE.off() being called in user application will then remain the peripheral connection if device is in the Listening mode.

This comment has been minimized.

Copy link
@XuGuohui

XuGuohui Aug 13, 2019

Author Contributor

And I think with that HAL APIs implemented, it's unnecessary to acquire/release the BLE HAL lock in this function, since the configurations are guarded by these APIs as well. No?

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Aug 13, 2019

Member

As long as there is some mechanism in place that makes the current application configuration non-modifiable, yes.

Let's also make sure that we don't forget to call hal_ble_notify_exit_listening_mode() in case something goes wrong (e.g. memory allocation fails etc).

@@ -45,6 +63,12 @@ class BleListeningModeHandler {
bool preAdvertising_;
bool preConnected_;
hal_ble_auto_adv_cfg_t preAutoAdv_;
bool userAdv_;

std::unique_ptr<uint8_t[]> ctrlReqAdvData_;

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Aug 12, 2019

Member

We could potentially used a Vector instead, since it's exactly a pointer + size.

This comment has been minimized.

Copy link
@XuGuohui

XuGuohui Aug 13, 2019

Author Contributor

Sounds good! 👍

@avtolstoy avtolstoy removed the needs review label Aug 12, 2019


int BleListeningModeHandler::exit() {
CHECK(restoreUserConfigurations());
CHECK(hal_ble_cancel_callback_on_adv_events(onBleAdvEvents, this, nullptr));

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Aug 12, 2019

Member

preAdvData_, preSrData_, ctrlReqAdvData_ and ctrlReqSrData_ need to be deallocated here.

This comment has been minimized.

Copy link
@XuGuohui

XuGuohui Aug 13, 2019

Author Contributor

In case that we were using Vector, the vector should be cleared on exiting listening mode as well.

@avtolstoy avtolstoy added this to the 1.4.0 milestone Aug 12, 2019

@XuGuohui XuGuohui requested a review from avtolstoy Aug 14, 2019

@XuGuohui

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

Merge #1890

@XuGuohui XuGuohui added needs review and removed do not merge labels Aug 22, 2019

system/src/ble_listening_mode_handler.h Show resolved Hide resolved
*
* @returns 0 on success, system_error_t on error.
*/
int hal_ble_notify_enter_listening_mode(void* reserved);

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Aug 23, 2019

Member

Let's perhaps rename this to something more generic. HAL ideally shouldn't be aware of high-level concepts like listening mode. Something generic like hal_ble_enter_locked_mode() (not ideal, but naming things is difficult) sounds better IMO.

CHECK_TRUE(preAdvData_, SYSTEM_ERROR_NO_MEMORY);
hal_ble_gap_get_advertising_data(preAdvData_.get(), preAdvDataLen_, nullptr);
}
size_t len = hal_ble_gap_get_advertising_data(tempAdvData.data(), BLE_MAX_ADV_DATA_LEN, nullptr);

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Aug 23, 2019

Member

ssize_t or even better:

size_t len = CHECK(hal_ble_gap_get_advertising_data(tempAdvData.data(), BLE_MAX_ADV_DATA_LEN, nullptr));

This comment has been minimized.

@@ -124,6 +124,9 @@ int BleListeningModeHandler::restoreUserConfigurations() {

// Do not allow other thread to modify the BLE configurations.
CHECK(hal_ble_lock(nullptr));
SCOPE_GUARD ({

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Aug 23, 2019

Member

Let's instead use BleLock class to utilize RAII.

hal_ble_cancel_callback_on_adv_events(onBleAdvEvents, this, nullptr);
hal_ble_unlock(nullptr);

exited_ = true;

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Aug 23, 2019

Member

Let's set it before unlocking the BLE lock.

This comment has been minimized.

Copy link
@XuGuohui

XuGuohui Aug 23, 2019

Author Contributor

If we used BleLock, it is always set before unlocking the BLE lock.

@avtolstoy avtolstoy modified the milestones: 1.4.0, 1.4.0-rc.1 Aug 23, 2019

@XuGuohui XuGuohui force-pushed the refactor/ble_listening_mode branch from 2dabf41 to a55192a Aug 23, 2019

@XuGuohui XuGuohui requested a review from avtolstoy Aug 23, 2019

@XuGuohui XuGuohui force-pushed the refactor/ble_listening_mode branch from 8873abf to 039c1d2 Aug 26, 2019

@XuGuohui XuGuohui merged commit c422fcd into develop Aug 26, 2019

1 of 2 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@XuGuohui XuGuohui deleted the refactor/ble_listening_mode branch Aug 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.