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

Bluetooth: Audio: Add tests for the CAP cancel command #82315

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions tests/bluetooth/audio/cap_commander/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,5 @@ target_sources(testbinary
src/test_micp.c
src/test_broadcast_reception.c
src/test_distribute_broadcast_code.c
src/test_cancel.c
)
183 changes: 183 additions & 0 deletions tests/bluetooth/audio/cap_commander/src/test_cancel.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,183 @@
/* test_cancel.c - unit test for cancel command */

/*
* Copyright (c) 2024 Nordic Semiconductor ASA
*
* SPDX-License-Identifier: Apache-2.0
*/

#include <stdlib.h>

#include <zephyr/bluetooth/audio/cap.h>
#include <zephyr/fff.h>

#include "bluetooth.h"
#include "cap_commander.h"
#include "conn.h"
#include "expects_util.h"
#include "cap_mocks.h"
#include "test_common.h"

#include <zephyr/logging/log.h>

void set_skip_add_src(int nr_to_skip);

LOG_MODULE_REGISTER(bt_cancel, CONFIG_BT_CAP_COMMANDER_LOG_LEVEL);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are registering a log here? Seems unused


#define FFF_GLOBALS

struct cap_commander_test_cancel_fixture {
struct bt_conn conns[CONFIG_BT_MAX_CONN];

struct bt_bap_bass_subgroup subgroups[CONFIG_BT_BAP_BASS_MAX_SUBGROUPS];
struct bt_cap_commander_broadcast_reception_start_member_param
start_member_params[CONFIG_BT_MAX_CONN];
struct bt_cap_commander_broadcast_reception_start_param start_param;
};

static void test_start_param_init(void *f)
{
struct cap_commander_test_cancel_fixture *fixture = f;
int err;

fixture->start_param.type = BT_CAP_SET_TYPE_AD_HOC;
fixture->start_param.param = fixture->start_member_params;

fixture->start_param.count = ARRAY_SIZE(fixture->start_member_params);

for (size_t i = 0; i < ARRAY_SIZE(fixture->subgroups); i++) {
fixture->subgroups[i].bis_sync = 1 << i;
fixture->subgroups[i].metadata_len = 0;
}

for (size_t i = 0U; i < ARRAY_SIZE(fixture->start_member_params); i++) {
fixture->start_member_params[i].member.member = &fixture->conns[i];
bt_addr_le_copy(&fixture->start_member_params[i].addr, BT_ADDR_LE_ANY);
fixture->start_member_params[i].adv_sid = 0;
fixture->start_member_params[i].pa_interval = 10;
fixture->start_member_params[i].broadcast_id = 0;
memcpy(fixture->start_member_params[i].subgroups, &fixture->subgroups[0],
sizeof(struct bt_bap_bass_subgroup) * CONFIG_BT_BAP_BASS_MAX_SUBGROUPS);
fixture->start_member_params[i].num_subgroups = CONFIG_BT_BAP_BASS_MAX_SUBGROUPS;
}

for (size_t i = 0; i < ARRAY_SIZE(fixture->conns); i++) {
err = bt_cap_commander_discover(&fixture->conns[i]);
zassert_equal(0, err, "Unexpected return value %d", err);
}
}

static void
cap_commander_test_cancel_fixture_init(struct cap_commander_test_cancel_fixture *fixture)
{
for (size_t i = 0; i < ARRAY_SIZE(fixture->conns); i++) {
test_conn_init(&fixture->conns[i]);
fixture->conns[i].index = i;
}

test_start_param_init(fixture);
}

static void *cap_commander_test_cancel_setup(void)
{
struct cap_commander_test_cancel_fixture *fixture;

fixture = malloc(sizeof(*fixture));
zassert_not_null(fixture);

return fixture;
}

static void cap_commander_test_cancel_before(void *f)
{
int err;
struct cap_commander_test_cancel_fixture *fixture = f;

memset(f, 0, sizeof(struct cap_commander_test_cancel_fixture));
cap_commander_test_cancel_fixture_init(fixture);

for (size_t i = 0; i < ARRAY_SIZE(fixture->conns); i++) {
err = bt_cap_commander_discover(&fixture->conns[i]);
zassert_equal(0, err, "Unexpected return value %d", err);
}
Comment on lines +99 to +102
Copy link
Collaborator

Choose a reason for hiding this comment

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

cap_commander_test_cancel_fixture_init also does discovery - Should remove one of them

}

static void cap_commander_test_cancel_after(void *f)
{
struct cap_commander_test_cancel_fixture *fixture = f;

bt_cap_commander_unregister_cb(&mock_cap_commander_cb);

for (size_t i = 0; i < ARRAY_SIZE(fixture->conns); i++) {
mock_bt_conn_disconnected(&fixture->conns[i], BT_HCI_ERR_REMOTE_USER_TERM_CONN);
}
}

