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

tests: CAP: Add testing of all audio configs #59073

Merged
merged 1 commit into from
Jul 18, 2023

Conversation

Thalley
Copy link
Collaborator

@Thalley Thalley commented Jun 8, 2023

Add test cases to test all audio configurations with
all presets.

Depends on #59061

@Thalley Thalley force-pushed the cap_ac_testing branch 5 times, most recently from b9e0a70 to 35a5e06 Compare June 8, 2023 16:07
@Thalley Thalley changed the title Cap ac testing tests: CAP: Add testing of all audio configs Jun 8, 2023
@Thalley Thalley force-pushed the cap_ac_testing branch 2 times, most recently from 4a8e1ac to 415ae86 Compare June 9, 2023 10:05
@cvinayak cvinayak self-requested a review June 9, 2023 11:06
@Thalley Thalley force-pushed the cap_ac_testing branch 4 times, most recently from d3fa80c to 832c6f1 Compare June 9, 2023 12:50
@Thalley Thalley force-pushed the cap_ac_testing branch 2 times, most recently from 67932ba to 88242a0 Compare June 19, 2023 13:56
@Thalley Thalley marked this pull request as ready for review June 19, 2023 14:02
Comment on lines 171 to 172
CONFIG_BT_CTLR_ADVANCED_FEATURES=y

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
CONFIG_BT_CTLR_ADVANCED_FEATURES=y


CONFIG_BT_CTLR_ISO_TX_BUFFERS=3


Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Controller advanced options, while be made defaults in Kconfig based on conformance testing requirements
CONFIG_BT_CTLR_ADVANCED_FEATURES=y

CONFIG_BT_CTLR_ADV_DATA_LEN_MAX=191
CONFIG_BT_CTLR_ADV_ISO_STREAM_COUNT=1
CONFIG_BT_CTLR_SYNC_ISO_STREAM_MAX=1

# Controller Connected ISO configs
CONFIG_BT_CTLR_CENTRAL_ISO=y
CONFIG_BT_CTLR_PERIPHERAL_ISO=y
CONFIG_BT_CTLR_ISOAL_SOURCES=2
CONFIG_BT_CTLR_ISOAL_SINKS=2
CONFIG_BT_CTLR_CONN_ISO_LOW_LATENCY_POLICY=y
Copy link
Contributor

Choose a reason for hiding this comment

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

Once support for flush timeout added, CONFIG_BT_CTLR_CONN_ISO_LOW_LATENCY_POLICY should not be used when tests require to use FT>1

larsgk
larsgk previously approved these changes Jul 17, 2023
for (size_t i = 0; i < param->conn_cnt; i++) {
/* Verify conn values */
if (param->snk_cnt[i] > CAP_AC_MAX_SNK) {
FAIL("Invalid conn_snk_cnt[%zu]: %zu\n", i, param->snk_cnt[i]);
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe change the printed var to match actual

Add test cases to test all audio configurations with
all presets.

Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
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.

LGTM, however I added few non-blocking comments

struct bt_conn *conn;
int err;

/* We're only interested in connectable events */
Copy link
Collaborator

Choose a reason for hiding this comment

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

This does not look like something we need in BSIM tests

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have this in the current tests as well; just copied from the common file

Comment on lines +381 to +385
/* connect only to devices in close proximity */
if (rssi < -70) {
FAIL("RSSI too low");
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have this in the current tests as well; just copied from the common file


err = bt_conn_le_create(
addr, BT_CONN_LE_CREATE_CONN,
BT_LE_CONN_PARAM(BT_GAP_INIT_CONN_INT_MIN, BT_GAP_INIT_CONN_INT_MIN, 0, 400),
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
BT_LE_CONN_PARAM(BT_GAP_INIT_CONN_INT_MIN, BT_GAP_INIT_CONN_INT_MIN, 0, 400),
BT_LE_CONN_PARAM_DEFAULT,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is specifically restricting the connection interval to 30ms so that is a multiple of both 7.5ms and 10ms SDU interval, which generally provides better chance of scheduling the CIS without conflicts with the ACL :)

Comment on lines +928 to +929
printk("stream %p\n", stream);
printk("named_preset %p\n", named_preset);
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
printk("stream %p\n", stream);
printk("named_preset %p\n", named_preset);
printk("stream %p named_preset %p\n", stream, named_preset);

}

for (size_t i = 0U; i < src_cnt; i++) {
src_uni_streams[i] = &unicast_streams[i + snk_cnt];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, consider some getter function here

@Thalley
Copy link
Collaborator Author

Thalley commented Jul 18, 2023

@MariuszSkamra We'd like to get this merged soon for the additional testing it provides. Your comments are fine, but I suggest we leave them as is so that this can get merged

@fabiobaltieri fabiobaltieri merged commit b140b70 into zephyrproject-rtos:main Jul 18, 2023
19 checks passed
@Thalley Thalley deleted the cap_ac_testing branch December 18, 2023 13:53
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.

6 participants