Skip to content

Bluetooth: Controller: Fix device address in Periodic Adv Sync #84908

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
Feb 12, 2025
Merged

Bluetooth: Controller: Fix device address in Periodic Adv Sync #84908

Show file tree
Hide file tree
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
87 changes: 75 additions & 12 deletions subsys/bluetooth/controller/ll_sw/ull_scan_aux.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,19 +444,30 @@ void ull_scan_aux_setup(memq_link_t *link, struct node_rx_pdu *rx)

ptr = h->data;

#if defined(CONFIG_BT_CTLR_SYNC_PERIODIC)
bool is_aux_addr_match = false;
#endif /* CONFIG_BT_CTLR_SYNC_PERIODIC */

if (h->adv_addr) {
#if defined(CONFIG_BT_CTLR_SYNC_PERIODIC)
/* Check if Periodic Advertising Synchronization to be created
*/
if (sync && (scan->periodic.state != LL_SYNC_STATE_CREATED)) {
/* Check address and update internal state */
#if defined(CONFIG_BT_CTLR_PRIVACY)
ull_sync_setup_addr_check(sync, scan, pdu->tx_addr, ptr,
ftr->rl_idx);
uint8_t rl_idx = ftr->rl_idx;
#else /* !CONFIG_BT_CTLR_PRIVACY */
ull_sync_setup_addr_check(sync, scan, pdu->tx_addr, ptr, 0U);
uint8_t rl_idx = 0U;
#endif /* !CONFIG_BT_CTLR_PRIVACY */

/* Check address and update internal state */
is_aux_addr_match =
ull_sync_setup_addr_check(sync, scan->periodic.filter_policy,
pdu->tx_addr, ptr, rl_idx);
if (is_aux_addr_match) {
scan->periodic.state = LL_SYNC_STATE_ADDR_MATCH;
} else {
scan->periodic.state = LL_SYNC_STATE_IDLE;
}
}
#endif /* CONFIG_BT_CTLR_SYNC_PERIODIC */

Expand Down Expand Up @@ -489,14 +500,21 @@ void ull_scan_aux_setup(memq_link_t *link, struct node_rx_pdu *rx)
si = (void *)ptr;
ptr += sizeof(*si);

#if defined(CONFIG_BT_CTLR_SYNC_PERIODIC)
/* Check if Periodic Advertising Synchronization to be created.
* Setup synchronization if address and SID match in the
* Periodic Advertiser List or with the explicitly supplied.
*
* is_aux_addr_match, device address in auxiliary channel PDU;
* scan->periodic.param has not been assigned yet.
* Otherwise, address was in primary channel PDU and we are now
* checking SID (in SyncInfo) in auxiliary channel PDU.
*/
if (IS_ENABLED(CONFIG_BT_CTLR_SYNC_PERIODIC) && aux && sync && adi &&
if (sync && aux && (is_aux_addr_match || (scan->periodic.param == aux)) && adi &&
ull_sync_setup_sid_match(sync, scan, PDU_ADV_ADI_SID_GET(adi))) {
ull_sync_setup(scan, aux->lll.phy, rx, si);
}
#endif /* CONFIG_BT_CTLR_SYNC_PERIODIC */
}

if (h->tx_pwr) {
Expand Down Expand Up @@ -692,6 +710,15 @@ void ull_scan_aux_setup(memq_link_t *link, struct node_rx_pdu *rx)
#endif /* CONFIG_BT_CTLR_JIT_SCHEDULING */

#if defined(CONFIG_BT_CTLR_SYNC_PERIODIC)
/* Store the aux context that has Periodic Advertising
* Synchronization address match.
*/
if (sync && (scan->periodic.state == LL_SYNC_STATE_ADDR_MATCH)) {
scan->periodic.param = aux;
}

/* Store the node rx allocated for incomplete report, if needed.
*/
aux->rx_incomplete = rx_incomplete;
rx_incomplete = NULL;
#endif /* CONFIG_BT_CTLR_SYNC_PERIODIC */
Expand Down Expand Up @@ -916,6 +943,7 @@ void ull_scan_aux_setup(memq_link_t *link, struct node_rx_pdu *rx)
#if defined(CONFIG_BT_CTLR_SYNC_PERIODIC)
if (sync && (scan->periodic.state != LL_SYNC_STATE_CREATED)) {
scan->periodic.state = LL_SYNC_STATE_IDLE;
scan->periodic.param = NULL;
}
#endif /* CONFIG_BT_CTLR_SYNC_PERIODIC */

Expand Down Expand Up @@ -1518,7 +1546,16 @@ static void ticker_op_cb(uint32_t status, void *param)
}

#else /* CONFIG_BT_CTLR_SCAN_AUX_USE_CHAINS */

/* NOTE: BT_CTLR_SCAN_AUX_USE_CHAINS is alternative new design with less RAM
* usage for supporting Extended Scanning of simultaneous interleaved
* Extended Advertising chains.
*
* TODO: As the previous design has Bluetooth Qualified Design Listing by
* Nordic Semiconductor ASA, both implementation are present in this file,
* and default builds use CONFIG_BT_CTLR_SCAN_AUX_USE_CHAINS=n. Remove old
* implementation when we have a new Bluetooth Qualified Design Listing
* with the new Extended Scanning and Periodic Sync implementation.
Comment on lines +1553 to +1557
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless there are any specific plans to qualify the Zephyr controller again by anyone, I think we can just remove the old implementation right? The existing qualification is for a much older version of Zephyr, and cannot be used with this version anyhow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless there are any specific plans to qualify the Zephyr controller again by anyone, I think we can just remove the old implementation right?

We do not have information about users of the Zephyr Controller doing/planning qualified end product listing of their downstream fork applications.

The existing qualification is for a much older version of Zephyr, and cannot be used with this version anyhow

Qualified design listing applies to a design and as long as the design remains same for the said finite feature set (ICS), newer versions of the implementation tested using the same test plan are listed under the same listing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I get your point, but I still feel like this TODO is out of place. We are modifying existing features in both the host and controller all the time, and using a design listing for Zephyr 2.3 with Zephyr 4.0 would, even without removing this specific old implementation, likely not go well. The design listing is specifically for 2.2 and 2.3, so I don't think we need to care about that for 4.1 and future releases.

We do not have information about users of the Zephyr Controller doing/planning qualified end product listing of their downstream fork applications.

Then that's up to them. We can deprecate the old implementation by following the deprecation process in Zephyr, and then it will be up to downstream forks to determine whether they want to follow this, fork, or revert the removal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The design listing is specifically for 2.2 and 2.3, so I don't think we need to care about that for 4.1 and future releases.

From https://docs.nordicsemi.com/bundle/comp_matrix_nrf52833/page/COMP/nrf52833/nrf52833_ble_qdid_qual_matrix.html

"The Zephyr™ Controller subsystem was qualified with QDID 150092 for nRF52833 and nRF Connect SDK 1.3.2 for core specification 5.1. Other qualifications for the Zephyr Controller subsystem are not planned."

This is a revision in the Zephyr v3.x release (v3.2.99) leading to the v3.7.0 LTS. We do not deprecate a design and go back to experimental as default, but deprecate a design when there is equivalent qualified design as default replacing it.
Example, #49573

@Tronil Do we know a newer design listing with the chain implementation, and shall we switch the default design upstream?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know a newer design listing with the chain implementation, and shall we switch the default design upstream?

I don't know of one (but I really only know what we have qualified here downstream). I can say that we regularly run EBQ on our downstream version that uses the new design and haven't had any failures.

In my opinion we can switch the default - I don't know what the usual procedure is wrt. qualifications and deprecations though, so I'll refrain from having an opinion on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

regularly run EBQ on our downstream version that uses the new design and haven't had any failures.

Sufficient to create this #85209 and #85210.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jakkra FYI.

*/
void ull_scan_aux_setup(memq_link_t *link, struct node_rx_pdu *rx)
{
struct node_rx_pdu *rx_incomplete;
Expand Down Expand Up @@ -1778,19 +1815,30 @@ void ull_scan_aux_setup(memq_link_t *link, struct node_rx_pdu *rx)

ptr = h->data;

#if defined(CONFIG_BT_CTLR_SYNC_PERIODIC)
bool is_aux_addr_match = false;
#endif /* CONFIG_BT_CTLR_SYNC_PERIODIC */

if (h->adv_addr) {
#if defined(CONFIG_BT_CTLR_SYNC_PERIODIC)
/* Check if Periodic Advertising Synchronization to be created
*/
if (sync && (scan->periodic.state != LL_SYNC_STATE_CREATED)) {
/* Check address and update internal state */
#if defined(CONFIG_BT_CTLR_PRIVACY)
ull_sync_setup_addr_check(sync, scan, pdu->tx_addr, ptr,
ftr->rl_idx);
uint8_t rl_idx = ftr->rl_idx;
#else /* !CONFIG_BT_CTLR_PRIVACY */
ull_sync_setup_addr_check(sync, scan, pdu->tx_addr, ptr, 0U);
uint8_t rl_idx = 0U;
#endif /* !CONFIG_BT_CTLR_PRIVACY */

/* Check address and update internal state */
is_aux_addr_match =
ull_sync_setup_addr_check(sync, scan->periodic.filter_policy,
pdu->tx_addr, ptr, rl_idx);
if (is_aux_addr_match) {
scan->periodic.state = LL_SYNC_STATE_ADDR_MATCH;
} else {
scan->periodic.state = LL_SYNC_STATE_IDLE;
}
}
#endif /* CONFIG_BT_CTLR_SYNC_PERIODIC */

Expand Down Expand Up @@ -1823,14 +1871,21 @@ void ull_scan_aux_setup(memq_link_t *link, struct node_rx_pdu *rx)
si = (void *)ptr;
ptr += sizeof(*si);

#if defined(CONFIG_BT_CTLR_SYNC_PERIODIC)
/* Check if Periodic Advertising Synchronization to be created.
* Setup synchronization if address and SID match in the
* Periodic Advertiser List or with the explicitly supplied.
*
* is_aux_addr_match, device address in auxiliary channel PDU;
* scan->periodic.param has not been assigned yet.
* Otherwise, address was in primary channel PDU and we are now
* checking SID (in SyncInfo) in auxiliary channel PDU.
*/
if (IS_ENABLED(CONFIG_BT_CTLR_SYNC_PERIODIC) && chain && sync && adi &&
ull_sync_setup_sid_match(sync, scan, PDU_ADV_ADI_SID_GET(adi))) {
if (sync && chain && (is_aux_addr_match || (scan->periodic.param == chain)) &&
adi && ull_sync_setup_sid_match(sync, scan, PDU_ADV_ADI_SID_GET(adi))) {
ull_sync_setup(scan, chain->lll.phy, rx, si);
}
#endif /* CONFIG_BT_CTLR_SYNC_PERIODIC */
}

if (h->tx_pwr) {
Expand Down Expand Up @@ -2012,6 +2067,13 @@ void ull_scan_aux_setup(memq_link_t *link, struct node_rx_pdu *rx)
#endif /* CONFIG_BT_CTLR_JIT_SCHEDULING */

#if defined(CONFIG_BT_CTLR_SYNC_PERIODIC)
/* Store the chain context that has Periodic Advertising
* Synchronization address match.
*/
if (sync && (scan->periodic.state == LL_SYNC_STATE_ADDR_MATCH)) {
scan->periodic.param = chain;
}

if (sync_lll) {
struct ll_sync_set *sync_set = HDR_LLL2ULL(sync_lll);

Expand Down Expand Up @@ -2153,6 +2215,7 @@ void ull_scan_aux_setup(memq_link_t *link, struct node_rx_pdu *rx)
#if defined(CONFIG_BT_CTLR_SYNC_PERIODIC)
if (sync && (scan->periodic.state != LL_SYNC_STATE_CREATED)) {
scan->periodic.state = LL_SYNC_STATE_IDLE;
scan->periodic.param = NULL;
}
#endif /* CONFIG_BT_CTLR_SYNC_PERIODIC */

Expand Down
5 changes: 5 additions & 0 deletions subsys/bluetooth/controller/ll_sw/ull_scan_types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ struct ll_scan_set {
* cancelling sync create, hence the volatile keyword.
*/
struct ll_sync_set *volatile sync;

/* Non-NULL when Periodic Advertising Synchronisation address
* matched.
*/
void *param;
} periodic;
#endif
};
Expand Down
19 changes: 13 additions & 6 deletions subsys/bluetooth/controller/ll_sw/ull_sync.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,11 +152,13 @@ uint8_t ll_sync_create(uint8_t options, uint8_t sid, uint8_t adv_addr_type,

scan->periodic.cancelled = 0U;
scan->periodic.state = LL_SYNC_STATE_IDLE;
scan->periodic.param = NULL;
scan->periodic.filter_policy =
options & BT_HCI_LE_PER_ADV_CREATE_SYNC_FP_USE_LIST;
if (IS_ENABLED(CONFIG_BT_CTLR_PHY_CODED)) {
scan_coded->periodic.cancelled = 0U;
scan_coded->periodic.state = LL_SYNC_STATE_IDLE;
scan_coded->periodic.param = NULL;
scan_coded->periodic.filter_policy =
scan->periodic.filter_policy;
}
Expand Down Expand Up @@ -880,12 +882,12 @@ void ull_sync_release(struct ll_sync_set *sync)
mem_release(sync, &sync_free);
}

void ull_sync_setup_addr_check(struct ll_sync_set *sync, struct ll_scan_set *scan,
bool ull_sync_setup_addr_check(struct ll_sync_set *sync, uint8_t filter_policy,
uint8_t addr_type, uint8_t *addr, uint8_t rl_idx)
{
/* Check if Periodic Advertiser list to be used */
if (IS_ENABLED(CONFIG_BT_CTLR_SYNC_PERIODIC_ADV_LIST) &&
scan->periodic.filter_policy) {
filter_policy) {
/* Check in Periodic Advertiser List */
if (ull_filter_ull_pal_addr_match(addr_type, addr)) {
/* Remember the address, to check with
Expand All @@ -896,7 +898,7 @@ void ull_sync_setup_addr_check(struct ll_sync_set *sync, struct ll_scan_set *sca
BDADDR_SIZE);

/* Address matched */
scan->periodic.state = LL_SYNC_STATE_ADDR_MATCH;
return true;

/* Check in Resolving List */
} else if (IS_ENABLED(CONFIG_BT_CTLR_PRIVACY) &&
Expand All @@ -911,14 +913,14 @@ void ull_sync_setup_addr_check(struct ll_sync_set *sync, struct ll_scan_set *sca
sync->peer_addr_resolved = 1U;

/* Address matched */
scan->periodic.state = LL_SYNC_STATE_ADDR_MATCH;
return true;
}

/* Check with explicitly supplied address */
} else if ((addr_type == sync->peer_id_addr_type) &&
!memcmp(addr, sync->peer_id_addr, BDADDR_SIZE)) {
/* Address matched */
scan->periodic.state = LL_SYNC_STATE_ADDR_MATCH;
return true;

/* Check identity address with explicitly supplied address */
} else if (IS_ENABLED(CONFIG_BT_CTLR_PRIVACY) &&
Expand All @@ -930,9 +932,11 @@ void ull_sync_setup_addr_check(struct ll_sync_set *sync, struct ll_scan_set *sca
sync->peer_addr_resolved = 1U;

/* Identity address matched */
scan->periodic.state = LL_SYNC_STATE_ADDR_MATCH;
return true;
}
}

return false;
}

bool ull_sync_setup_sid_match(struct ll_sync_set *sync, struct ll_scan_set *scan, uint8_t sid)
Expand Down Expand Up @@ -1060,6 +1064,7 @@ void ull_sync_setup(struct ll_scan_set *scan, uint8_t phy,

/* Set the state to sync create */
scan->periodic.state = LL_SYNC_STATE_CREATED;
scan->periodic.param = NULL;
if (IS_ENABLED(CONFIG_BT_CTLR_PHY_CODED)) {
struct ll_scan_set *scan_1m;

Expand All @@ -1069,8 +1074,10 @@ void ull_sync_setup(struct ll_scan_set *scan, uint8_t phy,

scan_coded = ull_scan_set_get(SCAN_HANDLE_PHY_CODED);
scan_coded->periodic.state = LL_SYNC_STATE_CREATED;
scan_coded->periodic.param = NULL;
} else {
scan_1m->periodic.state = LL_SYNC_STATE_CREATED;
scan_1m->periodic.param = NULL;
}
}

Expand Down
2 changes: 1 addition & 1 deletion subsys/bluetooth/controller/ll_sw/ull_sync_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ int ull_sync_reset(void);
uint16_t ull_sync_handle_get(struct ll_sync_set *sync);
struct ll_sync_set *ull_sync_is_enabled_get(uint16_t handle);
void ull_sync_release(struct ll_sync_set *sync);
void ull_sync_setup_addr_check(struct ll_sync_set *sync, struct ll_scan_set *scan,
bool ull_sync_setup_addr_check(struct ll_sync_set *sync, uint8_t filter_policy,
uint8_t addr_type, uint8_t *addr, uint8_t rl_idx);
bool ull_sync_setup_sid_match(struct ll_sync_set *sync, struct ll_scan_set *scan, uint8_t sid);
void ull_sync_create_from_sync_transfer(uint16_t conn_handle, uint16_t service_data,
Expand Down
Loading