Hi Luiz, > -----Original Message----- > From: Vikrampal [mailto:vikram.pal@xxxxxxxxxxx] > Sent: Monday, September 15, 2014 4:03 PM > To: 'Luiz Augusto von Dentz' > Cc: 'linux-bluetooth@xxxxxxxxxxxxxxx'; 'Dmitry Kasatkin'; > 'cpgs@xxxxxxxxxxx' > Subject: RE: [PATCH v2 2/2] Monitor: Add AVRCP GetElementAttributes > support > > Hi Luiz, > > > -----Original Message----- > > From: Luiz Augusto von Dentz [mailto:luiz.dentz@xxxxxxxxx] > > Sent: Monday, September 15, 2014 2:21 PM > > To: Vikrampal Yadav > > Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Dmitry Kasatkin; cpgs@xxxxxxxxxxx > > 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@xxxxxxxxx> wrote: > > > Hi Vikram, > > > > > > On Fri, Sep 12, 2014 at 4:16 PM, Vikrampal Yadav > > <vikram.pal@xxxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
