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

Thalley · 2024-09-26T09:43:59Z

subsys/bluetooth/audio/bap_scan_delegator.c

+		} else {
+			/* If past is supported - state will change */
+			if (supports_past(conn, pa_sync)) {
+				state_changed = true;
+			}
 		}


That is not how state changes in BASS works :) If there are not value changes, we shouldn't send a notification.

The current API is not great for PAST and should probably be improved, but the idea is that the application will call bt_bap_scan_delegator_set_pa_state with BT_BAP_PA_STATE_INFO_REQ to trigger PAST from the broadcast assistant. I think it makes sense for pa_sync_request to return this value in the future

subsys/bluetooth/controller/CMakeLists.txt

subsys/bluetooth/controller/hci/hci.c

Thalley · 2024-09-26T09:50:12Z

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

+static inline struct pdu_adv *lll_adv_sync_data_curr_get(struct lll_adv_sync *lll)
+{
+	return (void *)lll->data.pdu[lll->data.first];
+}


Something seems a bit out of place here: The function is called curr_get which I assume is short for current get, but it always uses lll->data.first.
Is that correct?

@cvinayak will you be able to add more information here, as this function follows the same design as struct pdu_adv *lll_adv_aux_data_curr_get(struct lll_adv_aux *lll)

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:30Z

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

+		if (err == 0) {
+			err = bt_bap_scan_delegator_set_pa_state(state->recv_state->src_id,
+								 BT_BAP_PA_STATE_INFO_REQ);


Very nice that you've added this support. Have you verified that it getting triggered correctly?

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 :)

Thalley · 2024-10-03T11:14:59Z

subsys/bluetooth/controller/ll_sw/ull_adv_types.h

+ #define INVALID_ADV_HANDLE 0xFF
+


We already have BT_HCI_ADV_HANDLE_INVALID which we can use

My bad, I'll fix that.

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

subsys/bluetooth/controller/ll_sw/ull_sync_types.h

+ #define INVALID_SYNC_HANDLE     0xFFFF
+


We have BT_HCI_SYNC_HANDLE_INVALID that you can use instead

Thalley · 2024-10-03T11:21:10Z

subsys/bluetooth/controller/ll_sw/ull_conn_types.h

+	struct {
+		uint8_t mode;
+		uint8_t cte_type;
+		uint16_t skip;
+		uint16_t timeout;
+	} past;


Would it make sense to define this struct by itself and use that in ull_conn, instead of the 4 individual values? Would make it easier to reset, and could even return those values as a reference or a single value instead of having 4 functions so that

conn->past.mode = ull_conn_default_past_param_mode_get(); conn->past.skip = ull_conn_default_past_param_skip_get(); conn->past.timeout = ull_conn_default_past_param_timeout_get(); conn->past.cte_type = ull_conn_default_past_param_ctetype_get();

could become something like

conn->past = ull_conn_default_past_param_get();

or

memcpy(&conn->past, ull_conn_default_past_param_get(), sizeof(conn->past));

or is there some value in having 4 separate functions and values for this?

No, I think it will make perfect sense to make it into a single get function instead of four.

I'll make a change for that :)

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.

Thalley · 2024-10-03T11:27:22Z

subsys/bluetooth/controller/ll_sw/ull_llcp.c

+	/* This is equivalent to:
+	 * exactly one bit set, and no bit set is rfu's
+	 */
+	return (phy == PHY_1M || phy == PHY_2M || phy == PHY_CODED);


Should PHY_2M be considered valid if CONFIG_BT_CTLR_PHY_2M=n? Ditto with coded?

It will be checked after: see: ull_llcp_past.c line:83:

`#if defined(CONFIG_BT_CTLR_PHY)
const uint8_t phy = pdu->llctrl.periodic_sync_ind.phy;

if (((phy & PHY_2M) && !IS_ENABLED(CONFIG_BT_CTLR_PHY_2M)) || ((phy & PHY_CODED) && !IS_ENABLED(CONFIG_BT_CTLR_PHY_CODED))) { /* Unsupported phy selected */ return BT_HCI_ERR_UNSUPP_FEATURE_PARAM_VAL; }

#endif /* CONFIG_BT_CTLR_PHY */`

Thalley · 2024-10-03T11:29:27Z

subsys/bluetooth/controller/ll_sw/ull_sync.c

+	conn_interval_us = conn->lll.interval * CONN_INT_UNIT_US;
+
+	/* Calculate offset and schedule sync radio events */
+	ready_delay_us = lll_radio_rx_ready_delay_get(lll->phy, 1);


Suggested change

ready_delay_us = lll_radio_rx_ready_delay_get(lll->phy, 1);

ready_delay_us = lll_radio_rx_ready_delay_get(lll->phy, PHY_FLAGS_S8);

Right?

Yes, that should be the most correct way. Will change that.

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

zephyrbot added the area: Bluetooth Host label Sep 25, 2024

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

Open

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 added 5 commits October 4, 2024 13:01

Bluetooth: Controller: Implement Periodic Sync Procedure

72a3e52

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

tests: Bluetooth: LLCP unittests for Periodic Sync Procedure

0e16363

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

Bluetooth: Controller: Implement PAST support in ULL

ec0f221

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

0813c81

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 0d53aa8 to 0813c81 Compare October 4, 2024 11:37

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

Thalley Sep 26, 2024

lutb-ot Sep 30, 2024

Thalley Sep 26, 2024

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 3, 2024

Thalley Oct 3, 2024

lutb-ot Oct 4, 2024

Thalley Oct 3, 2024

lutb-ot Oct 4, 2024

Thalley Oct 4, 2024

lutb-ot Oct 4, 2024

Thalley Oct 3, 2024

lutb-ot Oct 4, 2024

Thalley Oct 3, 2024

lutb-ot Oct 4, 2024

	ready_delay_us = lll_radio_rx_ready_delay_get(lll->phy, 1);
	ready_delay_us = lll_radio_rx_ready_delay_get(lll->phy, PHY_FLAGS_S8);

		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

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