[BLE] Change GattServer callbacks to CallChains

[AGlass0fMilk]

Summary of changes

Some of the GattServer callback functions, namely: onUpdatesEnabled, onUpdatesDisabled, and onConfirmationReceived, are implemented as simple function pointers. A subsequent call to any of these functions will override and replace the callback installed by a previous call. This is inconsistent with the precedent of behavior set by other GattServer callback-registering functions (eg: onDataSent).

This PR replaces the simple function pointers underlying each of these callbacks with CallChains, enabling any number of application modules to register handlers for these events.

Requires #13727 to be merged first.

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

@paul-szczepanek-arm @pan-

---

[ciarmcom]

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

[paul-szczepanek-arm]

If we're breaking backwards compatibility I think we should converge on eventHandler model instead of callchains. If you need multiple event handers you can still have that. There's a couple classes that already do.

When we changed the gap API, we kept the callchains, deprecated them and then added the eventHandler to take their place. Maybe we could do that here (I don't actually know if it's feasible in this case).

[AGlass0fMilk]

@paul-szczepanek-arm Could you point me to the code where multiple event handlers are possible?

My own feedback as a user:

After using the EventHandler paradigm, I prefer the flexibility of CallChains. It feels more natural to register a callback to a specific desired event as needed rather than group a bunch of callbacks into EventHandler classes. This works well for discrete classes that implement a distinct Gatt Service -- the Gatt Service can register callbacks to events it is interested in without introducing this class hierarchy stuff. Maybe that's the point (object-oriented after all) but I haven't seen the advantage. Can you point out the motives behind this design choice?

On the same topic, I've found the BLE stack's FunctionPointerWithContext class to be redundant. It accomplishes the same thing as Mbed's Callback. I'm assuming the BLE stack developed separate from Mbed and its Callback class for a while and that's why this redundancy exists.

Is there any reason we don't migrate BLE away from FunctionPointerWithContext to using Callback? It would probably save some flash space since most BLE applications (including the examples) pull in Callback anyway.

I recreated the deprecated (now removed?) CallChain that Mbed used to have internally:

https://github.com/EmbeddedPlanet/ep-oc-mcu/blob/master/extensions/CallChain.h

Perhaps that could be useful...

[AGlass0fMilk]

@pan- What do you think about EventHandler vs CallChains? I looked at the Gap API and the GattServer API and neither implementations support multiple EventHandlers. This forces developers to put all their BLE event handling logic into one place and makes it hard to make reusable services and such.

Multiple event handler objects is a possible solution, but it seems like it would be clunkier than just having CallChains (made of Mbed Callbacks) since Mbed already has a functor concept.

I don't want to start working on moving these PRs over to the EventHandler API until we come to a conclusion.

@ladislas, care to share your thoughts?

[pan-]

Can you point out the motives behind this design choice?

It is all about saving memory. With EventHandler we just need a single pointer to store all the callbacks we want. Whereas with FunctionPointer it is 20 bytes per callback. On a more personal note, I find it easier to use the class based callback paradigms for Gap and SecurityManager as I was defining handlers in my classes and I had to register each of them individually. Now a single registration is enough.
That being said, I see your point for Gatt services and when handlers are not collocated. More granularity is welcomed.
I see several options here:

• Provide an implementation of GattServer::EventHandler that accepts registration of user callbacks (example)
• Provide an implementation of GattServer::EventHandler that acts like a collection of GattServer::EventHandlers.

What do you think ?

Is there any reason we don't migrate BLE away from FunctionPointerWithContext to using Callback? It would probably save some flash space since most BLE applications (including the examples) pull in Callback anyway.

The reasons are backward compatibility and size of the Callback object. FunctionPointerWithContext was here well before Callback. Longterm plan for me would be to remove all FunctionPointerWithContext within the BLE code base and offers pre-baked event handlers that answer common use cases such as registering Callback objects or accepting multiple handlers. All of this can be build outside the main API and users that don't want the feature don't pay for it.

[AGlass0fMilk]

I see several options here:

