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

Option to overwrite a Receive State in the Scan Delegator #62609

Conversation

pmullt
Copy link

@pmullt pmullt commented Sep 13, 2023

Kconfig option CONFIG_SD_RECEIVE_STATE_OVERWRITE_OLDEST allows the Scan Delegator to overwrite the oldest updated receive state which is not being synced, if receive states are full.

There is also a recv_states_full() callback function added which can notify an application if all receive states are full and being synced.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello @pmullt, and thank you very much for your first pull request to the Zephyr project!

A project maintainer just triggered our CI pipeline to run it against your PR and ensure it's compliant and doesn't cause any issues. You might want to take this opportunity to review the project's Contributor Expectations and make any updates to your pull request if necessary. 😊

@pmullt pmullt force-pushed the bap_scan_delegator_recv_state_overwrite branch 4 times, most recently from b94a48a to dafbba7 Compare September 14, 2023 11:19
include/zephyr/bluetooth/audio/bap.h Outdated Show resolved Hide resolved
include/zephyr/bluetooth/audio/bap.h Outdated Show resolved Hide resolved
include/zephyr/bluetooth/audio/bap.h Outdated Show resolved Hide resolved
include/zephyr/bluetooth/audio/bap.h Outdated Show resolved Hide resolved
samples/bluetooth/broadcast_audio_sink/Kconfig Outdated Show resolved Hide resolved
samples/bluetooth/broadcast_audio_sink/Kconfig Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
@pmullt pmullt force-pushed the bap_scan_delegator_recv_state_overwrite branch from dafbba7 to a8e7c9c Compare September 20, 2023 16:44
include/zephyr/bluetooth/audio/bap.h Outdated Show resolved Hide resolved
include/zephyr/bluetooth/audio/bap.h Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
@pmullt pmullt force-pushed the bap_scan_delegator_recv_state_overwrite branch 3 times, most recently from e9638e0 to 0157c3e Compare September 26, 2023 15:25
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.

Nice work so far, but with some comments on the approach with the big as well as some formatting

subsys/bluetooth/audio/Kconfig.bap Outdated Show resolved Hide resolved
subsys/bluetooth/audio/Kconfig.bap Outdated Show resolved Hide resolved
include/zephyr/bluetooth/audio/bap.h Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
@carlescufi
Copy link
Member

@pmullt feature freeze is tomorrow. If you'd like to get this into Zephyr 3.5 please address all review comments before EOB tomorrow.

@pmullt pmullt force-pushed the bap_scan_delegator_recv_state_overwrite branch 2 times, most recently from cc966f4 to 8b395bf Compare September 28, 2023 14:46
@pmullt pmullt force-pushed the bap_scan_delegator_recv_state_overwrite branch 4 times, most recently from 9da387c to b1b66a0 Compare October 2, 2023 18:46
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.

A few more comments, but I think it's nearly there

include/zephyr/bluetooth/audio/bap.h Outdated Show resolved Hide resolved
include/zephyr/bluetooth/audio/bap.h Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_broadcast_sink.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
@pmullt pmullt force-pushed the bap_scan_delegator_recv_state_overwrite branch from b1b66a0 to ecc1f79 Compare October 16, 2023 18:36
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 pretty good now - Mostly some formatting comments

