[PATCH BlueZ 0/2] AVRCP: Add support for Vol Up/Down operations on TG

[PATCH BlueZ 0/2] AVRCP: Add support for Vol Up/Down operations on TG

[João Paulo Rechi Vita]

This series creates a new quirk called QUIRK_IGNORE to be able to accept and
ignore certain keys. Then it uses this infrastructure to accept and ignore
Volume Up/Down operations, instead of replying with "Not Implemented".

Suport for Volume Up/Down is mandatory on TG side if we claim to support
Category 2 (Monitor / Amplifier). It's also tested by PTS on test
TC_TG_PTT_BV_02_I.

João Paulo Rechi Vita (2):
  avctp: Create ignore quirk
  avctp: Receive and silent ignore Vol Up/Down operations

 profiles/audio/avctp.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

-- 
1.7.11.7

[PATCH BlueZ 1/2] avctp: Create ignore quirk

[João Paulo Rechi Vita]

Create a quirk to be able to accept and trow away certain keys.
---
 profiles/audio/avctp.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/profiles/audio/avctp.c b/profiles/audio/avctp.c
index 61890cc..f7e607e 100644
--- a/profiles/audio/avctp.c
+++ b/profiles/audio/avctp.c
@@ -58,7 +58,8 @@
 #include "avctp.h"
 #include "avrcp.h"
 
-#define QUIRK_NO_RELEASE 1 << 0
+#define QUIRK_NO_RELEASE	1 << 0
+#define QUIRK_IGNORE		1 << 1
 
 /* Message types */
 #define AVCTP_COMMAND		0
@@ -287,6 +288,11 @@ static size_t handle_panel_passthrough(struct avctp *session,
 
 		key_quirks = session->key_quirks[key_map[i].avc];
 
+		if (key_quirks & QUIRK_IGNORE) {
+			DBG("AV/C: ignoring %s %s", key_map[i].name, status);
+			break;
+		}
+
 		if (key_quirks & QUIRK_NO_RELEASE) {
 			if (!pressed) {
 				DBG("AV/C: Ignoring release");
-- 
1.7.11.7

[PATCH BlueZ 2/2] avctp: Receive and silent ignore Vol Up/Down operations

[João Paulo Rechi Vita]

The AVRCP spec mandates to support 'volume up' and 'volume down'
operations when claiming support for Category 2 TG.
---
 profiles/audio/avctp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/profiles/audio/avctp.c b/profiles/audio/avctp.c
index f7e607e..4ab6d6d 100644
--- a/profiles/audio/avctp.c
+++ b/profiles/audio/avctp.c
@@ -214,6 +214,8 @@ static struct {
 	uint8_t avc;
 	uint16_t uinput;
 } key_map[] = {
+	{ "VOLUME UP",		AVC_VOLUME_UP,		KEY_VOLUMEUP},
+	{ "VOLUME DOWN",	AVC_VOLUME_DOWN,	KEY_VOLUMEDOWN},
 	{ "PLAY",		AVC_PLAY,		KEY_PLAYCD },
 	{ "STOP",		AVC_STOP,		KEY_STOPCD },
 	{ "PAUSE",		AVC_PAUSE,		KEY_PAUSECD },
@@ -968,6 +970,9 @@ static void init_uinput(struct avctp *session)
 
 	dev = manager_get_audio_device(session->device, FALSE);
 
+	session->key_quirks[AVC_VOLUME_UP] |= QUIRK_IGNORE;
+	session->key_quirks[AVC_VOLUME_DOWN] |= QUIRK_IGNORE;
+
 	device_get_name(dev->btd_dev, name, sizeof(name));
 	if (g_str_equal(name, "Nokia CK-20W")) {
 		session->key_quirks[AVC_FORWARD] |= QUIRK_NO_RELEASE;
-- 
1.7.11.7

Re: [PATCH BlueZ 2/2] avctp: Receive and silent ignore Vol Up/Down operations

[Vinicius Costa Gomes]

Hi Joao,

On 15:18 Tue 22 Jan, João Paulo Rechi Vita wrote:
> The AVRCP spec mandates to support 'volume up' and 'volume down'
> operations when claiming support for Category 2 TG.
> ---
>  profiles/audio/avctp.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/profiles/audio/avctp.c b/profiles/audio/avctp.c
> index f7e607e..4ab6d6d 100644
> --- a/profiles/audio/avctp.c
> +++ b/profiles/audio/avctp.c
> @@ -214,6 +214,8 @@ static struct {
>  	uint8_t avc;
>  	uint16_t uinput;
>  } key_map[] = {
> +	{ "VOLUME UP",		AVC_VOLUME_UP,		KEY_VOLUMEUP},
> +	{ "VOLUME DOWN",	AVC_VOLUME_DOWN,	KEY_VOLUMEDOWN},
>  	{ "PLAY",		AVC_PLAY,		KEY_PLAYCD },
>  	{ "STOP",		AVC_STOP,		KEY_STOPCD },
>  	{ "PAUSE",		AVC_PAUSE,		KEY_PAUSECD },
> @@ -968,6 +970,9 @@ static void init_uinput(struct avctp *session)
>  
>  	dev = manager_get_audio_device(session->device, FALSE);
>  
> +	session->key_quirks[AVC_VOLUME_UP] |= QUIRK_IGNORE;
> +	session->key_quirks[AVC_VOLUME_DOWN] |= QUIRK_IGNORE;
> +

Having a quirk that applies to every device, doesn't seem to map to the
meaning of 'quirk', i.e. if everybody has the same quirk, it is the norm ;-)


Cheers,
-- 
Vinicius

Re: [PATCH BlueZ 2/2] avctp: Receive and silent ignore Vol Up/Down operations

[Joao Paulo Rechi Vita]

On Tue, Jan 22, 2013 at 3:56 PM, Vinicius Costa Gomes
<vinicius.gomes@openbossa.org> wrote:
> Hi Joao,
>
> On 15:18 Tue 22 Jan, João Paulo Rechi Vita wrote:
>> The AVRCP spec mandates to support 'volume up' and 'volume down'
>> operations when claiming support for Category 2 TG.
>> ---
>>  profiles/audio/avctp.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/profiles/audio/avctp.c b/profiles/audio/avctp.c
>> index f7e607e..4ab6d6d 100644
>> --- a/profiles/audio/avctp.c
>> +++ b/profiles/audio/avctp.c
>> @@ -214,6 +214,8 @@ static struct {
>>       uint8_t avc;
>>       uint16_t uinput;
>>  } key_map[] = {
>> +     { "VOLUME UP",          AVC_VOLUME_UP,          KEY_VOLUMEUP},
>> +     { "VOLUME DOWN",        AVC_VOLUME_DOWN,        KEY_VOLUMEDOWN},
>>       { "PLAY",               AVC_PLAY,               KEY_PLAYCD },
>>       { "STOP",               AVC_STOP,               KEY_STOPCD },
>>       { "PAUSE",              AVC_PAUSE,              KEY_PAUSECD },
>> @@ -968,6 +970,9 @@ static void init_uinput(struct avctp *session)
>>
>>       dev = manager_get_audio_device(session->device, FALSE);
>>
>> +     session->key_quirks[AVC_VOLUME_UP] |= QUIRK_IGNORE;
>> +     session->key_quirks[AVC_VOLUME_DOWN] |= QUIRK_IGNORE;
>> +
>
> Having a quirk that applies to every device, doesn't seem to map to the
> meaning of 'quirk', i.e. if everybody has the same quirk, it is the norm ;-)
>

It is not a _device quirk_ but it is still a _key quirk_, that is, a
special behavior to handle that key. I could also have checked
specifically for the key id, but I find clearer to do it this way.

--
João Paulo Rechi Vita
Openbossa Labs - INdT

Re: [PATCH BlueZ 1/2] avctp: Create ignore quirk

[Luiz Augusto von Dentz]

Hi Joao,

On Tue, Jan 22, 2013 at 8:18 PM, João Paulo Rechi Vita
<jprvita@openbossa.org> wrote:
> Create a quirk to be able to accept and trow away certain keys.
> ---
>  profiles/audio/avctp.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/profiles/audio/avctp.c b/profiles/audio/avctp.c
> index 61890cc..f7e607e 100644
> --- a/profiles/audio/avctp.c
> +++ b/profiles/audio/avctp.c
> @@ -58,7 +58,8 @@
>  #include "avctp.h"
>  #include "avrcp.h"
>
> -#define QUIRK_NO_RELEASE 1 << 0
> +#define QUIRK_NO_RELEASE       1 << 0
> +#define QUIRK_IGNORE           1 << 1
>
>  /* Message types */
>  #define AVCTP_COMMAND          0
> @@ -287,6 +288,11 @@ static size_t handle_panel_passthrough(struct avctp *session,
>
>                 key_quirks = session->key_quirks[key_map[i].avc];
>
> +               if (key_quirks & QUIRK_IGNORE) {
> +                       DBG("AV/C: ignoring %s %s", key_map[i].name, status);
> +                       break;
> +               }
> +
>                 if (key_quirks & QUIRK_NO_RELEASE) {
>                         if (!pressed) {
>                                 DBG("AV/C: Ignoring release");
> --
> 1.7.11.7

In the end I think we should just accept the commands normally, let me
quote the recommendations (RD=Rendering Device MP=Media Player):

  "Recommendation 16:

  If volume is changed on the RD, the RD should not send an AVRCP
volume command to the MP device.

  Motivation 16:

  Sending an AVRCP volume command to the MP may cause the MP to send
again an AVRCP volume
  command to the RD device which could lead to an endless loop of
AVRCP volume commands.

  Recommendation 17:

  If a device receives an AVRCP volume command, it shall not send back
an AVRCP volume command.

  Motivation 17:

  This will also ensure that endless loop does not happen with
existing devices which do not comply with the
  recommendation."

So there is nothing against the RD accepting the Volume Up/Down it
should just no send it back.


--
Luiz Augusto von Dentz

Re: [PATCH BlueZ 1/2] avctp: Create ignore quirk

[Joao Paulo Rechi Vita]

On Fri, Jan 25, 2013 at 10:35 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Joao,
>
> On Tue, Jan 22, 2013 at 8:18 PM, João Paulo Rechi Vita
> <jprvita@openbossa.org> wrote:
>> Create a quirk to be able to accept and trow away certain keys.
>> ---
>>  profiles/audio/avctp.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/profiles/audio/avctp.c b/profiles/audio/avctp.c
>> index 61890cc..f7e607e 100644
>> --- a/profiles/audio/avctp.c
>> +++ b/profiles/audio/avctp.c
>> @@ -58,7 +58,8 @@
>>  #include "avctp.h"
>>  #include "avrcp.h"
>>
>> -#define QUIRK_NO_RELEASE 1 << 0
>> +#define QUIRK_NO_RELEASE       1 << 0
>> +#define QUIRK_IGNORE           1 << 1
>>
>>  /* Message types */
>>  #define AVCTP_COMMAND          0
>> @@ -287,6 +288,11 @@ static size_t handle_panel_passthrough(struct avctp *session,
>>
>>                 key_quirks = session->key_quirks[key_map[i].avc];
>>
>> +               if (key_quirks & QUIRK_IGNORE) {
>> +                       DBG("AV/C: ignoring %s %s", key_map[i].name, status);
>> +                       break;
>> +               }
>> +
>>                 if (key_quirks & QUIRK_NO_RELEASE) {
>>                         if (!pressed) {
>>                                 DBG("AV/C: Ignoring release");
>> --
>> 1.7.11.7
>
> In the end I think we should just accept the commands normally, let me
> quote the recommendations (RD=Rendering Device MP=Media Player):
>
>   "Recommendation 16:
>
>   If volume is changed on the RD, the RD should not send an AVRCP
> volume command to the MP device.
>
>   Motivation 16:
>
>   Sending an AVRCP volume command to the MP may cause the MP to send
> again an AVRCP volume
>   command to the RD device which could lead to an endless loop of
> AVRCP volume commands.
>
>   Recommendation 17:
>
>   If a device receives an AVRCP volume command, it shall not send back
> an AVRCP volume command.
>
>   Motivation 17:
>
>   This will also ensure that endless loop does not happen with
> existing devices which do not comply with the
>   recommendation."
>
> So there is nothing against the RD accepting the Volume Up/Down it
> should just no send it back.
>

All right, so I'll update the patches to forward the Volume Up and
Down to uinput.


--
João Paulo Rechi Vita
Openbossa Labs - INdT