Decouple notification/indication_output from Client Characteristic Configuration

[kaofishy]

Right now server::notification_output()/server::indication_output() seems to check for CCCD bits regardless of whether it's actually used by the client, and from my understanding of the spec (3.3.3.3) it is entirely optional for either the client or the server to implement the descriptor (since a client can just drop the notification/indication if it doesn't want to handle it).

Nice to have for simple servers (also I had spent a few hours trying to figure out why I kept getting empty notifications and indications but that's neither here nor there).

[TorstenRobitzki]

Currently, the link_layer is calling that functions on the server object (link_layer<>::transmit_notifications()). It would do so, if it can dequeue a queued notification / indication by calling dequeue_indication_or_confirmation() on the connection details. If there is not a single characteristic configured for notification/indications, this function resolves to a specialization that allways return { notification_queue_entry_type::empty, 0 } to show, that there is no indication to send.

It depends a little bit on the compiler if it is smart enough to detect this to optimize this all away. This could be easily detected by commenting out the only call to transmit_notifications() in the link_layer.

If this makes a difference, we should consider to do a specialization earlier to give the compiler more informations.

[kaofishy]

Oh, I'm not using notification_queue at all, I guess I should clarify lol

It's more this part of notification_output()

if ( connection.client_configurations().flags( data.client_characteristic_configuration_index() ) & details::client_characteristic_configuration_notification_enabled &&
     out_size >= 3 )

It checks for the CCCD bits every time, and since neither client nor server are obligated to support it means that notification_output() will always return empty data if CCCD is not implemented. Right now I'm manually setting the bits to pretend that the client had done it, and it's fine for a single characteristic but I guess it just feels a bit hacky.

[TorstenRobitzki]

Am 22.06.2018 um 22:29 schrieb Tony Kao notifications@github.com:

Oh, I’m not using notification_queue at all, I guess I should clarify lol

I got that :-) I was talking about the code calling the code that you are talking about :-) The code that you are talking about, have to be called by the link layer. And the link layer do so, if there is a notification (or indication) pending for a given connection. This per connection data is stored in an instance of the connection_data object that is passed to the notification_output() function of the server.

And now comes the „trick“. If there are no characteristics with notification / indications (and thus no CCCD) the function that returns the information whether there is a pending notification or not, will always return: „No, there is no notification“. If the compiler can see that, it will hopefully remove a lot of code. So the situation boils down to:

in the link layer:

const auto notification = connection_details_.dequeue_indication_or_confirmation();

if ( notification.first != notification_queue_t::entry_type::empty )
{
server_->notification_output(…);
}

and if notification.first is always equal to notification_queue_t::entry_type::empty, this should boil down to:

const auto notification = connection_details_.dequeue_indication_or_confirmation();

if ( false )
{
server_->notification_output(…);
}

and then of cause the code might get be removed. But of cause, there is no guarantee, that the compiler will do, so it would be more safe to explicitly remove the call to notification_output() if there are no CCCDs in the ATT table.

I did a quick test with the blinky example, that contains no CCCDs and commented out the call to transmit_notifications() in the link layer (which is the only function that calls notification_output():

without call to transmit_notifications(): 11436 bytes
with call to transmit_notifications(): 11376 bytes

I’ve checked the numbers twice and yes, the binary gets even bigger :-| (Don’t ask me why) So I’ve directly commented out the code in server::notification_output() and server::indication_output() and then the code got reduced to:

without calls in server: 11380 bytes

Additional commenting out the calling code in the link layer again gives me the larger number

without calls in server and link_layer: 11436 bytes

Tested with arm-none-eabi-gcc 6.3.1 with -Os

[kaofishy]

That is very bizarre indeed. Just a wild guess, sometimes the compiler can figure out that certain variables will not actually change due to dead code and thus will treat them as constants, so maybe some stuff formerly in RAM are now in the ROM...?

In any case I’m working with an ESP32 for which Bluetoe is already infinitely less bloated than the supplied Bluedroid (WHY they decided to port a full Linux stack to an MCU is still beyond me, well probably for the Bluetooth Classic support). Their minimal BLE advertising example compiles to an almost 1MB (yes MB) flash image...and let’s not even get to all the dynamic memory usage.

[TorstenRobitzki]

Can be (I haven't looked at the RAM usage). To have a deeper look into this, one would have to use the newest GCC version and then, one could discuss this with the developers of GCC.

For your effort to run Bluetoe under an ESP32 Arduino: At my local Arduino Group here in Hannover, there is very much interest in the ESP32. I really would love to see that working. So if you have any questions, please contact me.

[kaofishy]

Yes I will definitely keep you updated on my progress!

[TorstenRobitzki]

To me, it looks like, that your effort could be described as: Bluetoe over HCI. As the standard defines several transports for HCI (USB, SPI etc.) I would suggest to decouple the transport from HCI. This would also allow for better testing.

I think this HCI implementation could get a little larger, so I would add an new folder into the bluetoe folder: bluetoe/hci . We could then add every element from that part of the library to the bluetoe::hci namespace. (comparable to bluetoe::link_layer)

Finally, when that is done, it should be fairly straightforward to add an esp32.hpp file to bluetoe/bindings, that contains something like this:

namespace bluetoe
{
    template < class Server, typename ... Options >
    using esp32 = hci::link_layer< Server, esp32_hci, Options... >;
}

which could then be used in an esp32 blinky.cpp example, as simple as this:

esp32< blinky_server > gatt_srv;

We should add a new branch for this and work on that branch until it's ready to be merged to the master branch. Improvements and fixes to current master should be applied there and then merged to the hci branch. What do you think?

I hope you don't find my comments on your commits to picky :-)

[kaofishy]

Oh I definitely welcome your comments! Honestly it's a bit embarrassing since the ESP32 specific files weren't supposed to be in the commit since they're extremely hacky at the moment. But I like your idea of making a more generalized HCI transport layer to Bluetoe.

[kaofishy]

Honestly I'm tempted to just firebomb my current fork and start fresh, I'm making a mess of my git commit history. Sigh.

I need to refactor my code to separate the HCI part from the ESP32 part anyway.

[TorstenRobitzki]

Shall we open a new ticket and assign it to you? I would first start with a generic HCI layer and when that is done, with a very specific ESP32 binding.

[kaofishy]

Sure thing. I'm still familiarizing myself with the link layer and trying to figure out how to fit HCI into everything so I'll probably be asking lots of questions :) hope you don't mind.