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.

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

Jump to bottom

Bluetooth: Audio: Rename bt_audio_codec_qos -> bt_bap_qos_cfg #76633

Merged
merged 1 commit into from
Oct 4, 2024
Merged

Bluetooth: Audio: Rename bt_audio_codec_qos -> bt_bap_qos_cfg #76633

merged 1 commit into from
Oct 4, 2024

Conversation

Thalley
Copy link
Collaborator

@Thalley Thalley commented Aug 2, 2024 •
edited
Loading

The QoS structure is not related to a codec, but rather a stream, and should thus not use the "Codec" name.

The BAP and ASCS specs refer to the QoS as
"QoS configuration" several places, so it is an obvious choice for a name.

Since the structure is defined and used by BAP, the prefix was changed from bt_audio to bt_bap.

fixes #72684

@Thalley Thalley marked this pull request as ready for review August 2, 2024 12:58
@Thalley Thalley assigned Thalley and unassigned jhedberg Aug 2, 2024
kruithofa
kruithofa reviewed Aug 9, 2024
View reviewed changes
include/zephyr/bluetooth/audio/bap.h Outdated
Comment on lines 252 to 264
* Unlike the other fields, this is not a preference but a minimum requirement.
*
* Value range 0 to @ref BT_AUDIO_PD_MAX, or @ref BT_AUDIO_PD_PREF_NONE
* to indicate no preference.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The documentation for the presentation delay fields seem wrong. Although fixing this is not strictly part of this PR it may as well be done here, instead of creating a separate PR for fixing it.

Suggested change
* Unlike the other fields, this is not a preference but a minimum requirement.
*
* Value range 0 to @ref BT_AUDIO_PD_MAX, or @ref BT_AUDIO_PD_PREF_NONE
* to indicate no preference.
* Unlike the other fields, this is not a preference but a minimum requirement.
*
* Value range 0 to @ref BT_AUDIO_PD_MIN

