Re: [PATCH v1 2/2] audio/avrcp: Add GetFolderItems support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Bharat,

On Mon, Aug 31, 2015 at 2:32 PM, Bharat Bhusan Panda
<bharat.panda@xxxxxxxxxxx> wrote:
> Hi Luiz,
>
>> -----Original Message-----
>> From: linux-bluetooth-owner@xxxxxxxxxxxxxxx [mailto:linux-bluetooth-
>> owner@xxxxxxxxxxxxxxx] On Behalf Of Luiz Augusto von Dentz
>> Sent: Monday, August 31, 2015 4:30 PM
>> To: Bharat Panda
>> Cc: linux-bluetooth@xxxxxxxxxxxxxxx; cpgs@xxxxxxxxxxx
>> Subject: Re: [PATCH v1 2/2] audio/avrcp: Add GetFolderItems support
>>
>> Hi Bharat,
>>
>> On Fri, Aug 28, 2015 at 5:32 PM, Bharat Panda
>> <bharat.panda@xxxxxxxxxxx> wrote:
>> > Support added to handle Get Folder Items browsing PDU for media player
>> > scope in TG role.
>> >
>> > e.g.
>> >       AVCTP Browsing: Response: type 0x00 label 0 PID 0x110e
>> >         AVRCP: GetFolderItems: len 0x0030
>> >           Status: 0x04 (Success)
>> >           UIDCounter: 0x0000 (0)
>> >           NumOfItems: 0x0001 (1)
>> >           Item: 0x01 (Media Player)
>> >           Length: 0x0028 (40)
>> >           PlayerID: 0x0000 (0)
>> >           PlayerType: 0x0001 (Audio)
>> >           PlayerSubType: 0x00000001 (Audio Book)
>> >           PlayStatus: 0x01 (PLAYING)
>> >           Features: 0x0000000000b701e80200000000000000
>> >           CharsetID: 0x006a (UTF-8)
>> >           NameLength: 0x000c (12)
>> >           Name: SimplePlayer
>> > ---
>> >  profiles/audio/avrcp.c | 146
>> +++++++++++++++++++++++++++++++++++++++++++++++++
>> >  profiles/audio/avrcp.h |   1 +
>> >  profiles/audio/media.c |   8 +++
>> >  3 files changed, 155 insertions(+)
>> >
>> > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c index
>> > 76e89af..77b10f5 100644
>> > --- a/profiles/audio/avrcp.c
>> > +++ b/profiles/audio/avrcp.c
>> > @@ -138,6 +138,11 @@
>> >
>> >  #define AVRCP_BROWSING_TIMEOUT         1
>> >
>> > +#define SCOPE_MEDIA_PLAYER_LIST                0x00
>> > +#define SCOPE_MEDIA_PLAYER_VFS                 0x01
>> > +#define SCOPE_SEARCH                           0x02
>> > +#define SCOPE_NOW_PLAYING                      0x03
>>
>> All protocol defines should use AVRCP_ as prefix.
>>
>> > +
>> >  #if __BYTE_ORDER == __LITTLE_ENDIAN
>> >
>> >  struct avrcp_header {
>> > @@ -176,6 +181,30 @@ struct avrcp_browsing_header {  } __attribute__
>> > ((packed));  #define AVRCP_BROWSING_HEADER_LENGTH 3
>> >
>> > +struct get_folder_items_rsp {
>> > +       uint8_t status;
>> > +       uint16_t uid_counter;
>> > +       uint16_t num_items;
>> > +       uint8_t data[0];
>> > +} __attribute__ ((packed));
>> > +
>> > +struct folder_item {
>> > +       uint8_t type;
>> > +       uint16_t len;
>> > +       uint8_t data[0];
>> > +} __attribute__ ((packed));
>> > +
>> > +struct player_item {
>> > +       uint16_t player_id;
>> > +       uint8_t type;
>> > +       uint32_t subtype;
>> > +       uint8_t status;
>> > +       uint8_t features[16];
>> > +       uint16_t charset;
>> > +       uint16_t namelen;
>> > +       char name[0];
>> > +} __attribute__ ((packed));
>> > +
>> >  struct avrcp_server {
>> >         struct btd_adapter *adapter;
>> >         uint32_t tg_record_id;
>> > @@ -261,6 +290,18 @@ struct control_pdu_handler {  static GSList
>> > *servers = NULL;  static unsigned int avctp_id = 0;
>> >
>> > +/* Default feature bit mask for media player
>> > + * supporting Play, Stop, Pause, Rewind, fast forward,
>> > + * Forward, Backward, Vendor Unique, Basic Group Navigation,
>> > + * Advanced Control Player, Browsing, UIDs unique,
>> > + * OnlyBrowsableWhenAddressed
>> > + */
>> > +static const uint8_t features[16] = {
>> > +                               0x00, 0x00, 0x00, 0x00, 0x00,
>> > +                               0xB7, 0x01, 0xE8, 0x02, 0x00,
>> > +                               0x00, 0x00, 0x00, 0x00, 0x00,
>> > +                               0x00 };
>>
>> That is not what we currently support, we actually support almost all pass-
>> through commands (see avctp.c) but we don't have any support for
>> browsing, etc.
>>
>> >  /* Company IDs supported by this device */  static uint32_t
>> > company_ids[] = {
>> >         IEEEID_BTSIG,
>> > @@ -1821,11 +1862,116 @@ err_metadata:
>> >         return AVRCP_HEADER_LENGTH + 1;  }
>> >
>> > +static uint16_t player_get_uid_counter(struct avrcp_player *player) {
>> > +       if (!player)
>> > +               return 0x0000;
>> > +
>> > +       return player->uid_counter;
>> > +}
>> > +
>> > +static void avrcp_handle_get_folder_items(struct avrcp *session,
>> > +                               struct avrcp_browsing_header *pdu,
>> > +                               uint8_t transaction) {
>> > +       struct avrcp_player *player = session->target->player;
>> > +       struct get_folder_items_rsp *rsp;
>> > +       uint32_t start_item = 0;
>> > +       uint32_t end_item = 0;
>> > +       uint8_t scope;
>> > +       uint8_t status = AVRCP_STATUS_SUCCESS;
>> > +       const char *name = NULL;
>> > +       GSList *l;
>> > +
>> > +       if (!pdu || ntohs(pdu->param_len) < 10)
>> > +               goto failed;
>> > +
>> > +       scope = pdu->params[0];
>> > +       start_item = bt_get_be32(&pdu->params[1]);
>> > +       end_item = bt_get_be32(&pdu->params[5]);
>> > +
>> > +       DBG("scope 0x%02x start_item 0x%08x end_item 0x%08x", scope,
>> > +                               start_item, end_item);
>> > +
>> > +       if (end_item < start_item) {
>> > +               status = AVRCP_STATUS_INVALID_PARAM;
>> > +               goto failed;
>> > +       }
>> > +
>> > +       switch (scope) {
>> > +       case SCOPE_MEDIA_PLAYER_LIST:
>>
>> Lets create another function for each scope, so the following code should be
>> move to avrcp_handle_media_player_list.
>>
>> > +               rsp = (void *)pdu->params;
>> > +               rsp->status = AVRCP_STATUS_SUCCESS;
>> > +               rsp->uid_counter = htons(player_get_uid_counter(player));
>> > +               rsp->num_items = 0;
>> > +               pdu->param_len = sizeof(*rsp);
>> > +
>> > +               for (l = g_slist_nth(session->server->players, start_item);
>> > +                                               l; l = g_slist_next(l)) {
>> > +                       struct avrcp_player *player = l->data;
>> > +                       struct folder_item *folder;
>> > +                       struct player_item *item;
>> > +                       uint16_t namelen;
>> > +
>> > +                       if (rsp->num_items == (end_item - start_item) + 1)
>> > +                               break;
>> > +
>> > +                       folder = (void *)&pdu->params[pdu->param_len];
>> > +                       folder->type = 0x01; /* Media Player */
>> > +
>> > +                       pdu->param_len += sizeof(*folder);
>> > +
>> > +                       item = (void *)folder->data;
>> > +                       item->player_id = htons(player->id);
>> > +                       item->type = 0x01; /* Audio */
>> > +                       item->subtype = htonl(0x01); /* Audio Book */
>> > +                       item->status = player_get_status(player);
>> > +                       memset(item->features, 0xff, 7);
>>
>> This is invalid, first this would leave the remaining bytes unset which may not
>> be 0x00, but you should actually use features, this clearly indicate to me that
>> you have not tested this with PTS since it would probably not match the
>> value in PIXIT.
> I agree this is invalid memset should be used with proper features length, I will fix this this and submit v2 with all review comments fixed.
>
> Yes it was not tested with PTS, due to some PTS frequent crash issue during startup, But I have tested the functionality to get the player list using car-kit and MecApp AVRCP. Once the PTS issues is fixed, I will run the specific TCs and attach the result along with the patch details.
>
>>
>> > +
>> > +                       memcpy(&item->features, &features, sizeof(features));
>> > +                       item->charset = htons(AVRCP_CHARSET_UTF8);
>> > +
>> > +                       name = player->cb->get_player_name(player->user_data);
>> > +                       namelen = strlen(name);
>> > +                       item->namelen = htons(namelen);
>> > +                       memcpy(item->name, name, namelen);
>> > +
>> > +                       folder->len = htons(sizeof(*item) + namelen);
>> > +                       pdu->param_len += sizeof(*item) + namelen;
>> > +                       rsp->num_items++;
>> > +               }
>> > +
>> > +               /* If no player could be found respond with an error */
>> > +               if (!rsp->num_items) {
>> > +                       status = AVRCP_STATUS_OUT_OF_BOUNDS;
>> > +                       goto failed;
>> > +               }
>> > +
>> > +               rsp->num_items = htons(rsp->num_items);
>> > +               pdu->param_len = htons(pdu->param_len);
>> > +
>> > +               break;
>> > +       case SCOPE_MEDIA_PLAYER_VFS:
>> > +       case SCOPE_SEARCH:
>> > +       case SCOPE_NOW_PLAYING:
>> > +       default:
>> > +               status = AVRCP_STATUS_INVALID_PARAM;
>> > +               goto failed;
>> > +       }
>> > +
>> > +       return;
>> > +
>> > +failed:
>> > +       pdu->params[0] = status;
>> > +       pdu->param_len = htons(1);
>> > +}
>> > +
>> >  static struct browsing_pdu_handler {
>> >         uint8_t pdu_id;
>> >         void (*func) (struct avrcp *session, struct avrcp_browsing_header
>> *pdu,
>> >                                                         uint8_t
>> > transaction);  } browsing_handlers[] = {
>> > +               { AVRCP_GET_FOLDER_ITEMS,
>> > + avrcp_handle_get_folder_items },
>> >                 { },
>> >  };
>> >
>> > diff --git a/profiles/audio/avrcp.h b/profiles/audio/avrcp.h index
>> > a9aeb1a..fbd84ce 100644
>> > --- a/profiles/audio/avrcp.h
>> > +++ b/profiles/audio/avrcp.h
>> > @@ -93,6 +93,7 @@ struct avrcp_player_cb {
>> >         const char *(*get_status) (void *user_data);
>> >         uint32_t (*get_position) (void *user_data);
>> >         uint32_t (*get_duration) (void *user_data);
>> > +       const char *(*get_player_name) (void *user_data);
>>
>> Use get_name, the struct is already referring to player no need to use the
>> term again.
> "get_name" is already being used for getting the endpoint name in the same file media.c, so I used get_player_name.

You can probably rename it to get_sender, anyway internally you can
name the function as get_player_name but the callback can still be
just get_name.


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



[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux