2014-10-29 19:52:28

by Jakub Pawlowski

[permalink] [raw]
Subject: [PATCH v1 1/2] lib: add start and stop discovery

This patch adds start and stop discovery definitions for new kernel
method.

Signed-off-by: Jakub Pawlowski <[email protected]>
---
lib/mgmt.h | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)

diff --git a/lib/mgmt.h b/lib/mgmt.h
index 46766a9..8f37937 100644
--- a/lib/mgmt.h
+++ b/lib/mgmt.h
@@ -445,6 +445,32 @@ struct mgmt_cp_set_public_address {
bdaddr_t bdaddr;
} __packed;

+#define MGMT_OP_START_SERVICE_DISCOVERY 0x003A
+
+#define MGMT_RANGE_NONE 0X00
+#define MGMT_RANGE_RSSI 0X01
+#define MGMT_RANGE_PATHLOSS 0X02
+
+struct mgmt_uuid_filter {
+ uint8_t range_method;
+ int8_t pathloss;
+ int8_t rssi;
+ uint8_t uuid[16];
+} __packed;
+
+struct mgmt_cp_start_service_discovery {
+ uint8_t type;
+ uint16_t filter_count;
+ struct mgmt_uuid_filter filter[0];
+} __packed;
+#define MGMT_START_SERVICE_DISCOVERY_SIZE 1
+
+#define MGMT_OP_STOP_SERVICE_DISCOVERY 0x003B
+struct mgmt_cp_stop_service_discovery {
+ uint8_t type;
+} __packed;
+#define MGMT_STOP_SERVICE_DISCOVERY_SIZE 1
+
#define MGMT_EV_CMD_COMPLETE 0x0001
struct mgmt_ev_cmd_complete {
uint16_t opcode;
--
2.1.0.rc2.206.gedb03e5



2014-10-29 19:52:29

by Jakub Pawlowski

[permalink] [raw]
Subject: [PATCH v1 2/2] service-find command for new kernel method usage

Example usage:
Find all low energy devices advertising service with UUID 'abcd' and
TxPower, that have pathloss less than 60 dB
btmgmt find-service -u abcd -p 60 -l

Find all low energy devices advertising service with UUID 'abcd', and
dont advertise TxPower and with RSSI over -50, or advertising TxPower
and pathloss less than 60 dB
btmgmt find-service -u abcd -r -50 -p 60 -l

Signed-off-by: Jakub Pawlowski <[email protected]>
---
tools/btmgmt.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 128 insertions(+)

diff --git a/tools/btmgmt.c b/tools/btmgmt.c
index 3326a46..45e4c51 100644
--- a/tools/btmgmt.c
+++ b/tools/btmgmt.c
@@ -1547,6 +1547,133 @@ static void cmd_con(struct mgmt *mgmt, uint16_t index, int argc, char **argv)
}
}

