RE: [PATCH v2 1/6] monitor: Add AVRCP ListPlayerApplicationSettingValues support

[Vikrampal <vikram.pal@samsung.com>]

Hi Luiz,

> -----Original Message-----
> From: Luiz Augusto von Dentz [mailto:luiz.dentz@gmail.com]
> Sent: Friday, August 22, 2014 4:05 PM
> To: Vikrampal
> Cc: linux-bluetooth@vger.kernel.org; Dmitry Kasatkin; cpgs@samsung.com
> Subject: Re: [PATCH v2 1/6] monitor: Add AVRCP
> ListPlayerApplicationSettingValues support
> 
> Hi Vikrampal,
> 
> On Fri, Aug 22, 2014 at 1:09 PM, Vikrampal <vikram.pal@samsung.com>
> wrote:
> > Hi Luiz,
> >
> >> -----Original Message-----
> >> From: Luiz Augusto von Dentz [mailto:luiz.dentz@gmail.com]
> >> Sent: Friday, August 22, 2014 3:32 PM
> >> To: Vikrampal
> >> Cc: linux-bluetooth@vger.kernel.org; Dmitry Kasatkin;
> >> cpgs@samsung.com
> >> 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@samsung.com>
> >> wrote:
> >> > Hi Luiz,
> >> >
> >> >> -----Original Message-----
> >> >> From: Luiz Augusto von Dentz [mailto:luiz.dentz@gmail.com]
> >> >> Sent: Friday, August 22, 2014 1:18 PM
> >> >> To: Vikrampal Yadav
> >> >> Cc: linux-bluetooth@vger.kernel.org; Dmitry Kasatkin;
> >> >> cpgs@samsung.com
> >> >> 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@samsung.com> 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

Yeah I saw your patch and it surely will make things simpler.
Thanks for your help!

Regards,
Vikram