Skip to content
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.

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

Bluetooth: Host: Fix Periodic Advertising Sync using Advertiser Lists #38594

Merged

Conversation

cvinayak
Copy link
Contributor

Fix Periodic Advertising Sync Establishment to accept
synchronization establishment to device listed in the
Periodic Advertisers List when filter policy was used.

Fixes #38520.

Signed-off-by: Vinayak Kariappa Chettimada [email protected]

@cvinayak cvinayak requested a review from Thalley September 16, 2021 10:47
@cvinayak cvinayak added the bug The issue is a bug, or the PR is fixing a bug label Sep 16, 2021
@cvinayak cvinayak added this to the v2.7.0 milestone Sep 16, 2021
@cvinayak cvinayak force-pushed the github_per_adv_list_fix branch from cfe9de5 to c45ec77 Compare September 16, 2021 11:07
@cvinayak
Copy link
Contributor Author

@saleh-unikie please check if this PR in combination with #38338 fixes #38520

@saleh-unikie
Copy link
Contributor

@saleh-unikie please check if this PR in combination with #38338 fixes #38520

Yes, Thank you!

Copy link
Collaborator

@Thalley Thalley left a comment

Choose a reason for hiding this comment

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

Looks correct, but a few minor requests

Comment on lines 697 to 700
(!atomic_test_bit(pending_per_adv_sync->flags,
BT_PER_ADV_SYNC_USE_LIST) &&
((pending_per_adv_sync->sid != evt->sid) ||
bt_addr_le_cmp(&pending_per_adv_sync->addr, &evt->adv_addr)))) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The check here is correct, although I wish we could make it a bit more readable, but I don't have any suggestions.

There's a little bit of indentation that should be fixed

Suggested change
(!atomic_test_bit(pending_per_adv_sync->flags,
BT_PER_ADV_SYNC_USE_LIST) &&
((pending_per_adv_sync->sid != evt->sid) ||
bt_addr_le_cmp(&pending_per_adv_sync->addr, &evt->adv_addr)))) {
(!atomic_test_bit(pending_per_adv_sync->flags,
BT_PER_ADV_SYNC_USE_LIST) &&
((pending_per_adv_sync->sid != evt->sid) ||
bt_addr_le_cmp(&pending_per_adv_sync->addr, &evt->adv_addr)))) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only line 698 was missing a whitespace, fixed.

Comment on lines 159 to 161
/** Periodic Advertising Sync Create using Advertiser List */
BT_PER_ADV_SYNC_USE_LIST,

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/** Periodic Advertising Sync Create using Advertiser List */
BT_PER_ADV_SYNC_USE_LIST,
/** Periodic advertising is attempting sync sync */ using Advertiser List */
BT_PER_ADV_SYNC_SYNCING_USE_LIST,

And move next to the BT_PER_ADV_SYNC_SYNCING flag as those are the most related flags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update this and other comments.

Fix Periodic Advertising Sync Establishment to accept
synchronization establishment to device listed in the
Periodic Advertisers List when filter policy was used.

Fixes zephyrproject-rtos#38520.

Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
@carlescufi carlescufi merged commit 9e2b9c7 into zephyrproject-rtos:main Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Bluetooth Host area: Bluetooth bug The issue is a bug, or the PR is fixing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bluetooth:Host:Scan: "bt_le_per_adv_list_add" function doesn't work
5 participants