Add support for extended advertising via Rust-only API #316
Conversation
* Extended functionality is gated on an "unstable" feature * Designed for very simple use and minimal interferance with existing legacy implementation * Intended to be temporary, until bumble can integrate extended advertising into its core functionality * Dropped `HciCommandWrapper` in favor of using bumble's `HCI_Command.from_bytes` for converting from PDL into bumble implementation * Refactored Address and Device constructors to better match what the python constructors expect
whitevegagabriel
force-pushed
the
extended
branch
from
fbfccca
to
1051648
Compare
| println!("Starting extended advertisement..."); | ||
| device.start_advertising_extended(adv_data).await?; | ||
| } else { | ||
| device.set_advertising_data(adv_data)?; |
There was a problem hiding this comment.
why does the non-extended advertising need set advertisement data separately?
There was a problem hiding this comment.
Non-extended advertising sets advertisement data separately to mirror the python behavior.
In adding extended advertising, I don't want to interfere with how advertisement_data is used for legacy use cases, so it takes advertisement data as a parameter.
There was a problem hiding this comment.
ah gotcha, perhaps set_advertising_data should be renamed to set_legacy_advertising_data then?
There was a problem hiding this comment.
This raises an interesting question.
set_advertising_data literally sets a new value for the property called advertising_data.
So, is it better to (1) rename it to set_legacy_advertising_data and document that it sets the property called advertising_data, or (2) to call it set_advertising_data and document that it gets used for legacy advertisements?
Since extended advertising is gated as an "unstable" feature, I'm inclined toward (2), but I want to know what you think.
There was a problem hiding this comment.
Really, both have their merits. 1 is a better API overall, but 2 will cause less cognitive overhead for people switching from using the Python API.
There was a problem hiding this comment.
Unless @barbibulle has some opinion on it, I'm probably going to stick with how it is currently. It may be confusing to some Rust users, but only if you opt-in for the unstable extended API, in which case you are expected to know what you're signing up for.
Plus, when extended is eventually added to bumble core, this API may disappear anyway.
whitevegagabriel
force-pushed
the
extended
branch
from
1c50e12
to
d5ea11b
Compare
whitevegagabriel
force-pushed
the
extended
branch
from
d5ea11b
to
180d84f
Compare
whitevegagabriel
force-pushed
the
extended
branch
from
180d84f
to
1004f10
Compare
rust/src/wrapper/device/mod.rs
Outdated
| } | ||
|
|
||
| // if you change this, don't forget to change the same handle in `stop_advertising_extended` | ||
| let advertising_handle = 0x00; |
There was a problem hiding this comment.
This will work temporarily for testing, but for real use cases, we'll need to support multiple advertising sets. For a typical device, you want to have one advertising set for legacy payloads (so that older peer devices can still find you) and one or more extended advertisers. Which does complicate the API somewhat (have to manage advertising instances, deal with running out of advertisers, etc.), which is why there's no official Bumble API for this yet (but working on it). The Android API is a good example for what an API could look like (even though it has its own gaps)
There was a problem hiding this comment.
Agreed that we will want to support multiple advertising sets in the future.
Once you've figured out how to support extended advertising in the core API, my plan is to completely drop this and adopt what you've implemented (which is why I've marked these additions as "unstable").
But in the meantime, we can at least use this for testing purposes.
| println!("Starting extended advertisement..."); | ||
| device.start_advertising_extended(adv_data).await?; | ||
| } else { | ||
| device.set_advertising_data(adv_data)?; |
There was a problem hiding this comment.
Really, both have their merits. 1 is a better API overall, but 2 will cause less cognitive overhead for people switching from using the Python API.
| .map(|any| Self(any.into())) | ||
| .map(|any| Self { | ||
| obj: any.into(), | ||
| advertising_status: AdvertisingStatus::NotAdvertising, |
There was a problem hiding this comment.
what if the device is a real dongle and it's actually already advertising?
There was a problem hiding this comment.
The device could pretty much be in any state (beyond just advertising/not advertising) until we send HCI_Reset during power_on.
We could (a) prevent some operations if power_on has not been called (b) reset the controller during __init__ to ensure a clean slate (c) leave as-is
I wrote this to match how its written in device.py, though I do think (b) would be better (with some parameter to disable reset if desired).
(a) could be fine, but could result in more invalid states by accident if not careful
(b) would ideally be implemented in device.py since I don't think it makes sense to fix the bug in Rust only, but am curious to hear thoughts from @barbibulle
After adding test for feature combinations, I found a corner case where, when Transport is dropped and the process is terminated in a test, the `close` Python future is not awaited. I don't know what other situations this issue may arise, so I have safe-guarded it via `block_on` instead of spawning a thread.
|
@barbibulle are there any other changes I should make to this PR to make it ready for merge? |
Sorry for the delay. Looks good to go. I'll merge it now. |
Not a problem, thank you! |
on_hci_le_set_scan_parameters_command(matching the BLE spec) to test error flow from RustHciCommandWrapperin favor of using bumble'sHCI_Command.from_bytesfor converting from PDL into bumble implementation