include/zephyr/bluetooth/audio/bap.h Outdated
/**
* @brief Preferred minimum Presentation Delay
*
* Value range 0 to @ref BT_AUDIO_PD_MAX.
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
* Value range 0 to @ref BT_AUDIO_PD_MAX.
* Value range pd_min to pref_pd_max, or @ref BT_AUDIO_PD_PREF_NONE
* to indicate no preference.

include/zephyr/bluetooth/audio/bap.h Outdated
Comment on lines 264 to 274
* Value range 0 to @ref BT_AUDIO_PD_MAX, or @ref BT_AUDIO_PD_PREF_NONE
* to indicate no preference.
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
* Value range 0 to @ref BT_AUDIO_PD_MAX, or @ref BT_AUDIO_PD_PREF_NONE
* to indicate no preference.
* Value range 0 to @ref BT_AUDIO_PD_MAX

include/zephyr/bluetooth/audio/bap.h Outdated
/**
* @brief Preferred maximum Presentation Delay
*
* Value range 0 to @ref BT_AUDIO_PD_MAX.
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
* Value range 0 to @ref BT_AUDIO_PD_MAX.
* Value range pref_pd_min to px_max, or @ref BT_AUDIO_PD_PREF_NONE
* to indicate no preference.

include/zephyr/bluetooth/audio/bap.h Outdated
uint32_t pd_min;

/**
* @brief Maximum Presentation Delay
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
* @brief Maximum Presentation Delay
* @brief Maximum Presentation Delay in microseconds

include/zephyr/bluetooth/audio/bap.h Outdated
uint32_t pd_max;

/**
* @brief Preferred minimum Presentation Delay
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
* @brief Preferred minimum Presentation Delay
* @brief Preferred minimum Presentation Delay in microseconds

include/zephyr/bluetooth/audio/bap.h Outdated
uint32_t pref_pd_min;

/**
* @brief Preferred maximum Presentation Delay
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
* @brief Preferred maximum Presentation Delay
* @brief Preferred maximum Presentation Delay in microseconds

@hermabe hermabe removed their request for review August 9, 2024 11:46
@Thalley
Copy link
Collaborator Author

Thalley commented Aug 13, 2024

@kruithofa would prefer not to modify more than necessary in this PR. It will likely conflict with #75763 and once one of them is one, I can update the remaining PR with your comments.

@Thalley Thalley requested a review from kruithofa August 13, 2024 12:22
kruithofa
kruithofa previously approved these changes Aug 14, 2024
View reviewed changes
Copy link
Collaborator

@kruithofa kruithofa left a comment

Choose a reason for hiding this comment

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

The comments will be handled in a separate PR, that's ok with me.

@Thalley
Copy link
Collaborator Author

Thalley commented Sep 11, 2024

Rebased to solve merge conflicts

@Thalley
Copy link
Collaborator Author

Thalley commented Sep 23, 2024

Rebased to solve merge conflicts

kruithofa
kruithofa previously approved these changes Sep 24, 2024
View reviewed changes
@Thalley
Copy link
Collaborator Author

Thalley commented Sep 27, 2024

Rebased to solve merge conflicts

sjanc
sjanc previously approved these changes Oct 1, 2024
View reviewed changes
fredrikdanebjer
fredrikdanebjer previously approved these changes Oct 2, 2024
View reviewed changes
jthm-ot
jthm-ot requested changes Oct 2, 2024
View reviewed changes
Copy link
Contributor

@jthm-ot jthm-ot left a comment

Choose a reason for hiding this comment

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

Great work! Just 2 comments to address

subsys/bluetooth/audio/bap_stream.c Outdated
LOG_DBG("Invalid combination of pref_pd_min %u, pd_min %u and pd_max: %u",
qos_pref->pref_pd_min, qos_pref->pd_min, qos_pref->pd_max);

return false;
}

if (qos_pref->pref_pd_max < qos_pref->pref_pd_min) {
if (qos_pref->pref_pd_max != BT_AUDIO_PD_PREF_NONE &&
qos_pref->pref_pd_max < qos_pref->pref_pd_min) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if qos_pref->pref_pd_min == BT_AUDIO_PD_PREF_NONE ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Then qos_pref->pref_pd_max < qos_pref->pref_pd_min is applied, ensuring that our pref_pd_max is greater or equal than pref_pd_min

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be:

if (qos_pref->pref_pd_max != BT_AUDIO_PD_PREF_NONE &&
	qos_pref->pref_pd_min != BT_AUDIO_PD_PREF_NONE && 
	qos_pref->pref_pd_max < qos_pref->pref_pd_min) {

subsys/bluetooth/audio/bap_stream.c Outdated
* the ASCS spec, but should also be >= MAX(pd_min, pref_pd_min)
*/
if (qos_pref->pref_pd_max != BT_AUDIO_PD_PREF_NONE) {
const uint32_t min_val = qos_pref->pref_pd_min == BT_AUDIO_PD_PREF_NONE
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the need for the min_val calculation. Above we have validated that

pd_min <= pref_pd_min <= pd_max
and
pref_pd_max >= pref_pd_min

We only need to validate

pd_min <= pref_pd_max <= pd_max

So just keep the old inrange check

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the case that qos_pref->pref_pd_min == BT_AUDIO_PD_PREF_NONE then neither the pd_min <= pref_pd_min <= pd_max nor pref_pd_max >= pref_pd_min checks are performed.

So if

  • pref_pd_min == BT_AUDIO_PD_PREF_NONE then pref_pd_max shall be greater or equal to pd_min
  • pref_pd_min != BT_AUDIO_PD_PREF_NONE then pref_pd_max shall be greater or equal to pref_pd_min

But you are right that MAX(qos_pref->pd_min, qos_pref->pref_pd_min) can be replaced with pref_pd_min, as pref_pd_min >= pd_min if pref_pd_min != BT_AUDIO_PD_PREF_NONE.

Actually the calculation can simply be const uint32_t min_val = MAX(qos_pref->pd_min, qos_pref->pref_pd_min)

@Thalley Thalley dismissed stale reviews from fredrikdanebjer and sjanc via 492099e October 2, 2024 12:47
jthm-ot
jthm-ot requested changes Oct 4, 2024
View reviewed changes
subsys/bluetooth/audio/bap_stream.c Outdated
LOG_DBG("Invalid combination of pref_pd_min %u, pd_min %u and pd_max: %u",
qos_pref->pref_pd_min, qos_pref->pd_min, qos_pref->pd_max);

return false;
}