* Provide an implementation of `GattServer::EventHandler` that accepts registration of user callbacks  ([example](https://github.com/ARMmbed/mbed-os/files/5342602/GattServerCallbackEventHandler.h.zip))

* Provide an implementation of `GattServer::EventHandler` that acts like a collection of `GattServer::EventHandlers`.

What do you think ?

The collection of GattServer::EventHandlers seems to be the more "C++" option. I'm not sure of the memory implications from subclassing a class with virtual members and then leaving some virtual functions default (an increase in vtable size of each object?)

What I would like to establish is a recommended pattern to design Gatt Service classes. There would be a lot of power in being able to offer a collection of premade, interoperable Gatt services that are simple to add by just calling register or something.

Right now, the examples show a few different ways that aren't consistent. Some Gatt services have a start method, some do not. Having a single event handler for GattServer events makes it harder to make reusable Gatt service classes that work together -- one Gatt service might override the event handler functions of a previously-added Gatt service.

That's why I like the implementation in the PR; registering an independent callback directly from the GattServer API is intuitive and clean. But I understand this could be cumbersome to those who have very restrictive memory constraints.

The way event handling works now is misleading as users may not know they are replacing any previously-installed callback handlers. I don't think that's ever desired behavior unless you only have one Gatt Service in your BLE profile.

One way I can see the "ChainableEventHandler" concept working is:

The paradigm GattService implementation is a subclass of GattServer::EventHandler if that GattService requires handling of any GattServer events. Something like a static DeviceInformationService might not care about when it's read and so does not need to handle any events.

In the simplest case where a user is trying to reduce memory usage and only needs one service they can simply set the GattServer::EventHandler to their single GattService.

If the user wants to have several separate GattService implementations that each need to handle events they will have to pull in the ChainableEventHandler. This collection of GattServer::EventHandlers can have a register and deregister function that adds/removes chained EventHandlers as needed. This ChainableEventHandler can then be set as the singular GattServer::EventHandler for the stack, delegating events to all registered services.

Side note: aside from additional dependencies, is there any reason to not use the standard library forward_list class for implementing such a chained event handler? I've found it to be very compact (<500B) and I like the idea of using a standard implmentation vs. creating a new (possibly buggy) linked list implementation each time I need one.

[pan-]

@AGlass0fMilk Thanks for the detailed comment

I'm not sure of the memory implications from subclassing a class with virtual members and then leaving some virtual functions default (an increase in vtable size of each object?)

It will consume ~8 bytes of flash per unused function in the vtable. We observed that usually pressure is more important on the RAM front so we went ahead with the EventHandler solution.

What I would like to establish is a recommended pattern to design Gatt Service classes. There would be a lot of power in being able to offer a collection of premade, interoperable Gatt services that are simple to add by just calling register or something.

Could you open another issue where we can discuss of the concepts ? We don't want to change the GattServer API today but we can define a framework on top of it.

Right now, the examples show a few different ways that aren't consistent. Some Gatt services have a start method, some do not. Having a single event handler for GattServer events makes it harder to make reusable Gatt service classes that work together -- one Gatt service might override the event handler functions of a previously-added Gatt service.

@paul-szczepanek-arm This might interest you.

I'm happy to accept a ChainableEventHandler in this PR. For me the main interrogation is an implementation one: Is it implemented with an array where the number of handlers that can be registered is fixed or if it uses memory allocation.

Side note: aside from additional dependencies, is there any reason to not use the standard library forward_list class

The main reasons to not use std container are exceptions. An exceptions is thrown when allocation of a node fail. But I agree a good, embedded friendly, implementation of list would be useful.

[AGlass0fMilk]

What I would like to establish is a recommended pattern to design Gatt Service classes. There would be a lot of power in being able to offer a collection of premade, interoperable Gatt services that are simple to add by just calling register or something.

Could you open another issue where we can discuss of the concepts ? We don't want to change the GattServer API today but we can define a framework on top of it.

It is a fantastic idea to create a framework on top of the existing BLE APIs. The mbed-os-example-ble applications are a great starting point for most simple applications but I think there's room for improvement. I've done a number of Mbed BLE applications and oftentimes I find myself facing the same design challenges and copy-pasting the same boilerplate code just to get up and running. The BLE stack would benefit immensely from a framework that helps users work out how to integrate BLE into an Mbed OS program. I usually just spin it off as a separate thread...

The mbed-os GitHub overlords (@0xc0170) may prefer us discussing this in a forum thread rather than an issue 😄

I'm happy to accept a ChainableEventHandler in this PR. For me the main interrogation is an implementation one: Is it implemented with an array where the number of handlers that can be registered is fixed or if it uses memory allocation.

I can change this PR to use a base EventHandler and deprecate the old FunctionPointerWithContext methods and submit another PR to introduce the ChainableEventHandler since that is kind of separate.

Side note: aside from additional dependencies, is there any reason to not use the standard library forward_list class

The main reasons to not use std container are exceptions. An exceptions is thrown when allocation of a node fail. But I agree a good, embedded friendly, implementation of list would be useful.

At least for GCC, isn't exception handling (and associated code size) excluded from compilation with the -fno-exceptions flag?

mbed-os/tools/profiles/develop.json

Lines 2 to 6 in ad40b1b

	"GCC_ARM": {
	"common": ["-Wall", "-Wextra",
	"-Wno-unused-parameter", "-Wno-missing-field-initializers",
	"-fmessage-length=0", "-fno-exceptions",
	"-ffunction-sections", "-fdata-sections", "-funsigned-char",

mbed-os/tools/profiles/develop.json

Lines 17 to 22 in ad40b1b

	"ARMC6": {
	"common": ["-c", "--target=arm-arm-none-eabi", "-mthumb", "-Os",
	"-Wno-armcc-pragma-push-pop", "-Wno-armcc-pragma-anon-unions",
	"-Wno-reserved-user-defined-literal", "-Wno-deprecated-register",
	"-DMULADDC_CANNOT_USE_R7", "-fdata-sections",
	"-fno-exceptions", "-MMD", "-fshort-enums", "-fshort-wchar",

[pan-]

The mbed-os GitHub overlords (@0xc0170) may prefer us discussing this in a forum thread rather than an issue

He's away this week, we can start it here 😂 . On a more serious tone the forum or the example repository might be better place since this will probably lives outside Mbed OS.

At least for GCC, isn't exception handling (and associated code size) excluded from compilation with the -fno-exceptions flag?

It indeed is*, but there is no way to report an allocation failure to the caller as it calls terminate. We want to report the error but I understand for specific applications this might not be a requirement.

* Code compiled with exception support stays with exception support (eg C++ library binaries).

[AGlass0fMilk]

Closing this PR since #13727 now supersedes this.

[paul-szczepanek-arm]

Event handlers can be chained if needed. You can register an event handler and have another event handler inside it. And if you want a callback you can create a callback on top of the event handler if you so require. Then you only pay for the cost of callbacks only if you need it. So it's a class that sits on top of the module you're interested in and allows you to register callbacks for individual calls. This could be even part of BLE what basically decorates the API with the callback translation. Who's going to maintain that though.

@pan- As for the example issues you mentioned I don't think the problem is the example as it shows the current API, the problem is our API is still not consistent, I can't fix that in the example.