Bluetooth: Controller: ISO Sync Receiver with ticks_slot_window #75760

cvinayak · 2024-07-11T08:14:37Z

Add implementation for ISO Sync Receiver to use ticks slot
window feature.

Added implementation to have ISO Sync Receiver resume its
subevents after being aborted by other overlapping events.

Introduce BT_CTLR_PERIPHERAL_RESERVE_MAX Kconfig option so
that disabling this option will use minimum time reservation
and exercise the peripheral connection event continuation
using is_abort_cb mechanism.

Add implementation for Extended Advertising Auxiliary PDUs
to use ticks slot window feature.

May fix #56201.

Thalley

This PR adds a lot of new code that is very hard to read due to the additional Kconfig options (#if defined generally makes things harder to read).

Additionally the general lack of documentation for internal functions and their behavior and the long functions make it nearly impossible to actually understand what is going on.

Thalley · 2024-07-17T08:54:24Z

subsys/bluetooth/controller/Kconfig.ll_sw_split

I always get sad when I see more Kconfig options for the LL :( It's already way too hard to configure, and we keep adding more and more.

The Kconfig's have been added incrementally, atleast trying to keep existing features not being disturbed too much. Most internal once should get hidden under the Advanced Features but some of them are being be used outside. But hopefully will make them defaults in the future when the options form matured options in default builds of samples. Least or seldom used Kconfigs can be delete there after.

Thalley · 2024-07-17T08:56:13Z

subsys/bluetooth/controller/Kconfig.ll_sw_split

+	depends on BT_BROADCASTER && BT_CTLR_ADV_EXT
+	select BT_TICKER_EXT


What is the reasoning behind selecting BT_TICKER_EXT rather than depending on it?

Yes, will fix this to be depends on

Thalley · 2024-07-17T08:56:50Z

subsys/bluetooth/controller/Kconfig.ll_sw_split

+config BT_CTLR_SYNC_ISO_SLOT_WINDOW_JITTER
+	bool "Wrap ISO Synchronized Receiver event around overlapping roles"
+	depends on BT_CTLR_SYNC_ISO && !BT_CTLR_SYNC_ISO_RESERVE_MAX
+	select BT_TICKER_EXT
+	help
+	  Wrap ISO Synchronized Receiver event around overlapping roles using


I may just be unaware, but what are roles in this context?

roles / states are defined in BT Spec LINK LAYER STATES section, for example central peripheral are the two roles in connection state.

Thalley · 2024-07-17T08:59:17Z

subsys/bluetooth/controller/Kconfig.ll_sw_split

@@ -742,8 +759,22 @@ config BT_CTLR_CENTRAL_RESERVE_MAX
 	  Note, currently this value is only used to space multiple central
 	  connections and not for actual ticker time reservations.

+config BT_CTLR_PERIPHERAL_RESERVE_MAX


Should this be placed next to BT_CTLR_PERIPHERAL_ISO_EARLY_CIG_START to group the BT_CTLR_PERIPHERAL* configs?

No, that is peripheral ISO related. Where as this new one is related to normal peripheral ACL scheduling.

Thalley · 2024-07-17T09:00:39Z

subsys/bluetooth/controller/ll_sw/lll_conn.h

 		struct {
 			uint8_t initiated:1;
 			uint8_t cancelled:1;
+			uint8_t forced:1;
+		};
+
+		struct {
+			uint8_t initiated:1;
+			uint8_t cancelled:1;
+			uint8_t forced:1;
 		} central;


What is the difference between these 2 structs now?

unnamed struct is now used by a common code block and hence did not want to have a named reference as central or peripheral.

Suggest to put that in a comment, as that is non-obvious. Looks more like a merge conflict gone wrong :P

Thalley · 2024-07-17T09:13:30Z

subsys/bluetooth/controller/ll_sw/nordic/lll/lll_sync_iso.c

+		payload_offset = (lll->latency_event * lll->bn) +
+				 (lll->bn_curr - 1U);


You are adding multiplying 2 uint8_ts and adding it to another uint8 and store the result in a uint8 without any checks for overflow.

This looks dangerous to me, and suggest to either store the result in a uint16 and check the result, or add an assert for the other values to ensure that we don't get an overflow here.

addressing here: #75638

Thalley · 2024-07-17T09:14:27Z

subsys/bluetooth/controller/ll_sw/nordic/lll/lll_sync_iso.c

@@ -699,10 +769,15 @@ static void isr_rx(void *param)
 			lll->bn_curr = 1U;
 			lll->irc_curr++;

+			payload_offset = (lll->latency_event * lll->bn);


Ditto: Nothing to prevent an overflow here.

This is, IMO, a general issue in the controller as values are rarely checked.

Thalley · 2024-07-17T09:16:24Z

subsys/bluetooth/controller/ll_sw/nordic/lll/lll_sync_iso.c

+	if (latency_event--) {
+		goto isr_rx_done_latency_event;
+	}


What is the reasoning behind using goto here instead of a while (latency_event--)?

it was a quick hack... will fix this

Thalley · 2024-07-17T09:20:39Z

subsys/bluetooth/controller/ticker/ticker.c

+		/* Pick the ticker expiry latency, to calculate ticks_at_expire
+		 */


Suggested change

/* Pick the ticker expiry latency, to calculate ticks_at_expire

*/

/* Pick the ticker expiry latency, to calculate ticks_at_expire */

Looks like you've still (mis?)configured your editor to use 80 instead of 100 line length with autoformatting.

Thalley · 2024-07-17T09:22:29Z

subsys/bluetooth/controller/Kconfig.ll_sw_split

Additionally, it doesn't look like these new Kconfig options are ever tested

No, I will add modified bsim tests to cover the enhancements, the bap_broadcast_sink variant with XO clock drifts to have ACL connection in a sink drift over the BIG sync events, and measure SDU drops.

cvinayak

Thank you for reviewing, valuable inputs, and as always it takes me some time to refine the implementation, hope you retain your patience :-)

cvinayak · 2024-07-17T09:47:22Z

subsys/bluetooth/controller/Kconfig.ll_sw_split

+	depends on BT_BROADCASTER && BT_CTLR_ADV_EXT
+	select BT_TICKER_EXT


Yes, will fix this to be depends on

cvinayak · 2024-07-17T09:51:50Z

subsys/bluetooth/controller/Kconfig.ll_sw_split

+config BT_CTLR_SYNC_ISO_SLOT_WINDOW_JITTER
+	bool "Wrap ISO Synchronized Receiver event around overlapping roles"
+	depends on BT_CTLR_SYNC_ISO && !BT_CTLR_SYNC_ISO_RESERVE_MAX
+	select BT_TICKER_EXT
+	help
+	  Wrap ISO Synchronized Receiver event around overlapping roles using


roles / states are defined in BT Spec LINK LAYER STATES section, for example central peripheral are the two roles in connection state.

cvinayak · 2024-07-17T09:55:16Z

subsys/bluetooth/controller/Kconfig.ll_sw_split

The Kconfig's have been added incrementally, atleast trying to keep existing features not being disturbed too much. Most internal once should get hidden under the Advanced Features but some of them are being be used outside. But hopefully will make them defaults in the future when the options form matured options in default builds of samples. Least or seldom used Kconfigs can be delete there after.

cvinayak · 2024-07-17T09:57:01Z

subsys/bluetooth/controller/Kconfig.ll_sw_split

No, I will add modified bsim tests to cover the enhancements, the bap_broadcast_sink variant with XO clock drifts to have ACL connection in a sink drift over the BIG sync events, and measure SDU drops.

cvinayak · 2024-07-17T10:00:19Z

subsys/bluetooth/controller/Kconfig.ll_sw_split

@@ -742,8 +759,22 @@ config BT_CTLR_CENTRAL_RESERVE_MAX
 	  Note, currently this value is only used to space multiple central
 	  connections and not for actual ticker time reservations.

+config BT_CTLR_PERIPHERAL_RESERVE_MAX


No, that is peripheral ISO related. Where as this new one is related to normal peripheral ACL scheduling.

cvinayak · 2024-07-17T10:02:30Z

subsys/bluetooth/controller/ll_sw/lll_conn.h

 		struct {
 			uint8_t initiated:1;
 			uint8_t cancelled:1;
+			uint8_t forced:1;
+		};
+
+		struct {
+			uint8_t initiated:1;
+			uint8_t cancelled:1;
+			uint8_t forced:1;
 		} central;


unnamed struct is now used by a common code block and hence did not want to have a named reference as central or peripheral.

cvinayak · 2024-07-17T10:05:40Z

subsys/bluetooth/controller/ll_sw/nordic/lll/lll_sync_iso.c

+		drift_us %= lll->sub_interval;
+		drift_us = lll->sub_interval - drift_us;


i am trying to calculate a remaining sub_interval of a remainder (modulus) of sub_interval of drift_us... now you see I am not good at explaining :-(...

cvinayak · 2024-07-17T10:09:56Z

subsys/bluetooth/controller/ll_sw/nordic/lll/lll_sync_iso.c

+static int is_abort_cb(void *next, void *curr, lll_prepare_cb_t *resume_cb)
+{
+#if defined(CONFIG_BT_CTLR_SYNC_ISO_SLOT_WINDOW_JITTER)
+	if (!next) {
+		*resume_cb = resume_prepare_cb;
+
+		return -EAGAIN;
+	}
+#endif /* CONFIG_BT_CTLR_SYNC_ISO_SLOT_WINDOW_JITTER */
+
+	return -ECANCELED;
+}


ok... it is not documented...i agree... i should...
0 - pre-emptee succeeds to continue radio use, and pre-emptor is informed
-EAGAIN - pre-emptee looses and requests a resumption later, and pre-emptor will be scheduled
-ECANCELED - pre-emptee looses and be aborted, and pre-emptor will be scheduled

cvinayak · 2024-07-17T10:13:25Z

subsys/bluetooth/controller/ll_sw/nordic/lll/lll_sync_iso.c

+	if (latency_event--) {
+		goto isr_rx_done_latency_event;
+	}


it was a quick hack... will fix this

cvinayak · 2024-08-05T01:28:26Z

rebased to ensure CI is passing, keeping this PR draft and will be sending smaller PRs with considerations to already provided review comments here.

Simplify ticker reschedule_in_window implementation. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Remove ticks_elapsed use in reschedule_in_window as it has already been used in the bottom half processing. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Introduce ticker reschedule with jitter so that role like BIG can start after overlapping ACL and receive subsequent subevents. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Add implementation for ISO Sync Receiver to use ticks slot window feature. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Add implementation to support resumption of cancelled prepare. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Added implementation to have ISO Sync Receiver resume its subevents after being aborted by other overlapping events. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Introduce BT_CTLR_PERIPHERAL_RESERVE_MAX Kconfig option so that disabling this option will use minimum time reservation and exercise the peripheral connection event continuation using is_abort_cb mechanism. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Fix BT_CTLR_SCAN_AUX_SYNC_RESERVE_MIN such that event is not aborted when near supervision timeout conditions. Relates to commit ddf0499 ("Bluetooth: Controller: Add abort fairness in overlapping Periodic Sync"). Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Fix incorrect elapsed events value when event prepare are aborted in the pipeline. This caused premature supervision timeouts. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Add implementation for Extended Advertising Auxiliary PDUs to use ticks slot window feature. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Thalley · 2024-09-25T22:34:20Z

@cvinayak what was the reason for closing this?

cvinayak · 2024-09-26T04:00:07Z

This PR contained a mix of some fundamental changes to nRF specific Low Level scheduling (for which I will need new bsim tests, only tested using manual bsim samples); and, commits that fix/improve inter-operability with Sony Xperia and Samsung S24 phones. The fix/improvement commits are now in #77921 (which is also draft, as I need to complete handling of is_abort_cb mechanism for CIG events).

Also, I would like #78894 to be merged ahead.

To summarize the order:

Bluetooth: Controller: Implement PAST Support #78894
Bluetooth: Controller: Fix CIS payload count under skipped events #78096
Bluetooth: Controller: Fix elapsed events calculation, peripheral and periodic sync with better abort support #77921
Bluetooth: Controller: ISO Broadcast/Receive interleaved packing support #67231 or remaining commits of Bluetooth: Controller: ISO Sync Receiver with ticks_slot_window #75760 (both are enhancements)

cvinayak force-pushed the github_iso_recv_slot_window branch 8 times, most recently from f90eb08 to 27e3f8a Compare July 15, 2024 15:26

cvinayak marked this pull request as ready for review July 17, 2024 07:49

zephyrbot added area: Bluetooth area: Bluetooth Host area: Bluetooth Controller labels Jul 17, 2024

zephyrbot requested review from alwa-nordic, carlescufi, erbr-ot, hermabe, jhedberg, jori-nordic, kruithofa, mtpr-ot, sjanc, Thalley, theob-pro, thoh-ot and wopu-ot July 17, 2024 07:50

zephyrbot assigned jori-nordic and jhedberg Jul 17, 2024

Thalley requested changes Jul 17, 2024

View reviewed changes

cvinayak commented Jul 17, 2024

View reviewed changes

cvinayak marked this pull request as draft July 17, 2024 10:35

cvinayak force-pushed the github_iso_recv_slot_window branch 3 times, most recently from 760de75 to 0ada552 Compare July 21, 2024 01:56

jhedberg assigned cvinayak and unassigned jhedberg and jori-nordic Jul 22, 2024

cvinayak force-pushed the github_iso_recv_slot_window branch from 0ada552 to f9ac084 Compare August 5, 2024 01:26

cvinayak mentioned this pull request Aug 5, 2024

Bluetooth: controller: Possibility to have an active CIS and extended advertising at the same time #56201

Open

cvinayak force-pushed the github_iso_recv_slot_window branch from f9ac084 to 03732f1 Compare August 5, 2024 10:48

cvinayak force-pushed the github_iso_recv_slot_window branch 2 times, most recently from 2141ac7 to 26363ee Compare August 31, 2024 04:41

cvinayak added 10 commits September 3, 2024 13:53

Bluetooth: Controller: Simplify ticker reschedule_in_window

db72c46

Simplify ticker reschedule_in_window implementation. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Bluetooth: Controller: Fix ticks_elapsed use in reschedule_in_window

0e02ef4

Remove ticks_elapsed use in reschedule_in_window as it has already been used in the bottom half processing. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Bluetooth: Controller: Introduce ticker reschedule with jitter

cbd9524

Introduce ticker reschedule with jitter so that role like BIG can start after overlapping ACL and receive subsequent subevents. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Bluetooth: Controller: ISO Sync Receiver with ticks_slot_window

e728cfb

Add implementation for ISO Sync Receiver to use ticks slot window feature. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Bluetooth: Controller: Resume feature for cancelled prepare

ab2b255

Add implementation to support resumption of cancelled prepare. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Bluetooth: Controller: ISO Sync Receiver with resume feature

0da05fc

Added implementation to have ISO Sync Receiver resume its subevents after being aborted by other overlapping events. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Bluetooth: Controller: Fix incorrect elapsed events value

1196492

Fix incorrect elapsed events value when event prepare are aborted in the pipeline. This caused premature supervision timeouts. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Bluetooth: Controller: Ext Adv Auxiliary PDUs with ticks_slot_window

0c4ab4b

Add implementation for Extended Advertising Auxiliary PDUs to use ticks slot window feature. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

cvinayak force-pushed the github_iso_recv_slot_window branch from 26363ee to 0c4ab4b Compare September 3, 2024 11:54

cvinayak closed this Sep 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bluetooth: Controller: ISO Sync Receiver with ticks_slot_window #75760

Bluetooth: Controller: ISO Sync Receiver with ticks_slot_window #75760

cvinayak commented Jul 11, 2024 •

edited

Loading

Thalley left a comment

Thalley Jul 17, 2024

cvinayak Jul 17, 2024

Thalley Jul 17, 2024

cvinayak Jul 17, 2024

Thalley Jul 17, 2024

cvinayak Jul 17, 2024

Thalley Jul 17, 2024

cvinayak Jul 17, 2024

Thalley Jul 17, 2024

cvinayak Jul 17, 2024

Thalley Jul 17, 2024

Thalley Jul 17, 2024

cvinayak Jul 22, 2024

Thalley Jul 17, 2024

Thalley Jul 17, 2024

cvinayak Jul 17, 2024

Thalley Jul 17, 2024

Thalley Jul 17, 2024

Thalley Jul 17, 2024

cvinayak Jul 17, 2024

cvinayak left a comment

cvinayak Jul 17, 2024

cvinayak Jul 17, 2024

cvinayak Jul 17, 2024

cvinayak Jul 17, 2024

cvinayak Jul 17, 2024

cvinayak Jul 17, 2024

cvinayak Jul 17, 2024

cvinayak Jul 17, 2024

cvinayak Jul 17, 2024

cvinayak commented Aug 5, 2024

Thalley commented Sep 25, 2024

cvinayak commented Sep 26, 2024

		depends on BT_BROADCASTER && BT_CTLR_ADV_EXT
		select BT_TICKER_EXT

		payload_offset = (lll->latency_event * lll->bn) +
		(lll->bn_curr - 1U);

		/* Pick the ticker expiry latency, to calculate ticks_at_expire
		*/

		drift_us %= lll->sub_interval;
		drift_us = lll->sub_interval - drift_us;

Bluetooth: Controller: ISO Sync Receiver with ticks_slot_window #75760

Bluetooth: Controller: ISO Sync Receiver with ticks_slot_window #75760

Conversation

cvinayak commented Jul 11, 2024 • edited Loading

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

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

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

cvinayak commented Aug 5, 2024

Thalley commented Sep 25, 2024

cvinayak commented Sep 26, 2024

cvinayak commented Jul 11, 2024 •

edited

Loading