if (qos_pref->pref_pd_max < qos_pref->pref_pd_min) {
if (qos_pref->pref_pd_max != BT_AUDIO_PD_PREF_NONE &&
qos_pref->pref_pd_max < qos_pref->pref_pd_min) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be:

if (qos_pref->pref_pd_max != BT_AUDIO_PD_PREF_NONE &&
	qos_pref->pref_pd_min != BT_AUDIO_PD_PREF_NONE && 
	qos_pref->pref_pd_max < qos_pref->pref_pd_min) {

subsys/bluetooth/audio/bap_stream.c Outdated
/* If pref_pd_max is not BT_AUDIO_PD_PREF_NONE then it shall pref_pd_max <= pd_max as per
* the ASCS spec, but should also be >= MAX(pd_min, pref_pd_min)
*/
if (qos_pref->pref_pd_max != BT_AUDIO_PD_PREF_NONE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should just be:

if (qos_pref->pref_pd_max != BT_AUDIO_PD_PREF_NONE) {
	if (!IN_RANGE(qos_pref->pref_pd_max, qos_pref->pd_min, qos_pref->pd_max)) {

With the change requested previously it has already been validated that
qos_pref->pref_pd_max <= qos_pref->pref_pd_min

@Thalley
Copy link
Collaborator Author

Thalley commented Oct 4, 2024

@sjanc @jthm-ot I've done what I should have done from the beginning and fixed the checks in a separate PR: #79413

@Thalley
Bluetooth: Audio: Rename bt_audio_codec_qos -> bt_bap_qos_cfg
5f14ccc 
The QoS structure is not related to a codec, but rather a
stream, and should thus not use the "Codec" name.

The BAP and ASCS specs refer to the QoS as
"QoS configuration" several places, so it is an obvious
choice for a name.

Since the structure is defined and used by BAP, the prefix
was changed from bt_audio to bt_bap.

Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
@Thalley Thalley requested a review from jthm-ot October 4, 2024 09:18
jthm-ot
jthm-ot approved these changes Oct 4, 2024
View reviewed changes
Copy link
Contributor

@jthm-ot jthm-ot left a comment

Choose a reason for hiding this comment

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

Great work! :-)

@fabiobaltieri fabiobaltieri merged commit dc97bbd into zephyrproject-rtos:main Oct 4, 2024
26 checks passed
@Thalley Thalley deleted the qos_cfg_rename branch October 4, 2024 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jthm-ot jthm-ot jthm-ot approved these changes

@sjanc sjanc sjanc approved these changes

@aescolar aescolar Awaiting requested review from aescolar

@asbjornsabo asbjornsabo Awaiting requested review from asbjornsabo

@Casper-Bonde-Bose Casper-Bonde-Bose Awaiting requested review from Casper-Bonde-Bose

@jhedberg jhedberg Awaiting requested review from jhedberg

@kartben kartben Awaiting requested review from kartben

@larsgk larsgk Awaiting requested review from larsgk

@MariuszSkamra MariuszSkamra Awaiting requested review from MariuszSkamra

@nashif nashif Awaiting requested review from nashif

@niym-ot niym-ot Awaiting requested review from niym-ot

@pin-zephyr pin-zephyr Awaiting requested review from pin-zephyr

@Vudentz Vudentz Awaiting requested review from Vudentz

@dkalowsk dkalowsk Awaiting requested review from dkalowsk

@kruithofa kruithofa Awaiting requested review from kruithofa

@fredrikdanebjer fredrikdanebjer Awaiting requested review from fredrikdanebjer

Assignees

@Thalley Thalley

Labels
area: Bluetooth Audio area: Bluetooth Qualification Bluetooth Qualification -related issues and pull requests area: Bluetooth area: Samples Samples Release Notes To be mentioned in the release notes
Projects
Bluetooth LE Audio
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

LE Audio: Rename struct bt_audio_codec_qos
8 participants
@Thalley @sjanc @fredrikdanebjer @kruithofa @jthm-ot @fabiobaltieri @jhedberg @zephyrbot