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

Add support for PACS autopts tests #1747

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add support for PACS autopts tests #1747

wants to merge 1 commit into from

Conversation

szymon-czapracki
Copy link
Contributor

Add support for Published Audio Capabilities Service in bttester application.

@szymon-czapracki szymon-czapracki force-pushed the bttester_pacs branch 12 times, most recently from e786abb to fbc0e03 Compare April 11, 2024 10:12
@szymon-czapracki szymon-czapracki changed the title [WIP][DNM] Add initial support for PACS autopts tests Add support for PACS autopts tests Apr 11, 2024
@szymon-czapracki szymon-czapracki marked this pull request as ready for review April 11, 2024 10:14
@szymon-czapracki szymon-czapracki force-pushed the bttester_pacs branch 3 times, most recently from 42dba4f to ce67d2d Compare April 11, 2024 10:58
@szymon-czapracki
Copy link
Contributor Author

@sjanc @KKopyscinski FYI

KKopyscinski
KKopyscinski requested changes Apr 12, 2024
View reviewed changes
apps/bttester/pkg.yml Show resolved Hide resolved
apps/bttester/syscfg.yml Outdated Show resolved Hide resolved
sjanc
sjanc requested changes Apr 12, 2024
View reviewed changes
apps/bttester/src/btp_pacs.c Outdated
{
int rc;
struct set_avail_cb_data *avail_data =
(struct set_avail_cb_data *) arg;
Copy link
Contributor

Choose a reason for hiding this comment

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

it shouldn't be needed to cast void*

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

apps/bttester/src/btp_pacs.c Outdated
cb_data.snk_ctxts = BLE_AUDIO_CONTEXT_TYPE_MEDIA;
cb_data.src_ctxts = BLE_AUDIO_CONTEXT_TYPE_MEDIA;

ble_gap_conn_foreach_handle(set_available, (void *)&cb_data);
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

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

apps/bttester/src/btp_pacs.c Outdated
}

static uint8_t
pacs_set_available_contexts()
Copy link
Contributor

Choose a reason for hiding this comment

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

functions with no params should be foo(void)

apps/bttester/src/btp_pacs.c
rc = ble_audio_codec_register(&snk_codec_params, NULL);
if (rc) {
return BTP_STATUS_FAILED;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm missing breaks in all cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added breaks.

apps/bttester/src/btp_pacs.c Outdated
},
{
.opcode = BTP_PACS_UPDATE_CHARACTERISTIC,
.expect_len = BTP_HANDLER_LENGTH_VARIABLE,
Copy link
Contributor

Choose a reason for hiding this comment

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

pacs_update_characteristic_cmd is fixed size

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

apps/bttester/src/btp_pacs.c Outdated
},
{
.opcode = BTP_PACS_SET_AVAILABLE_CONTEXTS,
.expect_len = BTP_HANDLER_LENGTH_VARIABLE,
Copy link
Contributor

Choose a reason for hiding this comment

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

those seems to be also fixed size (with no params)

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

@szymon-czapracki szymon-czapracki force-pushed the bttester_pacs branch 3 times, most recently from 873ff56 to cae7b62 Compare April 16, 2024 13:57
sjanc
sjanc requested changes Apr 19, 2024
View reviewed changes
Copy link
Contributor

@sjanc sjanc left a comment

Choose a reason for hiding this comment

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

in general all command handlers should validate correctness if parameters even if don't make use of it (than comment why), also double check command parameters since it doesn't match what is defined in doc/btp_pacs.txt

apps/bttester/src/btp_pacs.c Outdated
tester_set_bit(rp->data, BTP_PACS_SET_AVAILABLE_CONTEXTS);
tester_set_bit(rp->data, BTP_PACS_SET_SUPPORTED_CONTEXTS);

*rsp_len = sizeof(*rp) + 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 if there is only 1 octet used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

apps/bttester/src/btp/btp_pacs.h
#define BTP_PACS_CHARACTERISTIC_SOURCE_AUDIO_LOCATIONS 0x04
#define BTP_PACS_CHARACTERISTIC_AVAILABLE_AUDIO_CONTEXTS 0x05

struct btp_pacs_read_supported_commands_rp {
Copy link
Contributor

Choose a reason for hiding this comment

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

define those right after opcode (so that opcode, cmd struct and rsp stuct are in one place)

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

apps/bttester/src/btp/btp_pacs.h Outdated
#define BTP_PACS_UPDATE_CHARACTERISTIC 0x02
#define BTP_PACS_SET_LOCATION 0x03
#define BTP_PACS_SET_AVAILABLE_CONTEXTS 0x04
#define BTP_PACS_SET_SUPPORTED_CONTEXTS 0x05
Copy link
Contributor

Choose a reason for hiding this comment

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

cmd struct seems to be missing for this

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

apps/bttester/src/btp_pacs.c
static uint8_t
pacs_set_available_contexts(const void *cmd, uint16_t cmd_len,
void *rsp, uint16_t *rsp_len)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

why command parameters are ignored?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are no longer ignored, we now check if this originated from set_available, or update_characteristic. If this is aftermath of update_characteristic we update with hardcoded values, otherwise we use command parameters.

apps/bttester/src/btp_pacs.c Outdated
/* octet 0 */
tester_set_bit(rp->data, BTP_PACS_READ_SUPPORTED_COMMANDS);
tester_set_bit(rp->data, BTP_PACS_UPDATE_CHARACTERISTIC);
tester_set_bit(rp->data, BTP_PACS_SET_LOCATION);
Copy link
Contributor

Choose a reason for hiding this comment

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

this command seems to be missing in handlers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

apps/bttester/src/btp_pacs.c Outdated
},
{
.opcode = BTP_PACS_SET_SUPPORTED_CONTEXTS,
.expect_len = BTP_HANDLER_LENGTH_VARIABLE,
Copy link
Contributor

Choose a reason for hiding this comment

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

    Opcode 0x05: Set Supported Audio Contexts

            Controller Index:       <controller id>
            Command parameters:     Sink Contexts (2 octet)
                                    Source Contexts (2 octets)
            Response parameters:    <None>

this is not variable length

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to fixed size.

@szymon-czapracki szymon-czapracki force-pushed the bttester_pacs branch 2 times, most recently from 948ef0f to 8230311 Compare April 25, 2024 10:33
@szymon-czapracki szymon-czapracki force-pushed the bttester_pacs branch 2 times, most recently from 66040b3 to bf67ed3 Compare May 8, 2024 12:58
@szymon-czapracki
apps/bttester: Add initial support for PACS autopts tests
bf67ed3 
This commit adds support for Published
Audio Capabilities Service in bttester
application.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KKopyscinski KKopyscinski KKopyscinski approved these changes

@sjanc sjanc Awaiting requested review from sjanc

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@szymon-czapracki @sjanc @KKopyscinski