From: Luiz Augusto von Dentz <[email protected]>
Response may be rejected or not implemented so the operand used to
initialize the variable may not even exist.
---
audio/avrcp.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/audio/avrcp.c b/audio/avrcp.c
index 87a785b..2f96f27 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -1145,12 +1145,14 @@ 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;
if (code == AVC_CTYPE_REJECTED || code == AVC_CTYPE_NOT_IMPLEMENTED)
return FALSE;
- player->cb->set_volume(abs_volume, player->dev, player->user_data);
+ volume = pdu->params[1] & 0x7F;
+
+ player->cb->set_volume(volume, player->dev, player->user_data);
return TRUE;
}
--
1.7.7.6
On Fri, May 25, 2012 at 12:02 PM, Luiz Augusto von Dentz
<[email protected]> wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> 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 ? ? | ? 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> ?audio/avrcp.h ? ? | ? ?2 ++
> ?audio/transport.c | ? 16 ++++++++++++++++
> ?3 files changed, 71 insertions(+), 0 deletions(-)
>
> diff --git a/audio/avrcp.c b/audio/avrcp.c
> index 30d696a..b7be9e2 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
> @@ -1412,3 +1413,55 @@ void avrcp_unregister_player(struct avrcp_player *player)
>
> ? ? ? ?player_destroy(player);
> ?}
> +
> +static gboolean avrcp_handle_set_volume(struct avctp *session,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? uint8_t code, uint8_t subunit,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? uint8_t *operands, size_t operand_count,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? void *user_data)
> +{
> + ? ? ? struct avrcp_player *player = user_data;
> + ? ? ? struct avrcp_header *pdu = (void *) operands;
> + ? ? ? uint8_t volume;
> +
> + ? ? ? if (code == AVC_CTYPE_REJECTED || code == AVC_CTYPE_NOT_IMPLEMENTED)
> + ? ? ? ? ? ? ? return FALSE;
> +
> + ? ? ? volume = pdu->params[0] & 0x7F;
> +
> + ? ? ? player->cb->set_volume(volume, player->dev, player->user_data);
> +
> + ? ? ? return FALSE;
> +}
> +
> +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_CONTROL,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? AVC_SUBUNIT_PANEL, buf, sizeof(buf),
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? avrcp_handle_set_volume, 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 ac9d358..bf86390 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
> @@ -753,6 +754,21 @@ 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;
> +
> + ? ? ? ? ? ? ? return avrcp_set_volume(transport->device, volume);
How do you handle the case in which we are the sink? That is...
instead of sending a "set-volume" CONTROL we should respond to a
possible registered notification?
As far as I can see, this is not handled yet, right?
Lucas De Marchi
Hi Lucas,
On Fri, May 25, 2012 at 7:17 PM, Lucas De Marchi
<[email protected]> wrote:
> On Fri, May 25, 2012 at 12:02 PM, Luiz Augusto von Dentz
> <[email protected]> wrote:
>> From: Luiz Augusto von Dentz <[email protected]>
>>
>> The spec says:
>>
>> "A registered notification gets changed on receiving CHANGED event
>> notification. For a new notification additional NOTIFY command is
>> expected to be sent."
>> ---
>> ?audio/avrcp.c | ? ?7 +++++++
>> ?1 files changed, 7 insertions(+), 0 deletions(-)
>>
>> diff --git a/audio/avrcp.c b/audio/avrcp.c
>> index 2f96f27..30d696a 100644
>> --- a/audio/avrcp.c
>> +++ b/audio/avrcp.c
>> @@ -176,6 +176,8 @@ static uint32_t company_ids[] = {
>> ? ? ? ?IEEEID_BTSIG,
>> ?};
>>
>> +static void register_volume_notification(struct avrcp_player *player);
>> +
>> ?static sdp_record_t *avrcp_ct_record(void)
>> ?{
>> ? ? ? ?sdp_list_t *svclass_id, *pfseq, *apseq, *root;
>> @@ -1154,6 +1156,11 @@ static gboolean avrcp_handle_volume_changed(struct avctp *session,
>>
>> ? ? ? ?player->cb->set_volume(volume, player->dev, player->user_data);
>>
>> + ? ? ? if (code == AVC_CTYPE_CHANGED) {
>
> if code != AVC_CTYPE_CHANGED you shouldn't even set the volume
> above... you need to return early if code is not what it's expecting
> because you can't trust the other side.
I suppose we can do the other way round, just return early if the
response type is not changed or interim. Btw, the purpose of the check
above was for not trigger registration on interim response since that
is already a registration response and we should keep the handler
around in that case for changed response.
--
Luiz Augusto von Dentz
Hi Luiz,
On Fri, May 25, 2012, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> Response may be rejected or not implemented so the operand used to
> initialize the variable may not even exist.
> ---
> audio/avrcp.c | 6 ++++--
> 1 files changed, 4 insertions(+), 2 deletions(-)
All of these patches except 2/6 (since Lucas had some feedback on it)
have been applied. Thanks.
Johan
On Fri, May 25, 2012 at 12:02 PM, Luiz Augusto von Dentz
<[email protected]> wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> Response may be rejected or not implemented so the operand used to
> initialize the variable may not even exist.
> ---
> ?audio/avrcp.c | ? ?6 ++++--
> ?1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/audio/avrcp.c b/audio/avrcp.c
> index 87a785b..2f96f27 100644
> --- a/audio/avrcp.c
> +++ b/audio/avrcp.c
> @@ -1145,12 +1145,14 @@ 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;
>
> ? ? ? ?if (code == AVC_CTYPE_REJECTED || code == AVC_CTYPE_NOT_IMPLEMENTED)
> ? ? ? ? ? ? ? ?return FALSE;
>
> - ? ? ? player->cb->set_volume(abs_volume, player->dev, player->user_data);
> + ? ? ? volume = pdu->params[1] & 0x7F;
> +
> + ? ? ? player->cb->set_volume(volume, player->dev, player->user_data);
Ack.
Lucas De Marchi
On Fri, May 25, 2012 at 12:02 PM, Luiz Augusto von Dentz
<[email protected]> wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> The spec says:
>
> "A registered notification gets changed on receiving CHANGED event
> notification. For a new notification additional NOTIFY command is
> expected to be sent."
> ---
> ?audio/avrcp.c | ? ?7 +++++++
> ?1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/audio/avrcp.c b/audio/avrcp.c
> index 2f96f27..30d696a 100644
> --- a/audio/avrcp.c
> +++ b/audio/avrcp.c
> @@ -176,6 +176,8 @@ static uint32_t company_ids[] = {
> ? ? ? ?IEEEID_BTSIG,
> ?};
>
> +static void register_volume_notification(struct avrcp_player *player);
> +
> ?static sdp_record_t *avrcp_ct_record(void)
> ?{
> ? ? ? ?sdp_list_t *svclass_id, *pfseq, *apseq, *root;
> @@ -1154,6 +1156,11 @@ static gboolean avrcp_handle_volume_changed(struct avctp *session,
>
> ? ? ? ?player->cb->set_volume(volume, player->dev, player->user_data);
>
> + ? ? ? if (code == AVC_CTYPE_CHANGED) {
if code != AVC_CTYPE_CHANGED you shouldn't even set the volume
above... you need to return early if code is not what it's expecting
because you can't trust the other side.
Lucas De Marchi
From: Luiz Augusto von Dentz <[email protected]>
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 bf86390..4273282 100644
--- a/audio/transport.c
+++ b/audio/transport.c
@@ -858,6 +858,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,
@@ -1013,6 +1017,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
From: Luiz Augusto von Dentz <[email protected]>
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 | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
audio/avrcp.h | 2 ++
audio/transport.c | 16 ++++++++++++++++
3 files changed, 71 insertions(+), 0 deletions(-)
diff --git a/audio/avrcp.c b/audio/avrcp.c
index 30d696a..b7be9e2 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
@@ -1412,3 +1413,55 @@ void avrcp_unregister_player(struct avrcp_player *player)
player_destroy(player);
}
+
+static gboolean avrcp_handle_set_volume(struct avctp *session,
+ uint8_t code, uint8_t subunit,
+ uint8_t *operands, size_t operand_count,
+ void *user_data)
+{
+ struct avrcp_player *player = user_data;
+ struct avrcp_header *pdu = (void *) operands;
+ uint8_t volume;
+
+ if (code == AVC_CTYPE_REJECTED || code == AVC_CTYPE_NOT_IMPLEMENTED)
+ return FALSE;
+
+ volume = pdu->params[0] & 0x7F;
+
+ player->cb->set_volume(volume, player->dev, player->user_data);
+
+ return FALSE;
+}
+
+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_CONTROL,
+ AVC_SUBUNIT_PANEL, buf, sizeof(buf),
+ avrcp_handle_set_volume, 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 ac9d358..bf86390 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
@@ -753,6 +754,21 @@ 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;
+
+ return avrcp_set_volume(transport->device, volume);
}
return -EINVAL;
--
1.7.7.6
From: Luiz Augusto von Dentz <[email protected]>
Signature is now uint16 instead of byte
---
audio/transport.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/audio/transport.c b/audio/transport.c
index 4ad8608..ac9d358 100644
--- a/audio/transport.c
+++ b/audio/transport.c
@@ -77,7 +77,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;
@@ -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
From: Luiz Augusto von Dentz <[email protected]>
Volume property is A2DP only
---
audio/media.c | 8 +++-----
1 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/audio/media.c b/audio/media.c
index cb8872f..2a2cf37 100644
--- a/audio/media.c
+++ b/audio/media.c
@@ -1357,16 +1357,14 @@ static void set_volume(uint8_t volume, struct audio_device *dev, void *user_data
mp->volume = volume;
for (l = mp->adapter->endpoints; l; l = l->next) {
-
- struct media_endpoint *endpoint;
+ struct media_endpoint *endpoint = l->data;
struct media_transport *transport;
- if (l->data == NULL)
+ /* Volume is A2DP only */
+ if (endpoint->sep == NULL)
continue;
- endpoint = l->data;
transport = find_device_transport(endpoint, dev);
-
if (transport == NULL)
continue;
--
1.7.7.6
From: Luiz Augusto von Dentz <[email protected]>
The spec says:
"A registered notification gets changed on receiving CHANGED event
notification. For a new notification additional NOTIFY command is
expected to be sent."
---
audio/avrcp.c | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/audio/avrcp.c b/audio/avrcp.c
index 2f96f27..30d696a 100644
--- a/audio/avrcp.c
+++ b/audio/avrcp.c
@@ -176,6 +176,8 @@ static uint32_t company_ids[] = {
IEEEID_BTSIG,
};
+static void register_volume_notification(struct avrcp_player *player);
+
static sdp_record_t *avrcp_ct_record(void)
{
sdp_list_t *svclass_id, *pfseq, *apseq, *root;
@@ -1154,6 +1156,11 @@ static gboolean avrcp_handle_volume_changed(struct avctp *session,
player->cb->set_volume(volume, player->dev, player->user_data);
+ if (code == AVC_CTYPE_CHANGED) {
+ register_volume_notification(player);
+ return FALSE;
+ }
+
return TRUE;
}
--
1.7.7.6
Hi Lucas,
On Thu, May 24, 2012 at 8:08 PM, Lucas De Marchi
<[email protected]> 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
Hi Lucas,
On Thu, May 24, 2012 at 7:50 PM, Lucas De Marchi
<[email protected]> wrote:
> On Thu, May 24, 2012 at 10:22 AM, Luiz Augusto von Dentz
> <[email protected]> wrote:
>> From: Luiz Augusto von Dentz <[email protected]>
>>
>> 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
On Thu, May 24, 2012 at 10:22 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> 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
On Thu, May 24, 2012 at 10:22 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> 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
On Thu, May 24, 2012 at 10:22 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> 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