Hi Vikrampal, On Fri, Aug 22, 2014 at 1:09 PM, Vikrampal <vikram.pal@xxxxxxxxxxx> wrote: > Hi Luiz, > >> -----Original Message----- >> From: Luiz Augusto von Dentz [mailto:luiz.dentz@xxxxxxxxx] >> Sent: Friday, August 22, 2014 3:32 PM >> To: Vikrampal >> Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Dmitry Kasatkin; cpgs@xxxxxxxxxxx >> Subject: Re: [PATCH v2 1/6] monitor: Add AVRCP >> ListPlayerApplicationSettingValues support >> >> Hi Vikrampal, >> >> On Fri, Aug 22, 2014 at 11:27 AM, Vikrampal <vikram.pal@xxxxxxxxxxx> >> wrote: >> > Hi Luiz, >> > >> >> -----Original Message----- >> >> From: Luiz Augusto von Dentz [mailto:luiz.dentz@xxxxxxxxx] >> >> Sent: Friday, August 22, 2014 1:18 PM >> >> To: Vikrampal Yadav >> >> Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Dmitry Kasatkin; >> >> cpgs@xxxxxxxxxxx >> >> Subject: Re: [PATCH v2 1/6] monitor: Add AVRCP >> >> ListPlayerApplicationSettingValues support >> >> >> >> Hi Vikrampal, >> >> >> >> On Thu, Aug 21, 2014 at 1:30 PM, Vikrampal Yadav >> >> <vikram.pal@xxxxxxxxxxx> wrote: >> >> > Support for decoding AVRCP ListPlayerApplicationSettingValues >> >> > added in Bluetooth monitor. >> >> > --- >> >> > monitor/avctp.c | 79 >> >> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> >> > 1 file changed, 79 insertions(+) >> >> > >> >> > diff --git a/monitor/avctp.c b/monitor/avctp.c index >> >> > ec2adcd..35eca02 >> >> > 100644 >> >> > --- a/monitor/avctp.c >> >> > +++ b/monitor/avctp.c >> >> > @@ -429,6 +429,60 @@ static const char *attr2str(uint8_t attr) >> >> > } >> >> > } >> >> > >> >> > +static const char *value2str(uint8_t attr, uint8_t value) { >> >> > + switch (attr) { >> >> > + case AVRCP_ATTRIBUTE_ILEGAL: >> >> > + return "Illegal"; >> >> > + case AVRCP_ATTRIBUTE_EQUALIZER: >> >> > + switch (value) { >> >> > + case 0x01: >> >> > + return "OFF"; >> >> > + case 0x02: >> >> > + return "ON"; >> >> > + default: >> >> > + return "Reserved"; >> >> > + } >> >> > + case AVRCP_ATTRIBUTE_REPEAT_MODE: >> >> > + switch (value) { >> >> > + case 0x01: >> >> > + return "OFF"; >> >> > + case 0x02: >> >> > + return "Single Track Repeat"; >> >> > + case 0x03: >> >> > + return "All Track Repeat"; >> >> > + case 0x04: >> >> > + return "Group Repeat"; >> >> > + default: >> >> > + return "Reserved"; >> >> > + } >> >> > + case AVRCP_ATTRIBUTE_SHUFFLE: >> >> > + switch (value) { >> >> > + case 0x01: >> >> > + return "OFF"; >> >> > + case 0x02: >> >> > + return "All Track Suffle"; >> >> > + case 0x03: >> >> > + return "Group Suffle"; >> >> > + default: >> >> > + return "Reserved"; >> >> > + } >> >> > + case AVRCP_ATTRIBUTE_SCAN: >> >> > + switch (value) { >> >> > + case 0x01: >> >> > + return "OFF"; >> >> > + case 0x02: >> >> > + return "All Track Scan"; >> >> > + case 0x03: >> >> > + return "Group Scan"; >> >> > + default: >> >> > + return "Reserved"; >> >> > + } >> >> > + default: >> >> > + return "Unknown"; >> >> > + } >> >> > +} >> >> > + >> >> > static void avrcp_passthrough_packet(const struct l2cap_frame >> >> > *frame) { } @@ -504,6 +558,31 @@ static void >> >> > avrcp_list_player_values(const struct l2cap_frame *frame, >> >> > uint8_t ctype, uint8_t len, >> >> > uint8_t indent) { >> >> > + static uint8_t attr = 0; >> >> > + uint8_t num, i; >> >> > + >> >> > + if (len < 1) { >> >> > + print_text(COLOR_ERROR, "PDU malformed"); >> >> > + packet_hexdump(frame->data, frame->size); >> >> > + return; >> >> > + } >> >> > + >> >> > + if (ctype > AVC_CTYPE_GENERAL_INQUIRY) { >> >> > + num = *((uint8_t *) frame->data); >> >> > + print_field("%*cValueCount: 0x%02x", (indent - 8), ' >> >> > + ', num); >> >> > + >> >> > + for (i = 0; num > 0; num--, i++) { >> >> > + uint8_t value; >> >> > + >> >> > + value = *((uint8_t *) (frame->data + 1 + i)); >> >> > + print_field("%*cValueID: 0x%02x (%s)", (indent - 8), >> >> > + ' ', value, value2str(attr, value)); >> >> > + } >> >> > + } else { >> >> > + attr = *((uint8_t *) frame->data); >> >> > + print_field("%*cAttributeID: 0x%02x (%s)", (indent - 8), ' ', >> >> > + attr, attr2str(attr)); >> >> > + } >> >> > } >> >> >> >> What I suggested regarding the goto is valid for all the patches, >> >> also you seem to disregard my comment about advancing in the frame >> >> instead of an offset for the very begging of the frame. >> >> >> >> -- >> >> Luiz Augusto von Dentz >> > >> > I thought you suggested to use goto for deep indentations. So, >> > wherever the nesting is manageable, I chose to let it be as it is. >> > Otherwise at some places where indentations goes deep, I have made use >> of goto as suggested by you. >> >> The suggestion was to avoid nesting as much as possible and once you do for >> one function it probably makes sense to do for all functions to be consistent. >> >> > And for 2nd suggestion, I believe in any loop the loop index is an >> > important variable and we should use it wherever obvious pattern is >> > encountered in a loop. If understandability is a concern then I can >> > put proper comments explaining the intention. >> >> Well this is not really relevant, what Im suggesting is to move ahead in the >> frame using l2cap_frame_pull then the index can be used just to limit the >> amount of times you gonna pull, also note that usually you should check if >> there is still data remaining in the frame otherwise this code can access >> invalid data if the counter is invalid. >> >> -- >> Luiz Augusto von Dentz > > I'll do the changes accordingly and submit it again. Check out the patch Ive just sent, I believe with those function it will make things a lot simpler. -- Luiz Augusto von Dentz -- 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
