sys/nimble_autoadv: add new module by HendrikVE · Pull Request #13425 · RIOT-OS/RIOT

HendrikVE · 2020-02-20T15:51:07Z

Contribution description

This PR is outsourcing the module nimble_autoadv from my other PR #12012. Its main purpose is to restart the bluetooth advertising mechanism after a device disconnects, waiting for the next device connecting.

Testing procedure

You can test this module with #13506.

HendrikVE · 2020-06-08T14:34:50Z

Could you please have a look, @haukepetersen ? :)

haukepetersen · 2020-06-10T13:15:03Z

Yes, will do.

haukepetersen

Just some small remarks, else looks good to me

pkg/nimble/autoadv/nimble_autoadv.c

HendrikVE · 2020-06-10T17:34:10Z

Thank you, I pushed a fixup commit

haukepetersen

one thing I forgot to mention yesterday...

pkg/nimble/autoadv/include/nimble_autoadv.h

pkg/nimble/autoadv/nimble_autoadv.c

haukepetersen · 2020-06-11T13:35:48Z

pkg/nimble/autoadv/nimble_autoadv.c

+    }
+}
+
+int nimble_autoadv_add_field(uint8_t type, const void *data, size_t data_len)


just noticed: there is currently no way to reset the advertising data at runtime, right? I wonder if there is an easy and efficient way to add that functionality. I see two options: add a separate function like nimble_autoadv_ad_reset() or add something like a reset flag to this function here. I feel pretty much indifferent about this, so I let you decide whether you want to do this at all or which way would be best :-)

I like the idea. It gives us a reset function without a code overhead, because I can simply move the initialization code in to the reset function. I squashed the previous fixups and pushed a new commit with my proposal for a reset function.

So what I did is a complete reset. I believe you meant a reset only for the data stored in _ad. I don't really have a use case for a reset myself on hand, but I think if I would like to use it, than I would like to reset everything and make necessary changes afterwards.

HendrikVE · 2020-06-22T12:31:16Z

@haukepetersen So what do you think?

HendrikVE · 2020-07-07T15:06:35Z

Ping @haukepetersen

HendrikVE · 2020-08-21T20:57:16Z

@haukepetersen ? :)

fjmolinas · 2020-10-09T08:42:15Z

ping @haukepetersen :) !

stale review

pkg/nimble/autoadv/include/nimble_autoadv.h

pkg/nimble/autoadv/nimble_autoadv.c

fjmolinas · 2020-11-03T15:20:59Z

@HendrikVE I tested the example examples/nimble_gattand both on master and on this PR when I disconnect from the device I'm still able to connect to it afterwards, is that expected? I though this new module would enable me to do this but that It would not on master.

HendrikVE · 2020-11-04T13:04:12Z

I tested the example examples/nimble_gattand both on master and on this PR when I disconnect from the device I'm still able to connect to it afterwards, is that expected?

Yes, it is! :) You picked exactly the application where we can reduce the code size by using this module. Have look at #13506 / 68d5778

fjmolinas · 2020-11-04T13:33:25Z

I tested the example examples/nimble_gattand both on master and on this PR when I disconnect from the device I'm still able to connect to it afterwards, is that expected?

Yes, it is! :) You picked exactly the application where we can reduce the code size by using this module. Have look at #13506 / 68d5778

Then can you suggest a testing procedure to see the changes? Is there an application where the device would no start advertsiing again?

HendrikVE · 2020-11-04T14:02:56Z

Then can you suggest a testing procedure to see the changes?

I'm using Nordics mobile app nRF Connect. I rebased #13506 again to this PR, so nimble_gatt can be used for testing with the latest changes. But you'll need an nrf52 for testing.

Is there an application where the device would no start advertsiing again?

nimble_gatt and nimble_heart_rate_sensor both restart advertising. nimble_scanner does not use advertising at all. I'm not aware of an example not restarting advertising after a disconnect.

