Add support for PACS autopts tests #1747

szymon-czapracki · 2024-04-05T12:49:52Z

Add support for Published Audio Capabilities Service in bttester application.

szymon-czapracki · 2024-04-12T07:53:45Z

apps/bttester/pkg.yml

apps/bttester/syscfg.yml

sjanc · 2024-04-12T12:21:49Z

apps/bttester/src/btp_pacs.c

+{
+    int rc;
+    struct set_avail_cb_data *avail_data = 
+        (struct set_avail_cb_data *) arg;


it shouldn't be needed to cast void*

sjanc · 2024-04-12T12:21:58Z

apps/bttester/src/btp_pacs.c

+    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);


sjanc · 2024-04-12T12:25:00Z

apps/bttester/src/btp_pacs.c

+}
+
+static uint8_t
+pacs_set_available_contexts()


functions with no params should be foo(void)

sjanc · 2024-04-12T12:26:09Z

apps/bttester/src/btp_pacs.c

+            rc = ble_audio_codec_register(&snk_codec_params, NULL);
+            if (rc) {
+                return BTP_STATUS_FAILED;
+            }


hmm missing breaks in all cases?

Added breaks.

sjanc · 2024-04-12T12:27:57Z

apps/bttester/src/btp_pacs.c

+    },
+    {
+        .opcode = BTP_PACS_UPDATE_CHARACTERISTIC,
+        .expect_len = BTP_HANDLER_LENGTH_VARIABLE,


pacs_update_characteristic_cmd is fixed size

sjanc · 2024-04-12T12:29:36Z

apps/bttester/src/btp_pacs.c

+    },
+    {
+        .opcode = BTP_PACS_SET_AVAILABLE_CONTEXTS,
+        .expect_len = BTP_HANDLER_LENGTH_VARIABLE,


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

sjanc

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

sjanc · 2024-04-19T06:59:18Z

apps/bttester/src/btp_pacs.c

+    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;


+1 if there is only 1 octet used

sjanc · 2024-04-19T07:02:34Z

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 {


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

sjanc · 2024-04-19T07:02:56Z

apps/bttester/src/btp/btp_pacs.h

+#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


cmd struct seems to be missing for this

sjanc · 2024-04-19T07:03:55Z

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)
+{


why command parameters are ignored?

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.

sjanc · 2024-04-19T07:05:39Z

apps/bttester/src/btp_pacs.c

+    /* 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);


this command seems to be missing in handlers?

sjanc · 2024-04-19T07:06:15Z

apps/bttester/src/btp_pacs.c

+    },
+    {
+        .opcode = BTP_PACS_SET_SUPPORTED_CONTEXTS,
+        .expect_len = BTP_HANDLER_LENGTH_VARIABLE,


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

Changed to fixed size.

rymanluk

LGTM

rymanluk · 2024-07-01T09:43:35Z

@sjanc if OK with you we could merge it

szymon-czapracki · 2024-08-29T11:44:45Z

@sjanc ping

sjanc · 2024-09-02T07:18:35Z

apps/bttester/src/btp/btp_pacs.h

+} __packed;
+
+#define BTP_PACS_UPDATE_CHARACTERISTIC          0x02
+struct pacs_update_characteristic_cmd {


those structs name should have bap_ prefix

sjanc · 2024-09-02T07:19:31Z

apps/bttester/src/btp_pacs.c

+    const struct pacs_set_available_contexts_cmd *avail = cmd;
+
+    /* If this originated from pacs_update_characteristic - we update with unspecified */
+    if (avail->sink_contexts == BTP_PACS_CHARACTERISTIC_AVAILABLE_AUDIO_CONTEXTS) {


those are le16, should be converted to host order before read

sjanc

looks good in general, some nitpick regarding naming and endiannes

sjanc · 2024-10-28T10:34:08Z

apps/bttester/src/btp_pacs.c

+                            void *rsp, uint16_t *rsp_len)
+{
+    struct set_avail_cb_data cb_data;
+    const struct btp_pacs_set_available_contexts_cmd *avail = cmd;


typically we just name this *cp = cmd

apps/bttester/src/btp_pacs.c

This commit adds support for Published Audio Capabilities Service in bttester application.

szymon-czapracki force-pushed the bttester_pacs branch 12 times, most recently from e786abb to fbc0e03 Compare April 11, 2024 10:12

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 marked this pull request as ready for review April 11, 2024 10:14

szymon-czapracki force-pushed the bttester_pacs branch 3 times, most recently from 42dba4f to ce67d2d Compare April 11, 2024 10:58

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 requested changes Apr 12, 2024

View reviewed changes

szymon-czapracki force-pushed the bttester_pacs branch 3 times, most recently from 873ff56 to cae7b62 Compare April 16, 2024 13:57

szymon-czapracki requested review from KKopyscinski and sjanc April 16, 2024 13:59

KKopyscinski approved these changes Apr 19, 2024

View reviewed changes

sjanc requested changes Apr 19, 2024

View reviewed changes

szymon-czapracki force-pushed the bttester_pacs branch 2 times, most recently from 948ef0f to 8230311 Compare April 25, 2024 10:33

szymon-czapracki force-pushed the bttester_pacs branch from 8230311 to 2404288 Compare April 29, 2024 15:01

szymon-czapracki force-pushed the bttester_pacs branch from 2404288 to 42ee0b3 Compare April 29, 2024 15:23

szymon-czapracki requested a review from sjanc April 29, 2024 15:24

szymon-czapracki force-pushed the bttester_pacs branch from 42ee0b3 to 0e8edf8 Compare April 29, 2024 17:12

szymon-czapracki force-pushed the bttester_pacs branch 2 times, most recently from 66040b3 to bf67ed3 Compare May 8, 2024 12:58

rymanluk approved these changes Jul 1, 2024

View reviewed changes

sjanc reviewed Sep 2, 2024

View reviewed changes

szymon-czapracki force-pushed the bttester_pacs branch from bf67ed3 to 964b659 Compare September 5, 2024 16:41

github-actions bot added host size/M Medium PR labels Sep 5, 2024

szymon-czapracki requested a review from sjanc September 5, 2024 16:42

szymon-czapracki force-pushed the bttester_pacs branch 2 times, most recently from 7fa7cb8 to 73a293f Compare September 23, 2024 14:25

sjanc reviewed Oct 28, 2024

View reviewed changes

szymon-czapracki force-pushed the bttester_pacs branch 2 times, most recently from 5bb6d7d to e1617a5 Compare October 28, 2024 10:59

szymon-czapracki force-pushed the bttester_pacs branch 2 times, most recently from fffb86a to 607cfef Compare November 4, 2024 11:31

github-actions bot removed the host label Nov 4, 2024

apps/bttester: Add initial support for PACS autopts tests

394630d

This commit adds support for Published Audio Capabilities Service in bttester application.

szymon-czapracki force-pushed the bttester_pacs branch from 607cfef to 394630d Compare November 4, 2024 11:39

sjanc approved these changes Nov 4, 2024

View reviewed changes

sjanc merged commit c41feda into apache:master Nov 4, 2024
19 checks passed

Add support for PACS autopts tests #1747

Add support for PACS autopts tests #1747

Conversation

szymon-czapracki commented Apr 5, 2024

szymon-czapracki commented Apr 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjanc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rymanluk left a comment

Choose a reason for hiding this comment

rymanluk commented Jul 1, 2024

szymon-czapracki commented Aug 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjanc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment