[BlueZ] shared/asha: Add support for other side update

Commit Message

Sanchayan Maity Jan. 30, 2025, 9:01 a.m. UTC

ASHA specification requires that the status of the other side be
communicated with the start command. The status is also updated
if one of the device in the pair is connected/disconnected after
the other.

https://source.android.com/docs/core/connect/bluetooth/asha#audiocontrolpoint
---
 src/shared/asha.c | 163 +++++++++++++++++++++++++++++++++++++++++++++-
 src/shared/asha.h |   8 +++
 2 files changed, 168 insertions(+), 3 deletions(-)

Comments

[e-mail] Jan. 30, 2025, 10:13 a.m. UTC | #1

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=929215

---Test result---

Test Summary:
CheckPatch                    PENDING   0.18 seconds
GitLint                       PENDING   0.32 seconds
BuildEll                      PASS      20.51 seconds
BluezMake                     FAIL      76.11 seconds
MakeCheck                     FAIL      1982.74 seconds
MakeDistcheck                 PASS      158.16 seconds
CheckValgrind                 FAIL      55.42 seconds
CheckSmatch                   FAIL      140.26 seconds
bluezmakeextell               FAIL      66.27 seconds
IncrementalBuild              PENDING   0.28 seconds
ScanBuild                     PASS      855.99 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: BluezMake - FAIL
Desc: Build BlueZ
Output:

tools/mgmt-tester.c: In function ‘main’:
tools/mgmt-tester.c:12893:5: note: variable tracking size limit exceeded with ‘-fvar-tracking-assignments’, retrying without
12893 | int main(int argc, char *argv[])
      |     ^~~~
In file included from ./profiles/audio/asha.h:16,
                 from profiles/audio/asha.c:43:
./src/shared/asha.h:56:22: error: ‘asha_devices’ defined but not used [-Werror=unused-variable]
   56 | static struct queue *asha_devices;
      |                      ^~~~~~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:10783: profiles/audio/bluetoothd-asha.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:4693: all] Error 2
##############################
Test: MakeCheck - FAIL
Desc: Run Bluez Make Check
Output:

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[])
      |     ^~~~
In file included from profiles/audio/media.c:41:
./src/shared/asha.h:56:22: error: ‘asha_devices’ defined but not used [-Werror=unused-variable]
   56 | static struct queue *asha_devices;
      |                      ^~~~~~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:10111: profiles/audio/bluetoothd-media.o] Error 1
make: *** [Makefile:12344: check] Error 2
##############################
Test: CheckValgrind - FAIL
Desc: Run Bluez Make Check with Valgrind
Output:

tools/mgmt-tester.c: In function ‘main’:
tools/mgmt-tester.c:12893:5: note: variable tracking size limit exceeded with ‘-fvar-tracking-assignments’, retrying without
12893 | int main(int argc, char *argv[])
      |     ^~~~
In file included from ./profiles/audio/asha.h:16,
                 from profiles/audio/asha.c:43:
./src/shared/asha.h:56:22: error: ‘asha_devices’ defined but not used [-Werror=unused-variable]
   56 | static struct queue *asha_devices;
      |                      ^~~~~~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:10783: profiles/audio/bluetoothd-asha.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:12344: check] Error 2
##############################
Test: CheckSmatch - FAIL
Desc: Run smatch tool with source
Output:

