Skip to content

Add support for extended (>31 byte) BLE advertisements. #2301

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

Merged
merged 2 commits into from
Dec 7, 2019

Conversation

tannewt
Copy link
Member

@tannewt tannewt commented Nov 19, 2019

Tested with:

import adafruit_ble
from adafruit_ble.advertising import Advertisement

radio = adafruit_ble.BLERadio()
a = Advertisement()
a.complete_name = "This is a really long name that can only fit in extended."
print("len", len(a.complete_name))
radio.start_advertising(a)

while True:
    pass

extended=True must also be passed to start_scan.

For you @ntoll

@tannewt tannewt added this to the 5.0.0 milestone Nov 19, 2019
@tannewt tannewt requested a review from dhalbert November 19, 2019 22:05
@ntoll
Copy link

ntoll commented Nov 20, 2019

@tannewt it has been almost 20 years since I did any proper C/C++ coding so I'm definitely not qualified to comment on the C code 🤦‍♂️, but thank you for making these changes 👍 . As soon as they land I'll take the latest build and test with the adafruit_radio library. :shipit:

Question: is there a way to discover if the device is only BLE 4.x (i.e. not extended advertisements) which I could use to ensure the correct check on the length of the message is used in the adafruit_radio library? Alternatively, do the boards upon which you expect the adafruit_radio library to run all support BLE that does support extended advertisements -- in which case, I could safely assume 254 bytes not 31 bytes, right?

@tannewt
Copy link
Member Author

tannewt commented Nov 20, 2019

All of the nRF52840 boards support extended advertising. If a board doesn't then it will throw an exception when passed an advertisement that is too long. (255 is the nordic limit rather than the BLE one I believe.)

Builds are available for PRs if you click the latest green check on a commit and look in the top right for the "Artifacts" drop down.

@ntoll
Copy link

ntoll commented Nov 25, 2019

Related... adafruit/Adafruit_CircuitPython_BLE#33

@dhalbert
Copy link
Collaborator

Since extended advertisements are not receivable by peers that don't support this part of Bluetooth 5. I think invisibly switching extended advertising could be confusing ("why isn't the scanner seeing my advertisement?"). There could be a flag permitting extended advertisements, default False.

On the other hand, this is at the _bleio.Adapter level, so presumably the coder knows what they are doing.

I do think any extended advertising provided by adafruit_ble should not be done invisibly, but should be a specific class or flag to an advertisement constructor.

[Currently there merge conflicts, btw, so I won't review right now]

@tannewt
Copy link
Member Author

tannewt commented Nov 26, 2019

@dhalbert Are there specific devices you are worried about? iPhones have had BLE 5 since iPhone 8 and iPhone X in 2017. Looks like Google pixels have had them since Pixel 3. I enabled it automatically so that our devices have the best option between them by default.

@dhalbert
Copy link
Collaborator

We have four iPhone 6S's in our family :). I don't think we are that unusual.

@ntoll
Copy link

ntoll commented Nov 26, 2019

For what it's worth, I imagine the clientele of Adafruit are mostly tech savvy and, assuming the differences between BLE4 and 5 and how that effects advertisement length are well highlighted, they'll probably welcome the opportunity to upgrade their iPhones. ;-)

@dhalbert
Copy link
Collaborator

Just to be clear, I am not objecting to this PR, but I do think some awareness of creating extended advertisements should be visible in the BLE library.

@ntoll
Copy link

ntoll commented Nov 26, 2019

I understand, and completely agree with your concerns. 👍 🤗

I think that if there is a flag that either switches on or off extended advertising, libraries higher up the stack (such as the one I've been working on) should be able to detect the state of such a flag in order to adjust accordingly.

In any case, this is going to be very very cool and thanks to you both for your efforts in building this so others (like me) get to do fun stuff with wireless communication. 😄

@tannewt
Copy link
Member Author

tannewt commented Nov 26, 2019

I just ate lunch and think we should add length checking to Advertisement. Then, subclasses like AdafruitRadio can set extended_supported=True when initing the superclass.

@dhalbert
Copy link
Collaborator

@ntoll Rather than detecting whether extended advertising is on or off, I'm just saying that libraries that include code that might want to use extended advertising should allow that to be turned off. So for instance, it's great if the radio library can use extended advertising, but it would be good to be able to ask the radio library not to use extended advertising, in case one side can't deal with it.

As an example, I have a BT 4.0 dongle plugged into my host computer. I could write a radio library that ran on the host, but I'd want to make sure the radio side on the CPB doesn't attempt to send advertisements I would never see. So there should be an arg on the CPB side of the radio library that suppresses use of extended advertising, and throws an appropriate error if the data string is too long, etc. Am I missing something here?

@dhalbert
Copy link
Collaborator

I just ate lunch and think we should add length checking to Advertisement. Then, subclasses like AdafruitRadio can set extended_supported=True when initing the superclass.

👍

@ntoll
Copy link

ntoll commented Nov 26, 2019

@tannewt 👍

@dhalbert I think we're reflecting the same intention but I need to sleep so probably mangling it. I don't think you're missing something, I'm merely missing clarity of expression. ;-)

I'm happy to use whatever solution you come up with for flagging this feature, and definitely appreciate your work and engagement with this. 👏

@tannewt
Copy link
Member Author

tannewt commented Nov 26, 2019

@dhalbert just merged in the latest code. (Almost didn't realize it was my PR. :-) )

@dhalbert
Copy link
Collaborator

dhalbert commented Dec 6, 2019

I just ate lunch and think we should add length checking to Advertisement. Then, subclasses like AdafruitRadio can set extended_supported=True when initing the superclass.

@tannewt So I can accept this and we need a PR to the BLE library? I think that's what you mean.

@ntoll
Copy link

ntoll commented Dec 6, 2019

OK... once this lands I'll make sure the radio library has a flag (as @dhalbert suggests) to stop using extended advertising with the assumption that the programmer / user knows what they're doing. As always, I'm ears wide open for suggestions, constructive criticism and ideas. 👍

@tannewt
Copy link
Member Author

tannewt commented Dec 7, 2019

@dhalbert Yup, think so!

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

👍

@dhalbert dhalbert merged commit f5e9131 into adafruit:master Dec 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants