BLE: Introduce ChainableEventHandler and subclasses #13734

AGlass0fMilk · 2020-10-07T23:32:48Z

Summary of changes

~~Requires #13727 to be merged first~~
Merged #13727 since this PR is closely tied to it.

Introduces a ChainableEventHandler base class that is essentially a singly-linked-list of EventHandlers along with two subclasses:

ChainableGattServerEventHandler that enables chaining together GattServer::EventHandlers
ChainableGapEventHandler that enables chaining together Gap::EventHandlerss

The ChainableGattServerEventHandler class allows an application to register separate event handlers (eg: for different services that need to handle GattServer events) and then set the global GattServer::setEventHandler to the instance of ChainableGattServerEventHandler with all registered GattServer::EventHandlers.

The ChainableGapEventHandler accomplishes that same as above for Gap:EventHandler implementations.

Common functionality has been split off into ChainableEventHandler.

See #13728 for discussion around this implementation.

Impact of changes

None

Migration actions required

None

Documentation

None

Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[x] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@pan- @paul-szczepanek-arm The code compiles but my template-class-function-pointer kung fu isn't flawless. Please look over my implementation and point out anything that looks suspect.

I will test this with an example in the near future.

ciarmcom · 2020-10-08T00:00:30Z

@AGlass0fMilk, thank you for your changes.
@paul-szczepanek-arm @pan- @ARMmbed/mbed-os-maintainers please review.

paul-szczepanek-arm · 2020-10-08T07:52:47Z

That's very nice, we should add a mention in the gattserver class that this exists otherwise people might not even notice. It should be mentioned in the setEventHandler call.

connectivity/FEATURE_BLE/include/ble/gatt/ChainableGattServerEventHandler.h

AGlass0fMilk · 2020-10-08T20:04:10Z

@pan-

I ran into issues compiling the ChainableGapEventHandler due to the cv-ref qualified-ness of those handlers... I introduced a variant of execute_on_all in d6af9e1 that works for now but I think we will need some of @kjbracey-arm 's magic from Callback to remove cv-qualifiers and handle any kind of functions added to event handlers in the future...

eg:

mbed-os/connectivity/FEATURE_BLE/include/ble/Gap.h

