-
Notifications
You must be signed in to change notification settings - Fork 7k
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: Make ULL/LLL split the default #15781
Bluetooth: controller: Make ULL/LLL split the default #15781
Conversation
71559a4
to
2e797ac
Compare
All checks are passing now. Review history of this comment for details about previous failed status. |
72c2acb
to
b60e1e5
Compare
@@ -175,7 +175,10 @@ int ll_init(struct k_sem *sem_rx) | |||
return -ENOMEM; | |||
} | |||
|
|||
#if defined(CONFIG_BT_CTLR_FILTER) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible to use IS_ENABLED()
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/zephyrproject-rtos/zephyr/blob/master/tests/bluetooth/bsim_bt/bsim_test_app/prj.conf
was meant to test the legacy controller,
and
https://github.com/zephyrproject-rtos/zephyr/blob/master/tests/bluetooth/bsim_bt/bsim_test_app/prj_split.conf
the split one.
When changing the default, if we want to continue testing the legacy one,
we would need to be set the 1st one to compile with BT_LL_SW.
(And whenever we are not interested in testing the legacy one anymore, we could just remove it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will block this until we have confirmation that controller-based privacy is not a blocker for anyone, because this is a substantial feature that we are taking away from users.
b60e1e5
to
cb06d6d
Compare
@aescolar addressed the bsim test conf file in #15849 @aescolar @carlescufi moved commits out into new PR #15849, that are independent from switching the default controller implementation. |
I have run PTS with #15849 + #15781 and here is the summary: Summary: Some details about the result: The first run was based on the tip of the day + PRs and some GATT TCs failed and it seemed not related to this PR. After rebasing the baseline to the tip used on Wednesday test(and this PR), those failed tests were passed again. I will look into it separately. |
@tedd-an Thank you for running the tests. Please upload the .config file of the zephyr build here (just to confirm it built the new link layer with CONFIG_BT_LL_SW_SPLIT=y). |
attached .config.
|
Thank you |
cb06d6d
to
cb21dab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are also a couple of bugfixes for the legacy controller, that should be ported to the new controller.
cb21dab
to
ba15b7c
Compare
Privacy for platforms with HW support added in the split controller in #16037
ba15b7c
to
0801be2
Compare
@@ -110,7 +110,7 @@ BLE-enabled builds that can be produced from the Zephyr project codebase: | |||
* :option:`CONFIG_BT_HCI` ``=y`` | |||
* :option:`CONFIG_BT_HCI_RAW` ``=y`` | |||
* :option:`CONFIG_BT_CTLR` ``=y`` | |||
* :option:`CONFIG_BT_LL_SW` ``=y`` (if using the open source Link Layer) | |||
* :option:`CONFIG_BT_LL_SW_LEGACY` ``=y`` (if using the open source Link Layer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix this to use BT_LL_SW_SPLIT
@@ -131,7 +131,7 @@ BLE-enabled builds that can be produced from the Zephyr project codebase: | |||
* :option:`CONFIG_BT` ``=y`` | |||
* :option:`CONFIG_BT_HCI` ``=y`` | |||
* :option:`CONFIG_BT_CTLR` ``=y`` | |||
* :option:`CONFIG_BT_LL_SW` ``=y`` (if using the open source Link Layer) | |||
* :option:`CONFIG_BT_LL_SW_LEGACY` ``=y`` (if using the open source Link Layer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix this to use BT_LL_SW_SPLIT
drivers/counter/Kconfig.nrfx
Outdated
@@ -13,7 +13,7 @@ config COUNTER_NRF_RTC | |||
config COUNTER_TIMER0 | |||
bool "Enable Counter on TIMER0" | |||
depends on HAS_HW_NRF_TIMER0 | |||
depends on !BT_LL_SW | |||
depends on !BT_LL_SW_LEGACY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add BT_LL_SW_SPLIT
drivers/counter/Kconfig.nrfx
Outdated
@@ -44,7 +44,7 @@ config COUNTER_TIMER4 | |||
config COUNTER_RTC0 | |||
bool "Enable Counter on RTC0" | |||
depends on HAS_HW_NRF_RTC0 | |||
depends on !BT_LL_SW | |||
depends on !BT_LL_SW_LEGACY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add BT_LL_SW_SPLIT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doc changes LGTM, other than your own comment touse BT_LL_SW_SPLIT
0801be2
to
9e6aca4
Compare
@@ -16,26 +16,27 @@ if BT_CTLR | |||
|
|||
choice BT_LL_CHOICE | |||
prompt "Bluetooth Link Layer Selection" | |||
default BT_LL_SW |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BT_LL_SW_SPLIT being the first item in the choice block, it is the de facto default
9e6aca4
to
a8254d7
Compare
Rename the controller Kconfig option BT_LL_SW to BT_LL_SW_LEGACY in preparation towards switch to new Link Layer implementation. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
Use legacy LL until RAM utilization is optimized in the new LL. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
Make the Upper and Lower Link Layer split architecture implementation of the controller as the default when building Zephyr Bluetooth Low Energy controller support in applications. Noticeable missing feature (porting) in comparison to old architecture implementation is, Advanced scheduling of connection events. The missing features will subsequently be submitted upstream. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
a8254d7
to
0bbcf2b
Compare
help | ||
Select the Bluetooth Link Layer to compile. | ||
|
||
config BT_LL_SW_LEGACY | ||
config BT_LL_SW_SPLIT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to rename this to BT_LL_SW
, that's what we agreed with @joerchan right? no more "split" in the name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the same BT_LL_SW will make users’ config file be valid after switching to new LL but without them noticing the change. Users should know the change in design. I will keep the _SPLIT until legacy is deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's really not the case if they are just using CONFIG_BT
on a Nordic board, right? BT_LL_SW_SPLIT
will be automatically enabled without them noticing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those who dont use explicit BT_LL_SW dont care if the macro is called BT_LL_SW_SPLIT or anything. We get them to use the new LL. Those who did use (for any reason) BT_LL_SW will now know that it is deprecated and take an informed decision to use BT_LL_SW_SPLIT.
Make the Upper and Lower Link Layer split architecture
implementation of the controller as the default when
building Zephyr Bluetooth Low Energy controller support
in applications.
Noticeable missing feature (porting) in comparison to old
architecture implementation is, Advanced scheduling of
connection events.
The missing features will subsequently be submitted
upstream.
Signed-off-by: Vinayak Kariappa Chettimada [email protected]
Fixes #12681
Depends on #16691