Skip to content

Bluetooth: Controller: nrf54lx: Fix address resolution wait #100214

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+19 −6
Open

Bluetooth: Controller: nrf54lx: Fix address resolution wait #100214

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
25 changes: 19 additions & 6 deletions subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2723,6 +2723,23 @@ void radio_ar_status_reset(void)
hal_radio_nrf_ppi_channels_disable(BIT(HAL_TRIGGER_AAR_PPI));
}

static void ar_end_wait(void)
{
/* An arbitrary iteration which seems sufficient testing on target with one IRK to resolve.
* We should ideally be not be waiting anyway, and in theory be resolved before we are here.
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

Grammatical error: "be not be waiting" should be "not be waiting".

Suggested change
* We should ideally be not be waiting anyway, and in theory be resolved before we are here.
* We should ideally not be waiting anyway, and in theory be resolved before we are here.

Copilot uses AI. Check for mistakes.
*/
uint32_t countdown = 200U;
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

The magic number 200U for the countdown is arbitrary and lacks proper justification. While the comment mentions it "seems sufficient testing on target with one IRK to resolve," this doesn't provide guidance for:

  1. What happens if multiple IRKs need to be resolved
  2. How this value scales with different hardware configurations
  3. Whether timeout detection/handling is needed when countdown reaches 0

Consider either:

  • Defining this as a named constant (e.g., #define AAR_WAIT_COUNTDOWN_MAX 200U) with documentation explaining the expected timing
  • Adding timeout error handling/logging when countdown reaches 0
  • Documenting the expected maximum duration based on hardware specifications

Copilot uses AI. Check for mistakes.

while ((NRF_AAR->EVENTS_END == 0U) && (countdown != 0U)) {
countdown--;

#if !defined(CONFIG_SOC_SERIES_NRF54LX)
/* We do not sleep on target SoC. */
cpu_sleep();
#endif /* !CONFIG_SOC_SERIES_NRF54LX */
Comment on lines +2736 to +2739
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

The conditional compilation check uses CONFIG_SOC_SERIES_NRF54LX, but throughout the rest of this file, the consistent pattern is to use CONFIG_SOC_COMPATIBLE_NRF54LX (as seen in lines 2645, 2689, 2709, 2711, 2771, 2787, etc.). This inconsistency could lead to incorrect behavior where cpu_sleep() is called (or not called) on the wrong platforms.

Suggested fix:

#if !defined(CONFIG_SOC_COMPATIBLE_NRF54LX)
	/* We do not sleep on target SoC. */
	cpu_sleep();
#endif /* !CONFIG_SOC_COMPATIBLE_NRF54LX */
Suggested change
#if !defined(CONFIG_SOC_SERIES_NRF54LX)
/* We do not sleep on target SoC. */
cpu_sleep();
#endif /* !CONFIG_SOC_SERIES_NRF54LX */
#if !defined(CONFIG_SOC_COMPATIBLE_NRF54LX)
/* We do not sleep on target SoC. */
cpu_sleep();
#endif /* !CONFIG_SOC_COMPATIBLE_NRF54LX */

Copilot uses AI. Check for mistakes.
}
}
Comment on lines +2726 to +2741
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

Missing error handling or status return when the countdown expires. If NRF_AAR->EVENTS_END remains 0 after the countdown completes, the calling code has no way to detect this timeout condition. This could mask hardware issues or unexpected delays.

Consider:

  • Returning a status code from ar_end_wait() to indicate success/timeout
  • Adding an assertion or error log when the timeout occurs
  • Documenting the expected behavior when timeout happens

Copilot uses AI. Check for mistakes.

uint32_t radio_ar_has_match(void)
{
if (!radio_bc_has_match()) {
Expand All @@ -2731,9 +2748,7 @@ uint32_t radio_ar_has_match(void)

nrf_aar_int_enable(NRF_AAR, AAR_INTENSET_END_Msk);

while (NRF_AAR->EVENTS_END == 0U) {
cpu_sleep();
}
ar_end_wait();

nrf_aar_int_disable(NRF_AAR, AAR_INTENCLR_END_Msk);

Expand Down Expand Up @@ -2783,9 +2798,7 @@ uint8_t radio_ar_resolve(const uint8_t *addr)

nrf_aar_task_trigger(NRF_AAR, NRF_AAR_TASK_START);

while (NRF_AAR->EVENTS_END == 0) {
cpu_sleep();
}
ar_end_wait();

nrf_aar_int_disable(NRF_AAR, AAR_INTENCLR_END_Msk);

Expand Down