Bluetooth: Controller: Implement PAST Support #78894

lutb-ot · 2024-09-23T18:38:58Z

Implements:

LLCP PDU Flow and unittests for Periodic Sync Indication
Implements PAST support in ULL
PAST bsim tests
Fix PAST support bap_scan_delegator

aescolar · 2024-09-24T07:24:08Z

tests/bsim/bluetooth/ll/bis/testcase.yaml

+    platform_allow:
+      - nrf52_bsim
+      - nrf5340bsim/nrf5340/
+      - nrf5340bsim/nrf5340/cpuapp


If you want to build for the nrf5340bsim/nrf5340/cpuapp you'll need to use sysbuild (you can check other bsim tests building for it)

tests/bluetooth/controller/mock_ctrl/src/ull_sync.c

cvinayak

initial comments, will do re-review after these comments are addressed and CI jobs are passing.

subsys/bluetooth/controller/Kconfig

subsys/bluetooth/controller/hci/hci.c

subsys/bluetooth/controller/Kconfig.ll_sw_split

cvinayak

With a very quick pass, the changes LGTM, I do not see any objections from my side. Just get the CI happy :-)

cvinayak

nitpick.... use Bluetooth and Controller with B and C in commit title and message body etc... (BT Spec documentation uses so).

I change requests have been addressed.

Thalley

Very nice work. Great that you've added a bunch of tests for this

Most comments are code style and security related

subsys/bluetooth/audio/bap_scan_delegator.c

subsys/bluetooth/controller/CMakeLists.txt

subsys/bluetooth/controller/hci/hci.c

subsys/bluetooth/controller/ll_sw/nordic/lll/lll_adv_pdu.h

tests/bluetooth/controller/mock_ctrl/src/ull_adv_sync.c

tests/bsim/bluetooth/audio/src/bap_scan_delegator_test.c

Thalley · 2024-09-26T11:17:55Z

tests/bsim/bluetooth/ll/bis/prj_past.conf

+CONFIG_BT_ISO_BROADCASTER=y
+CONFIG_BT_ISO_SYNC_RECEIVER=y


Is ISO needed for this test?

Yes, same as earlier message.

Thalley · 2024-09-26T11:19:40Z

tests/bsim/bluetooth/ll/bis/src/test_past.c

+#define FAIL(...)					\
+	do {						\
+		bst_result = Failed;			\
+		bs_trace_error_time_line(__VA_ARGS__);	\
+	} while (0)
+
+#define PASS(...)					\
+	do {						\
+		bst_result = Passed;			\
+		bs_trace_info_time(1, __VA_ARGS__);	\
+	} while (0)


Consider having a look at the https://github.com/zephyrproject-rtos/zephyr/tree/main/tests/bsim/babblekit and https://github.com/zephyrproject-rtos/zephyr/tree/main/tests/bluetooth/common/testlib to see if you can reduce the overhead here by using the predefined test helpers

This is how it is done everywhere, so it should probably be a separate PR for fixing and cleaning this up everywhere in both host and controller bsim tests.

I understand your point, although I don't necessarily agree. We do not need to follow the structure of older tests if new tests could use frameworks that were not available when the older tests were written.

It's OK to keep as is, but could have saved you time to use the frameworks we have now :)

subsys/bluetooth/controller/ll_sw/ull_adv_types.h

subsys/bluetooth/controller/ll_sw/ull_sync_types.h

subsys/bluetooth/controller/ll_sw/ull_conn_types.h

Thalley · 2024-10-03T11:25:40Z

subsys/bluetooth/controller/ll_sw/ull_llcp.c

+							INVALID_SYNC_HANDLE;
+	ctx->data.periodic_sync.adv_handle = adv_sync ? ull_adv_sync_handle_get(adv_sync) :
+							INVALID_ADV_HANDLE;
+	ctx->data.periodic_sync.id = service_data;


Would it make sense to use service_data instead of id in the periodic_sync struct?

The only reason it is called ID is because of the naming in the Spec:

"ID shall be set to an identifier provided by the Host. This is for use by higher-level protocols and its value is not specified or used by this specification."

Aha, interesting. I'm having trouble finding where it's defined that the id here is the same value as the service_data provided by the host. I believe it's correct that it is, but haven't found where that's explicitly stated

I haven't seen it explicitly stated anywhere in the Spec. However, it is the only host parameter in HCI_LE_Periodic_Advertising_Sync_Transfer and HCI_LE_Periodic_Advertising_Sync_Transfer with the correct range and description, but it is not explicitly stated anywhere.

subsys/bluetooth/controller/ll_sw/ull_llcp.c

subsys/bluetooth/controller/ll_sw/ull_sync.c

Thalley

No more blocking comments from me, but we should really get @cvinayak to do another review and respond to some of the outstanding comments before I'll approve

cvinayak

No definite change requests from my end. LGTM.

tests/bluetooth/controller/ctrl_periodic_sync/Kconfig

cvinayak · 2024-10-08T11:26:25Z

tests/bluetooth/controller/ctrl_periodic_sync/prj.conf

+CONFIG_BT_CTLR_SYNC_ISO_STREAM_COUNT=1
+CONFIG_BT_CTLR_CONN_ISO_STREAMS=1
+CONFIG_BT_CTLR_CONN_ISO_GROUPS=1


@lutb-ot you should make sca available for PAST support too.

Background, periodic sync needs sca but uses it as local variable to calculate window widening. But a ISO sync is established using an inherited sca value, hence sca was stored in lll_sync context. Now that PAST feature will need the sca to be remembered, i think we should make the sca field available in the lll_sync context? or wait, if this is for window widening, then it would be calculated only once, do we really need it to be present in lll_sync context?

subsys/bluetooth/controller/ll_sw/ull_sync.c

subsys/bluetooth/controller/ll_sw/ull_conn.c

subsys/bluetooth/controller/ll_sw/nordic/lll/lll_adv_pdu.h

cvinayak · 2024-10-10T09:17:44Z

subsys/bluetooth/controller/ll_sw/ull_sync.c

+	       PDU_SYNC_INFO_SCA_CHM_SCA_BIT_MASK) >>
+	      PDU_SYNC_INFO_SCA_CHM_SCA_BIT_POS;
+
+	lll->sca = sca;


dont you need conditional compiles here?

The whole function is already flaged with CONFIG_BT_CTLR_SYNC_TRANSFER_RECEIVER.

Adding PDU flow for Periodic Sync Indication Signed-off-by: Lucas Mathias Balling <[email protected]>

Added LLCP PDU Periodic Sync Indication unittests Signed-off-by: Lucas Mathias Balling <[email protected]>

Implement PAST support in ULL and fixed test after changed dependencies in ull_sync_internal.c Signed-off-by: Lucas Mathias Balling <[email protected]>

Implemented: * Central sync to broadcaster, connects to peripheral and transfers sync. Peripheral Syncs to PA * Broadcaster acting Central connects to peripheral and transfers sync. Peripheral sync to PA * Moved previous bis tests to test_bis.c and new PAST tests was added in test_past.c Signed-off-by: Lucas Mathias Balling <[email protected]>

Fixed PAST support for bap_scan_delegator Signed-off-by: Lucas Mathias Balling <[email protected]>

zephyrbot added area: Bluetooth platform: nRF BSIM Nordic Semiconductors, nRF BabbleSim area: Bluetooth Controller area: Bluetooth Audio labels Sep 23, 2024

zephyrbot requested review from aescolar, asbjornsabo, carlescufi, Casper-Bonde-Bose, cvinayak, erbr-ot, fredrikdanebjer, jhedberg, kruithofa, MariuszSkamra, mtpr-ot, sjanc, Thalley, thoh-ot and wopu-ot September 23, 2024 18:39

zephyrbot assigned cvinayak Sep 23, 2024

lutb-ot marked this pull request as draft September 23, 2024 18:42

aescolar reviewed Sep 24, 2024

View reviewed changes

cvinayak previously requested changes Sep 24, 2024

View reviewed changes

subsys/bluetooth/controller/Kconfig Outdated Show resolved Hide resolved

subsys/bluetooth/controller/hci/hci.c Outdated Show resolved Hide resolved

lutb-ot force-pushed the add_past_support branch from 0cfd60a to d7b3d88 Compare September 24, 2024 09:43

cvinayak reviewed Sep 24, 2024

View reviewed changes

subsys/bluetooth/controller/Kconfig.ll_sw_split Outdated Show resolved Hide resolved

cvinayak reviewed Sep 24, 2024

View reviewed changes

lutb-ot force-pushed the add_past_support branch from d7b3d88 to 165f287 Compare September 25, 2024 08:50

lutb-ot marked this pull request as ready for review September 25, 2024 18:25

cvinayak previously approved these changes Sep 25, 2024

View reviewed changes

cvinayak mentioned this pull request Sep 26, 2024

Bluetooth: Controller: ISO Sync Receiver with ticks_slot_window #75760

Closed

Thalley requested changes Sep 26, 2024

View reviewed changes

cvinayak mentioned this pull request Sep 27, 2024

Bluetooth: Controller: Fix elapsed events calculation, peripheral and periodic sync with better abort support #77921

Merged

cvinayak added this to the v4.0.0 milestone Sep 27, 2024

lutb-ot dismissed cvinayak’s stale review via 0d53aa8 October 3, 2024 10:08

lutb-ot force-pushed the add_past_support branch from 165f287 to 0d53aa8 Compare October 3, 2024 10:08

Thalley requested changes Oct 3, 2024

View reviewed changes

lutb-ot force-pushed the add_past_support branch 2 times, most recently from 0813c81 to 4a2e12d Compare October 7, 2024 10:59

Thalley reviewed Oct 7, 2024

View reviewed changes

Thalley requested review from cvinayak and aescolar October 8, 2024 06:32

cvinayak previously approved these changes Oct 8, 2024

View reviewed changes

lutb-ot force-pushed the add_past_support branch from 4a2e12d to ae127b1 Compare October 8, 2024 20:04

lutb-ot dismissed cvinayak’s stale review via af1794a October 9, 2024 07:48

lutb-ot force-pushed the add_past_support branch from ae127b1 to af1794a Compare October 9, 2024 07:48

cvinayak mentioned this pull request Oct 10, 2024

Bluetooth:Controller: Document the design of the ADV PDU double buffer implementation #79643

Open

cvinayak requested changes Oct 10, 2024

View reviewed changes

kruithofa removed their request for review October 15, 2024 11:07

lutb-ot added 2 commits October 22, 2024 21:40

Bluetooth: Controller: Implement Periodic Sync Procedure

551b22e

Adding PDU flow for Periodic Sync Indication Signed-off-by: Lucas Mathias Balling <[email protected]>

tests: Bluetooth: LLCP unittests for Periodic Sync Procedure

ce58a18

Added LLCP PDU Periodic Sync Indication unittests Signed-off-by: Lucas Mathias Balling <[email protected]>

lutb-ot force-pushed the add_past_support branch 2 times, most recently from 66b4b23 to 1d91fa8 Compare October 22, 2024 20:08

lutb-ot added 3 commits October 22, 2024 22:14

Bluetooth: Controller: Implement PAST support in ULL

3f22553

Implement PAST support in ULL and fixed test after changed dependencies in ull_sync_internal.c Signed-off-by: Lucas Mathias Balling <[email protected]>

Bluetooth: Audio: Fix PAST support for bap_scan_delegator

89eaa5c

Fixed PAST support for bap_scan_delegator Signed-off-by: Lucas Mathias Balling <[email protected]>

lutb-ot force-pushed the add_past_support branch from 1d91fa8 to 89eaa5c Compare October 22, 2024 20:15

Thalley approved these changes Oct 22, 2024

View reviewed changes

Thalley requested a review from cvinayak October 22, 2024 21:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bluetooth: Controller: Implement PAST Support #78894

Bluetooth: Controller: Implement PAST Support #78894

lutb-ot commented Sep 23, 2024

aescolar Sep 24, 2024

cvinayak left a comment

cvinayak left a comment

cvinayak left a comment

Thalley left a comment

Thalley Sep 26, 2024

lutb-ot Oct 1, 2024

Thalley Sep 26, 2024

lutb-ot Oct 1, 2024

Thalley Oct 1, 2024

Thalley Oct 3, 2024

lutb-ot Oct 4, 2024

Thalley Oct 4, 2024

lutb-ot Oct 4, 2024

Thalley left a comment

cvinayak left a comment

cvinayak Oct 8, 2024

cvinayak Oct 10, 2024

lutb-ot Oct 22, 2024

		CONFIG_BT_ISO_BROADCASTER=y
		CONFIG_BT_ISO_SYNC_RECEIVER=y

Bluetooth: Controller: Implement PAST Support #78894

Are you sure you want to change the base?

Bluetooth: Controller: Implement PAST Support #78894

Conversation

lutb-ot commented Sep 23, 2024

Choose a reason for hiding this comment

cvinayak left a comment

Choose a reason for hiding this comment

cvinayak left a comment

Choose a reason for hiding this comment

cvinayak left a comment

Choose a reason for hiding this comment

Thalley left a 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

Thalley left a comment

Choose a reason for hiding this comment

cvinayak left a 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