fjmolinas · 2020-11-04T14:23:33Z

Then can you suggest a testing procedure to see the changes?

I'm using Nordics mobile app nRF Connect. I rebased #13506 again to this PR, so nimble_gatt can be used for testing with the latest changes. But you'll need an nrf52 for testing.

Yes I did that to verify that things where working as before.

Is there an application where the device would no start advertsiing again?

nimble_gatt and nimble_heart_rate_sensor both restart advertising. nimble_scanner does not use advertising at all. I'm not aware of an example not restarting advertising after a disconnect.

So I'm asking because in your PR description you state "Its main purpose is to restart the bluetooth advertising mechanism after a device disconnects, waiting for the next device connecting.".

Do you know what was allowing this in master version of examples/nimble_gatt? I'm sorry if I'm being dump and not understanding what I should be looking for in the test behavior. But for example with respect to examples/nimble_gatt in #13506, are you just changing the logic that is causing the device to resume advertising after a disconnect?

HendrikVE · 2020-11-04T15:12:34Z

Do you know what was allowing this in master version of examples/nimble_gatt?

The restart of advertising is done by the gap_event_cb callback. It's given as parameter to ble_gap_adv_start and eventually ends up being used within the nimble stack. If something changes, the callback receives e.g. BLE_GAP_EVENT_DISCONNECT and starts advertising again.

In 1513526 by using the new nimble_autoadv module I'm able to remove gap_event_cb and start_advertise.

gap_event_cb@nimble_gatt is mapped to _gap_event_cb@nimble_autoadv (exactly the same)
start_advertise@nimble_gatt is mapped to nimble_autoadv_start@nimble_autoadv
- nimble_autoadv_start is by default called automatically by nimble_autoadv_init via auto_init.c by nimble_riot_init

are you just changing the logic that is causing the device to resume advertising after a disconnect?

I would describe it like the following. nimble_autoadv is outsourcing the logic to a separate module so the logic for restarting the advertising is reusable for other applications. They don't need to define the same simple callback all over again. But by making it a module we also add some features so it's not stuck to a set of settings.

fjmolinas · 2020-11-05T08:07:03Z

I've tested that the examples are still operational with these changes. From what I see in the review all comments by @haukepetersen have been addressed and they where minor remarks, everything else was OK on his side #13425 (review). I'm trusting his close review of the code, I've tested and taken a general look. Please squash @HendrikVE!

module for automated bluetooth advertising

HendrikVE · 2020-11-05T11:02:10Z

Squashed and checks passed, thanks!

fjmolinas

ACK.

HendrikVE requested review from aabadie, fjmolinas and jia200x as code owners February 20, 2020 15:51

HendrikVE requested a review from haukepetersen February 20, 2020 15:51

HendrikVE added Area: BLE Area: Bluetooth Low Energy support Area: sys Area: System Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Feb 20, 2020

HendrikVE force-pushed the pr/nimble_auto_adv branch from 9dda892 to 7376cd6 Compare February 20, 2020 15:54

HendrikVE changed the title ~~sys/nimble_auto_adv add new module~~ sys/nimble_auto_adv: add new module Feb 20, 2020

HendrikVE mentioned this pull request Feb 20, 2020

sys/stdio_nimble: add new stdio module using nimble #12012

Merged

3 tasks

HendrikVE force-pushed the pr/nimble_auto_adv branch from 19a9755 to cf07988 Compare February 24, 2020 15:20

HendrikVE changed the title ~~sys/nimble_auto_adv: add new module~~ sys/nimble_autoadv: add new module Feb 24, 2020

HendrikVE force-pushed the pr/nimble_auto_adv branch 3 times, most recently from 4e7c9c4 to 499576e Compare February 25, 2020 00:16

HendrikVE mentioned this pull request Feb 28, 2020

examples/nimble_*: use nimble_autoadv module #13506

Merged

1 task

HendrikVE force-pushed the pr/nimble_auto_adv branch from 499576e to 15ab05f Compare February 28, 2020 11:54

