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
