Skip to content

Commit e7b6fd0

Browse files
Thalleykartben
Thalley
authored and
kartben
committed
Bluetooth: PACS: Fix several issues with pacs_register
Fixed the following issues: 1) Missing guard before accessing parameters 2) Fixed bad sizeof when resetting 3) Fixed several bad offsets when removing attributes Added tests to verify that it works now. Signed-off-by: Emil Gydesen <[email protected]>
1 parent 5860554 commit e7b6fd0

File tree

3 files changed

+173
-51
lines changed

3 files changed

+173
-51
lines changed

‎subsys/bluetooth/audio/pacs.c‎

Lines changed: 63 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -707,77 +707,89 @@ static sys_slist_t *pacs_get_pac(enum bt_audio_dir dir)
707707
BT_PAC_SUPPORTED_CONTEXT(supported_context_read) \
708708
}
709709

710+
static const struct bt_gatt_attr _pacs_attrs[] = BT_PACS_SERVICE_DEFINITION();
710711
static struct bt_gatt_attr pacs_attrs[] = BT_PACS_SERVICE_DEFINITION();
711712
static struct bt_gatt_service pacs_svc = (struct bt_gatt_service)BT_GATT_SERVICE(pacs_attrs);
712713

713-
714-
715-
716-
#if defined(BT_PAC_SNK_NOTIFIABLE)
717-
#define PACS_SINK_PAC_CHAR_ATTR_COUNT 3 /* declaration + value + cccd */
718-
#else
719-
#define PACS_SINK_PAC_CHAR_ATTR_COUNT 2 /* declaration + value */
720-
#endif /* BT_PAC_SNK_NOTIFIABLE */
721-
722-
#if defined(BT_PAC_SNK_LOC_NOTIFIABLE)
723-
#define PACS_SINK_PAC_LOC_CHAR_ATTR_COUNT 3 /* declaration + value + cccd */
724-
#else
725-
#define PACS_SINK_PAC_LOC_CHAR_ATTR_COUNT 2 /* declaration + value*/
726-
#endif /* BT_PAC_SNK_LOC_NOTIFIABLE */
727-
728-
#if defined(BT_PAC_SRC_NOTIFIABLE)
729-
#define PACS_SOURCE_PAC_CHAR_ATTR_COUNT 3 /* declaration + value + cccd */
714+
#if defined(CONFIG_BT_PAC_SNK)
715+
/* declaration + value [+ cccd] */
716+
#define PACS_SNK_PAC_CHAR_ATTR_COUNT COND_CODE_1(IS_ENABLED(CONFIG_BT_PAC_SNK_NOTIFIABLE), (3), (2))
717+
/* declaration + value [+ cccd] */
718+
#define PACS_SNK_PAC_LOC_CHAR_ATTR_COUNT \
719+
COND_CODE_1(IS_ENABLED(CONFIG_BT_PAC_SNK_LOC_NOTIFIABLE), (3), (2))
730720
#else
731-
#define PACS_SOURCE_PAC_CHAR_ATTR_COUNT 2 /* declaration + value */
732-
#endif /* BT_PAC_SRC_NOTIFIABLE */
721+
#define PACS_SNK_PAC_CHAR_ATTR_COUNT 0
722+
#define PACS_SNK_PAC_LOC_CHAR_ATTR_COUNT 0
723+
#endif /* CONFIG_BT_PAC_SNK */
733724

734-
#if defined(BT_PAC_SRC_LOC_NOTIFIABLE)
735-
#define PACS_SOURCE_PAC_LOC_CHAR_ATTR_COUNT 3 /* declaration + value + cccd */
725+
#if defined(CONFIG_BT_PAC_SRC)
726+
/* declaration + value [+ cccd] */
727+
#define PACS_SRC_PAC_CHAR_ATTR_COUNT COND_CODE_1(IS_ENABLED(CONFIG_BT_PAC_SRC_NOTIFIABLE), (3), (2))
728+
/* declaration + value [+ cccd] */
729+
#define PACS_SRC_PAC_LOC_CHAR_ATTR_COUNT \
730+
COND_CODE_1(IS_ENABLED(CONFIG_BT_PAC_SRC_LOC_NOTIFIABLE), (3), (2))
736731
#else
737-
#define PACS_SOURCE_PAC_LOC_CHAR_ATTR_COUNT 2 /* declaration + value*/
738-
#endif /* BT_PAC_SRC_LOC_NOTIFIABLE */
732+
#define PACS_SRC_PAC_CHAR_ATTR_COUNT 0
733+
#define PACS_SRC_PAC_LOC_CHAR_ATTR_COUNT 0
734+
#endif /* CONFIG_BT_PAC_SNK */
739735

740736
static void configure_pacs_char(const struct bt_bap_pacs_register_param *param)
741737
{
742-
size_t attrs_to_rem;
743-
uint8_t first_to_rem;
738+
const uint8_t first_attr_offset = 1U;
739+
struct bt_gatt_attr *svc_attrs =
740+
&pacs_svc.attrs[first_attr_offset]; /* first attribute is the service */
741+
uint8_t attrs_to_rem = 0U;
742+
uint8_t first_to_rem = 0U;
743+
uint8_t src_pac_offset;
744744

745745
/* Remove the Sink PAC and Location */
746+
#if defined(CONFIG_BT_PAC_SNK_LOC)
747+
if (!param->snk_loc) {
748+
first_to_rem = PACS_SNK_PAC_CHAR_ATTR_COUNT;
749+
attrs_to_rem = PACS_SNK_PAC_LOC_CHAR_ATTR_COUNT;
750+
}
751+
#endif /* CONFIG_BT_PAC_SNK_LOC */
752+
#if defined(CONFIG_BT_PAC_SNK)
746753
if (!param->snk_pac) {
747-
first_to_rem = 0;
748-
attrs_to_rem = PACS_SINK_PAC_CHAR_ATTR_COUNT + PACS_SINK_PAC_LOC_CHAR_ATTR_COUNT;
749-
} else if (!param->snk_loc) {
750-
first_to_rem = PACS_SINK_PAC_CHAR_ATTR_COUNT;
751-
attrs_to_rem = PACS_SINK_PAC_LOC_CHAR_ATTR_COUNT;
752-
} else {
753-
first_to_rem = pacs_svc.attr_count;
754-
attrs_to_rem = 0;
754+
first_to_rem = 0U;
755+
attrs_to_rem = PACS_SNK_PAC_CHAR_ATTR_COUNT + PACS_SNK_PAC_LOC_CHAR_ATTR_COUNT;
755756
}
757+
#endif /* CONFIG_BT_PAC_SNK */
756758

757-
for (size_t i = first_to_rem + attrs_to_rem; i < pacs_svc.attr_count; i++) {
758-
pacs_svc.attrs[i - attrs_to_rem] = pacs_svc.attrs[i];
759+
if (attrs_to_rem > 0U) {
760+
for (uint8_t i = first_to_rem + attrs_to_rem;
761+
i < pacs_svc.attr_count - first_attr_offset; i++) {
762+
svc_attrs[i - attrs_to_rem] = svc_attrs[i];
763+
}
764+
pacs_svc.attr_count -= attrs_to_rem;
759765
}
760-
pacs_svc.attr_count -= attrs_to_rem;
761766

762767
/* Set first_to_rem to the start of Source PAC Char, for cleaner offset calc */
763-
first_to_rem = PACS_SINK_PAC_CHAR_ATTR_COUNT + PACS_SINK_PAC_LOC_CHAR_ATTR_COUNT;
768+
src_pac_offset =
769+
(PACS_SNK_PAC_CHAR_ATTR_COUNT + PACS_SNK_PAC_LOC_CHAR_ATTR_COUNT) - attrs_to_rem;
770+
attrs_to_rem = 0U;
764771

765772
/* Remove the Source PAC and Location */
766-
if (!param->snk_pac) {
767-
first_to_rem -= attrs_to_rem;
768-
attrs_to_rem = PACS_SOURCE_PAC_CHAR_ATTR_COUNT +
769-
PACS_SOURCE_PAC_LOC_CHAR_ATTR_COUNT;
770-
} else if (!param->snk_loc) {
771-
first_to_rem = first_to_rem + PACS_SOURCE_PAC_CHAR_ATTR_COUNT - attrs_to_rem;
772-
attrs_to_rem = PACS_SINK_PAC_LOC_CHAR_ATTR_COUNT;
773-
} else {
774-
return;
773+
#if defined(CONFIG_BT_PAC_SRC_LOC)
774+
if (!param->src_loc) {
775+
first_to_rem = src_pac_offset + PACS_SRC_PAC_CHAR_ATTR_COUNT;
776+
attrs_to_rem = PACS_SRC_PAC_LOC_CHAR_ATTR_COUNT;
777+
}
778+
#endif /* CONFIG_BT_PAC_SRC_LOC */
779+
#if defined(CONFIG_BT_PAC_SRC)
780+
if (!param->src_pac) {
781+
first_to_rem = src_pac_offset;
782+
attrs_to_rem = PACS_SRC_PAC_CHAR_ATTR_COUNT + PACS_SRC_PAC_LOC_CHAR_ATTR_COUNT;
775783
}
784+
#endif /* CONFIG_BT_PAC_SRC */
776785

777-
for (size_t i = first_to_rem; i < pacs_svc.attr_count; i++) {
778-
pacs_svc.attrs[i - attrs_to_rem] = pacs_svc.attrs[i];
786+
if (attrs_to_rem > 0U) {
787+
for (uint8_t i = first_to_rem + attrs_to_rem;
788+
i < pacs_svc.attr_count - first_attr_offset; i++) {
789+
svc_attrs[i - attrs_to_rem] = svc_attrs[i];
790+
}
791+
pacs_svc.attr_count -= attrs_to_rem;
779792
}
780-
pacs_svc.attr_count -= attrs_to_rem;
781793
}
782794

783795
static bool valid_pacs_register_param(const struct bt_bap_pacs_register_param *param)
@@ -861,7 +873,6 @@ int bt_pacs_register(const struct bt_bap_pacs_register_param *param)
861873
int bt_pacs_unregister(void)
862874
{
863875
int err;
864-
struct bt_gatt_attr _pacs_attrs[] = BT_PACS_SERVICE_DEFINITION();
865876

866877
if (!atomic_test_bit(pacs.flags, PACS_FLAG_REGISTERED)) {
867878
LOG_DBG("No pacs instance registered");
@@ -888,7 +899,8 @@ int bt_pacs_unregister(void)
888899
return err;
889900
}
890901

891-
memcpy(pacs_svc.attrs, &_pacs_attrs, sizeof(struct bt_gatt_attr));
902+
/* Restore to original definition */
903+
memcpy(pacs_svc.attrs, &_pacs_attrs, sizeof(_pacs_attrs));
892904
pacs_svc.attr_count = ARRAY_SIZE(pacs_attrs);
893905

894906
atomic_clear_bit(pacs.flags, PACS_FLAG_REGISTERED);

‎tests/bluetooth/audio/mocks/src/gatt.c‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -547,3 +547,64 @@ bool bt_gatt_is_subscribed(struct bt_conn *conn,
547547
{
548548
return mock_bt_gatt_is_subscribed(conn, attr, ccc_type);
549549
}
550+
551+
uint16_t bt_gatt_attr_get_handle(const struct bt_gatt_attr *attr)
552+
{
553+
uint16_t handle = 1;
554+
555+
if (!attr) {
556+
return 0;
557+
}
558+
559+
if (attr->handle) {
560+
return attr->handle;
561+
}
562+
563+
STRUCT_SECTION_FOREACH(bt_gatt_service_static, static_svc) {
564+
/* Skip ahead if start is not within service attributes array */
565+
if ((attr < &static_svc->attrs[0]) ||
566+
(attr > &static_svc->attrs[static_svc->attr_count - 1])) {
567+
handle += static_svc->attr_count;
568+
continue;
569+
}
570+
571+
for (size_t i = 0; i < static_svc->attr_count; i++, handle++) {
572+
if (attr == &static_svc->attrs[i]) {
573+
return handle;
574+
}
575+
}
576+
}
577+
578+
return 0;
579+
}
580+
581+
static uint8_t find_next(const struct bt_gatt_attr *attr, uint16_t handle, void *user_data)
582+
{
583+
struct bt_gatt_attr **next = user_data;
584+
585+
*next = (struct bt_gatt_attr *)attr;
586+
587+
return BT_GATT_ITER_STOP;
588+
}
589+
590+
struct bt_gatt_attr *bt_gatt_find_by_uuid(const struct bt_gatt_attr *attr, uint16_t attr_count,
591+
const struct bt_uuid *uuid)
592+
{
593+
struct bt_gatt_attr *found = NULL;
594+
uint16_t start_handle = bt_gatt_attr_get_handle(attr);
595+
uint16_t end_handle = start_handle && attr_count
596+
? MIN(start_handle + attr_count, BT_ATT_LAST_ATTRIBUTE_HANDLE)
597+
: BT_ATT_LAST_ATTRIBUTE_HANDLE;
598+
599+
if (attr != NULL && start_handle == 0U) {
600+
/* If start_handle is 0 then `attr` is not in our database, and should not be used
601+
* as a starting point for the search
602+
*/
603+
LOG_DBG("Could not find handle of attr %p", attr);
604+
return NULL;
605+
}
606+
607+
bt_gatt_foreach_attr_type(start_handle, end_handle, uuid, NULL, 1, find_next, &found);
608+
609+
return found;
610+
}

‎tests/bluetooth/audio/pacs/src/main.c‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,12 @@
1010
#include <stdint.h>
1111
#include <stdlib.h>
1212

13+
#include <zephyr/bluetooth/att.h>
1314
#include <zephyr/bluetooth/audio/audio.h>
1415
#include <zephyr/bluetooth/audio/pacs.h>
1516
#include <zephyr/bluetooth/bluetooth.h>
17+
#include <zephyr/bluetooth/gatt.h>
18+
#include <zephyr/bluetooth/uuid.h>
1619
#include <zephyr/fff.h>
1720
#include <zephyr/sys/util.h>
1821
#include <zephyr/sys/util_macro.h>
@@ -60,13 +63,59 @@ static ZTEST(pacs_test_suite, test_pacs_register)
6063
};
6164

6265
for (size_t i = 0U; i < ARRAY_SIZE(pacs_params); i++) {
66+
struct bt_gatt_attr *attr;
6367
int err;
6468

6569
err = bt_pacs_register(&pacs_params[i]);
6670
zassert_equal(err, 0, "[%zu]: Unexpected return value %d", i, err);
6771

72+
#if defined(CONFIG_BT_PAC_SNK)
73+
attr = bt_gatt_find_by_uuid(NULL, 0, BT_UUID_PACS_SNK);
74+
if (pacs_params[i].snk_pac) {
75+
zassert_not_null(attr, "[%zu]: Could not find sink PAC", i);
76+
} else {
77+
zassert_is_null(attr, "[%zu]: Found unexpected sink PAC", i);
78+
}
79+
#endif /*CONFIG_BT_PAC_SNK */
80+
#if defined(CONFIG_BT_PAC_SNK_LOC)
81+
attr = bt_gatt_find_by_uuid(NULL, 0, BT_UUID_PACS_SNK_LOC);
82+
if (pacs_params[i].snk_loc) {
83+
zassert_not_null(attr, "[%zu]: Could not find sink loc", i);
84+
} else {
85+
zassert_is_null(attr, "[%zu]: Found unexpected sink loc", i);
86+
}
87+
#endif /*CONFIG_BT_PAC_SNK_LOC */
88+
#if defined(CONFIG_BT_PAC_SRC)
89+
attr = bt_gatt_find_by_uuid(NULL, 0, BT_UUID_PACS_SRC);
90+
if (pacs_params[i].src_pac) {
91+
zassert_not_null(attr, "[%zu]: Could not find source PAC", i);
92+
} else {
93+
zassert_is_null(attr, "[%zu]: Found unexpected source PAC", i);
94+
}
95+
#endif /*CONFIG_BT_PAC_SRC */
96+
#if defined(CONFIG_BT_PAC_SRC_LOC)
97+
attr = bt_gatt_find_by_uuid(NULL, 0, BT_UUID_PACS_SRC_LOC);
98+
if (pacs_params[i].src_loc) {
99+
zassert_not_null(attr, "[%zu]: Could not find source loc", i);
100+
} else {
101+
zassert_is_null(attr, "[%zu]: Found unexpected source loc", i);
102+
}
103+
#endif /*CONFIG_BT_PAC_SRC_LOC */
104+
68105
err = bt_pacs_unregister();
69106
zassert_equal(err, 0, "[%zu]: Unexpected return value %d", i, err);
107+
108+
attr = bt_gatt_find_by_uuid(NULL, 0, BT_UUID_PACS_SNK);
109+
zassert_is_null(attr, "[%zu]: Unexpected find of sink PAC", i);
110+
111+
attr = bt_gatt_find_by_uuid(NULL, 0, BT_UUID_PACS_SNK_LOC);
112+
zassert_is_null(attr, "[%zu]: Unexpected find of sink loc", i);
113+
114+
attr = bt_gatt_find_by_uuid(NULL, 0, BT_UUID_PACS_SRC);
115+
zassert_is_null(attr, "[%zu]: Unexpected find of source PAC", i);
116+
117+
attr = bt_gatt_find_by_uuid(NULL, 0, BT_UUID_PACS_SRC_LOC);
118+
zassert_is_null(attr, "[%zu]: Unexpected find of source loc", i);
70119
}
71120
}
72121

0 commit comments

Comments
 (0)