* Re: [PATCHv2 01/23] android/hal-gatt-api: Add missing opcodes in GATT Service
From: Marcel Holtmann @ 2014-02-26 8:51 UTC (permalink / raw)
To: Jakub Tyszkowski; +Cc: linux-bluetooth
In-Reply-To: <1393404081-1401-2-git-send-email-jakub.tyszkowski@tieto.com>
Hi Jakub,
> Add missing Listen and Set Advertising Data opcodes and reorder them as
> they appear in HAL's headers.
I left this out originally since they are only useful for when we are a peripheral. Curious to know if Android APIs actually support this.
GATT client and server are valid for central role. And the connect/disconnect need to be turned into being one pair of commands. No point in duplicating them. There is really no difference between a GATT client and server connect. The central will connect to the peripheral. Unless Android got fully confused and uses the APIs wrongly.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 2/3] android/avrcp: Add control handlers to avrcp-lib
From: Andrei Emeltchenko @ 2014-02-26 10:28 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <CABBYNZJFHyX0FKhEe0mYtUUodPP-TYyP98yyV=L5Gx6OUcO8hw@mail.gmail.com>
Hi Luiz,
On Wed, Feb 26, 2014 at 10:16:15AM +0200, Luiz Augusto von Dentz wrote:
> Hi Andrei,
>
> On Wed, Feb 26, 2014 at 9:56 AM, Andrei Emeltchenko
> <Andrei.Emeltchenko.news@gmail.com> wrote:
> > Hi Luiz,
> >
> > On Tue, Feb 25, 2014 at 04:19:53PM +0200, Luiz Augusto von Dentz wrote:
> >> Hi Andrei,
> >>
> >> On Tue, Feb 25, 2014 at 3:56 PM, Andrei Emeltchenko
> >> <Andrei.Emeltchenko.news@gmail.com> wrote:
> >> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> >> >
> >> > ---
> >> > android/avrcp-lib.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> > android/avrcp-lib.h | 12 +++++++++++
> >> > 2 files changed, 72 insertions(+)
> >> >
> >> > diff --git a/android/avrcp-lib.c b/android/avrcp-lib.c
> >> > index 136801e..95e10f2 100644
> >> > --- a/android/avrcp-lib.c
> >> > +++ b/android/avrcp-lib.c
> >> > @@ -56,6 +56,15 @@
> >> > #define AVRCP_PACKET_TYPE_CONTINUING 0x02
> >> > #define AVRCP_PACKET_TYPE_END 0x03
> >> >
> >> > +/* Capabilities for AVRCP_GET_CAPABILITIES pdu */
> >> > +#define CAP_COMPANY_ID 0x02
> >> > +#define CAP_EVENTS_SUPPORTED 0x03
> >> > +
> >> > +/* Company IDs supported by this device */
> >> > +static uint32_t company_ids[] = {
> >> > + IEEEID_BTSIG,
> >> > +};
> >> > +
> >> > #if __BYTE_ORDER == __LITTLE_ENDIAN
> >> >
> >> > struct avrcp_header {
> >> > @@ -108,6 +117,8 @@ struct avrcp {
> >> > const struct avrcp_passthrough_handler *passthrough_handlers;
> >> > void *passthrough_data;
> >> > unsigned int passthrough_id;
> >> > +
> >> > + uint16_t supported_events;
> >> > };
> >> >
> >> > void avrcp_shutdown(struct avrcp *session)
> >> > @@ -220,6 +231,53 @@ static void set_company_id(uint8_t cid[3], const uint32_t cid_in)
> >> > cid[2] = cid_in;
> >> > }
> >> >
> >> > +static uint8_t avrcp_handle_get_capabilities(struct avrcp *session,
> >> > + uint8_t transaction, uint16_t *params_len,
> >> > + uint8_t *params, void *user_data)
> >> > +{
> >> > + unsigned int i;
> >> > +
> >> > + DBG("id %d params_len %d", params[0], *params_len);
> >> > +
> >> > + if (*params_len != 1)
> >> > + goto fail;
> >> > +
> >> > + switch (params[0]) {
> >> > + case CAP_COMPANY_ID:
> >> > + for (i = 0; i < G_N_ELEMENTS(company_ids); i++)
> >> > + set_company_id(¶ms[2 + i * 3], company_ids[i]);
> >> > +
> >> > + *params_len = 2 + (3 * G_N_ELEMENTS(company_ids));
> >> > + params[1] = G_N_ELEMENTS(company_ids);
> >> > +
> >> > + return AVC_CTYPE_STABLE;
> >> > + case CAP_EVENTS_SUPPORTED:
> >> > + params[1] = 0;
> >> > + for (i = 1; i <= AVRCP_EVENT_LAST; i++) {
> >> > + if (session->supported_events & (1 << i)) {
> >> > + params[1]++;
> >> > + params[params[1] + 1] = i;
> >> > + }
> >> > + }
> >> > +
> >> > + *params_len = 2 + params[1];
> >> > +
> >> > + return AVC_CTYPE_STABLE;
> >> > + }
> >> > +
> >> > +fail:
> >> > + *params_len = htons(1);
> >> > + params[0] = AVRCP_STATUS_INVALID_PARAM;
> >> > +
> >> > + return AVC_CTYPE_REJECTED;
> >> > +}
> >> > +
> >> > +static const struct avrcp_control_handler control_handlers[] = {
> >> > + { AVRCP_GET_CAPABILITIES, AVC_CTYPE_STATUS,
> >> > + avrcp_handle_get_capabilities },
> >> > + { },
> >> > +};
> >> > +
> >> > struct avrcp *avrcp_new(int fd, size_t imtu, size_t omtu, uint16_t version)
> >> > {
> >> > struct avrcp *session;
> >> > @@ -241,6 +299,8 @@ struct avrcp *avrcp_new(int fd, size_t imtu, size_t omtu, uint16_t version)
> >> > handle_vendordep_pdu,
> >> > session);
> >> >
> >> > + avrcp_set_control_handlers(session, control_handlers, NULL);
> >> > +
> >> > return session;
> >> > }
> >> >
> >> > diff --git a/android/avrcp-lib.h b/android/avrcp-lib.h
> >> > index 4f3a632..0821287 100644
> >> > --- a/android/avrcp-lib.h
> >> > +++ b/android/avrcp-lib.h
> >> > @@ -46,6 +46,18 @@
> >> > #define AVRCP_ADD_TO_NOW_PLAYING 0x90
> >> > #define AVRCP_GENERAL_REJECT 0xA0
> >> >
> >> > +/* Notification events */
> >> > +#define AVRCP_EVENT_STATUS_CHANGED 0x01
> >> > +#define AVRCP_EVENT_TRACK_CHANGED 0x02
> >> > +#define AVRCP_EVENT_TRACK_REACHED_END 0x03
> >> > +#define AVRCP_EVENT_TRACK_REACHED_START 0x04
> >> > +#define AVRCP_EVENT_SETTINGS_CHANGED 0x08
> >> > +#define AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED 0x0a
> >> > +#define AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED 0x0b
> >> > +#define AVRCP_EVENT_UIDS_CHANGED 0x0c
> >> > +#define AVRCP_EVENT_VOLUME_CHANGED 0x0d
> >> > +#define AVRCP_EVENT_LAST AVRCP_EVENT_VOLUME_CHANGED
> >> > +
> >> > struct avrcp;
> >> >
> >> > struct avrcp_control_handler {
> >> > --
> >> > 1.8.3.2
> >>
> >> That is the actual AVRCP implementation not the library, the library
> >> only offer means to handle the commands but don't parse it there since
> >> we can't do anything with it. For unit tests you can implement dummy
> >> handlers as we did for passthrough.
> >
> > I can implement dummy handlers but this would mean that we are testing
> > that dummy handlers. Is the idea to test actual production code?
>
> Which is fine, the capabilities will anyway depend on the underline
> player implementation so a dummy one is fine here, in fact the unit
> tests should not care what is the capabilities just that the PDU is
> formed correctly.
OK, what about tests which sends this request, test TP/CFG/BV-01-C.
Best regards
Andrei Emeltchenko
^ permalink raw reply
* Re: [PATCH 10/17] emulator: Add handling inquiry_lenght from inquiry command
From: Lukasz Rymanowski @ 2014-02-26 10:32 UTC (permalink / raw)
To: linux-bluetooth@vger.kernel.org; +Cc: Lukasz Rymanowski
In-Reply-To: <1393379848-4031-11-git-send-email-lukasz.rymanowski@tieto.com>
Hi,
On 26 February 2014 02:57, Lukasz Rymanowski
<lukasz.rymanowski@tieto.com> wrote:
> ---
> emulator/btdev.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/emulator/btdev.c b/emulator/btdev.c
> index c74258b..bf0c9d2 100644
> --- a/emulator/btdev.c
> +++ b/emulator/btdev.c
> @@ -129,8 +129,10 @@ struct btdev {
> struct inquiry_data {
> struct btdev *btdev;
> int num_resp;
> + int max_iter; /* Calculated from inquiry_lenght */
>
> int sent_count;
> + int iter;
> };
>
> #define DEFAULT_INQUIRY_INTERVAL 2 /* 2 miliseconds */
> @@ -778,8 +780,12 @@ static bool inquiry_callback(void *user_data)
> if (data->num_resp && data->sent_count == data->num_resp)
> goto finish;
>
> + /* Check if we already spent as much time as required */
> + if (data->max_iter && data->iter++ == data->max_iter)
> + goto finish;
> +
> if (i == MAX_BTDEV_ENTRIES) {
> - if (!data->num_resp)
> + if (!data->max_iter && !data->num_resp)
> goto finish;
>
> /* Reset main iterator so we can start iterate btdev_list
> @@ -826,6 +832,7 @@ static void inquiry_cmd(struct btdev *btdev, const void *cmd)
> const struct bt_hci_cmd_inquiry *inq_cmd = cmd;
> struct inquiry_data *data;
> struct bt_hci_evt_inquiry_complete ic;
> + unsigned int inquiry_len_ms = 128 * inq_cmd->length;
>
> if (btdev->inquiry_id)
> return;
> @@ -838,6 +845,9 @@ static void inquiry_cmd(struct btdev *btdev, const void *cmd)
> data->btdev = btdev;
> data->num_resp = inq_cmd->num_resp;
>
> + if (inquiry_len_ms)
> + data->max_iter = inquiry_len_ms / DEFAULT_INQUIRY_INTERVAL;
> +
> btdev->inquiry_id = timeout_add(DEFAULT_INQUIRY_INTERVAL,
> inquiry_callback, data, inquiry_destroy);
> /* Return if success */
> --
> 1.8.4
>
Do not review this one, I will change the approach here
\Lukasz
^ permalink raw reply
* Re: [PATCHv2 01/23] android/hal-gatt-api: Add missing opcodes in GATT Service
From: Andrzej Kaczmarek @ 2014-02-26 11:34 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Jakub Tyszkowski, linux-bluetooth
In-Reply-To: <63DA7E43-4B20-4CF9-8539-3BBE1A7EBD21@holtmann.org>
Hi Marcel,
On 26 February 2014 09:51, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Jakub,
>
>> Add missing Listen and Set Advertising Data opcodes and reorder them as
>> they appear in HAL's headers.
>
> I left this out originally since they are only useful for when we are a p=
eripheral. Curious to know if Android APIs actually support this.
It does, but not as public API - internal apps can still use them but
only if platform has config_bluetooth_le_peripheral_mode_supported
flag set, so perhaps this is for some gadgets running Android.
> GATT client and server are valid for central role. And the connect/discon=
nect need to be turned into being one pair of commands. No point in duplica=
ting them. There is really no difference between a GATT client and server c=
onnect. The central will connect to the peripheral. Unless Android got full=
y confused and uses the APIs wrongly.
These commands are separated for client and server since they have
either client or server app id as parameter which is later used to
dispatch callbacks to proper application. So using one pair of
commands (and single notification) we can still figure out which
callback to use in HAL as long as we have 'type' parameter for app id.
BR,
Andrzej
^ permalink raw reply
* Re: [PATCH 1/3] unit/avrcp: Add /TP/CFG/BV-01-C test
From: Luiz Augusto von Dentz @ 2014-02-26 11:59 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <1393336605-2467-1-git-send-email-Andrei.Emeltchenko.news@gmail.com>
Hi Andrei,
On Tue, Feb 25, 2014 at 3:56 PM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> Test verifies that the get capabilities command issued from the
> Controller. For that we add the command to avrcp-lib and it will be used
> in AVRCP later.
> ---
> android/avrcp-lib.c | 40 ++++++++++++++++++++++++++++++++++++++++
> android/avrcp-lib.h | 2 ++
> android/avrcp.c | 1 +
> unit/test-avrcp.c | 17 +++++++++++++++++
> 4 files changed, 60 insertions(+)
>
> diff --git a/android/avrcp-lib.c b/android/avrcp-lib.c
> index c280cf8..136801e 100644
> --- a/android/avrcp-lib.c
> +++ b/android/avrcp-lib.c
> @@ -50,6 +50,12 @@
> #define AVRCP_STATUS_NO_AVAILABLE_PLAYERS 0x15
> #define AVRCP_STATUS_ADDRESSED_PLAYER_CHANGED 0x16
>
> +/* Packet types */
> +#define AVRCP_PACKET_TYPE_SINGLE 0x00
> +#define AVRCP_PACKET_TYPE_START 0x01
> +#define AVRCP_PACKET_TYPE_CONTINUING 0x02
> +#define AVRCP_PACKET_TYPE_END 0x03
> +
> #if __BYTE_ORDER == __LITTLE_ENDIAN
>
> struct avrcp_header {
> @@ -202,6 +208,18 @@ static bool handle_passthrough_pdu(struct avctp *conn, uint8_t op,
> return handler->func(session);
> }
>
> +/*
> + * set_company_id:
> + *
> + * Set three-byte Company_ID into outgoing AVRCP message
> + */
> +static void set_company_id(uint8_t cid[3], const uint32_t cid_in)
> +{
> + cid[0] = cid_in >> 16;
> + cid[1] = cid_in >> 8;
> + cid[2] = cid_in;
> +}
Check out what I did with ntoh24, you need to check the byte order as
well and create hton24.
> struct avrcp *avrcp_new(int fd, size_t imtu, size_t omtu, uint16_t version)
> {
> struct avrcp *session;
> @@ -253,3 +271,25 @@ int avrcp_init_uinput(struct avrcp *session, const char *name,
> {
> return avctp_init_uinput(session->conn, name, address);
> }
> +
> +#define AVRCP_GET_CAPABILITIES_PARAM_LENGTH 1
> +void avrcp_get_capabilities(struct avrcp *session, avctp_rsp_cb func)
> +{
> + uint8_t buf[AVRCP_HEADER_LENGTH + AVRCP_GET_CAPABILITIES_PARAM_LENGTH];
> + struct avrcp_header *pdu = (void *) buf;
> + uint8_t length;
> +
> + memset(buf, 0, sizeof(buf));
> +
> + set_company_id(pdu->company_id, IEEEID_BTSIG);
> + pdu->pdu_id = AVRCP_GET_CAPABILITIES;
> + pdu->packet_type = AVRCP_PACKET_TYPE_SINGLE;
> + pdu->params[0] = CAP_EVENTS_SUPPORTED;
> + pdu->params_len = htons(AVRCP_GET_CAPABILITIES_PARAM_LENGTH);
> +
> + length = AVRCP_HEADER_LENGTH + ntohs(pdu->params_len);
> +
> + avctp_send_vendordep_req(session->conn, AVC_CTYPE_STATUS,
> + AVC_SUBUNIT_PANEL, buf, length,
> + func, session);
> +}
I would create a helper function called avrc_send_req which takes the
callback + params and params_len, also Im not sure if it is a good
idea to reuse avctp_rsp_cb or have a different callback so we can
treat AVCTP internally.
> diff --git a/android/avrcp-lib.h b/android/avrcp-lib.h
> index 2337429..4f3a632 100644
> --- a/android/avrcp-lib.h
> +++ b/android/avrcp-lib.h
> @@ -74,3 +74,5 @@ void avrcp_set_passthrough_handlers(struct avrcp *session,
> void *user_data);
> int avrcp_init_uinput(struct avrcp *session, const char *name,
> const char *address);
> +
> +void avrcp_get_capabilities(struct avrcp *session, avctp_rsp_cb func);
> diff --git a/android/avrcp.c b/android/avrcp.c
> index 48444a4..3d39d91 100644
> --- a/android/avrcp.c
> +++ b/android/avrcp.c
> @@ -36,6 +36,7 @@
> #include "bluetooth.h"
> #include "hal-msg.h"
> #include "ipc.h"
> +#include "avctp.h"
> #include "avrcp-lib.h"
> #include "avrcp.h"
>
> diff --git a/unit/test-avrcp.c b/unit/test-avrcp.c
> index 53e9237..4a92860 100644
> --- a/unit/test-avrcp.c
> +++ b/unit/test-avrcp.c
> @@ -297,6 +297,16 @@ static void test_server(gconstpointer data)
> execute_context(context);
> }
>
> +static void test_client(gconstpointer data)
> +{
> + struct context *context = create_context(0x0100, data);
> +
> + if (g_str_equal(context->data->test_name, "/TP/CFG/BV-01-C"))
> + avrcp_get_capabilities(context->session, NULL);
> +
> + execute_context(context);
> +}
> +
> int main(int argc, char *argv[])
> {
> g_test_init(&argc, &argv, NULL);
> @@ -364,5 +374,12 @@ int main(int argc, char *argv[])
> raw_pdu(0x02, 0x11, 0x0e, 0x09, 0x48, 0x7c,
> AVC_PLAY | 0x80, 0x00));
>
> + /* Metadata transfer tests */
> +
> + define_test("/TP/CFG/BV-01-C", test_client,
> + raw_pdu(0x00, 0x11, 0x0e, 0x01, 0x48, 0x00,
> + 0x00, 0x19, 0x58, 0x10, 0x00, 0x00,
> + 0x01, 0x03));
> +
> return g_test_run();
> }
> --
> 1.8.3.2
>
> --
> 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
^ permalink raw reply
* Re: [PATCH 1/3] unit/avrcp: Add /TP/CFG/BV-01-C test
From: Andrei Emeltchenko @ 2014-02-26 12:22 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <CABBYNZJXfxwxnN2DjKBKV9Ts=Npqc31f5VhcNC0qAjcqhSwUsw@mail.gmail.com>
Hi Luiz,
On Wed, Feb 26, 2014 at 01:59:30PM +0200, Luiz Augusto von Dentz wrote:
> Hi Andrei,
>
> On Tue, Feb 25, 2014 at 3:56 PM, Andrei Emeltchenko
> <Andrei.Emeltchenko.news@gmail.com> wrote:
> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> >
> > Test verifies that the get capabilities command issued from the
> > Controller. For that we add the command to avrcp-lib and it will be used
> > in AVRCP later.
> > ---
> > android/avrcp-lib.c | 40 ++++++++++++++++++++++++++++++++++++++++
> > android/avrcp-lib.h | 2 ++
> > android/avrcp.c | 1 +
> > unit/test-avrcp.c | 17 +++++++++++++++++
> > 4 files changed, 60 insertions(+)
> >
> > diff --git a/android/avrcp-lib.c b/android/avrcp-lib.c
> > index c280cf8..136801e 100644
> > --- a/android/avrcp-lib.c
> > +++ b/android/avrcp-lib.c
> > @@ -50,6 +50,12 @@
> > #define AVRCP_STATUS_NO_AVAILABLE_PLAYERS 0x15
> > #define AVRCP_STATUS_ADDRESSED_PLAYER_CHANGED 0x16
> >
> > +/* Packet types */
> > +#define AVRCP_PACKET_TYPE_SINGLE 0x00
> > +#define AVRCP_PACKET_TYPE_START 0x01
> > +#define AVRCP_PACKET_TYPE_CONTINUING 0x02
> > +#define AVRCP_PACKET_TYPE_END 0x03
> > +
> > #if __BYTE_ORDER == __LITTLE_ENDIAN
> >
> > struct avrcp_header {
> > @@ -202,6 +208,18 @@ static bool handle_passthrough_pdu(struct avctp *conn, uint8_t op,
> > return handler->func(session);
> > }
> >
> > +/*
> > + * set_company_id:
> > + *
> > + * Set three-byte Company_ID into outgoing AVRCP message
> > + */
> > +static void set_company_id(uint8_t cid[3], const uint32_t cid_in)
> > +{
> > + cid[0] = cid_in >> 16;
> > + cid[1] = cid_in >> 8;
> > + cid[2] = cid_in;
> > +}
>
> Check out what I did with ntoh24, you need to check the byte order as
> well and create hton24.
>
> > struct avrcp *avrcp_new(int fd, size_t imtu, size_t omtu, uint16_t version)
> > {
> > struct avrcp *session;
> > @@ -253,3 +271,25 @@ int avrcp_init_uinput(struct avrcp *session, const char *name,
> > {
> > return avctp_init_uinput(session->conn, name, address);
> > }
> > +
> > +#define AVRCP_GET_CAPABILITIES_PARAM_LENGTH 1
> > +void avrcp_get_capabilities(struct avrcp *session, avctp_rsp_cb func)
> > +{
> > + uint8_t buf[AVRCP_HEADER_LENGTH + AVRCP_GET_CAPABILITIES_PARAM_LENGTH];
> > + struct avrcp_header *pdu = (void *) buf;
> > + uint8_t length;
> > +
> > + memset(buf, 0, sizeof(buf));
> > +
> > + set_company_id(pdu->company_id, IEEEID_BTSIG);
> > + pdu->pdu_id = AVRCP_GET_CAPABILITIES;
> > + pdu->packet_type = AVRCP_PACKET_TYPE_SINGLE;
> > + pdu->params[0] = CAP_EVENTS_SUPPORTED;
> > + pdu->params_len = htons(AVRCP_GET_CAPABILITIES_PARAM_LENGTH);
> > +
> > + length = AVRCP_HEADER_LENGTH + ntohs(pdu->params_len);
> > +
> > + avctp_send_vendordep_req(session->conn, AVC_CTYPE_STATUS,
> > + AVC_SUBUNIT_PANEL, buf, length,
> > + func, session);
> > +}
>
> I would create a helper function called avrc_send_req which takes the
> callback + params and params_len, also Im not sure if it is a good
> idea to reuse avctp_rsp_cb or have a different callback so we can
> treat AVCTP internally.
So avrcp_send_req does not really help here since it would be just wrapper
for avctp_send functions. Maybe then this unit test is not needed at all?
Best regards
Andrei Emeltchenko
^ permalink raw reply
* [PATCHv2] android/avrcp: Fix passing wrong len
From: Andrei Emeltchenko @ 2014-02-26 12:40 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <CABBYNZK9sq8arr71uvUxSg654ke0uZNHQfXxNvyLRz-iRFy8wA@mail.gmail.com>
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
When handling vendor dependent PDUs len was passed in wrong order to
callback function. It is really wrong to pass such a parameter and
expect that callbacks would handle it.
---
android/avrcp-lib.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/android/avrcp-lib.c b/android/avrcp-lib.c
index c78881f..2e5a565 100644
--- a/android/avrcp-lib.c
+++ b/android/avrcp-lib.c
@@ -128,14 +128,14 @@ static size_t handle_vendordep_pdu(struct avctp *conn, uint8_t transaction,
const struct avrcp_control_handler *handler;
struct avrcp_header *pdu = (void *) operands;
uint32_t company_id = ntoh24(pdu->company_id);
+ uint16_t params_len = ntohs(pdu->params_len);
if (company_id != IEEEID_BTSIG) {
*code = AVC_CTYPE_NOT_IMPLEMENTED;
return 0;
}
- DBG("AVRCP PDU 0x%02X, len 0x%04X", pdu->pdu_id,
- ntohs(pdu->params_len));
+ DBG("AVRCP PDU 0x%02X, len 0x%04X", pdu->pdu_id, params_len);
pdu->packet_type = 0;
pdu->rsvd = 0;
@@ -163,10 +163,12 @@ static size_t handle_vendordep_pdu(struct avctp *conn, uint8_t transaction,
goto reject;
}
- *code = handler->func(session, transaction, &pdu->params_len,
+ *code = handler->func(session, transaction, ¶ms_len,
pdu->params, session->control_data);
- return AVRCP_HEADER_LENGTH + ntohs(pdu->params_len);
+ pdu->params_len = htons(params_len);
+
+ return AVRCP_HEADER_LENGTH + params_len;
reject:
pdu->params_len = htons(1);
--
1.8.3.2
^ permalink raw reply related
* Re: [RFC 4/7] android: Add support for disabling notifications in IPC
From: Lukasz Rymanowski @ 2014-02-26 12:46 UTC (permalink / raw)
To: Szymon Janc; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <1393325419-16544-4-git-send-email-szymon.janc@tieto.com>
Hi Szymon,
On Tue, Feb 25, 2014 at 11:50 AM, Szymon Janc <szymon.janc@tieto.com> wrote:
> ---
> android/ipc.c | 20 ++++++++++++++++++--
> android/ipc.h | 2 ++
> 2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/android/ipc.c b/android/ipc.c
> index 4bddb58..e356439 100644
> --- a/android/ipc.c
> +++ b/android/ipc.c
> @@ -50,6 +50,7 @@ struct ipc {
> GIOChannel *cmd_io;
> guint cmd_watch;
>
> + bool notif_disabled;
> GIOChannel *notif_io;
> guint notif_watch;
>
> @@ -246,7 +247,7 @@ static gboolean notif_connect_cb(GIOChannel *io, GIOCondition cond,
>
> ipc->cmd_watch = g_io_add_watch(ipc->cmd_io, cond, cmd_watch_cb, ipc);
>
> - info("IPC: successfully connected");
> + info("IPC: successfully connected (with notifications)");
>
> return FALSE;
> }
> @@ -263,13 +264,23 @@ static gboolean cmd_connect_cb(GIOChannel *io, GIOCondition cond,
> goto failed;
> }
>
> + if (ipc->notif_disabled) {
> + cond = G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL;
> +
> + ipc->cmd_watch = g_io_add_watch(ipc->cmd_io, cond, cmd_watch_cb,
> + ipc);
> +
> + info("IPC: successfully connected (without notifications)");
> +
> + return FALSE;
> + }
> +
> ipc->notif_io = ipc_connect(ipc->path, ipc->size, notif_connect_cb,
> ipc);
> if (!ipc->notif_io)
> goto failed;
>
> return FALSE;
> -
> failed:
> ipc_disconnect(ipc, true);
>
> @@ -312,6 +323,11 @@ void ipc_set_fail_handler(struct ipc *ipc, ipc_failed_cb cb, void *data)
> ipc->failed_cb_data = data;
> }
>
> +void ipc_disable_notifications(struct ipc *ipc)
> +{
> + ipc->notif_disabled = true;
> +}
> +
> void ipc_send(int sk, uint8_t service_id, uint8_t opcode, uint16_t len,
> void *param, int fd)
> {
> diff --git a/android/ipc.h b/android/ipc.h
> index 3a6adc8..c4f8c41 100644
> --- a/android/ipc.h
> +++ b/android/ipc.h
> @@ -40,6 +40,8 @@ void ipc_cleanup(struct ipc *ipc);
> typedef void (*ipc_failed_cb) (void *data);
> void ipc_set_fail_handler(struct ipc *ipc, ipc_failed_cb cb, void *data);
>
> +void ipc_disable_notifications(struct ipc *ipc);
> +
Maybe it would be better to add disable_notification (or better
enable_notification) as a parameter to ipc_init() ? You want to set
it once I guess.
> GIOChannel *ipc_connect(const char *path, size_t size, GIOFunc connect_cb,
> void *user_data);
> int ipc_handle_msg(struct service_handler *handlers, size_t max_index,
> --
> 1.8.3.2
>
> --
> 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
BR
Lukasz
^ permalink raw reply
* Re: [RFC 2/7] android: Add support for registering failure callback in IPC
From: Lukasz Rymanowski @ 2014-02-26 12:58 UTC (permalink / raw)
To: Szymon Janc; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <1393325419-16544-2-git-send-email-szymon.janc@tieto.com>
Hi Szymon,
On Tue, Feb 25, 2014 at 11:50 AM, Szymon Janc <szymon.janc@tieto.com> wrote:
> Allow to register callback which is called in case of IPC failure
> (eg malformed message) or disconnection. This makes caller responsible
> for performing expected action in such case.
> ---
> android/ipc.c | 103 +++++++++++++++++++++++++++++++++++++--------------------
> android/ipc.h | 3 ++
> android/main.c | 7 ++++
> 3 files changed, 77 insertions(+), 36 deletions(-)
>
> diff --git a/android/ipc.c b/android/ipc.c
> index 3d6e2a3..4bddb58 100644
> --- a/android/ipc.c
> +++ b/android/ipc.c
> @@ -52,8 +52,39 @@ struct ipc {
>
> GIOChannel *notif_io;
> guint notif_watch;
> +
> + ipc_failed_cb failed_cb;
> + void *failed_cb_data;
> };
>
> +static void ipc_disconnect(struct ipc *ipc, bool failed)
> +{
> + if (ipc->cmd_watch) {
> + g_source_remove(ipc->cmd_watch);
> + ipc->cmd_watch = 0;
> + }
> +
> + if (ipc->cmd_io) {
> + g_io_channel_shutdown(ipc->cmd_io, TRUE, NULL);
> + g_io_channel_unref(ipc->cmd_io);
> + ipc->cmd_io = NULL;
> + }
> +
> + if (ipc->notif_watch) {
> + g_source_remove(ipc->notif_watch);
> + ipc->notif_watch = 0;
> + }
> +
> + if (ipc->notif_io) {
> + g_io_channel_shutdown(ipc->notif_io, TRUE, NULL);
> + g_io_channel_unref(ipc->notif_io);
> + ipc->notif_io = NULL;
> + }
> +
> + if (ipc->failed_cb && failed)
> + ipc->failed_cb(ipc->failed_cb_data);
> +}
> +
> int ipc_handle_msg(struct service_handler *handlers, size_t max_index,
> const void *buf, ssize_t len)
> {
> @@ -116,7 +147,9 @@ static gboolean cmd_watch_cb(GIOChannel *io, GIOCondition cond,
> int fd, err;
>
> if (cond & (G_IO_NVAL | G_IO_ERR | G_IO_HUP)) {
> - info("IPC: command socket closed, terminating");
> + info("IPC: command socket closed");
> +
> + ipc->cmd_watch = 0;
> goto fail;
> }
>
> @@ -124,30 +157,34 @@ static gboolean cmd_watch_cb(GIOChannel *io, GIOCondition cond,
>
> ret = read(fd, buf, sizeof(buf));
> if (ret < 0) {
> - error("IPC: command read failed, terminating (%s)",
> - strerror(errno));
> + error("IPC: command read failed (%s)", strerror(errno));
> goto fail;
> }
>
> err = ipc_handle_msg(ipc->services, ipc->service_max, buf, ret);
> if (err < 0) {
> - error("IPC: failed to handle message, terminating (%s)",
> - strerror(-err));
> + error("IPC: failed to handle message (%s)", strerror(-err));
> goto fail;
> }
>
> return TRUE;
>
> fail:
> - raise(SIGTERM);
> + ipc_disconnect(ipc, true);
> +
> return FALSE;
> }
>
> static gboolean notif_watch_cb(GIOChannel *io, GIOCondition cond,
> gpointer user_data)
> {
> - info("IPC: notification socket closed, terminating");
> - raise(SIGTERM);
> + struct ipc *ipc = user_data;
> +
> + info("IPC: notification socket closed");
> +
> + ipc->notif_watch = 0;
> +
> + ipc_disconnect(ipc, true);
>
> return FALSE;
> }
> @@ -194,8 +231,10 @@ static gboolean notif_connect_cb(GIOChannel *io, GIOCondition cond,
> DBG("");
>
> if (cond & (G_IO_NVAL | G_IO_ERR | G_IO_HUP)) {
> - error("IPC: notification socket connect failed, terminating");
> - raise(SIGTERM);
> + error("IPC: notification socket connect failed");
> +
> + ipc_disconnect(ipc, true);
> +
> return FALSE;
> }
>
> @@ -220,15 +259,19 @@ static gboolean cmd_connect_cb(GIOChannel *io, GIOCondition cond,
> DBG("");
>
> if (cond & (G_IO_NVAL | G_IO_ERR | G_IO_HUP)) {
> - error("IPC: command socket connect failed, terminating");
> - raise(SIGTERM);
> - return FALSE;
> + error("IPC: command socket connect failed");
> + goto failed;
> }
>
> ipc->notif_io = ipc_connect(ipc->path, ipc->size, notif_connect_cb,
> ipc);
> if (!ipc->notif_io)
> - raise(SIGTERM);
> + goto failed;
> +
> + return FALSE;
> +
> +failed:
> + ipc_disconnect(ipc, true);
>
> return FALSE;
> }
> @@ -257,32 +300,18 @@ struct ipc *ipc_init(const char *path, size_t size, int max_service_id)
>
> void ipc_cleanup(struct ipc *ipc)
> {
> - if (ipc->cmd_watch) {
> - g_source_remove(ipc->cmd_watch);
> - ipc->cmd_watch = 0;
> - }
> -
> - if (ipc->cmd_io) {
> - g_io_channel_shutdown(ipc->cmd_io, TRUE, NULL);
> - g_io_channel_unref(ipc->cmd_io);
> - ipc->cmd_io = NULL;
> - }
> -
> - if (ipc->notif_watch) {
> - g_source_remove(ipc->notif_watch);
> - ipc->notif_watch = 0;
> - }
> -
> - if (ipc->notif_io) {
> - g_io_channel_shutdown(ipc->notif_io, TRUE, NULL);
> - g_io_channel_unref(ipc->notif_io);
> - ipc->notif_io = NULL;
> - }
> + ipc_disconnect(ipc, false);
>
> g_free(ipc->services);
> g_free(ipc);
> }
>
> +void ipc_set_fail_handler(struct ipc *ipc, ipc_failed_cb cb, void *data)
> +{
> + ipc->failed_cb = cb;
> + ipc->failed_cb_data = data;
> +}
> +
> void ipc_send(int sk, uint8_t service_id, uint8_t opcode, uint16_t len,
> void *param, int fd)
> {
> @@ -323,7 +352,9 @@ void ipc_send(int sk, uint8_t service_id, uint8_t opcode, uint16_t len,
> }
>
> if (sendmsg(sk, &msg, 0) < 0) {
> - error("IPC send failed, terminating :%s", strerror(errno));
> + error("IPC send failed :%s", strerror(errno));
> +
> + /* TODO disconnect IPC here when this function becomes static */
> raise(SIGTERM);
> }
> }
> diff --git a/android/ipc.h b/android/ipc.h
> index 601301c..3a6adc8 100644
> --- a/android/ipc.h
> +++ b/android/ipc.h
> @@ -37,6 +37,9 @@ struct ipc;
> struct ipc *ipc_init(const char *path, size_t size, int max_service_id);
> void ipc_cleanup(struct ipc *ipc);
>
> +typedef void (*ipc_failed_cb) (void *data);
> +void ipc_set_fail_handler(struct ipc *ipc, ipc_failed_cb cb, void *data);
> +
This looks like destroy callback. Consider to add also to ipc_init()
as a parameter.
> GIOChannel *ipc_connect(const char *path, size_t size, GIOFunc connect_cb,
> void *user_data);
> int ipc_handle_msg(struct service_handler *handlers, size_t max_index,
> diff --git a/android/main.c b/android/main.c
> index 9f22486..a821dfa 100644
> --- a/android/main.c
> +++ b/android/main.c
> @@ -228,6 +228,11 @@ static void stop_bluetooth(void)
> g_timeout_add_seconds(SHUTDOWN_GRACE_SECONDS, quit_eventloop, NULL);
> }
>
> +static void ipc_disconnected(void *data)
> +{
> + stop_bluetooth();
> +}
> +
> static void adapter_ready(int err, const bdaddr_t *addr)
> {
> if (err < 0) {
> @@ -251,6 +256,8 @@ static void adapter_ready(int err, const bdaddr_t *addr)
> exit(EXIT_FAILURE);
> }
>
> + ipc_set_fail_handler(hal_ipc, ipc_disconnected, NULL);
> +
> ipc_register(hal_ipc, HAL_SERVICE_ID_CORE, cmd_handlers,
> G_N_ELEMENTS(cmd_handlers));
> }
> --
> 1.8.3.2
>
> --
> 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
BR
Lukasz
^ permalink raw reply
* Re: [PATCH] android/hal-ipc-api: Fix Add Set Volume command struct packing
From: Luiz Augusto von Dentz @ 2014-02-26 14:01 UTC (permalink / raw)
To: Grzegorz Kolodziejczyk; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <1393343453-4105-1-git-send-email-grzegorz.kolodziejczyk@tieto.com>
Hi Grzegorz,
On Tue, Feb 25, 2014 at 5:50 PM, Grzegorz Kolodziejczyk
<grzegorz.kolodziejczyk@tieto.com> wrote:
> This adds missed packed struct attribute to hal-ipc command.
> ---
> android/hal-msg.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/android/hal-msg.h b/android/hal-msg.h
> index 7c6110a..ee08c37 100644
> --- a/android/hal-msg.h
> +++ b/android/hal-msg.h
> @@ -987,7 +987,7 @@ struct hal_cmd_avrcp_register_notification {
> #define HAL_OP_AVRCP_SET_VOLUME 0x0a
> struct hal_cmd_avrcp_set_volume {
> uint8_t value;
> -};
> +} __attribute__((packed));
>
> #define HAL_EV_AVRCP_REMOTE_FEATURES 0x81
> struct hal_ev_avrcp_remote_features {
> --
> 1.8.5.2
Applied, thanks.
--
Luiz Augusto von Dentz
^ permalink raw reply
* [PATCH] android/avrcp: Add avrcp_get_capabilities request
From: Andrei Emeltchenko @ 2014-02-26 14:26 UTC (permalink / raw)
To: linux-bluetooth
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Implement avrcp_get_capabilities() request through
avrcp_send_vendordep_req(). avctp_send_req() is not exported so we use
the functions which are exported by AVCTP code.
---
android/avrcp-lib.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
android/avrcp.c | 1 +
2 files changed, 49 insertions(+)
diff --git a/android/avrcp-lib.c b/android/avrcp-lib.c
index 2e5a565..cf4cdaa 100644
--- a/android/avrcp-lib.c
+++ b/android/avrcp-lib.c
@@ -50,6 +50,18 @@
#define AVRCP_STATUS_NO_AVAILABLE_PLAYERS 0x15
#define AVRCP_STATUS_ADDRESSED_PLAYER_CHANGED 0x16
+/* Packet types */
+#define AVRCP_PACKET_TYPE_SINGLE 0x00
+#define AVRCP_PACKET_TYPE_START 0x01
+#define AVRCP_PACKET_TYPE_CONTINUING 0x02
+#define AVRCP_PACKET_TYPE_END 0x03
+
+/* Capabilities for AVRCP_GET_CAPABILITIES pdu */
+#define CAP_COMPANY_ID 0x02
+#define CAP_EVENTS_SUPPORTED 0x03
+
+#define AVRCP_GET_CAPABILITIES_PARAM_LENGTH 1
+
#if __BYTE_ORDER == __LITTLE_ENDIAN
struct avrcp_header {
@@ -92,6 +104,13 @@ static inline uint32_t ntoh24(const uint8_t src[3])
#error "Unknown byte order"
#endif
+static inline void hton24(uint8_t dst[3], uint32_t src)
+{
+ dst[0] = (src >> 16) & 0xff;
+ dst[1] = (src >> 8) & 0xff;
+ dst[2] = src & 0xff;
+}
+
struct avrcp {
struct avctp *conn;
@@ -255,3 +274,32 @@ int avrcp_init_uinput(struct avrcp *session, const char *name,
{
return avctp_init_uinput(session->conn, name, address);
}
+
+static int avrcp_send_vendordep_req(struct avrcp *session, uint8_t code,
+ uint8_t subunit, uint8_t *operands,
+ size_t operand_count,
+ avctp_rsp_cb func, void *user_data)
+{
+ return avctp_send_vendordep_req(session->conn, code, subunit, operands,
+ operand_count, func, user_data);
+}
+
+void avrcp_get_capabilities(struct avrcp *session, avctp_rsp_cb func)
+{
+ uint8_t buf[AVRCP_HEADER_LENGTH + AVRCP_GET_CAPABILITIES_PARAM_LENGTH];
+ struct avrcp_header *pdu = (void *) buf;
+ uint8_t length;
+
+ memset(buf, 0, sizeof(buf));
+
+ hton24(pdu->company_id, IEEEID_BTSIG);
+ pdu->pdu_id = AVRCP_GET_CAPABILITIES;
+ pdu->packet_type = AVRCP_PACKET_TYPE_SINGLE;
+ pdu->params[0] = CAP_EVENTS_SUPPORTED;
+ pdu->params_len = htons(AVRCP_GET_CAPABILITIES_PARAM_LENGTH);
+
+ length = AVRCP_HEADER_LENGTH + ntohs(pdu->params_len);
+
+ avrcp_send_vendordep_req(session, AVC_CTYPE_STATUS, AVC_SUBUNIT_PANEL,
+ buf, length, func, session);
+}
diff --git a/android/avrcp.c b/android/avrcp.c
index 48444a4..3d39d91 100644
--- a/android/avrcp.c
+++ b/android/avrcp.c
@@ -36,6 +36,7 @@
#include "bluetooth.h"
#include "hal-msg.h"
#include "ipc.h"
+#include "avctp.h"
#include "avrcp-lib.h"
#include "avrcp.h"
--
1.8.3.2
^ permalink raw reply related
* [PATCHv2] android/avrcp: Add avrcp_get_capabilities request
From: Andrei Emeltchenko @ 2014-02-26 14:32 UTC (permalink / raw)
To: linux-bluetooth
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Implement avrcp_get_capabilities() request through
avrcp_send_vendordep_req(). avctp_send_req() is not exported so we use
the functions which are exported by AVCTP code.
---
android/avrcp-lib.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
android/avrcp-lib.h | 2 ++
android/avrcp.c | 1 +
3 files changed, 51 insertions(+)
diff --git a/android/avrcp-lib.c b/android/avrcp-lib.c
index 2e5a565..cf4cdaa 100644
--- a/android/avrcp-lib.c
+++ b/android/avrcp-lib.c
@@ -50,6 +50,18 @@
#define AVRCP_STATUS_NO_AVAILABLE_PLAYERS 0x15
#define AVRCP_STATUS_ADDRESSED_PLAYER_CHANGED 0x16
+/* Packet types */
+#define AVRCP_PACKET_TYPE_SINGLE 0x00
+#define AVRCP_PACKET_TYPE_START 0x01
+#define AVRCP_PACKET_TYPE_CONTINUING 0x02
+#define AVRCP_PACKET_TYPE_END 0x03
+
+/* Capabilities for AVRCP_GET_CAPABILITIES pdu */
+#define CAP_COMPANY_ID 0x02
+#define CAP_EVENTS_SUPPORTED 0x03
+
+#define AVRCP_GET_CAPABILITIES_PARAM_LENGTH 1
+
#if __BYTE_ORDER == __LITTLE_ENDIAN
struct avrcp_header {
@@ -92,6 +104,13 @@ static inline uint32_t ntoh24(const uint8_t src[3])
#error "Unknown byte order"
#endif
+static inline void hton24(uint8_t dst[3], uint32_t src)
+{
+ dst[0] = (src >> 16) & 0xff;
+ dst[1] = (src >> 8) & 0xff;
+ dst[2] = src & 0xff;
+}
+
struct avrcp {
struct avctp *conn;
@@ -255,3 +274,32 @@ int avrcp_init_uinput(struct avrcp *session, const char *name,
{
return avctp_init_uinput(session->conn, name, address);
}
+
+static int avrcp_send_vendordep_req(struct avrcp *session, uint8_t code,
+ uint8_t subunit, uint8_t *operands,
+ size_t operand_count,
+ avctp_rsp_cb func, void *user_data)
+{
+ return avctp_send_vendordep_req(session->conn, code, subunit, operands,
+ operand_count, func, user_data);
+}
+
+void avrcp_get_capabilities(struct avrcp *session, avctp_rsp_cb func)
+{
+ uint8_t buf[AVRCP_HEADER_LENGTH + AVRCP_GET_CAPABILITIES_PARAM_LENGTH];
+ struct avrcp_header *pdu = (void *) buf;
+ uint8_t length;
+
+ memset(buf, 0, sizeof(buf));
+
+ hton24(pdu->company_id, IEEEID_BTSIG);
+ pdu->pdu_id = AVRCP_GET_CAPABILITIES;
+ pdu->packet_type = AVRCP_PACKET_TYPE_SINGLE;
+ pdu->params[0] = CAP_EVENTS_SUPPORTED;
+ pdu->params_len = htons(AVRCP_GET_CAPABILITIES_PARAM_LENGTH);
+
+ length = AVRCP_HEADER_LENGTH + ntohs(pdu->params_len);
+
+ avrcp_send_vendordep_req(session, AVC_CTYPE_STATUS, AVC_SUBUNIT_PANEL,
+ buf, length, func, session);
+}
diff --git a/android/avrcp-lib.h b/android/avrcp-lib.h
index 2337429..4f3a632 100644
--- a/android/avrcp-lib.h
+++ b/android/avrcp-lib.h
@@ -74,3 +74,5 @@ void avrcp_set_passthrough_handlers(struct avrcp *session,
void *user_data);
int avrcp_init_uinput(struct avrcp *session, const char *name,
const char *address);
+
+void avrcp_get_capabilities(struct avrcp *session, avctp_rsp_cb func);
diff --git a/android/avrcp.c b/android/avrcp.c
index 48444a4..3d39d91 100644
--- a/android/avrcp.c
+++ b/android/avrcp.c
@@ -36,6 +36,7 @@
#include "bluetooth.h"
#include "hal-msg.h"
#include "ipc.h"
+#include "avctp.h"
#include "avrcp-lib.h"
#include "avrcp.h"
--
1.8.3.2
^ permalink raw reply related
* [PATCH 1/3] unit/avrcp: Add /TP/CFG/BV-01-C test
From: Andrei Emeltchenko @ 2014-02-26 15:14 UTC (permalink / raw)
To: linux-bluetooth
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Test verifies that the get capabilities command issued from the
Controller.
---
unit/test-avrcp.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/unit/test-avrcp.c b/unit/test-avrcp.c
index 53e9237..4a92860 100644
--- a/unit/test-avrcp.c
+++ b/unit/test-avrcp.c
@@ -297,6 +297,16 @@ static void test_server(gconstpointer data)
execute_context(context);
}
+static void test_client(gconstpointer data)
+{
+ struct context *context = create_context(0x0100, data);
+
+ if (g_str_equal(context->data->test_name, "/TP/CFG/BV-01-C"))
+ avrcp_get_capabilities(context->session, NULL);
+
+ execute_context(context);
+}
+
int main(int argc, char *argv[])
{
g_test_init(&argc, &argv, NULL);
@@ -364,5 +374,12 @@ int main(int argc, char *argv[])
raw_pdu(0x02, 0x11, 0x0e, 0x09, 0x48, 0x7c,
AVC_PLAY | 0x80, 0x00));
+ /* Metadata transfer tests */
+
+ define_test("/TP/CFG/BV-01-C", test_client,
+ raw_pdu(0x00, 0x11, 0x0e, 0x01, 0x48, 0x00,
+ 0x00, 0x19, 0x58, 0x10, 0x00, 0x00,
+ 0x01, 0x03));
+
return g_test_run();
}
--
1.8.3.2
^ permalink raw reply related
* [PATCH 2/3] android/avrcp: Add control handlers to avrcp-lib
From: Andrei Emeltchenko @ 2014-02-26 15:14 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1393427669-15337-1-git-send-email-Andrei.Emeltchenko.news@gmail.com>
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
android/avrcp-lib.c | 23 +++--------------------
android/avrcp-lib.h | 33 +++++++++++++++++++++++++++++++++
unit/test-avrcp.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 82 insertions(+), 20 deletions(-)
diff --git a/android/avrcp-lib.c b/android/avrcp-lib.c
index cf4cdaa..5e5ba31 100644
--- a/android/avrcp-lib.c
+++ b/android/avrcp-lib.c
@@ -35,31 +35,12 @@
#include "avctp.h"
#include "avrcp-lib.h"
-/* Company IDs for vendor dependent commands */
-#define IEEEID_BTSIG 0x001958
-
-/* Status codes */
-#define AVRCP_STATUS_INVALID_COMMAND 0x00
-#define AVRCP_STATUS_INVALID_PARAM 0x01
-#define AVRCP_STATUS_PARAM_NOT_FOUND 0x02
-#define AVRCP_STATUS_INTERNAL_ERROR 0x03
-#define AVRCP_STATUS_SUCCESS 0x04
-#define AVRCP_STATUS_OUT_OF_BOUNDS 0x0b
-#define AVRCP_STATUS_INVALID_PLAYER_ID 0x11
-#define AVRCP_STATUS_PLAYER_NOT_BROWSABLE 0x12
-#define AVRCP_STATUS_NO_AVAILABLE_PLAYERS 0x15
-#define AVRCP_STATUS_ADDRESSED_PLAYER_CHANGED 0x16
-
/* Packet types */
#define AVRCP_PACKET_TYPE_SINGLE 0x00
#define AVRCP_PACKET_TYPE_START 0x01
#define AVRCP_PACKET_TYPE_CONTINUING 0x02
#define AVRCP_PACKET_TYPE_END 0x03
-/* Capabilities for AVRCP_GET_CAPABILITIES pdu */
-#define CAP_COMPANY_ID 0x02
-#define CAP_EVENTS_SUPPORTED 0x03
-
#define AVRCP_GET_CAPABILITIES_PARAM_LENGTH 1
#if __BYTE_ORDER == __LITTLE_ENDIAN
@@ -104,7 +85,7 @@ static inline uint32_t ntoh24(const uint8_t src[3])
#error "Unknown byte order"
#endif
-static inline void hton24(uint8_t dst[3], uint32_t src)
+void hton24(uint8_t dst[3], uint32_t src)
{
dst[0] = (src >> 16) & 0xff;
dst[1] = (src >> 8) & 0xff;
@@ -121,6 +102,8 @@ struct avrcp {
const struct avrcp_passthrough_handler *passthrough_handlers;
void *passthrough_data;
unsigned int passthrough_id;
+
+ uint16_t supported_events;
};
void avrcp_shutdown(struct avrcp *session)
diff --git a/android/avrcp-lib.h b/android/avrcp-lib.h
index 4f3a632..4115e09 100644
--- a/android/avrcp-lib.h
+++ b/android/avrcp-lib.h
@@ -46,6 +46,37 @@
#define AVRCP_ADD_TO_NOW_PLAYING 0x90
#define AVRCP_GENERAL_REJECT 0xA0
+/* Notification events */
+#define AVRCP_EVENT_STATUS_CHANGED 0x01
+#define AVRCP_EVENT_TRACK_CHANGED 0x02
+#define AVRCP_EVENT_TRACK_REACHED_END 0x03
+#define AVRCP_EVENT_TRACK_REACHED_START 0x04
+#define AVRCP_EVENT_SETTINGS_CHANGED 0x08
+#define AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED 0x0a
+#define AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED 0x0b
+#define AVRCP_EVENT_UIDS_CHANGED 0x0c
+#define AVRCP_EVENT_VOLUME_CHANGED 0x0d
+#define AVRCP_EVENT_LAST AVRCP_EVENT_VOLUME_CHANGED
+
+/* Status codes */
+#define AVRCP_STATUS_INVALID_COMMAND 0x00
+#define AVRCP_STATUS_INVALID_PARAM 0x01
+#define AVRCP_STATUS_PARAM_NOT_FOUND 0x02
+#define AVRCP_STATUS_INTERNAL_ERROR 0x03
+#define AVRCP_STATUS_SUCCESS 0x04
+#define AVRCP_STATUS_OUT_OF_BOUNDS 0x0b
+#define AVRCP_STATUS_INVALID_PLAYER_ID 0x11
+#define AVRCP_STATUS_PLAYER_NOT_BROWSABLE 0x12
+#define AVRCP_STATUS_NO_AVAILABLE_PLAYERS 0x15
+#define AVRCP_STATUS_ADDRESSED_PLAYER_CHANGED 0x16
+
+/* Capabilities for AVRCP_GET_CAPABILITIES pdu */
+#define CAP_COMPANY_ID 0x02
+#define CAP_EVENTS_SUPPORTED 0x03
+
+/* Company IDs for vendor dependent commands */
+#define IEEEID_BTSIG 0x001958
+
struct avrcp;
struct avrcp_control_handler {
@@ -62,6 +93,8 @@ struct avrcp_passthrough_handler {
typedef void (*avrcp_destroy_cb_t) (void *user_data);
+void hton24(uint8_t dst[3], uint32_t src);
+
struct avrcp *avrcp_new(int fd, size_t imtu, size_t omtu, uint16_t version);
void avrcp_shutdown(struct avrcp *session);
void avrcp_set_destroy_cb(struct avrcp *session, avrcp_destroy_cb_t cb,
diff --git a/unit/test-avrcp.c b/unit/test-avrcp.c
index 4a92860..dc704fa 100644
--- a/unit/test-avrcp.c
+++ b/unit/test-avrcp.c
@@ -64,6 +64,11 @@ struct context {
const struct test_data *data;
};
+/* Company IDs supported by this device */
+static uint32_t company_ids[] = {
+ IEEEID_BTSIG,
+};
+
#define data(args...) ((const unsigned char[]) { args })
#define raw_pdu(args...) \
@@ -285,12 +290,53 @@ static const struct avrcp_passthrough_handler passthrough_handlers[] = {
{ },
};
+static uint8_t avrcp_handle_get_capabilities(struct avrcp *session,
+ uint8_t transaction, uint16_t *params_len,
+ uint8_t *params, void *user_data)
+{
+ unsigned int i;
+
+ DBG("id %d params_len %d", params[0], *params_len);
+
+ if (*params_len != 1)
+ goto fail;
+
+ switch (params[0]) {
+ case CAP_COMPANY_ID:
+ for (i = 0; i < G_N_ELEMENTS(company_ids); i++)
+ hton24(¶ms[2 + i * 3], company_ids[i]);
+
+ *params_len = 2 + (3 * G_N_ELEMENTS(company_ids));
+ params[1] = G_N_ELEMENTS(company_ids);
+
+ return AVC_CTYPE_STABLE;
+ case CAP_EVENTS_SUPPORTED:
+ params[1] = 0;
+ *params_len = 2 + params[1];
+
+ return AVC_CTYPE_STABLE;
+ }
+
+fail:
+ *params_len = htons(1);
+ params[0] = AVRCP_STATUS_INVALID_PARAM;
+
+ return AVC_CTYPE_REJECTED;
+}
+
+static const struct avrcp_control_handler control_handlers[] = {
+ { AVRCP_GET_CAPABILITIES, AVC_CTYPE_STATUS,
+ avrcp_handle_get_capabilities },
+ { },
+};
+
static void test_server(gconstpointer data)
{
struct context *context = create_context(0x0100, data);
avrcp_set_passthrough_handlers(context->session, passthrough_handlers,
context);
+ avrcp_set_control_handlers(context->session, control_handlers, NULL);
g_idle_add(send_pdu, context);
--
1.8.3.2
^ permalink raw reply related
* [PATCH 3/3] unit/avrcp: Add /TP/CFG/BV-02-C test
From: Andrei Emeltchenko @ 2014-02-26 15:14 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1393427669-15337-1-git-send-email-Andrei.Emeltchenko.news@gmail.com>
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Test verifies that the Target responds to Get Capability request.
---
unit/test-avrcp.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/unit/test-avrcp.c b/unit/test-avrcp.c
index dc704fa..d052c8d 100644
--- a/unit/test-avrcp.c
+++ b/unit/test-avrcp.c
@@ -427,5 +427,13 @@ int main(int argc, char *argv[])
0x00, 0x19, 0x58, 0x10, 0x00, 0x00,
0x01, 0x03));
+ define_test("/TP/CFG/BV-02-C", test_server,
+ raw_pdu(0x00, 0x11, 0x0e, 0x01, 0x48, 0x00,
+ 0x00, 0x19, 0x58, 0x10, 0x00, 0x00,
+ 0x01, 0x02),
+ raw_pdu(0x02, 0x11, 0x0e, 0x0c, 0x48, 0x00,
+ 0x00, 0x19, 0x58, 0x10, 0x00, 0x00,
+ 0x05, 0x02, 0x01, 0x00, 0x19, 0x58));
+
return g_test_run();
}
--
1.8.3.2
^ permalink raw reply related
* Re: [PATCH] tty: Fix low_latency BUG
From: Peter Hurley @ 2014-02-26 15:40 UTC (permalink / raw)
To: Feng Tang
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, Beat Bolli,
Pavel Roskin, Linux Kernel Mailing List, Jiri Kosina,
David Sterba, Felipe Balbi, Grant Edwards, Stanislaw Gruszka,
Hal Murray, Alan Cox, stable, linux-bluetooth
In-Reply-To: <CA++bM2uq=vRo20FTMWvkDau+-X7h0+KU3n4Av7ef895mQKr5fA@mail.gmail.com>
[ +cc linux-bluetooth ]
Hi Feng,
On 02/26/2014 12:11 AM, Feng Tang wrote:
> Hi Peter,
>
> 2014-02-22 20:31 GMT+08:00 Peter Hurley <peter@hurleysoftware.com>:
>> The user-settable knob, low_latency, has been the source of
>> several BUG reports which stem from flush_to_ldisc() running
>> in interrupt context. Since 3.12, which added several sleeping
>> locks (termios_rwsem and buf->lock) to the input processing path,
>> the frequency of these BUG reports has increased.
>>
>> Note that changes in 3.12 did not introduce this regression;
>> sleeping locks were first added to the input processing path
>> with the removal of the BKL from N_TTY in commit
>> a88a69c91256418c5907c2f1f8a0ec0a36f9e6cc,
>> 'n_tty: Fix loss of echoed characters and remove bkl from n_tty'
>> and later in commit 38db89799bdf11625a831c5af33938dcb11908b6,
>> 'tty: throttling race fix'. Since those changes, executing
>> flush_to_ldisc() in interrupt_context (ie, low_latency set), is unsafe.
>>
>> However, since most devices do not validate if the low_latency
>> setting is appropriate for the context (process or interrupt) in
>> which they receive data, some reports are due to misconfiguration.
>> Further, serial dma devices for which dma fails, resort to
>> interrupt receiving as a backup without resetting low_latency.
>>
>> Historically, low_latency was used to force wake-up the reading
>> process rather than wait for the next scheduler tick. The
>> effect was to trim multiple milliseconds of latency from
>> when the process would receive new data.
>>
>> Recent tests [1] have shown that the reading process now receives
>> data with only 10's of microseconds latency without low_latency set.
>
> The 10's of miscroseconds is ok for 115200 bps like device, but it may
> hurt the high speed device like Bluetooth which runs at 3M/4M bps or
> higher.
The tests were run at 400Mbps, so 3Mbps or 4Mbps is not a problem
(but I think you may be confusing throughput with latency).
FWIW, two things affected the latency times of those particular tests;
1) the kernel firewire subsystem handles rx data in a tasklet (so not
directly from IRQ) which negatively affected the latency reported, and
2) the ftrace instrumentation is not free and there are several traces per rx.
If you look carefully at the test trace data, you'll see that the timestamps
from tty_flip_buffer_push() of the rx data to n_tty_write() of the
response averages _~11us_; this is the measured latency from tty driver
receiving the rx data to reading of that data *and* the process
response (which comes back up through several tty locks).
Naturally, in mainline kernel, the scheduler load will affect the
measured latency *but that's true regardless of low_latency rx steering
because the user-space process must still be woken to complete the read*.
> More and more smartphones are using uart as the Bluetooth data
> interface due to its low-pin, low-power feature, and many of them
> are using HZ=100 kernel, I'm afraid this added delay may cause
> some problem.
Some hard data showing a real problem would help further this
discussion; my belief is that 3.12+ w/o low_latency rx steering
will outperform 3.11- w/ low_latency rx steering in every test.
I'm glad to hear that the Bluetooth uart interface is getting
some use; that means someone will soon be fixing the hard lockup
in hci_uart_tx_wakeup() reported here:
http://www.spinics.net/lists/linux-serial/msg11529.html
Regards,
Peter Hurley
^ permalink raw reply
* Re: [PATCHv2] android/avrcp: Fix passing wrong len
From: Luiz Augusto von Dentz @ 2014-02-26 17:27 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <1393418420-8461-1-git-send-email-Andrei.Emeltchenko.news@gmail.com>
Hi Andrei,
On Wed, Feb 26, 2014 at 1:40 PM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> When handling vendor dependent PDUs len was passed in wrong order to
> callback function. It is really wrong to pass such a parameter and
> expect that callbacks would handle it.
> ---
> android/avrcp-lib.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/android/avrcp-lib.c b/android/avrcp-lib.c
> index c78881f..2e5a565 100644
> --- a/android/avrcp-lib.c
> +++ b/android/avrcp-lib.c
> @@ -128,14 +128,14 @@ static size_t handle_vendordep_pdu(struct avctp *conn, uint8_t transaction,
> const struct avrcp_control_handler *handler;
> struct avrcp_header *pdu = (void *) operands;
> uint32_t company_id = ntoh24(pdu->company_id);
> + uint16_t params_len = ntohs(pdu->params_len);
>
> if (company_id != IEEEID_BTSIG) {
> *code = AVC_CTYPE_NOT_IMPLEMENTED;
> return 0;
> }
>
> - DBG("AVRCP PDU 0x%02X, len 0x%04X", pdu->pdu_id,
> - ntohs(pdu->params_len));
> + DBG("AVRCP PDU 0x%02X, len 0x%04X", pdu->pdu_id, params_len);
>
> pdu->packet_type = 0;
> pdu->rsvd = 0;
> @@ -163,10 +163,12 @@ static size_t handle_vendordep_pdu(struct avctp *conn, uint8_t transaction,
> goto reject;
> }
>
> - *code = handler->func(session, transaction, &pdu->params_len,
> + *code = handler->func(session, transaction, ¶ms_len,
> pdu->params, session->control_data);
>
> - return AVRCP_HEADER_LENGTH + ntohs(pdu->params_len);
> + pdu->params_len = htons(params_len);
> +
> + return AVRCP_HEADER_LENGTH + params_len;
>
> reject:
> pdu->params_len = htons(1);
> --
> 1.8.3.2
Applied, thanks.
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [PATCH 1/3] unit/avrcp: Add /TP/CFG/BV-01-C test
From: Luiz Augusto von Dentz @ 2014-02-26 17:30 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <1393427669-15337-1-git-send-email-Andrei.Emeltchenko.news@gmail.com>
Hi Andrei,
On Wed, Feb 26, 2014 at 4:14 PM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> Test verifies that the get capabilities command issued from the
> Controller.
> ---
> unit/test-avrcp.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/unit/test-avrcp.c b/unit/test-avrcp.c
> index 53e9237..4a92860 100644
> --- a/unit/test-avrcp.c
> +++ b/unit/test-avrcp.c
> @@ -297,6 +297,16 @@ static void test_server(gconstpointer data)
> execute_context(context);
> }
>
> +static void test_client(gconstpointer data)
> +{
> + struct context *context = create_context(0x0100, data);
> +
> + if (g_str_equal(context->data->test_name, "/TP/CFG/BV-01-C"))
> + avrcp_get_capabilities(context->session, NULL);
> +
> + execute_context(context);
> +}
> +
> int main(int argc, char *argv[])
> {
> g_test_init(&argc, &argv, NULL);
> @@ -364,5 +374,12 @@ int main(int argc, char *argv[])
> raw_pdu(0x02, 0x11, 0x0e, 0x09, 0x48, 0x7c,
> AVC_PLAY | 0x80, 0x00));
>
> + /* Metadata transfer tests */
> +
> + define_test("/TP/CFG/BV-01-C", test_client,
> + raw_pdu(0x00, 0x11, 0x0e, 0x01, 0x48, 0x00,
> + 0x00, 0x19, 0x58, 0x10, 0x00, 0x00,
> + 0x01, 0x03));
> +
> return g_test_run();
> }
> --
> 1.8.3.2
Applied after doing a lot of changes, please don't copy code from
audio the library will most likely work differently as we do have
different APIs, also add code that you are actually testing for
example you were only testing part of the GetCapabilities handler and
doing a lot more work than necessary.
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [PATCHv2] android/avrcp: Add avrcp_get_capabilities request
From: Luiz Augusto von Dentz @ 2014-02-26 17:31 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <1393425134-12702-1-git-send-email-Andrei.Emeltchenko.news@gmail.com>
Hi Andrei,
On Wed, Feb 26, 2014 at 3:32 PM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> Implement avrcp_get_capabilities() request through
> avrcp_send_vendordep_req(). avctp_send_req() is not exported so we use
> the functions which are exported by AVCTP code.
> ---
> android/avrcp-lib.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> android/avrcp-lib.h | 2 ++
> android/avrcp.c | 1 +
> 3 files changed, 51 insertions(+)
>
> diff --git a/android/avrcp-lib.c b/android/avrcp-lib.c
> index 2e5a565..cf4cdaa 100644
> --- a/android/avrcp-lib.c
> +++ b/android/avrcp-lib.c
> @@ -50,6 +50,18 @@
> #define AVRCP_STATUS_NO_AVAILABLE_PLAYERS 0x15
> #define AVRCP_STATUS_ADDRESSED_PLAYER_CHANGED 0x16
>
> +/* Packet types */
> +#define AVRCP_PACKET_TYPE_SINGLE 0x00
> +#define AVRCP_PACKET_TYPE_START 0x01
> +#define AVRCP_PACKET_TYPE_CONTINUING 0x02
> +#define AVRCP_PACKET_TYPE_END 0x03
> +
> +/* Capabilities for AVRCP_GET_CAPABILITIES pdu */
> +#define CAP_COMPANY_ID 0x02
> +#define CAP_EVENTS_SUPPORTED 0x03
> +
> +#define AVRCP_GET_CAPABILITIES_PARAM_LENGTH 1
> +
> #if __BYTE_ORDER == __LITTLE_ENDIAN
>
> struct avrcp_header {
> @@ -92,6 +104,13 @@ static inline uint32_t ntoh24(const uint8_t src[3])
> #error "Unknown byte order"
> #endif
>
> +static inline void hton24(uint8_t dst[3], uint32_t src)
> +{
> + dst[0] = (src >> 16) & 0xff;
> + dst[1] = (src >> 8) & 0xff;
> + dst[2] = src & 0xff;
> +}
> +
> struct avrcp {
> struct avctp *conn;
>
> @@ -255,3 +274,32 @@ int avrcp_init_uinput(struct avrcp *session, const char *name,
> {
> return avctp_init_uinput(session->conn, name, address);
> }
> +
> +static int avrcp_send_vendordep_req(struct avrcp *session, uint8_t code,
> + uint8_t subunit, uint8_t *operands,
> + size_t operand_count,
> + avctp_rsp_cb func, void *user_data)
> +{
> + return avctp_send_vendordep_req(session->conn, code, subunit, operands,
> + operand_count, func, user_data);
> +}
> +
> +void avrcp_get_capabilities(struct avrcp *session, avctp_rsp_cb func)
> +{
> + uint8_t buf[AVRCP_HEADER_LENGTH + AVRCP_GET_CAPABILITIES_PARAM_LENGTH];
> + struct avrcp_header *pdu = (void *) buf;
> + uint8_t length;
> +
> + memset(buf, 0, sizeof(buf));
> +
> + hton24(pdu->company_id, IEEEID_BTSIG);
> + pdu->pdu_id = AVRCP_GET_CAPABILITIES;
> + pdu->packet_type = AVRCP_PACKET_TYPE_SINGLE;
> + pdu->params[0] = CAP_EVENTS_SUPPORTED;
> + pdu->params_len = htons(AVRCP_GET_CAPABILITIES_PARAM_LENGTH);
> +
> + length = AVRCP_HEADER_LENGTH + ntohs(pdu->params_len);
> +
> + avrcp_send_vendordep_req(session, AVC_CTYPE_STATUS, AVC_SUBUNIT_PANEL,
> + buf, length, func, session);
> +}
> diff --git a/android/avrcp-lib.h b/android/avrcp-lib.h
> index 2337429..4f3a632 100644
> --- a/android/avrcp-lib.h
> +++ b/android/avrcp-lib.h
> @@ -74,3 +74,5 @@ void avrcp_set_passthrough_handlers(struct avrcp *session,
> void *user_data);
> int avrcp_init_uinput(struct avrcp *session, const char *name,
> const char *address);
> +
> +void avrcp_get_capabilities(struct avrcp *session, avctp_rsp_cb func);
> diff --git a/android/avrcp.c b/android/avrcp.c
> index 48444a4..3d39d91 100644
> --- a/android/avrcp.c
> +++ b/android/avrcp.c
> @@ -36,6 +36,7 @@
> #include "bluetooth.h"
> #include "hal-msg.h"
> #include "ipc.h"
> +#include "avctp.h"
> #include "avrcp-lib.h"
> #include "avrcp.h"
>
> --
> 1.8.3.2
Applied after changing it quite a bit.
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [PATCH 03/17] Bluetooth: Stop scanning on LE connection
From: Andre Guedes @ 2014-02-26 19:34 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <C16121F0-C942-4E45-9D4A-089D22195D12@holtmann.org>
Hi Marcel,
On Wed, Feb 26, 2014 at 3:23 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Andre,
>
>> Some LE controllers don't support scanning and creating a connection
>> at the same time. So we should always stop scanning in order to
>> establish the connection.
>>
>> Since we may prematurely stop the discovery procedure in favor of
>> the connection establishment, we should also cancel hdev->le_scan_
>> disable delayed work and set the discovery state to DISCOVERY_STOPPED.
>>
>> This change does a small improvement since it is not mandatory the
>> user stops scanning before connecting anymore. Moreover, this change
>> is required by upcoming LE auto connection mechanism in order to work
>> properly with controllers that don't support background scanning and
>> connection establishment at the same time.
>>
>> In future, we might want to do a small optimization by checking if
>> controller is able to scan and connect at the same time. For now,
>> we want the simplest approach so we always stop scanning (even if
>> the controller is able to carry out both operations).
>>
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> include/net/bluetooth/hci.h | 1 +
>> net/bluetooth/hci_conn.c | 89 ++++++++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 88 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>> index 1bb45a4..c3834d3 100644
>> --- a/include/net/bluetooth/hci.h
>> +++ b/include/net/bluetooth/hci.h
>> @@ -356,6 +356,7 @@ enum {
>>
>> /* ---- HCI Error Codes ---- */
>> #define HCI_ERROR_AUTH_FAILURE 0x05
>> +#define HCI_ERROR_MEMORY_EXCEEDED 0x07
>> #define HCI_ERROR_CONNECTION_TIMEOUT 0x08
>> #define HCI_ERROR_REJ_BAD_ADDR 0x0f
>> #define HCI_ERROR_REMOTE_USER_TERM 0x13
>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
>> index dc8aad9..ae2c3e1 100644
>> --- a/net/bluetooth/hci_conn.c
>> +++ b/net/bluetooth/hci_conn.c
>> @@ -594,12 +594,83 @@ static int hci_create_le_conn(struct hci_conn *conn)
>> return 0;
>> }
>>
>> +static void hci_req_add_le_create_conn(struct hci_request *req,
>> + struct hci_conn *conn)
>> +{
>> + struct hci_cp_le_create_conn cp;
>> + struct hci_dev *hdev = conn->hdev;
>> + u8 own_addr_type;
>> +
>> + memset(&cp, 0, sizeof(cp));
>> +
>> + /* Update random address, but set require_privacy to false so
>> + * that we never connect with an unresolvable address.
>> + */
>> + if (hci_update_random_address(req, false, &own_addr_type))
>> + return;
>> +
>> + conn->src_type = own_addr_type;
>> +
>> + cp.scan_interval = cpu_to_le16(hdev->le_scan_interval);
>> + cp.scan_window = cpu_to_le16(hdev->le_scan_window);
>> + bacpy(&cp.peer_addr, &conn->dst);
>> + cp.peer_addr_type = conn->dst_type;
>> + cp.own_address_type = conn->src_type;
>
> the reason why you get the own_addr_type when setting the random address is to actually use it here.
>
> This is important since in cases where LE Privacy is enabled and we are using RPA, we want the random address used.
Year, I'm aware of it. 'own_addr_type' is assigned to
'conn->src_type'. Then, 'conn->src_type' is used in
'cp.own_address_type'.
IOW:
conn->src_type = own_addr_type;
...
cp.own_address_type = conn->src_type;
Or am I missing something?
BR,
Andre
^ permalink raw reply
* Re: [PATCH 08/17] Bluetooth: Introduce LE auto connection infrastructure
From: Andre Guedes @ 2014-02-26 19:34 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <BFCD757D-FF1F-40B4-AD64-61D6EC10C346@holtmann.org>
Hi Marcel,
On Wed, Feb 26, 2014 at 3:31 AM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
> Hi Andre,
>
>> This patch introduces the LE auto connection infrastructure which
>> will be used to implement the LE auto connection options.
>>
>> In summary, the auto connection mechanism works as follows: Once the
>> first pending LE connection is created, the background scanning is
>> started. When the target device is found in range, the kernel
>> autonomously starts the connection attempt. If connection is
>> established successfully, that pending LE connection is deleted and
>> the background is stopped.
>>
>> To achieve that, this patch introduces the hci_update_background_scan()
>> which controls the background scanning state. This function starts or
>> stops the background scanning based on the hdev->pend_le_conns list. If
>> there is no pending LE connection, the background scanning is stopped.
>> Otherwise, we start the background scanning.
>>
>> Then, every time a pending LE connection is added we call hci_update_
>> background_scan() so the background scanning is started (in case it is
>> not already running). Likewise, every time a pending LE connection is
>> deleted we call hci_update_background_scan() so the background scanning
>> is stopped (in case this was the last pending LE connection) or it is
>> started again (in case we have more pending LE connections). Finally,
>> we also call hci_update_background_scan() in hci_le_conn_failed() so
>> the background scan is restarted in case the connection establishment
>> fails. This way the background scanning keeps running until all pending
>> LE connection are established.
>>
>> At this point, resolvable addresses are not support by this
>> infrastructure. The proper support is added in upcoming patches.
>>
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> include/net/bluetooth/hci_core.h | 2 +
>> net/bluetooth/hci_conn.c | 5 +++
>> net/bluetooth/hci_core.c | 93 ++++++++++++++++++++++++++++++++++=
+++++-
>> net/bluetooth/hci_event.c | 38 ++++++++++++++++
>> 4 files changed, 136 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hc=
i_core.h
>> index e08405d..617cf49 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -806,6 +806,8 @@ void hci_pend_le_conn_add(struct hci_dev *hdev, bdad=
dr_t *addr, u8 addr_type);
>> void hci_pend_le_conn_del(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_=
type);
>> void hci_pend_le_conns_clear(struct hci_dev *hdev);
>>
>> +void hci_update_background_scan(struct hci_dev *hdev);
>> +
>> void hci_uuids_clear(struct hci_dev *hdev);
>>
>> void hci_link_keys_clear(struct hci_dev *hdev);
>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
>> index ccf4a4f..e79351c 100644
>> --- a/net/bluetooth/hci_conn.c
>> +++ b/net/bluetooth/hci_conn.c
>> @@ -527,6 +527,11 @@ void hci_le_conn_failed(struct hci_conn *conn, u8 s=
tatus)
>> hci_proto_connect_cfm(conn, status);
>>
>> hci_conn_del(conn);
>> +
>> + /* Since we may have temporarily stopped the background scanning i=
n
>> + * favor of connection establishment, we should restart it.
>> + */
>> + hci_update_background_scan(hdev);
>> }
>>
>> static void create_le_conn_complete(struct hci_dev *hdev, u8 status)
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 142ecd8..d242217 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -3281,7 +3281,7 @@ void hci_pend_le_conn_add(struct hci_dev *hdev, bd=
addr_t *addr, u8 addr_type)
>>
>> entry =3D hci_pend_le_conn_lookup(hdev, addr, addr_type);
>> if (entry)
>> - return;
>> + goto done;
>>
>> entry =3D kzalloc(sizeof(*entry), GFP_KERNEL);
>> if (!entry) {
>> @@ -3295,6 +3295,9 @@ void hci_pend_le_conn_add(struct hci_dev *hdev, bd=
addr_t *addr, u8 addr_type)
>> list_add(&entry->list, &hdev->pend_le_conns);
>>
>> BT_DBG("addr %pMR (type %u)", addr, addr_type);
>> +
>> +done:
>> + hci_update_background_scan(hdev);
>> }
>>
>> /* This function requires the caller holds hdev->lock */
>> @@ -3304,12 +3307,15 @@ void hci_pend_le_conn_del(struct hci_dev *hdev, =
bdaddr_t *addr, u8 addr_type)
>>
>> entry =3D hci_pend_le_conn_lookup(hdev, addr, addr_type);
>> if (!entry)
>> - return;
>> + goto done;
>>
>> list_del(&entry->list);
>> kfree(entry);
>>
>> BT_DBG("addr %pMR (type %u)", addr, addr_type);
>> +
>> +done:
>> + hci_update_background_scan(hdev);
>> }
>>
>> /* This function requires the caller holds hdev->lock */
>> @@ -4946,3 +4952,86 @@ void hci_req_add_le_scan_disable(struct hci_reque=
st *req)
>> cp.enable =3D LE_SCAN_DISABLE;
>> hci_req_add(req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
>> }
>> +
>> +static void update_background_scan_complete(struct hci_dev *hdev, u8 st=
atus)
>> +{
>> + if (status)
>> + BT_DBG("HCI request failed to update background scanning: =
"
>> + "status 0x%2.2x", status);
>> +}
>> +
>> +/* This function controls the background scanning based on hdev->pend_l=
e_conns
>> + * list. If there are pending LE connection we start the background sca=
nning,
>> + * otherwise we stop it.
>> + *
>> + * This function requires the caller holds hdev->lock.
>> + */
>> +void hci_update_background_scan(struct hci_dev *hdev)
>> +{
>> + struct hci_cp_le_set_scan_param param_cp;
>> + struct hci_cp_le_set_scan_enable enable_cp;
>> + struct hci_request req;
>> + struct hci_conn *conn;
>> + int err;
>> +
>> + hci_req_init(&req, hdev);
>> +
>> + if (list_empty(&hdev->pend_le_conns)) {
>> + /* If there is no pending LE connections, we should stop
>> + * the background scanning.
>> + */
>> +
>> + /* If controller is not scanning we are done. */
>> + if (!test_bit(HCI_LE_SCAN, &hdev->dev_flags))
>> + return;
>> +
>> + hci_req_add_le_scan_disable(&req);
>> +
>> + BT_DBG("%s stopping background scanning", hdev->name);
>> + } else {
>> + u8 own_addr_type;
>> +
>> + /* If there is at least one pending LE connection, we shou=
ld
>> + * keep the background scan running.
>> + */
>> +
>> + /* If controller is already scanning we are done. */
>> + if (test_bit(HCI_LE_SCAN, &hdev->dev_flags))
>> + return;
>> +
>> + /* If controller is connecting, we should not start scanni=
ng
>> + * since some controllers are not able to scan and connect=
at
>> + * the same time.
>> + */
>> + conn =3D hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONN=
ECT);
>> + if (conn)
>> + return;
>> +
>> + /* Use private address since remote doesn't need to identi=
fy us.
>> + * Strictly speaking, this is not required since no SCAN_R=
EQ is
>> + * sent in passive scanning.
>> + */
>> + if (hci_update_random_address(&req, true, &own_addr_type))
>> + return;
>
> the comment above is confusing. Reason is simple. We might use a RPA if L=
E Privacy has been enabled. The require_privacy =3D=3D true will fallback t=
o URPA in case privacy is not enabled. If privacy is enabled, then no matte=
r what require_privacy is set to, we will use a random address.
>
> It should say something along the lines of this:
>
> /* Set require_privacy to true to avoid identification from
> * unknown peer devices. Since this is passive scanning, no
> * SCAN_REQ using the local identity should be sent. Mandating
> * privacy is just an extra precaution.
> */
Ok, I'll fix this comment.
BR,
Andre
^ permalink raw reply
* Re: [PATCH 10/17] Bluetooth: Connection parameters and auto connection
From: Andre Guedes @ 2014-02-26 19:34 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <6791ADBE-F061-4979-97B8-CBCB119EA840@holtmann.org>
Hi Marcel,
On Wed, Feb 26, 2014 at 3:33 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Andre,
>
>> This patch modifies hci_conn_params_add() and hci_conn_params_del() so
>> they also add/delete pending LE connections according to the auto_
>> connect option. This way, background scan is automatically triggered/
>> untriggered when connection parameters are added/removed.
>>
>> For instance, when a new connection parameters with HCI_AUTO_CONN_ALWAYS
>> option is added and we are not connected to the device, we add a pending
>> LE connection for that device.
>>
>> Likewise, when the connection parameters are updated we add or delete
>> a pending LE connection according to its new auto_connect option.
>>
>> Finally, when the connection parameter is deleted we also delete the
>> pending LE connection (if any).
>
> what about disconnecting an existing connection for a device we have in our auto-connect list. I think that should happen as well.
This kind of logic seems to be more suitable if we implement it in the
upper layer (mgmt.c). The function that will handle connection
parameters removal would also terminate the connection in case there
is a connection for that device.
BR,
Andre
^ permalink raw reply
* Re: [PATCH 13/17] Bluetooth: Connection parameters and resolvable address
From: Andre Guedes @ 2014-02-26 19:34 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <7BB46396-5302-407C-874A-1DE043378DDE@holtmann.org>
Hi Marcel,
On Wed, Feb 26, 2014 at 3:37 AM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
> Hi Andre,
>
>> We should only add connection parameters for public, random static and
>> random private resolvable with IRK. If we allow non-resolvable or
>> resolvable without IRK, the background scan may run indefinitely. So, to
>> avoid this undesired behavior, we should check the address type in
>> hci_conn_params_add().
>
> this makes no sense. We should only allow auto-connection from public and=
static random addresses. These two are identity addresses.
>
> Every IRK has an identity address assigned to it. If you want to auto-con=
nect a device using a resolvable private address, then the identity address=
needs to be programmed into our auto-connection list. The RPA should never=
ever go there.
>
> That is how connect() works actually. You give it the identity address an=
d it will use the IRK to match it to the RPA in use. We need to do exactly =
the same.
>
> In addition please keep in mind that userspace only knows about RPA as lo=
ng as they are not identified. Once they are identified, the kernel will on=
ly tell us about identity addresses. The RPA will be in all mgmt commands a=
nd events automatically resolved.
Ok, so I'll change hci_conn_params_add() to accept public and random
static addresses only.
BR,
Andre
^ permalink raw reply
* Re: [PATCH 15/17] Bluetooth: Add le_auto_conn file on debugfs
From: Andre Guedes @ 2014-02-26 19:35 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <13751D39-57A3-4B04-9E1F-7BC4BE2DF4A7@holtmann.org>
Hi Marcel,
On Wed, Feb 26, 2014 at 3:41 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Andre,
>
>> This patch adds to debugfs the le_auto_conn file. This file will be
>> used to test LE auto connection infrastructure.
>>
>> To add a new auto connection address we write on le_auto_conn file
>> following the format <address> <address type> <auto_connect>.
>>
>> The <address type> values are:
>> * 0 for public address
>> * 1 for random address
>>
>> The <auto_connect> values are (for more details see struct hci_
>> conn_params):
>> * 0 for disabled
>> * 1 for always
>> * 2 for link loss
>>
>> So for instance, if you want the kernel autonomously establishes
>> connections with device AA:BB:CC:DD:EE:FF (public address) every
>> time the device enters in connectable mode (starts advertising),
>> you should run the command:
>> $ echo "AA:BB:CC:DD:EE:FF 0 1" > /sys/kernel/debug/bluetooth/hci0/le_auto_conn
>>
>> To get the list of connection parameters configured in kernel, read
>> the le_auto_conn file:
>> $ cat /sys/kernel/debug/bluetooth/hci0/le_auto_conn
>>
>> Finally, to clear the connection parameters list, write an empty
>> string:
>> $ echo "" > /sys/kernel/debug/bluetooth/hci0/le_auto_conn
>>
>> This file is created only if LE is enabled.
>
> I wonder if this should be prefixed with a command. For example like this:
>
> "add <address> <address_type> [auto_connect]"
> "del <address> <address_type>"
> "clr"
Year, no problem. I'll do like that.
BR,
Andre
^ permalink raw reply
* Re: [PATCH 16/17] Bluetooth: Create hci_req_add_le_passive_scan helper
From: Andre Guedes @ 2014-02-26 19:35 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <A552BAE1-F573-4940-9E30-CA7AF189D3DD@holtmann.org>
Hi Marcel,
On Wed, Feb 26, 2014 at 3:44 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Andre,
>
>> This patches creates the hci_req_add_le_passive_scan helper so it can
>> be re-used in the next patch.
>>
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> net/bluetooth/hci_core.c | 54 +++++++++++++++++++++++++++---------------------
>> 1 file changed, 30 insertions(+), 24 deletions(-)
>>
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index fb4c961..e776624 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -5095,6 +5095,35 @@ void hci_req_add_le_scan_disable(struct hci_request *req)
>> hci_req_add(req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
>> }
>>
>> +static void hci_req_add_le_passive_scan(struct hci_request *req,
>> + struct hci_dev *hdev)
>> +{
>
> struct hci_dev *hdev = req->hdev;
Sure, my bad.
Thanks,
Andre
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox