All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/2] Monitor: Modify design of AVRCP callback functions
@ 2014-09-12 13:16 Vikrampal Yadav
  2014-09-12 13:16 ` [PATCH v2 2/2] Monitor: Add AVRCP GetElementAttributes support Vikrampal Yadav
  2014-09-15  8:51 ` [PATCH v2 1/2] Monitor: Modify design of AVRCP callback functions Luiz Augusto von Dentz
  0 siblings, 2 replies; 7+ messages in thread
From: Vikrampal Yadav @ 2014-09-12 13:16 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: luiz.dentz, d.kasatkin, vikram.pal, cpgs

Modified the design of AVRCP callback functions.
---
 monitor/avctp.c | 103 ++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 66 insertions(+), 37 deletions(-)

diff --git a/monitor/avctp.c b/monitor/avctp.c
index c7e242b..f366b87 100644
--- a/monitor/avctp.c
+++ b/monitor/avctp.c
@@ -158,6 +158,13 @@
 #define AVRCP_ATTRIBUTE_SHUFFLE		0x03
 #define AVRCP_ATTRIBUTE_SCAN		0x04
 
+struct avctp_frame {
+	uint8_t hdr;
+	uint8_t pt;
+	uint16_t pid;
+	struct l2cap_frame l2cap_frame;
+};
+
 static const char *ctype2str(uint8_t ctype)
 {
 	switch (ctype & 0x0f) {
@@ -517,15 +524,19 @@ static const char *charset2str(uint16_t charset)
 	}
 }
 
-static bool avrcp_passthrough_packet(struct l2cap_frame *frame)
+static bool avrcp_passthrough_packet(struct avctp_frame *avctp_frame)
 {
+	struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
+
 	packet_hexdump(frame->data, frame->size);
 	return true;
 }
 
-static bool avrcp_get_capabilities(struct l2cap_frame *frame, uint8_t ctype,
-						uint8_t len, uint8_t indent)
+static bool avrcp_get_capabilities(struct avctp_frame *avctp_frame,
+					uint8_t ctype, uint8_t len,
+					uint8_t indent)
 {
+	struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
 	uint8_t cap, count;
 	int i;
 
@@ -576,10 +587,11 @@ static bool avrcp_get_capabilities(struct l2cap_frame *frame, uint8_t ctype,
 	return true;
 }
 
-static bool avrcp_list_player_attributes(struct l2cap_frame *frame,
+static bool avrcp_list_player_attributes(struct avctp_frame *avctp_frame,
 						uint8_t ctype, uint8_t len,
 						uint8_t indent)
 {
+	struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
 	uint8_t num;
 	int i;
 
@@ -604,9 +616,11 @@ static bool avrcp_list_player_attributes(struct l2cap_frame *frame,
 	return true;
 }
 
-static bool avrcp_list_player_values(struct l2cap_frame *frame, uint8_t ctype,
-						uint8_t len, uint8_t indent)
+static bool avrcp_list_player_values(struct avctp_frame *avctp_frame,
+					uint8_t ctype, uint8_t len,
+					uint8_t indent)
 {
+	struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
 	static uint8_t attr = 0;
 	uint8_t num;
 
@@ -640,10 +654,11 @@ response:
 	return true;
 }
 
-static bool avrcp_get_current_player_value(struct l2cap_frame *frame,
+static bool avrcp_get_current_player_value(struct avctp_frame *avctp_frame,
 						uint8_t ctype, uint8_t len,
 						uint8_t indent)
 {
+	struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
 	uint8_t num;
 
 	if (!l2cap_frame_get_u8(frame, &num))
@@ -688,9 +703,11 @@ response:
 	return true;
 }
 
-static bool avrcp_set_player_value(struct l2cap_frame *frame, uint8_t ctype,
-					uint8_t len, uint8_t indent)
+static bool avrcp_set_player_value(struct avctp_frame *avctp_frame,
+					uint8_t ctype, uint8_t len,
+					uint8_t indent)
 {
+	struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
 	uint8_t num;
 
 	if (ctype > AVC_CTYPE_GENERAL_INQUIRY)
@@ -720,10 +737,11 @@ static bool avrcp_set_player_value(struct l2cap_frame *frame, uint8_t ctype,
 	return true;
 }
 
-static bool avrcp_get_player_attribute_text(struct l2cap_frame *frame,
+static bool avrcp_get_player_attribute_text(struct avctp_frame *avctp_frame,
 						uint8_t ctype, uint8_t len,
 						uint8_t indent)
 {
+	struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
 	uint8_t num;
 
 	if (!l2cap_frame_get_u8(frame, &num))
@@ -783,10 +801,11 @@ response:
 	return true;
 }
 
-static bool avrcp_get_player_value_text(struct l2cap_frame *frame,
+static bool avrcp_get_player_value_text(struct avctp_frame *avctp_frame,
 					uint8_t ctype, uint8_t len,
 					uint8_t indent)
 {
+	struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
 	static uint8_t attr = 0;
 	uint8_t num;
 
@@ -858,9 +877,11 @@ response:
 	return true;
 }
 
-static bool avrcp_displayable_charset(struct l2cap_frame *frame, uint8_t ctype,
-					uint8_t len, uint8_t indent)
+static bool avrcp_displayable_charset(struct avctp_frame *avctp_frame,
+					uint8_t ctype, uint8_t len,
+					uint8_t indent)
 {
+	struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
 	uint8_t num;
 
 	if (ctype > AVC_CTYPE_GENERAL_INQUIRY)
@@ -886,8 +907,8 @@ static bool avrcp_displayable_charset(struct l2cap_frame *frame, uint8_t ctype,
 
 struct avrcp_ctrl_pdu_data {
 	uint8_t pduid;
-	bool (*func) (struct l2cap_frame *frame, uint8_t ctype, uint8_t len,
-							uint8_t indent);
+	bool (*func) (struct avctp_frame *avctp_frame, uint8_t ctype,
+						uint8_t len, uint8_t indent);
 };
 
 static const struct avrcp_ctrl_pdu_data avrcp_ctrl_pdu_table[] = {
@@ -915,10 +936,11 @@ static bool avrcp_rejected_packet(struct l2cap_frame *frame, uint8_t indent)
 	return true;
 }
 
-static bool avrcp_pdu_packet(struct l2cap_frame *frame, uint8_t ctype,
+static bool avrcp_pdu_packet(struct avctp_frame *avctp_frame, uint8_t ctype,
 								uint8_t indent)
 {
-	uint8_t pduid, pt;
+	struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
+	uint8_t pduid;
 	uint16_t len;
 	int i;
 	const struct avrcp_ctrl_pdu_data *ctrl_pdu_data = NULL;
@@ -926,14 +948,14 @@ static bool avrcp_pdu_packet(struct l2cap_frame *frame, uint8_t ctype,
 	if (!l2cap_frame_get_u8(frame, &pduid))
 		return false;
 
-	if (!l2cap_frame_get_u8(frame, &pt))
+	if (!l2cap_frame_get_u8(frame, &avctp_frame->pt))
 		return false;
 
 	if (!l2cap_frame_get_be16(frame, &len))
 		return false;
 
 	print_indent(indent, COLOR_OFF, "AVRCP: ", pdu2str(pduid), COLOR_OFF,
-					" pt %s len 0x%04x", pt2str(pt), len);
+			" pt %s len 0x%04x", pt2str(avctp_frame->pt), len);
 
 	if (frame->size != len)
 		return false;
@@ -953,11 +975,13 @@ static bool avrcp_pdu_packet(struct l2cap_frame *frame, uint8_t ctype,
 		return true;
 	}
 
-	return ctrl_pdu_data->func(frame, ctype, len, indent + 2);
+	return ctrl_pdu_data->func(avctp_frame, ctype, len, indent + 2);
 }
 
-static bool avrcp_control_packet(struct l2cap_frame *frame)
+static bool avrcp_control_packet(struct avctp_frame *avctp_frame)
 {
+	struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
+
 	uint8_t ctype, address, subunit, opcode, company[3], indent = 2;
 
 	if (!l2cap_frame_get_u8(frame, &ctype) ||
@@ -988,7 +1012,7 @@ static bool avrcp_control_packet(struct l2cap_frame *frame)
 
 	switch (opcode) {
 	case 0x7c:
-		return avrcp_passthrough_packet(frame);
+		return avrcp_passthrough_packet(avctp_frame);
 	case 0x00:
 		if (!l2cap_frame_get_u8(frame, &company[0]) ||
 				!l2cap_frame_get_u8(frame, &company[1]) ||
@@ -998,29 +1022,32 @@ static bool avrcp_control_packet(struct l2cap_frame *frame)
 		print_field("%*cCompany ID: 0x%02x%02x%02x", indent, ' ',
 					company[0], company[1], company[2]);
 
-		return avrcp_pdu_packet(frame, ctype, 10);
+		return avrcp_pdu_packet(avctp_frame, ctype, 10);
 	default:
 		packet_hexdump(frame->data, frame->size);
 		return true;
 	}
 }
 
-static bool avrcp_browsing_packet(struct l2cap_frame *frame, uint8_t hdr)
+static bool avrcp_browsing_packet(struct avctp_frame *avctp_frame)
 {
+	struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
+
 	packet_hexdump(frame->data, frame->size);
 	return true;
 }
 
-static void avrcp_packet(struct l2cap_frame *frame, uint8_t hdr)
+static void avrcp_packet(struct avctp_frame *avctp_frame)
 {
+	struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
 	bool ret;
 
 	switch (frame->psm) {
 	case 0x17:
-		ret = avrcp_control_packet(frame);
+		ret = avrcp_control_packet(avctp_frame);
 		break;
 	case 0x1B:
-		ret = avrcp_browsing_packet(frame, hdr);
+		ret = avrcp_browsing_packet(avctp_frame);
 		break;
 	default:
 		packet_hexdump(frame->data, frame->size);
@@ -1035,15 +1062,16 @@ static void avrcp_packet(struct l2cap_frame *frame, uint8_t hdr)
 
 void avctp_packet(const struct l2cap_frame *frame)
 {
-	uint8_t hdr;
-	uint16_t pid;
-	struct l2cap_frame avctp_frame;
+	struct l2cap_frame *l2cap_frame;
+	struct avctp_frame avctp_frame;
 	const char *pdu_color;
 
-	l2cap_frame_pull(&avctp_frame, frame, 0);
+	l2cap_frame_pull(&avctp_frame.l2cap_frame, frame, 0);
+
+	l2cap_frame = &avctp_frame.l2cap_frame;
 
-	if (!l2cap_frame_get_u8(&avctp_frame, &hdr) ||
-				!l2cap_frame_get_be16(&avctp_frame, &pid)) {
+	if (!l2cap_frame_get_u8(l2cap_frame, &avctp_frame.hdr) ||
+			!l2cap_frame_get_be16(l2cap_frame, &avctp_frame.pid)) {
 		print_text(COLOR_ERROR, "frame too short");
 		packet_hexdump(frame->data, frame->size);
 		return;
@@ -1057,11 +1085,12 @@ void avctp_packet(const struct l2cap_frame *frame)
 	print_indent(6, pdu_color, "AVCTP", "", COLOR_OFF,
 				" %s: %s: type 0x%02x label %d PID 0x%04x",
 				frame->psm == 23 ? "Control" : "Browsing",
-				hdr & 0x02 ? "Response" : "Command",
-				hdr & 0x0c, hdr >> 4, pid);
+				avctp_frame.hdr & 0x02 ? "Response" : "Command",
+				avctp_frame.hdr & 0x0c, avctp_frame.hdr >> 4,
+				avctp_frame.pid);
 
-	if (pid == 0x110e || pid == 0x110c)
-		avrcp_packet(&avctp_frame, hdr);
+	if (avctp_frame.pid == 0x110e || avctp_frame.pid == 0x110c)
+		avrcp_packet(&avctp_frame);
 	else
 		packet_hexdump(frame->data, frame->size);
 }
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 2/2] Monitor: Add AVRCP GetElementAttributes support
  2014-09-12 13:16 [PATCH v2 1/2] Monitor: Modify design of AVRCP callback functions Vikrampal Yadav
@ 2014-09-12 13:16 ` Vikrampal Yadav
  2014-09-15  8:30   ` Luiz Augusto von Dentz
  2014-09-15  8:51 ` [PATCH v2 1/2] Monitor: Modify design of AVRCP callback functions Luiz Augusto von Dentz
  1 sibling, 1 reply; 7+ messages in thread
From: Vikrampal Yadav @ 2014-09-12 13:16 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: luiz.dentz, d.kasatkin, vikram.pal, cpgs

Support for decoding AVRCP GetElementAttributes added in Bluetooth
monitor.
---
 monitor/avctp.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 157 insertions(+)

diff --git a/monitor/avctp.c b/monitor/avctp.c
index f366b87..89997d0 100644
--- a/monitor/avctp.c
+++ b/monitor/avctp.c
@@ -158,6 +158,16 @@
 #define AVRCP_ATTRIBUTE_SHUFFLE		0x03
 #define AVRCP_ATTRIBUTE_SCAN		0x04
 
+/* media attributes */
+#define AVRCP_MEDIA_ATTRIBUTE_ILLEGAL	0x00
+#define AVRCP_MEDIA_ATTRIBUTE_TITLE	0x01
+#define AVRCP_MEDIA_ATTRIBUTE_ARTIST	0x02
+#define AVRCP_MEDIA_ATTRIBUTE_ALBUM	0x03
+#define AVRCP_MEDIA_ATTRIBUTE_TRACK	0x04
+#define AVRCP_MEDIA_ATTRIBUTE_TOTAL	0x05
+#define AVRCP_MEDIA_ATTRIBUTE_GENRE	0x06
+#define AVRCP_MEDIA_ATTRIBUTE_DURATION	0x07
+
 struct avctp_frame {
 	uint8_t hdr;
 	uint8_t pt;
@@ -165,6 +175,11 @@ struct avctp_frame {
 	struct l2cap_frame l2cap_frame;
 };
 
+static struct avrcp_continuing {
+	uint16_t num;
+	uint16_t size;
+} avrcp_continuing;
+
 static const char *ctype2str(uint8_t ctype)
 {
 	switch (ctype & 0x0f) {
@@ -524,6 +539,30 @@ static const char *charset2str(uint16_t charset)
 	}
 }
 
+static const char *mediattr2str(uint32_t attr)
+{
+	switch (attr) {
+	case AVRCP_MEDIA_ATTRIBUTE_ILLEGAL:
+		return "Illegal";
+	case AVRCP_MEDIA_ATTRIBUTE_TITLE:
+		return "Title";
+	case AVRCP_MEDIA_ATTRIBUTE_ARTIST:
+		return "Artist";
+	case AVRCP_MEDIA_ATTRIBUTE_ALBUM:
+		return "Album";
+	case AVRCP_MEDIA_ATTRIBUTE_TRACK:
+		return "Track";
+	case AVRCP_MEDIA_ATTRIBUTE_TOTAL:
+		return "Track Total";
+	case AVRCP_MEDIA_ATTRIBUTE_GENRE:
+		return "Genre";
+	case AVRCP_MEDIA_ATTRIBUTE_DURATION:
+		return "Track duration";
+	default:
+		return "Reserved";
+	}
+}
+
 static bool avrcp_passthrough_packet(struct avctp_frame *avctp_frame)
 {
 	struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
@@ -905,6 +944,123 @@ static bool avrcp_displayable_charset(struct avctp_frame *avctp_frame,
 	return true;
 }
 
+static bool avrcp_get_element_attributes(struct avctp_frame *avctp_frame,
+						uint8_t ctype, uint8_t len,
+						uint8_t indent)
+{
+	struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
+	uint64_t id;
+	uint8_t num;
+
+	if (ctype > AVC_CTYPE_GENERAL_INQUIRY)
+		goto response;
+
+	if (!l2cap_frame_get_be64(frame, &id))
+		return false;
+
+	print_field("%*cIdentifier: 0x%jx (%s)", (indent - 8), ' ',
+					id, id ? "Reserved" : "PLAYING");
+
+	if (!l2cap_frame_get_u8(frame, &num))
+		return false;
+
+	print_field("%*cAttributeCount: 0x%02x", (indent - 8), ' ', num);
+
+	for (; num > 0; num--) {
+		uint32_t attr;
+
+		if (!l2cap_frame_get_be32(frame, &attr))
+			return false;
+
+		print_field("%*cAttribute: 0x%08x (%s)", (indent - 8),
+					' ', attr, mediattr2str(attr));
+	}
+
+	return true;
+
+response:
+	if (avctp_frame->pt == AVRCP_PACKET_TYPE_SINGLE
+				|| avctp_frame->pt == AVRCP_PACKET_TYPE_START) {
+		if (!l2cap_frame_get_u8(frame, &num))
+			return false;
+
+		avrcp_continuing.num = num;
+		print_field("%*cAttributeCount: 0x%02x", (indent - 8),
+								' ', num);
+		len--;
+	} else {
+		num = avrcp_continuing.num;
+
+		if (avrcp_continuing.size > 0) {
+			uint16_t size;
+
+			if (avrcp_continuing.size > len) {
+				size = len;
+				avrcp_continuing.size -= len;
+			} else {
+				size = avrcp_continuing.size;
+				avrcp_continuing.size = 0;
+			}
+
+			printf("ContinuingAttributeValue: ");
+			for (; size > 0; size--) {
+				uint8_t c;
+
+				if (!l2cap_frame_get_u8(frame, &c))
+					return false;
+
+				printf("%1c", isprint(c) ? c : '.');
+			}
+			printf("\n");
+
+			len -= size;
+		}
+	}
+
+	while (num > 0 && len > 0) {
+		uint32_t attr;
+		uint16_t charset, attrlen;
+
+		if (!l2cap_frame_get_be32(frame, &attr))
+			return false;
+
+		print_field("%*cAttribute: 0x%08x (%s)", (indent - 8),
+						' ', attr, mediattr2str(attr));
+
+		if (!l2cap_frame_get_be16(frame, &charset))
+			return false;
+
+		print_field("%*cCharsetID: 0x%04x (%s)", (indent - 8),
+				' ', charset, charset2str(charset));
+
+		if (!l2cap_frame_get_be16(frame, &attrlen))
+			return false;
+
+		print_field("%*cAttributeValueLength: 0x%04x",
+						(indent - 8), ' ', attrlen);
+
+		len -= sizeof(attr) + sizeof(charset) + sizeof(attrlen);
+		num--;
+
+		print_field("%*cAttributeValue: ", (indent - 8), ' ');
+		for (; attrlen > 0 && len > 0; attrlen--, len--) {
+			uint8_t c;
+
+			if (!l2cap_frame_get_u8(frame, &c))
+				return false;
+
+			printf("%1c", isprint(c) ? c : '.');
+		}
+
+		if (attrlen > 0)
+			avrcp_continuing.size = attrlen;
+	}
+
+	avrcp_continuing.num = num;
+
+	return true;
+}
+
 struct avrcp_ctrl_pdu_data {
 	uint8_t pduid;
 	bool (*func) (struct avctp_frame *avctp_frame, uint8_t ctype,
@@ -920,6 +1076,7 @@ static const struct avrcp_ctrl_pdu_data avrcp_ctrl_pdu_table[] = {
 	{ 0x15, avrcp_get_player_attribute_text		},
 	{ 0x16, avrcp_get_player_value_text		},
 	{ 0x17, avrcp_displayable_charset		},
+	{ 0x20, avrcp_get_element_attributes		},
 	{ }
 };
 
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 2/2] Monitor: Add AVRCP GetElementAttributes support
  2014-09-12 13:16 ` [PATCH v2 2/2] Monitor: Add AVRCP GetElementAttributes support Vikrampal Yadav
@ 2014-09-15  8:30   ` Luiz Augusto von Dentz
  2014-09-15  8:50     ` Luiz Augusto von Dentz
  0 siblings, 1 reply; 7+ messages in thread
From: Luiz Augusto von Dentz @ 2014-09-15  8:30 UTC (permalink / raw)
  To: Vikrampal Yadav; +Cc: linux-bluetooth@vger.kernel.org, Dmitry Kasatkin, cpgs

Hi Vikram,

On Fri, Sep 12, 2014 at 4:16 PM, Vikrampal Yadav <vikram.pal@samsung.com> wrote:
> Support for decoding AVRCP GetElementAttributes added in Bluetooth
> monitor.
> ---
>  monitor/avctp.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 157 insertions(+)
>
> diff --git a/monitor/avctp.c b/monitor/avctp.c
> index f366b87..89997d0 100644
> --- a/monitor/avctp.c
> +++ b/monitor/avctp.c
> @@ -158,6 +158,16 @@
>  #define AVRCP_ATTRIBUTE_SHUFFLE                0x03
>  #define AVRCP_ATTRIBUTE_SCAN           0x04
>
> +/* media attributes */
> +#define AVRCP_MEDIA_ATTRIBUTE_ILLEGAL  0x00
> +#define AVRCP_MEDIA_ATTRIBUTE_TITLE    0x01
> +#define AVRCP_MEDIA_ATTRIBUTE_ARTIST   0x02
> +#define AVRCP_MEDIA_ATTRIBUTE_ALBUM    0x03
> +#define AVRCP_MEDIA_ATTRIBUTE_TRACK    0x04
> +#define AVRCP_MEDIA_ATTRIBUTE_TOTAL    0x05
> +#define AVRCP_MEDIA_ATTRIBUTE_GENRE    0x06
> +#define AVRCP_MEDIA_ATTRIBUTE_DURATION 0x07
> +
>  struct avctp_frame {
>         uint8_t hdr;
>         uint8_t pt;
> @@ -165,6 +175,11 @@ struct avctp_frame {
>         struct l2cap_frame l2cap_frame;
>  };
>
> +static struct avrcp_continuing {
> +       uint16_t num;
> +       uint16_t size;
> +} avrcp_continuing;
> +
>  static const char *ctype2str(uint8_t ctype)
>  {
>         switch (ctype & 0x0f) {
> @@ -524,6 +539,30 @@ static const char *charset2str(uint16_t charset)
>         }
>  }
>
> +static const char *mediattr2str(uint32_t attr)
> +{
> +       switch (attr) {
> +       case AVRCP_MEDIA_ATTRIBUTE_ILLEGAL:
> +               return "Illegal";
> +       case AVRCP_MEDIA_ATTRIBUTE_TITLE:
> +               return "Title";
> +       case AVRCP_MEDIA_ATTRIBUTE_ARTIST:
> +               return "Artist";
> +       case AVRCP_MEDIA_ATTRIBUTE_ALBUM:
> +               return "Album";
> +       case AVRCP_MEDIA_ATTRIBUTE_TRACK:
> +               return "Track";
> +       case AVRCP_MEDIA_ATTRIBUTE_TOTAL:
> +               return "Track Total";
> +       case AVRCP_MEDIA_ATTRIBUTE_GENRE:
> +               return "Genre";
> +       case AVRCP_MEDIA_ATTRIBUTE_DURATION:
> +               return "Track duration";
> +       default:
> +               return "Reserved";
> +       }
> +}
> +
>  static bool avrcp_passthrough_packet(struct avctp_frame *avctp_frame)
>  {
>         struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
> @@ -905,6 +944,123 @@ static bool avrcp_displayable_charset(struct avctp_frame *avctp_frame,
>         return true;
>  }
>
> +static bool avrcp_get_element_attributes(struct avctp_frame *avctp_frame,
> +                                               uint8_t ctype, uint8_t len,
> +                                               uint8_t indent)
> +{
> +       struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
> +       uint64_t id;
> +       uint8_t num;
> +
> +       if (ctype > AVC_CTYPE_GENERAL_INQUIRY)
> +               goto response;
> +
> +       if (!l2cap_frame_get_be64(frame, &id))
> +               return false;
> +
> +       print_field("%*cIdentifier: 0x%jx (%s)", (indent - 8), ' ',
> +                                       id, id ? "Reserved" : "PLAYING");
> +
> +       if (!l2cap_frame_get_u8(frame, &num))
> +               return false;
> +
> +       print_field("%*cAttributeCount: 0x%02x", (indent - 8), ' ', num);
> +
> +       for (; num > 0; num--) {
> +               uint32_t attr;
> +
> +               if (!l2cap_frame_get_be32(frame, &attr))
> +                       return false;
> +
> +               print_field("%*cAttribute: 0x%08x (%s)", (indent - 8),
> +                                       ' ', attr, mediattr2str(attr));
> +       }
> +
> +       return true;
> +
> +response:
> +       if (avctp_frame->pt == AVRCP_PACKET_TYPE_SINGLE
> +                               || avctp_frame->pt == AVRCP_PACKET_TYPE_START) {
> +               if (!l2cap_frame_get_u8(frame, &num))
> +                       return false;
> +
> +               avrcp_continuing.num = num;
> +               print_field("%*cAttributeCount: 0x%02x", (indent - 8),
> +                                                               ' ', num);
> +               len--;
> +       } else {
> +               num = avrcp_continuing.num;
> +
> +               if (avrcp_continuing.size > 0) {
> +                       uint16_t size;
> +
> +                       if (avrcp_continuing.size > len) {
> +                               size = len;
> +                               avrcp_continuing.size -= len;
> +                       } else {
> +                               size = avrcp_continuing.size;
> +                               avrcp_continuing.size = 0;
> +                       }
> +
> +                       printf("ContinuingAttributeValue: ");
> +                       for (; size > 0; size--) {
> +                               uint8_t c;
> +
> +                               if (!l2cap_frame_get_u8(frame, &c))
> +                                       return false;
> +
> +                               printf("%1c", isprint(c) ? c : '.');
> +                       }
> +                       printf("\n");
> +
> +                       len -= size;
> +               }
> +       }

I would handle this with a switch and I need to double check but I
don't think it is valid to fragment on attribute level.


> +       while (num > 0 && len > 0) {
> +               uint32_t attr;
> +               uint16_t charset, attrlen;
> +
> +               if (!l2cap_frame_get_be32(frame, &attr))
> +                       return false;
> +
> +               print_field("%*cAttribute: 0x%08x (%s)", (indent - 8),
> +                                               ' ', attr, mediattr2str(attr));
> +
> +               if (!l2cap_frame_get_be16(frame, &charset))
> +                       return false;
> +
> +               print_field("%*cCharsetID: 0x%04x (%s)", (indent - 8),
> +                               ' ', charset, charset2str(charset));
> +
> +               if (!l2cap_frame_get_be16(frame, &attrlen))
> +                       return false;
> +
> +               print_field("%*cAttributeValueLength: 0x%04x",
> +                                               (indent - 8), ' ', attrlen);
> +
> +               len -= sizeof(attr) + sizeof(charset) + sizeof(attrlen);
> +               num--;
> +
> +               print_field("%*cAttributeValue: ", (indent - 8), ' ');
> +               for (; attrlen > 0 && len > 0; attrlen--, len--) {
> +                       uint8_t c;
> +
> +                       if (!l2cap_frame_get_u8(frame, &c))
> +                               return false;
> +
> +                       printf("%1c", isprint(c) ? c : '.');
> +               }
> +
> +               if (attrlen > 0)
> +                       avrcp_continuing.size = attrlen;
> +       }
> +
> +       avrcp_continuing.num = num;
> +
> +       return true;
> +}
> +
>  struct avrcp_ctrl_pdu_data {
>         uint8_t pduid;
>         bool (*func) (struct avctp_frame *avctp_frame, uint8_t ctype,
> @@ -920,6 +1076,7 @@ static const struct avrcp_ctrl_pdu_data avrcp_ctrl_pdu_table[] = {
>         { 0x15, avrcp_get_player_attribute_text         },
>         { 0x16, avrcp_get_player_value_text             },
>         { 0x17, avrcp_displayable_charset               },
> +       { 0x20, avrcp_get_element_attributes            },
>         { }
>  };
>
> --
> 1.9.1
>



-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 2/2] Monitor: Add AVRCP GetElementAttributes support
  2014-09-15  8:30   ` Luiz Augusto von Dentz
@ 2014-09-15  8:50     ` Luiz Augusto von Dentz
  2014-09-15 10:32       ` Vikrampal
  2014-09-16  8:53       ` Vikrampal
  0 siblings, 2 replies; 7+ messages in thread
From: Luiz Augusto von Dentz @ 2014-09-15  8:50 UTC (permalink / raw)
  To: Vikrampal Yadav; +Cc: linux-bluetooth@vger.kernel.org, Dmitry Kasatkin, cpgs

Hi Vikram,

On Mon, Sep 15, 2014 at 11:30 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Vikram,
>
> On Fri, Sep 12, 2014 at 4:16 PM, Vikrampal Yadav <vikram.pal@samsung.com> wrote:
>> Support for decoding AVRCP GetElementAttributes added in Bluetooth
>> monitor.
>> ---
>>  monitor/avctp.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 157 insertions(+)
>>
>> diff --git a/monitor/avctp.c b/monitor/avctp.c
>> index f366b87..89997d0 100644
>> --- a/monitor/avctp.c
>> +++ b/monitor/avctp.c
>> @@ -158,6 +158,16 @@
>>  #define AVRCP_ATTRIBUTE_SHUFFLE                0x03
>>  #define AVRCP_ATTRIBUTE_SCAN           0x04
>>
>> +/* media attributes */
>> +#define AVRCP_MEDIA_ATTRIBUTE_ILLEGAL  0x00
>> +#define AVRCP_MEDIA_ATTRIBUTE_TITLE    0x01
>> +#define AVRCP_MEDIA_ATTRIBUTE_ARTIST   0x02
>> +#define AVRCP_MEDIA_ATTRIBUTE_ALBUM    0x03
>> +#define AVRCP_MEDIA_ATTRIBUTE_TRACK    0x04
>> +#define AVRCP_MEDIA_ATTRIBUTE_TOTAL    0x05
>> +#define AVRCP_MEDIA_ATTRIBUTE_GENRE    0x06
>> +#define AVRCP_MEDIA_ATTRIBUTE_DURATION 0x07
>> +
>>  struct avctp_frame {
>>         uint8_t hdr;
>>         uint8_t pt;
>> @@ -165,6 +175,11 @@ struct avctp_frame {
>>         struct l2cap_frame l2cap_frame;
>>  };
>>
>> +static struct avrcp_continuing {
>> +       uint16_t num;
>> +       uint16_t size;
>> +} avrcp_continuing;
>> +
>>  static const char *ctype2str(uint8_t ctype)
>>  {
>>         switch (ctype & 0x0f) {
>> @@ -524,6 +539,30 @@ static const char *charset2str(uint16_t charset)
>>         }
>>  }
>>
>> +static const char *mediattr2str(uint32_t attr)
>> +{
>> +       switch (attr) {
>> +       case AVRCP_MEDIA_ATTRIBUTE_ILLEGAL:
>> +               return "Illegal";
>> +       case AVRCP_MEDIA_ATTRIBUTE_TITLE:
>> +               return "Title";
>> +       case AVRCP_MEDIA_ATTRIBUTE_ARTIST:
>> +               return "Artist";
>> +       case AVRCP_MEDIA_ATTRIBUTE_ALBUM:
>> +               return "Album";
>> +       case AVRCP_MEDIA_ATTRIBUTE_TRACK:
>> +               return "Track";
>> +       case AVRCP_MEDIA_ATTRIBUTE_TOTAL:
>> +               return "Track Total";
>> +       case AVRCP_MEDIA_ATTRIBUTE_GENRE:
>> +               return "Genre";
>> +       case AVRCP_MEDIA_ATTRIBUTE_DURATION:
>> +               return "Track duration";
>> +       default:
>> +               return "Reserved";
>> +       }
>> +}
>> +
>>  static bool avrcp_passthrough_packet(struct avctp_frame *avctp_frame)
>>  {
>>         struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
>> @@ -905,6 +944,123 @@ static bool avrcp_displayable_charset(struct avctp_frame *avctp_frame,
>>         return true;
>>  }
>>
>> +static bool avrcp_get_element_attributes(struct avctp_frame *avctp_frame,
>> +                                               uint8_t ctype, uint8_t len,
>> +                                               uint8_t indent)
>> +{
>> +       struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
>> +       uint64_t id;
>> +       uint8_t num;
>> +
>> +       if (ctype > AVC_CTYPE_GENERAL_INQUIRY)
>> +               goto response;
>> +
>> +       if (!l2cap_frame_get_be64(frame, &id))
>> +               return false;
>> +
>> +       print_field("%*cIdentifier: 0x%jx (%s)", (indent - 8), ' ',
>> +                                       id, id ? "Reserved" : "PLAYING");
>> +
>> +       if (!l2cap_frame_get_u8(frame, &num))
>> +               return false;
>> +
>> +       print_field("%*cAttributeCount: 0x%02x", (indent - 8), ' ', num);
>> +
>> +       for (; num > 0; num--) {
>> +               uint32_t attr;
>> +
>> +               if (!l2cap_frame_get_be32(frame, &attr))
>> +                       return false;
>> +
>> +               print_field("%*cAttribute: 0x%08x (%s)", (indent - 8),
>> +                                       ' ', attr, mediattr2str(attr));
>> +       }
>> +
>> +       return true;
>> +
>> +response:
>> +       if (avctp_frame->pt == AVRCP_PACKET_TYPE_SINGLE
>> +                               || avctp_frame->pt == AVRCP_PACKET_TYPE_START) {
>> +               if (!l2cap_frame_get_u8(frame, &num))
>> +                       return false;
>> +
>> +               avrcp_continuing.num = num;
>> +               print_field("%*cAttributeCount: 0x%02x", (indent - 8),
>> +                                                               ' ', num);
>> +               len--;
>> +       } else {
>> +               num = avrcp_continuing.num;
>> +
>> +               if (avrcp_continuing.size > 0) {
>> +                       uint16_t size;
>> +
>> +                       if (avrcp_continuing.size > len) {
>> +                               size = len;
>> +                               avrcp_continuing.size -= len;
>> +                       } else {
>> +                               size = avrcp_continuing.size;
>> +                               avrcp_continuing.size = 0;
>> +                       }
>> +
>> +                       printf("ContinuingAttributeValue: ");
>> +                       for (; size > 0; size--) {
>> +                               uint8_t c;
>> +
>> +                               if (!l2cap_frame_get_u8(frame, &c))
>> +                                       return false;
>> +
>> +                               printf("%1c", isprint(c) ? c : '.');
>> +                       }
>> +                       printf("\n");
>> +
>> +                       len -= size;
>> +               }
>> +       }
>
> I would handle this with a switch and I need to double check but I
> don't think it is valid to fragment on attribute level.

Seems to be a valid according to spec.

>
>> +       while (num > 0 && len > 0) {
>> +               uint32_t attr;
>> +               uint16_t charset, attrlen;
>> +
>> +               if (!l2cap_frame_get_be32(frame, &attr))
>> +                       return false;
>> +
>> +               print_field("%*cAttribute: 0x%08x (%s)", (indent - 8),
>> +                                               ' ', attr, mediattr2str(attr));
>> +
>> +               if (!l2cap_frame_get_be16(frame, &charset))
>> +                       return false;
>> +
>> +               print_field("%*cCharsetID: 0x%04x (%s)", (indent - 8),
>> +                               ' ', charset, charset2str(charset));
>> +
>> +               if (!l2cap_frame_get_be16(frame, &attrlen))
>> +                       return false;
>> +
>> +               print_field("%*cAttributeValueLength: 0x%04x",
>> +                                               (indent - 8), ' ', attrlen);
>> +
>> +               len -= sizeof(attr) + sizeof(charset) + sizeof(attrlen);
>> +               num--;
>> +
>> +               print_field("%*cAttributeValue: ", (indent - 8), ' ');
>> +               for (; attrlen > 0 && len > 0; attrlen--, len--) {
>> +                       uint8_t c;
>> +
>> +                       if (!l2cap_frame_get_u8(frame, &c))
>> +                               return false;
>> +
>> +                       printf("%1c", isprint(c) ? c : '.');
>> +               }
>> +
>> +               if (attrlen > 0)
>> +                       avrcp_continuing.size = attrlen;
>> +       }
>> +
>> +       avrcp_continuing.num = num;
>> +
>> +       return true;
>> +}
>> +
>>  struct avrcp_ctrl_pdu_data {
>>         uint8_t pduid;
>>         bool (*func) (struct avctp_frame *avctp_frame, uint8_t ctype,
>> @@ -920,6 +1076,7 @@ static const struct avrcp_ctrl_pdu_data avrcp_ctrl_pdu_table[] = {
>>         { 0x15, avrcp_get_player_attribute_text         },
>>         { 0x16, avrcp_get_player_value_text             },
>>         { 0x17, avrcp_displayable_charset               },
>> +       { 0x20, avrcp_get_element_attributes            },
>>         { }
>>  };
>>
>> --
>> 1.9.1
>>
>
>
>
> --
> Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/2] Monitor: Modify design of AVRCP callback functions
  2014-09-12 13:16 [PATCH v2 1/2] Monitor: Modify design of AVRCP callback functions Vikrampal Yadav
  2014-09-12 13:16 ` [PATCH v2 2/2] Monitor: Add AVRCP GetElementAttributes support Vikrampal Yadav
@ 2014-09-15  8:51 ` Luiz Augusto von Dentz
  1 sibling, 0 replies; 7+ messages in thread
From: Luiz Augusto von Dentz @ 2014-09-15  8:51 UTC (permalink / raw)
  To: Vikrampal Yadav; +Cc: linux-bluetooth@vger.kernel.org, Dmitry Kasatkin, cpgs

Hi,

On Fri, Sep 12, 2014 at 4:16 PM, Vikrampal Yadav <vikram.pal@samsung.com> wrote:
> Modified the design of AVRCP callback functions.
> ---
>  monitor/avctp.c | 103 ++++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 66 insertions(+), 37 deletions(-)
>
> diff --git a/monitor/avctp.c b/monitor/avctp.c
> index c7e242b..f366b87 100644
> --- a/monitor/avctp.c
> +++ b/monitor/avctp.c
> @@ -158,6 +158,13 @@
>  #define AVRCP_ATTRIBUTE_SHUFFLE                0x03
>  #define AVRCP_ATTRIBUTE_SCAN           0x04
>
> +struct avctp_frame {
> +       uint8_t hdr;
> +       uint8_t pt;
> +       uint16_t pid;
> +       struct l2cap_frame l2cap_frame;
> +};
> +
>  static const char *ctype2str(uint8_t ctype)
>  {
>         switch (ctype & 0x0f) {
> @@ -517,15 +524,19 @@ static const char *charset2str(uint16_t charset)
>         }
>  }
>
> -static bool avrcp_passthrough_packet(struct l2cap_frame *frame)
> +static bool avrcp_passthrough_packet(struct avctp_frame *avctp_frame)
>  {
> +       struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
> +
>         packet_hexdump(frame->data, frame->size);
>         return true;
>  }
>
> -static bool avrcp_get_capabilities(struct l2cap_frame *frame, uint8_t ctype,
> -                                               uint8_t len, uint8_t indent)
> +static bool avrcp_get_capabilities(struct avctp_frame *avctp_frame,
> +                                       uint8_t ctype, uint8_t len,
> +                                       uint8_t indent)
>  {
> +       struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
>         uint8_t cap, count;
>         int i;
>
> @@ -576,10 +587,11 @@ static bool avrcp_get_capabilities(struct l2cap_frame *frame, uint8_t ctype,
>         return true;
>  }
>
> -static bool avrcp_list_player_attributes(struct l2cap_frame *frame,
> +static bool avrcp_list_player_attributes(struct avctp_frame *avctp_frame,
>                                                 uint8_t ctype, uint8_t len,
>                                                 uint8_t indent)
>  {
> +       struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
>         uint8_t num;
>         int i;
>
> @@ -604,9 +616,11 @@ static bool avrcp_list_player_attributes(struct l2cap_frame *frame,
>         return true;
>  }
>
> -static bool avrcp_list_player_values(struct l2cap_frame *frame, uint8_t ctype,
> -                                               uint8_t len, uint8_t indent)
> +static bool avrcp_list_player_values(struct avctp_frame *avctp_frame,
> +                                       uint8_t ctype, uint8_t len,
> +                                       uint8_t indent)
>  {
> +       struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
>         static uint8_t attr = 0;
>         uint8_t num;
>
> @@ -640,10 +654,11 @@ response:
>         return true;
>  }
>
> -static bool avrcp_get_current_player_value(struct l2cap_frame *frame,
> +static bool avrcp_get_current_player_value(struct avctp_frame *avctp_frame,
>                                                 uint8_t ctype, uint8_t len,
>                                                 uint8_t indent)
>  {
> +       struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
>         uint8_t num;
>
>         if (!l2cap_frame_get_u8(frame, &num))
> @@ -688,9 +703,11 @@ response:
>         return true;
>  }
>
> -static bool avrcp_set_player_value(struct l2cap_frame *frame, uint8_t ctype,
> -                                       uint8_t len, uint8_t indent)
> +static bool avrcp_set_player_value(struct avctp_frame *avctp_frame,
> +                                       uint8_t ctype, uint8_t len,
> +                                       uint8_t indent)
>  {
> +       struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
>         uint8_t num;
>
>         if (ctype > AVC_CTYPE_GENERAL_INQUIRY)
> @@ -720,10 +737,11 @@ static bool avrcp_set_player_value(struct l2cap_frame *frame, uint8_t ctype,
>         return true;
>  }
>
> -static bool avrcp_get_player_attribute_text(struct l2cap_frame *frame,
> +static bool avrcp_get_player_attribute_text(struct avctp_frame *avctp_frame,
>                                                 uint8_t ctype, uint8_t len,
>                                                 uint8_t indent)
>  {
> +       struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
>         uint8_t num;
>
>         if (!l2cap_frame_get_u8(frame, &num))
> @@ -783,10 +801,11 @@ response:
>         return true;
>  }
>
> -static bool avrcp_get_player_value_text(struct l2cap_frame *frame,
> +static bool avrcp_get_player_value_text(struct avctp_frame *avctp_frame,
>                                         uint8_t ctype, uint8_t len,
>                                         uint8_t indent)
>  {
> +       struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
>         static uint8_t attr = 0;
>         uint8_t num;
>
> @@ -858,9 +877,11 @@ response:
>         return true;
>  }
>
> -static bool avrcp_displayable_charset(struct l2cap_frame *frame, uint8_t ctype,
> -                                       uint8_t len, uint8_t indent)
> +static bool avrcp_displayable_charset(struct avctp_frame *avctp_frame,
> +                                       uint8_t ctype, uint8_t len,
> +                                       uint8_t indent)
>  {
> +       struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
>         uint8_t num;
>
>         if (ctype > AVC_CTYPE_GENERAL_INQUIRY)
> @@ -886,8 +907,8 @@ static bool avrcp_displayable_charset(struct l2cap_frame *frame, uint8_t ctype,
>
>  struct avrcp_ctrl_pdu_data {
>         uint8_t pduid;
> -       bool (*func) (struct l2cap_frame *frame, uint8_t ctype, uint8_t len,
> -                                                       uint8_t indent);
> +       bool (*func) (struct avctp_frame *avctp_frame, uint8_t ctype,
> +                                               uint8_t len, uint8_t indent);
>  };
>
>  static const struct avrcp_ctrl_pdu_data avrcp_ctrl_pdu_table[] = {
> @@ -915,10 +936,11 @@ static bool avrcp_rejected_packet(struct l2cap_frame *frame, uint8_t indent)
>         return true;
>  }
>
> -static bool avrcp_pdu_packet(struct l2cap_frame *frame, uint8_t ctype,
> +static bool avrcp_pdu_packet(struct avctp_frame *avctp_frame, uint8_t ctype,
>                                                                 uint8_t indent)
>  {
> -       uint8_t pduid, pt;
> +       struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
> +       uint8_t pduid;
>         uint16_t len;
>         int i;
>         const struct avrcp_ctrl_pdu_data *ctrl_pdu_data = NULL;
> @@ -926,14 +948,14 @@ static bool avrcp_pdu_packet(struct l2cap_frame *frame, uint8_t ctype,
>         if (!l2cap_frame_get_u8(frame, &pduid))
>                 return false;
>
> -       if (!l2cap_frame_get_u8(frame, &pt))
> +       if (!l2cap_frame_get_u8(frame, &avctp_frame->pt))
>                 return false;
>
>         if (!l2cap_frame_get_be16(frame, &len))
>                 return false;
>
>         print_indent(indent, COLOR_OFF, "AVRCP: ", pdu2str(pduid), COLOR_OFF,
> -                                       " pt %s len 0x%04x", pt2str(pt), len);
> +                       " pt %s len 0x%04x", pt2str(avctp_frame->pt), len);
>
>         if (frame->size != len)
>                 return false;
> @@ -953,11 +975,13 @@ static bool avrcp_pdu_packet(struct l2cap_frame *frame, uint8_t ctype,
>                 return true;
>         }
>
> -       return ctrl_pdu_data->func(frame, ctype, len, indent + 2);
> +       return ctrl_pdu_data->func(avctp_frame, ctype, len, indent + 2);
>  }
>
> -static bool avrcp_control_packet(struct l2cap_frame *frame)
> +static bool avrcp_control_packet(struct avctp_frame *avctp_frame)
>  {
> +       struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
> +
>         uint8_t ctype, address, subunit, opcode, company[3], indent = 2;
>
>         if (!l2cap_frame_get_u8(frame, &ctype) ||
> @@ -988,7 +1012,7 @@ static bool avrcp_control_packet(struct l2cap_frame *frame)
>
>         switch (opcode) {
>         case 0x7c:
> -               return avrcp_passthrough_packet(frame);
> +               return avrcp_passthrough_packet(avctp_frame);
>         case 0x00:
>                 if (!l2cap_frame_get_u8(frame, &company[0]) ||
>                                 !l2cap_frame_get_u8(frame, &company[1]) ||
> @@ -998,29 +1022,32 @@ static bool avrcp_control_packet(struct l2cap_frame *frame)
>                 print_field("%*cCompany ID: 0x%02x%02x%02x", indent, ' ',
>                                         company[0], company[1], company[2]);
>
> -               return avrcp_pdu_packet(frame, ctype, 10);
> +               return avrcp_pdu_packet(avctp_frame, ctype, 10);
>         default:
>                 packet_hexdump(frame->data, frame->size);
>                 return true;
>         }
>  }
>
> -static bool avrcp_browsing_packet(struct l2cap_frame *frame, uint8_t hdr)
> +static bool avrcp_browsing_packet(struct avctp_frame *avctp_frame)
>  {
> +       struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
> +
>         packet_hexdump(frame->data, frame->size);
>         return true;
>  }
>
> -static void avrcp_packet(struct l2cap_frame *frame, uint8_t hdr)
> +static void avrcp_packet(struct avctp_frame *avctp_frame)
>  {
> +       struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
>         bool ret;
>
>         switch (frame->psm) {
>         case 0x17:
> -               ret = avrcp_control_packet(frame);
> +               ret = avrcp_control_packet(avctp_frame);
>                 break;
>         case 0x1B:
> -               ret = avrcp_browsing_packet(frame, hdr);
> +               ret = avrcp_browsing_packet(avctp_frame);
>                 break;
>         default:
>                 packet_hexdump(frame->data, frame->size);
> @@ -1035,15 +1062,16 @@ static void avrcp_packet(struct l2cap_frame *frame, uint8_t hdr)
>
>  void avctp_packet(const struct l2cap_frame *frame)
>  {
> -       uint8_t hdr;
> -       uint16_t pid;
> -       struct l2cap_frame avctp_frame;
> +       struct l2cap_frame *l2cap_frame;
> +       struct avctp_frame avctp_frame;
>         const char *pdu_color;
>
> -       l2cap_frame_pull(&avctp_frame, frame, 0);
> +       l2cap_frame_pull(&avctp_frame.l2cap_frame, frame, 0);
> +
> +       l2cap_frame = &avctp_frame.l2cap_frame;
>
> -       if (!l2cap_frame_get_u8(&avctp_frame, &hdr) ||
> -                               !l2cap_frame_get_be16(&avctp_frame, &pid)) {
> +       if (!l2cap_frame_get_u8(l2cap_frame, &avctp_frame.hdr) ||
> +                       !l2cap_frame_get_be16(l2cap_frame, &avctp_frame.pid)) {
>                 print_text(COLOR_ERROR, "frame too short");
>                 packet_hexdump(frame->data, frame->size);
>                 return;
> @@ -1057,11 +1085,12 @@ void avctp_packet(const struct l2cap_frame *frame)
>         print_indent(6, pdu_color, "AVCTP", "", COLOR_OFF,
>                                 " %s: %s: type 0x%02x label %d PID 0x%04x",
>                                 frame->psm == 23 ? "Control" : "Browsing",
> -                               hdr & 0x02 ? "Response" : "Command",
> -                               hdr & 0x0c, hdr >> 4, pid);
> +                               avctp_frame.hdr & 0x02 ? "Response" : "Command",
> +                               avctp_frame.hdr & 0x0c, avctp_frame.hdr >> 4,
> +                               avctp_frame.pid);
>
> -       if (pid == 0x110e || pid == 0x110c)
> -               avrcp_packet(&avctp_frame, hdr);
> +       if (avctp_frame.pid == 0x110e || avctp_frame.pid == 0x110c)
> +               avrcp_packet(&avctp_frame);
>         else
>                 packet_hexdump(frame->data, frame->size);
>  }
> --
> 1.9.1

Pushed, thanks.


-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH v2 2/2] Monitor: Add AVRCP GetElementAttributes support
  2014-09-15  8:50     ` Luiz Augusto von Dentz
@ 2014-09-15 10:32       ` Vikrampal
  2014-09-16  8:53       ` Vikrampal
  1 sibling, 0 replies; 7+ messages in thread
From: Vikrampal @ 2014-09-15 10:32 UTC (permalink / raw)
  To: 'Luiz Augusto von Dentz'
  Cc: linux-bluetooth, 'Dmitry Kasatkin', cpgs

Hi Luiz,

> -----Original Message-----
> From: Luiz Augusto von Dentz [mailto:luiz.dentz@gmail.com]
> Sent: Monday, September 15, 2014 2:21 PM
> To: Vikrampal Yadav
> Cc: linux-bluetooth@vger.kernel.org; Dmitry Kasatkin; cpgs@samsung.com
> Subject: Re: [PATCH v2 2/2] Monitor: Add AVRCP GetElementAttributes
> support
> 
> Hi Vikram,
> 
> On Mon, Sep 15, 2014 at 11:30 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> > Hi Vikram,
> >
> > On Fri, Sep 12, 2014 at 4:16 PM, Vikrampal Yadav
> <vikram.pal@samsung.com> wrote:
> >> Support for decoding AVRCP GetElementAttributes added in Bluetooth
> >> monitor.
> >> ---
> >>  monitor/avctp.c | 157
> >> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 157 insertions(+)
> >>
> >> diff --git a/monitor/avctp.c b/monitor/avctp.c index f366b87..89997d0
> >> 100644
> >> --- a/monitor/avctp.c
> >> +++ b/monitor/avctp.c
> >> @@ -158,6 +158,16 @@
> >>  #define AVRCP_ATTRIBUTE_SHUFFLE                0x03
> >>  #define AVRCP_ATTRIBUTE_SCAN           0x04
> >>
> >> +/* media attributes */
> >> +#define AVRCP_MEDIA_ATTRIBUTE_ILLEGAL  0x00
> >> +#define AVRCP_MEDIA_ATTRIBUTE_TITLE    0x01
> >> +#define AVRCP_MEDIA_ATTRIBUTE_ARTIST   0x02
> >> +#define AVRCP_MEDIA_ATTRIBUTE_ALBUM    0x03
> >> +#define AVRCP_MEDIA_ATTRIBUTE_TRACK    0x04
> >> +#define AVRCP_MEDIA_ATTRIBUTE_TOTAL    0x05
> >> +#define AVRCP_MEDIA_ATTRIBUTE_GENRE    0x06
> >> +#define AVRCP_MEDIA_ATTRIBUTE_DURATION 0x07
> >> +
> >>  struct avctp_frame {
> >>         uint8_t hdr;
> >>         uint8_t pt;
> >> @@ -165,6 +175,11 @@ struct avctp_frame {
> >>         struct l2cap_frame l2cap_frame;  };
> >>
> >> +static struct avrcp_continuing {
> >> +       uint16_t num;
> >> +       uint16_t size;
> >> +} avrcp_continuing;
> >> +
> >>  static const char *ctype2str(uint8_t ctype)  {
> >>         switch (ctype & 0x0f) {
> >> @@ -524,6 +539,30 @@ static const char *charset2str(uint16_t charset)
> >>         }
> >>  }
> >>
> >> +static const char *mediattr2str(uint32_t attr) {
> >> +       switch (attr) {
> >> +       case AVRCP_MEDIA_ATTRIBUTE_ILLEGAL:
> >> +               return "Illegal";
> >> +       case AVRCP_MEDIA_ATTRIBUTE_TITLE:
> >> +               return "Title";
> >> +       case AVRCP_MEDIA_ATTRIBUTE_ARTIST:
> >> +               return "Artist";
> >> +       case AVRCP_MEDIA_ATTRIBUTE_ALBUM:
> >> +               return "Album";
> >> +       case AVRCP_MEDIA_ATTRIBUTE_TRACK:
> >> +               return "Track";
> >> +       case AVRCP_MEDIA_ATTRIBUTE_TOTAL:
> >> +               return "Track Total";
> >> +       case AVRCP_MEDIA_ATTRIBUTE_GENRE:
> >> +               return "Genre";
> >> +       case AVRCP_MEDIA_ATTRIBUTE_DURATION:
> >> +               return "Track duration";
> >> +       default:
> >> +               return "Reserved";
> >> +       }
> >> +}
> >> +
> >>  static bool avrcp_passthrough_packet(struct avctp_frame
> >> *avctp_frame)  {
> >>         struct l2cap_frame *frame = &avctp_frame->l2cap_frame; @@
> >> -905,6 +944,123 @@ static bool avrcp_displayable_charset(struct
> avctp_frame *avctp_frame,
> >>         return true;
> >>  }
> >>
> >> +static bool avrcp_get_element_attributes(struct avctp_frame
> *avctp_frame,
> >> +                                               uint8_t ctype, uint8_t len,
> >> +                                               uint8_t indent) {
> >> +       struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
> >> +       uint64_t id;
> >> +       uint8_t num;
> >> +
> >> +       if (ctype > AVC_CTYPE_GENERAL_INQUIRY)
> >> +               goto response;
> >> +
> >> +       if (!l2cap_frame_get_be64(frame, &id))
> >> +               return false;
> >> +
> >> +       print_field("%*cIdentifier: 0x%jx (%s)", (indent - 8), ' ',
> >> +                                       id, id ? "Reserved" :
> >> + "PLAYING");
> >> +
> >> +       if (!l2cap_frame_get_u8(frame, &num))
> >> +               return false;
> >> +
> >> +       print_field("%*cAttributeCount: 0x%02x", (indent - 8), ' ',
> >> + num);
> >> +
> >> +       for (; num > 0; num--) {
> >> +               uint32_t attr;
> >> +
> >> +               if (!l2cap_frame_get_be32(frame, &attr))
> >> +                       return false;
> >> +
> >> +               print_field("%*cAttribute: 0x%08x (%s)", (indent - 8),
> >> +                                       ' ', attr, mediattr2str(attr));
> >> +       }
> >> +
> >> +       return true;
> >> +
> >> +response:
> >> +       if (avctp_frame->pt == AVRCP_PACKET_TYPE_SINGLE
> >> +                               || avctp_frame->pt == AVRCP_PACKET_TYPE_START) {
> >> +               if (!l2cap_frame_get_u8(frame, &num))
> >> +                       return false;
> >> +
> >> +               avrcp_continuing.num = num;
> >> +               print_field("%*cAttributeCount: 0x%02x", (indent - 8),
> >> +                                                               ' ', num);
> >> +               len--;
> >> +       } else {
> >> +               num = avrcp_continuing.num;
> >> +
> >> +               if (avrcp_continuing.size > 0) {
> >> +                       uint16_t size;
> >> +
> >> +                       if (avrcp_continuing.size > len) {
> >> +                               size = len;
> >> +                               avrcp_continuing.size -= len;
> >> +                       } else {
> >> +                               size = avrcp_continuing.size;
> >> +                               avrcp_continuing.size = 0;
> >> +                       }
> >> +
> >> +                       printf("ContinuingAttributeValue: ");
> >> +                       for (; size > 0; size--) {
> >> +                               uint8_t c;
> >> +
> >> +                               if (!l2cap_frame_get_u8(frame, &c))
> >> +                                       return false;
> >> +
> >> +                               printf("%1c", isprint(c) ? c : '.');
> >> +                       }
> >> +                       printf("\n");
> >> +
> >> +                       len -= size;
> >> +               }
> >> +       }
> >
> > I would handle this with a switch and I need to double check but I
> > don't think it is valid to fragment on attribute level.
> 
> Seems to be a valid according to spec.
> 
> >
> >> +       while (num > 0 && len > 0) {
> >> +               uint32_t attr;
> >> +               uint16_t charset, attrlen;
> >> +
> >> +               if (!l2cap_frame_get_be32(frame, &attr))
> >> +                       return false;
> >> +
> >> +               print_field("%*cAttribute: 0x%08x (%s)", (indent - 8),
> >> +                                               ' ', attr,
> >> + mediattr2str(attr));
> >> +
> >> +               if (!l2cap_frame_get_be16(frame, &charset))
> >> +                       return false;
> >> +
> >> +               print_field("%*cCharsetID: 0x%04x (%s)", (indent - 8),
> >> +                               ' ', charset, charset2str(charset));
> >> +
> >> +               if (!l2cap_frame_get_be16(frame, &attrlen))
> >> +                       return false;
> >> +
> >> +               print_field("%*cAttributeValueLength: 0x%04x",
> >> +                                               (indent - 8), ' ',
> >> + attrlen);
> >> +
> >> +               len -= sizeof(attr) + sizeof(charset) + sizeof(attrlen);
> >> +               num--;
> >> +
> >> +               print_field("%*cAttributeValue: ", (indent - 8), ' ');
> >> +               for (; attrlen > 0 && len > 0; attrlen--, len--) {
> >> +                       uint8_t c;
> >> +
> >> +                       if (!l2cap_frame_get_u8(frame, &c))
> >> +                               return false;
> >> +
> >> +                       printf("%1c", isprint(c) ? c : '.');
> >> +               }
> >> +
> >> +               if (attrlen > 0)
> >> +                       avrcp_continuing.size = attrlen;
> >> +       }
> >> +
> >> +       avrcp_continuing.num = num;
> >> +
> >> +       return true;
> >> +}
> >> +
> >>  struct avrcp_ctrl_pdu_data {
> >>         uint8_t pduid;
> >>         bool (*func) (struct avctp_frame *avctp_frame, uint8_t ctype,
> >> @@ -920,6 +1076,7 @@ static const struct avrcp_ctrl_pdu_data
> avrcp_ctrl_pdu_table[] = {
> >>         { 0x15, avrcp_get_player_attribute_text         },
> >>         { 0x16, avrcp_get_player_value_text             },
> >>         { 0x17, avrcp_displayable_charset               },
> >> +       { 0x20, avrcp_get_element_attributes            },
> >>         { }
> >>  };
> >>
> >> --
> >> 1.9.1
> >>
> >
> >
> >
> > --
> > Luiz Augusto von Dentz
> 
> 
> 
> --
> Luiz Augusto von Dentzv

The new function after your suggestion seems like:

static bool avrcp_get_element_attributes(struct avctp_frame *avctp_frame,
						uint8_t ctype, uint8_t len,
						uint8_t indent)
{
	struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
	uint64_t id;
	uint8_t num;

	if (ctype > AVC_CTYPE_GENERAL_INQUIRY)
		goto response;

	if (!l2cap_frame_get_be64(frame, &id))
		return false;

	print_field("%*cIdentifier: 0x%jx (%s)", (indent - 8), ' ',
					id, id ? "Reserved" : "PLAYING");

	if (!l2cap_frame_get_u8(frame, &num))
		return false;

	print_field("%*cAttributeCount: 0x%02x", (indent - 8), ' ', num);

	for (; num > 0; num--) {
		uint32_t attr;

		if (!l2cap_frame_get_be32(frame, &attr))
			return false;

		print_field("%*cAttribute: 0x%08x (%s)", (indent - 8),
					' ', attr, mediattr2str(attr));
	}

	return true;

response:
	switch (avctp_frame->pt) {
	case AVRCP_PACKET_TYPE_SINGLE:
	case AVRCP_PACKET_TYPE_START:
		if (!l2cap_frame_get_u8(frame, &num))
			return false;

		avrcp_continuing.num = num;
		print_field("%*cAttributeCount: 0x%02x", (indent - 8),
								' ', num);
		len--;
		break;
	case AVRCP_PACKET_TYPE_CONTINUING:
	case AVRCP_PACKET_TYPE_END:
		num = avrcp_continuing.num;

		if (avrcp_continuing.size > 0) {
			uint16_t size;

			if (avrcp_continuing.size > len) {
				size = len;
				avrcp_continuing.size -= len;
			} else {
				size = avrcp_continuing.size;
				avrcp_continuing.size = 0;
			}

			printf("ContinuingAttributeValue: ");
			for (; size > 0; size--) {
				uint8_t c;

				if (!l2cap_frame_get_u8(frame, &c))
					return false;

				printf("%1c", isprint(c) ? c : '.');
			}
			printf("\n");

			len -= size;
		}
		break;
	default:
		return false;
	}

	while (num > 0 && len > 0) {
		uint32_t attr;
		uint16_t charset, attrlen;

		if (!l2cap_frame_get_be32(frame, &attr))
			return false;

		print_field("%*cAttribute: 0x%08x (%s)", (indent - 8),
						' ', attr, mediattr2str(attr));

		if (!l2cap_frame_get_be16(frame, &charset))
			return false;

		print_field("%*cCharsetID: 0x%04x (%s)", (indent - 8),
				' ', charset, charset2str(charset));

		if (!l2cap_frame_get_be16(frame, &attrlen))
			return false;

		print_field("%*cAttributeValueLength: 0x%04x",
						(indent - 8), ' ', attrlen);

		len -= sizeof(attr) + sizeof(charset) + sizeof(attrlen);
		num--;

		print_field("%*cAttributeValue: ", (indent - 8), ' ');
		for (; attrlen > 0 && len > 0; attrlen--, len--) {
			uint8_t c;

			if (!l2cap_frame_get_u8(frame, &c))
				return false;

			printf("%1c", isprint(c) ? c : '.');
		}

		if (attrlen > 0)
			avrcp_continuing.size = attrlen;
	}

	avrcp_continuing.num = num;

	return true;
}

Regards,
Vikram


^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH v2 2/2] Monitor: Add AVRCP GetElementAttributes support
  2014-09-15  8:50     ` Luiz Augusto von Dentz
  2014-09-15 10:32       ` Vikrampal
@ 2014-09-16  8:53       ` Vikrampal
  1 sibling, 0 replies; 7+ messages in thread
From: Vikrampal @ 2014-09-16  8:53 UTC (permalink / raw)
  To: 'Luiz Augusto von Dentz'
  Cc: linux-bluetooth, 'Dmitry Kasatkin', cpgs

Hi Luiz,

> -----Original Message-----
> From: Vikrampal [mailto:vikram.pal@samsung.com]
> Sent: Monday, September 15, 2014 4:03 PM
> To: 'Luiz Augusto von Dentz'
> Cc: 'linux-bluetooth@vger.kernel.org'; 'Dmitry Kasatkin';
> 'cpgs@samsung.com'
> Subject: RE: [PATCH v2 2/2] Monitor: Add AVRCP GetElementAttributes
> support
> 
> Hi Luiz,
> 
> > -----Original Message-----
> > From: Luiz Augusto von Dentz [mailto:luiz.dentz@gmail.com]
> > Sent: Monday, September 15, 2014 2:21 PM
> > To: Vikrampal Yadav
> > Cc: linux-bluetooth@vger.kernel.org; Dmitry Kasatkin; cpgs@samsung.com
> > Subject: Re: [PATCH v2 2/2] Monitor: Add AVRCP GetElementAttributes
> > support
> >
> > Hi Vikram,
> >
> > On Mon, Sep 15, 2014 at 11:30 AM, Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > > Hi Vikram,
> > >
> > > On Fri, Sep 12, 2014 at 4:16 PM, Vikrampal Yadav
> > <vikram.pal@samsung.com> wrote:
> > >> Support for decoding AVRCP GetElementAttributes added in Bluetooth
> > >> monitor.
> > >> ---
> > >>  monitor/avctp.c | 157
> > >> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >>  1 file changed, 157 insertions(+)
> > >>
> > >> diff --git a/monitor/avctp.c b/monitor/avctp.c index
> > >> f366b87..89997d0
> > >> 100644
> > >> --- a/monitor/avctp.c
> > >> +++ b/monitor/avctp.c
> > >> @@ -158,6 +158,16 @@
> > >>  #define AVRCP_ATTRIBUTE_SHUFFLE                0x03
> > >>  #define AVRCP_ATTRIBUTE_SCAN           0x04
> > >>
> > >> +/* media attributes */
> > >> +#define AVRCP_MEDIA_ATTRIBUTE_ILLEGAL  0x00
> > >> +#define AVRCP_MEDIA_ATTRIBUTE_TITLE    0x01
> > >> +#define AVRCP_MEDIA_ATTRIBUTE_ARTIST   0x02
> > >> +#define AVRCP_MEDIA_ATTRIBUTE_ALBUM    0x03
> > >> +#define AVRCP_MEDIA_ATTRIBUTE_TRACK    0x04
> > >> +#define AVRCP_MEDIA_ATTRIBUTE_TOTAL    0x05
> > >> +#define AVRCP_MEDIA_ATTRIBUTE_GENRE    0x06
> > >> +#define AVRCP_MEDIA_ATTRIBUTE_DURATION 0x07
> > >> +
> > >>  struct avctp_frame {
> > >>         uint8_t hdr;
> > >>         uint8_t pt;
> > >> @@ -165,6 +175,11 @@ struct avctp_frame {
> > >>         struct l2cap_frame l2cap_frame;  };
> > >>
> > >> +static struct avrcp_continuing {
> > >> +       uint16_t num;
> > >> +       uint16_t size;
> > >> +} avrcp_continuing;
> > >> +
> > >>  static const char *ctype2str(uint8_t ctype)  {
> > >>         switch (ctype & 0x0f) {
> > >> @@ -524,6 +539,30 @@ static const char *charset2str(uint16_t charset)
> > >>         }
> > >>  }
> > >>
> > >> +static const char *mediattr2str(uint32_t attr) {
> > >> +       switch (attr) {
> > >> +       case AVRCP_MEDIA_ATTRIBUTE_ILLEGAL:
> > >> +               return "Illegal";
> > >> +       case AVRCP_MEDIA_ATTRIBUTE_TITLE:
> > >> +               return "Title";
> > >> +       case AVRCP_MEDIA_ATTRIBUTE_ARTIST:
> > >> +               return "Artist";
> > >> +       case AVRCP_MEDIA_ATTRIBUTE_ALBUM:
> > >> +               return "Album";
> > >> +       case AVRCP_MEDIA_ATTRIBUTE_TRACK:
> > >> +               return "Track";
> > >> +       case AVRCP_MEDIA_ATTRIBUTE_TOTAL:
> > >> +               return "Track Total";
> > >> +       case AVRCP_MEDIA_ATTRIBUTE_GENRE:
> > >> +               return "Genre";
> > >> +       case AVRCP_MEDIA_ATTRIBUTE_DURATION:
> > >> +               return "Track duration";
> > >> +       default:
> > >> +               return "Reserved";
> > >> +       }
> > >> +}
> > >> +
> > >>  static bool avrcp_passthrough_packet(struct avctp_frame
> > >> *avctp_frame)  {
> > >>         struct l2cap_frame *frame = &avctp_frame->l2cap_frame; @@
> > >> -905,6 +944,123 @@ static bool avrcp_displayable_charset(struct
> > avctp_frame *avctp_frame,
> > >>         return true;
> > >>  }
> > >>
> > >> +static bool avrcp_get_element_attributes(struct avctp_frame
> > *avctp_frame,
> > >> +                                               uint8_t ctype, uint8_t len,
> > >> +                                               uint8_t indent) {
> > >> +       struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
> > >> +       uint64_t id;
> > >> +       uint8_t num;
> > >> +
> > >> +       if (ctype > AVC_CTYPE_GENERAL_INQUIRY)
> > >> +               goto response;
> > >> +
> > >> +       if (!l2cap_frame_get_be64(frame, &id))
> > >> +               return false;
> > >> +
> > >> +       print_field("%*cIdentifier: 0x%jx (%s)", (indent - 8), ' ',
> > >> +                                       id, id ? "Reserved" :
> > >> + "PLAYING");
> > >> +
> > >> +       if (!l2cap_frame_get_u8(frame, &num))
> > >> +               return false;
> > >> +
> > >> +       print_field("%*cAttributeCount: 0x%02x", (indent - 8), ' ',
> > >> + num);
> > >> +
> > >> +       for (; num > 0; num--) {
> > >> +               uint32_t attr;
> > >> +
> > >> +               if (!l2cap_frame_get_be32(frame, &attr))
> > >> +                       return false;
> > >> +
> > >> +               print_field("%*cAttribute: 0x%08x (%s)", (indent - 8),
> > >> +                                       ' ', attr, mediattr2str(attr));
> > >> +       }
> > >> +
> > >> +       return true;
> > >> +
> > >> +response:
> > >> +       if (avctp_frame->pt == AVRCP_PACKET_TYPE_SINGLE
> > >> +                               || avctp_frame->pt == AVRCP_PACKET_TYPE_START) {
> > >> +               if (!l2cap_frame_get_u8(frame, &num))
> > >> +                       return false;
> > >> +
> > >> +               avrcp_continuing.num = num;
> > >> +               print_field("%*cAttributeCount: 0x%02x", (indent - 8),
> > >> +                                                               ' ', num);
> > >> +               len--;
> > >> +       } else {
> > >> +               num = avrcp_continuing.num;
> > >> +
> > >> +               if (avrcp_continuing.size > 0) {
> > >> +                       uint16_t size;
> > >> +
> > >> +                       if (avrcp_continuing.size > len) {
> > >> +                               size = len;
> > >> +                               avrcp_continuing.size -= len;
> > >> +                       } else {
> > >> +                               size = avrcp_continuing.size;
> > >> +                               avrcp_continuing.size = 0;
> > >> +                       }
> > >> +
> > >> +                       printf("ContinuingAttributeValue: ");
> > >> +                       for (; size > 0; size--) {
> > >> +                               uint8_t c;
> > >> +
> > >> +                               if (!l2cap_frame_get_u8(frame, &c))
> > >> +                                       return false;
> > >> +
> > >> +                               printf("%1c", isprint(c) ? c : '.');
> > >> +                       }
> > >> +                       printf("\n");
> > >> +
> > >> +                       len -= size;
> > >> +               }
> > >> +       }
> > >
> > > I would handle this with a switch and I need to double check but I
> > > don't think it is valid to fragment on attribute level.
> >
> > Seems to be a valid according to spec.
> >
> > >
> > >> +       while (num > 0 && len > 0) {
> > >> +               uint32_t attr;
> > >> +               uint16_t charset, attrlen;
> > >> +
> > >> +               if (!l2cap_frame_get_be32(frame, &attr))
> > >> +                       return false;
> > >> +
> > >> +               print_field("%*cAttribute: 0x%08x (%s)", (indent - 8),
> > >> +                                               ' ', attr,
> > >> + mediattr2str(attr));
> > >> +
> > >> +               if (!l2cap_frame_get_be16(frame, &charset))
> > >> +                       return false;
> > >> +
> > >> +               print_field("%*cCharsetID: 0x%04x (%s)", (indent - 8),
> > >> +                               ' ', charset,
> > >> + charset2str(charset));
> > >> +
> > >> +               if (!l2cap_frame_get_be16(frame, &attrlen))
> > >> +                       return false;
> > >> +
> > >> +               print_field("%*cAttributeValueLength: 0x%04x",
> > >> +                                               (indent - 8), ' ',
> > >> + attrlen);
> > >> +
> > >> +               len -= sizeof(attr) + sizeof(charset) + sizeof(attrlen);
> > >> +               num--;
> > >> +
> > >> +               print_field("%*cAttributeValue: ", (indent - 8), ' ');
> > >> +               for (; attrlen > 0 && len > 0; attrlen--, len--) {
> > >> +                       uint8_t c;
> > >> +
> > >> +                       if (!l2cap_frame_get_u8(frame, &c))
> > >> +                               return false;
> > >> +
> > >> +                       printf("%1c", isprint(c) ? c : '.');
> > >> +               }
> > >> +
> > >> +               if (attrlen > 0)
> > >> +                       avrcp_continuing.size = attrlen;
> > >> +       }
> > >> +
> > >> +       avrcp_continuing.num = num;
> > >> +
> > >> +       return true;
> > >> +}
> > >> +
> > >>  struct avrcp_ctrl_pdu_data {
> > >>         uint8_t pduid;
> > >>         bool (*func) (struct avctp_frame *avctp_frame, uint8_t
> > >> ctype, @@ -920,6 +1076,7 @@ static const struct avrcp_ctrl_pdu_data
> > avrcp_ctrl_pdu_table[] = {
> > >>         { 0x15, avrcp_get_player_attribute_text         },
> > >>         { 0x16, avrcp_get_player_value_text             },
> > >>         { 0x17, avrcp_displayable_charset               },
> > >> +       { 0x20, avrcp_get_element_attributes            },
> > >>         { }
> > >>  };
> > >>
> > >> --
> > >> 1.9.1
> > >>
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
> >
> >
> >
> > --
> > Luiz Augusto von Dentzv
> 
> The new function after your suggestion seems like:
> 
> static bool avrcp_get_element_attributes(struct avctp_frame *avctp_frame,
> 						uint8_t ctype, uint8_t len,
> 						uint8_t indent)
> {
> 	struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
> 	uint64_t id;
> 	uint8_t num;
> 
> 	if (ctype > AVC_CTYPE_GENERAL_INQUIRY)
> 		goto response;
> 
> 	if (!l2cap_frame_get_be64(frame, &id))
> 		return false;
> 
> 	print_field("%*cIdentifier: 0x%jx (%s)", (indent - 8), ' ',
> 					id, id ? "Reserved" : "PLAYING");
> 
> 	if (!l2cap_frame_get_u8(frame, &num))
> 		return false;
> 
> 	print_field("%*cAttributeCount: 0x%02x", (indent - 8), ' ', num);
> 
> 	for (; num > 0; num--) {
> 		uint32_t attr;
> 
> 		if (!l2cap_frame_get_be32(frame, &attr))
> 			return false;
> 
> 		print_field("%*cAttribute: 0x%08x (%s)", (indent - 8),
> 					' ', attr, mediattr2str(attr));
> 	}
> 
> 	return true;
> 
> response:
> 	switch (avctp_frame->pt) {
> 	case AVRCP_PACKET_TYPE_SINGLE:
> 	case AVRCP_PACKET_TYPE_START:
> 		if (!l2cap_frame_get_u8(frame, &num))
> 			return false;
> 
> 		avrcp_continuing.num = num;
> 		print_field("%*cAttributeCount: 0x%02x", (indent - 8),
> 								' ', num);
> 		len--;
> 		break;
> 	case AVRCP_PACKET_TYPE_CONTINUING:
> 	case AVRCP_PACKET_TYPE_END:
> 		num = avrcp_continuing.num;
> 
> 		if (avrcp_continuing.size > 0) {
> 			uint16_t size;
> 
> 			if (avrcp_continuing.size > len) {
> 				size = len;
> 				avrcp_continuing.size -= len;
> 			} else {
> 				size = avrcp_continuing.size;
> 				avrcp_continuing.size = 0;
> 			}
> 
> 			printf("ContinuingAttributeValue: ");
> 			for (; size > 0; size--) {
> 				uint8_t c;
> 
> 				if (!l2cap_frame_get_u8(frame, &c))
> 					return false;
> 
> 				printf("%1c", isprint(c) ? c : '.');
> 			}
> 			printf("\n");
> 
> 			len -= size;
> 		}
> 		break;
> 	default:
> 		return false;
> 	}
> 
> 	while (num > 0 && len > 0) {
> 		uint32_t attr;
> 		uint16_t charset, attrlen;
> 
> 		if (!l2cap_frame_get_be32(frame, &attr))
> 			return false;
> 
> 		print_field("%*cAttribute: 0x%08x (%s)", (indent - 8),
> 						' ', attr, mediattr2str(attr));
> 
> 		if (!l2cap_frame_get_be16(frame, &charset))
> 			return false;
> 
> 		print_field("%*cCharsetID: 0x%04x (%s)", (indent - 8),
> 				' ', charset, charset2str(charset));
> 
> 		if (!l2cap_frame_get_be16(frame, &attrlen))
> 			return false;
> 
> 		print_field("%*cAttributeValueLength: 0x%04x",
> 						(indent - 8), ' ', attrlen);
> 
> 		len -= sizeof(attr) + sizeof(charset) + sizeof(attrlen);
> 		num--;
> 
> 		print_field("%*cAttributeValue: ", (indent - 8), ' ');
> 		for (; attrlen > 0 && len > 0; attrlen--, len--) {
> 			uint8_t c;
> 
> 			if (!l2cap_frame_get_u8(frame, &c))
> 				return false;
> 
> 			printf("%1c", isprint(c) ? c : '.');
> 		}
> 
> 		if (attrlen > 0)
> 			avrcp_continuing.size = attrlen;
> 	}
> 
> 	avrcp_continuing.num = num;
> 
> 	return true;
> }
> 
> Regards,
> Vikram

The modified function tries to handle malformed packet case:

static bool avrcp_get_element_attributes(struct avctp_frame *avctp_frame,
						uint8_t ctype, uint8_t len,
						uint8_t indent)
{
	struct l2cap_frame *frame = &avctp_frame->l2cap_frame;
	uint64_t id;
	uint8_t num;

	if (ctype > AVC_CTYPE_GENERAL_INQUIRY)
		goto response;

	if (!l2cap_frame_get_be64(frame, &id))
		return false;

	print_field("%*cIdentifier: 0x%jx (%s)", (indent - 8), ' ',
					id, id ? "Reserved" : "PLAYING");

	if (!l2cap_frame_get_u8(frame, &num))
		return false;

	print_field("%*cAttributeCount: 0x%02x", (indent - 8), ' ', num);

	for (; num > 0; num--) {
		uint32_t attr;

		if (!l2cap_frame_get_be32(frame, &attr))
			return false;

		print_field("%*cAttribute: 0x%08x (%s)", (indent - 8),
					' ', attr, mediattr2str(attr));
	}

	return true;

response:
	switch (avctp_frame->pt) {
	case AVRCP_PACKET_TYPE_SINGLE:
	case AVRCP_PACKET_TYPE_START:
		if (!l2cap_frame_get_u8(frame, &num))
			return false;

		avrcp_continuing.num = num;
		print_field("%*cAttributeCount: 0x%02x", (indent - 8),
								' ', num);
		len--;
		break;
	case AVRCP_PACKET_TYPE_CONTINUING:
	case AVRCP_PACKET_TYPE_END:
		num = avrcp_continuing.num;

		if (avrcp_continuing.size > 0) {
			uint16_t size;

			if (avrcp_continuing.size > len) {
				size = len;
				avrcp_continuing.size -= len;
			} else {
				size = avrcp_continuing.size;
				avrcp_continuing.size = 0;
			}

			printf("ContinuingAttributeValue: ");
			for (; size > 0; size--) {
				uint8_t c;

				if (!l2cap_frame_get_u8(frame, &c))
					goto failed;

				printf("%1c", isprint(c) ? c : '.');
			}
			printf("\n");

			len -= size;
		}
		break;
	default:
		goto failed;
	}

	while (num > 0 && len > 0) {
		uint32_t attr;
		uint16_t charset, attrlen;

		if (!l2cap_frame_get_be32(frame, &attr))
			goto failed;

		print_field("%*cAttribute: 0x%08x (%s)", (indent - 8),
						' ', attr, mediattr2str(attr));

		if (!l2cap_frame_get_be16(frame, &charset))
			goto failed;

		print_field("%*cCharsetID: 0x%04x (%s)", (indent - 8),
				' ', charset, charset2str(charset));

		if (!l2cap_frame_get_be16(frame, &attrlen))
			goto failed;

		print_field("%*cAttributeValueLength: 0x%04x",
						(indent - 8), ' ', attrlen);

		len -= sizeof(attr) + sizeof(charset) + sizeof(attrlen);
		num--;

		print_field("%*cAttributeValue: ", (indent - 8), ' ');
		for (; attrlen > 0 && len > 0; attrlen--, len--) {
			uint8_t c;

			if (!l2cap_frame_get_u8(frame, &c))
				goto failed;

			printf("%1c", isprint(c) ? c : '.');
		}

		if (attrlen > 0)
			avrcp_continuing.size = attrlen;
	}

	avrcp_continuing.num = num;

	return true;

failed:
	avrcp_continuing.num = 0;
	avrcp_continuing.size = 0;
	return false;
}

Please have a look into it and if fine, I can put it on mailing list. Thanks!

Regards,
Vikram


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2014-09-16  8:53 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-12 13:16 [PATCH v2 1/2] Monitor: Modify design of AVRCP callback functions Vikrampal Yadav
2014-09-12 13:16 ` [PATCH v2 2/2] Monitor: Add AVRCP GetElementAttributes support Vikrampal Yadav
2014-09-15  8:30   ` Luiz Augusto von Dentz
2014-09-15  8:50     ` Luiz Augusto von Dentz
2014-09-15 10:32       ` Vikrampal
2014-09-16  8:53       ` Vikrampal
2014-09-15  8:51 ` [PATCH v2 1/2] Monitor: Modify design of AVRCP callback functions Luiz Augusto von Dentz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.