[PATCH 2/3] AVRCP: Add Passthrough signal

[PATCH 2/3] AVRCP: Add Passthrough signal

[David Stockwell]

Add Passthrough signal, passing key state.

If key is Vendor Unique (0x7E), also pass vendor's company_id and vendor-
unique message as string.

Signed-off-by: David Stockwell <dstockwell@frequency-one.com>
---
 audio/control.c     |   75 
++++++++++++++++++++++++++++++++++++++++++++++++++-
 doc/control-api.txt |   14 +++------
 2 files changed, 79 insertions(+), 10 deletions(-)

diff --git a/audio/control.c b/audio/control.c
index 882c9fb..4e10cac 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
 
@@ -470,6 +472,15 @@ static sdp_record_t *avrcp_tg_record(void)
 	return record;
 }
 
+/**
+ *	get_company_id:
+ *
+ *	Return three-byte Company_ID from AVRCP message
+ */
+static uint32_t get_company_id(uint8_t *cid) {
+	return *cid << 16 | *(cid + 1) << 8 | *(cid + 2);
+}
+
 static int send_event(int fd, uint16_t type, uint16_t code, int32_t value)
 {
 	struct uinput_event event;
@@ -491,16 +502,77 @@ 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,
 					int operand_count)
 {
 	const char *status;
 	int pressed, i;
-
+	uint8_t		key_pressed;
+	gboolean	key_status;
+	uint32_t	pass_company_id;
+	gchar		*pass_string;
+	
 	if (operand_count == 0)
 		return;
 
+	/*
+	 * Following creates the Passthrough signal.
+	 * Key_state is zero if key is pressed (AVRCP v13r00 sect 24, p89)
+	 */
+
+	key_pressed = operands[0] & 0x7F;
+	key_status = ((operands[0] & 0x80) == 0);
+
+	DBG("Passthrough Key: %x Pressed: %s", key_pressed,
+						key_status ? "true" : "false");
+	if (key_pressed==VENDOR_UNIQUE_OP) {
+		if (operands[1] > 3) {
+			pass_company_id = get_company_id((uint8_t *) &operands[2]);
+			pass_string = g_strndup((const char *) &operands[5],
+						(gsize) operands[1] - 3);
+			DBG("Passthrough Company_ID: %06X String: %s",
+			    pass_company_id, pass_string);
+		} else if (operands[1] == 3) {
+			pass_company_id = get_company_id((uint8_t *) &operands[2]);
+			pass_string = (gchar *) g_malloc0(1);
+			DBG("Passthrough Company_ID: %06X String: <none>",
+			    pass_company_id);
+		} else {
+			pass_company_id = 0;
+			pass_string = (gchar *) g_malloc0(1);
+			DBG("Passthrough: No Company_ID or String!");
+		};
+	} else {
+		pass_company_id = 0;
+		pass_string = (gchar *) 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_status,
+			   DBUS_TYPE_UINT32, &pass_company_id,
+			   DBUS_TYPE_STRING, &pass_string,
+			   DBUS_TYPE_INVALID);
+	
+	g_free(pass_string);
+
 	if (operands[0] & 0x80) {
 		status = "released";
 		pressed = 0;
@@ -2279,6 +2351,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 2/3] AVRCP: Add Passthrough signal

[Johan Hedberg]

Hi David,

On Sat, Aug 20, 2011, David Stockwell wrote:
> Add Passthrough signal, passing key state.
> 
> If key is Vendor Unique (0x7E), also pass vendor's company_id and vendor-
> unique message as string.
> 
> Signed-off-by: David Stockwell <dstockwell@frequency-one.com>

No signed-off-by in user-space patches please.

> +/**
> + *	get_company_id:
> + *
> + *	Return three-byte Company_ID from AVRCP message
> + */
> +static uint32_t get_company_id(uint8_t *cid) {
> +	return *cid << 16 | *(cid + 1) << 8 | *(cid + 2);
> +}

Please follow the coding style: new line before the opening {.  Also,
wouldn't it be a bit clearer if the input parameter was defined as:

	static uint32_t get_company_id(uint8_t cid[3])

>  static void handle_panel_passthrough(struct control *control,
>  					const unsigned char *operands,
>  					int operand_count)
>  {
>  	const char *status;
>  	int pressed, i;
> -
> +	uint8_t		key_pressed;
> +	gboolean	key_status;
> +	uint32_t	pass_company_id;
> +	gchar		*pass_string;

It seems to me you're adding some redundancies here with the existing
pressed and status variables. Try to consolidate these to the bare
minimum if possible.

> +	if (key_pressed==VENDOR_UNIQUE_OP) {

Coding style: space before and after ==

> +		if (operands[1] > 3) {
> +			pass_company_id = get_company_id((uint8_t *) &operands[2]);

You're not checking that operand_count is large enough before accessing
operands[1] and operands[2], i.e. essentially exposing yourself to a
buffer overflow dependent on unchecked data received from the remote
device.

> +			pass_string = g_strndup((const char *) &operands[5],
> +						(gsize) operands[1] - 3);

Same thing here with operands[5] and operands[1]

> +			DBG("Passthrough Company_ID: %06X String: %s",
> +			    pass_company_id, pass_string);

Mixed tabs and spaces for indentation in the line above.

> +		} else if (operands[1] == 3) {
> +			pass_company_id = get_company_id((uint8_t *) &operands[2]);

Seems line an unnecessary typecast here, or then you need to change the
parameter type for get_company_id

> +			pass_string = (gchar *) g_malloc0(1);

certainly an unnecessary typecast. gpointer and void* never need it.

> +			DBG("Passthrough Company_ID: %06X String: <none>",
> +			    pass_company_id);

Mixed tabs and spaces in the line above.

> +		} else {
> +			pass_company_id = 0;
> +			pass_string = (gchar *) g_malloc0(1);

Unnecessary typecast.

> +			DBG("Passthrough: No Company_ID or String!");
> +		};
> +	} else {
> +		pass_company_id = 0;
> +		pass_string = (gchar *) g_malloc0(1);

Same here.

> +	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_status,
> +			   DBUS_TYPE_UINT32, &pass_company_id,
> +			   DBUS_TYPE_STRING, &pass_string,
> +			   DBUS_TYPE_INVALID);
> +	

Mixed tabs and spaces above. Additionally you've got a tab on the last
line which should be empty (i.e. only contain the newline character).

Johan

Re: [PATCH 2/3] AVRCP: Add Passthrough signal

[David Stockwell]

Hello, Johan.

-----Original Message----- 
From: Johan Hedberg
Sent: Monday, August 22, 2011 5:27 AM
To: David Stockwell
Cc: linux-bluetooth@vger.kernel.org
Subject: Re: [PATCH 2/3] AVRCP: Add Passthrough signal

Hi David,

On Sat, Aug 20, 2011, David Stockwell wrote:
> Add Passthrough signal, passing key state.
>
> If key is Vendor Unique (0x7E), also pass vendor's company_id and vendor-
> unique message as string.
>
> Signed-off-by: David Stockwell <dstockwell@frequency-one.com>

No signed-off-by in user-space patches please.

+++++ Got it.  Will (not) do.

> +/**
> + * get_company_id:
> + *
> + * Return three-byte Company_ID from AVRCP message
> + */
> +static uint32_t get_company_id(uint8_t *cid) {
> + return *cid << 16 | *(cid + 1) << 8 | *(cid + 2);
> +}

Please follow the coding style: new line before the opening {.  Also,
wouldn't it be a bit clearer if the input parameter was defined as:

static uint32_t get_company_id(uint8_t cid[3])

Regarding new style...sorry, this is an old fix.  Should have run this 
through the check-patches tool.  Will fix and resubmit.
Also agree regarding the input...it is more clear the way you recommend.

>  static void handle_panel_passthrough(struct control *control,
>  const unsigned char *operands,
>  int operand_count)
>  {
>  const char *status;
>  int pressed, i;
> -
> + uint8_t key_pressed;
> + gboolean key_status;
> + uint32_t pass_company_id;
> + gchar *pass_string;

It seems to me you're adding some redundancies here with the existing
pressed and status variables. Try to consolidate these to the bare
minimum if possible.

+++++ OK, will take a look.

> + if (key_pressed==VENDOR_UNIQUE_OP) {

Coding style: space before and after ==

Right, forgot to run the patch check...got lazy.

> + if (operands[1] > 3) {
> + pass_company_id = get_company_id((uint8_t *) &operands[2]);

You're not checking that operand_count is large enough before accessing
operands[1] and operands[2], i.e. essentially exposing yourself to a
buffer overflow dependent on unchecked data received from the remote
device.

+++++  Yes... will fix.

> + pass_string = g_strndup((const char *) &operands[5],
> + (gsize) operands[1] - 3);

Same thing here with operands[5] and operands[1]

> + DBG("Passthrough Company_ID: %06X String: %s",
> +     pass_company_id, pass_string);

Mixed tabs and spaces for indentation in the line above.

> + } else if (operands[1] == 3) {
> + pass_company_id = get_company_id((uint8_t *) &operands[2]);

Seems line an unnecessary typecast here, or then you need to change the
parameter type for get_company_id

> + pass_string = (gchar *) g_malloc0(1);

certainly an unnecessary typecast. gpointer and void* never need it.

> + DBG("Passthrough Company_ID: %06X String: <none>",
> +     pass_company_id);

Mixed tabs and spaces in the line above.

> + } else {
> + pass_company_id = 0;
> + pass_string = (gchar *) g_malloc0(1);

Unnecessary typecast.

> + DBG("Passthrough: No Company_ID or String!");
> + };
> + } else {
> + pass_company_id = 0;
> + pass_string = (gchar *) g_malloc0(1);

Same here.

> + 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_status,
> +    DBUS_TYPE_UINT32, &pass_company_id,
> +    DBUS_TYPE_STRING, &pass_string,
> +    DBUS_TYPE_INVALID);
> +

Mixed tabs and spaces above. Additionally you've got a tab on the last
line which should be empty (i.e. only contain the newline character).

+++++ Actually, if I had just run check patches it would have caught all of 
this.

Sorry to see you expend the effort to write all of this (unless you have a 
"reject this patch" Perl script or something ;-).

Will resubmit, and it will be clean.



Johan 

Re: [PATCH 2/3] AVRCP: Add Passthrough signal

[Lucas De Marchi]

Hi David,

On Sat, Aug 20, 2011 at 7:51 PM, David Stockwell
<dstockwell@frequency-one.com> wrote:
> Add Passthrough signal, passing key state.
>
> If key is Vendor Unique (0x7E), also pass vendor's company_id and vendor-
> unique message as string.
>
> Signed-off-by: David Stockwell <dstockwell@frequency-one.com>
> ---
>  audio/control.c     |   75
> ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  doc/control-api.txt |   14 +++------
>  2 files changed, 79 insertions(+), 10 deletions(-)
>
> diff --git a/audio/control.c b/audio/control.c
> index 882c9fb..4e10cac 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
>
> @@ -470,6 +472,15 @@ static sdp_record_t *avrcp_tg_record(void)
>        return record;
>  }
>
> +/**
> + *     get_company_id:
> + *
> + *     Return three-byte Company_ID from AVRCP message
> + */
> +static uint32_t get_company_id(uint8_t *cid) {
> +       return *cid << 16 | *(cid + 1) << 8 | *(cid + 2);
> +}

I think if we are adding the generic function, It would be good to
split the patch and also change the handle_vendordep_pdu() function to
use it.


> +
>  static int send_event(int fd, uint16_t type, uint16_t code, int32_t value)
>  {
>        struct uinput_event event;
> @@ -491,16 +502,77 @@ 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,
>                                        int operand_count)
>  {
>        const char *status;
>        int pressed, i;
> -
> +       uint8_t         key_pressed;
> +       gboolean        key_status;
> +       uint32_t        pass_company_id;
> +       gchar           *pass_string;
> +
>        if (operand_count == 0)
>                return;
>
> +       /*
> +        * Following creates the Passthrough signal.
> +        * Key_state is zero if key is pressed (AVRCP v13r00 sect 24, p89)
> +        */
> +
> +       key_pressed = operands[0] & 0x7F;
> +       key_status = ((operands[0] & 0x80) == 0);
> +
> +       DBG("Passthrough Key: %x Pressed: %s", key_pressed,
> +                                               key_status ? "true" : "false");
> +       if (key_pressed==VENDOR_UNIQUE_OP) {
> +               if (operands[1] > 3) {
> +                       pass_company_id = get_company_id((uint8_t *) &operands[2]);
> +                       pass_string = g_strndup((const char *) &operands[5],
> +                                               (gsize) operands[1] - 3);
> +                       DBG("Passthrough Company_ID: %06X String: %s",
> +                           pass_company_id, pass_string);
> +               } else if (operands[1] == 3) {
> +                       pass_company_id = get_company_id((uint8_t *) &operands[2]);
> +                       pass_string = (gchar *) g_malloc0(1);
> +                       DBG("Passthrough Company_ID: %06X String: <none>",
> +                           pass_company_id);
> +               } else {
> +                       pass_company_id = 0;
> +                       pass_string = (gchar *) g_malloc0(1);
> +                       DBG("Passthrough: No Company_ID or String!");
> +               };
> +       } else {
> +               pass_company_id = 0;
> +               pass_string = (gchar *) 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_status,
> +                          DBUS_TYPE_UINT32, &pass_company_id,
> +                          DBUS_TYPE_STRING, &pass_string,
> +                          DBUS_TYPE_INVALID);
> +
> +       g_free(pass_string);

II think the API designed for these passthrough is not good for what
we have now. It should be changed the same way we changed for other
commands into a separated MediaPlayer interface.

Also notice that we only support BT SIG passthrough commands (and I
think these are the ones to be sent through an interface like this).
For other companies, adding them here would imply to add them to the
GetCapability command.


Luiz, do you have any comments on this API?


Lucas De Marchi