Lines 303 to 331 in ad40b1b

    
               struct EventHandler { 
        
                   /** 
        
                    * Called when an advertising device receive a scan response. 
        
                    * 
        
                    * @param event Scan request event. 
        
                    * 
        
                    * @version: 5+. 
        
                    * 
        
                    * @see AdvertisingParameters::setScanRequestNotification(). 
        
                    */ 
        
                   virtual void onScanRequestReceived(const ScanRequestEvent &event) 
        
                   { 
        
                   } 
        
                   /** 
        
                    * Called when advertising ends. 
        
                    * 
        
                    * Advertising ends when the process timeout or if it is stopped by the 
        
                    * application or if the local device accepts a connection request. 
        
                    * 
        
                    * @param event Advertising end event. 
        
                    * 
        
                    * @see startAdvertising() 
        
                    * @see stopAdvertising() 
        
                    * @see onConnectionComplete() 
        
                    */ 
        
                   virtual void onAdvertisingEnd(const AdvertisingEndEvent &event) 
        
                   { 
        
                   }

pan- · 2020-10-08T20:27:15Z

@AGlass0fMilk Did you try to use forwarding references (&& in template context) ? It might be good to use std::forward to pass the arguments to the handler too.

connectivity/FEATURE_BLE/include/ble/common/ChainableEventHandler.h

AGlass0fMilk · 2020-10-09T13:39:15Z

@pan- That change seems to work well! Thanks for the suggestion!

pan-

Great change, I left few comments for improvment.

connectivity/FEATURE_BLE/include/ble/common/ChainableEventHandler.h

connectivity/FEATURE_BLE/include/ble/gap/ChainableGapEventHandler.h

Pull request has been modified.

pan- · 2020-10-09T15:43:00Z

connectivity/FEATURE_BLE/include/ble/gap/ChainableGapEventHandler.h

@@ -83,6 +83,7 @@ class ChainableGapEventHandler : public ble::Gap::EventHandler,
    }

    void onDisconnectionComplete(const ble::DisconnectionCompleteEvent &event) override {
+       execute_on_all(&ble::Gap::EventHandler::onDisconnectionComplete, event);


Thanks, I forgot to comment for that one 😅 .

pan-

LGTM

AGlass0fMilk · 2020-10-09T15:49:12Z

@pan- Do you have any good resources where I can learn why your execute_on_all change was needed (and how it works), more about std::forward and related concepts? I need to improve my understanding of lvalues/rvalues/etc.

Any search terms I could use would be appreciated 😄

Currently looking at these:
https://www.artima.com/cppsource/rvalue.html
http://thbecker.net/articles/rvalue_references/section_02.html
https://accu.org/journals/overload/12/61/kilpelainen_227/

Pull request has been modified.

AGlass0fMilk · 2020-10-09T16:01:01Z

@pan- I decided to merge #13727 into this one since the additional GattServer::EventHandler functions were referenced. It started to get messy to maintain them separately.

Let me know if this is OK.

pan- · 2020-10-09T16:29:14Z

@AGlass0fMilk

Why it didn't work: A template function deduces the template types based on the parameter passed in input.

template<typename T>
void foo(T v);

foo('a'); // T is a char
foo(0); // T is an int

In our case, the arguments passed to the function execute_on_all doesn't always match the arguments specified in the handle and that's where the compiler get confused.

template<typename... Args>
void execute_on_all(void (*fn)(Args...), Args... args);

void foo(const int&);
execute_on_all(foo, 0); // Error, Args is deduced as const int& from the function and int from the second parameter.


// using forwarding references
template<typename... Args>
void execute_on_all(void (*fn)(Args...), Args&&... args); 

 void foo(const int&);
execute_on_all(foo, 0); // Error, Args is deduced as const int& from the function and int& from the second parameter.

Having two variadic arguments helps the compiler at recognizing the handler type and the parameter type. If they are not compatible with one another the compiler will complain at the call site.

template<typename... FnArgs, typename... Args>
void execute_on_all(void (*fn)(FnArgs...), Args&&... args);

One of the most confusing thing about lvalue and rvalue reference to me was the usage of the && token that means different things depending on the context:

void foo(int&& a); // non template context: a is an lvalue reference 

template<typename T>
void foo(T&& a); // template context: a is a forwarding reference, it is an lvalue reference or rvalue reference depending on what is passed in the function.

I recommend reading the C++ proposal papers, they capture very well the reason why these constructs where introduced in the language by detailing the problem they solve:

AGlass0fMilk · 2020-10-09T17:47:03Z

@pan- Thanks for the detailed explanation!

Let me know if this PR needs any more work.

pan-

LGTM

Pull request has been modified.

AGlass0fMilk · 2020-10-13T20:08:05Z

@0xc0170 Glad to have you back 😄

I have rebased with master and cleaned up the history of this PR.

Let me know if it's good to go.

0xc0170 · 2020-10-14T05:48:05Z

@AGlass0fMilk the history looks good but feature should not be one liner in the commit message. There are details in this PR that could be also in the commits, please add them there.

…lasses. Common functionality has been split off into a generic ChainableEventHandler for use by other EventHandler implementations. The ChainableEventHandler is essentially singly-linked list that propagates callbacks to all objects in the list. The ChainableGattServerEventHandler enables chaining together GattServer::EventHandlers. An application can register separate event handlers (eg: for different services that need to handle GattServer events) and then set the global GattServer::setEventHandler to the instance of ChainableGattServerEventHandler with all registered GattServer::EventHandlers.

The ChainableGapEventHandler enables chaining together Gap::EventHandlers so separate parts of an application can be notified of Gap events as needed. The application can register multiple Gap::EventHandler objects to one ChainableGapEventHandler and then set the global Gap::EventHandler to the ChainableGapEventHandler. All registered EventHandlers will then be called when a Gap Event occurs.

Update parameters passed to onDataSent, onUpdatesEnabled/Disabled, and onConfirmationReceived callbacks. Deprecate single-callback-registering functions for event handling in lieu of the new EventHandler-based API. Introduce new GattServer::EventHandler callback functions to replace the deprecated versions.

AGlass0fMilk · 2020-10-14T16:51:10Z

@0xc0170 I have updated the commit messages. Let me know if they are OK.

0xc0170 · 2020-10-16T08:42:28Z

CI started

mbed-ci · 2020-10-16T09:54:08Z

Jenkins CI Test : ❌ FAILED

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_build-ARM	✔️
jenkins-ci/mbed-os-ci_build-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_dynamic-memory-usage	✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest	❌
jenkins-ci/mbed-os-ci_greentea-test	✔️

0xc0170 · 2020-10-17T09:52:03Z

CI restarted

mbed-ci · 2020-10-17T11:41:19Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 3 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_build-ARM	✔️
jenkins-ci/mbed-os-ci_build-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_dynamic-memory-usage	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest	✔️

ciarmcom added the release-type: feature label Oct 8, 2020

ciarmcom requested review from pan-, paul-szczepanek-arm and a team October 8, 2020 00:00

ciarmcom added the needs: review label Oct 8, 2020

paul-szczepanek-arm reviewed Oct 8, 2020

View reviewed changes

connectivity/FEATURE_BLE/include/ble/gatt/ChainableGattServerEventHandler.h Outdated Show resolved Hide resolved

paul-szczepanek-arm reviewed Oct 8, 2020

View reviewed changes

connectivity/FEATURE_BLE/include/ble/gatt/ChainableGattServerEventHandler.h Outdated Show resolved Hide resolved

AGlass0fMilk changed the title ~~[BLE] Introduce ChainableGattServerEventHandler subclass~~ [BLE] Introduce ChainableEventHandler and subclasses Oct 8, 2020

pan- reviewed Oct 9, 2020

View reviewed changes

connectivity/FEATURE_BLE/include/ble/common/ChainableEventHandler.h Outdated Show resolved Hide resolved

pan- previously requested changes Oct 9, 2020

View reviewed changes

mergify bot added needs: work and removed needs: review labels Oct 9, 2020

AGlass0fMilk force-pushed the chainable-event-handler branch from 700dcc0 to 929ba9c Compare October 9, 2020 15:41

pan- reviewed Oct 9, 2020

View reviewed changes

pan- previously approved these changes Oct 9, 2020

View reviewed changes

AGlass0fMilk mentioned this pull request Oct 9, 2020

[BLE] Update parameters passed to onDataSent, onUpdatesEnabled/Disabled, and onConfirmationReceived callbacks #13727

Closed

AGlass0fMilk force-pushed the chainable-event-handler branch from 6240874 to 10f64b3 Compare October 9, 2020 16:05

pan- previously approved these changes Oct 12, 2020

View reviewed changes

mergify bot added the needs: CI label Oct 12, 2020

AGlass0fMilk force-pushed the chainable-event-handler branch from 41a24c3 to aad3c4b Compare October 14, 2020 16:40

AGlass0fMilk added 3 commits October 14, 2020 12:42

AGlass0fMilk force-pushed the chainable-event-handler branch from aad3c4b to 9a6d207 Compare October 14, 2020 16:45

0xc0170 added needs: CI and removed needs: review labels Oct 16, 2020

mergify bot added needs: work and removed needs: CI labels Oct 16, 2020

0xc0170 added needs: CI and removed needs: work labels Oct 17, 2020

0xc0170 approved these changes Oct 19, 2020

View reviewed changes

0xc0170 added ready for merge and removed needs: CI labels Oct 19, 2020

0xc0170 merged commit b242be1 into ARMmbed:master Oct 19, 2020

mergify bot removed the ready for merge label Oct 19, 2020

mbedmain added release-version: 6.4.0 Release-pending labels Oct 20, 2020

This was referenced Oct 21, 2020

BLE: Data sent event parameter is useless information #13726

Closed

BLE: It is no longer possible to send out data Fast! #12602

Closed

mbedmain removed release-type: feature Release-pending labels Oct 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BLE: Introduce ChainableEventHandler and subclasses #13734

BLE: Introduce ChainableEventHandler and subclasses #13734

AGlass0fMilk commented Oct 7, 2020 •

edited

Loading

ciarmcom commented Oct 8, 2020

paul-szczepanek-arm commented Oct 8, 2020

AGlass0fMilk commented Oct 8, 2020 •

edited

Loading

pan- commented Oct 8, 2020 •

edited

Loading

AGlass0fMilk commented Oct 9, 2020

pan- left a comment

pan- Oct 9, 2020

pan- left a comment

AGlass0fMilk commented Oct 9, 2020

AGlass0fMilk commented Oct 9, 2020

pan- commented Oct 9, 2020

AGlass0fMilk commented Oct 9, 2020

pan- left a comment

AGlass0fMilk commented Oct 13, 2020

0xc0170 commented Oct 14, 2020

AGlass0fMilk commented Oct 14, 2020

0xc0170 commented Oct 16, 2020

mbed-ci commented Oct 16, 2020

0xc0170 commented Oct 17, 2020

mbed-ci commented Oct 17, 2020

BLE: Introduce ChainableEventHandler and subclasses #13734

BLE: Introduce ChainableEventHandler and subclasses #13734

Conversation

AGlass0fMilk commented Oct 7, 2020 • edited Loading

Summary of changes

Impact of changes

Migration actions required

Documentation

None

Pull request type

Test results

Reviewers

ciarmcom commented Oct 8, 2020

paul-szczepanek-arm commented Oct 8, 2020

AGlass0fMilk commented Oct 8, 2020 • edited Loading

pan- commented Oct 8, 2020 • edited Loading

AGlass0fMilk commented Oct 9, 2020

pan- left a comment

Choose a reason for hiding this comment

pan- Oct 9, 2020

Choose a reason for hiding this comment

pan- left a comment

Choose a reason for hiding this comment

AGlass0fMilk commented Oct 9, 2020

AGlass0fMilk commented Oct 9, 2020

pan- commented Oct 9, 2020

AGlass0fMilk commented Oct 9, 2020

pan- left a comment

Choose a reason for hiding this comment

AGlass0fMilk commented Oct 13, 2020

0xc0170 commented Oct 14, 2020

AGlass0fMilk commented Oct 14, 2020

0xc0170 commented Oct 16, 2020

mbed-ci commented Oct 16, 2020

Jenkins CI Test : ❌ FAILED

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

0xc0170 commented Oct 17, 2020

mbed-ci commented Oct 17, 2020

Jenkins CI Test : ✔️ SUCCESS

Build Number: 3 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

AGlass0fMilk commented Oct 7, 2020 •

edited

Loading

AGlass0fMilk commented Oct 8, 2020 •

edited

Loading

pan- commented Oct 8, 2020 •

edited

Loading