Skip to content

Bluetooth: Controller: Fix missing ISO Receiver address capture #95348

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 2 commits into from
Sep 8, 2025
Merged

Bluetooth: Controller: Fix missing ISO Receiver address capture #95348

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1273947
Bluetooth: Controller: Fix ISO Sync Receiver subevent jitter
cvinayak Jan 3, 2025
ff7e93a
Bluetooth: Controller: Fix missing ISO Receiver address capture
cvinayak Sep 2, 2025
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
24 changes: 22 additions & 2 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 @@ -1301,6 +1301,13 @@ static void isr_rx(void *param)

radio_switch_complete_and_disable();

/* Setup Access Address capture for subsequent subevent if there has been no anchor point
* sync previously.
*/
if (radio_tmr_aa_restore() == 0U) {
radio_tmr_aa_capture();
}

/* PDU Header Complete TimeOut, calculate the absolute timeout in
* microseconds by when a PDU header is to be received for each
* subevent.
Expand Down Expand Up @@ -1344,16 +1351,30 @@ static void isr_rx(void *param)
hcto -= radio_rx_chain_delay_get(lll->phy, PHY_FLAGS_S8);
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_us += (EVENT_CLOCK_JITTER_US << 1);

LL_ASSERT(EVENT_IFS_US > overhead_us);
Copy link
Preview

Copilot AI Sep 3, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider adding descriptive error messages to these assertions to help with debugging when they fail. For example: LL_ASSERT_MSG(EVENT_IFS_US > overhead_us, \"IFS time insufficient for overhead\");

Suggested change
LL_ASSERT(EVENT_IFS_US > overhead_us);
LL_ASSERT_MSG(EVENT_IFS_US > overhead_us, "IFS time insufficient for overhead");

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do not want a message.


jitter_max_us = (EVENT_IFS_US - overhead_us) >> 1;
jitter_max_us -= RANGE_DELAY_US + HAL_RADIO_TMR_START_DELAY_US;
jitter_max_us = (jitter_max_us * nse) / (lll->num_bis * lll->nse);
Copy link
Preview

Copilot AI Sep 3, 2025

Choose a reason for hiding this comment

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

The expression can be simplified to jitter_max_us = jitter_max_us / lll->num_bis since nse appears in both numerator and denominator and cancels out.

Suggested change
jitter_max_us = (jitter_max_us * nse) / (lll->num_bis * lll->nse);
jitter_max_us = jitter_max_us / lll->num_bis;

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, nse and lll->nse are different values.

overhead_us = HAL_RADIO_TMR_START_DELAY_US;
if (jitter_max_us > overhead_us) {
jitter_max_us -= overhead_us;
} else {
jitter_max_us = 0U;
}

jitter_us = (EVENT_CLOCK_JITTER_US << 1) * nse;
if (jitter_us > jitter_max_us) {
jitter_us = jitter_max_us;
}

LL_ASSERT(hcto > jitter_us);

Comment on lines +1375 to +1377
Copy link
Contributor

Choose a reason for hiding this comment

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

Funny; was about to suggest this in my last review, but noticed that it wasn't there before either, so I didn't :) Glad to see it added

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I noticed that a newly added assert was caught in CI for high number of subevents which I had to convert to if-then-else; hence, decided to add a assert here as well.

Do not hesitate to leave a comment, your reviews help think differently, and hence better solutions.

That said, I do not like the calculations here for the jitter, it is always calculating for nse i.e. from the first subevent; ideally, current jitter should be from the last sync-ed subevent.

The problem to solve, active clock is permitted a +/-2 us jitter which means over the duration of the subevents, a subevent can jitter by +/- 2 us multiplied by current subevent count nse when none of the previous subevent where received (anchor point sync).

I will comeback and solve it some other day, for now, bsim is happy; my other changes does expose too little jitter or drift causing assertion... fix for that another day.

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider creating a GH issue for the above so you don't forget it ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not going to forget it, listed (updated with the above problem statement now) in #82399

hcto -= jitter_us;

start_us = hcto;
Expand All @@ -1365,7 +1386,6 @@ static void isr_rx(void *param)
* the current subevent we are listening.
*/
hcto += (jitter_us << 1);
hcto += RANGE_DELAY_US + HAL_RADIO_TMR_START_DELAY_US;
} else {
/* First subevent PDU was not received, hence setup radio packet
* timer header complete timeout from where the first subevent
Expand Down