[PATCH BlueZ 1/6] AVCTP: Fix setting wrong transaction id expected for responses

[PATCH BlueZ 1/6] AVCTP: Fix setting wrong transaction id expected for responses

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

The id were incremented after being set to the request so it is always
+1 of the actual transaction.
---
 audio/avctp.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/audio/avctp.c b/audio/avctp.c
index 1bc2a1d..5f9afa1 100644
--- a/audio/avctp.c
+++ b/audio/avctp.c
@@ -982,7 +982,7 @@ int avctp_send_vendordep_req(struct avctp *session, uint8_t code,
 	struct avctp_rsp_handler *handler;
 	int err;
 
-	err = avctp_send(session, id++, AVCTP_COMMAND, code, subunit,
+	err = avctp_send(session, id, AVCTP_COMMAND, code, subunit,
 				AVC_OP_VENDORDEP, operands, operand_count);
 	if (err < 0)
 		return err;
@@ -994,6 +994,8 @@ int avctp_send_vendordep_req(struct avctp *session, uint8_t code,
 
 	session->handlers = g_slist_prepend(session->handlers, handler);
 
+	id++;
+
 	return 0;
 }
 
-- 
1.7.7.6

[PATCH BlueZ 2/6] AVRCP: Fix not setting audio device connected to player

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

While connecting device should be set to match the AVCTP session and when
disconnected reset it back to NULL.
---
 audio/avrcp.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/audio/avrcp.c b/audio/avrcp.c
index cb6906d..b09fab0 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -1197,6 +1197,7 @@ static void state_changed(struct audio_device *dev, avctp_state_t old_state,
 	switch (new_state) {
 	case AVCTP_STATE_DISCONNECTED:
 		player->session = NULL;
+		player->dev = NULL;
 		player->registered_events = 0;
 
 		if (player->handler) {
@@ -1207,6 +1208,7 @@ static void state_changed(struct audio_device *dev, avctp_state_t old_state,
 		break;
 	case AVCTP_STATE_CONNECTING:
 		player->session = avctp_connect(&dev->src, &dev->dst);
+		player->dev = dev;
 
 		if (!player->handler)
 			player->handler = avctp_register_pdu_handler(
-- 
1.7.7.6

[PATCH BlueZ 3/6] media-api: Update documentation to include Volume property

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

---
 doc/media-api.txt |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/doc/media-api.txt b/doc/media-api.txt
index 7a593c3..4446439 100644
--- a/doc/media-api.txt
+++ b/doc/media-api.txt
@@ -341,3 +341,11 @@ Properties	object Device [readonly]
 			Optional. Indicates where is the transport being routed
 
 			Possible Values: "HCI" or "PCM"
+
+		uint16 Volume [readwrite]
+
+			Optional. Indicates volume level of the transport,
+			this property is only writeable when the transport was
+			acquired by the sender.
+
+			Possible Values: 0-127
-- 
1.7.7.6

[PATCH BlueZ 4/6] audio: Fix signature type for transport Volume

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Signature is now uint16 instead of byte
---
 audio/transport.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/audio/transport.c b/audio/transport.c
index 4ad8608..6ed5d21 100644
--- a/audio/transport.c
+++ b/audio/transport.c
@@ -1076,5 +1076,5 @@ void media_transport_update_volume(struct media_transport *transport,
 
 	emit_property_changed(transport->conn, transport->path,
 				MEDIA_TRANSPORT_INTERFACE, "Volume",
-				DBUS_TYPE_BYTE, &transport->volume);
+				DBUS_TYPE_UINT16, &transport->volume);
 }
-- 
1.7.7.6

[PATCH BlueZ 5/6] AVRCP: Add support for sending SetAbsoluteVolume

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Once the transport volume is changed update the remote volume by sending
SetAbsoluteVolume:

< AVCTP: Command : pt 0x00 transaction 9 pid 0x110e
    AV/C: Changed: address 0x48 opcode 0x00
      Subunit: Panel
      Opcode: Vendor Dependent
      Company ID: 0x001958
      AVRCP: SetAbsoluteVolume: pt Single len 0x0001
        Volume: 100.00% (127/127)
> AVCTP: Response : pt 0x00 transaction 9 pid 0x110e
    AV/C: Accepted: address 0x48 opcode 0x00
      Subunit: Panel
      Opcode: Vendor Dependent
      Company ID: 0x001958
      AVRCP: SetAbsoluteVolume: pt Single len 0x0001
        Volume: 100.00% (127/127)
---
 audio/avrcp.c     |   45 ++++++++++++++++++++++++++++++++++++++++++---
 audio/avrcp.h     |    2 ++
 audio/transport.c |   20 +++++++++++++++++++-
 3 files changed, 63 insertions(+), 4 deletions(-)

diff --git a/audio/avrcp.c b/audio/avrcp.c
index b09fab0..eb5241f 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -87,6 +87,7 @@
 #define AVRCP_REGISTER_NOTIFICATION	0x31
 #define AVRCP_REQUEST_CONTINUING	0x40
 #define AVRCP_ABORT_CONTINUING		0x41
+#define AVRCP_SET_ABSOLUTE_VOLUME	0x50
 
 /* Capabilities for AVRCP_GET_CAPABILITIES pdu */
 #define CAP_COMPANY_ID		0x02
@@ -1145,15 +1146,20 @@ static gboolean avrcp_handle_volume_changed(struct avctp *session,
 {
 	struct avrcp_player *player = user_data;
 	struct avrcp_header *pdu = (void *) operands;
-	uint8_t abs_volume = pdu->params[1] & 0x7F;
+	uint8_t volume = pdu->params[1] & 0x7F;
 
 	if (code == AVC_CTYPE_REJECTED || code == AVC_CTYPE_NOT_IMPLEMENTED)
 		return FALSE;
 
+	if (pdu->pdu_id == AVRCP_REGISTER_NOTIFICATION)
+		volume = pdu->params[1] & 0x7F;
+	else
+		volume = pdu->params[0] & 0x7F;
+
 	if (player->cb->set_volume != NULL)
-		player->cb->set_volume(abs_volume, player->dev, player->user_data);
+		player->cb->set_volume(volume, player->dev, player->user_data);
 
-	return TRUE;
+	return pdu->pdu_id == AVRCP_REGISTER_NOTIFICATION ? TRUE : FALSE;
 }
 
 static void register_volume_notification(struct avrcp_player *player)
@@ -1404,3 +1410,36 @@ void avrcp_unregister_player(struct avrcp_player *player)
 
 	player_destroy(player);
 }
+
+int avrcp_set_volume(struct audio_device *dev, uint8_t volume)
+{
+	struct avrcp_server *server;
+	struct avrcp_player *player;
+	uint8_t buf[AVRCP_HEADER_LENGTH + 1];
+	struct avrcp_header *pdu = (void *) buf;
+
+	server = find_server(servers, &dev->src);
+	if (server == NULL)
+		return -EINVAL;
+
+	player = server->active_player;
+	if (player == NULL)
+		return -ENOTSUP;
+
+	if (player->session == NULL)
+		return -ENOTCONN;
+
+	memset(buf, 0, sizeof(buf));
+
+	set_company_id(pdu->company_id, IEEEID_BTSIG);
+
+	pdu->pdu_id = AVRCP_SET_ABSOLUTE_VOLUME;
+	pdu->params[0] = volume;
+	pdu->params_len = htons(1);
+
+	DBG("volume=%u", volume);
+
+	return avctp_send_vendordep_req(player->session, AVC_CTYPE_CHANGED,
+					AVC_SUBUNIT_PANEL, buf, sizeof(buf),
+					avrcp_handle_volume_changed, player);
+}
diff --git a/audio/avrcp.h b/audio/avrcp.h
index b520ef6..bf11a6c 100644
--- a/audio/avrcp.h
+++ b/audio/avrcp.h
@@ -93,6 +93,7 @@ void avrcp_unregister(const bdaddr_t *src);
 
 gboolean avrcp_connect(struct audio_device *dev);
 void avrcp_disconnect(struct audio_device *dev);
+int avrcp_set_volume(struct audio_device *dev, uint8_t volume);
 
 struct avrcp_player *avrcp_register_player(const bdaddr_t *src,
 						struct avrcp_player_cb *cb,
@@ -102,4 +103,5 @@ void avrcp_unregister_player(struct avrcp_player *player);
 
 int avrcp_player_event(struct avrcp_player *player, uint8_t id, void *data);
 
+
 size_t avrcp_handle_vendor_reject(uint8_t *code, uint8_t *operands);
diff --git a/audio/transport.c b/audio/transport.c
index 6ed5d21..6406ec1 100644
--- a/audio/transport.c
+++ b/audio/transport.c
@@ -43,6 +43,7 @@
 #include "a2dp.h"
 #include "headset.h"
 #include "gateway.h"
+#include "avrcp.h"
 
 #ifndef DBUS_TYPE_UNIX_FD
 #define DBUS_TYPE_UNIX_FD -1
@@ -77,7 +78,7 @@ struct media_transport {
 	uint16_t		omtu;		/* Transport output mtu */
 	uint16_t		delay;		/* Transport delay (a2dp only) */
 	unsigned int		nrec_id;	/* Transport nrec watch (headset only) */
-	uint8_t			volume;		/* Transport volume */
+	uint16_t		volume;		/* Transport volume */
 	gboolean		read_lock;
 	gboolean		write_lock;
 	gboolean		in_use;
@@ -753,6 +754,23 @@ static int set_property_a2dp(struct media_transport *transport,
 
 		/* FIXME: send new delay */
 		return 0;
+	} else if (g_strcmp0(property, "Volume") == 0) {
+		uint16_t volume;
+
+		if (dbus_message_iter_get_arg_type(value) != DBUS_TYPE_UINT16)
+			return -EINVAL;
+
+		dbus_message_iter_get_basic(value, &volume);
+
+		if (volume > 127)
+			return -EINVAL;
+
+		if (transport->volume == volume)
+			return 0;
+
+		transport->volume = volume;
+
+		return avrcp_set_volume(transport->device, volume);
 	}
 
 	return -EINVAL;
-- 
1.7.7.6

[PATCH BlueZ 6/6] audio: Add Volume property to A2DP transport GetProperties

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Now that volume is being handled by SetProperty it should also be
available in GetProperties.
---
 audio/transport.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/audio/transport.c b/audio/transport.c
index 6406ec1..4f86a8b 100644
--- a/audio/transport.c
+++ b/audio/transport.c
@@ -860,6 +860,10 @@ static void get_properties_a2dp(struct media_transport *transport,
 						DBusMessageIter *dict)
 {
 	dict_append_entry(dict, "Delay", DBUS_TYPE_UINT16, &transport->delay);
+
+	if (transport->volume <= 127)
+		dict_append_entry(dict, "Volume", DBUS_TYPE_UINT16,
+							&transport->volume);
 }
 
 static void get_properties_headset(struct media_transport *transport,
@@ -1015,6 +1019,7 @@ struct media_transport *media_transport_create(DBusConnection *conn,
 	transport->size = size;
 	transport->path = g_strdup_printf("%s/fd%d", device->path, fd++);
 	transport->fd = -1;
+	transport->volume = -1;
 
 	uuid = media_endpoint_get_uuid(endpoint);
 	if (strcasecmp(uuid, A2DP_SOURCE_UUID) == 0 ||
-- 
1.7.7.6

Re: [PATCH BlueZ 4/6] audio: Fix signature type for transport Volume

[Lucas De Marchi]

On Thu, May 24, 2012 at 10:22 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> Signature is now uint16 instead of byte
> ---
>  audio/transport.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/audio/transport.c b/audio/transport.c
> index 4ad8608..6ed5d21 100644
> --- a/audio/transport.c
> +++ b/audio/transport.c
> @@ -1076,5 +1076,5 @@ void media_transport_update_volume(struct media_transport *transport,
>
>        emit_property_changed(transport->conn, transport->path,
>                                MEDIA_TRANSPORT_INTERFACE, "Volume",
> -                               DBUS_TYPE_BYTE, &transport->volume);
> +                               DBUS_TYPE_UINT16, &transport->volume);

If you are changing it to uint16_t, why are you saying it's a "fix"?

You should change transport->volume type together with this patch as well.


Lucas De Marchi

Re: [PATCH BlueZ 5/6] AVRCP: Add support for sending SetAbsoluteVolume

[Lucas De Marchi]

On Thu, May 24, 2012 at 10:22 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> Once the transport volume is changed update the remote volume by sending
> SetAbsoluteVolume:
>
> < AVCTP: Command : pt 0x00 transaction 9 pid 0x110e
>    AV/C: Changed: address 0x48 opcode 0x00
>      Subunit: Panel
>      Opcode: Vendor Dependent
>      Company ID: 0x001958
>      AVRCP: SetAbsoluteVolume: pt Single len 0x0001
>        Volume: 100.00% (127/127)
>> AVCTP: Response : pt 0x00 transaction 9 pid 0x110e
>    AV/C: Accepted: address 0x48 opcode 0x00
>      Subunit: Panel
>      Opcode: Vendor Dependent
>      Company ID: 0x001958
>      AVRCP: SetAbsoluteVolume: pt Single len 0x0001
>        Volume: 100.00% (127/127)
> ---
>  audio/avrcp.c     |   45 ++++++++++++++++++++++++++++++++++++++++++---
>  audio/avrcp.h     |    2 ++
>  audio/transport.c |   20 +++++++++++++++++++-
>  3 files changed, 63 insertions(+), 4 deletions(-)
>
> diff --git a/audio/avrcp.c b/audio/avrcp.c
> index b09fab0..eb5241f 100644
> --- a/audio/avrcp.c
> +++ b/audio/avrcp.c
> @@ -87,6 +87,7 @@
>  #define AVRCP_REGISTER_NOTIFICATION    0x31
>  #define AVRCP_REQUEST_CONTINUING       0x40
>  #define AVRCP_ABORT_CONTINUING         0x41
> +#define AVRCP_SET_ABSOLUTE_VOLUME      0x50
>
>  /* Capabilities for AVRCP_GET_CAPABILITIES pdu */
>  #define CAP_COMPANY_ID         0x02
> @@ -1145,15 +1146,20 @@ static gboolean avrcp_handle_volume_changed(struct avctp *session,
>  {
>        struct avrcp_player *player = user_data;
>        struct avrcp_header *pdu = (void *) operands;
> -       uint8_t abs_volume = pdu->params[1] & 0x7F;
> +       uint8_t volume = pdu->params[1] & 0x7F;

Unneeded initialization.

>
>        if (code == AVC_CTYPE_REJECTED || code == AVC_CTYPE_NOT_IMPLEMENTED)
>                return FALSE;
>
> +       if (pdu->pdu_id == AVRCP_REGISTER_NOTIFICATION)
> +               volume = pdu->params[1] & 0x7F;
> +       else
> +               volume = pdu->params[0] & 0x7F;
> +
>        if (player->cb->set_volume != NULL)
> -               player->cb->set_volume(abs_volume, player->dev, player->user_data);
> +               player->cb->set_volume(volume, player->dev, player->user_data);
>
> -       return TRUE;
> +       return pdu->pdu_id == AVRCP_REGISTER_NOTIFICATION ? TRUE : FALSE;

I don't think tweaking this function to handle both callbacks as you
did is a good idea. One of the reasons is something is already missing
in this function: when you receive a "changed notification" you need
to re-register it for receiving subsequent notifications... i.e. in
this function you need to call register_volume_notification() again.
See the end of avrcp_player_event() function and section 6.7.2 of
AVRCP 1.4 spec:

"A registered notification gets changed on receiving CHANGED event
notification. For a new notification additional NOTIFY command is
expected to be sent. Only one EventID shall be used per notification
registration."

>  }
>
>  static void register_volume_notification(struct avrcp_player *player)
> @@ -1404,3 +1410,36 @@ void avrcp_unregister_player(struct avrcp_player *player)
>
>        player_destroy(player);
>  }
> +
> +int avrcp_set_volume(struct audio_device *dev, uint8_t volume)
> +{
> +       struct avrcp_server *server;
> +       struct avrcp_player *player;
> +       uint8_t buf[AVRCP_HEADER_LENGTH + 1];
> +       struct avrcp_header *pdu = (void *) buf;
> +
> +       server = find_server(servers, &dev->src);
> +       if (server == NULL)
> +               return -EINVAL;
> +
> +       player = server->active_player;
> +       if (player == NULL)
> +               return -ENOTSUP;
> +
> +       if (player->session == NULL)
> +               return -ENOTCONN;
> +
> +       memset(buf, 0, sizeof(buf));
> +
> +       set_company_id(pdu->company_id, IEEEID_BTSIG);
> +
> +       pdu->pdu_id = AVRCP_SET_ABSOLUTE_VOLUME;
> +       pdu->params[0] = volume;
> +       pdu->params_len = htons(1);
> +
> +       DBG("volume=%u", volume);
> +
> +       return avctp_send_vendordep_req(player->session, AVC_CTYPE_CHANGED,

wrong CTYPE... it should be CONTROL

> +                                       AVC_SUBUNIT_PANEL, buf, sizeof(buf),
> +                                       avrcp_handle_volume_changed, player);
> +}
> diff --git a/audio/avrcp.h b/audio/avrcp.h
> index b520ef6..bf11a6c 100644
> --- a/audio/avrcp.h
> +++ b/audio/avrcp.h
> @@ -93,6 +93,7 @@ void avrcp_unregister(const bdaddr_t *src);
>
>  gboolean avrcp_connect(struct audio_device *dev);
>  void avrcp_disconnect(struct audio_device *dev);
> +int avrcp_set_volume(struct audio_device *dev, uint8_t volume);
>
>  struct avrcp_player *avrcp_register_player(const bdaddr_t *src,
>                                                struct avrcp_player_cb *cb,
> @@ -102,4 +103,5 @@ void avrcp_unregister_player(struct avrcp_player *player);
>
>  int avrcp_player_event(struct avrcp_player *player, uint8_t id, void *data);
>
> +

extra newline.

>  size_t avrcp_handle_vendor_reject(uint8_t *code, uint8_t *operands);
> diff --git a/audio/transport.c b/audio/transport.c
> index 6ed5d21..6406ec1 100644
> --- a/audio/transport.c
> +++ b/audio/transport.c
> @@ -43,6 +43,7 @@
>  #include "a2dp.h"
>  #include "headset.h"
>  #include "gateway.h"
> +#include "avrcp.h"
>
>  #ifndef DBUS_TYPE_UNIX_FD
>  #define DBUS_TYPE_UNIX_FD -1
> @@ -77,7 +78,7 @@ struct media_transport {
>        uint16_t                omtu;           /* Transport output mtu */
>        uint16_t                delay;          /* Transport delay (a2dp only) */
>        unsigned int            nrec_id;        /* Transport nrec watch (headset only) */
> -       uint8_t                 volume;         /* Transport volume */
> +       uint16_t                volume;         /* Transport volume */

This should be in the previous patch, in which you changed the type.

>        gboolean                read_lock;
>        gboolean                write_lock;
>        gboolean                in_use;
> @@ -753,6 +754,23 @@ static int set_property_a2dp(struct media_transport *transport,
>
>                /* FIXME: send new delay */
>                return 0;
> +       } else if (g_strcmp0(property, "Volume") == 0) {
> +               uint16_t volume;
> +
> +               if (dbus_message_iter_get_arg_type(value) != DBUS_TYPE_UINT16)
> +                       return -EINVAL;
> +
> +               dbus_message_iter_get_basic(value, &volume);
> +
> +               if (volume > 127)
> +                       return -EINVAL;
> +
> +               if (transport->volume == volume)
> +                       return 0;
> +
> +               transport->volume = volume;
> +
> +               return avrcp_set_volume(transport->device, volume);
>        }
>

Regards,
Lucas De Marchi

Re: [PATCH BlueZ 6/6] audio: Add Volume property to A2DP transport GetProperties

[Lucas De Marchi]

On Thu, May 24, 2012 at 10:22 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> Now that volume is being handled by SetProperty it should also be
> available in GetProperties.
> ---
>  audio/transport.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/audio/transport.c b/audio/transport.c
> index 6406ec1..4f86a8b 100644
> --- a/audio/transport.c
> +++ b/audio/transport.c
> @@ -860,6 +860,10 @@ static void get_properties_a2dp(struct media_transport *transport,
>                                                DBusMessageIter *dict)
>  {
>        dict_append_entry(dict, "Delay", DBUS_TYPE_UINT16, &transport->delay);
> +
> +       if (transport->volume <= 127)
> +               dict_append_entry(dict, "Volume", DBUS_TYPE_UINT16,
> +                                                       &transport->volume);
>  }
>
>  static void get_properties_headset(struct media_transport *transport,
> @@ -1015,6 +1019,7 @@ struct media_transport *media_transport_create(DBusConnection *conn,
>        transport->size = size;
>        transport->path = g_strdup_printf("%s/fd%d", device->path, fd++);
>        transport->fd = -1;
> +       transport->volume = -1;

Ack


Lucas De Marchi

Re: [PATCH BlueZ 1/6] AVCTP: Fix setting wrong transaction id expected for responses

[Johan Hedberg]

Hi Luiz,

On Thu, May 24, 2012, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> The id were incremented after being set to the request so it is always
> +1 of the actual transaction.
> ---
>  audio/avctp.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)

Patches 1-3 have been applied. For the rest I'm waiting for a v2 based
on the comments that they received.

Johan

Re: [PATCH BlueZ 4/6] audio: Fix signature type for transport Volume

[Luiz Augusto von Dentz]

Hi Lucas,

On Thu, May 24, 2012 at 7:50 PM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> On Thu, May 24, 2012 at 10:22 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>
>> Signature is now uint16 instead of byte
>> ---
>>  audio/transport.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/audio/transport.c b/audio/transport.c
>> index 4ad8608..6ed5d21 100644
>> --- a/audio/transport.c
>> +++ b/audio/transport.c
>> @@ -1076,5 +1076,5 @@ void media_transport_update_volume(struct media_transport *transport,
>>
>>        emit_property_changed(transport->conn, transport->path,
>>                                MEDIA_TRANSPORT_INTERFACE, "Volume",
>> -                               DBUS_TYPE_BYTE, &transport->volume);
>> +                               DBUS_TYPE_UINT16, &transport->volume);
>
> If you are changing it to uint16_t, why are you saying it's a "fix"?
>
> You should change transport->volume type together with this patch as well.

Its a fix because this was first introduced and byte type, but you are
right about uint16_t should be done in this patch.


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 5/6] AVRCP: Add support for sending SetAbsoluteVolume

[Luiz Augusto von Dentz]

Hi Lucas,

On Thu, May 24, 2012 at 8:08 PM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
>>        if (code == AVC_CTYPE_REJECTED || code == AVC_CTYPE_NOT_IMPLEMENTED)
>>                return FALSE;
>>
>> +       if (pdu->pdu_id == AVRCP_REGISTER_NOTIFICATION)
>> +               volume = pdu->params[1] & 0x7F;
>> +       else
>> +               volume = pdu->params[0] & 0x7F;
>> +
>>        if (player->cb->set_volume != NULL)
>> -               player->cb->set_volume(abs_volume, player->dev, player->user_data);
>> +               player->cb->set_volume(volume, player->dev, player->user_data);
>>
>> -       return TRUE;
>> +       return pdu->pdu_id == AVRCP_REGISTER_NOTIFICATION ? TRUE : FALSE;
>
> I don't think tweaking this function to handle both callbacks as you
> did is a good idea. One of the reasons is something is already missing
> in this function: when you receive a "changed notification" you need
> to re-register it for receiving subsequent notifications... i.e. in
> this function you need to call register_volume_notification() again.
> See the end of avrcp_player_event() function and section 6.7.2 of
> AVRCP 1.4 spec:
>
> "A registered notification gets changed on receiving CHANGED event
> notification. For a new notification additional NOTIFY command is
> expected to be sent. Only one EventID shall be used per notification
> registration."

Yep, spec authors always surprises me, apparently they choose racy
design of one shot notification instead of normal
subscribe/unsubscribe mechanism.

>>  }
>>
>>  static void register_volume_notification(struct avrcp_player *player)
>> @@ -1404,3 +1410,36 @@ void avrcp_unregister_player(struct avrcp_player *player)
>>
>>        player_destroy(player);
>>  }
>> +
>> +int avrcp_set_volume(struct audio_device *dev, uint8_t volume)
>> +{
>> +       struct avrcp_server *server;
>> +       struct avrcp_player *player;
>> +       uint8_t buf[AVRCP_HEADER_LENGTH + 1];
>> +       struct avrcp_header *pdu = (void *) buf;
>> +
>> +       server = find_server(servers, &dev->src);
>> +       if (server == NULL)
>> +               return -EINVAL;
>> +
>> +       player = server->active_player;
>> +       if (player == NULL)
>> +               return -ENOTSUP;
>> +
>> +       if (player->session == NULL)
>> +               return -ENOTCONN;
>> +
>> +       memset(buf, 0, sizeof(buf));
>> +
>> +       set_company_id(pdu->company_id, IEEEID_BTSIG);
>> +
>> +       pdu->pdu_id = AVRCP_SET_ABSOLUTE_VOLUME;
>> +       pdu->params[0] = volume;
>> +       pdu->params_len = htons(1);
>> +
>> +       DBG("volume=%u", volume);
>> +
>> +       return avctp_send_vendordep_req(player->session, AVC_CTYPE_CHANGED,
>
> wrong CTYPE... it should be CONTROL

Gonna fix it.



-- 
Luiz Augusto von Dentz