Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 8, 2025
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 13 additions & 10 deletions subsys/bluetooth/controller/ll_sw/nordic/lll/lll_sync_iso.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1352,22 +1352,25 @@ static void isr_rx(void *param)
hcto -= addr_us_get(lll->phy);
hcto -= radio_rx_ready_delay_get(lll->phy, PHY_FLAGS_S8);

overhead_us = radio_rx_chain_delay_get(lll->phy, PHY_FLAGS_S8);
overhead_us += addr_us_get(lll->phy);
overhead_us += radio_rx_ready_delay_get(lll->phy, PHY_FLAGS_S8);
/* 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 */
Copy link

Copilot AI Oct 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
overhead_us += HAL_RADIO_ISR_LATENCY_MAX_US;
Copy link
Contributor Author

@cvinayak cvinayak Oct 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/* Add chain delay overhead */
overhead_us += radio_rx_chain_delay_get(lll->phy, PHY_FLAGS_S8);
Copy link

Copilot AI Sep 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@cvinayak cvinayak Sep 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/* Add base clock jitter overhead */
overhead_us += (EVENT_CLOCK_JITTER_US << 1);

LL_ASSERT(EVENT_IFS_US > overhead_us);

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

Copilot AI Oct 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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;

Copilot uses AI. Check for mistakes.
/* 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;
Comment on lines 1369 to 1374
Copy link

Copilot AI Oct 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copilot uses AI. Check for mistakes.
if (jitter_us > jitter_max_us) {
Comment on lines +1366 to 1375
Copy link

Copilot AI Oct 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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 uses AI. Check for mistakes.
jitter_us = jitter_max_us;
Expand Down