HendrikVE force-pushed the pr/nimble_auto_adv branch from 8fe6a71 to e156796 Compare June 8, 2020 11:49

HendrikVE added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Jun 8, 2020

haukepetersen requested changes Jun 10, 2020

View reviewed changes

haukepetersen requested changes Jun 11, 2020

View reviewed changes

pkg/nimble/autoadv/include/nimble_autoadv.h Show resolved Hide resolved

haukepetersen requested changes Jun 11, 2020

View reviewed changes

pkg/nimble/autoadv/nimble_autoadv.c Outdated Show resolved Hide resolved

HendrikVE force-pushed the pr/nimble_auto_adv branch 2 times, most recently from 2e52ead to 35070e8 Compare June 11, 2020 13:22

haukepetersen previously requested changes Jun 11, 2020

View reviewed changes

HendrikVE force-pushed the pr/nimble_auto_adv branch from a096b94 to 9d55a34 Compare June 11, 2020 15:46

benpicco requested a review from kaspar030 October 2, 2020 13:52

benpicco removed the CI: run tests If set, CI server will run tests on hardware for the labeled PR label Oct 2, 2020

fjmolinas reviewed Nov 3, 2020

View reviewed changes

pkg/nimble/autoadv/include/nimble_autoadv.h Show resolved Hide resolved

fjmolinas reviewed Nov 3, 2020

View reviewed changes

pkg/nimble/autoadv/include/nimble_autoadv.h Show resolved Hide resolved

fjmolinas reviewed Nov 3, 2020

View reviewed changes

pkg/nimble/autoadv/nimble_autoadv.c Outdated Show resolved Hide resolved

fjmolinas reviewed Nov 3, 2020

View reviewed changes

pkg/nimble/autoadv/nimble_autoadv.c Outdated Show resolved Hide resolved

fjmolinas self-assigned this Nov 3, 2020

HendrikVE force-pushed the pr/nimble_auto_adv branch from dc50520 to 24a29c1 Compare November 5, 2020 09:23

sys/nimble_autoadv: add new module

fe4f0a6

module for automated bluetooth advertising

HendrikVE force-pushed the pr/nimble_auto_adv branch from 24a29c1 to fe4f0a6 Compare November 5, 2020 09:54

fjmolinas approved these changes Nov 5, 2020

View reviewed changes

fjmolinas merged commit 5a9223e into RIOT-OS:master Nov 5, 2020

HendrikVE deleted the pr/nimble_auto_adv branch December 28, 2021 22:35

Conversation

HendrikVE commented Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Uh oh!

HendrikVE commented Jun 8, 2020

Uh oh!

haukepetersen commented Jun 10, 2020

Uh oh!

haukepetersen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HendrikVE commented Jun 10, 2020

Uh oh!

haukepetersen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

haukepetersen Jun 11, 2020

Choose a reason for hiding this comment

Uh oh!

HendrikVE Jun 11, 2020

Choose a reason for hiding this comment

Uh oh!

HendrikVE Jun 11, 2020

Choose a reason for hiding this comment

Uh oh!

HendrikVE commented Jun 22, 2020

Uh oh!

HendrikVE commented Jul 7, 2020

Uh oh!

HendrikVE commented Aug 21, 2020

Uh oh!

fjmolinas commented Oct 9, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fjmolinas commented Nov 3, 2020

Uh oh!

HendrikVE commented Nov 4, 2020

Uh oh!

fjmolinas commented Nov 4, 2020

Uh oh!

HendrikVE commented Nov 4, 2020

Uh oh!

fjmolinas commented Nov 4, 2020

Uh oh!

HendrikVE commented Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjmolinas commented Nov 5, 2020

Uh oh!

HendrikVE commented Nov 5, 2020

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

HendrikVE commented Feb 20, 2020 •

edited

Loading

HendrikVE commented Nov 4, 2020 •

edited

Loading