static void cap_commander_test_cancel_teardown(void *f)
{
free(f);
}

static void test_cancel(void)
{
int err;

err = bt_cap_commander_cancel();
zassert_equal(0, err, "Unexpected return value %d", err);

zexpect_call_count("bt_cap_commander_cb.broadcast_reception_start", 1,
mock_cap_commander_broadcast_reception_start_cb_fake.call_count);
zassert_equal(-ECANCELED,
mock_cap_commander_broadcast_reception_start_cb_fake.arg1_history[0]);
}

ZTEST_SUITE(cap_commander_test_cancel, NULL, cap_commander_test_cancel_setup,
cap_commander_test_cancel_before, cap_commander_test_cancel_after,
cap_commander_test_cancel_teardown);

ZTEST_F(cap_commander_test_cancel, test_commander_cancel)
{
int err;

err = bt_cap_commander_register_cb(&mock_cap_commander_cb);
zassert_equal(0, err, "Unexpected return value %d", err);

/* Do not run the add_src callback, so that the broadcast reception start procedure does not
* run until completion
*/
set_skip_add_src(1);

/* initiate a CAP procedure; for this test we use broadcast reception start*/
err = bt_cap_commander_broadcast_reception_start(&fixture->start_param);
zassert_equal(0, err, "Could not start CAP procedure: %d", err);
Comment on lines +151 to +152
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there value in testing the cancel for the other CAP Commander procedures too, and not just the broadcast reception start?


test_cancel();
}

ZTEST_F(cap_commander_test_cancel, test_commander_cancel_double)
{
int err;

err = bt_cap_commander_register_cb(&mock_cap_commander_cb);
zassert_equal(0, err, "Unexpected return value %d", err);

set_skip_add_src(1);
err = bt_cap_commander_broadcast_reception_start(&fixture->start_param);
zassert_equal(0, err, "Could not start CAP procedure: %d", err);

test_cancel();

err = bt_cap_commander_cancel();
zassert_equal(-EALREADY, err, "Unexpected return value %d", err);
}

ZTEST_F(cap_commander_test_cancel, test_commander_cancel_no_proc_in_progress)
{
int err;

err = bt_cap_commander_register_cb(&mock_cap_commander_cb);
zassert_equal(0, err, "Unexpected return value %d", err);

err = bt_cap_commander_cancel();
zassert_equal(-EALREADY, err, "Unexpected return value %d", err);
}
16 changes: 16 additions & 0 deletions tests/bluetooth/audio/cap_commander/uut/bap_broadcast_assistant.c
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,16 @@

static sys_slist_t broadcast_assistant_cbs = SYS_SLIST_STATIC_INIT(&broadcast_assistant_cbs);

/* when > 0 immediately return from the add_src callback the specified number of times
* This allows us to test the CAP cancel command
*/
static int add_src_skip;

void set_skip_add_src(int setting)
Comment on lines +15 to +17
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
static int add_src_skip;
void set_skip_add_src(int setting)
static unsigned int add_src_skip;
void set_skip_add_src(unsigned int setting)

{
add_src_skip = setting;
}

