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 -- 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
