Hi Vikram, On Fri, Sep 12, 2014 at 3:00 PM, Vikrampal <vikram.pal@xxxxxxxxxxx> wrote: > Hi Luiz, > >> -----Original Message----- >> From: Luiz Augusto von Dentz [mailto:luiz.dentz@xxxxxxxxx] >> Sent: Friday, September 12, 2014 4:45 PM >> To: Vikrampal >> Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Dmitry Kasatkin; cpgs@xxxxxxxxxxx >> Subject: Re: [PATCH 1/2] Monitor: Modify design of AVRCP callback functions >> >> Hi Vikram, >> >> On Fri, Sep 12, 2014 at 1:25 PM, Vikrampal <vikram.pal@xxxxxxxxxxx> >> wrote: >> > Hi Luiz, >> > >> >> -----Original Message----- >> >> From: linux-bluetooth-owner@xxxxxxxxxxxxxxx [mailto:linux-bluetooth- >> >> owner@xxxxxxxxxxxxxxx] On Behalf Of Luiz Augusto von Dentz >> >> Sent: Friday, September 12, 2014 3:37 PM >> >> To: Vikrampal Yadav >> >> Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Dmitry Kasatkin; >> >> cpgs@xxxxxxxxxxx >> >> Subject: Re: [PATCH 1/2] Monitor: Modify design of AVRCP callback >> >> functions >> >> >> >> Hi Vikram, >> >> >> >> On Thu, Sep 11, 2014 at 3:35 PM, Vikrampal Yadav >> >> <vikram.pal@xxxxxxxxxxx> wrote: >> >> > Modified the design of AVRCP callback functions. >> >> > --- >> >> > monitor/avctp.c | 94 >> >> > +++++++++++++++++++++++++++++++++++++++------------------ >> >> > 1 file changed, 65 insertions(+), 29 deletions(-) >> >> > >> >> > diff --git a/monitor/avctp.c b/monitor/avctp.c index >> >> > c7e242b..41b6380 >> >> > 100644 >> >> > --- a/monitor/avctp.c >> >> > +++ b/monitor/avctp.c >> >> > @@ -158,6 +158,12 @@ >> >> > #define AVRCP_ATTRIBUTE_SHUFFLE 0x03 >> >> > #define AVRCP_ATTRIBUTE_SCAN 0x04 >> >> > >> >> > +struct avctp_frame { >> >> > + uint8_t hdr; >> >> > + uint8_t pt; >> >> > + struct l2cap_frame *l2cap_frame; }; >> >> > + >> >> > static const char *ctype2str(uint8_t ctype) { >> >> > switch (ctype & 0x0f) { >> >> > @@ -517,15 +523,19 @@ static const char *charset2str(uint16_t >> charset) >> >> > } >> >> > } >> >> > >> >> > -static bool avrcp_passthrough_packet(struct l2cap_frame *frame) >> >> > +static bool avrcp_passthrough_packet(struct avctp_frame >> >> > +*avctp_frame) >> >> > { >> >> > + struct l2cap_frame *frame = avctp_frame->l2cap_frame; >> >> > + >> >> > packet_hexdump(frame->data, frame->size); >> >> > return true; >> >> > } >> >> > >> >> > -static bool avrcp_get_capabilities(struct l2cap_frame *frame, >> >> > uint8_t >> >> ctype, >> >> > - uint8_t len, uint8_t indent) >> >> > +static bool avrcp_get_capabilities(struct avctp_frame *avctp_frame, >> >> > + uint8_t ctype, uint8_t len, >> >> > + uint8_t indent) >> >> > { >> >> > + struct l2cap_frame *frame = avctp_frame->l2cap_frame; >> >> > uint8_t cap, count; >> >> > int i; >> >> > >> >> > @@ -576,10 +586,11 @@ static bool avrcp_get_capabilities(struct >> >> l2cap_frame *frame, uint8_t ctype, >> >> > return true; >> >> > } >> >> > >> >> > -static bool avrcp_list_player_attributes(struct l2cap_frame >> >> > *frame, >> >> > +static bool avrcp_list_player_attributes(struct avctp_frame >> >> > +*avctp_frame, >> >> > uint8_t ctype, uint8_t len, >> >> > uint8_t indent) { >> >> > + struct l2cap_frame *frame = avctp_frame->l2cap_frame; >> >> > uint8_t num; >> >> > int i; >> >> > >> >> > @@ -604,9 +615,11 @@ static bool >> >> > avrcp_list_player_attributes(struct >> >> l2cap_frame *frame, >> >> > return true; >> >> > } >> >> > >> >> > -static bool avrcp_list_player_values(struct l2cap_frame *frame, >> >> > uint8_t >> >> ctype, >> >> > - uint8_t len, uint8_t indent) >> >> > +static bool avrcp_list_player_values(struct avctp_frame *avctp_frame, >> >> > + uint8_t ctype, uint8_t len, >> >> > + uint8_t indent) >> >> > { >> >> > + struct l2cap_frame *frame = avctp_frame->l2cap_frame; >> >> > static uint8_t attr = 0; >> >> > uint8_t num; >> >> > >> >> > @@ -640,10 +653,11 @@ response: >> >> > return true; >> >> > } >> >> > >> >> > -static bool avrcp_get_current_player_value(struct l2cap_frame >> >> > *frame, >> >> > +static bool avrcp_get_current_player_value(struct avctp_frame >> >> > +*avctp_frame, >> >> > uint8_t ctype, uint8_t len, >> >> > uint8_t indent) { >> >> > + struct l2cap_frame *frame = avctp_frame->l2cap_frame; >> >> > uint8_t num; >> >> > >> >> > if (!l2cap_frame_get_u8(frame, &num)) @@ -688,9 +702,11 @@ >> >> > response: >> >> > return true; >> >> > } >> >> > >> >> > -static bool avrcp_set_player_value(struct l2cap_frame *frame, >> >> > uint8_t >> >> ctype, >> >> > - uint8_t len, uint8_t indent) >> >> > +static bool avrcp_set_player_value(struct avctp_frame *avctp_frame, >> >> > + uint8_t ctype, uint8_t len, >> >> > + uint8_t indent) >> >> > { >> >> > + struct l2cap_frame *frame = avctp_frame->l2cap_frame; >> >> > uint8_t num; >> >> > >> >> > if (ctype > AVC_CTYPE_GENERAL_INQUIRY) @@ -720,10 +736,11 >> >> > @@ static bool avrcp_set_player_value(struct l2cap_frame *frame, >> >> > uint8_t >> >> ctype, >> >> > return true; >> >> > } >> >> > >> >> > -static bool avrcp_get_player_attribute_text(struct l2cap_frame >> >> > *frame, >> >> > +static bool avrcp_get_player_attribute_text(struct avctp_frame >> >> > +*avctp_frame, >> >> > uint8_t ctype, uint8_t len, >> >> > uint8_t indent) { >> >> > + struct l2cap_frame *frame = avctp_frame->l2cap_frame; >> >> > uint8_t num; >> >> > >> >> > if (!l2cap_frame_get_u8(frame, &num)) @@ -783,10 +800,11 @@ >> >> > response: >> >> > return true; >> >> > } >> >> > >> >> > -static bool avrcp_get_player_value_text(struct l2cap_frame *frame, >> >> > +static bool avrcp_get_player_value_text(struct avctp_frame >> >> > +*avctp_frame, >> >> > uint8_t ctype, uint8_t len, >> >> > uint8_t indent) { >> >> > + struct l2cap_frame *frame = avctp_frame->l2cap_frame; >> >> > static uint8_t attr = 0; >> >> > uint8_t num; >> >> > >> >> > @@ -858,9 +876,11 @@ response: >> >> > return true; >> >> > } >> >> > >> >> > -static bool avrcp_displayable_charset(struct l2cap_frame *frame, >> >> > uint8_t >> >> ctype, >> >> > - uint8_t len, uint8_t indent) >> >> > +static bool avrcp_displayable_charset(struct avctp_frame >> *avctp_frame, >> >> > + uint8_t ctype, uint8_t len, >> >> > + uint8_t indent) >> >> > { >> >> > + struct l2cap_frame *frame = avctp_frame->l2cap_frame; >> >> > uint8_t num; >> >> > >> >> > if (ctype > AVC_CTYPE_GENERAL_INQUIRY) @@ -886,8 +906,8 @@ >> >> > static bool avrcp_displayable_charset(struct l2cap_frame *frame, >> >> > uint8_t ctype, >> >> > >> >> > struct avrcp_ctrl_pdu_data { >> >> > uint8_t pduid; >> >> > - bool (*func) (struct l2cap_frame *frame, uint8_t ctype, uint8_t len, >> >> > - uint8_t indent); >> >> > + bool (*func) (struct avctp_frame *avctp_frame, uint8_t ctype, >> >> > + uint8_t len, >> >> > + uint8_t indent); >> >> > }; >> >> > >> >> > static const struct avrcp_ctrl_pdu_data avrcp_ctrl_pdu_table[] = { >> >> > @@ >> >> > -915,9 +935,10 @@ static bool avrcp_rejected_packet(struct >> >> > l2cap_frame >> >> *frame, uint8_t indent) >> >> > return true; >> >> > } >> >> > >> >> > -static bool avrcp_pdu_packet(struct l2cap_frame *frame, uint8_t >> >> > ctype, >> >> > +static bool avrcp_pdu_packet(struct avctp_frame *avctp_frame, >> >> > +uint8_t ctype, >> >> > >> >> > uint8_t indent) { >> >> > + struct l2cap_frame *frame = avctp_frame->l2cap_frame; >> >> > uint8_t pduid, pt; >> >> > uint16_t len; >> >> > int i; >> >> > @@ -929,6 +950,9 @@ static bool avrcp_pdu_packet(struct >> l2cap_frame >> >> *frame, uint8_t ctype, >> >> > if (!l2cap_frame_get_u8(frame, &pt)) >> >> > return false; >> >> > >> >> > + /* For further use */ >> >> > + avctp_frame->pt = pt; >> >> > + >> >> > if (!l2cap_frame_get_be16(frame, &len)) >> >> > return false; >> >> > >> >> > @@ -953,11 +977,13 @@ static bool avrcp_pdu_packet(struct >> >> > l2cap_frame >> >> *frame, uint8_t ctype, >> >> > return true; >> >> > } >> >> > >> >> > - return ctrl_pdu_data->func(frame, ctype, len, indent + 2); >> >> > + return ctrl_pdu_data->func(avctp_frame, ctype, len, indent >> >> > + + 2); >> >> > } >> >> > >> >> > -static bool avrcp_control_packet(struct l2cap_frame *frame) >> >> > +static bool avrcp_control_packet(struct avctp_frame *avctp_frame) >> >> > { >> >> > + struct l2cap_frame *frame = avctp_frame->l2cap_frame; >> >> > + >> >> > uint8_t ctype, address, subunit, opcode, company[3], indent >> >> > = 2; >> >> > >> >> > if (!l2cap_frame_get_u8(frame, &ctype) || @@ -988,7 +1014,7 >> >> > @@ static bool avrcp_control_packet(struct l2cap_frame *frame) >> >> > >> >> > switch (opcode) { >> >> > case 0x7c: >> >> > - return avrcp_passthrough_packet(frame); >> >> > + return avrcp_passthrough_packet(avctp_frame); >> >> > case 0x00: >> >> > if (!l2cap_frame_get_u8(frame, &company[0]) || >> >> > !l2cap_frame_get_u8(frame, >> >> > &company[1]) || @@ -998,29 +1024,32 @@ static bool >> >> avrcp_control_packet(struct l2cap_frame *frame) >> >> > print_field("%*cCompany ID: 0x%02x%02x%02x", indent, ' ', >> >> > company[0], company[1], >> >> > company[2]); >> >> > >> >> > - return avrcp_pdu_packet(frame, ctype, 10); >> >> > + return avrcp_pdu_packet(avctp_frame, ctype, 10); >> >> > default: >> >> > packet_hexdump(frame->data, frame->size); >> >> > return true; >> >> > } >> >> > } >> >> > >> >> > -static bool avrcp_browsing_packet(struct l2cap_frame *frame, >> >> > uint8_t >> >> > hdr) >> >> > +static bool avrcp_browsing_packet(struct avctp_frame *avctp_frame) >> >> > { >> >> > + struct l2cap_frame *frame = avctp_frame->l2cap_frame; >> >> > + >> >> > packet_hexdump(frame->data, frame->size); >> >> > return true; >> >> > } >> >> > >> >> > -static void avrcp_packet(struct l2cap_frame *frame, uint8_t hdr) >> >> > +static void avrcp_packet(struct avctp_frame *avctp_frame) >> >> > { >> >> > + struct l2cap_frame *frame = avctp_frame->l2cap_frame; >> >> > bool ret; >> >> > >> >> > switch (frame->psm) { >> >> > case 0x17: >> >> > - ret = avrcp_control_packet(frame); >> >> > + ret = avrcp_control_packet(avctp_frame); >> >> > break; >> >> > case 0x1B: >> >> > - ret = avrcp_browsing_packet(frame, hdr); >> >> > + ret = avrcp_browsing_packet(avctp_frame); >> >> > break; >> >> > default: >> >> > packet_hexdump(frame->data, frame->size); @@ >> >> > -1037,18 >> >> > +1066,25 @@ void avctp_packet(const struct l2cap_frame *frame) { >> >> > uint8_t hdr; >> >> > uint16_t pid; >> >> > - struct l2cap_frame avctp_frame; >> >> > + struct l2cap_frame l2cap_frame; >> >> > + struct avctp_frame avctp_frame; >> >> > const char *pdu_color; >> >> > >> >> > - l2cap_frame_pull(&avctp_frame, frame, 0); >> >> > + l2cap_frame_pull(&l2cap_frame, frame, 0); >> >> >> >> I think it better to use only one variable, avctp_frame, and change >> >> the struct to actually have the data inline instead of a pointer: >> >> >> > >> > I couldn't understand as to how having data inline instead of a pointer is >> better here. >> > Having pointer is sufficient. Isn't it? Moreover, in function avctp_packet(), >> we are receiving a pointer only. >> >> It is one variable less in the stack and since you will be forwarding >> avctp_frame it will include l2cap_frame as well but it is not a pointer to a >> second variable in the stack, it is a small optimization but as I said we are >> always passing the avctp_frame around having a separate variable in the >> stack does not buy us anything. >> >> >> struct avctp_frame { >> >> uint8_t hdr; >> >> uint8_t pt; >> >> struct l2cap_frame l2cap_frame; /* note that is not a pointer */ } >> >> >> >> > >> >> > - if (!l2cap_frame_get_u8(&avctp_frame, &hdr) || >> >> > - !l2cap_frame_get_be16(&avctp_frame, &pid)) { >> >> > + /* For further use */ >> >> > + avctp_frame.l2cap_frame = &l2cap_frame; >> >> > + >> >> > + if (!l2cap_frame_get_u8(&l2cap_frame, &hdr) || >> >> > + !l2cap_frame_get_be16(&l2cap_frame, &pid)) >> >> > + { >> >> > print_text(COLOR_ERROR, "frame too short"); >> >> > packet_hexdump(frame->data, frame->size); >> >> > return; >> >> > } >> >> >> >> You should be able to use the fields from the struct directly and >> >> eliminate the user of extra variable here. >> >> >> >> > + /* For further use */ >> >> > + avctp_frame.hdr = hdr; >> >> > + >> >> > if (frame->in) >> >> > pdu_color = COLOR_MAGENTA; >> >> > else >> >> > @@ -1061,7 +1097,7 @@ void avctp_packet(const struct l2cap_frame >> >> *frame) >> >> > hdr & 0x0c, hdr >> 4, pid); >> >> > >> >> > if (pid == 0x110e || pid == 0x110c) >> >> > - avrcp_packet(&avctp_frame, hdr); >> >> > + avrcp_packet(&avctp_frame); >> >> > else >> >> > packet_hexdump(frame->data, frame->size); >> >> >> >> pid should probably be part of the avctp_frame as well. >> >> >> >> > } >> >> > -- >> >> > 1.9.1 >> >> >> >> Btw, this patch does not apply when checking with check patch: >> >> >> >> Applying: Monitor: Modify design of AVRCP callback functions >> >> bluez/.git/rebase-apply/patch:132: trailing whitespace. >> >> uint8_t ctype, uint8_t len, >> >> fatal: 1 line adds whitespace errors. >> >> >> >> Please add a hook to check patch, for example what I use is: >> >> >> >> exec git diff --cached | checkpatch.pl -q --no-signoff --ignore >> >> >> CAMELCASE,NEW_TYPEDEFS,INITIALISED_STATIC,GLOBAL_INITIALISERS,PREF >> >> >> ER_PACKED,SPACING,FSF_MAILING_ADDRESS,TRAILING_STATEMENTS,RETUR >> >> N_VOID,FILE_PATH_CHANGES >> >> --show-types - >> >> >> >> -- >> >> 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 >> > >> > Regards, >> > Vikram >> > >> >> >> >> -- >> Luiz Augusto von Dentz > > The structure becomes now: > struct avctp_frame { > uint8_t hdr; > uint8_t pt; > uint16_t pid; > struct l2cap_frame l2cap_frame; > }; > > And the new function: > > void avctp_packet(const struct l2cap_frame *frame) > { > struct l2cap_frame *l2cap_frame; > struct avctp_frame avctp_frame; > const char *pdu_color; > > l2cap_frame_pull(&avctp_frame.l2cap_frame, frame, 0); > > l2cap_frame = &avctp_frame.l2cap_frame; > > if (!l2cap_frame_get_u8(l2cap_frame, &avctp_frame.hdr) || > !l2cap_frame_get_be16(l2cap_frame, &avctp_frame.pid)) { > print_text(COLOR_ERROR, "frame too short"); > packet_hexdump(frame->data, frame->size); > return; > } > > if (frame->in) > pdu_color = COLOR_MAGENTA; > else > pdu_color = COLOR_BLUE; > > print_indent(6, pdu_color, "AVCTP", "", COLOR_OFF, > " %s: %s: type 0x%02x label %d PID 0x%04x", > frame->psm == 23 ? "Control" : "Browsing", > avctp_frame.hdr & 0x02 ? "Response" : "Command", > avctp_frame.hdr & 0x0c, avctp_frame.hdr >> 4, > avctp_frame.pid); > > if (avctp_frame.pid == 0x110e || avctp_frame.pid == 0x110c) > avrcp_packet(&avctp_frame); > else > packet_hexdump(frame->data, frame->size); > } This looks better, just make sure you fix the coding style as well. -- 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
