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);
- 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;
}
+ /* 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);
}
--
1.9.1
Hi Vikram,
On Fri, Sep 12, 2014 at 3:00 PM, Vikrampal <[email protected]> wrote:
> Hi Luiz,
>
>> -----Original Message-----
>> From: Luiz Augusto von Dentz [mailto:[email protected]]
>> Sent: Friday, September 12, 2014 4:45 PM
>> To: Vikrampal
>> Cc: [email protected]; Dmitry Kasatkin; [email protected]
>> Subject: Re: [PATCH 1/2] Monitor: Modify design of AVRCP callback functions
>>
>> Hi Vikram,
>>
>> On Fri, Sep 12, 2014 at 1:25 PM, Vikrampal <[email protected]>
>> wrote:
>> > Hi Luiz,
>> >
>> >> -----Original Message-----
>> >> From: [email protected] [mailto:linux-bluetooth-
>> >> [email protected]] On Behalf Of Luiz Augusto von Dentz
>> >> Sent: Friday, September 12, 2014 3:37 PM
>> >> To: Vikrampal Yadav
>> >> Cc: [email protected]; Dmitry Kasatkin;
>> >> [email protected]
>> >> 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
>> >> <[email protected]> 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
>> >> [email protected] 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
Hi Luiz,
> -----Original Message-----
> From: Luiz Augusto von Dentz [mailto:[email protected]]
> Sent: Friday, September 12, 2014 4:45 PM
> To: Vikrampal
> Cc: [email protected]; Dmitry Kasatkin; [email protected]
> Subject: Re: [PATCH 1/2] Monitor: Modify design of AVRCP callback functions
>
> Hi Vikram,
>
> On Fri, Sep 12, 2014 at 1:25 PM, Vikrampal <[email protected]>
> wrote:
> > Hi Luiz,
> >
> >> -----Original Message-----
> >> From: [email protected] [mailto:linux-bluetooth-
> >> [email protected]] On Behalf Of Luiz Augusto von Dentz
> >> Sent: Friday, September 12, 2014 3:37 PM
> >> To: Vikrampal Yadav
> >> Cc: [email protected]; Dmitry Kasatkin;
> >> [email protected]
> >> 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
> >> <[email protected]> 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
> >> [email protected] 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
Hi Vikram,
On Fri, Sep 12, 2014 at 1:25 PM, Vikrampal <[email protected]> wrote:
> Hi Luiz,
>
>> -----Original Message-----
>> From: [email protected] [mailto:linux-bluetooth-
>> [email protected]] On Behalf Of Luiz Augusto von Dentz
>> Sent: Friday, September 12, 2014 3:37 PM
>> To: Vikrampal Yadav
>> Cc: [email protected]; Dmitry Kasatkin; [email protected]
>> 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
>> <[email protected]> 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 [email protected] More majordomo
>> info at http://vger.kernel.org/majordomo-info.html
>
> Regards,
> Vikram
>
--
Luiz Augusto von Dentz
Hi Luiz,
> -----Original Message-----
> From: [email protected] [mailto:linux-bluetooth-
> [email protected]] On Behalf Of Luiz Augusto von Dentz
> Sent: Friday, September 12, 2014 3:37 PM
> To: Vikrampal Yadav
> Cc: [email protected]; Dmitry Kasatkin; [email protected]
> 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
> <[email protected]> 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.
> 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 [email protected] More majordomo
> info at http://vger.kernel.org/majordomo-info.html
Regards,
Vikram
Hi Vikram,
On Thu, Sep 11, 2014 at 3:35 PM, Vikrampal Yadav <[email protected]> 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:
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,PREFER_PACKED,SPACING,FSF_MAILING_ADDRESS,TRAILING_STATEMENTS,RETURN_VOID,FILE_PATH_CHANGES
--show-types -
--
Luiz Augusto von Dentz
Support for decoding AVRCP GetElementAttributes added in Bluetooth
monitor.
---
monitor/avctp.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 157 insertions(+)
diff --git a/monitor/avctp.c b/monitor/avctp.c
index 41b6380..505cc5d 100644
--- a/monitor/avctp.c
+++ b/monitor/avctp.c
@@ -158,12 +158,27 @@
#define AVRCP_ATTRIBUTE_SHUFFLE 0x03
#define AVRCP_ATTRIBUTE_SCAN 0x04
+/* media attributes */
+#define AVRCP_MEDIA_ATTRIBUTE_ILLEGAL 0x00
+#define AVRCP_MEDIA_ATTRIBUTE_TITLE 0x01
+#define AVRCP_MEDIA_ATTRIBUTE_ARTIST 0x02
+#define AVRCP_MEDIA_ATTRIBUTE_ALBUM 0x03
+#define AVRCP_MEDIA_ATTRIBUTE_TRACK 0x04
+#define AVRCP_MEDIA_ATTRIBUTE_TOTAL 0x05
+#define AVRCP_MEDIA_ATTRIBUTE_GENRE 0x06
+#define AVRCP_MEDIA_ATTRIBUTE_DURATION 0x07
+
struct avctp_frame {
uint8_t hdr;
uint8_t pt;
struct l2cap_frame *l2cap_frame;
};
+static struct avrcp_continuing {
+ uint16_t num;
+ uint16_t size;
+} avrcp_continuing;
+
static const char *ctype2str(uint8_t ctype)
{
switch (ctype & 0x0f) {
@@ -523,6 +538,30 @@ static const char *charset2str(uint16_t charset)
}
}
+static const char *mediattr2str(uint32_t attr)
+{
+ switch (attr) {
+ case AVRCP_MEDIA_ATTRIBUTE_ILLEGAL:
+ return "Illegal";
+ case AVRCP_MEDIA_ATTRIBUTE_TITLE:
+ return "Title";
+ case AVRCP_MEDIA_ATTRIBUTE_ARTIST:
+ return "Artist";
+ case AVRCP_MEDIA_ATTRIBUTE_ALBUM:
+ return "Album";
+ case AVRCP_MEDIA_ATTRIBUTE_TRACK:
+ return "Track";
+ case AVRCP_MEDIA_ATTRIBUTE_TOTAL:
+ return "Track Total";
+ case AVRCP_MEDIA_ATTRIBUTE_GENRE:
+ return "Genre";
+ case AVRCP_MEDIA_ATTRIBUTE_DURATION:
+ return "Track duration";
+ default:
+ return "Reserved";
+ }
+}
+
static bool avrcp_passthrough_packet(struct avctp_frame *avctp_frame)
{
struct l2cap_frame *frame = avctp_frame->l2cap_frame;
@@ -904,6 +943,123 @@ static bool avrcp_displayable_charset(struct avctp_frame *avctp_frame,
return true;
}
+static bool avrcp_get_element_attributes(struct avctp_frame *avctp_frame,
+ uint8_t ctype, uint8_t len,
+ uint8_t indent)
+{
+ struct l2cap_frame *frame = avctp_frame->l2cap_frame;
+ uint64_t id;
+ uint8_t num;
+
+ if (ctype > AVC_CTYPE_GENERAL_INQUIRY)
+ goto response;
+
+ if (!l2cap_frame_get_be64(frame, &id))
+ return false;
+
+ print_field("%*cIdentifier: 0x%jx (%s)", (indent - 8), ' ',
+ id, id ? "Reserved" : "PLAYING");
+
+ if (!l2cap_frame_get_u8(frame, &num))
+ return false;
+
+ print_field("%*cAttributeCount: 0x%02x", (indent - 8), ' ', num);
+
+ for (; num > 0; num--) {
+ uint32_t attr;
+
+ if (!l2cap_frame_get_be32(frame, &attr))
+ return false;
+
+ print_field("%*cAttribute: 0x%08x (%s)", (indent - 8),
+ ' ', attr, mediattr2str(attr));
+ }
+
+ return true;
+
+response:
+ if (avctp_frame->pt == AVRCP_PACKET_TYPE_SINGLE
+ || avctp_frame->pt == AVRCP_PACKET_TYPE_START) {
+ if (!l2cap_frame_get_u8(frame, &num))
+ return false;
+
+ avrcp_continuing.num = num;
+ print_field("%*cAttributeCount: 0x%02x", (indent - 8),
+ ' ', num);
+ len--;
+ } else {
+ num = avrcp_continuing.num;
+
+ if (avrcp_continuing.size > 0) {
+ uint16_t size;
+
+ if (avrcp_continuing.size > len) {
+ size = len;
+ avrcp_continuing.size -= len;
+ } else {
+ size = avrcp_continuing.size;
+ avrcp_continuing.size = 0;
+ }
+
+ printf("ContinuingAttributeValue: ");
+ for (; size > 0; size--) {
+ uint8_t c;
+
+ if (!l2cap_frame_get_u8(frame, &c))
+ return false;
+
+ printf("%1c", isprint(c) ? c : '.');
+ }
+ printf("\n");
+
+ len -= size;
+ }
+ }
+
+ while (num > 0 && len > 0) {
+ uint32_t attr;
+ uint16_t charset, attrlen;
+
+ if (!l2cap_frame_get_be32(frame, &attr))
+ return false;
+
+ print_field("%*cAttribute: 0x%08x (%s)", (indent - 8),
+ ' ', attr, mediattr2str(attr));
+
+ if (!l2cap_frame_get_be16(frame, &charset))
+ return false;
+
+ print_field("%*cCharsetID: 0x%04x (%s)", (indent - 8),
+ ' ', charset, charset2str(charset));
+
+ if (!l2cap_frame_get_be16(frame, &attrlen))
+ return false;
+
+ print_field("%*cAttributeValueLength: 0x%04x",
+ (indent - 8), ' ', attrlen);
+
+ len -= sizeof(attr) + sizeof(charset) + sizeof(attrlen);
+ num--;
+
+ print_field("%*cAttributeValue: ", (indent - 8), ' ');
+ for (; attrlen > 0 && len > 0; attrlen--, len--) {
+ uint8_t c;
+
+ if (!l2cap_frame_get_u8(frame, &c))
+ return false;
+
+ printf("%1c", isprint(c) ? c : '.');
+ }
+
+ if (attrlen > 0)
+ avrcp_continuing.size = attrlen;
+ }
+
+ avrcp_continuing.num = num;
+
+ return true;
+}
+
struct avrcp_ctrl_pdu_data {
uint8_t pduid;
bool (*func) (struct avctp_frame *avctp_frame, uint8_t ctype,
@@ -919,6 +1075,7 @@ static const struct avrcp_ctrl_pdu_data avrcp_ctrl_pdu_table[] = {
{ 0x15, avrcp_get_player_attribute_text },
{ 0x16, avrcp_get_player_value_text },
{ 0x17, avrcp_displayable_charset },
+ { 0x20, avrcp_get_element_attributes },
{ }
};
--
1.9.1