Bluetooth: controller: llcp: set refactored LLCP as default #46462

kruithofa · 2022-06-13T06:54:09Z

This PR sets the refactored LLCP as default

This exposed some missing include files, and updates to some KConfig settings,
which are also added in this PR

Issue #48823 has been created because of an observed instability in
the tests/bluetooth/bsim_bt/bsim_test_multiple test

Issue #44339 has been created for the missing support of advanced scheduling

Fixes #46065

subsys/bluetooth/controller/ll_sw/ull_conn_iso.c

Thalley · 2022-08-09T07:42:27Z

subsys/bluetooth/controller/ll_sw/ull_conn_types.h

+	uint8_t  cis_id;
+	uint32_t c_max_sdu:12;


Could uint16_t conn_event_count; be placed between cis_id and c_max_sdu?

At least to me it looks like that would be better in terms of grouping by word size of 32-bit.

Yes, however this is the way it was defined for legacy.

subsys/bluetooth/controller/ll_sw/ull_iso.c

subsys/bluetooth/controller/ll_sw/ull_llcp_conn_upd.c

subsys/bluetooth/controller/ll_sw/ull_peripheral_iso.c

tests/bluetooth/bsim_bt/bsim_test_eatt/src/common.h

Thalley · 2022-08-09T07:48:02Z

tests/bluetooth/bsim_bt/bsim_test_multiple/prj.conf

@@ -10,7 +10,7 @@ CONFIG_BT_AUTO_PHY_UPDATE=y
 CONFIG_BT_USER_DATA_LEN_UPDATE=y
 CONFIG_BT_AUTO_DATA_LEN_UPDATE=y

-CONFIG_BT_MAX_CONN=250
+CONFIG_BT_MAX_CONN=150


The "new" LLCP doesn't support 250 connections?
Should CONFIG_BT_ID_MAX be reduced as well then?

No, we need to fix the new LLCP so that it supports 250 connections

Tracked by: #48823, but is this a bug or a design limitation?

I believe this is a bug

subsys/bluetooth/controller/Kconfig.ll_sw_split

cvinayak · 2022-08-10T04:03:44Z

subsys/bluetooth/controller/Kconfig.ll_sw_split

+	default y if (BT_CENTRAL || (BT_BROADCASTER && BT_CTLR_ADV_EXT) || BT_CTLR_ADV_ISO) && \
+		     BT_LL_SW_LLCP_LEGACY


Link a GH issue to fix the missing support for advanced scheduling in new LLCP

cvinayak · 2022-08-10T04:06:21Z

tests/bluetooth/bsim_bt/bsim_test_eatt/prj_autoconnect.conf

+CONFIG_BT_CTLR_ADVANCED_FEATURES=y
+CONFIG_BT_CTLR_LLCP_LOCAL_PROC_CTX_BUF_NUM=4


Make CONFIG_BT_CTLR_LLCP_LOCAL_PROC_CTX_BUF_NUM=4 as default in Kconfig. Setting CONFIG_BT_CTLR_ADVANCED_FEATURES=y in projects will display build warning.

cvinayak · 2022-08-10T04:07:00Z

tests/bluetooth/bsim_bt/bsim_test_eatt/prj_collision.conf

+CONFIG_BT_CTLR_ADVANCED_FEATURES=y
+CONFIG_BT_CTLR_LLCP_LOCAL_PROC_CTX_BUF_NUM=4


Make CONFIG_BT_CTLR_LLCP_LOCAL_PROC_CTX_BUF_NUM=4 as default in Kconfig. Setting CONFIG_BT_CTLR_ADVANCED_FEATURES=y in projects will display build warning.

cvinayak · 2022-08-10T04:07:11Z

tests/bluetooth/bsim_bt/bsim_test_eatt/prj_multiple_conn.conf

+CONFIG_BT_CTLR_ADVANCED_FEATURES=y
+CONFIG_BT_CTLR_LLCP_LOCAL_PROC_CTX_BUF_NUM=4


Make CONFIG_BT_CTLR_LLCP_LOCAL_PROC_CTX_BUF_NUM=4 as default in Kconfig. Setting CONFIG_BT_CTLR_ADVANCED_FEATURES=y in projects will display build warning.

cvinayak · 2022-08-10T04:12:02Z

tests/bluetooth/bsim_bt/bsim_test_eatt/src/main_collision.c

+	/*
+	 * we need to sync with peer to ensure that we get collisions
+	 */
+	backchannel_sync_send();
+	backchannel_sync_wait();
+


This is test procedure change, changes should be separate PR and merged before this PR. And these changes should pass CI before and after switch to refactored LLCP.

This is test procedure change, changes should be separate PR and merged before this PR. And these changes should pass CI before and after switch to refactored LLCP.

See PR #48859

cvinayak · 2022-08-10T04:13:39Z

tests/bluetooth/bsim_bt/bsim_test_eatt/src/main_reconfigure.c

+	if (rx == NEW_MTU || tx == NEW_MTU) {
+		SET_FLAG(flag_reconfigured);
+	}


Again test case execution change, have in separate PR and be merged before switch to refactor PR.

See PR #48859

cvinayak · 2022-08-10T04:13:52Z

tests/bluetooth/bsim_bt/bsim_test_gatt_write/prj.conf

+
+CONFIG_BT_CTLR_ADVANCED_FEATURES=y
+CONFIG_BT_CTLR_LLCP_LOCAL_PROC_CTX_BUF_NUM=4


Make CONFIG_BT_CTLR_LLCP_LOCAL_PROC_CTX_BUF_NUM=4 as default in Kconfig. Setting CONFIG_BT_CTLR_ADVANCED_FEATURES=y in projects will display build warning.

cvinayak · 2022-08-10T04:20:12Z

tests/bluetooth/bsim_bt/bsim_test_multiple/prj.conf

@@ -10,7 +10,7 @@ CONFIG_BT_AUTO_PHY_UPDATE=y
 CONFIG_BT_USER_DATA_LEN_UPDATE=y
 CONFIG_BT_AUTO_DATA_LEN_UPDATE=y

-CONFIG_BT_MAX_CONN=250
+CONFIG_BT_MAX_CONN=150


Tracked by: #48823, but is this a bug or a design limitation?

cvinayak · 2022-08-10T04:26:47Z

tests/bluetooth/bsim_bt/bsim_test_multiple/prj.conf

@@ -43,3 +43,4 @@ CONFIG_BT_CTLR_RX_BUFFERS=6
 # accuracy, current value here is sufficient for 500ppm at 1 second interval.
 CONFIG_BT_CTLR_ADVANCED_FEATURES=y
 CONFIG_BT_CTLR_SCHED_ADVANCED_CENTRAL_CONN_SPACING=1000
+CONFIG_BT_CTLR_SCHED_ADVANCED=y


Why turn of in Kconfig file and enable CONFIG_BT_CTLR_SCHED_ADVANCED here?

This babblesim test is reuse of https://github.com/zephyrproject-rtos/zephyr/tree/main/samples/bluetooth/central_multilink and https://github.com/zephyrproject-rtos/zephyr/tree/main/samples/bluetooth/peripheral_identity, have you tested them on target?

jori-nordic

https://github.com/zephyrproject-rtos/zephyr/pull/46462/files#r941999012 . You can assign me to the PR you create for that test.

kruithofa · 2022-08-10T09:56:13Z

https://github.com/zephyrproject-rtos/zephyr/pull/46462/files#r941999012 . You can assign me to the PR you create for that test.

See PR #48859

Set the new refactored LLCP the default Signed-off-by: Andries Kruithof <andries.kruithof@nordicsemi.no>

addressed in #48859

Thalley · 2022-08-11T10:37:57Z

subsys/bluetooth/controller/ll_sw/ull_sched.c

@@ -58,7 +58,7 @@ static void win_offset_calc(struct ll_conn *conn_curr, uint8_t is_select,
 #endif
 #endif /* CONFIG_BT_CTLR_CONN_PARAM_REQ */

-#if defined(CONFIG_BT_CONN)
+#if defined(CONFIG_BT_CONN) && (defined(CONFIG_BT_CENTRAL) || defined(CONFIG_BT_LEGACY))


Should update the #endif on line 65 too

Thalley · 2022-08-11T10:39:34Z

subsys/bluetooth/controller/ll_sw/ull_sched.c

@@ -58,7 +58,7 @@ static void win_offset_calc(struct ll_conn *conn_curr, uint8_t is_select,
 #endif
 #endif /* CONFIG_BT_CTLR_CONN_PARAM_REQ */

-#if defined(CONFIG_BT_CONN)
+#if defined(CONFIG_BT_CONN) && (defined(CONFIG_BT_CENTRAL) || defined(CONFIG_BT_LEGACY))


Since CONFIG_BT_CENTRAL implies CONFIG_BT_CONN, this should perhaps have been

Suggested change

#if defined(CONFIG_BT_CONN) && (defined(CONFIG_BT_CENTRAL) || defined(CONFIG_BT_LEGACY))

#if (defined(CONFIG_BT_CONN) && defined(CONFIG_BT_LEGACY)) || defined(CONFIG_BT_CENTRAL)

Normally yes, but I'll leave it as is, since this change has to be reverted once issue #44339 is resolved

erbr-ot

LGTM

Setting the new LLCP as default exposed errors in CI tests, which are fixed here Note that advanced scheduling needs to be disabled. Work is in progress for implementing this for the new LLCP Signed-off-by: Andries Kruithof <andries.kruithof@nordicsemi.no>

kruithofa added the area: Bluetooth LLCP label Jun 13, 2022

kruithofa requested review from sjanc, erbr-ot and ppryga-nordic June 13, 2022 06:54

kruithofa requested review from carlescufi, cvinayak and thoh-ot as code owners June 13, 2022 06:54

zephyrbot added area: Bluetooth Controller area: Bluetooth labels Jun 13, 2022

zephyrbot requested review from mtpr-ot and wopu-ot June 13, 2022 06:54

zephyrbot assigned cvinayak Jun 13, 2022

kruithofa force-pushed the llcp_set_default branch from fa7a8ce to a11e332 Compare August 8, 2022 12:22

kruithofa requested review from alwa-nordic, jhedberg and Vudentz as code owners August 8, 2022 12:22

zephyrbot requested review from asbjornsabo, hermabe, jori-nordic and Thalley August 8, 2022 12:22

kruithofa removed request for Thalley, asbjornsabo and jori-nordic August 8, 2022 12:22

kruithofa force-pushed the llcp_set_default branch from a11e332 to c64d81f Compare August 8, 2022 12:36

zephyrbot requested review from asbjornsabo, jori-nordic and Thalley August 8, 2022 12:37

Thalley requested changes Aug 9, 2022

View reviewed changes

kruithofa force-pushed the llcp_set_default branch 2 times, most recently from 5fdcfa8 to dc938bc Compare August 9, 2022 12:34

Thalley previously approved these changes Aug 9, 2022

View reviewed changes

kruithofa dismissed Thalley’s stale review via 10f224f August 9, 2022 13:58

kruithofa force-pushed the llcp_set_default branch from dc938bc to 10f224f Compare August 9, 2022 13:58

Thalley previously approved these changes Aug 9, 2022

View reviewed changes

cvinayak requested changes Aug 10, 2022

View reviewed changes

jori-nordic previously requested changes Aug 10, 2022

View reviewed changes

Bluetooth: controller: llcp: set refactored as default

89689df

Set the new refactored LLCP the default Signed-off-by: Andries Kruithof <andries.kruithof@nordicsemi.no>

kruithofa dismissed Thalley’s stale review via 0052df3 August 10, 2022 12:49

kruithofa force-pushed the llcp_set_default branch from 10f224f to 0052df3 Compare August 10, 2022 12:49

kruithofa force-pushed the llcp_set_default branch 3 times, most recently from f8ecff9 to e0df2df Compare August 11, 2022 09:43

Thalley reviewed Aug 11, 2022

View reviewed changes

kruithofa force-pushed the llcp_set_default branch from e0df2df to f1488a3 Compare August 11, 2022 10:49

erbr-ot previously approved these changes Aug 11, 2022

View reviewed changes

kruithofa dismissed erbr-ot’s stale review via 5d3b1d2 August 11, 2022 11:38

kruithofa force-pushed the llcp_set_default branch from f1488a3 to 5d3b1d2 Compare August 11, 2022 11:38

Thalley approved these changes Aug 11, 2022

View reviewed changes

cvinayak approved these changes Aug 12, 2022

View reviewed changes

carlescufi requested review from erbr-ot and jori-nordic August 12, 2022 09:29

jori-nordic approved these changes Aug 12, 2022

View reviewed changes

carlescufi merged commit 6fe1299 into zephyrproject-rtos:main Aug 14, 2022

carlescufi mentioned this pull request Aug 17, 2022

Link Layer Control Procedure overhaul #15256

Closed

35 tasks

kruithofa mentioned this pull request Aug 26, 2022

bluetooth: controller: llcp: set refactored LLCP as default #46510

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bluetooth: controller: llcp: set refactored LLCP as default #46462

Bluetooth: controller: llcp: set refactored LLCP as default #46462

kruithofa commented Jun 13, 2022 •

edited

Loading

Thalley Aug 9, 2022

kruithofa Aug 9, 2022

Thalley Aug 9, 2022

kruithofa Aug 9, 2022

cvinayak Aug 10, 2022

kruithofa Aug 10, 2022

cvinayak Aug 10, 2022

kruithofa Aug 10, 2022

cvinayak Aug 10, 2022

kruithofa Aug 10, 2022

cvinayak Aug 10, 2022

kruithofa Aug 10, 2022

cvinayak Aug 10, 2022

kruithofa Aug 10, 2022

cvinayak Aug 10, 2022

kruithofa Aug 10, 2022

cvinayak Aug 10, 2022

kruithofa Aug 10, 2022

cvinayak Aug 10, 2022

kruithofa Aug 10, 2022

cvinayak Aug 10, 2022

cvinayak Aug 10, 2022

kruithofa Aug 10, 2022

jori-nordic left a comment

kruithofa commented Aug 10, 2022

Thalley Aug 11, 2022

kruithofa Aug 11, 2022

Thalley Aug 11, 2022

kruithofa Aug 11, 2022

erbr-ot left a comment

		default y if (BT_CENTRAL \|\| (BT_BROADCASTER && BT_CTLR_ADV_EXT) \|\| BT_CTLR_ADV_ISO) && \
		BT_LL_SW_LLCP_LEGACY

		CONFIG_BT_CTLR_ADVANCED_FEATURES=y
		CONFIG_BT_CTLR_LLCP_LOCAL_PROC_CTX_BUF_NUM=4

	#if defined(CONFIG_BT_CONN) && (defined(CONFIG_BT_CENTRAL) \|\| defined(CONFIG_BT_LEGACY))
	#if (defined(CONFIG_BT_CONN) && defined(CONFIG_BT_LEGACY)) \|\| defined(CONFIG_BT_CENTRAL)

Bluetooth: controller: llcp: set refactored LLCP as default #46462

Bluetooth: controller: llcp: set refactored LLCP as default #46462

Conversation

kruithofa commented Jun 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jori-nordic left a comment

Choose a reason for hiding this comment

kruithofa commented Aug 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erbr-ot left a comment

Choose a reason for hiding this comment

kruithofa commented Jun 13, 2022 •

edited

Loading