src/shared/crypto.c:271:21: warning: Variable length array is used.
src/shared/crypto.c:272:23: warning: Variable length array is used.
src/shared/gatt-helpers.c:768:31: warning: Variable length array is used.
src/shared/gatt-helpers.c:830:31: warning: Variable length array is used.
src/shared/gatt-helpers.c:1323:31: warning: Variable length array is used.
src/shared/gatt-helpers.c:1354:23: warning: Variable length array is used.
src/shared/gatt-server.c:278:25: warning: Variable length array is used.
src/shared/gatt-server.c:618:25: warning: Variable length array is used.
src/shared/gatt-server.c:716:25: warning: Variable length array is used.
src/shared/bap.c:296:25: warning: array of flexible structures
src/shared/bap.c: note: in included file:
./src/shared/ascs.h:88:25: warning: array of flexible structures
src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):
/usr/include/readline/rltypedefs.h:35:23: warning: non-ANSI function declaration of function 'Function'
/usr/include/readline/rltypedefs.h:36:25: warning: non-ANSI function declaration of function 'VFunction'
/usr/include/readline/rltypedefs.h:37:27: warning: non-ANSI function declaration of function 'CPFunction'
/usr/include/readline/rltypedefs.h:38:29: warning: non-ANSI function declaration of function 'CPPFunction'
src/shared/crypto.c:271:21: warning: Variable length array is used.
src/shared/crypto.c:272:23: warning: Variable length array is used.
src/shared/gatt-helpers.c:768:31: warning: Variable length array is used.
src/shared/gatt-helpers.c:830:31: warning: Variable length array is used.
src/shared/gatt-helpers.c:1323:31: warning: Variable length array is used.
src/shared/gatt-helpers.c:1354:23: warning: Variable length array is used.
src/shared/gatt-server.c:278:25: warning: Variable length array is used.
src/shared/gatt-server.c:618:25: warning: Variable length array is used.
src/shared/gatt-server.c:716:25: warning: Variable length array is used.
src/shared/bap.c:296:25: warning: array of flexible structures
src/shared/bap.c: note: in included file:
./src/shared/ascs.h:88:25: warning: array of flexible structures
src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):
/usr/include/readline/rltypedefs.h:35:23: warning: non-ANSI function declaration of function 'Function'
/usr/include/readline/rltypedefs.h:36:25: warning: non-ANSI function declaration of function 'VFunction'
/usr/include/readline/rltypedefs.h:37:27: warning: non-ANSI function declaration of function 'CPFunction'
/usr/include/readline/rltypedefs.h:38:29: warning: non-ANSI function declaration of function 'CPPFunction'
tools/mesh-cfgtest.c:1453:17: warning: unknown escape sequence: '\%'
tools/sco-tester.c: note: in included file:
./lib/bluetooth.h:232:15: warning: array of flexible structures
./lib/bluetooth.h:237:31: warning: array of flexible structures
tools/bneptest.c:634:39: warning: unknown escape sequence: '\%'
tools/seq2bseq.c:57:26: warning: Variable length array is used.
tools/obex-client-tool.c: note: in included file (through /usr/include/readline/readline.h):
/usr/include/readline/rltypedefs.h:35:23: warning: non-ANSI function declaration of function 'Function'
/usr/include/readline/rltypedefs.h:36:25: warning: non-ANSI function declaration of function 'VFunction'
/usr/include/readline/rltypedefs.h:37:27: warning: non-ANSI function declaration of function 'CPFunction'
/usr/include/readline/rltypedefs.h:38:29: warning: non-ANSI function declaration of function 'CPPFunction'
android/avctp.c:505:34: warning: Variable length array is used.
android/avctp.c:556:34: warning: Variable length array is used.
unit/test-avrcp.c:373:26: warning: Variable length array is used.
unit/test-avrcp.c:398:26: warning: Variable length array is used.
unit/test-avrcp.c:414:24: warning: Variable length array is used.
android/avrcp-lib.c:1085:34: warning: Variable length array is used.
android/avrcp-lib.c:1583:34: warning: Variable length array is used.
android/avrcp-lib.c:1612:34: warning: Variable length array is used.
android/avrcp-lib.c:1638:34: warning: Variable length array is used.
In file included from ./profiles/audio/asha.h:16,
                 from profiles/audio/asha.c:43:
./src/shared/asha.h:56:22: error: ‘asha_devices’ defined but not used [-Werror=unused-variable]
   56 | static struct queue *asha_devices;
      |                      ^~~~~~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:10783: profiles/audio/bluetoothd-asha.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:4693: all] Error 2
##############################
Test: bluezmakeextell - FAIL
Desc: Build Bluez with External ELL
Output:

In file included from ./profiles/audio/asha.h:16,
                 from profiles/audio/asha.c:43:
./src/shared/asha.h:56:22: error: ‘asha_devices’ defined but not used [-Werror=unused-variable]
   56 | static struct queue *asha_devices;
      |                      ^~~~~~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:10783: profiles/audio/bluetoothd-asha.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:4693: all] Error 2
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

Arun Raghavan Feb. 3, 2025, 3:40 p.m. UTC | #2

On Thu, 30 Jan 2025 at 04:02, Sanchayan Maity <[e-mail]> wrote:
>
> ASHA specification requires that the status of the other side be
> communicated with the start command. The status is also updated
> if one of the device in the pair is connected/disconnected after
> the other.
>
> https://source.android.com/docs/core/connect/bluetooth/asha#audiocontrolpoint
> ---
>  src/shared/asha.c | 163 +++++++++++++++++++++++++++++++++++++++++++++-
>  src/shared/asha.h |   8 +++
>  2 files changed, 168 insertions(+), 3 deletions(-)

