All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH BlueZ 1/4] doc: extend MediaEndpoint1 API with SelectQoS
@ 2023-10-28 14:39 Pauli Virtanen
  2023-10-28 14:39 ` [PATCH BlueZ 2/4] shared/bap: rename PAC op select -> select_codec, add select_qos Pauli Virtanen
                   ` (4 more replies)
  0 siblings, 5 replies; 7+ messages in thread
From: Pauli Virtanen @ 2023-10-28 14:39 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Pauli Virtanen

Change unicast BAP configuration to proceed as:

0. SelectProperties(endpoint)
1. ASCS Config Codec
2. ASCS Server notifies ASE
3. SelectQoS(configuration)  [optional]
4. ASCS Config QoS
5. SetConfiguration(transport)

Previously, SelectProperties had to return also the QoS
configuration. However, it is impossible for it to provide it properly,
because the values supported by the server are known only after ASCS
Config Codec.

This resolves the issue by adding a new method call, which is supposed
to return suitable QoS values.

Remove the QoS input parameter from the SelectProperties() call, as the
server supported QoS settings may depend on the codec configuration, and
are not known yet at that point.

For convenience, e.g. when mandatory QoS presets are used, the endpoint
does not need to implement SelectQoS(). In this case the QoS values
returned by SelectProperties are used.
---

Notes:
    Alternative to this is calling SelectProperties() twice at the
    two different stages of the ASE setup steps.
    
    However, if the second SelectProperties() call returns a different
    Capabilities configuration, we'd need to either (i) do Config Codec again
    or (ii) fail the configuration.
    
    Doing Config Codec again introduces a chance of getting stuck looping,
    if client is not behaving correctly, which doesn't sound like good
    design. Failing the configuration raises question why have the
    Capabilities as return parameters at all. So instead, make it a separate
    method.
    
    ***
    
    If two methods is too much, we could in principle get rid of the
    SelectProperties() call and leave only SelectQoS.
    
    Instead, the sound server would call SetConfiguration() on a remote
    endpoint it chooses, and provide the configuration parameters there.
    IIUC, this is how it is supposed to work for BAP Broadcast currently.
    This might need some sort of "Ready" property on the Device1 DBus object
    or elsewhere (e.g. the endpoints), so that it's simple for the sound
    server to wait until all endpoints have been exposed in DBus.
    
    This might also be preferable way to do it, since only the component
    closer to the user i.e. the sound server knows which endpoint the user
    wanted to use, and when BlueZ guesses wrong it avoids needing to tear
    down the old configuration and reconfigure (which we have to do for
    A2DP).
    
    ***
    
    This series was tested also vs. this
    https://gitlab.freedesktop.org/pvir/pipewire/-/commits/bap-selectqos

 doc/org.bluez.MediaEndpoint.rst | 66 +++++++++++++++++++++++++++++----
 1 file changed, 58 insertions(+), 8 deletions(-)

diff --git a/doc/org.bluez.MediaEndpoint.rst b/doc/org.bluez.MediaEndpoint.rst
index 6754d6e3b..4ffe6951c 100644
--- a/doc/org.bluez.MediaEndpoint.rst
+++ b/doc/org.bluez.MediaEndpoint.rst
@@ -66,6 +66,8 @@ array{byte} SelectConfiguration(array{byte} capabilities)
 	Note: There is no need to cache the selected configuration since on
 	success the configuration is send back as parameter of SetConfiguration.
 
+.. _SelectProperties:
+
 dict SelectProperties(dict capabilities)
 ````````````````````````````````````````
 
@@ -79,8 +81,58 @@ dict SelectProperties(dict capabilities)
 
 	:uint32 Locations:
 
+	See `MediaEndpoint Properties`_ for their possible values.
+
+	Returns a configuration which can be used to setup a transport:
+
+	:array{byte} Capabilities:
+
+		See **org.bluez.MediaTransport(5)**.
+
+	:array{byte} Metadata [optional]:
+
+		See **org.bluez.MediaTransport(5)**.
+
+	:dict QoS:
+
+		See **org.bluez.MediaTransport(5)**.
+
+		The following fields shall be provided:
+
+		:byte TargetLatency:
+		:byte PHY:
+
+		If `SelectQoS`_ is not implemented, then values for
+		all other ``QoS`` fields are also determined by the
+		value returned here.
+
+	Note: There is no need to cache the selected properties since
+	on success the configuration is sent back as parameter of
+	`SetConfiguration`_ and `SelectQoS`_.
+
+.. _SelectQoS:
+
+dict SelectQoS(dict configuration)
+``````````````````````````````````
+
+	Select BAP unicast QoS to be used for a transport, based on
+	server capabilities and selected configuration.
+
+	:object Endpoint:
+
+	:array{byte} Capabilities:
+
+		The configuration, as returned by `SelectProperties`_.
+
+	:array{byte} Metadata [optional]:
+
+		The metadata, as returned by `SelectProperties`_.
+
 	:dict QoS:
 
+		Server endpoint supported and preferred values.	 See
+		`MediaEndpoint Properties`_ for their possible values.
+
 		:byte Framing:
 		:byte PHY:
 		:uint16 MaximumLatency:
@@ -89,18 +141,16 @@ dict SelectProperties(dict capabilities)
 		:uint32 PreferredMinimumDelay:
 		:uint32 PreferredMaximumDelay:
 
-	See `MediaEndpoint Properties`_ for their possible values.
+	Returns a QoS configuration which can be used to setup a transport:
 
-	Returns a configuration which can be used to setup a transport:
-
-	:array{byte} Capabilities:
-	:array{byte} Metadata [optional]:
 	:dict QoS:
 
-	See `SetConfiguration`_ for their possible values.
+		See **org.bluez.MediaTransport(5)** QoS property for
+		possible values.
 
-	Note: There is no need to cache the selected properties since on
-	success the configuration is send back as parameter of SetConfiguration.
+	Note: There is no need to cache the selected properties since
+	on success the configuration is sent back as parameter of
+	`SetConfiguration`_.
 
 void ClearConfiguration(object transport)
 `````````````````````````````````````````
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH BlueZ 2/4] shared/bap: rename PAC op select -> select_codec, add select_qos
  2023-10-28 14:39 [PATCH BlueZ 1/4] doc: extend MediaEndpoint1 API with SelectQoS Pauli Virtanen
@ 2023-10-28 14:39 ` Pauli Virtanen
  2023-10-28 14:39 ` [PATCH BlueZ 3/4] bap: obtain BAP ucast client QoS via calling endpoint SelectQoS() Pauli Virtanen
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: Pauli Virtanen @ 2023-10-28 14:39 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Pauli Virtanen

The BAP Unicast Client stream configuration flow has two steps,
Config Codec and Config QoS.

Previously both the ASE Codec & QoS configuration was obtained from the
local PAC via bt_bap_pac_ops.select, but the information what the BAP
Server supports becomes available only after Config Codec completes
successfully.  So the single-step configuration doesn't work out
correctly in the API.

Split out the QoS configuration to a separate PAC operation.

Rename "select" to "select_codec", and add select_qos and
bt_bap_stream_select_qos for the QoS configuration step.

Also add bt_bap_stream_get_lpac/rpac, which will be needed in the QoS
configuration callback.
---
 src/shared/bap.c | 37 ++++++++++++++++++++++++++++++++-----
 src/shared/bap.h | 23 +++++++++++++++++------
 2 files changed, 49 insertions(+), 11 deletions(-)

diff --git a/src/shared/bap.c b/src/shared/bap.c
index 13bbcf793..6155b8640 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -4617,17 +4617,34 @@ static bool match_pac(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
 	return false;
 }
 
-int bt_bap_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
-			bt_bap_pac_select_t func, void *user_data)
+int bt_bap_select_codec(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
+			bt_bap_pac_select_codec_t func, void *user_data)
 {
 	if (!lpac || !rpac || !func)
 		return -EINVAL;
 
-	if (!lpac->ops || !lpac->ops->select)
+	if (!lpac->ops || !lpac->ops->select_codec)
 		return -EOPNOTSUPP;
 
-	lpac->ops->select(lpac, rpac, &rpac->qos,
-					func, user_data, lpac->user_data);
+	lpac->ops->select_codec(lpac, rpac, func, user_data, lpac->user_data);
+
+	return 0;
+}
+
+int bt_bap_stream_select_qos(struct bt_bap_stream *stream,
+				bt_bap_pac_select_qos_t func, void *user_data)
+{
+	struct bt_bap_pac *lpac = stream->lpac;
+	struct bt_bap_pac *rpac = stream->rpac;
+
+	if (!lpac || !rpac || !func)
+		return -EINVAL;
+
+	if (!lpac->ops || !lpac->ops->select_qos)
+		return -EOPNOTSUPP;
+
+	lpac->ops->select_qos(stream, &rpac->qos, func, user_data,
+							lpac->user_data);
 
 	return 0;
 }
@@ -5124,6 +5141,16 @@ uint32_t bt_bap_stream_get_location(struct bt_bap_stream *stream)
 		return stream->bap->ldb->pacs->source_loc_value;
 }
 
+struct bt_bap_pac *bt_bap_stream_get_lpac(struct bt_bap_stream *stream)
+{
+	return stream->lpac;
+}
+
+struct bt_bap_pac *bt_bap_stream_get_rpac(struct bt_bap_stream *stream)
+{
+	return stream->rpac;
+}
+
 struct iovec *bt_bap_stream_get_config(struct bt_bap_stream *stream)
 {
 	if (!stream)
diff --git a/src/shared/bap.h b/src/shared/bap.h
index 23edbf4c6..d8fae0ef8 100644
--- a/src/shared/bap.h
+++ b/src/shared/bap.h
@@ -104,12 +104,15 @@ typedef void (*bt_bap_pac_func_t)(struct bt_bap_pac *pac, void *user_data);
 typedef bool (*bt_bap_pac_foreach_t)(struct bt_bap_pac *lpac,
 					struct bt_bap_pac *rpac,
 					void *user_data);
-typedef void (*bt_bap_pac_select_t)(struct bt_bap_pac *pac, int err,
+typedef void (*bt_bap_pac_select_codec_t)(struct bt_bap_pac *pac, int err,
 					struct iovec *caps,
 					struct iovec *metadata,
 					struct bt_bap_qos *qos,
 					void *user_data);
 typedef void (*bt_bap_pac_config_t)(struct bt_bap_stream *stream, int err);
+typedef void (*bt_bap_pac_select_qos_t)(struct bt_bap_stream *stream,
+					int err, struct bt_bap_qos *qos,
+					void *user_data);
 typedef void (*bt_bap_state_func_t)(struct bt_bap_stream *stream,
 					uint8_t old_state, uint8_t new_state,
 					void *user_data);
@@ -150,9 +153,12 @@ struct bt_bap_pac *bt_bap_add_pac(struct gatt_db *db, const char *name,
 					struct iovec *metadata);
 
 struct bt_bap_pac_ops {
-	int (*select)(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
-			struct bt_bap_pac_qos *qos,
-			bt_bap_pac_select_t cb, void *cb_data, void *user_data);
+	int (*select_codec)(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
+			bt_bap_pac_select_codec_t cb, void *cb_data,
+			void *user_data);
+	int (*select_qos)(struct bt_bap_stream *stream,
+			struct bt_bap_pac_qos *qos, bt_bap_pac_select_qos_t cb,
+			void *cb_data, void *user_data);
 	int (*config)(struct bt_bap_stream *stream, struct iovec *cfg,
 			struct bt_bap_qos *qos, bt_bap_pac_config_t cb,
 			void *user_data);
@@ -233,8 +239,8 @@ void bt_bap_pac_set_user_data(struct bt_bap_pac *pac, void *user_data);
 void *bt_bap_pac_get_user_data(struct bt_bap_pac *pac);
 
 /* Stream related functions */
-int bt_bap_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
-			bt_bap_pac_select_t func, void *user_data);
+int bt_bap_select_codec(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
+			bt_bap_pac_select_codec_t func, void *user_data);
 
 struct bt_bap_stream *bt_bap_stream_new(struct bt_bap *bap,
 					struct bt_bap_pac *lpac,
@@ -249,6 +255,9 @@ bool bt_bap_stream_set_user_data(struct bt_bap_stream *stream, void *user_data);
 
 void *bt_bap_stream_get_user_data(struct bt_bap_stream *stream);
 
+int bt_bap_stream_select_qos(struct bt_bap_stream *stream,
+				bt_bap_pac_select_qos_t func, void *user_data);
+
 unsigned int bt_bap_stream_config(struct bt_bap_stream *stream,
 					struct bt_bap_qos *pqos,
 					struct iovec *data,
@@ -293,6 +302,8 @@ uint32_t bt_bap_stream_get_location(struct bt_bap_stream *stream);
 struct iovec *bt_bap_stream_get_config(struct bt_bap_stream *stream);
 struct bt_bap_qos *bt_bap_stream_get_qos(struct bt_bap_stream *stream);
 struct iovec *bt_bap_stream_get_metadata(struct bt_bap_stream *stream);
+struct bt_bap_pac *bt_bap_stream_get_lpac(struct bt_bap_stream *stream);
+struct bt_bap_pac *bt_bap_stream_get_rpac(struct bt_bap_stream *stream);
 
 struct io *bt_bap_stream_get_io(struct bt_bap_stream *stream);
 bool bt_bap_match_bcast_sink_stream(const void *data, const void *user_data);
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH BlueZ 3/4] bap: obtain BAP ucast client QoS via calling endpoint SelectQoS()
  2023-10-28 14:39 [PATCH BlueZ 1/4] doc: extend MediaEndpoint1 API with SelectQoS Pauli Virtanen
  2023-10-28 14:39 ` [PATCH BlueZ 2/4] shared/bap: rename PAC op select -> select_codec, add select_qos Pauli Virtanen
@ 2023-10-28 14:39 ` Pauli Virtanen
  2023-11-10 18:48   ` Pauli Virtanen
  2023-10-28 14:39 ` [PATCH BlueZ 4/4] client: implement SelectQoS Pauli Virtanen
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 7+ messages in thread
From: Pauli Virtanen @ 2023-10-28 14:39 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Pauli Virtanen

Enable the client endpoint to implement SelectQoS() to configure
the QoS as a second step in the configuration flow.

Remove the QoS parameter from SelectProperties(), as the values
are not actually know at that point of the configuration flow.

If the client does not implement SelectQoS() we will just use all the
QoS values returned by SelectProperties().  If they are one of the
mandatory configurations, then maybe devices will accept them.
---
 profiles/audio/bap.c   |  98 +++++++++++++-------
 profiles/audio/media.c | 201 +++++++++++++++++++++++++++++++++--------
 2 files changed, 225 insertions(+), 74 deletions(-)

diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
index b74498c4c..a289daf15 100644
--- a/profiles/audio/bap.c
+++ b/profiles/audio/bap.c
@@ -725,23 +725,17 @@ fail:
 	return -EINVAL;
 }
 
-static void qos_cb(struct bt_bap_stream *stream, uint8_t code, uint8_t reason,
-					void *user_data)
+static void ep_reply_msg(struct bap_ep *ep, const char *error)
 {
-	struct bap_ep *ep = user_data;
 	DBusMessage *reply;
 
-	DBG("stream %p code 0x%02x reason 0x%02x", stream, code, reason);
-
-	ep->id = 0;
-
 	if (!ep->msg)
 		return;
 
-	if (!code)
+	if (!error)
 		reply = dbus_message_new_method_return(ep->msg);
 	else
-		reply = btd_error_failed(ep->msg, "Unable to configure");
+		reply = btd_error_failed(ep->msg, error);
 
 	g_dbus_send_message(btd_get_dbus_connection(), reply);
 
@@ -749,28 +743,30 @@ static void qos_cb(struct bt_bap_stream *stream, uint8_t code, uint8_t reason,
 	ep->msg = NULL;
 }
 
-static void config_cb(struct bt_bap_stream *stream,
-					uint8_t code, uint8_t reason,
+static void qos_cb(struct bt_bap_stream *stream, uint8_t code, uint8_t reason,
 					void *user_data)
 {
 	struct bap_ep *ep = user_data;
-	DBusMessage *reply;
 
 	DBG("stream %p code 0x%02x reason 0x%02x", stream, code, reason);
 
 	ep->id = 0;
 
-	if (!code)
-		return;
+	ep_reply_msg(ep, code ? "Unable to configure" : NULL);
+}
 
-	if (!ep->msg)
-		return;
+static void config_cb(struct bt_bap_stream *stream,
+					uint8_t code, uint8_t reason,
+					void *user_data)
+{
+	struct bap_ep *ep = user_data;
 
-	reply = btd_error_failed(ep->msg, "Unable to configure");
-	g_dbus_send_message(btd_get_dbus_connection(), reply);
+	DBG("stream %p code 0x%02x reason 0x%02x", stream, code, reason);
 
-	dbus_message_unref(ep->msg);
-	ep->msg = NULL;
+	ep->id = 0;
+
+	if (code)
+		ep_reply_msg(ep, "Unable to configure");
 }
 
 static void bap_io_close(struct bap_ep *ep)
@@ -1202,7 +1198,7 @@ static void bap_config(void *data, void *user_data)
 	bt_bap_stream_set_user_data(ep->stream, ep->path);
 }
 
-static void select_cb(struct bt_bap_pac *pac, int err, struct iovec *caps,
+static void select_codec_cb(struct bt_bap_pac *pac, int err, struct iovec *caps,
 				struct iovec *metadata, struct bt_bap_qos *qos,
 				void *user_data)
 {
@@ -1252,7 +1248,7 @@ static bool pac_found(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
 
 	/* TODO: Cache LRU? */
 	if (btd_service_is_initiator(service)) {
-		if (!bt_bap_select(lpac, rpac, select_cb, ep))
+		if (!bt_bap_select_codec(lpac, rpac, select_codec_cb, ep))
 			ep->data->selecting++;
 	}
 
@@ -1877,6 +1873,36 @@ static void bap_create_io(struct bap_data *data, struct bap_ep *ep,
 	}
 }
 
+static void select_qos_cb(struct bt_bap_stream *stream, int err,
+					struct bt_bap_qos *qos, void *user_data)
+{
+	struct bap_ep *ep = user_data;
+
+	DBG("stream %p err %d qos %p", stream, err, qos);
+
+	if (err || ep->id)
+		goto fail;
+
+	if (qos)
+		ep->qos = *qos;
+
+	bap_create_io(ep->data, ep, stream, true);
+	if (!ep->io) {
+		error("Unable to create io");
+		goto fail;
+	}
+
+	ep->id = bt_bap_stream_qos(stream, &ep->qos, qos_cb, ep);
+	if (!ep->id)
+		goto fail;
+
+	return;
+
+fail:
+	error("Failed to Configure QoS");
+	ep_reply_msg(ep, "Unable to configure");
+}
+
 static void bap_state(struct bt_bap_stream *stream, uint8_t old_state,
 				uint8_t new_state, void *user_data)
 {
@@ -1902,25 +1928,27 @@ static void bap_state(struct bt_bap_stream *stream, uint8_t old_state,
 			queue_remove(data->streams, stream);
 		break;
 	case BT_BAP_STREAM_STATE_CONFIG:
-		if (ep && !ep->id) {
+		if (!ep || ep->id)
+			break;
+
+		switch (bt_bap_stream_get_type(stream)) {
+		case BT_BAP_STREAM_TYPE_UCAST:
+			if (bt_bap_stream_select_qos(stream,
+							select_qos_cb, ep)) {
+				error("Failed to Configure QoS");
+				bt_bap_stream_release(stream,
+						NULL, NULL);
+				return;
+			}
+			break;
+		case BT_BAP_STREAM_TYPE_BCAST:
 			bap_create_io(data, ep, stream, true);
 			if (!ep->io) {
 				error("Unable to create io");
 				bt_bap_stream_release(stream, NULL, NULL);
 				return;
 			}
-
-			if (bt_bap_stream_get_type(stream) ==
-					BT_BAP_STREAM_TYPE_UCAST) {
-				/* Wait QoS response to respond */
-				ep->id = bt_bap_stream_qos(stream, &ep->qos,
-								qos_cb,	ep);
-				if (!ep->id) {
-					error("Failed to Configure QoS");
-					bt_bap_stream_release(stream,
-								NULL, NULL);
-				}
-			}
+			break;
 		}
 		break;
 	case BT_BAP_STREAM_STATE_QOS:
diff --git a/profiles/audio/media.c b/profiles/audio/media.c
index 4d9a6aa03..42bc21386 100644
--- a/profiles/audio/media.c
+++ b/profiles/audio/media.c
@@ -318,6 +318,17 @@ static void endpoint_reply(DBusPendingCall *call, void *user_data)
 
 	dbus_error_init(&err);
 	if (dbus_set_error_from_message(&err, reply)) {
+		/* Endpoint is not required to implement SelectQoS */
+		if (dbus_error_has_name(&err, DBUS_ERROR_UNKNOWN_METHOD) &&
+		    dbus_message_is_method_call(request->msg,
+				    MEDIA_ENDPOINT_INTERFACE, "SelectQoS")) {
+			dbus_error_free(&err);
+			value = FALSE;
+			size = sizeof(value);
+			ret = &value;
+			goto done;
+		}
+
 		error("Endpoint replied with an error: %s",
 				err.name);
 
@@ -358,6 +369,13 @@ static void endpoint_reply(DBusPendingCall *call, void *user_data)
 		dbus_message_iter_recurse(&args, &props);
 		ret = &props;
 		goto done;
+	} else if (dbus_message_is_method_call(request->msg,
+						MEDIA_ENDPOINT_INTERFACE,
+						"SelectQoS")) {
+		dbus_message_iter_init(reply, &args);
+		dbus_message_iter_recurse(&args, &props);
+		ret = &props;
+		goto done;
 	} else if (!dbus_message_get_args(reply, &err, DBUS_TYPE_INVALID)) {
 		error("Wrong reply signature: %s", err.message);
 		dbus_error_free(&err);
@@ -725,9 +743,9 @@ static bool endpoint_init_a2dp_sink(struct media_endpoint *endpoint, int *err)
 	return true;
 }
 
-struct pac_select_data {
+struct pac_select_codec_data {
 	struct bt_bap_pac *pac;
-	bt_bap_pac_select_t cb;
+	bt_bap_pac_select_codec_t cb;
 	void *user_data;
 };
 
@@ -881,10 +899,10 @@ fail:
 	return -EINVAL;
 }
 
-static void pac_select_cb(struct media_endpoint *endpoint, void *ret, int size,
-							void *user_data)
+static void pac_select_codec_cb(struct media_endpoint *endpoint, void *ret,
+						int size, void *user_data)
 {
-	struct pac_select_data *data = user_data;
+	struct pac_select_codec_data *data = user_data;
 	DBusMessageIter *iter = ret;
 	int err;
 	struct iovec caps, meta;
@@ -920,15 +938,15 @@ done:
 	data->cb(data->pac, err, &caps, &meta, &qos, data->user_data);
 }
 
-static int pac_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
-			struct bt_bap_pac_qos *qos,
-			bt_bap_pac_select_t cb, void *cb_data, void *user_data)
+static int pac_select_codec(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
+			bt_bap_pac_select_codec_t cb, void *cb_data,
+			void *user_data)
 {
 	struct media_endpoint *endpoint = user_data;
 	struct iovec *caps;
 	struct iovec *metadata;
 	const char *endpoint_path;
-	struct pac_select_data *data;
+	struct pac_select_codec_data *data;
 	DBusMessage *msg;
 	DBusMessageIter iter, dict;
 	const char *key = "Capabilities";
@@ -946,7 +964,7 @@ static int pac_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
 		return -ENOMEM;
 	}
 
-	data = new0(struct pac_select_data, 1);
+	data = new0(struct pac_select_codec_data, 1);
 	data->pac = lpac;
 	data->cb = cb;
 	data->user_data = cb_data;
@@ -977,47 +995,151 @@ static int pac_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
 						metadata->iov_len);
 	}
 
-	if (qos && qos->phy) {
-		DBusMessageIter entry, variant, qos_dict;
+	dbus_message_iter_close_container(&iter, &dict);
+
+	return media_endpoint_async_call(msg, endpoint, NULL,
+					pac_select_codec_cb, data, free);
+}
+
+struct pac_select_qos_data {
+	struct bt_bap_stream *stream;
+	bt_bap_pac_select_qos_t cb;
+	void *user_data;
+};
+
+static void pac_select_qos_cb(struct media_endpoint *endpoint, void *ret,
+						int size, void *user_data)
+{
+	struct pac_select_qos_data *data = user_data;
+	DBusMessageIter *iter = ret;
+	int err;
+	struct bt_bap_qos qos;
+
+	if (!ret) {
+		data->cb(data->stream, -EPERM, NULL, data->user_data);
+		return;
+	} else if (size > 0) {
+		/* Endpoint doesn't implement the method, use old values */
+		data->cb(data->stream, 0, NULL, data->user_data);
+		return;
+	}
+
+	if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_DICT_ENTRY) {
+		DBG("Unexpected argument type: %c != %c",
+			    dbus_message_iter_get_arg_type(iter),
+			    DBUS_TYPE_DICT_ENTRY);
+		data->cb(data->stream, -EINVAL, NULL, data->user_data);
+		return;
+	}
+
+	memset(&qos, 0, sizeof(qos));
+
+	/* Mark CIG and CIS to be auto assigned */
+	qos.ucast.cig_id = BT_ISO_QOS_CIG_UNSET;
+	qos.ucast.cis_id = BT_ISO_QOS_CIS_UNSET;
+
+	err = parse_select_properties(iter, NULL, NULL, &qos);
+	if (err < 0)
+		DBG("Unable to parse properties");
+
+	data->cb(data->stream, err, &qos, data->user_data);
+}
 
-		key = "QoS";
-		dbus_message_iter_open_container(&dict, DBUS_TYPE_DICT_ENTRY,
-								NULL, &entry);
-		dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING, &key);
-		dbus_message_iter_open_container(&entry, DBUS_TYPE_VARIANT,
-							"a{sv}", &variant);
-		dbus_message_iter_open_container(&variant, DBUS_TYPE_ARRAY,
-							"{sv}", &qos_dict);
+static int pac_select_qos(struct bt_bap_stream *stream,
+			struct bt_bap_pac_qos *qos, bt_bap_pac_select_qos_t cb,
+			void *cb_data, void *user_data)
+{
+	struct media_endpoint *endpoint = user_data;
+	struct bt_bap_pac *rpac;
+	const char *endpoint_path;
+	struct pac_select_qos_data *data;
+	struct iovec *caps, *metadata;
+	DBusMessage *msg;
+	DBusMessageIter iter, dict;
+	DBusMessageIter entry, variant, qos_dict;
+	const char *key = "Capabilities";
+
+	rpac = bt_bap_stream_get_rpac(stream);
+	if (!rpac)
+		return -EINVAL;
 
-		g_dbus_dict_append_entry(&qos_dict, "Framing", DBUS_TYPE_BYTE,
-							&qos->framing);
+	caps = bt_bap_stream_get_config(stream);
+	if (!caps)
+		return -EINVAL;
 
-		g_dbus_dict_append_entry(&qos_dict, "PHY", DBUS_TYPE_BYTE,
-							&qos->phy);
+	msg = dbus_message_new_method_call(endpoint->sender, endpoint->path,
+						MEDIA_ENDPOINT_INTERFACE,
+						"SelectQoS");
+	if (msg == NULL) {
+		error("Couldn't allocate D-Bus message");
+		return -ENOMEM;
+	}
 
-		g_dbus_dict_append_entry(&qos_dict, "MaximumLatency",
-					DBUS_TYPE_UINT16, &qos->latency);
+	data = new0(struct pac_select_qos_data, 1);
+	data->stream = stream;
+	data->cb = cb;
+	data->user_data = cb_data;
 
-		g_dbus_dict_append_entry(&qos_dict, "MinimumDelay",
-					DBUS_TYPE_UINT32, &qos->pd_min);
+	dbus_message_iter_init_append(msg, &iter);
 
-		g_dbus_dict_append_entry(&qos_dict, "MaximumDelay",
-					DBUS_TYPE_UINT32, &qos->pd_max);
+	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY, "{sv}", &dict);
 
-		g_dbus_dict_append_entry(&qos_dict, "PreferredMinimumDelay",
-					DBUS_TYPE_UINT32, &qos->ppd_min);
+	endpoint_path = bt_bap_pac_get_user_data(rpac);
+	if (endpoint_path)
+		g_dbus_dict_append_entry(&dict, "Endpoint",
+					DBUS_TYPE_OBJECT_PATH, &endpoint_path);
 
-		g_dbus_dict_append_entry(&qos_dict, "PreferredMaximumDelay",
-					DBUS_TYPE_UINT32, &qos->ppd_max);
+	key = "Capabilities";
+	g_dbus_dict_append_basic_array(&dict, DBUS_TYPE_STRING, &key,
+						DBUS_TYPE_BYTE, &caps->iov_base,
+						caps->iov_len);
 
-		dbus_message_iter_close_container(&variant, &qos_dict);
-		dbus_message_iter_close_container(&entry, &variant);
-		dbus_message_iter_close_container(&dict, &entry);
+	metadata = bt_bap_stream_get_metadata(stream);
+	if (metadata) {
+		key = "Metadata";
+		g_dbus_dict_append_basic_array(&dict, DBUS_TYPE_STRING, &key,
+						DBUS_TYPE_BYTE,
+						&metadata->iov_base,
+						metadata->iov_len);
 	}
 
+	key = "QoS";
+	dbus_message_iter_open_container(&dict, DBUS_TYPE_DICT_ENTRY,
+			NULL, &entry);
+	dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING, &key);
+	dbus_message_iter_open_container(&entry, DBUS_TYPE_VARIANT,
+			"a{sv}", &variant);
+	dbus_message_iter_open_container(&variant, DBUS_TYPE_ARRAY,
+			"{sv}", &qos_dict);
+
+	g_dbus_dict_append_entry(&qos_dict, "Framing", DBUS_TYPE_BYTE,
+			&qos->framing);
+
+	g_dbus_dict_append_entry(&qos_dict, "PHY", DBUS_TYPE_BYTE,
+			&qos->phy);
+
+	g_dbus_dict_append_entry(&qos_dict, "MaximumLatency",
+			DBUS_TYPE_UINT16, &qos->latency);
+
+	g_dbus_dict_append_entry(&qos_dict, "MinimumDelay",
+			DBUS_TYPE_UINT32, &qos->pd_min);
+
+	g_dbus_dict_append_entry(&qos_dict, "MaximumDelay",
+			DBUS_TYPE_UINT32, &qos->pd_max);
+
+	g_dbus_dict_append_entry(&qos_dict, "PreferredMinimumDelay",
+			DBUS_TYPE_UINT32, &qos->ppd_min);
+
+	g_dbus_dict_append_entry(&qos_dict, "PreferredMaximumDelay",
+			DBUS_TYPE_UINT32, &qos->ppd_max);
+
+	dbus_message_iter_close_container(&variant, &qos_dict);
+	dbus_message_iter_close_container(&entry, &variant);
+	dbus_message_iter_close_container(&dict, &entry);
+
 	dbus_message_iter_close_container(&iter, &dict);
 
-	return media_endpoint_async_call(msg, endpoint, NULL, pac_select_cb,
+	return media_endpoint_async_call(msg, endpoint, NULL, pac_select_qos_cb,
 								data, free);
 }
 
@@ -1187,8 +1309,9 @@ static void pac_clear(struct bt_bap_stream *stream, void *user_data)
 }
 
 static struct bt_bap_pac_ops pac_ops = {
-	.select = pac_select,
+	.select_codec = pac_select_codec,
 	.config = pac_config,
+	.select_qos = pac_select_qos,
 	.clear = pac_clear,
 };
 
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH BlueZ 4/4] client: implement SelectQoS
  2023-10-28 14:39 [PATCH BlueZ 1/4] doc: extend MediaEndpoint1 API with SelectQoS Pauli Virtanen
  2023-10-28 14:39 ` [PATCH BlueZ 2/4] shared/bap: rename PAC op select -> select_codec, add select_qos Pauli Virtanen
  2023-10-28 14:39 ` [PATCH BlueZ 3/4] bap: obtain BAP ucast client QoS via calling endpoint SelectQoS() Pauli Virtanen
@ 2023-10-28 14:39 ` Pauli Virtanen
  2023-10-28 16:41 ` [BlueZ,1/4] doc: extend MediaEndpoint1 API with SelectQoS bluez.test.bot
  2023-11-15  0:22 ` [PATCH BlueZ 1/4] " Luiz Augusto von Dentz
  4 siblings, 0 replies; 7+ messages in thread
From: Pauli Virtanen @ 2023-10-28 14:39 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Pauli Virtanen

Implement SelectQoS() so that the server-provided values get printed.
We don't take them into account yet otherwise, though.
---
 client/player.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/client/player.c b/client/player.c
index 715598aa9..4ad6b99d9 100644
--- a/client/player.c
+++ b/client/player.c
@@ -91,6 +91,7 @@ struct endpoint {
 	struct queue *transports;
 	DBusMessage *msg;
 	struct preset *preset;
+	struct codec_preset *codec_preset;
 	bool broadcast;
 	struct iovec *bcode;
 };
@@ -2078,6 +2079,8 @@ static void select_properties_response(const char *input, void *user_data)
 	p = preset_find_name(ep->preset, input);
 	if (p) {
 		reply = endpoint_select_properties_reply(ep, ep->msg, p);
+		if (reply)
+			ep->codec_preset = p;
 		goto done;
 	}
 
@@ -2130,6 +2133,49 @@ static DBusMessage *endpoint_select_properties(DBusConnection *conn,
 	return reply;
 }
 
+static DBusMessage *endpoint_select_qos(DBusConnection *conn,
+					DBusMessage *msg, void *user_data)
+{
+	struct endpoint *ep = user_data;
+	struct codec_preset *preset = ep->codec_preset;
+	DBusMessageIter args;
+	DBusMessageIter iter, dict;
+	DBusMessage *reply;
+	struct endpoint_config cfg;
+
+	dbus_message_iter_init(msg, &args);
+
+	bt_shell_printf("Endpoint: SelectQoS\n");
+	print_iter("\t", "Properties", &args);
+
+	if (!preset)
+		return g_dbus_create_error(msg, "org.bluez.Error.Rejected",
+						"No previous codec preset");
+
+	reply = dbus_message_new_method_return(msg);
+	if (!reply)
+		return NULL;
+
+	dbus_message_iter_init_append(reply, &iter);
+
+	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY, "{sv}", &dict);
+
+	/* TODO: we ignore BAP Server supported values here. If they are not
+	 * suitable for the preset, we should prompt for a preset again, and
+	 * call SetConfiguration on the remote endpoint with the new
+	 * configuration to retry.
+	 */
+
+	memset(&cfg, 0, sizeof(cfg));
+	cfg.ep = ep;
+	cfg.qos = &preset->qos;
+	append_qos(&dict, &cfg);
+
+	dbus_message_iter_close_container(&iter, &dict);
+
+	return reply;
+}
+
 static bool match_str(const void *data, const void *user_data)
 {
 	return !strcmp(data, user_data);
@@ -2248,6 +2294,10 @@ static const GDBusMethodTable endpoint_methods[] = {
 					GDBUS_ARGS({ "properties", "a{sv}" } ),
 					GDBUS_ARGS({ "properties", "a{sv}" } ),
 					endpoint_select_properties) },
+	{ GDBUS_ASYNC_METHOD("SelectQoS",
+					GDBUS_ARGS({ "properties", "a{sv}" } ),
+					GDBUS_ARGS({ "properties", "a{sv}" } ),
+					endpoint_select_qos) },
 	{ GDBUS_ASYNC_METHOD("ClearConfiguration",
 					GDBUS_ARGS({ "transport", "o" } ),
 					NULL, endpoint_clear_configuration) },
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* RE: [BlueZ,1/4] doc: extend MediaEndpoint1 API with SelectQoS
  2023-10-28 14:39 [PATCH BlueZ 1/4] doc: extend MediaEndpoint1 API with SelectQoS Pauli Virtanen
                   ` (2 preceding siblings ...)
  2023-10-28 14:39 ` [PATCH BlueZ 4/4] client: implement SelectQoS Pauli Virtanen
@ 2023-10-28 16:41 ` bluez.test.bot
  2023-11-15  0:22 ` [PATCH BlueZ 1/4] " Luiz Augusto von Dentz
  4 siblings, 0 replies; 7+ messages in thread
From: bluez.test.bot @ 2023-10-28 16:41 UTC (permalink / raw)
  To: linux-bluetooth, pav

[-- Attachment #1: Type: text/plain, Size: 26441 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=797322

---Test result---

Test Summary:
CheckPatch                    FAIL      2.16 seconds
GitLint                       FAIL      1.20 seconds
BuildEll                      PASS      32.88 seconds
BluezMake                     PASS      1121.38 seconds
MakeCheck                     PASS      12.66 seconds
MakeDistcheck                 PASS      208.91 seconds
CheckValgrind                 PASS      354.45 seconds
CheckSmatch                   PASS      467.29 seconds
bluezmakeextell               PASS      146.12 seconds
IncrementalBuild              FAIL      2009.02 seconds
ScanBuild                     FAIL      904.53 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[BlueZ,4/4] client: implement SelectQoS
ERROR:SPACING: space prohibited before that close parenthesis ')'
#168: FILE: client/player.c:2298:
+					GDBUS_ARGS({ "properties", "a{sv}" } ),

ERROR:SPACING: space prohibited before that close parenthesis ')'
#169: FILE: client/player.c:2299:
+					GDBUS_ARGS({ "properties", "a{sv}" } ),

/github/workspace/src/src/13439494.patch total: 2 errors, 0 warnings, 74 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/13439494.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.


##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[BlueZ,1/4] doc: extend MediaEndpoint1 API with SelectQoS

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
32: B2 Line has trailing whitespace: "    "
36: B2 Line has trailing whitespace: "    "
42: B2 Line has trailing whitespace: "    "
44: B2 Line has trailing whitespace: "    "
47: B2 Line has trailing whitespace: "    "
54: B2 Line has trailing whitespace: "    "
60: B2 Line has trailing whitespace: "    "
62: B2 Line has trailing whitespace: "    "
##############################
Test: IncrementalBuild - FAIL
Desc: Incremental build with the patches in the series
Output:
[BlueZ,2/4] shared/bap: rename PAC op select -> select_codec, add select_qos

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[])
      |     ^~~~
profiles/audio/bap.c: In function ‘pac_found’:
profiles/audio/bap.c:1255:8: error: implicit declaration of function ‘bt_bap_select’; did you mean ‘bt_bap_detach’? [-Werror=implicit-function-declaration]
 1255 |   if (!bt_bap_select(lpac, rpac, select_cb, ep))
      |        ^~~~~~~~~~~~~
      |        bt_bap_detach
cc1: all warnings being treated as errors
make[1]: *** [Makefile:10576: profiles/audio/bluetoothd-bap.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:4677: all] Error 2
##############################
Test: ScanBuild - FAIL
Desc: Run Scan Build
Output:

src/shared/gatt-client.c:451:21: warning: Use of memory after it is freed
        gatt_db_unregister(op->client->db, op->db_id);
                           ^~~~~~~~~~
src/shared/gatt-client.c:696:2: warning: Use of memory after it is freed
        discovery_op_complete(op, false, att_ecode);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:993:2: warning: Use of memory after it is freed
        discovery_op_complete(op, success, att_ecode);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1099:2: warning: Use of memory after it is freed
        discovery_op_complete(op, success, att_ecode);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1291:2: warning: Use of memory after it is freed
        discovery_op_complete(op, success, att_ecode);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1356:2: warning: Use of memory after it is freed
        discovery_op_complete(op, success, att_ecode);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1631:6: warning: Use of memory after it is freed
        if (read_db_hash(op)) {
            ^~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1636:2: warning: Use of memory after it is freed
        discover_all(op);
        ^~~~~~~~~~~~~~~~
src/shared/gatt-client.c:2140:6: warning: Use of memory after it is freed
        if (read_db_hash(op)) {
            ^~~~~~~~~~~~~~~~
src/shared/gatt-client.c:2148:8: warning: Use of memory after it is freed
                                                        discovery_op_ref(op),
                                                        ^~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:3237:2: warning: Use of memory after it is freed
        complete_write_long_op(req, success, 0, false);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:3259:2: warning: Use of memory after it is freed
        request_unref(req);
        ^~~~~~~~~~~~~~~~~~
12 warnings generated.
src/shared/shell.c:1228:13: warning: Access to field 'options' results in a dereference of a null pointer (loaded from variable 'opt')
                        if (c != opt->options[index - offset].val) {
                                 ^~~~~~~~~~~~
1 warning generated.
src/shared/gatt-client.c:451:21: warning: Use of memory after it is freed
        gatt_db_unregister(op->client->db, op->db_id);
                           ^~~~~~~~~~
src/shared/gatt-client.c:696:2: warning: Use of memory after it is freed
        discovery_op_complete(op, false, att_ecode);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:993:2: warning: Use of memory after it is freed
        discovery_op_complete(op, success, att_ecode);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1099:2: warning: Use of memory after it is freed
        discovery_op_complete(op, success, att_ecode);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1291:2: warning: Use of memory after it is freed
        discovery_op_complete(op, success, att_ecode);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1356:2: warning: Use of memory after it is freed
        discovery_op_complete(op, success, att_ecode);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1631:6: warning: Use of memory after it is freed
        if (read_db_hash(op)) {
            ^~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1636:2: warning: Use of memory after it is freed
        discover_all(op);
        ^~~~~~~~~~~~~~~~
src/shared/gatt-client.c:2140:6: warning: Use of memory after it is freed
        if (read_db_hash(op)) {
            ^~~~~~~~~~~~~~~~
src/shared/gatt-client.c:2148:8: warning: Use of memory after it is freed
                                                        discovery_op_ref(op),
                                                        ^~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:3237:2: warning: Use of memory after it is freed
        complete_write_long_op(req, success, 0, false);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:3259:2: warning: Use of memory after it is freed
        request_unref(req);
        ^~~~~~~~~~~~~~~~~~
12 warnings generated.
src/shared/shell.c:1228:13: warning: Access to field 'options' results in a dereference of a null pointer (loaded from variable 'opt')
                        if (c != opt->options[index - offset].val) {
                                 ^~~~~~~~~~~~
1 warning generated.
tools/hciattach.c:816:7: warning: Although the value stored to 'n' is used in the enclosing expression, the value is never actually read from 'n'
        if ((n = read_hci_event(fd, resp, 10)) < 0) {
             ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/hciattach.c:864:7: warning: Although the value stored to 'n' is used in the enclosing expression, the value is never actually read from 'n'
        if ((n = read_hci_event(fd, resp, 4)) < 0) {
             ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/hciattach.c:886:8: warning: Although the value stored to 'n' is used in the enclosing expression, the value is never actually read from 'n'
                if ((n = read_hci_event(fd, resp, 10)) < 0) {
                     ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/hciattach.c:908:7: warning: Although the value stored to 'n' is used in the enclosing expression, the value is never actually read from 'n'
        if ((n = read_hci_event(fd, resp, 4)) < 0) {
             ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/hciattach.c:929:7: warning: Although the value stored to 'n' is used in the enclosing expression, the value is never actually read from 'n'
        if ((n = read_hci_event(fd, resp, 4)) < 0) {
             ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/hciattach.c:973:7: warning: Although the value stored to 'n' is used in the enclosing expression, the value is never actually read from 'n'
        if ((n = read_hci_event(fd, resp, 6)) < 0) {
             ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~
6 warnings generated.
src/oui.c:50:2: warning: Value stored to 'hwdb' is never read
        hwdb = udev_hwdb_unref(hwdb);
        ^      ~~~~~~~~~~~~~~~~~~~~~
src/oui.c:53:2: warning: Value stored to 'udev' is never read
        udev = udev_unref(udev);
        ^      ~~~~~~~~~~~~~~~~
2 warnings generated.
tools/hcidump.c:180:9: warning: Potential leak of memory pointed to by 'dp'
                                if (fds[i].fd == sock)
                                    ^~~
tools/hcidump.c:248:17: warning: Assigned value is garbage or undefined
                                dh->ts_sec  = htobl(frm.ts.tv_sec);
                                            ^ ~~~~~~~~~~~~~~~~~~~~
tools/hcidump.c:326:9: warning: 1st function call argument is an uninitialized value
                                if (be32toh(dp.flags) & 0x02) {
                                    ^~~~~~~~~~~~~~~~~
/usr/include/endian.h:46:22: note: expanded from macro 'be32toh'
#  define be32toh(x) __bswap_32 (x)
                     ^~~~~~~~~~~~~~
tools/hcidump.c:341:20: warning: 1st function call argument is an uninitialized value
                                frm.data_len = be32toh(dp.len);
                                               ^~~~~~~~~~~~~~~
/usr/include/endian.h:46:22: note: expanded from macro 'be32toh'
#  define be32toh(x) __bswap_32 (x)
                     ^~~~~~~~~~~~~~
tools/hcidump.c:346:14: warning: 1st function call argument is an uninitialized value
                                opcode = be32toh(dp.flags) & 0xffff;
                                         ^~~~~~~~~~~~~~~~~
/usr/include/endian.h:46:22: note: expanded from macro 'be32toh'
#  define be32toh(x) __bswap_32 (x)
                     ^~~~~~~~~~~~~~
tools/hcidump.c:384:17: warning: Assigned value is garbage or undefined
                        frm.data_len = btohs(dh.len);
                                     ^ ~~~~~~~~~~~~~
tools/hcidump.c:394:11: warning: Assigned value is garbage or undefined
                frm.len = frm.data_len;
                        ^ ~~~~~~~~~~~~
tools/hcidump.c:398:9: warning: 1st function call argument is an uninitialized value
                        ts = be64toh(ph.ts);
                             ^~~~~~~~~~~~~~
/usr/include/endian.h:51:22: note: expanded from macro 'be64toh'
#  define be64toh(x) __bswap_64 (x)
                     ^~~~~~~~~~~~~~
tools/hcidump.c:403:13: warning: 1st function call argument is an uninitialized value
                        frm.in = be32toh(dp.flags) & 0x01;
                                 ^~~~~~~~~~~~~~~~~
/usr/include/endian.h:46:22: note: expanded from macro 'be32toh'
#  define be32toh(x) __bswap_32 (x)
                     ^~~~~~~~~~~~~~
tools/hcidump.c:408:11: warning: Assigned value is garbage or undefined
                        frm.in = dh.in;
                               ^ ~~~~~
tools/hcidump.c:437:7: warning: Null pointer passed to 1st parameter expecting 'nonnull'
        fd = open(file, open_flags, 0644);
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
11 warnings generated.
tools/rfcomm.c:228:3: warning: Value stored to 'i' is never read
                i = execvp(cmdargv[0], cmdargv);
                ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/rfcomm.c:228:7: warning: Null pointer passed to 1st parameter expecting 'nonnull'
                i = execvp(cmdargv[0], cmdargv);
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/rfcomm.c:348:8: warning: Although the value stored to 'fd' is used in the enclosing expression, the value is never actually read from 'fd'
                if ((fd = open(devname, O_RDONLY | O_NOCTTY)) < 0) {
                     ^    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/rfcomm.c:491:14: warning: Assigned value is garbage or undefined
        req.channel = raddr.rc_channel;
                    ^ ~~~~~~~~~~~~~~~~
tools/rfcomm.c:509:8: warning: Although the value stored to 'fd' is used in the enclosing expression, the value is never actually read from 'fd'
                if ((fd = open(devname, O_RDONLY | O_NOCTTY)) < 0) {
                     ^    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
5 warnings generated.
src/sdp-xml.c:126:10: warning: Assigned value is garbage or undefined
                buf[1] = data[i + 1];
                       ^ ~~~~~~~~~~~
src/sdp-xml.c:300:11: warning: Assigned value is garbage or undefined
                        buf[1] = data[i + 1];
                               ^ ~~~~~~~~~~~
src/sdp-xml.c:338:11: warning: Assigned value is garbage or undefined
                        buf[1] = data[i + 1];
                               ^ ~~~~~~~~~~~
3 warnings generated.
tools/ciptool.c:350:7: warning: 5th function call argument is an uninitialized value
        sk = do_connect(ctl, dev_id, &src, &dst, psm, (1 << CMTP_LOOPBACK));
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
tools/sdptool.c:941:26: warning: Result of 'malloc' is converted to a pointer of type 'uint32_t', which is incompatible with sizeof operand type 'int'
                        uint32_t *value_int = malloc(sizeof(int));
                        ~~~~~~~~~~            ^~~~~~ ~~~~~~~~~~~
tools/sdptool.c:980:4: warning: 1st function call argument is an uninitialized value
                        free(allocArray[i]);
                        ^~~~~~~~~~~~~~~~~~~
tools/sdptool.c:3777:2: warning: Potential leak of memory pointed to by 'si.name'
        return add_service(0, &si);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~
tools/sdptool.c:4112:4: warning: Potential leak of memory pointed to by 'context.svc'
                        return -1;
                        ^~~~~~~~~
4 warnings generated.
tools/avtest.c:224:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 3);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:234:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 4);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:243:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 3);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:257:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf,
                                ^     ~~~~~~~~~~~~~~
tools/avtest.c:264:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf,
                                ^     ~~~~~~~~~~~~~~
tools/avtest.c:271:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf,
                                ^     ~~~~~~~~~~~~~~
tools/avtest.c:278:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf,
                                ^     ~~~~~~~~~~~~~~
tools/avtest.c:289:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 4);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:293:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 2);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:302:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 3);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:306:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 2);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:315:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 3);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:322:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 2);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:344:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 4);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:348:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 2);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:357:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 3);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:361:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 2);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:374:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 4);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:378:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 2);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:385:4: warning: Value stored to 'len' is never read
                        len = write(sk, buf, 2);
                        ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:395:4: warning: Value stored to 'len' is never read
                        len = write(sk, buf, 2);
                        ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:559:3: warning: Value stored to 'len' is never read
                len = write(sk, buf, 2);
                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:567:3: warning: Value stored to 'len' is never read
                len = write(sk, buf, invalid ? 2 : 3);
                ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/avtest.c:581:3: warning: Value stored to 'len' is never read
                len = write(sk, buf, 4 + sizeof(media_transport));
                ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/avtest.c:594:3: warning: Value stored to 'len' is never read
                len = write(sk, buf, 3);
                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:604:3: warning: Value stored to 'len' is never read
                len = write(sk, buf, 3);
                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:616:3: warning: Value stored to 'len' is never read
                len = write(sk, buf, 3);
                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:631:3: warning: Value stored to 'len' is never read
                len = write(sk, buf, 3);
                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:643:3: warning: Value stored to 'len' is never read
                len = write(sk, buf, 3);
                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:652:3: warning: Value stored to 'len' is never read
                len = write(sk, buf, 3);
                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:659:3: warning: Value stored to 'len' is never read
                len = write(sk, buf, 2);
                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:695:2: warning: Value stored to 'len' is never read
        len = write(sk, buf, AVCTP_HEADER_LENGTH + sizeof(play_pressed));
        ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
32 warnings generated.
tools/btproxy.c:836:15: warning: Null pointer passed to 1st parameter expecting 'nonnull'
                        tcp_port = atoi(optarg);
                                   ^~~~~~~~~~~~
tools/btproxy.c:839:8: warning: Null pointer passed to 1st parameter expecting 'nonnull'
                        if (strlen(optarg) > 3 && !strncmp(optarg, "hci", 3))
                            ^~~~~~~~~~~~~~
2 warnings generated.
tools/create-image.c:76:3: warning: Value stored to 'fd' is never read
                fd = -1;
                ^    ~~
tools/create-image.c:84:3: warning: Value stored to 'fd' is never read
                fd = -1;
                ^    ~~
tools/create-image.c:92:3: warning: Value stored to 'fd' is never read
                fd = -1;
                ^    ~~
tools/create-image.c:105:2: warning: Value stored to 'fd' is never read
        fd = -1;
        ^    ~~
4 warnings generated.
tools/btgatt-client.c:1597:2: warning: Value stored to 'argv' is never read
        argv += optind;
        ^       ~~~~~~
1 warning generated.
tools/btgatt-server.c:1212:2: warning: Value stored to 'argv' is never read
        argv -= optind;
        ^       ~~~~~~
1 warning generated.
tools/check-selftest.c:42:3: warning: Value stored to 'ptr' is never read
                ptr = fgets(result, sizeof(result), fp);
                ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
tools/gatt-service.c:294:2: warning: 2nd function call argument is an uninitialized value
        chr_write(chr, value, len);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
tools/obex-server-tool.c:133:13: warning: Null pointer passed to 1st parameter expecting 'nonnull'
        data->fd = open(name, O_WRONLY | O_CREAT | O_NOCTTY, 0600);
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/obex-server-tool.c:192:13: warning: Null pointer passed to 1st parameter expecting 'nonnull'
        data->fd = open(name, O_RDONLY | O_NOCTTY, 0);
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
tools/test-runner.c:945:2: warning: 2nd function call argument is an uninitialized value
        printf("Running command %s\n", cmdname ? cmdname : argv[0]);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
tools/btpclientctl.c:402:3: warning: Value stored to 'bit' is never read
                bit = 0;
                ^     ~
tools/btpclientctl.c:1655:2: warning: Null pointer passed to 2nd parameter expecting 'nonnull'
        memcpy(cp->data, ad_data, ad_len);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
src/sdpd-request.c:211:13: warning: Result of 'malloc' is converted to a pointer of type 'char', which is incompatible with sizeof operand type 'uint16_t'
                                pElem = malloc(sizeof(uint16_t));
                                        ^~~~~~ ~~~~~~~~~~~~~~~~
src/sdpd-request.c:239:13: warning: Result of 'malloc' is converted to a pointer of type 'char', which is incompatible with sizeof operand type 'uint32_t'
                                pElem = malloc(sizeof(uint32_t));
                                        ^~~~~~ ~~~~~~~~~~~~~~~~
2 warnings generated.
android/avrcp-lib.c:1968:3: warning: 1st function call argument is an uninitialized value
                g_free(text[i]);
                ^~~~~~~~~~~~~~~
1 warning generated.
profiles/health/hdp.c:644:3: warning: Use of memory after it is freed
                hdp_tmp_dc_data_unref(dc_data);
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
profiles/health/hdp.c:800:19: warning: Use of memory after it is freed
                path = g_strdup(chan->path);
                                ^~~~~~~~~~
profiles/health/hdp.c:1779:6: warning: Use of memory after it is freed
                                        hdp_tmp_dc_data_ref(hdp_conn),
                                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
profiles/health/hdp.c:1836:30: warning: Use of memory after it is freed
        reply = g_dbus_create_error(data->msg, ERROR_INTERFACE ".HealthError",
                                    ^~~~~~~~~
4 warnings generated.
profiles/health/hdp_util.c:1053:2: warning: Use of memory after it is freed
        conn_data->func(conn_data->data, gerr);
        ^~~~~~~~~~~~~~~
1 warning generated.
profiles/audio/bap.c: In function ‘pac_found’:
profiles/audio/bap.c:1255:8: error: implicit declaration of function ‘bt_bap_select’; did you mean ‘bt_bap_detach’? [-Werror=implicit-function-declaration]
 1255 |   if (!bt_bap_select(lpac, rpac, select_cb, ep))
      |        ^~~~~~~~~~~~~
      |        bt_bap_detach
cc1: all warnings being treated as errors
make[1]: *** [Makefile:10576: profiles/audio/bluetoothd-bap.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:4677: all] Error 2


---
Regards,
Linux Bluetooth


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH BlueZ 3/4] bap: obtain BAP ucast client QoS via calling endpoint SelectQoS()
  2023-10-28 14:39 ` [PATCH BlueZ 3/4] bap: obtain BAP ucast client QoS via calling endpoint SelectQoS() Pauli Virtanen
@ 2023-11-10 18:48   ` Pauli Virtanen
  0 siblings, 0 replies; 7+ messages in thread
From: Pauli Virtanen @ 2023-11-10 18:48 UTC (permalink / raw)
  To: linux-bluetooth

la, 2023-10-28 kello 17:39 +0300, Pauli Virtanen kirjoitti:
> Enable the client endpoint to implement SelectQoS() to configure
> the QoS as a second step in the configuration flow.
> 
> Remove the QoS parameter from SelectProperties(), as the values
> are not actually know at that point of the configuration flow.
> 
> If the client does not implement SelectQoS() we will just use all the
> QoS values returned by SelectProperties().  If they are one of the
> mandatory configurations, then maybe devices will accept them.
> ---
>  profiles/audio/bap.c   |  98 +++++++++++++-------
>  profiles/audio/media.c | 201 +++++++++++++++++++++++++++++++++--------
>  2 files changed, 225 insertions(+), 74 deletions(-)
> 
> diff --git a/profiles/audio/bap.c b/profiles/audio/bap.c
> index b74498c4c..a289daf15 100644
> --- a/profiles/audio/bap.c
> +++ b/profiles/audio/bap.c
> @@ -725,23 +725,17 @@ fail:
>  	return -EINVAL;
>  }
>  
> -static void qos_cb(struct bt_bap_stream *stream, uint8_t code, uint8_t reason,
> -					void *user_data)
> +static void ep_reply_msg(struct bap_ep *ep, const char *error)
>  {
> -	struct bap_ep *ep = user_data;
>  	DBusMessage *reply;
>  
> -	DBG("stream %p code 0x%02x reason 0x%02x", stream, code, reason);
> -
> -	ep->id = 0;
> -
>  	if (!ep->msg)
>  		return;
>  
> -	if (!code)
> +	if (!error)
>  		reply = dbus_message_new_method_return(ep->msg);
>  	else
> -		reply = btd_error_failed(ep->msg, "Unable to configure");
> +		reply = btd_error_failed(ep->msg, error);
>  
>  	g_dbus_send_message(btd_get_dbus_connection(), reply);
>  
> @@ -749,28 +743,30 @@ static void qos_cb(struct bt_bap_stream *stream, uint8_t code, uint8_t reason,
>  	ep->msg = NULL;
>  }
>  
> -static void config_cb(struct bt_bap_stream *stream,
> -					uint8_t code, uint8_t reason,
> +static void qos_cb(struct bt_bap_stream *stream, uint8_t code, uint8_t reason,
>  					void *user_data)
>  {
>  	struct bap_ep *ep = user_data;
> -	DBusMessage *reply;
>  
>  	DBG("stream %p code 0x%02x reason 0x%02x", stream, code, reason);
>  
>  	ep->id = 0;
>  
> -	if (!code)
> -		return;
> +	ep_reply_msg(ep, code ? "Unable to configure" : NULL);
> +}
>  
> -	if (!ep->msg)
> -		return;
> +static void config_cb(struct bt_bap_stream *stream,
> +					uint8_t code, uint8_t reason,
> +					void *user_data)
> +{
> +	struct bap_ep *ep = user_data;
>  
> -	reply = btd_error_failed(ep->msg, "Unable to configure");
> -	g_dbus_send_message(btd_get_dbus_connection(), reply);
> +	DBG("stream %p code 0x%02x reason 0x%02x", stream, code, reason);
>  
> -	dbus_message_unref(ep->msg);
> -	ep->msg = NULL;
> +	ep->id = 0;
> +
> +	if (code)
> +		ep_reply_msg(ep, "Unable to configure");
>  }
>  
>  static void bap_io_close(struct bap_ep *ep)
> @@ -1202,7 +1198,7 @@ static void bap_config(void *data, void *user_data)
>  	bt_bap_stream_set_user_data(ep->stream, ep->path);
>  }
>  
> -static void select_cb(struct bt_bap_pac *pac, int err, struct iovec *caps,
> +static void select_codec_cb(struct bt_bap_pac *pac, int err, struct iovec *caps,
>  				struct iovec *metadata, struct bt_bap_qos *qos,
>  				void *user_data)
>  {
> @@ -1252,7 +1248,7 @@ static bool pac_found(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
>  
>  	/* TODO: Cache LRU? */
>  	if (btd_service_is_initiator(service)) {
> -		if (!bt_bap_select(lpac, rpac, select_cb, ep))
> +		if (!bt_bap_select_codec(lpac, rpac, select_codec_cb, ep))
>  			ep->data->selecting++;
>  	}
>  
> @@ -1877,6 +1873,36 @@ static void bap_create_io(struct bap_data *data, struct bap_ep *ep,
>  	}
>  }
>  
> +static void select_qos_cb(struct bt_bap_stream *stream, int err,
> +					struct bt_bap_qos *qos, void *user_data)
> +{
> +	struct bap_ep *ep = user_data;
> +
> +	DBG("stream %p err %d qos %p", stream, err, qos);
> +
> +	if (err || ep->id)
> +		goto fail;
> +
> +	if (qos)
> +		ep->qos = *qos;
> +
> +	bap_create_io(ep->data, ep, stream, true);

This uses old QoS values, needs to be fixed.

For v2.

> +	if (!ep->io) {
> +		error("Unable to create io");
> +		goto fail;
> +	}
> +
> +	ep->id = bt_bap_stream_qos(stream, &ep->qos, qos_cb, ep);
> +	if (!ep->id)
> +		goto fail;
> +
> +	return;
> +
> +fail:
> +	error("Failed to Configure QoS");
> +	ep_reply_msg(ep, "Unable to configure");
> +}
> +
>  static void bap_state(struct bt_bap_stream *stream, uint8_t old_state,
>  				uint8_t new_state, void *user_data)
>  {
> @@ -1902,25 +1928,27 @@ static void bap_state(struct bt_bap_stream *stream, uint8_t old_state,
>  			queue_remove(data->streams, stream);
>  		break;
>  	case BT_BAP_STREAM_STATE_CONFIG:
> -		if (ep && !ep->id) {
> +		if (!ep || ep->id)
> +			break;
> +
> +		switch (bt_bap_stream_get_type(stream)) {
> +		case BT_BAP_STREAM_TYPE_UCAST:
> +			if (bt_bap_stream_select_qos(stream,
> +							select_qos_cb, ep)) {
> +				error("Failed to Configure QoS");
> +				bt_bap_stream_release(stream,
> +						NULL, NULL);
> +				return;
> +			}
> +			break;
> +		case BT_BAP_STREAM_TYPE_BCAST:
>  			bap_create_io(data, ep, stream, true);
>  			if (!ep->io) {
>  				error("Unable to create io");
>  				bt_bap_stream_release(stream, NULL, NULL);
>  				return;
>  			}
> -
> -			if (bt_bap_stream_get_type(stream) ==
> -					BT_BAP_STREAM_TYPE_UCAST) {
> -				/* Wait QoS response to respond */
> -				ep->id = bt_bap_stream_qos(stream, &ep->qos,
> -								qos_cb,	ep);
> -				if (!ep->id) {
> -					error("Failed to Configure QoS");
> -					bt_bap_stream_release(stream,
> -								NULL, NULL);
> -				}
> -			}
> +			break;
>  		}
>  		break;
>  	case BT_BAP_STREAM_STATE_QOS:
> diff --git a/profiles/audio/media.c b/profiles/audio/media.c
> index 4d9a6aa03..42bc21386 100644
> --- a/profiles/audio/media.c
> +++ b/profiles/audio/media.c
> @@ -318,6 +318,17 @@ static void endpoint_reply(DBusPendingCall *call, void *user_data)
>  
>  	dbus_error_init(&err);
>  	if (dbus_set_error_from_message(&err, reply)) {
> +		/* Endpoint is not required to implement SelectQoS */
> +		if (dbus_error_has_name(&err, DBUS_ERROR_UNKNOWN_METHOD) &&
> +		    dbus_message_is_method_call(request->msg,
> +				    MEDIA_ENDPOINT_INTERFACE, "SelectQoS")) {
> +			dbus_error_free(&err);
> +			value = FALSE;
> +			size = sizeof(value);
> +			ret = &value;
> +			goto done;
> +		}
> +
>  		error("Endpoint replied with an error: %s",
>  				err.name);
>  
> @@ -358,6 +369,13 @@ static void endpoint_reply(DBusPendingCall *call, void *user_data)
>  		dbus_message_iter_recurse(&args, &props);
>  		ret = &props;
>  		goto done;
> +	} else if (dbus_message_is_method_call(request->msg,
> +						MEDIA_ENDPOINT_INTERFACE,
> +						"SelectQoS")) {
> +		dbus_message_iter_init(reply, &args);
> +		dbus_message_iter_recurse(&args, &props);
> +		ret = &props;
> +		goto done;
>  	} else if (!dbus_message_get_args(reply, &err, DBUS_TYPE_INVALID)) {
>  		error("Wrong reply signature: %s", err.message);
>  		dbus_error_free(&err);
> @@ -725,9 +743,9 @@ static bool endpoint_init_a2dp_sink(struct media_endpoint *endpoint, int *err)
>  	return true;
>  }
>  
> -struct pac_select_data {
> +struct pac_select_codec_data {
>  	struct bt_bap_pac *pac;
> -	bt_bap_pac_select_t cb;
> +	bt_bap_pac_select_codec_t cb;
>  	void *user_data;
>  };
>  
> @@ -881,10 +899,10 @@ fail:
>  	return -EINVAL;
>  }
>  
> -static void pac_select_cb(struct media_endpoint *endpoint, void *ret, int size,
> -							void *user_data)
> +static void pac_select_codec_cb(struct media_endpoint *endpoint, void *ret,
> +						int size, void *user_data)
>  {
> -	struct pac_select_data *data = user_data;
> +	struct pac_select_codec_data *data = user_data;
>  	DBusMessageIter *iter = ret;
>  	int err;
>  	struct iovec caps, meta;
> @@ -920,15 +938,15 @@ done:
>  	data->cb(data->pac, err, &caps, &meta, &qos, data->user_data);
>  }
>  
> -static int pac_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
> -			struct bt_bap_pac_qos *qos,
> -			bt_bap_pac_select_t cb, void *cb_data, void *user_data)
> +static int pac_select_codec(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
> +			bt_bap_pac_select_codec_t cb, void *cb_data,
> +			void *user_data)
>  {
>  	struct media_endpoint *endpoint = user_data;
>  	struct iovec *caps;
>  	struct iovec *metadata;
>  	const char *endpoint_path;
> -	struct pac_select_data *data;
> +	struct pac_select_codec_data *data;
>  	DBusMessage *msg;
>  	DBusMessageIter iter, dict;
>  	const char *key = "Capabilities";
> @@ -946,7 +964,7 @@ static int pac_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
>  		return -ENOMEM;
>  	}
>  
> -	data = new0(struct pac_select_data, 1);
> +	data = new0(struct pac_select_codec_data, 1);
>  	data->pac = lpac;
>  	data->cb = cb;
>  	data->user_data = cb_data;
> @@ -977,47 +995,151 @@ static int pac_select(struct bt_bap_pac *lpac, struct bt_bap_pac *rpac,
>  						metadata->iov_len);
>  	}
>  
> -	if (qos && qos->phy) {
> -		DBusMessageIter entry, variant, qos_dict;
> +	dbus_message_iter_close_container(&iter, &dict);
> +
> +	return media_endpoint_async_call(msg, endpoint, NULL,
> +					pac_select_codec_cb, data, free);
> +}
> +
> +struct pac_select_qos_data {
> +	struct bt_bap_stream *stream;
> +	bt_bap_pac_select_qos_t cb;
> +	void *user_data;
> +};
> +
> +static void pac_select_qos_cb(struct media_endpoint *endpoint, void *ret,
> +						int size, void *user_data)
> +{
> +	struct pac_select_qos_data *data = user_data;
> +	DBusMessageIter *iter = ret;
> +	int err;
> +	struct bt_bap_qos qos;
> +
> +	if (!ret) {
> +		data->cb(data->stream, -EPERM, NULL, data->user_data);
> +		return;
> +	} else if (size > 0) {
> +		/* Endpoint doesn't implement the method, use old values */
> +		data->cb(data->stream, 0, NULL, data->user_data);
> +		return;
> +	}
> +
> +	if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_DICT_ENTRY) {
> +		DBG("Unexpected argument type: %c != %c",
> +			    dbus_message_iter_get_arg_type(iter),
> +			    DBUS_TYPE_DICT_ENTRY);
> +		data->cb(data->stream, -EINVAL, NULL, data->user_data);
> +		return;
> +	}
> +
> +	memset(&qos, 0, sizeof(qos));
> +
> +	/* Mark CIG and CIS to be auto assigned */
> +	qos.ucast.cig_id = BT_ISO_QOS_CIG_UNSET;
> +	qos.ucast.cis_id = BT_ISO_QOS_CIS_UNSET;
> +
> +	err = parse_select_properties(iter, NULL, NULL, &qos);
> +	if (err < 0)
> +		DBG("Unable to parse properties");
> +
> +	data->cb(data->stream, err, &qos, data->user_data);
> +}
>  
> -		key = "QoS";
> -		dbus_message_iter_open_container(&dict, DBUS_TYPE_DICT_ENTRY,
> -								NULL, &entry);
> -		dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING, &key);
> -		dbus_message_iter_open_container(&entry, DBUS_TYPE_VARIANT,
> -							"a{sv}", &variant);
> -		dbus_message_iter_open_container(&variant, DBUS_TYPE_ARRAY,
> -							"{sv}", &qos_dict);
> +static int pac_select_qos(struct bt_bap_stream *stream,
> +			struct bt_bap_pac_qos *qos, bt_bap_pac_select_qos_t cb,
> +			void *cb_data, void *user_data)
> +{
> +	struct media_endpoint *endpoint = user_data;
> +	struct bt_bap_pac *rpac;
> +	const char *endpoint_path;
> +	struct pac_select_qos_data *data;
> +	struct iovec *caps, *metadata;
> +	DBusMessage *msg;
> +	DBusMessageIter iter, dict;
> +	DBusMessageIter entry, variant, qos_dict;
> +	const char *key = "Capabilities";
> +
> +	rpac = bt_bap_stream_get_rpac(stream);
> +	if (!rpac)
> +		return -EINVAL;
>  
> -		g_dbus_dict_append_entry(&qos_dict, "Framing", DBUS_TYPE_BYTE,
> -							&qos->framing);
> +	caps = bt_bap_stream_get_config(stream);
> +	if (!caps)
> +		return -EINVAL;
>  
> -		g_dbus_dict_append_entry(&qos_dict, "PHY", DBUS_TYPE_BYTE,
> -							&qos->phy);
> +	msg = dbus_message_new_method_call(endpoint->sender, endpoint->path,
> +						MEDIA_ENDPOINT_INTERFACE,
> +						"SelectQoS");
> +	if (msg == NULL) {
> +		error("Couldn't allocate D-Bus message");
> +		return -ENOMEM;
> +	}
>  
> -		g_dbus_dict_append_entry(&qos_dict, "MaximumLatency",
> -					DBUS_TYPE_UINT16, &qos->latency);
> +	data = new0(struct pac_select_qos_data, 1);
> +	data->stream = stream;
> +	data->cb = cb;
> +	data->user_data = cb_data;
>  
> -		g_dbus_dict_append_entry(&qos_dict, "MinimumDelay",
> -					DBUS_TYPE_UINT32, &qos->pd_min);
> +	dbus_message_iter_init_append(msg, &iter);
>  
> -		g_dbus_dict_append_entry(&qos_dict, "MaximumDelay",
> -					DBUS_TYPE_UINT32, &qos->pd_max);
> +	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY, "{sv}", &dict);
>  
> -		g_dbus_dict_append_entry(&qos_dict, "PreferredMinimumDelay",
> -					DBUS_TYPE_UINT32, &qos->ppd_min);
> +	endpoint_path = bt_bap_pac_get_user_data(rpac);
> +	if (endpoint_path)
> +		g_dbus_dict_append_entry(&dict, "Endpoint",
> +					DBUS_TYPE_OBJECT_PATH, &endpoint_path);
>  
> -		g_dbus_dict_append_entry(&qos_dict, "PreferredMaximumDelay",
> -					DBUS_TYPE_UINT32, &qos->ppd_max);
> +	key = "Capabilities";
> +	g_dbus_dict_append_basic_array(&dict, DBUS_TYPE_STRING, &key,
> +						DBUS_TYPE_BYTE, &caps->iov_base,
> +						caps->iov_len);
>  
> -		dbus_message_iter_close_container(&variant, &qos_dict);
> -		dbus_message_iter_close_container(&entry, &variant);
> -		dbus_message_iter_close_container(&dict, &entry);
> +	metadata = bt_bap_stream_get_metadata(stream);
> +	if (metadata) {
> +		key = "Metadata";
> +		g_dbus_dict_append_basic_array(&dict, DBUS_TYPE_STRING, &key,
> +						DBUS_TYPE_BYTE,
> +						&metadata->iov_base,
> +						metadata->iov_len);
>  	}
>  
> +	key = "QoS";
> +	dbus_message_iter_open_container(&dict, DBUS_TYPE_DICT_ENTRY,
> +			NULL, &entry);
> +	dbus_message_iter_append_basic(&entry, DBUS_TYPE_STRING, &key);
> +	dbus_message_iter_open_container(&entry, DBUS_TYPE_VARIANT,
> +			"a{sv}", &variant);
> +	dbus_message_iter_open_container(&variant, DBUS_TYPE_ARRAY,
> +			"{sv}", &qos_dict);
> +
> +	g_dbus_dict_append_entry(&qos_dict, "Framing", DBUS_TYPE_BYTE,
> +			&qos->framing);
> +
> +	g_dbus_dict_append_entry(&qos_dict, "PHY", DBUS_TYPE_BYTE,
> +			&qos->phy);
> +
> +	g_dbus_dict_append_entry(&qos_dict, "MaximumLatency",
> +			DBUS_TYPE_UINT16, &qos->latency);
> +
> +	g_dbus_dict_append_entry(&qos_dict, "MinimumDelay",
> +			DBUS_TYPE_UINT32, &qos->pd_min);
> +
> +	g_dbus_dict_append_entry(&qos_dict, "MaximumDelay",
> +			DBUS_TYPE_UINT32, &qos->pd_max);
> +
> +	g_dbus_dict_append_entry(&qos_dict, "PreferredMinimumDelay",
> +			DBUS_TYPE_UINT32, &qos->ppd_min);
> +
> +	g_dbus_dict_append_entry(&qos_dict, "PreferredMaximumDelay",
> +			DBUS_TYPE_UINT32, &qos->ppd_max);
> +
> +	dbus_message_iter_close_container(&variant, &qos_dict);
> +	dbus_message_iter_close_container(&entry, &variant);
> +	dbus_message_iter_close_container(&dict, &entry);
> +
>  	dbus_message_iter_close_container(&iter, &dict);
>  
> -	return media_endpoint_async_call(msg, endpoint, NULL, pac_select_cb,
> +	return media_endpoint_async_call(msg, endpoint, NULL, pac_select_qos_cb,
>  								data, free);
>  }
>  
> @@ -1187,8 +1309,9 @@ static void pac_clear(struct bt_bap_stream *stream, void *user_data)
>  }
>  
>  static struct bt_bap_pac_ops pac_ops = {
> -	.select = pac_select,
> +	.select_codec = pac_select_codec,
>  	.config = pac_config,
> +	.select_qos = pac_select_qos,
>  	.clear = pac_clear,
>  };
>  


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH BlueZ 1/4] doc: extend MediaEndpoint1 API with SelectQoS
  2023-10-28 14:39 [PATCH BlueZ 1/4] doc: extend MediaEndpoint1 API with SelectQoS Pauli Virtanen
                   ` (3 preceding siblings ...)
  2023-10-28 16:41 ` [BlueZ,1/4] doc: extend MediaEndpoint1 API with SelectQoS bluez.test.bot
@ 2023-11-15  0:22 ` Luiz Augusto von Dentz
  4 siblings, 0 replies; 7+ messages in thread
From: Luiz Augusto von Dentz @ 2023-11-15  0:22 UTC (permalink / raw)
  To: Pauli Virtanen; +Cc: linux-bluetooth

Hi Pauli,

On Sat, Oct 28, 2023 at 10:42 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> Change unicast BAP configuration to proceed as:
>
> 0. SelectProperties(endpoint)
> 1. ASCS Config Codec
> 2. ASCS Server notifies ASE
> 3. SelectQoS(configuration)  [optional]
> 4. ASCS Config QoS
> 5. SetConfiguration(transport)
>
> Previously, SelectProperties had to return also the QoS
> configuration. However, it is impossible for it to provide it properly,
> because the values supported by the server are known only after ASCS
> Config Codec.
>
> This resolves the issue by adding a new method call, which is supposed
> to return suitable QoS values.
>
> Remove the QoS input parameter from the SelectProperties() call, as the
> server supported QoS settings may depend on the codec configuration, and
> are not known yet at that point.
>
> For convenience, e.g. when mandatory QoS presets are used, the endpoint
> does not need to implement SelectQoS(). In this case the QoS values
> returned by SelectProperties are used.

Id leave the endpoint the option to return the QoS on
SelectProperties, if it doesn't then we call SelectQoS, that said this
also means we need to wait until the endpoint respond with the QoS in
order to create the socket or we have to create the socket without any
QoS which afaik is required to bind.

> ---
>
> Notes:
>     Alternative to this is calling SelectProperties() twice at the
>     two different stages of the ASE setup steps.
>
>     However, if the second SelectProperties() call returns a different
>     Capabilities configuration, we'd need to either (i) do Config Codec again
>     or (ii) fail the configuration.
>
>     Doing Config Codec again introduces a chance of getting stuck looping,
>     if client is not behaving correctly, which doesn't sound like good
>     design. Failing the configuration raises question why have the
>     Capabilities as return parameters at all. So instead, make it a separate
>     method.

Well it is also possible that the server returns non-optiomal
preferences compared to a different codec configuration but we can
only know that once the codec is configured, so we need to be prepared
for the headset to misbehave like many do in case of A2DP, but in case
of BAP they can messed up in both Codec configuration and QoS stages.

>
>     ***
>
>     If two methods is too much, we could in principle get rid of the
>     SelectProperties() call and leave only SelectQoS.
>
>     Instead, the sound server would call SetConfiguration() on a remote
>     endpoint it chooses, and provide the configuration parameters there.
>     IIUC, this is how it is supposed to work for BAP Broadcast currently.
>     This might need some sort of "Ready" property on the Device1 DBus object
>     or elsewhere (e.g. the endpoints), so that it's simple for the sound
>     server to wait until all endpoints have been exposed in DBus.
>
>     This might also be preferable way to do it, since only the component
>     closer to the user i.e. the sound server knows which endpoint the user
>     wanted to use, and when BlueZ guesses wrong it avoids needing to tear
>     down the old configuration and reconfigure (which we have to do for
>     A2DP).

In principle Im nore in favor of keeping the logic of bluetoothd
calling into the endpoint object when it is necessary, we also may
apply the same logic as for A2DP that use the last configuration when
reconnecting, so there is no guessing but more of a policy of using
the last configuration to make the reconnections as quick as possible.
If the user wants to change the configuration he can do via
SetConfiguration, but this normally requires a user interaction, also
if the idea would be to have the reconnection policy on top of
bluetoothd Id say it is probably not recommended as that would
probably be slower and it may not know all the endpoints registered in
order to properly infer what is the best configuration to use.

>     ***
>
>     This series was tested also vs. this
>     https://gitlab.freedesktop.org/pvir/pipewire/-/commits/bap-selectqos
>
>  doc/org.bluez.MediaEndpoint.rst | 66 +++++++++++++++++++++++++++++----
>  1 file changed, 58 insertions(+), 8 deletions(-)
>
> diff --git a/doc/org.bluez.MediaEndpoint.rst b/doc/org.bluez.MediaEndpoint.rst
> index 6754d6e3b..4ffe6951c 100644
> --- a/doc/org.bluez.MediaEndpoint.rst
> +++ b/doc/org.bluez.MediaEndpoint.rst
> @@ -66,6 +66,8 @@ array{byte} SelectConfiguration(array{byte} capabilities)
>         Note: There is no need to cache the selected configuration since on
>         success the configuration is send back as parameter of SetConfiguration.
>
> +.. _SelectProperties:
> +
>  dict SelectProperties(dict capabilities)
>  ````````````````````````````````````````
>
> @@ -79,8 +81,58 @@ dict SelectProperties(dict capabilities)
>
>         :uint32 Locations:
>
> +       See `MediaEndpoint Properties`_ for their possible values.
> +
> +       Returns a configuration which can be used to setup a transport:
> +
> +       :array{byte} Capabilities:
> +
> +               See **org.bluez.MediaTransport(5)**.
> +
> +       :array{byte} Metadata [optional]:
> +
> +               See **org.bluez.MediaTransport(5)**.
> +
> +       :dict QoS:
> +
> +               See **org.bluez.MediaTransport(5)**.
> +
> +               The following fields shall be provided:
> +
> +               :byte TargetLatency:
> +               :byte PHY:
> +
> +               If `SelectQoS`_ is not implemented, then values for
> +               all other ``QoS`` fields are also determined by the
> +               value returned here.
> +
> +       Note: There is no need to cache the selected properties since
> +       on success the configuration is sent back as parameter of
> +       `SetConfiguration`_ and `SelectQoS`_.
> +
> +.. _SelectQoS:
> +
> +dict SelectQoS(dict configuration)
> +``````````````````````````````````
> +
> +       Select BAP unicast QoS to be used for a transport, based on
> +       server capabilities and selected configuration.
> +
> +       :object Endpoint:
> +
> +       :array{byte} Capabilities:
> +
> +               The configuration, as returned by `SelectProperties`_.
> +
> +       :array{byte} Metadata [optional]:
> +
> +               The metadata, as returned by `SelectProperties`_.
> +
>         :dict QoS:
>
> +               Server endpoint supported and preferred values.  See
> +               `MediaEndpoint Properties`_ for their possible values.
> +
>                 :byte Framing:
>                 :byte PHY:
>                 :uint16 MaximumLatency:
> @@ -89,18 +141,16 @@ dict SelectProperties(dict capabilities)
>                 :uint32 PreferredMinimumDelay:
>                 :uint32 PreferredMaximumDelay:
>
> -       See `MediaEndpoint Properties`_ for their possible values.
> +       Returns a QoS configuration which can be used to setup a transport:
>
> -       Returns a configuration which can be used to setup a transport:
> -
> -       :array{byte} Capabilities:
> -       :array{byte} Metadata [optional]:
>         :dict QoS:
>
> -       See `SetConfiguration`_ for their possible values.
> +               See **org.bluez.MediaTransport(5)** QoS property for
> +               possible values.
>
> -       Note: There is no need to cache the selected properties since on
> -       success the configuration is send back as parameter of SetConfiguration.
> +       Note: There is no need to cache the selected properties since
> +       on success the configuration is sent back as parameter of
> +       `SetConfiguration`_.
>
>  void ClearConfiguration(object transport)
>  `````````````````````````````````````````
> --
> 2.41.0
>


-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-11-15  0:22 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-28 14:39 [PATCH BlueZ 1/4] doc: extend MediaEndpoint1 API with SelectQoS Pauli Virtanen
2023-10-28 14:39 ` [PATCH BlueZ 2/4] shared/bap: rename PAC op select -> select_codec, add select_qos Pauli Virtanen
2023-10-28 14:39 ` [PATCH BlueZ 3/4] bap: obtain BAP ucast client QoS via calling endpoint SelectQoS() Pauli Virtanen
2023-11-10 18:48   ` Pauli Virtanen
2023-10-28 14:39 ` [PATCH BlueZ 4/4] client: implement SelectQoS Pauli Virtanen
2023-10-28 16:41 ` [BlueZ,1/4] doc: extend MediaEndpoint1 API with SelectQoS bluez.test.bot
2023-11-15  0:22 ` [PATCH BlueZ 1/4] " Luiz Augusto von Dentz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.