[PATCH 3/3] AVRCP: Add Passthrough Signal

[PATCH 3/3] AVRCP: Add Passthrough Signal

[David Stockwell]

AVRCP: Add Passthrough Signal

Send Passthrough signal, not only for simple keystrokes,
but especially for Vendor Unique key, passing company-id
and vendor-unique string as well.
---
 audio/control.c     |   90 
+++++++++++++++++++++++++++++++++++++++++++--------
 doc/control-api.txt |   14 +++-----
 2 files changed, 81 insertions(+), 23 deletions(-)

diff --git a/audio/control.c b/audio/control.c
index dd2930c..fcaad7c 100644
--- a/audio/control.c
+++ b/audio/control.c
@@ -106,6 +106,8 @@
 #define FORWARD_OP		0x4b
 #define BACKWARD_OP		0x4c
 
+#define VENDOR_UNIQUE_OP	0x7E
+
 /* Company IDs for vendor dependent commands */
 #define IEEEID_BTSIG		0x001958
 
@@ -515,28 +517,88 @@ static void send_key(int fd, uint16_t key, int pressed)
 	send_event(fd, EV_SYN, SYN_REPORT, 0);
 }
 
+/**
+ *	handle_panel_passthrough:
+ *
+ *	Handles AVRCP 1.0+ PASSTHROUGH command, passes the keystroke to uinput.
+ *
+ *	Added a Passthrough signal, with the key state and the optional
+ *	following company_id and vendor-unique message.
+ */
+
 static void handle_panel_passthrough(struct control *control,
-					const unsigned char *operands,
+					const uint8_t *operands,
 					int operand_count)
 {
 	const char *status;
-	int pressed, i;
-
-	if (operand_count == 0)
+	int i;
+	uint8_t key_pressed;
+	gboolean key_state;
+	uint32_t pass_company_id;
+	char *pass_string;
+	/*
+	 * operands[1] is operation_data_field_length (AV/C Panel Specification
+	 * v1.23, sect 9.4.5). Should always be present, even if zero.
+	 */
+	if (operand_count < 2)
 		return;
 
-	if (operands[0] & 0x80) {
-		status = "released";
-		pressed = 0;
+	key_pressed = operands[0] & 0x7F;
+
+	/* If key is pressed, key state bit is zero (AVRCP v13r00 p89). */
+	key_state = ((operands[0] & 0x80) == 0);
+	status = key_state ? "pressed" : "released";
+
+	DBG("Passthrough Key: %x %s", key_pressed, status);
+
+	if (key_pressed == VENDOR_UNIQUE_OP) {
+		if (operands[1] == 0 || operand_count < 5) {
+			pass_company_id = 0;
+			pass_string = g_malloc0(1);
+			DBG("Passthrough: No Company_ID or String");
+		} else if (operands[1] == 3 && operand_count == 5) {
+			pass_company_id = get_company_id(operands + 2);
+			pass_string = g_malloc0(1);
+			DBG("Passthrough Company_ID: %06X String: <none>",
+						pass_company_id);
+		} else if (operands[1] > 3 &&
+					operand_count == operands[1] + 2) {
+			pass_company_id = get_company_id(operands + 2);
+			pass_string = g_strndup((gchar *) operands + 5,
+							operands[1] - 3);
+			DBG("Passthrough Company_ID: %06X String: %s",
+						pass_company_id, pass_string);
+		} else { /* op_length does not match operand_count */
+			DBG("Passthrough: Malformed message");
+			DBG("op_len %u, op_cnt %u", operands[1], operand_count);
+			pass_company_id = 0;
+			pass_string = g_malloc0(1);
+		}
 	} else {
-		status = "pressed";
-		pressed = 1;
+		pass_company_id = 0;
+		pass_string = g_malloc0(1);
 	}
 
+	/*
+	 * Generate passthrough signal only if not BTSIG Company_ID.
+	 * For BTSIG, passthrough only for Group Navigation (unimplemented).
+	 */
+
+	if (pass_company_id != IEEEID_BTSIG)
+		g_dbus_emit_signal(control->dev->conn, control->dev->path,
+			   AUDIO_CONTROL_INTERFACE, "Passthrough",
+			   DBUS_TYPE_BYTE, &key_pressed,
+			   DBUS_TYPE_BOOLEAN, &key_state,
+			   DBUS_TYPE_UINT32, &pass_company_id,
+			   DBUS_TYPE_STRING, &pass_string,
+			   DBUS_TYPE_INVALID);
+
+	g_free(pass_string);
+
 	for (i = 0; key_map[i].name != NULL; i++) {
 		uint8_t key_quirks;
 
-		if ((operands[0] & 0x7F) != key_map[i].avrcp)
+		if (key_pressed != key_map[i].avrcp)
 			continue;
 
 		DBG("AVRCP: %s %s", key_map[i].name, status);
@@ -544,7 +606,7 @@ static void handle_panel_passthrough(struct control 
*control,
 		key_quirks = control->key_quirks[key_map[i].avrcp];
 
 		if (key_quirks & QUIRK_NO_RELEASE) {
-			if (!pressed) {
+			if (!key_state) {
 				DBG("AVRCP: Ignoring release");
 				break;
 			}
@@ -555,13 +617,12 @@ static void handle_panel_passthrough(struct control 
*control,
 			break;
 		}
 
-		send_key(control->uinput, key_map[i].uinput, pressed);
+		send_key(control->uinput, key_map[i].uinput, key_state);
 		break;
 	}
 
 	if (key_map[i].name == NULL)
-		DBG("AVRCP: unknown button 0x%02X %s",
-						operands[0] & 0x7F, status);
+		DBG("AVRCP: unknown button 0x%02X %s", key_pressed, status);
 }
 
 static unsigned int attr_get_max_val(uint8_t attr)
@@ -2291,6 +2352,7 @@ static GDBusSignalTable control_signals[] = {
 	{ "Connected",			"",	G_DBUS_SIGNAL_FLAG_DEPRECATED},
 	{ "Disconnected",		"",	G_DBUS_SIGNAL_FLAG_DEPRECATED},
 	{ "PropertyChanged",		"sv"	},
+	{ "Passthrough",		"ybus"	},
 	{ NULL, NULL }
 };
 
diff --git a/doc/control-api.txt b/doc/control-api.txt
index a7e5cbb..64ea5d3 100644
--- a/doc/control-api.txt
+++ b/doc/control-api.txt
@@ -55,18 +55,14 @@ Signals		Connected()
 			Sent when the AVRCP connection to the remote device
 			has been disconnected.
 
-		Passthrough(uint8 key, boolean state, int32 company_id,
+		Passthrough(uint8 key, boolean state, uint32 company_id,
 								string op_data)
 
-			Called when Passthrough command is received from
-			connected device.
+			Sent when Passthrough received from CT.
 
-			NOTE: according to the AV/C Subpanel Spec, company_id
-			and op_data are passed ONLY when the key is
-			"Vendor_Unique", or 0x7E.
-
-			When the key is NOT 0x7E, the signal returns
-			company_id=-1, and zero-length op_data.
+			Company_id and op_data returned only when key is 0x7E
+			(OP_VENDOR_UNIQUE).  Otherwise, returns zero for
+			company_id, and zero-length op_data.
 
 		VendorDependentReceived(string op_data)
 
-- 
1.7.3.4

Re: [PATCH 3/3] AVRCP: Add Passthrough Signal

[Johan Hedberg]

Hi David,

On Tue, Aug 23, 2011, David Stockwell wrote:
> +	if (key_pressed == VENDOR_UNIQUE_OP) {
> +		if (operands[1] == 0 || operand_count < 5) {
> +			pass_company_id = 0;
> +			pass_string = g_malloc0(1);

The above is a quite obscure way to allocate an empty string. Could you
change it to g_strdup(""). That requires a second or two less thinking
to figure out what the line is supposed to do :)

> +		} else if (operands[1] == 3 && operand_count == 5) {
> +			pass_company_id = get_company_id(operands + 2);
> +			pass_string = g_malloc0(1);

Same here.

> +		} else { /* op_length does not match operand_count */
> +			DBG("Passthrough: Malformed message");
> +			DBG("op_len %u, op_cnt %u", operands[1], operand_count);
> +			pass_company_id = 0;
> +			pass_string = g_malloc0(1);

And here.

> +		}
>  	} else {
> -		status = "pressed";
> -		pressed = 1;
> +		pass_company_id = 0;
> +		pass_string = g_malloc0(1);

And here.

> +		g_dbus_emit_signal(control->dev->conn, control->dev->path,
> +			   AUDIO_CONTROL_INTERFACE, "Passthrough",
> +			   DBUS_TYPE_BYTE, &key_pressed,
> +			   DBUS_TYPE_BOOLEAN, &key_state,
> +			   DBUS_TYPE_UINT32, &pass_company_id,
> +			   DBUS_TYPE_STRING, &pass_string,
> +			   DBUS_TYPE_INVALID);

The above lines still mix tabs and spaces for indentation. Just use
tabs.

Johan

Re: [PATCH 3/3] AVRCP: Add Passthrough Signal

[David Stockwell]

Johan,

Good suggestions (I guess I am an old assembly-language programmer)

-----Original Message----- 
From: Johan Hedberg
Sent: Wednesday, August 24, 2011 3:36 AM
To: David Stockwell
Cc: linux-bluetooth@vger.kernel.org ; lucas.demarchi@profusion.mobi ; 
luiz.dentz@gmail.com
Subject: Re: [PATCH 3/3] AVRCP: Add Passthrough Signal

Hi David,

On Tue, Aug 23, 2011, David Stockwell wrote:
> + if (key_pressed == VENDOR_UNIQUE_OP) {
> + if (operands[1] == 0 || operand_count < 5) {
> + pass_company_id = 0;
> + pass_string = g_malloc0(1);

The above is a quite obscure way to allocate an empty string. Could you
change it to g_strdup(""). That requires a second or two less thinking
to figure out what the line is supposed to do :)

+++++OK

> + } else if (operands[1] == 3 && operand_count == 5) {
> + pass_company_id = get_company_id(operands + 2);
> + pass_string = g_malloc0(1);

Same here.

> + } else { /* op_length does not match operand_count */
> + DBG("Passthrough: Malformed message");
> + DBG("op_len %u, op_cnt %u", operands[1], operand_count);
> + pass_company_id = 0;
> + pass_string = g_malloc0(1);

And here. +++++

> + }
>  } else {
> - status = "pressed";
> - pressed = 1;
> + pass_company_id = 0;
> + pass_string = g_malloc0(1);

And here.

> + g_dbus_emit_signal(control->dev->conn, control->dev->path,
> +    AUDIO_CONTROL_INTERFACE, "Passthrough",
> +    DBUS_TYPE_BYTE, &key_pressed,
> +    DBUS_TYPE_BOOLEAN, &key_state,
> +    DBUS_TYPE_UINT32, &pass_company_id,
> +    DBUS_TYPE_STRING, &pass_string,
> +    DBUS_TYPE_INVALID);

The above lines still mix tabs and spaces for indentation. Just use
tabs.

++++ Sorry, thought I had fixed this in Kate.  checkpatch.pl did not catch 
it...will fix.

Johan 

[PATCH 3/3] AVRCP: Add Passthrough Signal

[David Stockwell]

[PATCH 3/3] AVRCP: Add Passthrough Signal

Send Passthrough signal, not only for simple keystrokes,
but especially for Vendor Unique key, passing company-id
and vendor-unique string as well.
---
 audio/control.c     |   90 
+++++++++++++++++++++++++++++++++++++++++++--------
 doc/control-api.txt |   14 +++-----
 2 files changed, 81 insertions(+), 23 deletions(-)

diff --git a/audio/control.c b/audio/control.c
index eb86029..269c793 100644
--- a/audio/control.c
+++ b/audio/control.c
@@ -106,6 +106,8 @@
 #define FORWARD_OP		0x4b
 #define BACKWARD_OP		0x4c
 
+#define VENDOR_UNIQUE_OP	0x7E
+
 /* Company IDs for vendor dependent commands */
 #define IEEEID_BTSIG		0x001958
 
@@ -515,28 +517,88 @@ static void send_key(int fd, uint16_t key, int pressed)
 	send_event(fd, EV_SYN, SYN_REPORT, 0);
 }
 
+/**
+ *	handle_panel_passthrough:
+ *
+ *	Handles AVRCP 1.0+ PASSTHROUGH command, passes the keystroke to uinput.
+ *
+ *	Added a Passthrough signal, with the key state and the optional
+ *	following company_id and vendor-unique message.
+ */
+
 static void handle_panel_passthrough(struct control *control,
-					const unsigned char *operands,
+					const uint8_t *operands,
 					int operand_count)
 {
 	const char *status;
-	int pressed, i;
-
-	if (operand_count == 0)
+	int i;
+	uint8_t key_pressed;
+	gboolean key_state;
+	uint32_t pass_company_id;
+	char *pass_string;
+	/*
+	 * operands[1] is operation_data_field_length (AV/C Panel Specification
+	 * v1.23, sect 9.4.5). Should always be present, even if zero.
+	 */
+	if (operand_count < 2)
 		return;
 
-	if (operands[0] & 0x80) {
-		status = "released";
-		pressed = 0;
+	key_pressed = operands[0] & 0x7F;
+
+	/* If key is pressed, key state bit is zero (AVRCP v13r00 p89). */
+	key_state = ((operands[0] & 0x80) == 0);
+	status = key_state ? "pressed" : "released";
+
+	DBG("Passthrough Key: %x %s", key_pressed, status);
+
+	if (key_pressed == VENDOR_UNIQUE_OP) {
+		if (operands[1] == 0 || operand_count < 5) {
+			pass_company_id = 0;
+			pass_string = g_strdup("");
+			DBG("Passthrough: No Company_ID or String");
+		} else if (operands[1] == 3 && operand_count == 5) {
+			pass_company_id = get_company_id(operands + 2);
+			pass_string = g_strdup("");
+			DBG("Passthrough Company_ID: %06X String: <none>",
+						pass_company_id);
+		} else if (operands[1] > 3 &&
+					operand_count == operands[1] + 2) {
+			pass_company_id = get_company_id(operands + 2);
+			pass_string = g_strndup((gchar *) operands + 5,
+							operands[1] - 3);
+			DBG("Passthrough Company_ID: %06X String: %s",
+						pass_company_id, pass_string);
+		} else { /* op_length does not match operand_count */
+			DBG("Passthrough: Malformed message");
+			DBG("op_len %u, op_cnt %u", operands[1], operand_count);
+			pass_company_id = 0;
+			pass_string = g_strdup("");
+		}
 	} else {
-		status = "pressed";
-		pressed = 1;
+		pass_company_id = 0;
+		pass_string = g_strdup("");
 	}
 
+	/*
+	 * Generate passthrough signal only if not BTSIG Company_ID.
+	 * For BTSIG, passthrough only for Group Navigation (unimplemented).
+	 */
+
+	if (pass_company_id != IEEEID_BTSIG)
+		g_dbus_emit_signal(control->dev->conn, control->dev->path,
+			AUDIO_CONTROL_INTERFACE, "Passthrough",
+			DBUS_TYPE_BYTE, &key_pressed,
+			DBUS_TYPE_BOOLEAN, &key_state,
+			DBUS_TYPE_UINT32, &pass_company_id,
+			DBUS_TYPE_STRING, &pass_string,
+			DBUS_TYPE_INVALID);
+
+	g_free(pass_string);
+
 	for (i = 0; key_map[i].name != NULL; i++) {
 		uint8_t key_quirks;
 
-		if ((operands[0] & 0x7F) != key_map[i].avrcp)
+		if (key_pressed != key_map[i].avrcp)
 			continue;
 
 		DBG("AVRCP: %s %s", key_map[i].name, status);
@@ -544,7 +606,7 @@ static void handle_panel_passthrough(struct control 
*control,
 		key_quirks = control->key_quirks[key_map[i].avrcp];
 
 		if (key_quirks & QUIRK_NO_RELEASE) {
-			if (!pressed) {
+			if (!key_state) {
 				DBG("AVRCP: Ignoring release");
 				break;
 			}
@@ -555,13 +617,12 @@ static void handle_panel_passthrough(struct control 
*control,
 			break;
 		}
 
-		send_key(control->uinput, key_map[i].uinput, pressed);
+		send_key(control->uinput, key_map[i].uinput, key_state);
 		break;
 	}
 
 	if (key_map[i].name == NULL)
-		DBG("AVRCP: unknown button 0x%02X %s",
-						operands[0] & 0x7F, status);
+		DBG("AVRCP: unknown button 0x%02X %s", key_pressed, status);
 }
 
 static unsigned int attr_get_max_val(uint8_t attr)
@@ -2291,6 +2352,7 @@ static GDBusSignalTable control_signals[] = {
 	{ "Connected",			"",	G_DBUS_SIGNAL_FLAG_DEPRECATED},
 	{ "Disconnected",		"",	G_DBUS_SIGNAL_FLAG_DEPRECATED},
 	{ "PropertyChanged",		"sv"	},
+	{ "Passthrough",		"ybus"	},
 	{ NULL, NULL }
 };
 
diff --git a/doc/control-api.txt b/doc/control-api.txt
index a7e5cbb..64ea5d3 100644
--- a/doc/control-api.txt
+++ b/doc/control-api.txt
@@ -55,18 +55,14 @@ Signals		Connected()
 			Sent when the AVRCP connection to the remote device
 			has been disconnected.
 
-		Passthrough(uint8 key, boolean state, int32 company_id,
+		Passthrough(uint8 key, boolean state, uint32 company_id,
 								string op_data)
 
-			Called when Passthrough command is received from
-			connected device.
+			Sent when Passthrough received from CT.
 
-			NOTE: according to the AV/C Subpanel Spec, company_id
-			and op_data are passed ONLY when the key is
-			"Vendor_Unique", or 0x7E.
-
-			When the key is NOT 0x7E, the signal returns
-			company_id=-1, and zero-length op_data.
+			Company_id and op_data returned only when key is 0x7E
+			(OP_VENDOR_UNIQUE).  Otherwise, returns zero for
+			company_id, and zero-length op_data.
 
 		VendorDependentReceived(string op_data)
 
-- 
1.7.3.4

Re: [PATCH 3/3] AVRCP: Add Passthrough Signal

[Luiz Augusto von Dentz]

Hi David,

On Wed, Aug 24, 2011 at 4:07 PM, David Stockwell
<dstockwell@frequency-one.com> wrote:
> [PATCH 3/3] AVRCP: Add Passthrough Signal
>
> Send Passthrough signal, not only for simple keystrokes,
> but especially for Vendor Unique key, passing company-id
> and vendor-unique string as well.

I would like to do this differently, with target/controller/player
registering themselves to avoid having to blindly emit signals like
this which can no effect at all or loop effect, this should also make
us able to detect if there are multiple entities and address them
properly which is needed for 1.4.


-- 
Luiz Augusto von Dentz

Re: [PATCH 3/3] AVRCP: Add Passthrough Signal

[David Stockwell]

Hi Luiz,

-----Original Message----- 
From: Luiz Augusto von Dentz
Sent: Wednesday, August 24, 2011 5:47 PM
To: David Stockwell
Cc: linux-bluetooth@vger.kernel.org ; johan.hedberg@gmail.com
Subject: Re: [PATCH 3/3] AVRCP: Add Passthrough Signal

Hi David,

On Wed, Aug 24, 2011 at 4:07 PM, David Stockwell
<dstockwell@frequency-one.com> wrote:
> [PATCH 3/3] AVRCP: Add Passthrough Signal
>
> Send Passthrough signal, not only for simple keystrokes,
> but especially for Vendor Unique key, passing company-id
> and vendor-unique string as well.

I would like to do this differently, with target/controller/player
registering themselves to avoid having to blindly emit signals like
this which can no effect at all or loop effect, this should also make
us able to detect if there are multiple entities and address them
properly which is needed for 1.4.

+++++ OK, maybe a readwrite Passthrough property indicating how passthroughs 
are handled: Signal, Uinput, Both, None?  Or simply a PassthruSignal 
property (true/false, default false), given that the presence/absence of 
/dev/uinput handles the other case?

I really think the Control should emit the signal...since it comes from a CT 
(and not really from a MediaPlayer or somesuch).

-- 
Luiz Augusto von Dentz 

Re: [PATCH 3/3] AVRCP: Add Passthrough Signal

[Luiz Augusto von Dentz]

Hi David,

On Thu, Aug 25, 2011 at 2:58 AM, David Stockwell
<dstockwell@frequency-one.com> wrote:
> +++++ OK, maybe a readwrite Passthrough property indicating how passthroughs
> are handled: Signal, Uinput, Both, None?  Or simply a PassthruSignal
> property (true/false, default false), given that the presence/absence of
> /dev/uinput handles the other case?
>
> I really think the Control should emit the signal...since it comes from a CT
> (and not really from a MediaPlayer or somesuch).

You are missing a very important point here, parsing of these vendor
specific commands would have to be split to each target thus we cannot
guarantee a consistent handling of them, if we stick to uinput I think
we should use KEY_VENDOR, otherwise we should send the commands
directly to the player.

-- 
Luiz Augusto von Dentz

Re: [PATCH 3/3] AVRCP: Add Passthrough Signal

[David Stockwell]

Hello, Luiz

-----Original Message----- 
From: Luiz Augusto von Dentz
Sent: Thursday, August 25, 2011 2:44 AM
To: David Stockwell
Cc: linux-bluetooth@vger.kernel.org ; johan.hedberg@gmail.com
Subject: Re: [PATCH 3/3] AVRCP: Add Passthrough Signal

Hi David,

On Thu, Aug 25, 2011 at 2:58 AM, David Stockwell
<dstockwell@frequency-one.com> wrote:
> +++++ OK, maybe a readwrite Passthrough property indicating how 
> passthroughs
> are handled: Signal, Uinput, Both, None?  Or simply a PassthruSignal
> property (true/false, default false), given that the presence/absence of
> /dev/uinput handles the other case?
>
> I really think the Control should emit the signal...since it comes from a 
> CT
> (and not really from a MediaPlayer or somesuch).

You are missing a very important point here, parsing of these vendor
specific commands would have to be split to each target thus we cannot
guarantee a consistent handling of them, if we stick to uinput I think
we should use KEY_VENDOR, otherwise we should send the commands
directly to the player.

+++++
Actually, I expected that the routing of these passthroughs would be handled 
outside BlueZ, from Control(s) to (whatever) media playing app(s) in a 
supervisory app/daemon.  Whether the signal is issued from the Control or 
from the MediaPlayer interface is not significant to me; an easy code change 
in the patch and in my supervisory app.  Any additional thoughts or 
comments?
+++++
-- 
Luiz Augusto von Dentz 

Re: [PATCH 3/3] AVRCP: Add Passthrough Signal

[David Stockwell]

Luiz,

-----Original Message----- 
From: David Stockwell

Hello, Luiz

-----Original Message----- 
From: Luiz Augusto von Dentz

Hi David,

On Thu, Aug 25, 2011 at 2:58 AM, David Stockwell
<dstockwell@frequency-one.com> wrote:
> +++++ OK, maybe a readwrite Passthrough property indicating how 
> passthroughs
> are handled: Signal, Uinput, Both, None?  Or simply a PassthruSignal
> property (true/false, default false), given that the presence/absence of
> /dev/uinput handles the other case?
>
> I really think the Control should emit the signal...since it comes from a 
> CT
> (and not really from a MediaPlayer or somesuch).

You are missing a very important point here, parsing of these vendor
specific commands would have to be split to each target thus we cannot
guarantee a consistent handling of them, if we stick to uinput I think
we should use KEY_VENDOR, otherwise we should send the commands
directly to the player.

+++++
Actually, I expected that the routing of these passthroughs would be handled
outside BlueZ, from Control(s) to (whatever) media playing app(s) in a
supervisory app/daemon.  Whether the signal is issued from the Control or
from the MediaPlayer interface is not significant to me; an easy code change
in the patch and in my supervisory app.  Any additional thoughts or
comments?
+++++

+++++

On second/third thought: while right now the Path of the Control and the 
MediaPlayer are the same (just different interfaces), this will probably not 
be the same in the future.

For my purposes, any Passthroughs received from a Control need to be tied 
back to the Control, hopefully by the path (which includes the bdaddr of the 
device).  Any routing from Control(s) to the media playing app(s) is really 
better handled outside, in the supervisory app.  Any impacts on the state of 
MediaPlayer are better handled outside BlueZ, looping back via the 
supervisory app.

Anyway, I know I am looking at things differently.  If this does not fit 
into the BlueZ Scheme of things, I will be quite happy to maintain this (and 
more) in a private fork, rebasing when warranted (while following GPL for 
any redistribution, etc.).

+++++
-- 
Luiz Augusto von Dentz