Skip to content

Conversation

cvinayak
Copy link
Contributor

Fix incorrect device address reported in the LE Periodic Advertising Sync Established event when using Periodic Advertiser List.

@cvinayak cvinayak force-pushed the github_sync_accept_list_addr_match_fix branch 3 times, most recently from 8d95a5b to 076d213 Compare January 30, 2025 19:20
@cvinayak cvinayak added backport v3.7-branch Request backport to the v3.7-branch backport v4.0-branch Backport to the v4.0-branch labels Jan 30, 2025
@cvinayak cvinayak force-pushed the github_sync_accept_list_addr_match_fix branch 3 times, most recently from 151b8b4 to be90e62 Compare January 31, 2025 08:59
@cvinayak cvinayak marked this pull request as ready for review January 31, 2025 13:34
@cvinayak
Copy link
Contributor Author

cvinayak commented Jan 31, 2025

@Tronil FYI, a bug reported by @jakkra in a private devzone.nordicsemi.com ticket.

A draft nRF Connect SDK v2.2.0 backport pull requests here: nrfconnect/sdk-nrf#20142

@cvinayak cvinayak requested a review from Thalley February 4, 2025 04:59
Comment on lines 1553 to 1555
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the TODO here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated comment.

Fix incorrect device address reported in the LE Periodic
Advertising Sync Established event when using Periodic
Advertiser List.

During Extended Scanning there can be an ADV_EXT_IND PDU
received between currently being received ADV_EXT_IND PDU
and AUX_ADV_IND PDU; if the one received between has an
address match then incorrectly the Periodic Synchronization
was established to the device whos AUX_ADV_IND PDU is being
received. Fix by storing the auxiliary context that has the
address match and compare with it when matching the SID in
SyncInfo of AUX_ADV_IND PDU being received prior to creating
the synchronization.

Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
@cvinayak cvinayak force-pushed the github_sync_accept_list_addr_match_fix branch from be90e62 to 7c351a5 Compare February 4, 2025 20:12
Comment on lines +1553 to +1557
* 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.
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.

@kartben kartben merged commit 83e2ec3 into zephyrproject-rtos:main Feb 12, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Bluetooth Controller area: Bluetooth backport v3.7-branch Request backport to the v3.7-branch backport v4.0-branch Backport to the v4.0-branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants