BASS support for bttester. #1813

szymon-czapracki · 2024-07-29T13:17:39Z

Add BASS support in bttester.

rymanluk · 2024-08-01T20:01:10Z

@szymon-czapracki could you help with failing tests?

rymanluk · 2024-08-05T09:21:33Z

apps/bttester/src/btp/btp_bap.h

@@ -83,12 +83,27 @@ struct bap_bap_broadcast_source_stop_cmd {
    uint8_t broadcast_id[3];
 } __packed;

-#define BTP_BAP_BROADCAST_SINK_SETUP            0x0a
+#define BTP_BAP_BROADCAST_SINK_SETUP            0xa


I would stick to 0x0a

rymanluk · 2024-08-05T09:22:51Z

apps/bttester/src/btp_bap.c

+        console_printf("Source Add:\nsource_id=%u\n", action->source_add.source_id);
+        if (action->source_add.out_source_id_to_swap == NULL) {
+            return 0;
+        } else {


else not needed

rymanluk · 2024-08-05T09:25:08Z

apps/bttester/syscfg.yml

@@ -110,11 +118,12 @@ syscfg.vals:
    MSYS_1_BLOCK_COUNT: 100

    BLE_MONITOR_RTT: 1
-    CONSOLE_RTT: 0
+    BLE_MONITOR_RTT_BUFFER_SIZE: 4096


do we want this configuration to be delivered inside BASS test?

rymanluk · 2024-08-05T09:28:36Z

nimble/host/audio/services/bass/include/services/bass/ble_audio_svc_bass.h

@@ -286,6 +286,9 @@ struct ble_svc_audio_bass_operation {
            /** Number of subgroups */
            uint8_t num_subgroups;

+            /** BIS Synchronisation of subgroups */


I think we could split it to small patches which address different problems.

This is no longer present in current patch set.

rymanluk · 2024-08-05T09:29:19Z

targets/nordic_pca10095_net-blehci/syscfg.yml

@@ -39,4 +39,7 @@ syscfg.vals:
    BLE_MULTI_ADV_INSTANCES: 5
    BLE_PERIODIC_ADV: 1
    BLE_PERIODIC_ADV_SYNC_TRANSFER: 1
-    BLE_VERSION: 51
+    BLE_PERIODIC_ADV_SYNC_BIGINFO_REPORTS: 1


@sjanc how to handle that?

I think we should enable all (non-experimental) features in default netcore image, so this is fine but should be in separate PR

No longer in current patch set.

rymanluk · 2024-08-05T09:42:23Z

targets/nordic_pca10095_net-blehci/syscfg.yml

@@ -39,4 +39,7 @@ syscfg.vals:
    BLE_MULTI_ADV_INSTANCES: 5
    BLE_PERIODIC_ADV: 1
    BLE_PERIODIC_ADV_SYNC_TRANSFER: 1
-    BLE_VERSION: 51
+    BLE_PERIODIC_ADV_SYNC_BIGINFO_REPORTS: 1
+    BLE_LL_ISO_BROADCASTER: 1


BLE_LL -> BLE_

rymanluk · 2024-08-05T09:43:44Z

nimble/host/audio/services/bass/src/ble_audio_svc_bass.c

    memcpy(operation.add_source.adv_addr.val, &data[offset], 6);
    offset += 6;
    operation.add_source.adv_sid = data[offset++];
+    if (operation.add_source.adv_sid < 0 ||
+        operation.add_source.adv_sid > 0xF) {


rymanluk · 2024-08-05T09:44:14Z

nimble/host/audio/services/bass/src/ble_audio_svc_bass.c

    operation.add_source.adv_addr.type = data[offset++];
+    if (operation.add_source.adv_addr.type < 0 ||
+        operation.add_source.adv_addr.type > 3) {


do not use magic number

sjanc

and build failure is actual typo in code affected, it just happen that CI was never building it (and now it was pulled into bttester build), should be fixed in separate PR

sjanc · 2024-08-06T11:20:32Z

apps/bttester/src/btp/bttester.h

+tester_init_bass(void);
+uint8_t
+tester_unregister_bass(void);
+#endif /* MYNEWT_VAL(BLE_ISO_BROADCASTER) */


this #endif looks odd, some rebase misclick?

sjanc · 2024-08-06T11:21:06Z

apps/bttester/src/btp_bap.c

+broadcast_code_set(const void *cmd, uint16_t cmd_len, void *rsp,
+                   uint16_t *rsp_len)
+{
+    return BTP_STATUS_SUCCESS;


why those have no implementation?

sjanc · 2024-08-06T11:22:42Z

apps/bttester/src/btp_bap.c

+    {
+        .opcode = BTP_BAP_BROADCAST_SINK_SETUP,
+        .index = BTP_INDEX,
+        .expect_len = BTP_HANDLER_LENGTH_VARIABLE,


BTP defines this as fixed size

Changed - sink setup has 0 size.

sjanc · 2024-08-06T11:22:48Z

apps/bttester/src/btp_bap.c

+    {
+        .opcode = BTP_BAP_BROADCAST_SINK_STOP,
+        .index = BTP_INDEX,
+        .expect_len = BTP_HANDLER_LENGTH_VARIABLE,


sjanc · 2024-08-06T11:23:52Z

apps/bttester/src/btp_bap.c

+{
+    int rc;
+
+    rc = ble_audio_broadcast_sink_stop(0);


why all BTP params are ignored?

No longer ignored.

sjanc · 2024-08-06T11:25:16Z

apps/bttester/src/btp_bap.c

+{
+    switch (event->type) {
+    case BLE_AUDIO_EVENT_BROADCAST_ANNOUNCEMENT:
+        console_printf("\n");


Leftover - removed.

sjanc · 2024-08-06T11:27:44Z

apps/bttester/src/btp_gap.c

@@ -2000,6 +2001,44 @@ set_filter_accept_list(const void *cmd, uint16_t cmd_len,
    return BTP_STATUS_SUCCESS;
 }

+#if MYNEWT_VAL(BLE_EXT_ADV)
+static uint8_t
+set_ext_advertising(const void *cmd, uint16_t cmd_len,


This is not valid implementation of this command. From BTP spec: "This command is used to enable/disable Extended Advertising when Start/Stop Advertising is used."

So it should be used to tune if advertising is using legacy or ext_adv PDUs, not to start advertising

sjanc · 2024-08-06T11:29:26Z

targets/nordic_pca10095_net-blehci/syscfg.yml

@@ -39,4 +39,7 @@ syscfg.vals:
    BLE_MULTI_ADV_INSTANCES: 5
    BLE_PERIODIC_ADV: 1
    BLE_PERIODIC_ADV_SYNC_TRANSFER: 1
-    BLE_VERSION: 51
+    BLE_PERIODIC_ADV_SYNC_BIGINFO_REPORTS: 1


I think we should enable all (non-experimental) features in default netcore image, so this is fine but should be in separate PR

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

@sjanc ping

sjanc · 2024-09-02T09:30:33Z

apps/bttester/src/btp/bttester.h


-#endif /* __BTTESTER_H__ */
+#endif /* __BTTESTER_H */


thi is closing BTTESTER_H

sjanc · 2024-09-02T09:32:06Z

apps/bttester/src/btp_bap.c

+    sinks[sink_num].bd_addr_type = cp->bd_addr_type;
+    sinks[sink_num].source_id = cp->source_id;
+
+    for (i = 0; i < BLE_AUDIO_BROADCAST_CODE_SIZE; i++) {


just memcpy this

sjanc · 2024-09-02T09:33:33Z

apps/bttester/src/btp_bap.c

+    int i;
+    const struct btp_bap_set_broadcast_code_cmd *cp = cmd;
+
+    sinks[sink_num].addr = cp->addr;


we should reject if sink_num is too big, now it will write pass sinks[]

sjanc · 2024-09-02T09:34:45Z

apps/bttester/src/btp_bap.c

 #include "audio/ble_audio.h"
 #include "host/ble_iso.h"

 #define BROADCAST_ADV_INSTANCE                 1

 static struct ble_audio_big_subgroup big_subgroup;

+static struct broadcast_sink {
+    uint8_t bd_addr_type;


ble_addr_t already has type in it...

sjanc · 2024-09-02T09:36:54Z

apps/bttester/src/btp_bap.c

+{
+    ble_addr_t addr;
+    const struct btp_bap_broadcast_sink_stop_cmd *cp = cmd;
+    uint8_t broadcast_to_stop[BLE_AUDIO_BROADCAST_CODE_SIZE];


this seems to be unused...

sjanc · 2024-09-02T09:47:04Z

apps/bttester/src/btp_bap.c

+    /* Figure out address to use while scanning. */
+    rc = ble_hs_id_infer_auto(0, &own_addr_type);
+    if (rc != 0) {
+        console_printf("determining own address type failed (%d)", rc);


console_printf doesn add any additional context like function name, you should probably make those less generic (or include func maybe?)

same for other messages, include function name or make them less generic

Added func to make this less generic.

sjanc · 2024-09-24T13:23:48Z

apps/bttester/src/btp/btp_bap.h

+#define BTP_BAP_BROADCAST_SINK_STOP             0x0f
+struct btp_bap_broadcast_sink_stop_cmd {
+    uint8_t addr_type;
+    ble_addr_t address;


ble_addr_t already has address type...

sjanc · 2024-09-24T13:23:56Z

apps/bttester/src/btp/btp_bap.h

+
+#define BTP_BAP_SET_BROADCAST_CODE              0x17
+struct btp_bap_set_broadcast_code_cmd {
+    uint8_t bd_addr_type;


Adding BASS support in bttester

szymon-czapracki force-pushed the bass_bttester branch from daaf152 to 840d1ad Compare August 2, 2024 14:13

github-actions bot added host size/L Large PR labels Aug 2, 2024

szymon-czapracki force-pushed the bass_bttester branch 2 times, most recently from 31f6def to 0dc37c5 Compare August 5, 2024 08:38

rymanluk reviewed Aug 5, 2024

View reviewed changes

sjanc requested changes Aug 6, 2024

View reviewed changes

szymon-czapracki force-pushed the bass_bttester branch from 0dc37c5 to 104f088 Compare August 23, 2024 13:09

github-actions bot added size/M Medium PR and removed host labels Aug 23, 2024

szymon-czapracki force-pushed the bass_bttester branch 5 times, most recently from 5bfc36c to d103710 Compare August 27, 2024 13:19

szymon-czapracki requested review from rymanluk and sjanc August 27, 2024 13:32

szymon-czapracki changed the title ~~BASS bttester & various fixes~~ BASS support for bttester. Aug 29, 2024

sjanc reviewed Sep 2, 2024

View reviewed changes

szymon-czapracki force-pushed the bass_bttester branch from d103710 to 12cf43f Compare September 5, 2024 16:14

szymon-czapracki force-pushed the bass_bttester branch from 12cf43f to 5ef2315 Compare September 23, 2024 11:35

github-actions bot added the host label Sep 23, 2024

szymon-czapracki force-pushed the bass_bttester branch from 5ef2315 to d8c0e75 Compare September 23, 2024 11:47

github-actions bot removed the host label Sep 23, 2024

szymon-czapracki force-pushed the bass_bttester branch 3 times, most recently from cc2b637 to 159d0ef Compare September 23, 2024 14:20

sjanc requested changes Sep 24, 2024

View reviewed changes

Initial commit adding BASS in bttester

fb2807c

Adding BASS support in bttester

szymon-czapracki force-pushed the bass_bttester branch from 159d0ef to fb2807c Compare October 7, 2024 18:29

szymon-czapracki requested a review from sjanc October 7, 2024 18:29

BASS support for bttester. #1813

Are you sure you want to change the base?

BASS support for bttester. #1813

Conversation

szymon-czapracki commented Jul 29, 2024 • edited Loading

rymanluk commented Aug 1, 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

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

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

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

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

szymon-czapracki commented Jul 29, 2024 •

edited

Loading