Bluetooth: Controller: Fix single switch timer use in ISO Sync #96653

cvinayak · 2025-09-27T15:00:11Z

Fix implementation of Broadcast ISO Synchronized Receiver using single switch timer to consider minimum compare value requirement.

This fix reduces latencies to setup radio receptions and fixes an assertion in lll_sync_iso when radio_tmr_start_us() is checked for latencies.

Relates to commit 5dfc58c ("Bluetooth: Controller: Fix single switch timer minimum compare value").

Copilot

Pull Request Overview

This PR fixes the implementation of Broadcast ISO Synchronized Receiver when using a single switch timer to properly handle minimum compare value requirements. The changes update the overhead calculation logic to account for maximum ISR latency and improve timing precision for radio receptions.

Refactors overhead calculation to include HAL_RADIO_ISR_LATENCY_MAX_US consideration
Removes timer start delay subtraction from jitter calculation
Adds detailed comments explaining each overhead component

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-28T19:53:58Z

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

+		/* If single timer used, then consider required max. latency */
+		overhead_us = MAX(overhead_us, HAL_RADIO_ISR_LATENCY_MAX_US);
+		/* Add chain delay overhead */
+		overhead_us += radio_rx_chain_delay_get(lll->phy, PHY_FLAGS_S8);


The overhead calculation logic has changed significantly but the addr_us_get(lll->phy) component that was previously included in the overhead is now missing. This could affect the total overhead calculation unless this component is intentionally excluded in the new implementation.

Suggested change

overhead_us += radio_rx_chain_delay_get(lll->phy, PHY_FLAGS_S8);

overhead_us += radio_rx_chain_delay_get(lll->phy, PHY_FLAGS_S8);

/* Add address timing overhead */

overhead_us += addr_us_get(lll->phy);

Intentionally excluded, use of duration until address is incorrect; only overhead duration between last bit on-air to first bit on-air be calculated.

Copilot

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-04T04:25:53Z

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

+		overhead_us += HAL_RADIO_ISR_LATENCY_MAX_US;
+		/* Add chain delay overhead */
+		overhead_us += radio_rx_chain_delay_get(lll->phy, PHY_FLAGS_S8);
+		/* Add active clock jitter overhead per subevent */


The comment says 'per subevent', but the code adds a single 2×EVENT_CLOCK_JITTER_US term independent of the number of subevents. Either reword the comment (e.g., 'Add base active clock jitter overhead') or, if the intent was to account per subevent, incorporate the subevent factor explicitly.

Suggested change

/* Add active clock jitter overhead per subevent */

/* Add base active clock jitter overhead */

Copilot · 2025-10-04T04:25:54Z

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

 		LL_ASSERT(EVENT_IFS_US > overhead_us);

+		/* Max. available jitter */
 		jitter_max_us = (EVENT_IFS_US - overhead_us) >> 1;


[nitpick] Using a bit shift to divide by 2 reduces readability with no performance benefit on modern compilers. Prefer an explicit division: jitter_max_us = (EVENT_IFS_US - overhead_us) / 2;

Suggested change

jitter_max_us = (EVENT_IFS_US - overhead_us) >> 1;

jitter_max_us = (EVENT_IFS_US - overhead_us) / 2;

cvinayak · 2025-10-04T04:26:08Z

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

+		/* Required radio ready duration, settling time */
+		overhead_us = radio_rx_ready_delay_get(lll->phy, PHY_FLAGS_S8);
+		/* If single timer used, then consider required max. latency */
+		overhead_us += HAL_RADIO_ISR_LATENCY_MAX_US;


Previous use of MAX here was incorrect, the required HAL_RADIO_ISR_LATENCY_MAX_US is addition to required radio_rx_ready_delay_get in order to have radio ready when using single timer for packet and tIFS swtich timer in nRF54Lx.

Test that covers the change in this PR is in #75760, which checks for drifting bap_broadcast_assistant ACL over BIG sync events, exposes the use of higher jitter duration needed when later subevents are required to be received.

Fix implementation of Broadcast ISO Synchronized Receiver using single switch timer to consider minimum compare value requirement. This fix reduces latencies to setup radio receptions and fixes an assertion in lll_sync_iso when radio_tmr_start_us() is checked for latencies. Relates to commit 5dfc58c ("Bluetooth: Controller: Fix single switch timer minimum compare value"). Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Copilot

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-04T04:56:58Z

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

+		/* Max. available clock jitter */
 		jitter_max_us = (EVENT_IFS_US - overhead_us) >> 1;
+		/* Max. clock jitter per subevent */
 		jitter_max_us = (jitter_max_us * nse) / (lll->num_bis * lll->nse);
-		overhead_us = HAL_RADIO_TMR_START_DELAY_US;
-		if (jitter_max_us > overhead_us) {
-			jitter_max_us -= overhead_us;
-		} else {
-			jitter_max_us = 0U;
-		}
+		/* Min. clock jitter we shall use */
+		jitter_max_us = MAX(jitter_max_us, (EVENT_CLOCK_JITTER_US << 1));