struct bap_broadcast_assistant_recv_state_info {
uint8_t src_id;
/** Cached PAST available */
Expand Down Expand Up @@ -83,6 +93,12 @@ int bt_bap_broadcast_assistant_add_src(struct bt_conn *conn,
struct bt_bap_scan_delegator_recv_state state;
struct bt_bap_broadcast_assistant_cb *listener, *next;

if (add_src_skip != 0) {
add_src_skip--;

return 0;
}
Comment on lines +96 to +100
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do I understand this feature correctly:

We can set a value, add_src_skip, that effectively delays the response from the server indefinitely, thus causing the procedure to never finish on it's own, which allows us to cancel it?

If so, then it's a creative solution. Could this have been done with the mocked k_work implementation we have?

Two comments about the design of this:

  1. In some ways, it's a bit odd that we skip the first N callbacks, and not the last N callbacks, but the result will be the same
  2. This is not a very scalable solution if we want to do the commander cancel test for the remaining CAP commander procedures. On the top of my head, I don't have a general solution for this though.


/* Note that proper parameter checking is done in the caller */
zassert_not_null(conn, "conn is NULL");
zassert_not_null(param, "param is NULL");
Expand Down
94 changes: 89 additions & 5 deletions tests/bsim/bluetooth/audio/src/cap_commander_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,16 @@ static void cap_discovery_complete_cb(struct bt_conn *conn, int err,
}

#if defined(CONFIG_BT_VCP_VOL_CTLR)
static void cap_volume_changed_fail_cb(struct bt_conn *conn, int err)
{
if (err != -ECANCELED) {
FAIL("CAP command not cancelled for conn %p: %d\n", conn, err);
return;
}

SET_FLAG(flag_volume_changed);
}
Comment on lines +104 to +112
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of a new callback, how about we add a new flag, flag_cancelled, and then in cap_volume_changed_cb we set the flag_cancelled if err == -ECANCELED?

That way we can apply this in a generic way for the other procedures as well


static void cap_volume_changed_cb(struct bt_conn *conn, int err)
{
if (err != 0) {
Expand Down Expand Up @@ -811,8 +821,7 @@ static void discover_mics(size_t acceptor_cnt)
}
}
}

static void test_change_volume(void)
static int init_change_volume(void)
{
union bt_cap_set_member members[CONFIG_BT_MAX_CONN];
const struct bt_cap_commander_change_volume_param param = {
Expand All @@ -824,7 +833,6 @@ static void test_change_volume(void)
int err;

printk("Changing volume to %u\n", param.volume);
UNSET_FLAG(flag_volume_changed);

for (size_t i = 0U; i < param.count; i++) {
param.members[i].member = connected_conns[i];
Expand All @@ -833,11 +841,19 @@ static void test_change_volume(void)
err = bt_cap_commander_change_volume(&param);
if (err != 0) {
FAIL("Failed to change volume: %d\n", err);
return;
return err;
}
return param.volume;
}
static void test_change_volume(void)
{
int new_volume;

UNSET_FLAG(flag_volume_changed);
new_volume = init_change_volume();
WAIT_FOR_FLAG(flag_volume_changed);
printk("Volume changed to %u\n", param.volume);

printk("Volume changed to %u\n", new_volume);
Comment on lines +850 to +856
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the purpose of adding the return value? On error FAIL is already called

}

static void test_change_volume_mute(bool mute)
Expand Down Expand Up @@ -1026,6 +1042,18 @@ static void test_broadcast_reception_stop(size_t acceptor_count)
WAIT_FOR_FLAG(flag_broadcast_reception_stop);
}

static void test_cancel(bool cap_in_progress)
{
const int expected_err = cap_in_progress ? 0 : -EALREADY;
int err;

err = bt_cap_commander_cancel();
if (err != expected_err) {
FAIL("Could not cancel CAP command: %d\n", err);
return;
}
}

static void test_main_cap_commander_capture_and_render(void)
{
const size_t acceptor_cnt = get_dev_cnt() - 1; /* Assume all other devices are acceptors
Expand Down Expand Up @@ -1117,6 +1145,56 @@ static void test_main_cap_commander_broadcast_reception(void)
PASS("Broadcast reception passed\n");
}

static void test_main_cap_commander_cancel(void)
{
size_t acceptor_count;

/* The test consists of N devices
* 1 device is the broadcast source
* 1 device is the CAP commander
* This leaves N - 2 devices for the acceptor
*/
acceptor_count = get_dev_cnt() - 1;
printk("Acceptor count: %d\n", acceptor_count);

init(acceptor_count);

for (size_t i = 0U; i < acceptor_count; i++) {
scan_and_connect();

WAIT_FOR_FLAG(flag_mtu_exchanged);
}

/* TODO: We should use CSIP to find set members */
discover_cas(acceptor_count);

if (IS_ENABLED(CONFIG_BT_CSIP_SET_COORDINATOR)) {
if (IS_ENABLED(CONFIG_BT_VCP_VOL_CTLR)) {
/* As a result of the cancel command the callback is called with err ==
* -ECANCELED so we can not use the default callback
*/
cap_cb.volume_changed = cap_volume_changed_fail_cb;

discover_vcs(acceptor_count);

init_change_volume();

test_cancel(true);
WAIT_FOR_FLAG(flag_volume_changed);
}
}

test_cancel(false);

/* Disconnect all CAP acceptors */
disconnect_acl(acceptor_count);

/* restore the default callback */
cap_cb.volume_changed = cap_volume_changed_cb;

PASS("Broadcast reception passed\n");
}

static const struct bst_test_instance test_cap_commander[] = {
{
.test_id = "cap_commander_capture_and_render",
Expand All @@ -1130,6 +1208,12 @@ static const struct bst_test_instance test_cap_commander[] = {
.test_tick_f = test_tick,
.test_main_f = test_main_cap_commander_broadcast_reception,
},
{
.test_id = "cap_commander_cancel",
.test_post_init_f = test_init,
.test_tick_f = test_tick,
.test_main_f = test_main_cap_commander_cancel,
},
BSTEST_END_MARKER,
};

Expand Down
Loading
Loading