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); } Regards, Vikram -- 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