+		/* Jitter for current subevent */
 		jitter_us = (EVENT_CLOCK_JITTER_US << 1) * nse;
 		if (jitter_us > jitter_max_us) {


"Raising a variable named jitter_max_us with MAX(...) can exceed the true available jitter budget derived from EVENT_IFS_US - overhead_us, potentially allowing jitter_us to surpass the timing slack and causing missed radio timings. Instead, keep jitter_max_us as the budget ceiling and enforce the minimum on jitter_us (if needed) or remove the minimum entirely. For example: compute jitter_max_us on lines 1367–1369 as-is, then clamp jitter_us with both lower and upper bounds:

Copilot · 2025-10-04T04:56:58Z

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

+		/* Overhead within EVENT_IFS_US to exclude from max. jitter */
+		/* Required radio ready duration, settling time */
+		overhead_us = radio_rx_ready_delay_get(lll->phy, PHY_FLAGS_S8);
+		/* If single timer used, then consider required max. latency */


The comment states this addition applies 'If single timer used,' but the code unconditionally adds HAL_RADIO_ISR_LATENCY_MAX_US. Either guard this with the appropriate build-time/runtime condition for single-switch timer, or update the comment to reflect that this latency is always considered here.

Suggested change

/* If single timer used, then consider required max. latency */

/* Always consider required max. latency (HAL_RADIO_ISR_LATENCY_MAX_US) */

Copilot · 2025-10-04T04:56:58Z

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

 		jitter_max_us = (jitter_max_us * nse) / (lll->num_bis * lll->nse);
-		overhead_us = HAL_RADIO_TMR_START_DELAY_US;
-		if (jitter_max_us > overhead_us) {
-			jitter_max_us -= overhead_us;
-		} else {
-			jitter_max_us = 0U;
-		}
+		/* Min. clock jitter we shall use */
+		jitter_max_us = MAX(jitter_max_us, (EVENT_CLOCK_JITTER_US << 1));

+		/* Jitter for current subevent */
 		jitter_us = (EVENT_CLOCK_JITTER_US << 1) * nse;


[nitpick] Using both nse (local) and lll->nse (struct field) in the same expression is confusing. Consider renaming the local nse to something like subevent_idx or adding an inline comment clarifying that nse here is the current subevent index while lll->nse is the total number of subevents.

sonarqubecloud · 2025-10-04T04:58:47Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

cvinayak force-pushed the github_iso_sync_single_timer_fix branch 2 times, most recently from 0ca5b96 to 826c113 Compare September 27, 2025 16:06

cvinayak marked this pull request as ready for review September 27, 2025 17:10

zephyrbot added area: Bluetooth Controller area: Bluetooth labels Sep 27, 2025

zephyrbot requested review from Tronil, carlescufi, erbr-ot, ppryga, thoh-ot and wopu-ot September 27, 2025 17:11

zephyrbot assigned cvinayak Sep 27, 2025

cvinayak requested a review from Copilot September 28, 2025 19:53

Copilot AI reviewed Sep 28, 2025

View reviewed changes

cvinayak requested a review from Thalley October 3, 2025 03:49

cvinayak added this to the v4.3.0 milestone Oct 3, 2025

cvinayak added the bug The issue is a bug, or the PR is fixing a bug label Oct 3, 2025

Thalley previously approved these changes Oct 3, 2025

View reviewed changes

cvinayak dismissed Thalley’s stale review via 36f1766 October 4, 2025 04:23

cvinayak force-pushed the github_iso_sync_single_timer_fix branch from 826c113 to 36f1766 Compare October 4, 2025 04:23

cvinayak requested a review from Copilot October 4, 2025 04:23

Copilot AI reviewed Oct 4, 2025

View reviewed changes

cvinayak commented Oct 4, 2025

View reviewed changes

cvinayak force-pushed the github_iso_sync_single_timer_fix branch from 36f1766 to cca9d4e Compare October 4, 2025 04:53

cvinayak requested a review from Copilot October 4, 2025 04:55

Copilot AI reviewed Oct 4, 2025

View reviewed changes

Thalley approved these changes Oct 4, 2025

View reviewed changes

carlescufi approved these changes Oct 8, 2025

View reviewed changes

cfriedt merged commit 8eae0bf into zephyrproject-rtos:main Oct 8, 2025
28 checks passed

cvinayak mentioned this pull request Oct 30, 2025

Bluetooth: Controller: Place holder for a list of known assertions #82399

Open

15 tasks

	/* Add active clock jitter overhead per subevent */
	/* Add base active clock jitter overhead */

	jitter_max_us = (EVENT_IFS_US - overhead_us) >> 1;
	jitter_max_us = (EVENT_IFS_US - overhead_us) / 2;

	/* If single timer used, then consider required max. latency */
	/* Always consider required max. latency (HAL_RADIO_ISR_LATENCY_MAX_US) */

Bluetooth: Controller: Fix single switch timer use in ISO Sync #96653

Bluetooth: Controller: Fix single switch timer use in ISO Sync #96653

Uh oh!

Conversation

cvinayak commented Sep 27, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

cvinayak Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

cvinayak Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Oct 4, 2025

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants