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

LE Audio: Add ISO test parameters to the BAP API #54050

Merged
merged 2 commits into from
Oct 20, 2023

Conversation

Thalley
Copy link
Collaborator

@Thalley Thalley commented Jan 24, 2023

Add support for setting advanced ISO parameters in the BAP API.

Fixes #50950

Copy link
Collaborator

@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.

Is the "advanced" term used somewhere in the spec? I'd prefer to stick to the Core defined naming.
I see the parameters that are about to be passed to the HCI_LE_Set_CIG_Parameters_Test are named as "advanced" in this PR. This introduces confusion, as this command should be used for test purposing only to let the controller to choose the parameters.

As per Core 5.3, Vol 4, Part E :
"The HCI_LE_Set_CIG_Parameters_Test command should only be used for
testing purposes."

My feeling is that this shall not be part of public API

@Thalley
Copy link
Collaborator Author

Thalley commented Aug 16, 2023

@MariuszSkamra Those are fair points. There has been a user request for using the test commands for more fine-grained control of the ISO channels (some of the fields for the test commands shoul arguably have been part of the "regular" commands in the first place).

The "advanced" term is thus not used in the core spec, as these does use the test commands, but adding "test" fields in a public API for non-test purposes also felt incorrect. The "advanced" term was first used and introduced by #53945 in the ISO API

@MariuszSkamra
Copy link
Collaborator

The "advanced" term is thus not used in the core spec, as these does use the test commands, but adding "test" fields in a public API for non-test purposes also felt incorrect.

Yes, because this command is test command. What's the difference in naming it advanced as it's still test command that should not be used except for testing?

@Thalley
Copy link
Collaborator Author

Thalley commented Aug 16, 2023

What's the difference in naming it advanced as it's still test command that should not be used except for testing?

It is specifically because we (or rather the user that requested this) do want to use it for other things than testing. The test commands provides additional control of how the ISO channels are configured, which is useful for some advanced use cases where the application want the ISO channels to behave in a specific and deterministic way. Leaving many of these values up to the controller, does not provide the flexibility that some applications require.

While the core spec does state that these command should not be used for other things than testing, it is still allowed.

@carlescufi
Copy link
Member

@MariuszSkamra please review

@Thalley
Copy link
Collaborator Author

Thalley commented Sep 20, 2023

As discussed, I will provide some alternative naming schemes for this feature

@Thalley
Copy link
Collaborator Author

Thalley commented Sep 20, 2023

Other naming options instead of "CONFIG_BT_ISO_ADVANCED"

CONFIG_BT_ISO_TEST
CONFIG_BT_ISO_TEST_PARAMS
CONFIG_BT_ISO_HOST_SCHED_PARAMS
CONFIG_BT_ISO_EXTENDED

@MariuszSkamra et.al. please provide your thoughts on the above

Another alternative is to create a new set of parameter structs and functions for the ISO API to use the ISO test commands, and then change the guard used for the LE Audio API to be something like CONFIG_BT_BAP_XXXX

@fredrikdanebjer
Copy link
Contributor

@MariuszSkamra et.al. please provide your thoughts on the above

My five cents on this is that I agree with Mariusz. Since this clearly is referenced as 'test' in the spec, I think that is what we should continue to use here. If a user requested this, is it far-fetched to assume that the same user is well traversed enough in the spec to know about the test HCI_LE_Set_CIG_Parameters_Test ?

I would vote for either CONFIG_BT_ISO_TEST or CONFIG_BT_ISO_TEST_PARAMS, preferably the latter :)

@MariuszSkamra
Copy link
Collaborator

CONFIG_BT_ISO_TEST_PARAMS would suit here IMO

@Thalley
Copy link
Collaborator Author

Thalley commented Sep 25, 2023

Will update to use CONFIG_BT_ISO_TEST_PARAMS

Rename the Kconfig option from BT_ISO_ADVANCED to
BT_ISO_TEST_PARAMS to more explicitly denote that it
enables support for using the ISO test parameters.

Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
Add support for the ISO test parameters in the BAP API.

Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
@Thalley
Copy link
Collaborator Author

Thalley commented Sep 25, 2023

@MariuszSkamra @fredrikdanebjer added a commit to update the naming of the Kconfig option before adding the support for it in the BAP API

@Thalley Thalley changed the title LE Audio: Add advanced ISO support LE Audio: Add ISO test parameters to the BAP API Sep 29, 2023
@Thalley
Copy link
Collaborator Author

Thalley commented Oct 3, 2023

@fredrikdanebjer Please re-approve if you can :)

@fabiobaltieri fabiobaltieri added this to the v3.6.0 milestone Oct 9, 2023
@carlescufi carlescufi merged commit 84c01bb into zephyrproject-rtos:main Oct 20, 2023
24 checks passed
@Thalley Thalley deleted the audio_advanced_iso branch December 18, 2023 13:52
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.

LE Audio: Support for ISO test commands
7 participants