From: Vikrampal <vikram.pal@samsung.com>
To: 'Luiz Augusto von Dentz' <luiz.dentz@gmail.com>
Cc: linux-bluetooth@vger.kernel.org,
'Dmitry Kasatkin' <d.kasatkin@samsung.com>,
cpgs@samsung.com
Subject: RE: [PATCH v2 2/2] Monitor: Add AVRCP GetElementAttributes support
Date: Tue, 16 Sep 2014 14:23:15 +0530 [thread overview]
Message-ID: <012801cfd18b$a8103ed0$f830bc70$@samsung.com> (raw)
In-Reply-To:
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
next prev parent reply other threads:[~2014-09-16 8:53 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2014-09-15 8:51 ` [PATCH v2 1/2] Monitor: Modify design of AVRCP callback functions Luiz Augusto von Dentz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='012801cfd18b$a8103ed0$f830bc70$@samsung.com' \
--to=vikram.pal@samsung.com \
--cc=cpgs@samsung.com \
--cc=d.kasatkin@samsung.com \
--cc=linux-bluetooth@vger.kernel.org \
--cc=luiz.dentz@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.