Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkg/nimble/autoadv: make AD flag optional #16703

Merged
merged 1 commit into from
Aug 24, 2021

Conversation

fjmolinas
Copy link
Contributor

@fjmolinas fjmolinas commented Aug 3, 2021

Contribution description

These fields can be omitted if all other FLAGS are 0 and the advertising
packets are not connectable, allowing for 3 extra bytes for advertisement
payload.

In an application I need every byte I can get out of the advertisement packet so It's useful/required to gain these 3 extra bytes.

Testing procedure

Flash with and without the flag set the FLAGS are then omitted from the payload:

CFLAGS=-DNIMBLE_AUTOADV_FLAG_FIELD=0 BOARD=nrf52840-mdk make -C examples/nimble_heart_rate_sensor/ flash term -j7

image
image

@fjmolinas fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 3, 2021
@fjmolinas fjmolinas requested a review from jia200x as a code owner August 3, 2021 10:29
@github-actions github-actions bot added Area: BLE Area: Bluetooth Low Energy support Area: doc Area: Documentation Area: pkg Area: External package ports labels Aug 3, 2021
@fjmolinas fjmolinas force-pushed the pr_nimble_autoadv_flag_field branch from 6252ee3 to ded4478 Compare August 3, 2021 10:29
@HendrikVE
Copy link
Contributor

Tested for my nrf52dk and the changes do as intended (after adding the missing C to the FLAGS at least ;P).

You are talking about saving three bytes, but I can only see two bytes that are missing. Where is the third byte hidden?

and the advertising packet is connectable, otherwise the Flags data type may be
omitted.

If your application is not connectable (eg. a temperature sensor advertising
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we determine if the application is connectable based on the modules that are used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, because although it could not be advertising, it could still connect to another peripheral device, so I can't use svc_gap/svc_gat for this, and when doing info-modules I'm not sure I see anything relevant there.

@fjmolinas
Copy link
Contributor Author

Tested for my nrf52dk and the changes do as intended (after adding the missing C to the FLAGS at least ;P).

You are talking about saving three bytes, but I can only see two bytes that are missing. Where is the third byte hidden?

Hmm I think its because nimble is then using AltBeacons where the ADFlags are encoded in only two bytes, while iBeacons use a LTV (length type value) encoding which is 3 bytes, I'll double check and update accordingly.

@fjmolinas
Copy link
Contributor Author

Tested for my nrf52dk and the changes do as intended (after adding the missing C to the FLAGS at least ;P).
You are talking about saving three bytes, but I can only see two bytes that are missing. Where is the third byte hidden?

Hmm I think its because nimble is then using AltBeacons where the ADFlags are encoded in only two bytes, while iBeacons use a LTV (length type value) encoding which is 3 bytes, I'll double check and update accordingly.

Never mind this, I got confused myself when looking at the debug output, but the if you look at the raw payloads you can see that actually 3 bytes are saved, since LTV 0x020106 is actually gone compared with the second message.

@fjmolinas
Copy link
Contributor Author

@HendrikVE I think your testing validates this PR since it was just confusion on both of us on how we looked at the nrfconnect log formatting, @benpicco can we move on without #16703 (comment) (I don't know how to address that)?

@HendrikVE
Copy link
Contributor

@fjmolinas My bad, sorry for the confusion 😅 But yes, I had exactly the same output in my nrfconnect as the screenshots show.

These fields can be omitted if all other FLAGS are 0 and the advertising
packets is not connectable, allowing for 3 extra bytes for advertisement
payload.

Co-authored-by: Roudy Dagher <roudy.dagher@inria.fr>
@fjmolinas
Copy link
Contributor Author

Rebase to get the tools-test fix in.

@fjmolinas fjmolinas merged commit 0e920e8 into RIOT-OS:master Aug 24, 2021
@fjmolinas fjmolinas deleted the pr_nimble_autoadv_flag_field branch August 24, 2021 08:10
@benpicco benpicco added this to the Release 2021.10 milestone Oct 20, 2021
@benpicco benpicco changed the title pkg/nimble/autoadv: make AD flag optionnal pkg/nimble/autoadv: make AD flag optional Oct 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: BLE Area: Bluetooth Low Energy support Area: doc Area: Documentation Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants