RE: [PATCH v1 5/6] monitor/rfcomm: Add handlers for mcc frame type

[Gowtham Anandha Babu <gowtham.ab@samsung.com>]

Hi,

> -----Original Message-----
> From: Luiz Augusto von Dentz [mailto:luiz.dentz@gmail.com]
> Sent: Monday, November 03, 2014 9:02 PM
> To: Gowtham Anandha Babu
> Cc: linux-bluetooth@vger.kernel.org; Dmitry Kasatkin; Bharat Panda;
> cpgs@samsung.com
> Subject: Re: [PATCH v1 5/6] monitor/rfcomm: Add handlers for mcc frame
> type
> 
> Hi,
> 
> On Mon, Nov 3, 2014 at 5:24 PM, Gowtham Anandha Babu
> <gowtham.ab@samsung.com> wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: Luiz Augusto von Dentz [mailto:luiz.dentz@gmail.com]
> >> Sent: Monday, November 03, 2014 8:09 PM
> >> To: Gowtham Anandha Babu
> >> Cc: linux-bluetooth@vger.kernel.org; Dmitry Kasatkin; Bharat Panda;
> >> cpgs@samsung.com
> >> Subject: Re: [PATCH v1 5/6] monitor/rfcomm: Add handlers for mcc
> >> frame type
> >>
> >> Hi,
> >>
> >> On Mon, Nov 3, 2014 at 12:41 PM, Gowtham Anandha Babu
> >> <gowtham.ab@samsung.com> wrote:
> >> > From: Bharat Panda <bharat.panda@samsung.com>
> >> >
> >> > Changes made to decode different mcc frame type in RFCOMM for
> btmon.
> >> > ---
> >> >  monitor/rfcomm.c | 78
> >> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> -
> >> >  1 file changed, 76 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/monitor/rfcomm.c b/monitor/rfcomm.c index
> >> > a43b2a2..b85e8fd 100644
> >> > --- a/monitor/rfcomm.c
> >> > +++ b/monitor/rfcomm.c
> >> > @@ -90,6 +90,51 @@ static void print_rfcomm_hdr(const struct
> >> l2cap_frame *frame,
> >> >         print_field("FCS : (0x%2.2x)", fcs);  }
> >> >
> >> > +static inline void mcc_test(const struct l2cap_frame *frame,
> >> > +                               struct rfcomm_lhdr hdr, struct
> >> > +rfcomm_lmcc mcc) { }
> >> > +
> >> > +static inline void mcc_fcon(const struct l2cap_frame *frame,
> >> > +                               struct rfcomm_lhdr hdr, struct
> >> > +rfcomm_lmcc mcc) { }
> >> > +
> >> > +static inline void mcc_fcoff(const struct l2cap_frame *frame,
> >> > +                               struct rfcomm_lhdr hdr, struct
> >> > +rfcomm_lmcc mcc) { }
> >> > +
> >> > +static inline void mcc_msc(const struct l2cap_frame *frame,
> >> > +                               struct rfcomm_lhdr hdr, struct
> >> > +rfcomm_lmcc mcc) {
> >> > +       packet_hexdump(frame->data, frame->size); }
> >> > +
> >> > +static inline void mcc_rpn(const struct l2cap_frame *frame,
> >> > +                               struct rfcomm_lhdr hdr, struct
> >> > +rfcomm_lmcc mcc) {
> >> > +       packet_hexdump(frame->data, frame->size); }
> >> > +
> >> > +static inline void mcc_rls(const struct l2cap_frame *frame,
> >> > +                               struct rfcomm_lhdr hdr, struct
> >> > +rfcomm_lmcc mcc) {
> >> > +       packet_hexdump(frame->data, frame->size); }
> >> > +
> >> > +static inline void mcc_pn(const struct l2cap_frame *frame,
> >> > +                               struct rfcomm_lhdr hdr, struct
> >> > +rfcomm_lmcc mcc) {
> >> > +       packet_hexdump(frame->data, frame->size); }
> >> > +
> >> > +static inline void mcc_nsc(const struct l2cap_frame *frame,
> >> > +                               struct rfcomm_lhdr hdr, struct
> >> > +rfcomm_lmcc mcc) {
> >> > +       packet_hexdump(frame->data, frame->size); }
> >>
> >> Im fine if you want to add parsing functions upfront but they should
> >> do something useful other than just packet_hexdump otherwise don't
> >> bother, btw please add the output that this changes are producing in
> >> the description so we can more easily visualize the format you are
> proposing.
> >>
> >> >  static const char *type2str(uint8_t type)  {
> >> >         switch (type) {
> >> > @@ -141,8 +186,37 @@ static inline bool mcc_frame(const struct
> >> l2cap_frame *frame,
> >> >         print_indent(7, opcode_color, "RFCOMM(s): ", "", COLOR_OFF,
> >> > "%s
> >> %s",
> >> >                                         type2str(type),
> >> > CR_STR(mcc.type));
> >> >
> >> > -       print_rfcomm_hdr(&rfcomm_frame, hdr);
> >> > -       packet_hexdump(rfcomm_frame.data, rfcomm_frame.size);
> >> > +       switch (type) {
> >> > +       case RFCOMM_TEST:
> >> > +               mcc_test(&rfcomm_frame, hdr, mcc);
> >> > +               packet_hexdump(rfcomm_frame.data, rfcomm_frame.size);
> >> > +               break;
> >> > +       case RFCOMM_FCON:
> >> > +               mcc_fcon(&rfcomm_frame, hdr, mcc);
> >> > +               break;
> >> > +       case RFCOMM_FCOFF:
> >> > +               mcc_fcoff(&rfcomm_frame, hdr, mcc);
> >> > +               break;
> >> > +       case RFCOMM_MSC:
> >> > +               mcc_msc(&rfcomm_frame, hdr, mcc);
> >> > +               break;
> >> > +       case RFCOMM_RPN:
> >> > +               mcc_rpn(&rfcomm_frame, hdr, mcc);
> >> > +               break;
> >> > +       case RFCOMM_RLS:
> >> > +               mcc_rls(&rfcomm_frame, hdr, mcc);
> >> > +               break;
> >> > +       case RFCOMM_PN:
> >> > +               mcc_pn(&rfcomm_frame, hdr, mcc);
> >> > +               break;
> >> > +       case RFCOMM_NSC:
> >> > +               mcc_nsc(&rfcomm_frame, hdr, mcc);
> >> > +               break;
> >> > +       default:
> >> > +               print_field("MCC message type 0x%02x: ", type);
> >> > +               print_rfcomm_hdr(&rfcomm_frame, hdr);
> >> > +               packet_hexdump(rfcomm_frame.data, rfcomm_frame.size);
> >> > +       }
> >> >
> >> >         return true;
> >> >  }
> >> > --
> >> > 1.9.1
> >> >
> >> > --
> >> > To unsubscribe from this list: send the line "unsubscribe
> >> > linux-bluetooth" in the body of a message to
> >> > majordomo@vger.kernel.org More majordomo info at
> >> > http://vger.kernel.org/majordomo-info.html
> >>
> >>
> >>
> >> --
> >> Luiz Augusto von Dentz
> >
> > I added Andoird.mk in the corresponding patch and included the logs in the
> final patch and submitted v2 for the same. If you want the complete log, I am
> ready to send it.
> 
> I don't want the complete log just a single frame where we can see what the
> patch does.
> 
> > Btw we are almost done with the implementation for all the mcc frames. It
> will not be the hexdump kind of things.
> > Since it is an initial skeleton we proposed this kind of changes.
> 
> I would understand if you add a skeleton to public function not for static
> ones.
> 
> --
> Luiz Augusto von Dentz

Here the frame means, a common structure for rfcomm packets. Like as below:

Struct rfcomm_data {
	Struct rfcomm_lhdr hdr;
	Struct rfcomm_lmcc mcc;
};

Or just an explanation about all the frame in the patch description is enough?

You are expecting me to add skeleton only for public functions rather than static ones. Is that correct?

Regards,
Gowtham Anandha Babu