Reviewed-by: Arun Raghavan <[e-mail]>

Cheers,
Arun

Luiz Augusto von Dentz Feb. 3, 2025, 5:59 p.m. UTC | #3

Hi,

On Mon, Feb 3, 2025 at 10:41 AM Arun Raghavan <[e-mail]> wrote:
>
> On Thu, 30 Jan 2025 at 04:02, Sanchayan Maity <[e-mail]> wrote:
> >
> > ASHA specification requires that the status of the other side be
> > communicated with the start command. The status is also updated
> > if one of the device in the pair is connected/disconnected after
> > the other.
> >
> > https://source.android.com/docs/core/connect/bluetooth/asha#audiocontrolpoint
> > ---
> >  src/shared/asha.c | 163 +++++++++++++++++++++++++++++++++++++++++++++-
> >  src/shared/asha.h |   8 +++
> >  2 files changed, 168 insertions(+), 3 deletions(-)
>
> Reviewed-by: Arun Raghavan <[e-mail]>

Well we still need to fix the following error:

./src/shared/asha.h:56:22: error: ‘asha_devices’ defined but not used
[-Werror=unused-variable]
   56 | static struct queue *asha_devices;
      |                      ^~~~~~~~~~~~
cc1: all warnings being treated as errors


> Cheers,
> Arun
>

Patch

diff --git a/src/shared/asha.c b/src/shared/asha.c
index 67f2631cd..f2170d758 100644
--- a/src/shared/asha.c
+++ b/src/shared/asha.c
@@ -32,55 +32,166 @@ 
 
 #include "asha.h"
 
 /* We use strings instead of uint128_t to maintain readability */
 #define ASHA_CHRC_READ_ONLY_PROPERTIES_UUID "6333651e-c481-4a3e-9169-7c902aad37bb"
 #define ASHA_CHRC_AUDIO_CONTROL_POINT_UUID "f0d4de7e-4a88-476c-9d9f-1937b0996cc0"
 #define ASHA_CHRC_AUDIO_STATUS_UUID "38663f1a-e711-4cac-b641-326b56404837"
 #define ASHA_CHRC_VOLUME_UUID "00e4ca9e-ab14-41e4-8823-f9e70c7e91df"
 #define ASHA_CHRC_LE_PSM_OUT_UUID "2d410339-82b6-42aa-b34e-e2e01df8cc1a"
 
+unsigned int bt_asha_status(struct bt_asha *asha, bool connected);
+
+static bool match_hisyncid(const void *data, const void *user_data)
+{
+	const struct bt_asha_set *set = data;
+	const struct bt_asha *asha = user_data;
+
+	return (memcmp(set->hisyncid, asha->hisyncid, 8) == 0);
+}
+
+static struct bt_asha_set *find_asha_set(struct bt_asha *asha)
+{
+	return queue_find(asha_devices, match_hisyncid, asha);
+}
+
+static uint8_t is_other_connected(struct bt_asha *asha)
+{
+	struct bt_asha_set *set = find_asha_set(asha);
+
+	if (set) {
+		if (asha->right_side && set->left) {
+			DBG("ASHA right and left side connected");
+			return 1;
+		}
+		if (!asha->right_side && set->right) {
+			DBG("ASHA left and right side connected");
+			return 1;
+		}
+	}
+
+	if (asha->right_side)
+		DBG("ASHA right side connected");
+	else
+		DBG("ASHA left side connected");
+
+	return 0;
+}
+
+static void update_asha_set(struct bt_asha *asha, bool connected)
+{
+	struct bt_asha_set *set;
+	set = queue_find(asha_devices, match_hisyncid, asha);
+
+	if (connected) {
+		if (!set) {
+			set = new0(struct bt_asha_set, 1);
+			memcpy(set->hisyncid, asha->hisyncid, 8);
+			queue_push_tail(asha_devices, set);
+			DBG("Created ASHA set");
+		}
+
+		if (asha->right_side) {
+			set->right = asha;
+			DBG("Right side registered for ASHA set");
+		} else {
+			set->left = asha;
+			DBG("Left side registered for ASHA set");
+		}
+	} else {
+		if (!set) {
+			error("Missing ASHA set");
+			return;
+		}
+
+		if (asha->right_side && set->right) {
+			set->right = NULL;
+			DBG("Right side unregistered for ASHA set");
+		} else if (!asha->right_side && set->left) {
+			set->left = NULL;
+			DBG("Left side unregistered for ASHA set");
+		}
+
+		if (!set->right && !set->left) {
+			if (queue_remove(asha_devices, set)) {
+				free(set);
+				DBG("Freeing ASHA set");
+			}
+
+			if (!queue_peek_tail(asha_devices)) {
+				queue_destroy(asha_devices, NULL);
+				asha_devices = NULL;
+			}
+		}
+	}
+}
+
+static int asha_set_send_status(struct bt_asha *asha, bool other_connected)
+{
+	struct bt_asha_set *set;
+	int ret = 0;
+
+	set = queue_find(asha_devices, match_hisyncid, asha);
+
+	if (set) {
+		if (asha->right_side && set->left) {
+			ret = bt_asha_status(set->left, other_connected);
+			DBG("ASHA left side update: %d, ret: %d", other_connected, ret);
+		}
+
+		if (!asha->right_side && set->right) {
+			ret = bt_asha_status(set->right, other_connected);
+			DBG("ASHA right side update: %d, ret: %d", other_connected, ret);
+		}
+	}
+
+	return ret;
+}
+
 struct bt_asha *bt_asha_new(void)
 {
 	struct bt_asha *asha;
 
 	asha = new0(struct bt_asha, 1);
 
 	return asha;
 }
 
 void bt_asha_reset(struct bt_asha *asha)
 {
 	if (asha->status_notify_id) {
 		bt_gatt_client_unregister_notify(asha->client,
 						asha->status_notify_id);
 	}
 
 	gatt_db_unref(asha->db);
 	asha->db = NULL;
 
 	bt_gatt_client_unref(asha->client);
 	asha->client = NULL;
 
 	asha->psm = 0;
+
+	update_asha_set(asha, false);
 }
 
 void bt_asha_state_reset(struct bt_asha *asha)
 {
 	asha->state = ASHA_STOPPED;
 
 	asha->cb = NULL;
 	asha->cb_user_data = NULL;
 }
 
 void bt_asha_free(struct bt_asha *asha)
 {
+	update_asha_set(asha, false);
 	gatt_db_unref(asha->db);
 	bt_gatt_client_unref(asha->client);
 	free(asha);
 }
 
 static void asha_acp_sent(bool success, uint8_t err, void *user_data)
 {
 	struct bt_asha *asha = user_data;
 
 	if (success) {
@@ -103,61 +214,101 @@  static int asha_send_acp(struct bt_asha *asha, uint8_t *cmd,
 		error("Error writing ACP command");
 		return -1;
 	}
 
 	asha->cb = cb;
 	asha->cb_user_data = user_data;
 
 	return 0;
 }
 
+static int asha_send_acp_without_response(struct bt_asha *asha, uint8_t *cmd,
+		unsigned int len)
+{
+	if (!bt_gatt_client_write_without_response(asha->client, asha->acp_handle,
+				false, cmd, len)) {
+		error("Error writing ACP command");
+		return -1;
+	}
+
+	return 0;
+}
+
 unsigned int bt_asha_start(struct bt_asha *asha, bt_asha_cb_t cb,
 								void *user_data)
 {
+	uint8_t other_connected = is_other_connected(asha);
 	uint8_t acp_start_cmd[] = {
 		0x01,		/* START */
 		0x01,		/* G.722, 16 kHz */
 		0,			/* Unknown media type */
 		asha->volume,	/* Volume */
-		0,			/* Other disconnected */
+		other_connected,
 	};
 	int ret;
 
 	if (asha->state != ASHA_STOPPED) {
 		error("ASHA device start failed. Bad state %d", asha->state);
 		return 0;
 	}
 
 	ret = asha_send_acp(asha, acp_start_cmd, sizeof(acp_start_cmd), cb,
 			user_data);
 	if (ret < 0)
 		return ret;
 
 	asha->state = ASHA_STARTING;
 
 	return 0;
 }
 
 unsigned int bt_asha_stop(struct bt_asha *asha, bt_asha_cb_t cb,
 								void *user_data)
 {
 	uint8_t acp_stop_cmd[] = {
 		0x02, /* STOP */
 	};
+	int ret;
 
 	if (asha->state != ASHA_STARTED)
 		return 0;
 
 	asha->state = ASHA_STOPPING;
 
-	return asha_send_acp(asha, acp_stop_cmd, sizeof(acp_stop_cmd),
-								cb, user_data);
+	ret = asha_send_acp(asha, acp_stop_cmd, sizeof(acp_stop_cmd),
+			cb, user_data);
+	asha_set_send_status(asha, false);
+
+	return ret;
+}
+
+unsigned int bt_asha_status(struct bt_asha *asha, bool other_connected)
+{
+	uint8_t status = other_connected ? 1 : 0;
+	uint8_t acp_status_cmd[] = {
+		0x03, /* STATUS */
+		status,
+	};
+	int ret;
+
+	if (asha->state != ASHA_STARTED) {
+		const char *side = asha->right_side ? "right" : "left";
+		DBG("ASHA %s device not started for status update", side);
+		return 0;
+	}
+
+	ret = asha_send_acp_without_response(asha, acp_status_cmd,
+			sizeof(acp_status_cmd));
+	if (ret < 0)
+		return ret;
+
+	return 0;
 }
 
 bool bt_asha_set_volume(struct bt_asha *asha, int8_t volume)
 {
 	if (!bt_gatt_client_write_without_response(asha->client,
 						asha->volume_handle, false,
 						(const uint8_t *)&volume, 1)) {
 		error("Error writing volume");
 		return false;
 	}
@@ -231,43 +382,46 @@  static void read_rops(bool success,
 	asha->coc_streaming_supported = (value[10] & 0x1) != 0;
 	/* RenderDelay */
 	asha->render_delay = get_le16(&value[11]);
 	/* byte 13 & 14 are reserved */
 	/* Codec IDs */
 	asha->codec_ids = get_le16(&value[15]);
 
 	DBG("Got ROPS: side %u, binaural %u, csis: %u, delay %u, codecs: %u",
 			asha->right_side, asha->binaural, asha->csis_supported,
 			asha->render_delay, asha->codec_ids);
+
+	update_asha_set(asha, true);
 }
 
 static void audio_status_register(uint16_t att_ecode, void *user_data)
 {
 	if (att_ecode)
 		DBG("AudioStatusPoint register failed 0x%04x", att_ecode);
 	else
 		DBG("AudioStatusPoint register succeeded");
 }
 
 static void audio_status_notify(uint16_t value_handle, const uint8_t *value,
 					uint16_t length, void *user_data)
 {
 	struct bt_asha *asha = user_data;
 	uint8_t status = *value;
 	/* Back these up to survive the reset paths */
 	bt_asha_cb_t cb = asha->cb;
 	bt_asha_cb_t cb_user_data = asha->cb_user_data;
 
 	if (asha->state == ASHA_STARTING) {
 		if (status == 0) {
 			asha->state = ASHA_STARTED;
 			DBG("ASHA start complete");
+			asha_set_send_status(asha, true);
 		} else {
 			bt_asha_state_reset(asha);
 			DBG("ASHA start failed");
 		}
 	} else if (asha->state == ASHA_STOPPING) {
 		/* We reset our state, regardless */
 		bt_asha_state_reset(asha);
 		DBG("ASHA stop %s", status == 0 ? "complete" : "failed");
 	}
 
@@ -348,12 +502,15 @@  bool bt_asha_probe(struct bt_asha *asha, struct gatt_db *db,
 
 	bt_uuid16_create(&asha_uuid, ASHA_SERVICE);
 	gatt_db_foreach_service(db, &asha_uuid, foreach_asha_service, asha);
 
 	if (!asha->attr) {
 		error("ASHA attribute not found");
 		bt_asha_reset(asha);
 		return false;
 	}
 
+	if (!asha_devices)
+		asha_devices = queue_new();
+
 	return true;
 }
diff --git a/src/shared/asha.h b/src/shared/asha.h
index c2c232fff..1a970e67a 100644
--- a/src/shared/asha.h
+++ b/src/shared/asha.h
@@ -40,20 +40,28 @@  struct bt_asha {
 	uint8_t hisyncid[8];
 	uint16_t render_delay;
 	uint16_t codec_ids;
 	int8_t volume;
 
 	enum bt_asha_state_t state;
 	bt_asha_cb_t cb;
 	void *cb_user_data;
 };
 
+struct bt_asha_set {
+	uint8_t hisyncid[8];
+	struct bt_asha *left;
+	struct bt_asha *right;
+};
+
+static struct queue *asha_devices;
+
 struct bt_asha *bt_asha_new(void);
 void bt_asha_reset(struct bt_asha *asha);
 void bt_asha_state_reset(struct bt_asha *asha);
 void bt_asha_free(struct bt_asha *asha);
 
 unsigned int bt_asha_start(struct bt_asha *asha, bt_asha_cb_t cb,
 							void *user_data);
 unsigned int bt_asha_stop(struct bt_asha *asha, bt_asha_cb_t cb,
 							void *user_data);