+static void find_service_rsp(uint8_t status, uint16_t len, const void *param,
+ void *user_data)
+{
+ free(user_data);
+ if (status != 0) {
+ fprintf(stderr,
+ "Unable to start service discovery. status 0x%02x (%s)\n",
+ status, mgmt_errstr(status));
+ mainloop_quit();
+ return;
+ }
+
+ printf("Service discovery started\n");
+ discovery = true;
+}
+
+static void find_service_usage(void)
+{
+ printf("Usage: btmgmt find-service -u UUID [-r RSSI_Threshold] [-p Pathloss_Threshold] [-l|-b]>\n");
+}
+
+static struct option find_service_options[] = {
+ { "help", no_argument, 0, 'h' },
+ { "le-only", no_argument, 0, 'l' },
+ { "bredr-only", no_argument, 0, 'b' },
+ { "uuid", required_argument, 0, 'u' },
+ { "rssi", required_argument, 0, 'r' },
+ { "pathloss", required_argument, 0, 'p' },
+ { 0, 0, 0, 0 }
+};
+
+static void uuid_to_uuid128(uuid_t *uuid128, const uuid_t *uuid);
+
+static void cmd_find_service(struct mgmt *mgmt, uint16_t index, int argc, char **argv)
+{
+ struct mgmt_cp_start_service_discovery *cp;
+ struct mgmt_uuid_filter *filter;
+ uint8_t type;
+ int opt;
+ int total_size = sizeof(struct mgmt_cp_start_service_discovery) + sizeof(struct mgmt_uuid_filter);
+ uuid_t uuid;
+ uint128_t uint128;
+ uuid_t uuid128;
+
+ if (index == MGMT_INDEX_NONE)
+ index = 0;
+
+ type = 0;
+ hci_set_bit(BDADDR_BREDR, &type);
+ hci_set_bit(BDADDR_LE_PUBLIC, &type);
+ hci_set_bit(BDADDR_LE_RANDOM, &type);
+
+ cp = malloc(total_size);
+ if(cp == NULL) {
+ fprintf(stderr, "Unable to allocate memory for mgmt_cp_start_service_discovery structure.\n");
+ }
+
+ memset(cp, 0, total_size);
+
+ filter = cp->filter;
+
+ if(argc == 1) {
+ find_service_usage();
+ exit(EXIT_FAILURE);
+ }
+
+ while ((opt = getopt_long(argc, argv, "+lbu:r:p:h", find_service_options,
+ NULL)) != -1) {
+ switch (opt) {
+ case 'l':
+ hci_clear_bit(BDADDR_BREDR, &type);
+ hci_set_bit(BDADDR_LE_PUBLIC, &type);
+ hci_set_bit(BDADDR_LE_RANDOM, &type);
+ break;
+ case 'b':
+ hci_set_bit(BDADDR_BREDR, &type);
+ hci_clear_bit(BDADDR_LE_PUBLIC, &type);
+ hci_clear_bit(BDADDR_LE_RANDOM, &type);
+ break;
+ case 'u':
+ if (bt_string2uuid(&uuid, optarg) < 0) {
+ printf("Invalid UUID: %s\n", optarg);
+ exit(EXIT_FAILURE);
+ }
+ uuid_to_uuid128(&uuid128, &uuid);
+ ntoh128((uint128_t *) uuid128.value.uuid128.data, &uint128);
+ htob128(&uint128, (uint128_t *) filter->uuid);
+ break;
+ case 'r':
+ filter->rssi = atoi(optarg);
+ filter->range_method |= MGMT_RANGE_RSSI;
+ printf("rssi filter: %d %d", filter->rssi, filter->range_method);
+ break;
+ case 'p':
+ filter->pathloss = atoi(optarg);
+ filter->range_method |= MGMT_RANGE_PATHLOSS;
+ printf("pathloss filter: %d %d", filter->pathloss, filter->range_method);
+ break;
+ case 'h':
+ find_service_usage();
+ exit(EXIT_SUCCESS);
+ default:
+ find_service_usage();
+ exit(EXIT_FAILURE);
+ }
+ }
+
+ argc -= optind;
+ argv += optind;
+ optind = 0;
+
+ if (argc > 0) {
+ find_service_usage();
+ exit(EXIT_FAILURE);
+ }
+
+ cp->type = type;
+ cp->filter_count = 1;
+
+ if (mgmt_send(mgmt, MGMT_OP_START_SERVICE_DISCOVERY, index, total_size, cp,
+ find_service_rsp, cp, NULL) == 0) {
+ free(cp);
+ fprintf(stderr, "Unable to send start_service_discovery cmd\n");
+ exit(EXIT_FAILURE);
+ }
+}
+
static void find_rsp(uint8_t status, uint16_t len, const void *param,
void *user_data)
{
@@ -2867,6 +2994,7 @@ static struct {
{ "disconnect", cmd_disconnect, "Disconnect device" },
{ "con", cmd_con, "List connections" },
{ "find", cmd_find, "Discover nearby devices" },
+ { "find-service", cmd_find_service, "Discover nearby service" },
{ "name", cmd_name, "Set local name" },
{ "pair", cmd_pair, "Pair with a remote device" },
{ "cancelpair", cmd_cancel_pair,"Cancel pairing" },
--
2.1.0.rc2.206.gedb03e5


2014-11-04 06:39:18

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH v1 2/2] service-find command for new kernel method usage

Hi Jakub,

On Mon, Nov 03, 2014, Jakub Pawlowski wrote:
> On Sun, Nov 2, 2014 at 5:23 AM, Johan Hedberg <[email protected]> wrote:
> > Hi Jakub,
> >
> > On Wed, Oct 29, 2014, Jakub Pawlowski wrote:
> >> +static void uuid_to_uuid128(uuid_t *uuid128, const uuid_t *uuid);
> >
> > Please just move up this function instead of creating a
> > forward-declaration for it.
> >
> >> +static void cmd_find_service(struct mgmt *mgmt, uint16_t index, int argc, char **argv)
> >> +{
> >> + struct mgmt_cp_start_service_discovery *cp;
> >> + struct mgmt_uuid_filter *filter;
> >> + uint8_t type;
> >> + int opt;
> >> + int total_size = sizeof(struct mgmt_cp_start_service_discovery) + sizeof(struct mgmt_uuid_filter);
> >> + uuid_t uuid;
> >> + uint128_t uint128;
> >> + uuid_t uuid128;
> >> +
> >> + if (index == MGMT_INDEX_NONE)
> >> + index = 0;
> >> +
> >> + type = 0;
> >> + hci_set_bit(BDADDR_BREDR, &type);
> >> + hci_set_bit(BDADDR_LE_PUBLIC, &type);
> >> + hci_set_bit(BDADDR_LE_RANDOM, &type);
> >> +
> >> + cp = malloc(total_size);
> >> + if(cp == NULL) {
> >
> > Missing space after 'if', however I wonder is there any point to do
> > dynamic allocation here since you always just have just one filter
> > entry. Having the send buffer on the stack might create simpler code
> > here.
>
> mgmt_cp_start_service_discovery have array of mgmt_uuid_filter at the
> end, and mgmt_send internally just do one malloc and memcpy to copy
> the whole data.
> Right now I just reserve all needed space. If I use stack variable how
> would I make sure that mgmt_uuid_filter is exactly at end of
> mgmt_cp_start_service_discovery ?

Something like the following should work:

struct mgmt_cp_start_service_discovery *cp;
struct mgmt_uuid_filter *filter;
uint8_t buf[sizeof(*cp) + sizeof(*filter)];

cp = (void *) buf;
filter = cp->filter;

...

This kind of style has been used in a few other places, however in this
case a possibly better approach (resulting in a bit simpler code) would
be to use alloca().

Johan

2014-11-03 20:50:58

by Jakub Pawlowski

[permalink] [raw]
Subject: Re: [PATCH v1 2/2] service-find command for new kernel method usage

On Sun, Nov 2, 2014 at 5:23 AM, Johan Hedberg <[email protected]> wrote:
> Hi Jakub,
>
> On Wed, Oct 29, 2014, Jakub Pawlowski wrote:
>> +static void uuid_to_uuid128(uuid_t *uuid128, const uuid_t *uuid);
>
> Please just move up this function instead of creating a
> forward-declaration for it.
>
>> +static void cmd_find_service(struct mgmt *mgmt, uint16_t index, int argc, char **argv)
>> +{
>> + struct mgmt_cp_start_service_discovery *cp;
>> + struct mgmt_uuid_filter *filter;
>> + uint8_t type;
>> + int opt;
>> + int total_size = sizeof(struct mgmt_cp_start_service_discovery) + sizeof(struct mgmt_uuid_filter);
>> + uuid_t uuid;
>> + uint128_t uint128;
>> + uuid_t uuid128;
>> +
>> + if (index == MGMT_INDEX_NONE)
>> + index = 0;
>> +
>> + type = 0;
>> + hci_set_bit(BDADDR_BREDR, &type);
>> + hci_set_bit(BDADDR_LE_PUBLIC, &type);
>> + hci_set_bit(BDADDR_LE_RANDOM, &type);
>> +
>> + cp = malloc(total_size);
>> + if(cp == NULL) {
>
> Missing space after 'if', however I wonder is there any point to do
> dynamic allocation here since you always just have just one filter
> entry. Having the send buffer on the stack might create simpler code
> here.

mgmt_cp_start_service_discovery have array of mgmt_uuid_filter at the
end, and mgmt_send internally just do one malloc and memcpy to copy
the whole data.
Right now I just reserve all needed space. If I use stack variable how
would I make sure that mgmt_uuid_filter is exactly at end of
mgmt_cp_start_service_discovery ?

I've fixed all other issues, and would re-send my patch after making
sure that Marcel is fine with mgmt API as I defined it.

>
>> + fprintf(stderr, "Unable to allocate memory for mgmt_cp_start_service_discovery structure.\n");
>> + }
>
> Wouldn't you need to return from the function in this case?
>
>> + memset(cp, 0, total_size);
>> +
>> + filter = cp->filter;
>> +
>> + if(argc == 1) {
>
> Missing space after 'if'.
>
>> + if (mgmt_send(mgmt, MGMT_OP_START_SERVICE_DISCOVERY, index, total_size, cp,
>> + find_service_rsp, cp, NULL) == 0) {
>> + free(cp);
>> + fprintf(stderr, "Unable to send start_service_discovery cmd\n");
>> + exit(EXIT_FAILURE);
>> + }
>> +}
>
> mgmt_send allocates its own buffer for the send parameters so there's no
> need for you to keep your own buffer around until the command completion
> callback (where all you do is free it). However, this whole issue would
> go away if you'd just use a stack variable.
>
> Johan

2014-11-02 13:18:50

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH v1 1/2] lib: add start and stop discovery

Hi Jakub,

On Wed, Oct 29, 2014, Jakub Pawlowski wrote:
> This patch adds start and stop discovery definitions for new kernel
> method.
>
> Signed-off-by: Jakub Pawlowski <[email protected]>
> ---
> lib/mgmt.h | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)

I'd first like to see a patch to properly update doc/mgmt-api.txt with
this new API and only then start making any code changes. Did you reach
a consensus on the version Marcel sent on Oct 10th? To me that thread
looked unfinished (looking at Marcel's email on Oct 20th).

A couple of minor cosmetic change suggestions:

> diff --git a/lib/mgmt.h b/lib/mgmt.h
> index 46766a9..8f37937 100644
> --- a/lib/mgmt.h
> +++ b/lib/mgmt.h
> @@ -445,6 +445,32 @@ struct mgmt_cp_set_public_address {
> bdaddr_t bdaddr;
> } __packed;
>
> +#define MGMT_OP_START_SERVICE_DISCOVERY 0x003A

It seems you could use one tab less to align the value here which should
make it line up with the other (at least nearby) opcode values.

> +#define MGMT_RANGE_NONE 0X00
> +#define MGMT_RANGE_RSSI 0X01
> +#define MGMT_RANGE_PATHLOSS 0X02

Please use lower-case 'x' for consistency here. You can also align these
(with tabs and not spaces like you now use) with nearby define values.

Johan

2014-11-02 13:23:32

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH v1 2/2] service-find command for new kernel method usage

Hi Jakub,

On Wed, Oct 29, 2014, Jakub Pawlowski wrote:
> +static void uuid_to_uuid128(uuid_t *uuid128, const uuid_t *uuid);

Please just move up this function instead of creating a
forward-declaration for it.

> +static void cmd_find_service(struct mgmt *mgmt, uint16_t index, int argc, char **argv)
> +{
> + struct mgmt_cp_start_service_discovery *cp;
> + struct mgmt_uuid_filter *filter;
> + uint8_t type;
> + int opt;
> + int total_size = sizeof(struct mgmt_cp_start_service_discovery) + sizeof(struct mgmt_uuid_filter);
> + uuid_t uuid;
> + uint128_t uint128;
> + uuid_t uuid128;
> +
> + if (index == MGMT_INDEX_NONE)
> + index = 0;
> +
> + type = 0;
> + hci_set_bit(BDADDR_BREDR, &type);
> + hci_set_bit(BDADDR_LE_PUBLIC, &type);
> + hci_set_bit(BDADDR_LE_RANDOM, &type);
> +
> + cp = malloc(total_size);
> + if(cp == NULL) {

Missing space after 'if', however I wonder is there any point to do
dynamic allocation here since you always just have just one filter
entry. Having the send buffer on the stack might create simpler code
here.

> + fprintf(stderr, "Unable to allocate memory for mgmt_cp_start_service_discovery structure.\n");
> + }

Wouldn't you need to return from the function in this case?

> + memset(cp, 0, total_size);
> +
> + filter = cp->filter;
> +
> + if(argc == 1) {

Missing space after 'if'.

> + if (mgmt_send(mgmt, MGMT_OP_START_SERVICE_DISCOVERY, index, total_size, cp,
> + find_service_rsp, cp, NULL) == 0) {
> + free(cp);
> + fprintf(stderr, "Unable to send start_service_discovery cmd\n");
> + exit(EXIT_FAILURE);
> + }
> +}

mgmt_send allocates its own buffer for the send parameters so there's no
need for you to keep your own buffer around until the command completion
callback (where all you do is free it). However, this whole issue would
go away if you'd just use a stack variable.

Johan