Return-Path: MIME-Version: 1.0 In-Reply-To: <1440772352-7564-2-git-send-email-bharat.panda@samsung.com> References: <1440772352-7564-1-git-send-email-bharat.panda@samsung.com> <1440772352-7564-2-git-send-email-bharat.panda@samsung.com> Date: Mon, 31 Aug 2015 13:59:50 +0300 Message-ID: Subject: Re: [PATCH v1 2/2] audio/avrcp: Add GetFolderItems support From: Luiz Augusto von Dentz To: Bharat Panda Cc: "linux-bluetooth@vger.kernel.org" , cpgs@samsung.com Content-Type: text/plain; charset=UTF-8 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Bharat, On Fri, Aug 28, 2015 at 5:32 PM, Bharat Panda 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. > + > + 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. > void (*set_volume) (uint8_t volume, struct btd_device *dev, > void *user_data); > bool (*play) (void *user_data); > diff --git a/profiles/audio/media.c b/profiles/audio/media.c > index 106b18a..ba7662e 100644 > --- a/profiles/audio/media.c > +++ b/profiles/audio/media.c > @@ -1013,6 +1013,13 @@ static const char *get_setting(const char *key, void *user_data) > return g_hash_table_lookup(mp->settings, key); > } > > +static const char *get_player_name(void *user_data) > +{ > + struct media_player *mp = user_data; > + > + return mp->name; > +} > + > static void set_shuffle_setting(DBusMessageIter *iter, const char *value) > { > const char *key = "Shuffle"; > @@ -1273,6 +1280,7 @@ static struct avrcp_player_cb player_cb = { > .get_position = get_position, > .get_duration = get_duration, > .get_status = get_status, > + .get_player_name = get_player_name, > .set_volume = set_volume, > .play = play, > .stop = stop, > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Luiz Augusto von Dentz