corneliusmunz / legoino

Arduino Library for controlling Powered UP and Boost controllers
MIT License
257 stars 34 forks source link

Feature Request: Non-blocking init #41

Closed bjgz closed 3 years ago

bjgz commented 3 years ago

Thank you for writing a great library, I’m really enjoying exploring its potential.

One thing that seems out of place however is the blocking init call. When you are connecting to multiple hubs, and trying to control a complex environment, even one second can be too long for comms and the program to pause.

I notice the NimBLE library now supports a non-blocking scan function, is there any chance this can be brought across to Legoino?

Mattze0815 commented 3 years ago

+1

corneliusmunz commented 3 years ago

@bjgz Thanks for opening that issue! I will have a look on the changes in the NimBLE library about non-blocking scan function. Can you give me a hint where the changes are described?

fvanroie commented 3 years ago

What I gathered from the docs:

In this example we will scan for 10 seconds. This is a blocking function (a non blocking overload is also available).

#include "NimBLEDevice.h"

// void setup() in Arduino
void app_main(void)  
{
    NimBLEDevice::init("");

    NimBLEScan *pScan = NimBLEDevice::getScan();
    NimBLEScanResults results = pScan->start(10);
}

NimBLEScan::start indeed has a non-blocking overload, that accepts a callback function.

Non-blocking:

/**
 * @brief Start scanning.
 */
bool NimBLEScan::start(uint32_t duration, void (*scanCompleteCB)(NimBLEScanResults), bool is_continue)

vs. blocking:

/**
 * @brief Start scanning and block until scanning has been completed.
 */
NimBLEScanResults NimBLEScan::start(uint32_t duration, bool is_continue)

It seems to me that all that is needed is to add an additional parameter to pBLEScan->start in Lpf2Hub::Init()

void Lpf2Hub::init()
{
    ...
    pBLEScan->start(_scanDuration, scanEndedCB);
}

An example for scanEndedCBcan be found here.

I added that callback parameter to Lpf2Hub.cpp and the scan was then non-blocking. Not sure about what other things are needed in the library, but I hope that puts you on the right track.

corneliusmunz commented 3 years ago

@fvanroie thanks so much for your detailed description and for the guidance in that direction. I will try it out in the next days

corneliusmunz commented 3 years ago

@fvanroie I have tested it too and it works without blocking if adding a callback to the start method. I have tried it even with a nullptr as callback and it works.

    pBLEScan->start(_scanDuration, nullptr);

We don't need the callback with the NimBLEScanResults so even the nullptr should be fine. What do you think?

corneliusmunz commented 3 years ago

Fixed with commit https://github.com/corneliusmunz/legoino/commit/56847d63bcbe010480c6b37a84ccdcc8a9d538aa and published with release https://github.com/corneliusmunz/legoino/releases/tag/1.0.4

fvanroie commented 3 years ago

Great, I didn't even consider passing a nullptr but they seem to handle that case as well. So it's even easier then I thought! While you can just use it like this, I think the actual implementation goes together with issue: Make connection flow easier #38.

I currently have a project which handles 9 hubs that connect, disconnect and reconnect at random times. I used tasks to get around the blocking init() function: all other tasks keep running even if one is blocked scanning. Using non-blocking scans only solves half the problem, which is that only 1 scan be active at the same time. So, you need a semaphore to manage which task can scan at any given time.

Even with a non-blocking scan, you can't just do:

hub1.init(10);
hub2.init(10);
hub3.init(10);

you get an error

E NimBLEScan: "Scan already in progress"
E NimBLEScan: "Scan already in progress"

There needs to be more logic behind it to manage the token, etc... hence issue #38 might be a good place to add some wrapper around that management code. Ironically having a non-blocking scan, might complicates the flow quite a bit.

corneliusmunz commented 3 years ago

@fvanroie You are right. Non blocking is fine but also evil 😉

I think of a HubManager class who will initiate the scan process by itself with his own callback. Then the HubManager can use the Lpf2Hub::connectHub method to connect to individual hubs which are added to the HubManager. Maybe the HubManger can even do a automatic hub discovery and add each available lego hub automatically.

I think in a multi Hub environment it is better that a instance above each hub initiates the scan process and only the connection should be handled by the hub itself. then we can do one scan and do multiple connects almost parallel. This could be done in the void onResult(NimBLEAdvertisedDevice *advertisedDevice) callback or after a defined scan period in the scanEndedCallback where a list of found Devices are available.

So i see two scenarios

Discovery mode

1) Start the HubManager 2) HubManager will scan for a defined period (maybe 5-10s) 3) All lego hubs which will found in that duration will be added as objects to the HubManager and could be accessed by Name or Address 4) HubManager would be handle the reconnect logic if a hub is disconnected

Defined system mode

1) Add a "system" structure defined by adding manually hubs defined by name, address or type to the HubManager 2) Start the HubManager 3) Hub manager will scan as long as all defined hubs are found. If a defined hub is found the hub will be directly start the connect procedure 4) HubManager would be handle the reconnect logic if a hub is disconnected

What do you think about that idea?

fvanroie commented 3 years ago

I like the idea of a HubManager class! I'm not a fan of a Discovery mode, it sounds it would be blocking other code execution. Idealy scan and system mode could run concurrently:

Thinking out loud: