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

Bluetooth: Controller: Implement Periodic Advertiser List #38338

Merged
merged 5 commits into from
Oct 1, 2021

Conversation

cvinayak
Copy link
Contributor

@cvinayak cvinayak commented Sep 6, 2021

Implement support for Periodic Advertiser List to be used
in LE Periodic Advertising Create Sync command.

Fixes #38316.

Signed-off-by: Vinayak Kariappa Chettimada vich@nordicsemi.no

subsys/bluetooth/controller/Kconfig Outdated Show resolved Hide resolved
subsys/bluetooth/controller/Kconfig Outdated Show resolved Hide resolved
subsys/bluetooth/controller/Kconfig Outdated Show resolved Hide resolved
subsys/bluetooth/controller/ll_sw/lll_filter.h Outdated Show resolved Hide resolved
subsys/bluetooth/controller/ll_sw/lll_filter.h Outdated Show resolved Hide resolved
subsys/bluetooth/controller/ll_sw/lll_filter.h Outdated Show resolved Hide resolved
subsys/bluetooth/controller/ll_sw/ull_filter.c Outdated Show resolved Hide resolved
subsys/bluetooth/controller/ll_sw/ull_filter.c Outdated Show resolved Hide resolved
@cvinayak cvinayak force-pushed the github_per_adv_list branch 4 times, most recently from f77e105 to f4b3b59 Compare September 10, 2021 09:08
@github-actions github-actions bot added the area: Tests Issues related to a particular existing or missing test label Sep 10, 2021
@@ -497,6 +497,22 @@ config BT_CTLR_SYNC_PERIODIC
config BT_CTLR_SYNC_PERIODIC
bool "LE Periodic Advertising in Synchronization State [EXPERIMENTAL]" if BT_LL_SW_SPLIT

config BT_CTLR_SYNC_PERIODIC_ADV_LIST
Copy link
Contributor

@saleh-unikie saleh-unikie Sep 14, 2021

Choose a reason for hiding this comment

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

Is it possible to use this parameter option "BT_LE_PER_ADV_SYNC_OPT_USE_PER_ADV_LIST" (defined at bluetooth.h) automatically instead of defining a new kconfig? or at least generating a warning when this option is used but this config is not set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that is an API parameter, and this is a Controller feature.

help
Enable support for LE Periodic Advertiser List support.

config BT_CTLR_SYNC_PERIODIC_ADV_LIST_SIZE
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this config a redundant to the "CONFIG_BT_PER_ADV_SYNC_MAX"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, maximum supported simultaneous synchronizations can be lower than listed number of advertisers; only one advertiser is synchronized for every LE Periodic Advertising Create Sync Command, and Memory Capacity Exceeded error is used to convey insufficient resources to handle any more periodic advertising trains.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it! so setting the "CONFIG_BT_PER_ADV_SYNC_MAX" to a number greater than the "BT_CTLR_SYNC_PERIODIC_ADV_LIST_SIZE" is useless, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maximum simultaneous synchronizations CONFIG_BT_PER_ADV_SYNC_MAX can be greater than, says number of trusted peer device stored in a list CONFIG_BT_CTLR_SYNC_PERIODIC_ADV_LIST_SIZE. List is just a convenience, to be able to synchronize to any of the listed peers on a first-seen basis. Applications are permitted to use explicit peer device address to create additional synchronizations beyond the number of entries that be accommodated in the Periodic Advertiser List.

@cvinayak
Copy link
Contributor Author

@carlescufi Request your attention to this commit 4bc5d45 in this PR which partly handles inclusive naming related to filter implementation in the Controller. Your review is welcome here in this PR until the commit is ready to be cherry-picked out of this PR (so that other commits in this PR will have reduced rework effort)

@@ -391,7 +391,7 @@ config BT_CTLR_FAL_SIZE
range 1 8 if (SOC_COMPATIBLE_NRF || SOC_OPENISA_RV32M1_RISCV32)
range 1 16 if !(SOC_COMPATIBLE_NRF || SOC_OPENISA_RV32M1_RISCV32)
help
Set the size of the White List for LE Controller-based Privacy.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@carlescufi Do you want to send a separate PR for this being missed upstream?

subsys/bluetooth/controller/hci/hci.c Outdated Show resolved Hide resolved
subsys/bluetooth/controller/include/ll.h Outdated Show resolved Hide resolved
subsys/bluetooth/controller/ll_sw/lll_filter.h Outdated Show resolved Hide resolved
@Thalley
Copy link
Collaborator

Thalley commented Sep 22, 2021

@cvinayak I feel like this PR is becoming a catch-all for several issue (updates to filter accept list and addition of const), and that kind of distracts from what the PR should be doing.

Since I've already reviewed it, it's okay for me to keep as is, but probably beneficial for others if those changes were split to 3 PRs.

Refactoring related to filter accept list, resolving list,
and removed the redundant anon variable.

Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
Refactor `ull_filter_adva_get` and `ull_filter_adva_get` to
input resolving index compare to adv context reference.

Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
Implement support for Periodic Advertiser List to be used
in LE Periodic Advertising Create Sync command.

Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
Refactor out the implementation of Periodic Sync Setup
address check including Periodic Advertiser List and SID.

Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
Updated the bsim_bt_advx test with use of periodic
advertiser list.

Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
Copy link
Collaborator

@ppryga-nordic ppryga-nordic left a comment

Choose a reason for hiding this comment

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

Few minor things.

I'm not a fan of abbreviations like pal for periodic advertising list. It is hard to figure out what is it until you are in the topic.

subsys/bluetooth/controller/hci/hci.c Show resolved Hide resolved
subsys/bluetooth/controller/hci/hci.c Show resolved Hide resolved
subsys/bluetooth/controller/hci/hci.c Show resolved Hide resolved
subsys/bluetooth/controller/hci/hci.c Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Bluetooth Controller area: Bluetooth Qualification Bluetooth Qualification -related issues and pull requests area: Bluetooth area: Tests Issues related to a particular existing or missing test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Synchronize multiple DF TX devices in the DF Connectionless RX Example "Periodic Advertising list"
5 participants