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

Add support for LE Extended Advertising #334

Merged
merged 2 commits into from
Nov 27, 2023
Merged

Add support for LE Extended Advertising #334

merged 2 commits into from
Nov 27, 2023

Conversation

zxzxwu
Copy link
Collaborator

@zxzxwu zxzxwu commented Nov 16, 2023

No description provided.

@zxzxwu
Copy link
Collaborator Author

zxzxwu commented Nov 16, 2023

Need to figure out what's the interops between legacy and extended adv.

@zxzxwu zxzxwu force-pushed the extadv branch 3 times, most recently from 80eb4a1 to 9f4c186 Compare November 18, 2023 07:10
bumble/device.py Outdated Show resolved Hide resolved
bumble/device.py Show resolved Hide resolved
@zxzxwu zxzxwu force-pushed the extadv branch 2 times, most recently from d1d100c to ce67ce7 Compare November 23, 2023 07:26
@zxzxwu zxzxwu marked this pull request as ready for review November 23, 2023 07:27

# Set the advertising data if present
if advertising_data is not None:
await self.send_command(
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this command is rejected (invalid parameter, if for example the data is too big, or if there are not enough advertising sets allowed in the controller), this will leak the handle.
The same issue may happen with the following two commands (they could fail, and in that case not only would the handle leak, but also a partial state may be set in the controller)
It would be good to have a way to clean up any intermediate state if any command fails here, before raising an error to the caller.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice catch. It also reminds me that we have to remove the advertising set on stop.

bumble/utils.py Outdated Show resolved Hide resolved
@zxzxwu
Copy link
Collaborator Author

zxzxwu commented Nov 26, 2023

Note here: According to Core Spec Vol.4 Part E, Section 3.1.1 - Legacy and extended advertising, Host should not issue Legacy Advertising commands when the Controller supports Extended Advertising, so there is no interops issue.

@zxzxwu zxzxwu merged commit 7bc7d0f into google:main Nov 27, 2023
51 checks passed
@zxzxwu
Copy link
Collaborator Author

zxzxwu commented Nov 27, 2023

Regarding the follow-up refactor, what will be changed exactly?
Will the legacy advertising API be changed as well? I don't like the static advertising data and scan response, so maybe we can add additional fields, but keep a default fallback to use static data.

@zxzxwu zxzxwu deleted the extadv branch December 2, 2023 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants