diff mbox series

[2/2] shared/bap: Add support for Audio Locations

Message ID 20231016065228.424400-2-kiran.k@intel.com (mailing list archive)
State New, archived
Headers show
Series [1/2] profiles: Add support for Audio Locations | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch warning WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line) #68: This adds support to provide Audio Locations for BAP Sink and Source Endpoints /github/workspace/src/src/13422604.patch total: 0 errors, 1 warnings, 161 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. /github/workspace/src/src/13422604.patch has style problems, please review. NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS.
tedd_an/GitLint success Gitlint PASS
tedd_an/IncrementalBuild fail [2/2] shared/bap: Add support for Audio Locations tools/mgmt-tester.c: In function ‘main’: tools/mgmt-tester.c:12763:5: note: variable tracking size limit exceeded with ‘-fvar-tracking-assignments’, retrying without 12763 | int main(int argc, char *argv[]) | ^~~~ unit/test-avdtp.c: In function ‘main’: unit/test-avdtp.c:766:5: note: variable tracking size limit exceeded with ‘-fvar-tracking-assignments’, retrying without 766 | int main(int argc, char *argv[]) | ^~~~ unit/test-avrcp.c: In function ‘main’: unit/test-avrcp.c:989:5: note: variable tracking size limit exceeded with ‘-fvar-tracking-assignments’, retrying without 989 | int main(int argc, char *argv[]) | ^~~~ unit/test-bap.c: In function ‘test_client_config’: unit/test-bap.c:376:16: error: too few arguments to function ‘bt_bap_add_vendor_pac’ 376 | data->snk = bt_bap_add_vendor_pac(data->db, | ^~~~~~~~~~~~~~~~~~~~~ In file included from unit/test-bap.c:32: ./src/shared/bap.h:139:20: note: declared here 139 | struct bt_bap_pac *bt_bap_add_vendor_pac(struct gatt_db *db, | ^~~~~~~~~~~~~~~~~~~~~ unit/test-bap.c:382:16: error: too few arguments to function ‘bt_bap_add_pac’ 382 | data->snk = bt_bap_add_pac(data->db, "test-bap-snk", | ^~~~~~~~~~~~~~ In file included from unit/test-bap.c:32: ./src/shared/bap.h:147:20: note: declared here 147 | struct bt_bap_pac *bt_bap_add_pac(struct gatt_db *db, const char *name, | ^~~~~~~~~~~~~~ unit/test-bap.c:390:16: error: too few arguments to function ‘bt_bap_add_vendor_pac’ 390 | data->src = bt_bap_add_vendor_pac(data->db, | ^~~~~~~~~~~~~~~~~~~~~ In file included from unit/test-bap.c:32: ./src/shared/bap.h:139:20: note: declared here 139 | struct bt_bap_pac *bt_bap_add_vendor_pac(struct gatt_db *db, | ^~~~~~~~~~~~~~~~~~~~~ unit/test-bap.c:396:16: error: too few arguments to function ‘bt_bap_add_pac’ 396 | data->src = bt_bap_add_pac(data->db, "test-bap-src", | ^~~~~~~~~~~~~~ In file included from unit/test-bap.c:32: ./src/shared/bap.h:147:20: note: declared here 147 | struct bt_bap_pac *bt_bap_add_pac(struct gatt_db *db, const char *name, | ^~~~~~~~~~~~~~ make[1]: *** [Makefile:7803: unit/test-bap.o] Error 1 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:4668: all] Error 2

Commit Message

K, Kiran Oct. 16, 2023, 6:52 a.m. UTC 
This adds support to provide Audio Locations for BAP Sink and Source Endpoints
---
 profiles/audio/media.c |  2 +-
 src/shared/bap.c       | 56 ++++++++++++++++++++++++++++++++----------
 src/shared/bap.h       |  6 +++--
 3 files changed, 48 insertions(+), 16 deletions(-)

Comments

Luiz Augusto von Dentz Oct. 16, 2023, 5:38 p.m. UTC | #1 
Hi Kiran,

On Sun, Oct 15, 2023 at 11:40 PM Kiran K <kiran.k@intel.com> wrote:
>
> This adds support to provide Audio Locations for BAP Sink and Source Endpoints
> ---
>  profiles/audio/media.c |  2 +-
>  src/shared/bap.c       | 56 ++++++++++++++++++++++++++++++++----------
>  src/shared/bap.h       |  6 +++--
>  3 files changed, 48 insertions(+), 16 deletions(-)
>
> diff --git a/profiles/audio/media.c b/profiles/audio/media.c
> index 51e3ab65d12d..d063bbf11cf9 100644
> --- a/profiles/audio/media.c
> +++ b/profiles/audio/media.c
> @@ -1250,7 +1250,7 @@ static bool endpoint_init_pac(struct media_endpoint *endpoint, uint8_t type,
>
>         endpoint->pac = bt_bap_add_vendor_pac(db, name, type, endpoint->codec,
>                                 endpoint->cid, endpoint->vid, &endpoint->qos,
> -                               &data, metadata);
> +                               &data, metadata, endpoint->location);
>         if (!endpoint->pac) {
>                 error("Unable to create PAC");
>                 free(metadata);
> diff --git a/src/shared/bap.c b/src/shared/bap.c
> index 925501c48d98..bee19039900f 100644
> --- a/src/shared/bap.c
> +++ b/src/shared/bap.c
> @@ -190,6 +190,7 @@ struct bt_bap_pac {
>         uint8_t type;
>         struct bt_bap_codec codec;
>         struct bt_bap_pac_qos qos;
> +       uint32_t location;
>         struct iovec *data;
>         struct iovec *metadata;
>         struct bt_bap_pac_ops *ops;
> @@ -368,6 +369,14 @@ static void pac_foreach(void *data, void *user_data)
>                 meta->len = 0;
>  }
>
> +static void get_pac_loc(void *data, void *user_data)
> +{
> +       struct bt_bap_pac *pac = data;
> +       uint32_t *location = user_data;
> +
> +       *location |= pac->location;
> +}
> +
>  static void pacs_sink_read(struct gatt_db_attribute *attrib,
>                                 unsigned int id, uint16_t offset,
>                                 uint8_t opcode, struct bt_att *att,
> @@ -395,7 +404,15 @@ static void pacs_sink_loc_read(struct gatt_db_attribute *attrib,
>                                 void *user_data)
>  {
>         struct bt_pacs *pacs = user_data;
> -       uint32_t value = cpu_to_le32(pacs->sink_loc_value);
> +       struct bt_bap_db *bdb = pacs->bdb;
> +       uint32_t value;
> +
> +       queue_foreach(bdb->sinks, get_pac_loc, &pacs->sink_loc_value);
> +       if (pacs->sink_loc_value)
> +               value = cpu_to_le32(pacs->sink_loc_value);
> +       else
> +               /* Set default value */
> +               value = cpu_to_le32(PACS_SNK_LOCATION);
>
>         gatt_db_attribute_read_result(attrib, id, 0, (void *) &value,
>                                                         sizeof(value));
> @@ -428,7 +445,15 @@ static void pacs_source_loc_read(struct gatt_db_attribute *attrib,
>                                 void *user_data)
>  {
>         struct bt_pacs *pacs = user_data;
> -       uint32_t value = cpu_to_le32(pacs->source_loc_value);
> +       struct bt_bap_db *bdb = pacs->bdb;
> +       uint32_t value;
> +
> +       queue_foreach(bdb->sources, get_pac_loc, &pacs->source_loc_value);
> +       if (pacs->source_loc_value)
> +               value = cpu_to_le32(pacs->source_loc_value);
> +       else
> +               /* Set default value */
> +               value = cpu_to_le32(PACS_SRC_LOCATION);
>
>         gatt_db_attribute_read_result(attrib, id, 0, (void *) &value,
>                                                         sizeof(value));
> @@ -474,9 +499,8 @@ static struct bt_pacs *pacs_new(struct gatt_db *db)
>
>         pacs = new0(struct bt_pacs, 1);
>
> -       /* Set default values */
> -       pacs->sink_loc_value = PACS_SNK_LOCATION;
> -       pacs->source_loc_value = PACS_SRC_LOCATION;
> +       pacs->sink_loc_value = 0;
> +       pacs->source_loc_value = 0;
>         pacs->sink_context_value = PACS_SNK_CTXT;
>         pacs->source_context_value = PACS_SRC_CTXT;
>         pacs->supported_sink_context_value = PACS_SUPPORTED_SNK_CTXT;
> @@ -2451,7 +2475,8 @@ static struct bt_bap_pac *bap_pac_new(struct bt_bap_db *bdb, const char *name,
>                                         struct bt_bap_codec *codec,
>                                         struct bt_bap_pac_qos *qos,
>                                         struct iovec *data,
> -                                       struct iovec *metadata)
> +                                       struct iovec *metadata,
> +                                       uint32_t location)
>  {
>         struct bt_bap_pac *pac;
>
> @@ -2468,6 +2493,8 @@ static struct bt_bap_pac *bap_pac_new(struct bt_bap_db *bdb, const char *name,
>         if (qos)
>                 pac->qos = *qos;
>
> +       pac->location = location;
> +
>         return pac;
>  }
>
> @@ -2679,7 +2706,8 @@ struct bt_bap_pac *bt_bap_add_vendor_pac(struct gatt_db *db,
>                                         uint8_t id, uint16_t cid, uint16_t vid,
>                                         struct bt_bap_pac_qos *qos,
>                                         struct iovec *data,
> -                                       struct iovec *metadata)
> +                                       struct iovec *metadata,
> +                                       uint32_t location)
>  {
>         struct bt_bap_db *bdb;
>         struct bt_bap_pac *pac, *pac_broadcast_sink;
> @@ -2699,7 +2727,8 @@ struct bt_bap_pac *bt_bap_add_vendor_pac(struct gatt_db *db,
>         codec.cid = cid;
>         codec.vid = vid;
>
> -       pac = bap_pac_new(bdb, name, type, &codec, qos, data, metadata);
> +       pac = bap_pac_new(bdb, name, type, &codec, qos, data, metadata,
> +                               location);
>
>         switch (type) {
>         case BT_BAP_SINK:
> @@ -2716,7 +2745,7 @@ struct bt_bap_pac *bt_bap_add_vendor_pac(struct gatt_db *db,
>                          */
>                         pac_broadcast_sink = bap_pac_new(bdb, name,
>                                         BT_BAP_BCAST_SINK, &codec, qos,
> -                                       data, metadata);
> +                                       data, metadata, 0);
>                         bap_add_broadcast_sink(pac_broadcast_sink);
>                 }
>                 break;
> @@ -2737,10 +2766,11 @@ struct bt_bap_pac *bt_bap_add_pac(struct gatt_db *db, const char *name,
>                                         uint8_t type, uint8_t id,
>                                         struct bt_bap_pac_qos *qos,
>                                         struct iovec *data,
> -                                       struct iovec *metadata)
> +                                       struct iovec *metadata,
> +                                       uint32_t location)
>  {
>         return bt_bap_add_vendor_pac(db, name, type, id, 0x0000, 0x0000, qos,
> -                                                       data, metadata);
> +                                               data, metadata, location);
>  }
>
>  uint8_t bt_bap_pac_get_type(struct bt_bap_pac *pac)
> @@ -3256,7 +3286,7 @@ static void bap_parse_pacs(struct bt_bap *bap, uint8_t type,
>                 }
>
>                 pac = bap_pac_new(bap->rdb, NULL, type, &p->codec, NULL, &data,
> -                                                               &metadata);
> +                                                       &metadata, 0);
>                 if (!pac)
>                         continue;
>
> @@ -5481,7 +5511,7 @@ bool bt_bap_new_bcast_source(struct bt_bap *bap, const char *name)
>                 return true;
>
>         pac_broadcast_source = bap_pac_new(bap->rdb, name, BT_BAP_BCAST_SOURCE,
> -                       NULL, NULL, NULL, NULL);
> +                       NULL, NULL, NULL, NULL, 0);
>         queue_push_tail(bap->rdb->broadcast_sources, pac_broadcast_source);
>
>         if (!pac_broadcast_source)
> diff --git a/src/shared/bap.h b/src/shared/bap.h
> index ebe4dbf7d858..10e82f35e547 100644
> --- a/src/shared/bap.h
> +++ b/src/shared/bap.h
> @@ -141,13 +141,15 @@ struct bt_bap_pac *bt_bap_add_vendor_pac(struct gatt_db *db,
>                                         uint8_t id, uint16_t cid, uint16_t vid,
>                                         struct bt_bap_pac_qos *qos,
>                                         struct iovec *data,
> -                                       struct iovec *metadata);
> +                                       struct iovec *metadata,
> +                                       uint32_t location);
>
>  struct bt_bap_pac *bt_bap_add_pac(struct gatt_db *db, const char *name,
>                                         uint8_t type, uint8_t id,
>                                         struct bt_bap_pac_qos *qos,
>                                         struct iovec *data,
> -                                       struct iovec *metadata);
> +                                       struct iovec *metadata,
> +                                       uint32_t location);

If you change the API you will need to fix their users as well
otherwise it won't build and CI will fail.

>  struct bt_bap_pac_ops {
>         int (*select)(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
> --
> 2.34.1
K, Kiran Oct. 17, 2023, 2:31 a.m. UTC | #2 
Hi Luiz,

> >  struct bt_bap_pac *bt_bap_add_pac(struct gatt_db *db, const char *name,
> >                                         uint8_t type, uint8_t id,
> >                                         struct bt_bap_pac_qos *qos,
> >                                         struct iovec *data,
> > -                                       struct iovec *metadata);
> > +                                       struct iovec *metadata,
> > +                                       uint32_t location);
> 
> If you change the API you will need to fix their users as well otherwise it
> won't build and CI will fail.

Agree. I also found later that location field is already part of qos structure. I would re-work to use the same instead of changing API and submit v2 patchset.
> 
> >  struct bt_bap_pac_ops {
> >         int (*select)(struct bt_bap_pac *lpac, struct bt_bap_pac
> > *rpac,
> > --
> > 2.34.1
> 
> 
> 
> --
> Luiz Augusto von Dentz

Thanks,
Kiran
diff mbox series

Patch

diff --git a/profiles/audio/media.c b/profiles/audio/media.c
index 51e3ab65d12d..d063bbf11cf9 100644
--- a/profiles/audio/media.c
+++ b/profiles/audio/media.c
@@ -1250,7 +1250,7 @@  static bool endpoint_init_pac(struct media_endpoint *endpoint, uint8_t type,
 
 	endpoint->pac = bt_bap_add_vendor_pac(db, name, type, endpoint->codec,
 				endpoint->cid, endpoint->vid, &endpoint->qos,
-				&data, metadata);
+				&data, metadata, endpoint->location);
 	if (!endpoint->pac) {
 		error("Unable to create PAC");
 		free(metadata);
diff --git a/src/shared/bap.c b/src/shared/bap.c
index 925501c48d98..bee19039900f 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -190,6 +190,7 @@  struct bt_bap_pac {
 	uint8_t type;
 	struct bt_bap_codec codec;
 	struct bt_bap_pac_qos qos;
+	uint32_t location;
 	struct iovec *data;
 	struct iovec *metadata;
 	struct bt_bap_pac_ops *ops;
@@ -368,6 +369,14 @@  static void pac_foreach(void *data, void *user_data)
 		meta->len = 0;
 }
 
+static void get_pac_loc(void *data, void *user_data)
+{
+	struct bt_bap_pac *pac = data;
+	uint32_t *location = user_data;
+
+	*location |= pac->location;
+}
+
 static void pacs_sink_read(struct gatt_db_attribute *attrib,
 				unsigned int id, uint16_t offset,
 				uint8_t opcode, struct bt_att *att,
@@ -395,7 +404,15 @@  static void pacs_sink_loc_read(struct gatt_db_attribute *attrib,
 				void *user_data)
 {
 	struct bt_pacs *pacs = user_data;
-	uint32_t value = cpu_to_le32(pacs->sink_loc_value);
+	struct bt_bap_db *bdb = pacs->bdb;
+	uint32_t value;
+
+	queue_foreach(bdb->sinks, get_pac_loc, &pacs->sink_loc_value);
+	if (pacs->sink_loc_value)
+		value = cpu_to_le32(pacs->sink_loc_value);
+	else
+		/* Set default value */
+		value = cpu_to_le32(PACS_SNK_LOCATION);
 
 	gatt_db_attribute_read_result(attrib, id, 0, (void *) &value,
 							sizeof(value));
@@ -428,7 +445,15 @@  static void pacs_source_loc_read(struct gatt_db_attribute *attrib,
 				void *user_data)
 {
 	struct bt_pacs *pacs = user_data;
-	uint32_t value = cpu_to_le32(pacs->source_loc_value);
+	struct bt_bap_db *bdb = pacs->bdb;
+	uint32_t value;
+
+	queue_foreach(bdb->sources, get_pac_loc, &pacs->source_loc_value);
+	if (pacs->source_loc_value)
+		value = cpu_to_le32(pacs->source_loc_value);
+	else
+		/* Set default value */
+		value = cpu_to_le32(PACS_SRC_LOCATION);
 
 	gatt_db_attribute_read_result(attrib, id, 0, (void *) &value,
 							sizeof(value));
@@ -474,9 +499,8 @@  static struct bt_pacs *pacs_new(struct gatt_db *db)
 
 	pacs = new0(struct bt_pacs, 1);
 
-	/* Set default values */
-	pacs->sink_loc_value = PACS_SNK_LOCATION;
-	pacs->source_loc_value = PACS_SRC_LOCATION;
+	pacs->sink_loc_value = 0;
+	pacs->source_loc_value = 0;
 	pacs->sink_context_value = PACS_SNK_CTXT;
 	pacs->source_context_value = PACS_SRC_CTXT;
 	pacs->supported_sink_context_value = PACS_SUPPORTED_SNK_CTXT;
@@ -2451,7 +2475,8 @@  static struct bt_bap_pac *bap_pac_new(struct bt_bap_db *bdb, const char *name,
 					struct bt_bap_codec *codec,
 					struct bt_bap_pac_qos *qos,
 					struct iovec *data,
-					struct iovec *metadata)
+					struct iovec *metadata,
+					uint32_t location)
 {
 	struct bt_bap_pac *pac;
 
@@ -2468,6 +2493,8 @@  static struct bt_bap_pac *bap_pac_new(struct bt_bap_db *bdb, const char *name,
 	if (qos)
 		pac->qos = *qos;
 
+	pac->location = location;
+
 	return pac;
 }
 
@@ -2679,7 +2706,8 @@  struct bt_bap_pac *bt_bap_add_vendor_pac(struct gatt_db *db,
 					uint8_t id, uint16_t cid, uint16_t vid,
 					struct bt_bap_pac_qos *qos,
 					struct iovec *data,
-					struct iovec *metadata)
+					struct iovec *metadata,
+					uint32_t location)
 {
 	struct bt_bap_db *bdb;
 	struct bt_bap_pac *pac, *pac_broadcast_sink;
@@ -2699,7 +2727,8 @@  struct bt_bap_pac *bt_bap_add_vendor_pac(struct gatt_db *db,
 	codec.cid = cid;
 	codec.vid = vid;
 
-	pac = bap_pac_new(bdb, name, type, &codec, qos, data, metadata);
+	pac = bap_pac_new(bdb, name, type, &codec, qos, data, metadata,
+				location);
 
 	switch (type) {
 	case BT_BAP_SINK:
@@ -2716,7 +2745,7 @@  struct bt_bap_pac *bt_bap_add_vendor_pac(struct gatt_db *db,
 			 */
 			pac_broadcast_sink = bap_pac_new(bdb, name,
 					BT_BAP_BCAST_SINK, &codec, qos,
-					data, metadata);
+					data, metadata, 0);
 			bap_add_broadcast_sink(pac_broadcast_sink);
 		}
 		break;
@@ -2737,10 +2766,11 @@  struct bt_bap_pac *bt_bap_add_pac(struct gatt_db *db, const char *name,
 					uint8_t type, uint8_t id,
 					struct bt_bap_pac_qos *qos,
 					struct iovec *data,
-					struct iovec *metadata)
+					struct iovec *metadata,
+					uint32_t location)
 {
 	return bt_bap_add_vendor_pac(db, name, type, id, 0x0000, 0x0000, qos,
-							data, metadata);
+						data, metadata, location);
 }
 
 uint8_t bt_bap_pac_get_type(struct bt_bap_pac *pac)
@@ -3256,7 +3286,7 @@  static void bap_parse_pacs(struct bt_bap *bap, uint8_t type,
 		}
 
 		pac = bap_pac_new(bap->rdb, NULL, type, &p->codec, NULL, &data,
-								&metadata);
+							&metadata, 0);
 		if (!pac)
 			continue;
 
@@ -5481,7 +5511,7 @@  bool bt_bap_new_bcast_source(struct bt_bap *bap, const char *name)
 		return true;
 
 	pac_broadcast_source = bap_pac_new(bap->rdb, name, BT_BAP_BCAST_SOURCE,
-			NULL, NULL, NULL, NULL);
+			NULL, NULL, NULL, NULL, 0);
 	queue_push_tail(bap->rdb->broadcast_sources, pac_broadcast_source);
 
 	if (!pac_broadcast_source)
diff --git a/src/shared/bap.h b/src/shared/bap.h
index ebe4dbf7d858..10e82f35e547 100644
--- a/src/shared/bap.h
+++ b/src/shared/bap.h
@@ -141,13 +141,15 @@  struct bt_bap_pac *bt_bap_add_vendor_pac(struct gatt_db *db,
 					uint8_t id, uint16_t cid, uint16_t vid,
 					struct bt_bap_pac_qos *qos,
 					struct iovec *data,
-					struct iovec *metadata);
+					struct iovec *metadata,
+					uint32_t location);
 
 struct bt_bap_pac *bt_bap_add_pac(struct gatt_db *db, const char *name,
 					uint8_t type, uint8_t id,
 					struct bt_bap_pac_qos *qos,
 					struct iovec *data,
-					struct iovec *metadata);
+					struct iovec *metadata,
+					uint32_t location);
 
 struct bt_bap_pac_ops {
 	int (*select)(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,