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:
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
--
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
