Skip to content

Conversation

@szymon-czapracki
Copy link
Contributor

This commits fixes the handling of codec configuration in case there is no channel allocation field present.

@szymon-czapracki
Copy link
Contributor Author

@MariuszSkamra FYI

Copy link
Contributor

@MariuszSkamra MariuszSkamra left a comment

Choose a reason for hiding this comment

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

I have 2 minor comments, otherwise LGTM

Comment on lines 222 to 225
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this comment describes BT_AUDIO_LOCATION_UNSPECIFIED, not enum bt_audio_location, so you could put it right before BT_AUDIO_LOCATION_UNSPECIFIED

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

I would put here sentence taken from the spec:

BAP v1.0.1 4.3.2 Codec_Specific_Configuration LTV requirements
The absence of the Audio_Channel_Allocation LTV structure
shall be interpreted as a single channel with no specified Audio Location.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

This commits fixes the handling of codec configuration in case
there is no channel allocation field present.

Signed-off-by: Szymon Czapracki <[email protected]>
* The absence of the Audio_Channel_Allocation LTV structure
* shall be interpreted as a single channel with no specified Audio Location.
*/
return BT_AUDIO_LOCATION_UNSPECIFIED;
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 reasoning here to return 0 (BT_AUDIO_LOCATION_UNSPECIFIED, which is btw also an bt_audio_location and not an int) here, instead of an error code?

Seems odd to return error code in all other instances, except this one (and even more weirdly to return 0, i.e. success, in that case).

If we just want to return 0/sucess, instead of an error, I suggest that we just return 0 directly, instead of putting BT_AUDIO_LOCATION_UNSPECIFIED into bt_audio_location and use that here.

@github-actions
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 Nov 25, 2022
@github-actions github-actions bot closed this Dec 9, 2022
@Thalley
Copy link
Contributor

Thalley commented Dec 19, 2022

@szymon-czapracki Are you going to reopen this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants