[PATCH BlueZ] shared/bap: wait for GATT client ready before ASCS/PACS discovery

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH BlueZ] shared/bap: wait for GATT client ready before ASCS/PACS discovery
@ 2023-02-12 18:34 Pauli Virtanen
  2023-02-12 20:05 ` [BlueZ] " bluez.test.bot
  2023-02-12 20:45 ` [PATCH BlueZ] " Luiz Augusto von Dentz
  0 siblings, 2 replies; 5+ messages in thread
From: Pauli Virtanen @ 2023-02-12 18:34 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Pauli Virtanen

BAP client fails to find endpoints if GATT services are not resolved
when the GATT client is attached to the session.  This condition
almost always occurs on the first BAP connection to a device between
bluetoothd restarts, and may occur also otherwise.

The BAP client code discovers ASCS/PACS services and characteristics at
client attach time.

Fix the connection issue by postponing PAC/ASE discovery until the
GATT client is ready, if it was not ready at attach time.
---

Notes:
    Typical bluetoothctl output in the failing case, without this patch:
    
    $ sudo systemctl restart bluetooth
    $ bluetoothctl
    ...
    [bluetooth]# connect XX:XX:XX:XX:XX:XX
    Attempting to connect to XX:XX:XX:XX:XX:XX
    [CHG] Device XX:XX:XX:XX:XX:XX Connected: yes
    [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 0000184e-0000-1000-8000-00805f9b34fb
    [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 00001850-0000-1000-8000-00805f9b34fb
    [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 03b80e5a-ede8-4b33-a751-6ce34ec4c700
    Connection successful
    [NEW] Primary Service (Handle 0x0000)
            /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/service0008
            00001801-0000-1000-8000-00805f9b34fb
            Generic Attribute Profile
    ...
    [NEW] Descriptor (Handle 0x0000)
            /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/service0065/char0066/desc0069
            00002901-0000-1000-8000-00805f9b34fb
            Characteristic User Description
    [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 00001800-0000-1000-8000-00805f9b34fb
    [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 00001801-0000-1000-8000-00805f9b34fb
    [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 0000180a-0000-1000-8000-00805f9b34fb
    [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 00001844-0000-1000-8000-00805f9b34fb
    [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 0000184e-0000-1000-8000-00805f9b34fb
    [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 00001850-0000-1000-8000-00805f9b34fb
    [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 03b80e5a-ede8-4b33-a751-6ce34ec4c700
    [CHG] Device XX:XX:XX:XX:XX:XX ServicesResolved: yes
    [CHG] Device XX:XX:XX:XX:XX:XX Paired: yes
    [xxx]# endpoint.list
    [xxx]# disconnect
    Attempting to disconnect from XX:XX:XX:XX:XX:XX
    [CHG] Device XX:XX:XX:XX:XX:XX ServicesResolved: no
    Successful disconnected
    [CHG] Device XX:XX:XX:XX:XX:XX Connected: no
    [bluetooth]# connect XX:XX:XX:XX:XX:XX
    Attempting to connect to XX:XX:XX:XX:XX:XX
    [CHG] Device XX:XX:XX:XX:XX:XX Connected: yes
    Connection successful
    [NEW] Endpoint /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0
    [NEW] Endpoint /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0
    [NEW] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0
    [NEW] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1
    [CHG] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0 Links: /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1
    [CHG] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1 Links: /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0
    [CHG] Device XX:XX:XX:XX:XX:XX ServicesResolved: yes
    [xxx]# endpoint.list
    Endpoint /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0
    Endpoint /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0
    [xxx]#
    
    Endpoints and transports failed to appear on the first connect to the
    BAP server, since GATT discovery was not yet completed when the BAP code
    tried to discover the endpoints. Second connection succeeded.
    Occasionally, it also happens that endpoints appear but transports do
    not, but this is hard to reproduce.
    
    With this patch:
    
    $ sudo systemctl restart bluetooth
    $ bluetoothctl
    ...
    [bluetooth]# connect XX:XX:XX:XX:XX:XX
    Attempting to connect to XX:XX:XX:XX:XX:XX
    [CHG] Device XX:XX:XX:XX:XX:XX Connected: yes
    [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 0000184e-0000-1000-8000-00805f9b34fb
    [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 00001850-0000-1000-8000-00805f9b34fb
    [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 03b80e5a-ede8-4b33-a751-6ce34ec4c700
    Connection successful
    [NEW] Primary Service (Handle 0x0000)
            /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/service0008
            00001801-0000-1000-8000-00805f9b34fb
            Generic Attribute Profile
    ...
    [NEW] Descriptor (Handle 0x0000)
            /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/service0065/char0066/desc0069
            00002901-0000-1000-8000-00805f9b34fb
            Characteristic User Description
    [CHG] Device XX:XX:XX:XX:XX:XX ServicesResolved: yes
    [CHG] Device XX:XX:XX:XX:XX:XX Paired: yes
    [NEW] Endpoint /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0
    [NEW] Endpoint /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0
    [NEW] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0
    [NEW] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1
    [CHG] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0 Links: /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1
    [CHG] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1 Links: /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0
    [xxx]#
    
    Now the first connection works properly.
    
    On first reading, the spec does not seem to clearly comment if ASEs and
    PACs could be removed/added by the server while client is connected. If
    that's allowed, then we'd need some bigger changes here. Regardless,
    waiting for GATT client ready before first scan seems good also in this
    case.

 src/shared/bap.c | 93 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 60 insertions(+), 33 deletions(-)

diff --git a/src/shared/bap.c b/src/shared/bap.c
index 22f2e6714..2d676d96f 100644
--- a/src/shared/bap.c
+++ b/src/shared/bap.c
@@ -158,6 +158,7 @@ struct bt_bap {
 	struct bt_att *att;
 	struct bt_bap_req *req;
 	unsigned int cp_id;
+	unsigned int client_ready_id;
 
 	unsigned int process_id;
 	unsigned int disconn_id;
@@ -3733,41 +3734,10 @@ static void bap_attach_att(struct bt_bap *bap, struct bt_att *att)
 							bap, NULL);
 }
 
-bool bt_bap_attach(struct bt_bap *bap, struct bt_gatt_client *client)
+static void bap_attach_finish(struct bt_bap *bap)
 {
 	bt_uuid_t uuid;
 
-	if (queue_find(sessions, NULL, bap)) {
-		/* If instance already been set but there is no client proceed
-		 * to clone it otherwise considered it already attached.
-		 */
-		if (client && !bap->client)
-			goto clone;
-		return true;
-	}
-
-	if (!sessions)
-		sessions = queue_new();
-
-	queue_push_tail(sessions, bap);
-
-	queue_foreach(bap_cbs, bap_attached, bap);
-
-	if (!client) {
-		bap_attach_att(bap, bap->att);
-		return true;
-	}
-
-	if (bap->client)
-		return false;
-
-clone:
-	bap->client = bt_gatt_client_clone(client);
-	if (!bap->client)
-		return false;
-
-	bap_attach_att(bap, bt_gatt_client_get_att(client));
-
 	if (bap->rdb->pacs) {
 		uint16_t value_handle;
 		struct bt_pacs *pacs = bap->rdb->pacs;
@@ -3798,7 +3768,7 @@ clone:
 
 		bap_notify_ready(bap);
 
-		return true;
+		return;
 	}
 
 	bt_uuid16_create(&uuid, PACS_UUID);
@@ -3806,6 +3776,57 @@ clone:
 
 	bt_uuid16_create(&uuid, ASCS_UUID);
 	gatt_db_foreach_service(bap->rdb->db, &uuid, foreach_ascs_service, bap);
+}
+
+static void bap_attach_ready_cb(bool success, uint8_t att_ecode,
+								void *user_data)
+{
+	struct bt_bap *bap = user_data;
+
+	bap->client_ready_id = 0;
+
+	bap_attach_finish(bap);
+}
+
+bool bt_bap_attach(struct bt_bap *bap, struct bt_gatt_client *client)
+{
+	if (queue_find(sessions, NULL, bap)) {
+		/* If instance already been set but there is no client proceed
+		 * to clone it otherwise considered it already attached.
+		 */
+		if (client && !bap->client)
+			goto clone;
+		return true;
+	}
+
+	if (!sessions)
+		sessions = queue_new();
+
+	queue_push_tail(sessions, bap);
+
+	queue_foreach(bap_cbs, bap_attached, bap);
+
+	if (!client) {
+		bap_attach_att(bap, bap->att);
+		return true;
+	}
+
+	if (bap->client)
+		return false;
+
+clone:
+	bap->client = bt_gatt_client_clone(client);
+	if (!bap->client)
+		return false;
+
+	bap_attach_att(bap, bt_gatt_client_get_att(bap->client));
+
+	if (bt_gatt_client_is_ready(bap->client)) {
+		bap_attach_finish(bap);
+	} else {
+		bap->client_ready_id = bt_gatt_client_ready_register(
+				bap->client, bap_attach_ready_cb, bap, NULL);
+	}
 
 	return true;
 }
@@ -3824,6 +3845,12 @@ void bt_bap_detach(struct bt_bap *bap)
 	if (!queue_remove(sessions, bap))
 		return;
 
+	if (bap->client_ready_id) {
+		bt_gatt_client_ready_unregister(bap->client,
+						bap->client_ready_id);
+		bap->client_ready_id = 0;
+	}
+
 	bt_gatt_client_unref(bap->client);
 	bap->client = NULL;
 
-- 
2.39.1


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

* RE: [BlueZ] shared/bap: wait for GATT client ready before ASCS/PACS discovery
  2023-02-12 18:34 [PATCH BlueZ] shared/bap: wait for GATT client ready before ASCS/PACS discovery Pauli Virtanen
@ 2023-02-12 20:05 ` bluez.test.bot
  2023-02-12 20:45 ` [PATCH BlueZ] " Luiz Augusto von Dentz
  1 sibling, 0 replies; 5+ messages in thread
From: bluez.test.bot @ 2023-02-12 20:05 UTC (permalink / raw)
  To: linux-bluetooth, pav

[-- Attachment #1: Type: text/plain, Size: 2438 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=721100

---Test result---

Test Summary:
CheckPatch                    PASS      0.43 seconds
GitLint                       FAIL      0.53 seconds
BuildEll                      PASS      26.40 seconds
BluezMake                     PASS      787.59 seconds
MakeCheck                     PASS      11.08 seconds
MakeDistcheck                 PASS      147.95 seconds
CheckValgrind                 PASS      241.21 seconds
CheckSmatch                   PASS      319.48 seconds
bluezmakeextell               PASS      95.77 seconds
IncrementalBuild              PASS      604.62 seconds
ScanBuild                     PASS      954.53 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[BlueZ] shared/bap: wait for GATT client ready before ASCS/PACS discovery

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
17: B2 Line has trailing whitespace: "    "
60: B1 Line exceeds max length (132>80): "    [CHG] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0 Links: /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1"
61: B1 Line exceeds max length (132>80): "    [CHG] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1 Links: /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0"
67: B2 Line has trailing whitespace: "    "
73: B2 Line has trailing whitespace: "    "
75: B2 Line has trailing whitespace: "    "
101: B1 Line exceeds max length (132>80): "    [CHG] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0 Links: /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1"
102: B1 Line exceeds max length (132>80): "    [CHG] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1 Links: /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0"
104: B2 Line has trailing whitespace: "    "
106: B2 Line has trailing whitespace: "    "


---
Regards,
Linux Bluetooth


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

* Re: [PATCH BlueZ] shared/bap: wait for GATT client ready before ASCS/PACS discovery
  2023-02-12 18:34 [PATCH BlueZ] shared/bap: wait for GATT client ready before ASCS/PACS discovery Pauli Virtanen
  2023-02-12 20:05 ` [BlueZ] " bluez.test.bot
@ 2023-02-12 20:45 ` Luiz Augusto von Dentz
  2023-02-12 21:49   ` Pauli Virtanen
  1 sibling, 1 reply; 5+ messages in thread
From: Luiz Augusto von Dentz @ 2023-02-12 20:45 UTC (permalink / raw)
  To: Pauli Virtanen; +Cc: linux-bluetooth

Hi Pauli,

On Sun, Feb 12, 2023 at 10:41 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> BAP client fails to find endpoints if GATT services are not resolved
> when the GATT client is attached to the session.  This condition
> almost always occurs on the first BAP connection to a device between
> bluetoothd restarts, and may occur also otherwise.

Hmm, this shouldn't happen if you have caching enabled, did you
disable it for some reason?

> The BAP client code discovers ASCS/PACS services and characteristics at
> client attach time.
>
> Fix the connection issue by postponing PAC/ASE discovery until the
> GATT client is ready, if it was not ready at attach time.

This should probably be done by the core, it should check if the
services are cached then it can call accept, otherwise it needs to
wait for the client to be ready, perhaps there is something not quite
right with the way to probe services as afaik if we don't have any
cache the driver shouldn't have been called to begin with.

> ---
>
> Notes:
>     Typical bluetoothctl output in the failing case, without this patch:
>
>     $ sudo systemctl restart bluetooth
>     $ bluetoothctl
>     ...
>     [bluetooth]# connect XX:XX:XX:XX:XX:XX
>     Attempting to connect to XX:XX:XX:XX:XX:XX
>     [CHG] Device XX:XX:XX:XX:XX:XX Connected: yes
>     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 0000184e-0000-1000-8000-00805f9b34fb
>     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 00001850-0000-1000-8000-00805f9b34fb
>     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 03b80e5a-ede8-4b33-a751-6ce34ec4c700
>     Connection successful
>     [NEW] Primary Service (Handle 0x0000)
>             /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/service0008
>             00001801-0000-1000-8000-00805f9b34fb
>             Generic Attribute Profile
>     ...
>     [NEW] Descriptor (Handle 0x0000)
>             /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/service0065/char0066/desc0069
>             00002901-0000-1000-8000-00805f9b34fb
>             Characteristic User Description
>     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 00001800-0000-1000-8000-00805f9b34fb
>     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 00001801-0000-1000-8000-00805f9b34fb
>     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 0000180a-0000-1000-8000-00805f9b34fb
>     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 00001844-0000-1000-8000-00805f9b34fb
>     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 0000184e-0000-1000-8000-00805f9b34fb
>     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 00001850-0000-1000-8000-00805f9b34fb
>     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 03b80e5a-ede8-4b33-a751-6ce34ec4c700
>     [CHG] Device XX:XX:XX:XX:XX:XX ServicesResolved: yes
>     [CHG] Device XX:XX:XX:XX:XX:XX Paired: yes
>     [xxx]# endpoint.list
>     [xxx]# disconnect
>     Attempting to disconnect from XX:XX:XX:XX:XX:XX
>     [CHG] Device XX:XX:XX:XX:XX:XX ServicesResolved: no
>     Successful disconnected
>     [CHG] Device XX:XX:XX:XX:XX:XX Connected: no
>     [bluetooth]# connect XX:XX:XX:XX:XX:XX
>     Attempting to connect to XX:XX:XX:XX:XX:XX
>     [CHG] Device XX:XX:XX:XX:XX:XX Connected: yes
>     Connection successful
>     [NEW] Endpoint /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0
>     [NEW] Endpoint /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0
>     [NEW] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0
>     [NEW] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1
>     [CHG] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0 Links: /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1
>     [CHG] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1 Links: /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0
>     [CHG] Device XX:XX:XX:XX:XX:XX ServicesResolved: yes
>     [xxx]# endpoint.list
>     Endpoint /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0
>     Endpoint /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0
>     [xxx]#
>
>     Endpoints and transports failed to appear on the first connect to the
>     BAP server, since GATT discovery was not yet completed when the BAP code
>     tried to discover the endpoints. Second connection succeeded.
>     Occasionally, it also happens that endpoints appear but transports do
>     not, but this is hard to reproduce.
>
>     With this patch:
>
>     $ sudo systemctl restart bluetooth
>     $ bluetoothctl
>     ...
>     [bluetooth]# connect XX:XX:XX:XX:XX:XX
>     Attempting to connect to XX:XX:XX:XX:XX:XX
>     [CHG] Device XX:XX:XX:XX:XX:XX Connected: yes
>     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 0000184e-0000-1000-8000-00805f9b34fb
>     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 00001850-0000-1000-8000-00805f9b34fb
>     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 03b80e5a-ede8-4b33-a751-6ce34ec4c700
>     Connection successful
>     [NEW] Primary Service (Handle 0x0000)
>             /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/service0008
>             00001801-0000-1000-8000-00805f9b34fb
>             Generic Attribute Profile
>     ...
>     [NEW] Descriptor (Handle 0x0000)
>             /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/service0065/char0066/desc0069
>             00002901-0000-1000-8000-00805f9b34fb
>             Characteristic User Description
>     [CHG] Device XX:XX:XX:XX:XX:XX ServicesResolved: yes
>     [CHG] Device XX:XX:XX:XX:XX:XX Paired: yes
>     [NEW] Endpoint /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0
>     [NEW] Endpoint /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0
>     [NEW] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0
>     [NEW] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1
>     [CHG] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0 Links: /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1
>     [CHG] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1 Links: /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0
>     [xxx]#
>
>     Now the first connection works properly.
>
>     On first reading, the spec does not seem to clearly comment if ASEs and
>     PACs could be removed/added by the server while client is connected. If
>     that's allowed, then we'd need some bigger changes here. Regardless,
>     waiting for GATT client ready before first scan seems good also in this
>     case.
>
>  src/shared/bap.c | 93 +++++++++++++++++++++++++++++++-----------------
>  1 file changed, 60 insertions(+), 33 deletions(-)
>
> diff --git a/src/shared/bap.c b/src/shared/bap.c
> index 22f2e6714..2d676d96f 100644
> --- a/src/shared/bap.c
> +++ b/src/shared/bap.c
> @@ -158,6 +158,7 @@ struct bt_bap {
>         struct bt_att *att;
>         struct bt_bap_req *req;
>         unsigned int cp_id;
> +       unsigned int client_ready_id;
>
>         unsigned int process_id;
>         unsigned int disconn_id;
> @@ -3733,41 +3734,10 @@ static void bap_attach_att(struct bt_bap *bap, struct bt_att *att)
>                                                         bap, NULL);
>  }
>
> -bool bt_bap_attach(struct bt_bap *bap, struct bt_gatt_client *client)
> +static void bap_attach_finish(struct bt_bap *bap)
>  {
>         bt_uuid_t uuid;
>
> -       if (queue_find(sessions, NULL, bap)) {
> -               /* If instance already been set but there is no client proceed
> -                * to clone it otherwise considered it already attached.
> -                */
> -               if (client && !bap->client)
> -                       goto clone;
> -               return true;
> -       }
> -
> -       if (!sessions)
> -               sessions = queue_new();
> -
> -       queue_push_tail(sessions, bap);
> -
> -       queue_foreach(bap_cbs, bap_attached, bap);
> -
> -       if (!client) {
> -               bap_attach_att(bap, bap->att);
> -               return true;
> -       }
> -
> -       if (bap->client)
> -               return false;
> -
> -clone:
> -       bap->client = bt_gatt_client_clone(client);
> -       if (!bap->client)
> -               return false;
> -
> -       bap_attach_att(bap, bt_gatt_client_get_att(client));
> -
>         if (bap->rdb->pacs) {
>                 uint16_t value_handle;
>                 struct bt_pacs *pacs = bap->rdb->pacs;
> @@ -3798,7 +3768,7 @@ clone:
>
>                 bap_notify_ready(bap);
>
> -               return true;
> +               return;
>         }
>
>         bt_uuid16_create(&uuid, PACS_UUID);
> @@ -3806,6 +3776,57 @@ clone:
>
>         bt_uuid16_create(&uuid, ASCS_UUID);
>         gatt_db_foreach_service(bap->rdb->db, &uuid, foreach_ascs_service, bap);
> +}
> +
> +static void bap_attach_ready_cb(bool success, uint8_t att_ecode,
> +                                                               void *user_data)
> +{
> +       struct bt_bap *bap = user_data;
> +
> +       bap->client_ready_id = 0;
> +
> +       bap_attach_finish(bap);
> +}
> +
> +bool bt_bap_attach(struct bt_bap *bap, struct bt_gatt_client *client)
> +{
> +       if (queue_find(sessions, NULL, bap)) {
> +               /* If instance already been set but there is no client proceed
> +                * to clone it otherwise considered it already attached.
> +                */
> +               if (client && !bap->client)
> +                       goto clone;
> +               return true;
> +       }
> +
> +       if (!sessions)
> +               sessions = queue_new();
> +
> +       queue_push_tail(sessions, bap);
> +
> +       queue_foreach(bap_cbs, bap_attached, bap);
> +
> +       if (!client) {
> +               bap_attach_att(bap, bap->att);
> +               return true;
> +       }
> +
> +       if (bap->client)
> +               return false;
> +
> +clone:
> +       bap->client = bt_gatt_client_clone(client);
> +       if (!bap->client)
> +               return false;
> +
> +       bap_attach_att(bap, bt_gatt_client_get_att(bap->client));
> +
> +       if (bt_gatt_client_is_ready(bap->client)) {
> +               bap_attach_finish(bap);
> +       } else {
> +               bap->client_ready_id = bt_gatt_client_ready_register(
> +                               bap->client, bap_attach_ready_cb, bap, NULL);
> +       }
>
>         return true;
>  }
> @@ -3824,6 +3845,12 @@ void bt_bap_detach(struct bt_bap *bap)
>         if (!queue_remove(sessions, bap))
>                 return;
>
> +       if (bap->client_ready_id) {
> +               bt_gatt_client_ready_unregister(bap->client,
> +                                               bap->client_ready_id);
> +               bap->client_ready_id = 0;
> +       }
> +
>         bt_gatt_client_unref(bap->client);
>         bap->client = NULL;
>
> --
> 2.39.1
>


-- 
Luiz Augusto von Dentz

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

* Re: [PATCH BlueZ] shared/bap: wait for GATT client ready before ASCS/PACS discovery
  2023-02-12 20:45 ` [PATCH BlueZ] " Luiz Augusto von Dentz
@ 2023-02-12 21:49   ` Pauli Virtanen
  2023-02-13 20:40     ` Luiz Augusto von Dentz
  0 siblings, 1 reply; 5+ messages in thread
From: Pauli Virtanen @ 2023-02-12 21:49 UTC (permalink / raw)
  To: Luiz Augusto von Dentz; +Cc: linux-bluetooth

Hi Luiz,

su, 2023-02-12 kello 12:45 -0800, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
> 
> On Sun, Feb 12, 2023 at 10:41 AM Pauli Virtanen <pav@iki.fi> wrote:
> > 
> > BAP client fails to find endpoints if GATT services are not resolved
> > when the GATT client is attached to the session.  This condition
> > almost always occurs on the first BAP connection to a device between
> > bluetoothd restarts, and may occur also otherwise.
> 
> Hmm, this shouldn't happen if you have caching enabled, did you
> disable it for some reason?

Caching has not been disabled. The files in /var/lib/bluetooth/*/cache
also get updated so presumably it is working.

> > The BAP client code discovers ASCS/PACS services and characteristics at
> > client attach time.
> > 
> > Fix the connection issue by postponing PAC/ASE discovery until the
> > GATT client is ready, if it was not ready at attach time.
> 
> This should probably be done by the core, it should check if the
> services are cached then it can call accept, otherwise it needs to
> wait for the client to be ready, perhaps there is something not quite
> right with the way to probe services as afaik if we don't have any
> cache the driver shouldn't have been called to begin with.

bap_accept gets called from

#0  bap_accept (service=0x604000005f50) at profiles/audio/bap.c:1304
#1  0x0000000000631755 in service_accept (service=0x604000005f50, initiator=true) at src/service.c:203
#2  0x0000000000668561 in device_accept_gatt_profiles (device=0x617000000080) at src/device.c:3739
#3  0x0000000000676128 in gatt_client_init (device=0x617000000080) at src/device.c:5161
#4  0x00000000006782e5 in device_attach_att (dev=0x617000000080, io=0x60c00000b2c0) at src/device.c:5331
#5  0x00000000006788e5 in att_connect_cb (io=0x60c00000b2c0, gerr=0x0, user_data=0x617000000080) at src/device.c:5378
#6  0x000000000053de23 in connect_cb (io=0x60c00000b2c0, cond=G_IO_OUT, user_data=0x60300001f8d0) at btio/btio.c:229
#7  0x00007f4955d16cbf in g_main_context_dispatch () from target:/lib64/libglib-2.0.so.0
#8  0x00007f4955d6c598 in g_main_context_iterate.constprop () from target:/lib64/libglib-2.0.so.0
#9  0x00007f4955d1628f in g_main_loop_run () from target:/lib64/libglib-2.0.so.0
#10 0x00000000007df9ca in mainloop_run () at src/shared/mainloop-glib.c:66
#11 0x00000000007e075a in mainloop_run_with_signal (func=0x558597 <signal_callback>, user_data=0x0) at src/shared/mainloop-notify.c:188
#12 0x0000000000558e97 in main (argc=1, argv=0x7ffd399f7788) at src/main.c:1304

which does not wait for GATT client ready. The comment above that

5157│         /*
5158│          * Notify notify existing service about the new connection so they can
5159│          * react to notifications while discovering services
5160│          */

looks like this is intentional. If it's like this, then services would
need to explicitly wait.

Didn't look yet how come it knows the gatt service is there then,
however I guess it only looks for the PACS service uuid, as I think
nothing tells device.c also ASCS would be needed.

Best,
Pauli

> 
> > ---
> > 
> > Notes:
> >     Typical bluetoothctl output in the failing case, without this patch:
> > 
> >     $ sudo systemctl restart bluetooth
> >     $ bluetoothctl
> >     ...
> >     [bluetooth]# connect XX:XX:XX:XX:XX:XX
> >     Attempting to connect to XX:XX:XX:XX:XX:XX
> >     [CHG] Device XX:XX:XX:XX:XX:XX Connected: yes
> >     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 0000184e-0000-1000-8000-00805f9b34fb
> >     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 00001850-0000-1000-8000-00805f9b34fb
> >     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 03b80e5a-ede8-4b33-a751-6ce34ec4c700
> >     Connection successful
> >     [NEW] Primary Service (Handle 0x0000)
> >             /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/service0008
> >             00001801-0000-1000-8000-00805f9b34fb
> >             Generic Attribute Profile
> >     ...
> >     [NEW] Descriptor (Handle 0x0000)
> >             /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/service0065/char0066/desc0069
> >             00002901-0000-1000-8000-00805f9b34fb
> >             Characteristic User Description
> >     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 00001800-0000-1000-8000-00805f9b34fb
> >     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 00001801-0000-1000-8000-00805f9b34fb
> >     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 0000180a-0000-1000-8000-00805f9b34fb
> >     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 00001844-0000-1000-8000-00805f9b34fb
> >     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 0000184e-0000-1000-8000-00805f9b34fb
> >     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 00001850-0000-1000-8000-00805f9b34fb
> >     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 03b80e5a-ede8-4b33-a751-6ce34ec4c700
> >     [CHG] Device XX:XX:XX:XX:XX:XX ServicesResolved: yes
> >     [CHG] Device XX:XX:XX:XX:XX:XX Paired: yes
> >     [xxx]# endpoint.list
> >     [xxx]# disconnect
> >     Attempting to disconnect from XX:XX:XX:XX:XX:XX
> >     [CHG] Device XX:XX:XX:XX:XX:XX ServicesResolved: no
> >     Successful disconnected
> >     [CHG] Device XX:XX:XX:XX:XX:XX Connected: no
> >     [bluetooth]# connect XX:XX:XX:XX:XX:XX
> >     Attempting to connect to XX:XX:XX:XX:XX:XX
> >     [CHG] Device XX:XX:XX:XX:XX:XX Connected: yes
> >     Connection successful
> >     [NEW] Endpoint /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0
> >     [NEW] Endpoint /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0
> >     [NEW] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0
> >     [NEW] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1
> >     [CHG] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0 Links: /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1
> >     [CHG] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1 Links: /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0
> >     [CHG] Device XX:XX:XX:XX:XX:XX ServicesResolved: yes
> >     [xxx]# endpoint.list
> >     Endpoint /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0
> >     Endpoint /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0
> >     [xxx]#
> > 
> >     Endpoints and transports failed to appear on the first connect to the
> >     BAP server, since GATT discovery was not yet completed when the BAP code
> >     tried to discover the endpoints. Second connection succeeded.
> >     Occasionally, it also happens that endpoints appear but transports do
> >     not, but this is hard to reproduce.
> > 
> >     With this patch:
> > 
> >     $ sudo systemctl restart bluetooth
> >     $ bluetoothctl
> >     ...
> >     [bluetooth]# connect XX:XX:XX:XX:XX:XX
> >     Attempting to connect to XX:XX:XX:XX:XX:XX
> >     [CHG] Device XX:XX:XX:XX:XX:XX Connected: yes
> >     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 0000184e-0000-1000-8000-00805f9b34fb
> >     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 00001850-0000-1000-8000-00805f9b34fb
> >     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 03b80e5a-ede8-4b33-a751-6ce34ec4c700
> >     Connection successful
> >     [NEW] Primary Service (Handle 0x0000)
> >             /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/service0008
> >             00001801-0000-1000-8000-00805f9b34fb
> >             Generic Attribute Profile
> >     ...
> >     [NEW] Descriptor (Handle 0x0000)
> >             /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/service0065/char0066/desc0069
> >             00002901-0000-1000-8000-00805f9b34fb
> >             Characteristic User Description
> >     [CHG] Device XX:XX:XX:XX:XX:XX ServicesResolved: yes
> >     [CHG] Device XX:XX:XX:XX:XX:XX Paired: yes
> >     [NEW] Endpoint /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0
> >     [NEW] Endpoint /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0
> >     [NEW] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0
> >     [NEW] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1
> >     [CHG] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0 Links: /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1
> >     [CHG] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1 Links: /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0
> >     [xxx]#
> > 
> >     Now the first connection works properly.
> > 
> >     On first reading, the spec does not seem to clearly comment if ASEs and
> >     PACs could be removed/added by the server while client is connected. If
> >     that's allowed, then we'd need some bigger changes here. Regardless,
> >     waiting for GATT client ready before first scan seems good also in this
> >     case.
> > 
> >  src/shared/bap.c | 93 +++++++++++++++++++++++++++++++-----------------
> >  1 file changed, 60 insertions(+), 33 deletions(-)
> > 
> > diff --git a/src/shared/bap.c b/src/shared/bap.c
> > index 22f2e6714..2d676d96f 100644
> > --- a/src/shared/bap.c
> > +++ b/src/shared/bap.c
> > @@ -158,6 +158,7 @@ struct bt_bap {
> >         struct bt_att *att;
> >         struct bt_bap_req *req;
> >         unsigned int cp_id;
> > +       unsigned int client_ready_id;
> > 
> >         unsigned int process_id;
> >         unsigned int disconn_id;
> > @@ -3733,41 +3734,10 @@ static void bap_attach_att(struct bt_bap *bap, struct bt_att *att)
> >                                                         bap, NULL);
> >  }
> > 
> > -bool bt_bap_attach(struct bt_bap *bap, struct bt_gatt_client *client)
> > +static void bap_attach_finish(struct bt_bap *bap)
> >  {
> >         bt_uuid_t uuid;
> > 
> > -       if (queue_find(sessions, NULL, bap)) {
> > -               /* If instance already been set but there is no client proceed
> > -                * to clone it otherwise considered it already attached.
> > -                */
> > -               if (client && !bap->client)
> > -                       goto clone;
> > -               return true;
> > -       }
> > -
> > -       if (!sessions)
> > -               sessions = queue_new();
> > -
> > -       queue_push_tail(sessions, bap);
> > -
> > -       queue_foreach(bap_cbs, bap_attached, bap);
> > -
> > -       if (!client) {
> > -               bap_attach_att(bap, bap->att);
> > -               return true;
> > -       }
> > -
> > -       if (bap->client)
> > -               return false;
> > -
> > -clone:
> > -       bap->client = bt_gatt_client_clone(client);
> > -       if (!bap->client)
> > -               return false;
> > -
> > -       bap_attach_att(bap, bt_gatt_client_get_att(client));
> > -
> >         if (bap->rdb->pacs) {
> >                 uint16_t value_handle;
> >                 struct bt_pacs *pacs = bap->rdb->pacs;
> > @@ -3798,7 +3768,7 @@ clone:
> > 
> >                 bap_notify_ready(bap);
> > 
> > -               return true;
> > +               return;
> >         }
> > 
> >         bt_uuid16_create(&uuid, PACS_UUID);
> > @@ -3806,6 +3776,57 @@ clone:
> > 
> >         bt_uuid16_create(&uuid, ASCS_UUID);
> >         gatt_db_foreach_service(bap->rdb->db, &uuid, foreach_ascs_service, bap);
> > +}
> > +
> > +static void bap_attach_ready_cb(bool success, uint8_t att_ecode,
> > +                                                               void *user_data)
> > +{
> > +       struct bt_bap *bap = user_data;
> > +
> > +       bap->client_ready_id = 0;
> > +
> > +       bap_attach_finish(bap);
> > +}
> > +
> > +bool bt_bap_attach(struct bt_bap *bap, struct bt_gatt_client *client)
> > +{
> > +       if (queue_find(sessions, NULL, bap)) {
> > +               /* If instance already been set but there is no client proceed
> > +                * to clone it otherwise considered it already attached.
> > +                */
> > +               if (client && !bap->client)
> > +                       goto clone;
> > +               return true;
> > +       }
> > +
> > +       if (!sessions)
> > +               sessions = queue_new();
> > +
> > +       queue_push_tail(sessions, bap);
> > +
> > +       queue_foreach(bap_cbs, bap_attached, bap);
> > +
> > +       if (!client) {
> > +               bap_attach_att(bap, bap->att);
> > +               return true;
> > +       }
> > +
> > +       if (bap->client)
> > +               return false;
> > +
> > +clone:
> > +       bap->client = bt_gatt_client_clone(client);
> > +       if (!bap->client)
> > +               return false;
> > +
> > +       bap_attach_att(bap, bt_gatt_client_get_att(bap->client));
> > +
> > +       if (bt_gatt_client_is_ready(bap->client)) {
> > +               bap_attach_finish(bap);
> > +       } else {
> > +               bap->client_ready_id = bt_gatt_client_ready_register(
> > +                               bap->client, bap_attach_ready_cb, bap, NULL);
> > +       }
> > 
> >         return true;
> >  }
> > @@ -3824,6 +3845,12 @@ void bt_bap_detach(struct bt_bap *bap)
> >         if (!queue_remove(sessions, bap))
> >                 return;
> > 
> > +       if (bap->client_ready_id) {
> > +               bt_gatt_client_ready_unregister(bap->client,
> > +                                               bap->client_ready_id);
> > +               bap->client_ready_id = 0;
> > +       }
> > +
> >         bt_gatt_client_unref(bap->client);
> >         bap->client = NULL;
> > 
> > --
> > 2.39.1
> > 
> 
> 


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

* Re: [PATCH BlueZ] shared/bap: wait for GATT client ready before ASCS/PACS discovery
  2023-02-12 21:49   ` Pauli Virtanen
@ 2023-02-13 20:40     ` Luiz Augusto von Dentz
  0 siblings, 0 replies; 5+ messages in thread
From: Luiz Augusto von Dentz @ 2023-02-13 20:40 UTC (permalink / raw)
  To: Pauli Virtanen; +Cc: linux-bluetooth

Hi Pauli,

On Sun, Feb 12, 2023 at 1:50 PM Pauli Virtanen <pav@iki.fi> wrote:
>
> Hi Luiz,
>
> su, 2023-02-12 kello 12:45 -0800, Luiz Augusto von Dentz kirjoitti:
> > Hi Pauli,
> >
> > On Sun, Feb 12, 2023 at 10:41 AM Pauli Virtanen <pav@iki.fi> wrote:
> > >
> > > BAP client fails to find endpoints if GATT services are not resolved
> > > when the GATT client is attached to the session.  This condition
> > > almost always occurs on the first BAP connection to a device between
> > > bluetoothd restarts, and may occur also otherwise.
> >
> > Hmm, this shouldn't happen if you have caching enabled, did you
> > disable it for some reason?
>
> Caching has not been disabled. The files in /var/lib/bluetooth/*/cache
> also get updated so presumably it is working.
>
> > > The BAP client code discovers ASCS/PACS services and characteristics at
> > > client attach time.
> > >
> > > Fix the connection issue by postponing PAC/ASE discovery until the
> > > GATT client is ready, if it was not ready at attach time.
> >
> > This should probably be done by the core, it should check if the
> > services are cached then it can call accept, otherwise it needs to
> > wait for the client to be ready, perhaps there is something not quite
> > right with the way to probe services as afaik if we don't have any
> > cache the driver shouldn't have been called to begin with.
>
> bap_accept gets called from
>
> #0  bap_accept (service=0x604000005f50) at profiles/audio/bap.c:1304
> #1  0x0000000000631755 in service_accept (service=0x604000005f50, initiator=true) at src/service.c:203
> #2  0x0000000000668561 in device_accept_gatt_profiles (device=0x617000000080) at src/device.c:3739
> #3  0x0000000000676128 in gatt_client_init (device=0x617000000080) at src/device.c:5161
> #4  0x00000000006782e5 in device_attach_att (dev=0x617000000080, io=0x60c00000b2c0) at src/device.c:5331
> #5  0x00000000006788e5 in att_connect_cb (io=0x60c00000b2c0, gerr=0x0, user_data=0x617000000080) at src/device.c:5378
> #6  0x000000000053de23 in connect_cb (io=0x60c00000b2c0, cond=G_IO_OUT, user_data=0x60300001f8d0) at btio/btio.c:229
> #7  0x00007f4955d16cbf in g_main_context_dispatch () from target:/lib64/libglib-2.0.so.0
> #8  0x00007f4955d6c598 in g_main_context_iterate.constprop () from target:/lib64/libglib-2.0.so.0
> #9  0x00007f4955d1628f in g_main_loop_run () from target:/lib64/libglib-2.0.so.0
> #10 0x00000000007df9ca in mainloop_run () at src/shared/mainloop-glib.c:66
> #11 0x00000000007e075a in mainloop_run_with_signal (func=0x558597 <signal_callback>, user_data=0x0) at src/shared/mainloop-notify.c:188
> #12 0x0000000000558e97 in main (argc=1, argv=0x7ffd399f7788) at src/main.c:1304
>
> which does not wait for GATT client ready. The comment above that
>
> 5157│         /*
> 5158│          * Notify notify existing service about the new connection so they can
> 5159│          * react to notifications while discovering services
> 5160│          */
>
> looks like this is intentional. If it's like this, then services would
> need to explicitly wait.
>
> Didn't look yet how come it knows the gatt service is there then,
> however I guess it only looks for the PACS service uuid, as I think
> nothing tells device.c also ASCS would be needed.

We do load the database from cache in case there is one:

https://github.com/bluez/bluez/blob/master/src/device.c#L5328

Or are you experiencing some problem where PACS is cached but ASCS isn't?

> Best,
> Pauli
>
> >
> > > ---
> > >
> > > Notes:
> > >     Typical bluetoothctl output in the failing case, without this patch:
> > >
> > >     $ sudo systemctl restart bluetooth
> > >     $ bluetoothctl
> > >     ...
> > >     [bluetooth]# connect XX:XX:XX:XX:XX:XX
> > >     Attempting to connect to XX:XX:XX:XX:XX:XX
> > >     [CHG] Device XX:XX:XX:XX:XX:XX Connected: yes
> > >     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 0000184e-0000-1000-8000-00805f9b34fb
> > >     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 00001850-0000-1000-8000-00805f9b34fb
> > >     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 03b80e5a-ede8-4b33-a751-6ce34ec4c700
> > >     Connection successful
> > >     [NEW] Primary Service (Handle 0x0000)
> > >             /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/service0008
> > >             00001801-0000-1000-8000-00805f9b34fb
> > >             Generic Attribute Profile
> > >     ...
> > >     [NEW] Descriptor (Handle 0x0000)
> > >             /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/service0065/char0066/desc0069
> > >             00002901-0000-1000-8000-00805f9b34fb
> > >             Characteristic User Description
> > >     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 00001800-0000-1000-8000-00805f9b34fb
> > >     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 00001801-0000-1000-8000-00805f9b34fb
> > >     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 0000180a-0000-1000-8000-00805f9b34fb
> > >     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 00001844-0000-1000-8000-00805f9b34fb
> > >     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 0000184e-0000-1000-8000-00805f9b34fb
> > >     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 00001850-0000-1000-8000-00805f9b34fb
> > >     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 03b80e5a-ede8-4b33-a751-6ce34ec4c700
> > >     [CHG] Device XX:XX:XX:XX:XX:XX ServicesResolved: yes
> > >     [CHG] Device XX:XX:XX:XX:XX:XX Paired: yes
> > >     [xxx]# endpoint.list
> > >     [xxx]# disconnect
> > >     Attempting to disconnect from XX:XX:XX:XX:XX:XX
> > >     [CHG] Device XX:XX:XX:XX:XX:XX ServicesResolved: no
> > >     Successful disconnected
> > >     [CHG] Device XX:XX:XX:XX:XX:XX Connected: no
> > >     [bluetooth]# connect XX:XX:XX:XX:XX:XX
> > >     Attempting to connect to XX:XX:XX:XX:XX:XX
> > >     [CHG] Device XX:XX:XX:XX:XX:XX Connected: yes
> > >     Connection successful
> > >     [NEW] Endpoint /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0
> > >     [NEW] Endpoint /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0
> > >     [NEW] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0
> > >     [NEW] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1
> > >     [CHG] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0 Links: /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1
> > >     [CHG] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1 Links: /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0
> > >     [CHG] Device XX:XX:XX:XX:XX:XX ServicesResolved: yes
> > >     [xxx]# endpoint.list
> > >     Endpoint /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0
> > >     Endpoint /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0
> > >     [xxx]#
> > >
> > >     Endpoints and transports failed to appear on the first connect to the
> > >     BAP server, since GATT discovery was not yet completed when the BAP code
> > >     tried to discover the endpoints. Second connection succeeded.
> > >     Occasionally, it also happens that endpoints appear but transports do
> > >     not, but this is hard to reproduce.
> > >
> > >     With this patch:
> > >
> > >     $ sudo systemctl restart bluetooth
> > >     $ bluetoothctl
> > >     ...
> > >     [bluetooth]# connect XX:XX:XX:XX:XX:XX
> > >     Attempting to connect to XX:XX:XX:XX:XX:XX
> > >     [CHG] Device XX:XX:XX:XX:XX:XX Connected: yes
> > >     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 0000184e-0000-1000-8000-00805f9b34fb
> > >     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 00001850-0000-1000-8000-00805f9b34fb
> > >     [CHG] Device XX:XX:XX:XX:XX:XX UUIDs: 03b80e5a-ede8-4b33-a751-6ce34ec4c700
> > >     Connection successful
> > >     [NEW] Primary Service (Handle 0x0000)
> > >             /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/service0008
> > >             00001801-0000-1000-8000-00805f9b34fb
> > >             Generic Attribute Profile
> > >     ...
> > >     [NEW] Descriptor (Handle 0x0000)
> > >             /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/service0065/char0066/desc0069
> > >             00002901-0000-1000-8000-00805f9b34fb
> > >             Characteristic User Description
> > >     [CHG] Device XX:XX:XX:XX:XX:XX ServicesResolved: yes
> > >     [CHG] Device XX:XX:XX:XX:XX:XX Paired: yes
> > >     [NEW] Endpoint /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0
> > >     [NEW] Endpoint /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0
> > >     [NEW] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0
> > >     [NEW] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1
> > >     [CHG] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0 Links: /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1
> > >     [CHG] Transport /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_sink0/fd1 Links: /org/bluez/hci0/dev_XX_XX_XX_XX_XX_XX/pac_source0/fd0
> > >     [xxx]#
> > >
> > >     Now the first connection works properly.
> > >
> > >     On first reading, the spec does not seem to clearly comment if ASEs and
> > >     PACs could be removed/added by the server while client is connected. If
> > >     that's allowed, then we'd need some bigger changes here. Regardless,
> > >     waiting for GATT client ready before first scan seems good also in this
> > >     case.
> > >
> > >  src/shared/bap.c | 93 +++++++++++++++++++++++++++++++-----------------
> > >  1 file changed, 60 insertions(+), 33 deletions(-)
> > >
> > > diff --git a/src/shared/bap.c b/src/shared/bap.c
> > > index 22f2e6714..2d676d96f 100644
> > > --- a/src/shared/bap.c
> > > +++ b/src/shared/bap.c
> > > @@ -158,6 +158,7 @@ struct bt_bap {
> > >         struct bt_att *att;
> > >         struct bt_bap_req *req;
> > >         unsigned int cp_id;
> > > +       unsigned int client_ready_id;
> > >
> > >         unsigned int process_id;
> > >         unsigned int disconn_id;
> > > @@ -3733,41 +3734,10 @@ static void bap_attach_att(struct bt_bap *bap, struct bt_att *att)
> > >                                                         bap, NULL);
> > >  }
> > >
> > > -bool bt_bap_attach(struct bt_bap *bap, struct bt_gatt_client *client)
> > > +static void bap_attach_finish(struct bt_bap *bap)
> > >  {
> > >         bt_uuid_t uuid;
> > >
> > > -       if (queue_find(sessions, NULL, bap)) {
> > > -               /* If instance already been set but there is no client proceed
> > > -                * to clone it otherwise considered it already attached.
> > > -                */
> > > -               if (client && !bap->client)
> > > -                       goto clone;
> > > -               return true;
> > > -       }
> > > -
> > > -       if (!sessions)
> > > -               sessions = queue_new();
> > > -
> > > -       queue_push_tail(sessions, bap);
> > > -
> > > -       queue_foreach(bap_cbs, bap_attached, bap);
> > > -
> > > -       if (!client) {
> > > -               bap_attach_att(bap, bap->att);
> > > -               return true;
> > > -       }
> > > -
> > > -       if (bap->client)
> > > -               return false;
> > > -
> > > -clone:
> > > -       bap->client = bt_gatt_client_clone(client);
> > > -       if (!bap->client)
> > > -               return false;
> > > -
> > > -       bap_attach_att(bap, bt_gatt_client_get_att(client));
> > > -
> > >         if (bap->rdb->pacs) {
> > >                 uint16_t value_handle;
> > >                 struct bt_pacs *pacs = bap->rdb->pacs;
> > > @@ -3798,7 +3768,7 @@ clone:
> > >
> > >                 bap_notify_ready(bap);
> > >
> > > -               return true;
> > > +               return;
> > >         }
> > >
> > >         bt_uuid16_create(&uuid, PACS_UUID);
> > > @@ -3806,6 +3776,57 @@ clone:
> > >
> > >         bt_uuid16_create(&uuid, ASCS_UUID);
> > >         gatt_db_foreach_service(bap->rdb->db, &uuid, foreach_ascs_service, bap);
> > > +}
> > > +
> > > +static void bap_attach_ready_cb(bool success, uint8_t att_ecode,
> > > +                                                               void *user_data)
> > > +{
> > > +       struct bt_bap *bap = user_data;
> > > +
> > > +       bap->client_ready_id = 0;
> > > +
> > > +       bap_attach_finish(bap);
> > > +}
> > > +
> > > +bool bt_bap_attach(struct bt_bap *bap, struct bt_gatt_client *client)
> > > +{
> > > +       if (queue_find(sessions, NULL, bap)) {
> > > +               /* If instance already been set but there is no client proceed
> > > +                * to clone it otherwise considered it already attached.
> > > +                */
> > > +               if (client && !bap->client)
> > > +                       goto clone;
> > > +               return true;
> > > +       }
> > > +
> > > +       if (!sessions)
> > > +               sessions = queue_new();
> > > +
> > > +       queue_push_tail(sessions, bap);
> > > +
> > > +       queue_foreach(bap_cbs, bap_attached, bap);
> > > +
> > > +       if (!client) {
> > > +               bap_attach_att(bap, bap->att);
> > > +               return true;
> > > +       }
> > > +
> > > +       if (bap->client)
> > > +               return false;
> > > +
> > > +clone:
> > > +       bap->client = bt_gatt_client_clone(client);
> > > +       if (!bap->client)
> > > +               return false;
> > > +
> > > +       bap_attach_att(bap, bt_gatt_client_get_att(bap->client));
> > > +
> > > +       if (bt_gatt_client_is_ready(bap->client)) {
> > > +               bap_attach_finish(bap);
> > > +       } else {
> > > +               bap->client_ready_id = bt_gatt_client_ready_register(
> > > +                               bap->client, bap_attach_ready_cb, bap, NULL);
> > > +       }
> > >
> > >         return true;
> > >  }
> > > @@ -3824,6 +3845,12 @@ void bt_bap_detach(struct bt_bap *bap)
> > >         if (!queue_remove(sessions, bap))
> > >                 return;
> > >
> > > +       if (bap->client_ready_id) {
> > > +               bt_gatt_client_ready_unregister(bap->client,
> > > +                                               bap->client_ready_id);
> > > +               bap->client_ready_id = 0;
> > > +       }
> > > +
> > >         bt_gatt_client_unref(bap->client);
> > >         bap->client = NULL;
> > >
> > > --
> > > 2.39.1
> > >
> >
> >
>


-- 
Luiz Augusto von Dentz

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

end of thread, other threads:[~2023-02-13 20:40 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-12 18:34 [PATCH BlueZ] shared/bap: wait for GATT client ready before ASCS/PACS discovery Pauli Virtanen
2023-02-12 20:05 ` [BlueZ] " bluez.test.bot
2023-02-12 20:45 ` [PATCH BlueZ] " Luiz Augusto von Dentz
2023-02-12 21:49   ` Pauli Virtanen
2023-02-13 20:40     ` 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.