include/zephyr/bluetooth/audio/bap.h Outdated Show resolved Hide resolved
include/zephyr/bluetooth/audio/bap.h Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_broadcast_sink.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_broadcast_sink.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
Comment on lines +1342 to +1334
if (scan_delegator_cbs != NULL &&
scan_delegator_cbs->recv_state_add_failed != NULL) {
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
if (scan_delegator_cbs != NULL &&
scan_delegator_cbs->recv_state_add_failed != NULL) {
if (scan_delegator_cbs != NULL &&
scan_delegator_cbs->recv_state_add_failed != NULL) {

subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
@pmullt pmullt force-pushed the bap_scan_delegator_recv_state_overwrite branch from ecc1f79 to 76083d7 Compare October 18, 2023 09:12
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.

Mostly formatting.

Have you consider using something like clang-format to help your formatting?

subsys/bluetooth/audio/bap_broadcast_sink.c Outdated Show resolved Hide resolved
Comment on lines +396 to +394
if (state->pa_sync_state == BT_BAP_PA_STATE_SYNCED ||
state->pa_sync_state == BT_BAP_PA_STATE_INFO_REQ) {
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
if (state->pa_sync_state == BT_BAP_PA_STATE_SYNCED ||
state->pa_sync_state == BT_BAP_PA_STATE_INFO_REQ) {
if (state->pa_sync_state == BT_BAP_PA_STATE_SYNCED ||
state->pa_sync_state == BT_BAP_PA_STATE_INFO_REQ) {

subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
Comment on lines +405 to +402
if ((oldest_inactive_state == NULL) ||
(current_state->aging_counter < oldest_inactive_state->aging_counter)) {
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
if ((oldest_inactive_state == NULL) ||
(current_state->aging_counter < oldest_inactive_state->aging_counter)) {
if ((oldest_inactive_state == NULL) ||
(current_state->aging_counter < oldest_inactive_state->aging_counter)) {

subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
Comment on lines 1324 to 1317
(void)memcpy(&state_info.subgroups, &param->subgroups,
sizeof(state_info.subgroups));
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
(void)memcpy(&state_info.subgroups, &param->subgroups,
sizeof(state_info.subgroups));
(void)memcpy(&state_info.subgroups, &param->subgroups,
sizeof(state_info.subgroups));

Comment on lines +1381 to +1374
LOG_DBG("New source ID 0x%02x (%d) added to Scan Delegator at Index %u: ",
state->src_id, state->src_id, internal_state->index);
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
LOG_DBG("New source ID 0x%02x (%d) added to Scan Delegator at Index %u: ",
state->src_id, state->src_id, internal_state->index);
LOG_DBG("New source ID 0x%02x (%d) added to Scan Delegator at Index %u: ",
state->src_id, state->src_id, internal_state->index);

subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
@pmullt pmullt force-pushed the bap_scan_delegator_recv_state_overwrite branch from 76083d7 to 9c8437c Compare October 18, 2023 19:15
include/zephyr/bluetooth/audio/bap.h Outdated Show resolved Hide resolved
include/zephyr/bluetooth/audio/bap.h Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_broadcast_sink.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
@pmullt pmullt force-pushed the bap_scan_delegator_recv_state_overwrite branch from 9c8437c to be1ecf6 Compare October 24, 2023 09:56
@zephyrbot zephyrbot added the area: Samples Samples label Oct 24, 2023
@zephyrbot zephyrbot requested a review from kartben October 24, 2023 09:57
@pmullt pmullt force-pushed the bap_scan_delegator_recv_state_overwrite branch from be1ecf6 to 60c916e Compare October 24, 2023 10:05
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.

Still a few formatting issues.

Also, you've (accidentally?) resolved a few comments that weren't fixed.

include/zephyr/bluetooth/audio/bap.h Outdated Show resolved Hide resolved
include/zephyr/bluetooth/audio/bap.h Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
subsys/bluetooth/audio/bap_scan_delegator.c Outdated Show resolved Hide resolved
@pmullt pmullt force-pushed the bap_scan_delegator_recv_state_overwrite branch from 60c916e to 10b1548 Compare November 8, 2023 12:48
Kconfig option BT_BAP_SCAN_DELEGATOR_OVERWRITE_OLDEST
allows the Scan Delegator to overwrite the oldest inactive
receive state if there aren't any free receive states.

A inactive receive state is a receive state that has neither a
Periodic Advertising or Broadcast Isochronous Group (BIG) sync.

There is also a recv_state_add_fail callback function added
which can notify an application if all receive states
are not inactive and can't be freed.

Signed-off-by: Paul Mulligan <[email protected]>
@pmullt pmullt force-pushed the bap_scan_delegator_recv_state_overwrite branch from 10b1548 to a2f9362 Compare November 10, 2023 09:28
@Thalley
Copy link
Collaborator

Thalley commented Nov 10, 2023

@pmullt Looks like it needs a rebase :)

Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Jan 10, 2024
@kruithofa kruithofa removed their request for review January 19, 2024 09:58
@github-actions github-actions bot removed the Stale label Jan 20, 2024
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Mar 20, 2024
@github-actions github-actions bot closed this Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants