[PATCH hcidump 1/2] AVRCP: Add parsing for SetAddressedPlayer PDU

[PATCH hcidump 1/2] AVRCP: Add parsing for SetAddressedPlayer PDU

[Luiz Augusto von Dentz]

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

---
 parser/avrcp.c |   35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/parser/avrcp.c b/parser/avrcp.c
index 850741b..0f2f1e6 100644
--- a/parser/avrcp.c
+++ b/parser/avrcp.c
@@ -1222,6 +1222,38 @@ static void avrcp_set_absolute_volume_dump(int level, struct frame *frm,
 	printf("Volume: %.2f%% (%d/127)\n", value/1.27, value);
 }
 
+static void avrcp_set_addressed_player(int level, struct frame *frm,
+						uint8_t ctype, uint16_t len)
+{
+	uint16_t id;
+	uint8_t status;
+
+	p_indent(level, frm);
+
+	if (ctype > AVC_CTYPE_GENERAL_INQUIRY)
+		goto response;
+
+	if (len < 2) {
+		printf("PDU Malformed\n");
+		raw_dump(level, frm);
+		return;
+	}
+
+	id = get_u16(frm);
+	printf("PlayerID: 0x%04x", id);
+	return;
+
+response:
+	if (len < 1) {
+		printf("PDU Malformed\n");
+		raw_dump(level, frm);
+		return;
+	}
+
+	status = get_u8(frm);
+	printf("Status: 0x%02x (%s)\n", status, error2str(status));
+}
+
 static void avrcp_pdu_dump(int level, struct frame *frm, uint8_t ctype)
 {
 	uint8_t pduid, pt;
@@ -1291,6 +1323,9 @@ static void avrcp_pdu_dump(int level, struct frame *frm, uint8_t ctype)
 	case AVRCP_SET_ABSOLUTE_VOLUME:
 		avrcp_set_absolute_volume_dump(level + 1, frm, ctype, len);
 		break;
+	case AVRCP_SET_ADDRESSED_PLAYER:
+		avrcp_set_addressed_player(level + 1, frm, ctype, len);
+		break;
 	default:
 		raw_dump(level, frm);
 	}
-- 
1.7.10.2

[PATCH hcidump 2/2] AVRCP: Add support for Addressed Player Changed event

[Luiz Augusto von Dentz]

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

---
 parser/avrcp.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/parser/avrcp.c b/parser/avrcp.c
index 0f2f1e6..6fa3742 100644
--- a/parser/avrcp.c
+++ b/parser/avrcp.c
@@ -1107,6 +1107,7 @@ static void avrcp_register_notification_dump(int level, struct frame *frm,
 						uint8_t ctype, uint16_t len)
 {
 	uint8_t event, status;
+	uint16_t uid;
 	uint32_t interval;
 	uint64_t id;
 
@@ -1202,6 +1203,15 @@ response:
 		status = get_u8(frm) & 0x7F;
 		printf("Volume: %.2f%% (%d/127)\n", status/1.27, status);
 		break;
+	case AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED:
+		uid = get_u16(frm);
+		printf("PlayerID: 0x%04x", uid);
+
+		p_indent(level, frm);
+
+		uid = get_u16(frm);
+		printf("UID: 0x%04x", uid);
+		break;
 	}
 }
 
-- 
1.7.10.2

RE: [PATCH hcidump 1/2] AVRCP: Add parsing for SetAddressedPlayer PDU

[Michal.Labedzki]

Hi Luiz,

Some cosmetic comments.

> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> ---
>  parser/avrcp.c |   35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/parser/avrcp.c b/parser/avrcp.c
> index 850741b..0f2f1e6 100644
> --- a/parser/avrcp.c
> +++ b/parser/avrcp.c
> @@ -1222,6 +1222,38 @@ static void avrcp_set_absolute_volume_dump(int level, struct frame *frm,
>         printf("Volume: %.2f%% (%d/127)\n", value/1.27, value);
>  }
> 
> +static void avrcp_set_addressed_player(int level, struct frame *frm,
> +                                               uint8_t ctype, uint16_t len)
> +{
> +       uint16_t id;
> +       uint8_t status;
> +
> +       p_indent(level, frm);
> +
> +       if (ctype > AVC_CTYPE_GENERAL_INQUIRY)
> +               goto response;

This command type is only AVC_CTYPE_CONTROL with AVC_CTYPE_NOT_IMPLEMENTED, AVC_CTYPE_ACCEPTED, AVC_CTYPE_REJECTED responses, so maybe all other should be tread as malformed.

> +
> +       if (len < 2) {

What do you think about replace "<" by "!="? Large packet probably is not correct. But this can be what it is.

> +               printf("PDU Malformed\n");
> +               raw_dump(level, frm);
> +               return;
> +       }
> +
> +       id = get_u16(frm);
> +       printf("PlayerID: 0x%04x", id);
> +       return;

ID in hex? Decimal seems to be more natural.

> +
> +response:
> +       if (len < 1) {

The same. What do you think about replace "<" by "!="?

> +               printf("PDU Malformed\n");
> +               raw_dump(level, frm);
> +               return;
> +       }
> +
> +       status = get_u8(frm);
> +       printf("Status: 0x%02x (%s)\n", status, error2str(status));
> +}
> +
>  static void avrcp_pdu_dump(int level, struct frame *frm, uint8_t ctype)
>  {
>         uint8_t pduid, pt;
> @@ -1291,6 +1323,9 @@ static void avrcp_pdu_dump(int level, struct frame *frm, uint8_t ctype)
>         case AVRCP_SET_ABSOLUTE_VOLUME:
>                 avrcp_set_absolute_volume_dump(level + 1, frm, ctype, len);
>                 break;
> +       case AVRCP_SET_ADDRESSED_PLAYER:
> +               avrcp_set_addressed_player(level + 1, frm, ctype, len);
> +               break;
>         default:
>                 raw_dump(level, frm);
>         }
> --
> 1.7.10.2
> 



Regards / Pozdrawiam
-------------------------------------------------------------------------------------------------------------
Michał Łabędzki
ASCII: Michal Labedzki
e-mail: michal.labedzki@tieto.com
office communicator: michal.labedzki@tieto.com
location: Poland, Wrocław, Legnicka 55F
room: 315
phone: +48 717 740 340
---
Tieto Corporation / Tieto Poland
http://www.tieto.com / http://www.tieto.pl
---
Tieto Poland spółka z ograniczoną odpowiedzialnością z siedzibą w Szczecinie, ul. Malczewskiego 26. Zarejestrowana w Sądzie Rejonowym Szczecin-Centrum w Szczecinie, XIII Wydział Gospodarczy Krajowego Rejestru Sądowego pod numerem 0000124858. NIP: 8542085557. REGON: 812023656. Kapitał zakładowy: 4 271500 PLN

RE: [PATCH hcidump 2/2] AVRCP: Add support for Addressed Player Changed event

[Michal.Labedzki]

Hi Luiz, 

One important comment and one cosmetic.

> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> ---
>  parser/avrcp.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/parser/avrcp.c b/parser/avrcp.c
> index 0f2f1e6..6fa3742 100644
> --- a/parser/avrcp.c
> +++ b/parser/avrcp.c
> @@ -1107,6 +1107,7 @@ static void avrcp_register_notification_dump(int level, struct frame *frm,
>                                                  uint8_t ctype, uint16_t len)
>  {
>          uint8_t event, status;
> +       uint16_t uid;
>          uint32_t interval;
>          uint64_t id;
> 
> @@ -1202,6 +1203,15 @@ response:
>                  status = get_u8(frm) & 0x7F;
>                  printf("Volume: %.2f%% (%d/127)\n", status/1.27, status);
>                  break;
> +       case AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED:
> +               uid = get_u16(frm);
> +               printf("PlayerID: 0x%04x", uid);

uid? You can use "id" to replace "uid" in this place.
ID can be decimal.

> +
> +               p_indent(level, frm);
> +
> +               uid = get_u16(frm);
> +               printf("UID: 0x%04x", uid);
> +               break;

This is not "UID". This is "UID Counter".

>          }
>  }
> 
> --
> 1.7.10.2

Regards / Pozdrawiam
-------------------------------------------------------------------------------------------------------------
Michał Łabędzki
ASCII: Michal Labedzki
e-mail: michal.labedzki@tieto.com
office communicator: michal.labedzki@tieto.com
location: Poland, Wrocław, Legnicka 55F
room: 315
phone: +48 717 740 340
---
Tieto Corporation / Tieto Poland
http://www.tieto.com / http://www.tieto.pl
---
Tieto Poland spółka z ograniczoną odpowiedzialnością z siedzibą w Szczecinie, ul. Malczewskiego 26. Zarejestrowana w Sądzie Rejonowym Szczecin-Centrum w Szczecinie, XIII Wydział Gospodarczy Krajowego Rejestru Sądowego pod numerem 0000124858. NIP: 8542085557. REGON: 812023656. Kapitał zakładowy: 4 271500 PLN

Re: [PATCH hcidump 1/2] AVRCP: Add parsing for SetAddressedPlayer PDU

[Luiz Augusto von Dentz]

Hi Michal,

On Tue, Jun 19, 2012 at 9:39 AM,  <Michal.Labedzki@tieto.com> wrote:
> Hi Luiz,
>
> Some cosmetic comments.
>
>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>
>> ---
>>  parser/avrcp.c |   35 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>>
>> diff --git a/parser/avrcp.c b/parser/avrcp.c
>> index 850741b..0f2f1e6 100644
>> --- a/parser/avrcp.c
>> +++ b/parser/avrcp.c
>> @@ -1222,6 +1222,38 @@ static void avrcp_set_absolute_volume_dump(int level, struct frame *frm,
>>         printf("Volume: %.2f%% (%d/127)\n", value/1.27, value);
>>  }
>>
>> +static void avrcp_set_addressed_player(int level, struct frame *frm,
>> +                                               uint8_t ctype, uint16_t len)
>> +{
>> +       uint16_t id;
>> +       uint8_t status;
>> +
>> +       p_indent(level, frm);
>> +
>> +       if (ctype > AVC_CTYPE_GENERAL_INQUIRY)
>> +               goto response;
>
> This command type is only AVC_CTYPE_CONTROL with AVC_CTYPE_NOT_IMPLEMENTED, AVC_CTYPE_ACCEPTED, AVC_CTYPE_REJECTED responses, so maybe all other should be tread as malformed.

This is a dumping tool, so the purpose to print as much as possible.

>
>> +
>> +       if (len < 2) {
>
> What do you think about replace "<" by "!="? Large packet probably is not correct. But this can be what it is.

Again the purpose is to print as much as possible.

>
>> +               printf("PDU Malformed\n");
>> +               raw_dump(level, frm);
>> +               return;
>> +       }
>> +
>> +       id = get_u16(frm);
>> +       printf("PlayerID: 0x%04x", id);
>> +       return;
>
> ID in hex? Decimal seems to be more natural.

This was taken from the spec, but either way is fine.

-- 
Luiz Augusto von Dentz