* [PATCH v2] monitor: Add AVRCP GetElementAttributes
@ 2014-09-09 9:10 Vikrampal Yadav
2014-09-10 9:11 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 5+ messages in thread
From: Vikrampal Yadav @ 2014-09-09 9:10 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 | 158 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
monitor/l2cap.h | 16 +++---
2 files changed, 167 insertions(+), 7 deletions(-)
diff --git a/monitor/avctp.c b/monitor/avctp.c
index c7e242b..89f0f9c 100644
--- a/monitor/avctp.c
+++ b/monitor/avctp.c
@@ -158,6 +158,21 @@
#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
+
+static struct avrcp_continuing {
+ uint16_t num;
+ uint16_t size;
+} avrcp_continuing;
+
static const char *ctype2str(uint8_t ctype)
{
switch (ctype & 0x0f) {
@@ -517,6 +532,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 l2cap_frame *frame)
{
packet_hexdump(frame->data, frame->size);
@@ -884,6 +923,122 @@ static bool avrcp_displayable_charset(struct l2cap_frame *frame, uint8_t ctype,
return true;
}
+static bool avrcp_get_element_attributes(struct l2cap_frame *frame,
+ uint8_t ctype, uint8_t len,
+ uint8_t indent)
+{
+ 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 (frame->pkt_type == AVRCP_PACKET_TYPE_SINGLE
+ || frame->pkt_type == 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 l2cap_frame *frame, uint8_t ctype, uint8_t len,
@@ -899,6 +1054,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 },
{ }
};
@@ -932,6 +1088,8 @@ static bool avrcp_pdu_packet(struct l2cap_frame *frame, uint8_t ctype,
if (!l2cap_frame_get_be16(frame, &len))
return false;
+ frame->pkt_type = pt;
+
print_indent(indent, COLOR_OFF, "AVRCP: ", pdu2str(pduid), COLOR_OFF,
" pt %s len 0x%04x", pt2str(pt), len);
diff --git a/monitor/l2cap.h b/monitor/l2cap.h
index 5faaea6..9ade06f 100644
--- a/monitor/l2cap.h
+++ b/monitor/l2cap.h
@@ -33,6 +33,7 @@ struct l2cap_frame {
uint16_t psm;
uint16_t chan;
uint8_t mode;
+ uint8_t pkt_type;
const void *data;
uint16_t size;
};
@@ -41,13 +42,14 @@ static inline void l2cap_frame_pull(struct l2cap_frame *frame,
const struct l2cap_frame *source, uint16_t len)
{
if (frame != source) {
- frame->index = source->index;
- frame->in = source->in;
- frame->handle = source->handle;
- frame->cid = source->cid;
- frame->psm = source->psm;
- frame->chan = source->chan;
- frame->mode = source->mode;
+ frame->index = source->index;
+ frame->in = source->in;
+ frame->handle = source->handle;
+ frame->cid = source->cid;
+ frame->psm = source->psm;
+ frame->chan = source->chan;
+ frame->mode = source->mode;
+ frame->pkt_type = source->pkt_type;
}
frame->data = source->data + len;
--
1.9.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v2] monitor: Add AVRCP GetElementAttributes
2014-09-09 9:10 [PATCH v2] monitor: Add AVRCP GetElementAttributes Vikrampal Yadav
@ 2014-09-10 9:11 ` Luiz Augusto von Dentz
2014-09-10 10:12 ` Vikrampal
0 siblings, 1 reply; 5+ messages in thread
From: Luiz Augusto von Dentz @ 2014-09-10 9:11 UTC (permalink / raw)
To: Vikrampal Yadav; +Cc: linux-bluetooth, Dmitry Kasatkin, cpgs
Hi Vikram,
On Tue, Sep 9, 2014 at 12:10 PM, Vikrampal Yadav <vikram.pal@samsung.com> wrote:
> Support for decoding AVRCP GetElementAttributes added in Bluetooth
> monitor.
> ---
> monitor/avctp.c | 158 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> monitor/l2cap.h | 16 +++---
> 2 files changed, 167 insertions(+), 7 deletions(-)
>
> diff --git a/monitor/avctp.c b/monitor/avctp.c
> index c7e242b..89f0f9c 100644
> --- a/monitor/avctp.c
> +++ b/monitor/avctp.c
> @@ -158,6 +158,21 @@
> #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
> +
> +static struct avrcp_continuing {
> + uint16_t num;
> + uint16_t size;
> +} avrcp_continuing;
> +
> static const char *ctype2str(uint8_t ctype)
> {
> switch (ctype & 0x0f) {
> @@ -517,6 +532,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 l2cap_frame *frame)
> {
> packet_hexdump(frame->data, frame->size);
> @@ -884,6 +923,122 @@ static bool avrcp_displayable_charset(struct l2cap_frame *frame, uint8_t ctype,
> return true;
> }
>
> +static bool avrcp_get_element_attributes(struct l2cap_frame *frame,
> + uint8_t ctype, uint8_t len,
> + uint8_t indent)
> +{
> + 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 (frame->pkt_type == AVRCP_PACKET_TYPE_SINGLE
> + || frame->pkt_type == 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 l2cap_frame *frame, uint8_t ctype, uint8_t len,
> @@ -899,6 +1054,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 },
> { }
> };
>
> @@ -932,6 +1088,8 @@ static bool avrcp_pdu_packet(struct l2cap_frame *frame, uint8_t ctype,
> if (!l2cap_frame_get_be16(frame, &len))
> return false;
>
> + frame->pkt_type = pt;
> +
This does not belong here, you should probably pass pt in the callback
not via frame, or even better have AVCTP frame representation which
points to the L2CAP frame that one can contain things AVCTP specifics
like pt.
> print_indent(indent, COLOR_OFF, "AVRCP: ", pdu2str(pduid), COLOR_OFF,
> " pt %s len 0x%04x", pt2str(pt), len);
>
> diff --git a/monitor/l2cap.h b/monitor/l2cap.h
> index 5faaea6..9ade06f 100644
> --- a/monitor/l2cap.h
> +++ b/monitor/l2cap.h
> @@ -33,6 +33,7 @@ struct l2cap_frame {
> uint16_t psm;
> uint16_t chan;
> uint8_t mode;
> + uint8_t pkt_type;
> const void *data;
> uint16_t size;
> };
> @@ -41,13 +42,14 @@ static inline void l2cap_frame_pull(struct l2cap_frame *frame,
> const struct l2cap_frame *source, uint16_t len)
> {
> if (frame != source) {
> - frame->index = source->index;
> - frame->in = source->in;
> - frame->handle = source->handle;
> - frame->cid = source->cid;
> - frame->psm = source->psm;
> - frame->chan = source->chan;
> - frame->mode = source->mode;
> + frame->index = source->index;
> + frame->in = source->in;
> + frame->handle = source->handle;
> + frame->cid = source->cid;
> + frame->psm = source->psm;
> + frame->chan = source->chan;
> + frame->mode = source->mode;
> + frame->pkt_type = source->pkt_type;
> }
>
> frame->data = source->data + len;
> --
> 1.9.1
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 5+ messages in thread* RE: [PATCH v2] monitor: Add AVRCP GetElementAttributes
2014-09-10 9:11 ` Luiz Augusto von Dentz
@ 2014-09-10 10:12 ` Vikrampal
2014-09-10 12:49 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 5+ messages in thread
From: Vikrampal @ 2014-09-10 10:12 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: Wednesday, September 10, 2014 2:42 PM
> To: Vikrampal Yadav
> Cc: linux-bluetooth@vger.kernel.org; Dmitry Kasatkin; cpgs@samsung.com
> Subject: Re: [PATCH v2] monitor: Add AVRCP GetElementAttributes
>
> Hi Vikram,
>
> On Tue, Sep 9, 2014 at 12:10 PM, Vikrampal Yadav
> <vikram.pal@samsung.com> wrote:
> > Support for decoding AVRCP GetElementAttributes added in Bluetooth
> > monitor.
> > ---
> > monitor/avctp.c | 158
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > monitor/l2cap.h | 16 +++---
> > 2 files changed, 167 insertions(+), 7 deletions(-)
> >
> > diff --git a/monitor/avctp.c b/monitor/avctp.c index c7e242b..89f0f9c
> > 100644
> > --- a/monitor/avctp.c
> > +++ b/monitor/avctp.c
> > @@ -158,6 +158,21 @@
> > #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
> > +
> > +static struct avrcp_continuing {
> > + uint16_t num;
> > + uint16_t size;
> > +} avrcp_continuing;
> > +
> > static const char *ctype2str(uint8_t ctype) {
> > switch (ctype & 0x0f) {
> > @@ -517,6 +532,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 l2cap_frame *frame) {
> > packet_hexdump(frame->data, frame->size); @@ -884,6 +923,122
> > @@ static bool avrcp_displayable_charset(struct l2cap_frame *frame,
> uint8_t ctype,
> > return true;
> > }
> >
> > +static bool avrcp_get_element_attributes(struct l2cap_frame *frame,
> > + uint8_t ctype, uint8_t len,
> > + uint8_t indent) {
> > + 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 (frame->pkt_type == AVRCP_PACKET_TYPE_SINGLE
> > + || frame->pkt_type == 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 l2cap_frame *frame, uint8_t ctype,
> > uint8_t len, @@ -899,6 +1054,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 },
> > { }
> > };
> >
> > @@ -932,6 +1088,8 @@ static bool avrcp_pdu_packet(struct l2cap_frame
> *frame, uint8_t ctype,
> > if (!l2cap_frame_get_be16(frame, &len))
> > return false;
> >
> > + frame->pkt_type = pt;
> > +
>
> This does not belong here, you should probably pass pt in the callback not
> via frame, or even better have AVCTP frame representation which points to
> the L2CAP frame that one can contain things AVCTP specifics like pt.
Currently, I see only this information to be saved. So, can we not declare a static
variable pkt_type in avctp.c. Later on, if more information need to be saved then
we can define a frame structure.
>
> > print_indent(indent, COLOR_OFF, "AVRCP: ", pdu2str(pduid),
> COLOR_OFF,
> > " pt %s len 0x%04x",
> > pt2str(pt), len);
> >
> > diff --git a/monitor/l2cap.h b/monitor/l2cap.h index 5faaea6..9ade06f
> > 100644
> > --- a/monitor/l2cap.h
> > +++ b/monitor/l2cap.h
> > @@ -33,6 +33,7 @@ struct l2cap_frame {
> > uint16_t psm;
> > uint16_t chan;
> > uint8_t mode;
> > + uint8_t pkt_type;
> > const void *data;
> > uint16_t size;
> > };
> > @@ -41,13 +42,14 @@ static inline void l2cap_frame_pull(struct
> l2cap_frame *frame,
> > const struct l2cap_frame *source,
> > uint16_t len) {
> > if (frame != source) {
> > - frame->index = source->index;
> > - frame->in = source->in;
> > - frame->handle = source->handle;
> > - frame->cid = source->cid;
> > - frame->psm = source->psm;
> > - frame->chan = source->chan;
> > - frame->mode = source->mode;
> > + frame->index = source->index;
> > + frame->in = source->in;
> > + frame->handle = source->handle;
> > + frame->cid = source->cid;
> > + frame->psm = source->psm;
> > + frame->chan = source->chan;
> > + frame->mode = source->mode;
> > + frame->pkt_type = source->pkt_type;
> > }
> >
> > frame->data = source->data + len;
> > --
> > 1.9.1
> >
>
>
>
> --
> Luiz Augusto von Dentz
Regards,
Vikram
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] monitor: Add AVRCP GetElementAttributes
2014-09-10 10:12 ` Vikrampal
@ 2014-09-10 12:49 ` Luiz Augusto von Dentz
2014-09-11 8:36 ` Vikrampal
0 siblings, 1 reply; 5+ messages in thread
From: Luiz Augusto von Dentz @ 2014-09-10 12:49 UTC (permalink / raw)
To: Vikrampal; +Cc: linux-bluetooth, Dmitry Kasatkin, cpgs
Hi Vikram,
On Wed, Sep 10, 2014 at 1:12 PM, Vikrampal <vikram.pal@samsung.com> wrote:
> Hi Luiz,
>
>> -----Original Message-----
>> From: Luiz Augusto von Dentz [mailto:luiz.dentz@gmail.com]
>> Sent: Wednesday, September 10, 2014 2:42 PM
>> To: Vikrampal Yadav
>> Cc: linux-bluetooth@vger.kernel.org; Dmitry Kasatkin; cpgs@samsung.com
>> Subject: Re: [PATCH v2] monitor: Add AVRCP GetElementAttributes
>>
>> Hi Vikram,
>>
>> On Tue, Sep 9, 2014 at 12:10 PM, Vikrampal Yadav
>> <vikram.pal@samsung.com> wrote:
>> > Support for decoding AVRCP GetElementAttributes added in Bluetooth
>> > monitor.
>> > ---
>> > monitor/avctp.c | 158
>> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> > monitor/l2cap.h | 16 +++---
>> > 2 files changed, 167 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/monitor/avctp.c b/monitor/avctp.c index c7e242b..89f0f9c
>> > 100644
>> > --- a/monitor/avctp.c
>> > +++ b/monitor/avctp.c
>> > @@ -158,6 +158,21 @@
>> > #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
>> > +
>> > +static struct avrcp_continuing {
>> > + uint16_t num;
>> > + uint16_t size;
>> > +} avrcp_continuing;
>> > +
>> > static const char *ctype2str(uint8_t ctype) {
>> > switch (ctype & 0x0f) {
>> > @@ -517,6 +532,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 l2cap_frame *frame) {
>> > packet_hexdump(frame->data, frame->size); @@ -884,6 +923,122
>> > @@ static bool avrcp_displayable_charset(struct l2cap_frame *frame,
>> uint8_t ctype,
>> > return true;
>> > }
>> >
>> > +static bool avrcp_get_element_attributes(struct l2cap_frame *frame,
>> > + uint8_t ctype, uint8_t len,
>> > + uint8_t indent) {
>> > + 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 (frame->pkt_type == AVRCP_PACKET_TYPE_SINGLE
>> > + || frame->pkt_type == 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 l2cap_frame *frame, uint8_t ctype,
>> > uint8_t len, @@ -899,6 +1054,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 },
>> > { }
>> > };
>> >
>> > @@ -932,6 +1088,8 @@ static bool avrcp_pdu_packet(struct l2cap_frame
>> *frame, uint8_t ctype,
>> > if (!l2cap_frame_get_be16(frame, &len))
>> > return false;
>> >
>> > + frame->pkt_type = pt;
>> > +
>>
>> This does not belong here, you should probably pass pt in the callback not
>> via frame, or even better have AVCTP frame representation which points to
>> the L2CAP frame that one can contain things AVCTP specifics like pt.
>
> Currently, I see only this information to be saved. So, can we not declare a static
> variable pkt_type in avctp.c. Later on, if more information need to be saved then
> we can define a frame structure.
I would probably pass pt to avrcp_packet to be forward to the callback
just as you did with hdr, but I still think and AVCTP frame
representation would be better as it can grow without always having to
change the callbacks in the pdu table e.g.:
struct avctp_frame {
uint8_t hdr;
uint8_t pt;
struct l2cap_frame l2cap_frame;
};
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 5+ messages in thread* RE: [PATCH v2] monitor: Add AVRCP GetElementAttributes
2014-09-10 12:49 ` Luiz Augusto von Dentz
@ 2014-09-11 8:36 ` Vikrampal
0 siblings, 0 replies; 5+ messages in thread
From: Vikrampal @ 2014-09-11 8:36 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: Wednesday, September 10, 2014 6:19 PM
> To: Vikrampal
> Cc: linux-bluetooth@vger.kernel.org; Dmitry Kasatkin; cpgs@samsung.com
> Subject: Re: [PATCH v2] monitor: Add AVRCP GetElementAttributes
>
> Hi Vikram,
>
> On Wed, Sep 10, 2014 at 1:12 PM, Vikrampal <vikram.pal@samsung.com>
> wrote:
> > Hi Luiz,
> >
> >> -----Original Message-----
> >> From: Luiz Augusto von Dentz [mailto:luiz.dentz@gmail.com]
> >> Sent: Wednesday, September 10, 2014 2:42 PM
> >> To: Vikrampal Yadav
> >> Cc: linux-bluetooth@vger.kernel.org; Dmitry Kasatkin;
> >> cpgs@samsung.com
> >> Subject: Re: [PATCH v2] monitor: Add AVRCP GetElementAttributes
> >>
> >> Hi Vikram,
> >>
> >> On Tue, Sep 9, 2014 at 12:10 PM, Vikrampal Yadav
> >> <vikram.pal@samsung.com> wrote:
> >> > Support for decoding AVRCP GetElementAttributes added in Bluetooth
> >> > monitor.
> >> > ---
> >> > monitor/avctp.c | 158
> >> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> > monitor/l2cap.h | 16 +++---
> >> > 2 files changed, 167 insertions(+), 7 deletions(-)
> >> >
> >> > diff --git a/monitor/avctp.c b/monitor/avctp.c index
> >> > c7e242b..89f0f9c
> >> > 100644
> >> > --- a/monitor/avctp.c
> >> > +++ b/monitor/avctp.c
> >> > @@ -158,6 +158,21 @@
> >> > #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
> >> > +
> >> > +static struct avrcp_continuing {
> >> > + uint16_t num;
> >> > + uint16_t size;
> >> > +} avrcp_continuing;
> >> > +
> >> > static const char *ctype2str(uint8_t ctype) {
> >> > switch (ctype & 0x0f) {
> >> > @@ -517,6 +532,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 l2cap_frame *frame) {
> >> > packet_hexdump(frame->data, frame->size); @@ -884,6
> >> > +923,122 @@ static bool avrcp_displayable_charset(struct
> >> > l2cap_frame *frame,
> >> uint8_t ctype,
> >> > return true;
> >> > }
> >> >
> >> > +static bool avrcp_get_element_attributes(struct l2cap_frame *frame,
> >> > + uint8_t ctype, uint8_t len,
> >> > + uint8_t indent) {
> >> > + 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 (frame->pkt_type == AVRCP_PACKET_TYPE_SINGLE
> >> > + || frame->pkt_type == 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 l2cap_frame *frame, uint8_t ctype,
> >> > uint8_t len, @@ -899,6 +1054,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 },
> >> > { }
> >> > };
> >> >
> >> > @@ -932,6 +1088,8 @@ static bool avrcp_pdu_packet(struct
> >> > l2cap_frame
> >> *frame, uint8_t ctype,
> >> > if (!l2cap_frame_get_be16(frame, &len))
> >> > return false;
> >> >
> >> > + frame->pkt_type = pt;
> >> > +
> >>
> >> This does not belong here, you should probably pass pt in the
> >> callback not via frame, or even better have AVCTP frame
> >> representation which points to the L2CAP frame that one can contain
> things AVCTP specifics like pt.
> >
> > Currently, I see only this information to be saved. So, can we not
> > declare a static variable pkt_type in avctp.c. Later on, if more
> > information need to be saved then we can define a frame structure.
>
> I would probably pass pt to avrcp_packet to be forward to the callback just as
> you did with hdr, but I still think and AVCTP frame representation would be
> better as it can grow without always having to change the callbacks in the
> pdu table e.g.:
>
> struct avctp_frame {
> uint8_t hdr;
> uint8_t pt;
> struct l2cap_frame l2cap_frame;
> };
>
> --
> Luiz Augusto von Dentz
I agree with your suggestion. I'll make the necessary changes in design. Thanks!
Regards,
Vikram
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-09-11 8:36 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-09 9:10 [PATCH v2] monitor: Add AVRCP GetElementAttributes Vikrampal Yadav
2014-09-10 9:11 ` Luiz Augusto von Dentz
2014-09-10 10:12 ` Vikrampal
2014-09-10 12:49 ` Luiz Augusto von Dentz
2014-09-11 8:36 ` Vikrampal
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.