[PATCH BlueZ 1/5] doc/advertising-api: Add Flags property

[PATCH BlueZ 1/5] doc/advertising-api: Add Flags property

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This adds Flags which the application can use in case it want to set
it own flags.

Note: This would allow for example an application to advertise as
discoverable even if the adapter is not discoverable which may be
required by dual-mode as it may not require BR/EDR to be discoverable.
---
 doc/advertising-api.txt | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/doc/advertising-api.txt b/doc/advertising-api.txt
index eef98ad91..4e015f282 100644
--- a/doc/advertising-api.txt
+++ b/doc/advertising-api.txt
@@ -78,6 +78,21 @@ Properties	string Type
 				<Transport Discovery> <Organization Flags...>
 				0x26                   0x01         0x01...
 
+		array{byte} Flags [Experimental]
+
+			Advertising Flags to include. When present this will
+			override existing flags such as Discoverable.
+
+			Note: This property shall not be set when Type is set
+			to broadcast.
+
+			Possible value:
+				bit 0 - LE Limited Discoverable Mode
+				bit 1 - LE General Discoverable Mode
+				bit 2 - BR/EDR Not Supported
+				bit 3 - Simultaneous LE and BR/EDR (Controller)
+				bit 4 - Simultaneous LE and BR/EDR (Host)
+
 		array{string} Includes
 
 			List of features to be included in the advertising
-- 
2.14.3

[PATCH BlueZ 2/5] shared/ad: Add bt_ad_add_flags and bt_ad_clear_flags

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This adds basically functionality to add and remove AD flags.
---
 src/shared/ad.c | 92 +++++++++++++++++++++++++++++++++++++--------------------
 src/shared/ad.h |  4 +++
 2 files changed, 64 insertions(+), 32 deletions(-)

diff --git a/src/shared/ad.c b/src/shared/ad.c
index f0e62cef1..102fe0e13 100644
--- a/src/shared/ad.c
+++ b/src/shared/ad.c
@@ -127,6 +127,48 @@ void bt_ad_unref(struct bt_ad *ad)
 	free(ad);
 }
 
+static bool data_type_match(const void *data, const void *user_data)
+{
+	const struct bt_ad_data *a = data;
+	const uint8_t type = PTR_TO_UINT(user_data);
+
+	return a->type == type;
+}
+
+static bool ad_replace_data(struct bt_ad *ad, uint8_t type, void *data,
+							size_t len)
+{
+	struct bt_ad_data *new_data;
+
+	new_data = queue_find(ad->data, data_type_match, UINT_TO_PTR(type));
+	if (new_data) {
+		if (new_data->len == len && !memcmp(new_data->data, data, len))
+			return false;
+		new_data->data = realloc(new_data->data, len);
+		memcpy(new_data->data, data, len);
+		new_data->len = len;
+		return true;
+	}
+
+	new_data = new0(struct bt_ad_data, 1);
+	new_data->type = type;
+	new_data->data = malloc(len);
+	if (!new_data->data) {
+		free(new_data);
+		return false;
+	}
+
+	memcpy(new_data->data, data, len);
+	new_data->len = len;
+
+	if (queue_push_tail(ad->data, new_data))
+		return true;
+
+	data_destroy(new_data);
+
+	return false;
+}
+
 static size_t uuid_list_length(struct queue *uuid_queue)
 {
 	bool uuid16_included = false;
@@ -809,12 +851,25 @@ void bt_ad_clear_appearance(struct bt_ad *ad)
 	ad->appearance = UINT16_MAX;
 }
 
-static bool data_type_match(const void *data, const void *user_data)
+bool bt_ad_add_flags(struct bt_ad *ad, uint8_t *flags, size_t len)
 {
-	const struct bt_ad_data *a = data;
-	const uint8_t type = PTR_TO_UINT(user_data);
+	if (!ad)
+		return false;
 
-	return a->type == type;
+	/* TODO: Add table to check other flags */
+	if (len > 1 || flags[0] & 0xe0)
+		return false;
+
+	return ad_replace_data(ad, BT_AD_FLAGS, flags, len);
+}
+
+void bt_ad_clear_flags(struct bt_ad *ad)
+{
+	if (!ad)
+		return;
+
+	queue_remove_all(ad->data, data_type_match, UINT_TO_PTR(BT_AD_FLAGS),
+							data_destroy);
 }
 
 static uint8_t type_blacklist[] = {
@@ -862,7 +917,6 @@ static uint8_t type_blacklist[] = {
 
 bool bt_ad_add_data(struct bt_ad *ad, uint8_t type, void *data, size_t len)
 {
-	struct bt_ad_data *new_data;
 	size_t i;
 
 	if (!ad)
@@ -871,38 +925,12 @@ bool bt_ad_add_data(struct bt_ad *ad, uint8_t type, void *data, size_t len)
 	if (len > (MAX_ADV_DATA_LEN - 2))
 		return false;
 
-	new_data = queue_find(ad->data, data_type_match, UINT_TO_PTR(type));
-	if (new_data) {
-		if (new_data->len == len && !memcmp(new_data->data, data, len))
-			return false;
-		new_data->data = realloc(new_data->data, len);
-		memcpy(new_data->data, data, len);
-		new_data->len = len;
-		return true;
-	}
-
 	for (i = 0; i < sizeof(type_blacklist); i++) {
 		if (type == type_blacklist[i])
 			return false;
 	}
 
-	new_data = new0(struct bt_ad_data, 1);
-	new_data->type = type;
-	new_data->data = malloc(len);
-	if (!new_data->data) {
-		free(new_data);
-		return false;
-	}
-
-	memcpy(new_data->data, data, len);
-	new_data->len = len;
-
-	if (queue_push_tail(ad->data, new_data))
-		return true;
-
-	data_destroy(new_data);
-
-	return false;
+	return ad_replace_data(ad, type, data, len);
 }
 
 static bool data_match(const void *data, const void *user_data)
diff --git a/src/shared/ad.h b/src/shared/ad.h
index 8d705323b..0e30cdca9 100644
--- a/src/shared/ad.h
+++ b/src/shared/ad.h
@@ -147,6 +147,10 @@ bool bt_ad_add_appearance(struct bt_ad *ad, uint16_t appearance);
 
 void bt_ad_clear_appearance(struct bt_ad *ad);
 
+bool bt_ad_add_flags(struct bt_ad *ad, uint8_t *flags, size_t len);
+
+void bt_ad_clear_flags(struct bt_ad *ad);
+
 bool bt_ad_add_data(struct bt_ad *ad, uint8_t type, void *data, size_t len);
 
 bool bt_ad_has_data(struct bt_ad *ad, const struct bt_ad_data *data);
-- 
2.14.3

[PATCH BlueZ 3/5] advertising: Add implementation of Flags property

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This parses the contents of Flags property and add it to AD data
directly using bt_ad_add_flags.
---
 src/advertising.c | 43 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/src/advertising.c b/src/advertising.c
index 441d0a235..202b6eec5 100644
--- a/src/advertising.c
+++ b/src/advertising.c
@@ -641,6 +641,43 @@ fail:
 	return false;
 }
 
+static bool parse_flags(DBusMessageIter *iter, struct btd_adv_client *client)
+{
+	DBusMessageIter array;
+	uint8_t *flags;
+	int len;
+
+	if (!iter) {
+		bt_ad_clear_flags(client->data);
+		client->flags &= ~MGMT_ADV_FLAG_MANAGED_FLAGS;
+		return true;
+	}
+
+	if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_ARRAY)
+		return false;
+
+	dbus_message_iter_recurse(iter, &array);
+
+	if (dbus_message_iter_get_arg_type(&array) != DBUS_TYPE_BYTE)
+		goto fail;
+
+	dbus_message_iter_get_fixed_array(&array, &flags, &len);
+
+	if (!bt_ad_add_flags(client->data, flags, len))
+		goto fail;
+
+	DBG("Adding Flags 0x%02x", flags[0]);
+
+	client->flags |= MGMT_ADV_FLAG_MANAGED_FLAGS;
+
+	return true;
+
+fail:
+	bt_ad_clear_flags(client->data);
+	client->flags &= ~MGMT_ADV_FLAG_MANAGED_FLAGS;
+	return false;
+}
+
 static struct adv_parser {
 	const char *name;
 	bool (*func)(DBusMessageIter *iter, struct btd_adv_client *client);
@@ -656,6 +693,7 @@ static struct adv_parser {
 	{ "Duration", parse_duration },
 	{ "Timeout", parse_timeout },
 	{ "Data", parse_data },
+	{ "Flags", parse_flags },
 	{ },
 };
 
@@ -737,11 +775,12 @@ static int refresh_adv(struct btd_adv_client *client, mgmt_request_func_t func)
 	if (client->type == AD_TYPE_PERIPHERAL) {
 		flags = MGMT_ADV_FLAG_CONNECTABLE;
 
-		if (btd_adapter_get_discoverable(client->manager->adapter))
+		if (btd_adapter_get_discoverable(client->manager->adapter) &&
+				!(client->flags & MGMT_ADV_FLAG_MANAGED_FLAGS))
 			flags |= MGMT_ADV_FLAG_DISCOV;
 	}
 
-	flags |= client->flags;
+	flags |= client->flags & ~MGMT_ADV_FLAG_MANAGED_FLAGS;
 
 	adv_data = generate_adv_data(client, &flags, &adv_data_len);
 	if (!adv_data || (adv_data_len > calc_max_adv_len(client, flags))) {
-- 
2.14.3

[PATCH BlueZ 4/5] client: Add advertise.flags command

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This adds advertise.flags command which can be used to set it own
instance:

[bluetooth]# advertise.flags 0x02
[bluetooth]# advertise on

@ MGMT Command: Add Advertising (0x003e) plen 14
        Instance: 1
        Flags: 0x00000001
          Switch into Connectable mode
        Duration: 0
        Timeout: 0
        Advertising data length: 3
        Flags: 0x02
          LE General Discoverable Mode
        Scan response length: 0
< HCI Command: LE Set Advertising Data (0x08|0x0008) plen 32
        Length: 3
        Flags: 0x02
          LE General Discoverable Mode
---
 client/advertising.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 client/advertising.h |  1 +
 client/main.c        |  7 +++++++
 3 files changed, 55 insertions(+)

diff --git a/client/advertising.c b/client/advertising.c
index 045133aa3..a0fc32eea 100644
--- a/client/advertising.c
+++ b/client/advertising.c
@@ -71,6 +71,7 @@ static struct ad {
 	struct service_data service;
 	struct manufacturer_data manufacturer;
 	struct data data;
+	uint8_t flags[1];
 	bool tx_power;
 	bool name;
 	bool appearance;
@@ -401,6 +402,26 @@ static gboolean get_data(const GDBusPropertyTable *property,
 	return TRUE;
 }
 
+static gboolean flags_exists(const GDBusPropertyTable *property, void *data)
+{
+	return ad.flags[0];
+}
+
+static gboolean get_flags(const GDBusPropertyTable *property,
+					DBusMessageIter *iter, void *user_data)
+{
+	DBusMessageIter array;
+	uint8_t *val = ad.flags;
+
+	dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY, "y", &array);
+
+	dbus_message_iter_append_fixed_array(&array, DBUS_TYPE_BYTE, &val, 1);
+
+	dbus_message_iter_close_container(iter, &array);
+
+	return TRUE;
+}
+
 static const GDBusPropertyTable ad_props[] = {
 	{ "Type", "s", get_type },
 	{ "ServiceUUIDs", "as", get_uuids, NULL, uuids_exists },
@@ -408,6 +429,7 @@ static const GDBusPropertyTable ad_props[] = {
 	{ "ManufacturerData", "a{qv}", get_manufacturer_data, NULL,
 						manufacturer_data_exists },
 	{ "Data", "a{yv}", get_data, NULL, data_exists },
+	{ "Flags", "ay", get_flags, NULL, flags_exists },
 	{ "Includes", "as", get_includes, NULL, includes_exists },
 	{ "LocalName", "s", get_local_name, NULL, local_name_exits },
 	{ "Appearance", "q", get_appearance, NULL, appearance_exits },
@@ -708,6 +730,31 @@ void ad_disable_data(DBusConnection *conn)
 	return bt_shell_noninteractive_quit(EXIT_SUCCESS);
 }
 
+void ad_advertise_flags(DBusConnection *conn, int argc, char *argv[])
+{
+	char *endptr = NULL;
+	long int val;
+
+	if (argc < 2 || !strlen(argv[1])) {
+		if (ad.flags[0])
+			bt_shell_hexdump(ad.flags, sizeof(ad.flags));
+
+		return bt_shell_noninteractive_quit(EXIT_SUCCESS);
+	}
+
+	val = strtol(argv[1], &endptr, 0);
+	if (!endptr || *endptr != '\0' || val > UINT8_MAX) {
+		bt_shell_printf("Invalid value\n");
+		return bt_shell_noninteractive_quit(EXIT_FAILURE);
+	}
+
+	ad.flags[0] = val;
+
+	g_dbus_emit_property_changed(conn, AD_PATH, AD_IFACE, "Flags");
+
+	return bt_shell_noninteractive_quit(EXIT_SUCCESS);
+}
+
 void ad_advertise_tx_power(DBusConnection *conn, dbus_bool_t *value)
 {
 	if (!value) {
diff --git a/client/advertising.h b/client/advertising.h
index 12b4d69c1..42e51c693 100644
--- a/client/advertising.h
+++ b/client/advertising.h
@@ -39,3 +39,4 @@ void ad_advertise_duration(DBusConnection *conn, long int *value);
 void ad_advertise_timeout(DBusConnection *conn, long int *value);
 void ad_advertise_data(DBusConnection *conn, int argc, char *argv[]);
 void ad_disable_data(DBusConnection *conn);
+void ad_advertise_flags(DBusConnection *conn, int argc, char *argv[]);
diff --git a/client/main.c b/client/main.c
index 54bd5371a..52ed159ad 100644
--- a/client/main.c
+++ b/client/main.c
@@ -2192,6 +2192,11 @@ static void cmd_advertise_data(int argc, char *argv[])
 	ad_advertise_data(dbus_conn, argc, argv);
 }
 
+static void cmd_advertise_flags(int argc, char *argv[])
+{
+	ad_advertise_flags(dbus_conn, argc, argv);
+}
+
 static void cmd_advertise_tx_power(int argc, char *argv[])
 {
 	dbus_bool_t powered;
@@ -2382,6 +2387,8 @@ static const struct bt_shell_menu advertise_menu = {
 			"Set/Get advertise manufacturer data" },
 	{ "data", "[type] [data=xx xx ...]", cmd_advertise_data,
 			"Set/Get advertise data" },
+	{ "flags", "[value]", cmd_advertise_flags,
+			"Set/Get advertise flags" },
 	{ "tx-power", "[on/off]", cmd_advertise_tx_power,
 			"Show/Enable/Disable TX power to be advertised",
 							NULL },
-- 
2.14.3

[PATCH BlueZ 5/5] client: Print AD Data and Flags once registered

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This prints both Data and Flags if advertise command succeeds:

[bluetooth]# advertise.data 0x26 0x01 0x00
[bluetooth]# advertise.flags 0x02
[bluetooth]# advertise on
Advertising object registered
Data Type: 0x26
  01 00                                            ..
Flags:
  02                                               .
Tx Power: off
Name: off
Apperance: off
---
 client/advertising.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/client/advertising.c b/client/advertising.c
index a0fc32eea..44ffd4a94 100644
--- a/client/advertising.c
+++ b/client/advertising.c
@@ -164,6 +164,16 @@ static void print_ad(void)
 						ad.manufacturer.data.len);
 	}
 
+	if (ad.data.data.len) {
+		bt_shell_printf("Data Type: 0x%02x\n", ad.data.type);
+		bt_shell_hexdump(ad.data.data.data, ad.data.data.len);
+	}
+
+	if (ad.flags[0]) {
+		bt_shell_printf("Flags:\n");
+		bt_shell_hexdump(ad.flags, sizeof(ad.flags));
+	}
+
 	bt_shell_printf("Tx Power: %s\n", ad.tx_power ? "on" : "off");
 
 	if (ad.local_name)
-- 
2.14.3

Re: [PATCH BlueZ 1/5] doc/advertising-api: Add Flags property

[Luiz Augusto von Dentz]

Hi David,
On Mon, May 7, 2018 at 7:14 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com>
wrote:

> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

> This adds Flags which the application can use in case it want to set
> it own flags.

> Note: This would allow for example an application to advertise as
> discoverable even if the adapter is not discoverable which may be
> required by dual-mode as it may not require BR/EDR to be discoverable.
> ---
>   doc/advertising-api.txt | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)

> diff --git a/doc/advertising-api.txt b/doc/advertising-api.txt
> index eef98ad91..4e015f282 100644
> --- a/doc/advertising-api.txt
> +++ b/doc/advertising-api.txt
> @@ -78,6 +78,21 @@ Properties   string Type
>                                  <Transport Discovery> <Organization
Flags...>
>                                  0x26                   0x01
0x01...

> +               array{byte} Flags [Experimental]
> +
> +                       Advertising Flags to include. When present this
will
> +                       override existing flags such as Discoverable.
> +
> +                       Note: This property shall not be set when Type is
set
> +                       to broadcast.
> +
> +                       Possible value:
> +                               bit 0 - LE Limited Discoverable Mode
> +                               bit 1 - LE General Discoverable Mode
> +                               bit 2 - BR/EDR Not Supported
> +                               bit 3 - Simultaneous LE and BR/EDR
(Controller)
> +                               bit 4 - Simultaneous LE and BR/EDR (Host)
> +
>                  array{string} Includes

>                          List of features to be included in the advertising
> --
> 2.14.3

Any feedback this set?


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 1/5] doc/advertising-api: Add Flags property

[Marcel Holtmann]

Hi Luiz,

> This adds Flags which the application can use in case it want to set
> it own flags.
> 
> Note: This would allow for example an application to advertise as
> discoverable even if the adapter is not discoverable which may be
> required by dual-mode as it may not require BR/EDR to be discoverable.
> ---
> doc/advertising-api.txt | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
> 
> diff --git a/doc/advertising-api.txt b/doc/advertising-api.txt
> index eef98ad91..4e015f282 100644
> --- a/doc/advertising-api.txt
> +++ b/doc/advertising-api.txt
> @@ -78,6 +78,21 @@ Properties	string Type
> 				<Transport Discovery> <Organization Flags...>
> 				0x26                   0x01         0x01...
> 
> +		array{byte} Flags [Experimental]
> +
> +			Advertising Flags to include. When present this will
> +			override existing flags such as Discoverable.
> +
> +			Note: This property shall not be set when Type is set
> +			to broadcast.
> +
> +			Possible value:
> +				bit 0 - LE Limited Discoverable Mode
> +				bit 1 - LE General Discoverable Mode
> +				bit 2 - BR/EDR Not Supported
> +				bit 3 - Simultaneous LE and BR/EDR (Controller)
> +				bit 4 - Simultaneous LE and BR/EDR (Host)
> +

I think this is the wrong approach. Exposing this low-level part is just giving an API to circumvent and violate the Bluetooth spec. You are inviting apps to create bad advertising data. So NAK.

Regards

Marcel

Re: [PATCH BlueZ 1/5] doc/advertising-api: Add Flags property

[David Llewellyn-Jones]

Hi Luiz,

On 08/05/18 08:40, Luiz Augusto von Dentz wrote:
> Hi David,
> On Mon, May 7, 2018 at 7:14 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com>
> wrote:
> 
>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
>> This adds Flags which the application can use in case it want to set
>> it own flags.
> 
>> Note: This would allow for example an application to advertise as
>> discoverable even if the adapter is not discoverable which may be
>> required by dual-mode as it may not require BR/EDR to be discoverable.
>> ---
>>   doc/advertising-api.txt | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
> 
>> diff --git a/doc/advertising-api.txt b/doc/advertising-api.txt
>> index eef98ad91..4e015f282 100644
>> --- a/doc/advertising-api.txt
>> +++ b/doc/advertising-api.txt
>> @@ -78,6 +78,21 @@ Properties   string Type
>>                                  <Transport Discovery> <Organization
> Flags...>
>>                                  0x26                   0x01
> 0x01...
> 
>> +               array{byte} Flags [Experimental]
>> +
>> +                       Advertising Flags to include. When present this
> will
>> +                       override existing flags such as Discoverable.
>> +
>> +                       Note: This property shall not be set when Type is
> set
>> +                       to broadcast.
>> +
>> +                       Possible value:
>> +                               bit 0 - LE Limited Discoverable Mode
>> +                               bit 1 - LE General Discoverable Mode
>> +                               bit 2 - BR/EDR Not Supported
>> +                               bit 3 - Simultaneous LE and BR/EDR
> (Controller)
>> +                               bit 4 - Simultaneous LE and BR/EDR (Host)
>> +
>>                  array{string} Includes
> 
>>                          List of features to be included in the advertising
>> --
>> 2.14.3
> 
> Any feedback this set?

Thanks for making the patches. This certainly looks like it would
achieve what I need. My use of it would be to read the flags, OR bit 1
(general discovery) and write them back again, so as not to affect the
other flags.

I plan to test it by applying your patches to my local version, but I've
not yet had the chance. I'll comment back as soon as I have.

David
-- 
Website: http://www.flypig.co.uk

Re: [PATCH BlueZ 1/5] doc/advertising-api: Add Flags property

[Luiz Augusto von Dentz]

Hi Marcel,
On Tue, May 8, 2018 at 10:53 AM Marcel Holtmann <marcel@holtmann.org> wrote:

> Hi Luiz,

> > This adds Flags which the application can use in case it want to set
> > it own flags.
> >
> > Note: This would allow for example an application to advertise as
> > discoverable even if the adapter is not discoverable which may be
> > required by dual-mode as it may not require BR/EDR to be discoverable.
> > ---
> > doc/advertising-api.txt | 15 +++++++++++++++
> > 1 file changed, 15 insertions(+)
> >
> > diff --git a/doc/advertising-api.txt b/doc/advertising-api.txt
> > index eef98ad91..4e015f282 100644
> > --- a/doc/advertising-api.txt
> > +++ b/doc/advertising-api.txt
> > @@ -78,6 +78,21 @@ Properties string Type
> >                               <Transport Discovery> <Organization
Flags...>
> >                               0x26                   0x01
0x01...
> >
> > +             array{byte} Flags [Experimental]
> > +
> > +                     Advertising Flags to include. When present this
will
> > +                     override existing flags such as Discoverable.
> > +
> > +                     Note: This property shall not be set when Type is
set
> > +                     to broadcast.
> > +
> > +                     Possible value:
> > +                             bit 0 - LE Limited Discoverable Mode
> > +                             bit 1 - LE General Discoverable Mode
> > +                             bit 2 - BR/EDR Not Supported
> > +                             bit 3 - Simultaneous LE and BR/EDR
(Controller)
> > +                             bit 4 - Simultaneous LE and BR/EDR (Host)
> > +

> I think this is the wrong approach. Exposing this low-level part is just
giving an API to circumvent and violate the Bluetooth spec. You are
inviting apps to create bad advertising data. So NAK.

Then we should find some alternative, perhaps only allow certain flags to
be set this way or change the property to just Discoverable.

> Regards

> Marcel



-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 1/5] doc/advertising-api: Add Flags property

[Marcel Holtmann]

Hi Luiz,

>>> This adds Flags which the application can use in case it want to set
>>> it own flags.
>>> 
>>> Note: This would allow for example an application to advertise as
>>> discoverable even if the adapter is not discoverable which may be
>>> required by dual-mode as it may not require BR/EDR to be discoverable.
>>> ---
>>> doc/advertising-api.txt | 15 +++++++++++++++
>>> 1 file changed, 15 insertions(+)
>>> 
>>> diff --git a/doc/advertising-api.txt b/doc/advertising-api.txt
>>> index eef98ad91..4e015f282 100644
>>> --- a/doc/advertising-api.txt
>>> +++ b/doc/advertising-api.txt
>>> @@ -78,6 +78,21 @@ Properties string Type
>>>                              <Transport Discovery> <Organization
> Flags...>
>>>                              0x26                   0x01
> 0x01...
>>> 
>>> +             array{byte} Flags [Experimental]
>>> +
>>> +                     Advertising Flags to include. When present this
> will
>>> +                     override existing flags such as Discoverable.
>>> +
>>> +                     Note: This property shall not be set when Type is
> set
>>> +                     to broadcast.
>>> +
>>> +                     Possible value:
>>> +                             bit 0 - LE Limited Discoverable Mode
>>> +                             bit 1 - LE General Discoverable Mode
>>> +                             bit 2 - BR/EDR Not Supported
>>> +                             bit 3 - Simultaneous LE and BR/EDR
> (Controller)
>>> +                             bit 4 - Simultaneous LE and BR/EDR (Host)
>>> +
> 
>> I think this is the wrong approach. Exposing this low-level part is just
> giving an API to circumvent and violate the Bluetooth spec. You are
> inviting apps to create bad advertising data. So NAK.
> 
> Then we should find some alternative, perhaps only allow certain flags to
> be set this way or change the property to just Discoverable.

I would just add a single property Discoverable that maps to General Discoverable Mode. If you want to get extra credits, then adding DiscoverableTimeout (like we have in adapter-api.txt) would allow to switch it back from discoverable to connectable.

Regards

Marcel

Re: [PATCH BlueZ 1/5] doc/advertising-api: Add Flags property

[Luiz Augusto von Dentz]

Hi Marcel,

On Tue, May 8, 2018 at 2:17 PM Marcel Holtmann <marcel@holtmann.org> wrote:

> Hi Luiz,

> >>> This adds Flags which the application can use in case it want to set
> >>> it own flags.
> >>>
> >>> Note: This would allow for example an application to advertise as
> >>> discoverable even if the adapter is not discoverable which may be
> >>> required by dual-mode as it may not require BR/EDR to be discoverable.
> >>> ---
> >>> doc/advertising-api.txt | 15 +++++++++++++++
> >>> 1 file changed, 15 insertions(+)
> >>>
> >>> diff --git a/doc/advertising-api.txt b/doc/advertising-api.txt
> >>> index eef98ad91..4e015f282 100644
> >>> --- a/doc/advertising-api.txt
> >>> +++ b/doc/advertising-api.txt
> >>> @@ -78,6 +78,21 @@ Properties string Type
> >>>                              <Transport Discovery> <Organization
> > Flags...>
> >>>                              0x26                   0x01
> > 0x01...
> >>>
> >>> +             array{byte} Flags [Experimental]
> >>> +
> >>> +                     Advertising Flags to include. When present this
> > will
> >>> +                     override existing flags such as Discoverable.
> >>> +
> >>> +                     Note: This property shall not be set when Type
is
> > set
> >>> +                     to broadcast.
> >>> +
> >>> +                     Possible value:
> >>> +                             bit 0 - LE Limited Discoverable Mode
> >>> +                             bit 1 - LE General Discoverable Mode
> >>> +                             bit 2 - BR/EDR Not Supported
> >>> +                             bit 3 - Simultaneous LE and BR/EDR
> > (Controller)
> >>> +                             bit 4 - Simultaneous LE and BR/EDR
(Host)
> >>> +
> >
> >> I think this is the wrong approach. Exposing this low-level part is
just
> > giving an API to circumvent and violate the Bluetooth spec. You are
> > inviting apps to create bad advertising data. So NAK.
> >
> > Then we should find some alternative, perhaps only allow certain flags
to
> > be set this way or change the property to just Discoverable.

> I would just add a single property Discoverable that maps to General
Discoverable Mode. If you want to get extra credits, then adding
DiscoverableTimeout (like we have in adapter-api.txt) would allow to switch
it back from discoverable to connectable.

Alright, perhaps we could maintain it as Flags and only allow certain flags
to be set by the application since I do intend to use the same interface to
expose device advertisement so we can remove the AdvertisingData and
AdvertisingFlags from the Device interface. Regarding the Timeout we
already have something similar with Duration, or do you think that there
are use cases that we cannot fulfill? For bit 3-4 I guess we never set
those, but BR/EDR Not Supported could be useful for the application to
control the bearer it wants the remote to connect to.

> Regards

> Marcel



-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 1/5] doc/advertising-api: Add Flags property

[David Llewellyn-Jones]

Hi Luiz, Marcel,

On 11/05/18 12:12, Luiz Augusto von Dentz wrote:
> Hi Marcel,
> 
> On Tue, May 8, 2018 at 2:17 PM Marcel Holtmann <marcel@holtmann.org> wrote:
> 
>> Hi Luiz,
> 
>>>>> This adds Flags which the application can use in case it want to set
>>>>> it own flags.
>>>>>
>>>>> Note: This would allow for example an application to advertise as
>>>>> discoverable even if the adapter is not discoverable which may be
>>>>> required by dual-mode as it may not require BR/EDR to be discoverable.
>>>>> ---
>>>>> doc/advertising-api.txt | 15 +++++++++++++++
>>>>> 1 file changed, 15 insertions(+)
>>>>>
>>>>> diff --git a/doc/advertising-api.txt b/doc/advertising-api.txt
>>>>> index eef98ad91..4e015f282 100644
>>>>> --- a/doc/advertising-api.txt
>>>>> +++ b/doc/advertising-api.txt
>>>>> @@ -78,6 +78,21 @@ Properties string Type
>>>>>                              <Transport Discovery> <Organization
>>> Flags...>
>>>>>                              0x26                   0x01
>>> 0x01...
>>>>>
>>>>> +             array{byte} Flags [Experimental]
>>>>> +
>>>>> +                     Advertising Flags to include. When present this
>>> will
>>>>> +                     override existing flags such as Discoverable.
>>>>> +
>>>>> +                     Note: This property shall not be set when Type
> is
>>> set
>>>>> +                     to broadcast.
>>>>> +
>>>>> +                     Possible value:
>>>>> +                             bit 0 - LE Limited Discoverable Mode
>>>>> +                             bit 1 - LE General Discoverable Mode
>>>>> +                             bit 2 - BR/EDR Not Supported
>>>>> +                             bit 3 - Simultaneous LE and BR/EDR
>>> (Controller)
>>>>> +                             bit 4 - Simultaneous LE and BR/EDR
> (Host)
>>>>> +
>>>
>>>> I think this is the wrong approach. Exposing this low-level part is
> just
>>> giving an API to circumvent and violate the Bluetooth spec. You are
>>> inviting apps to create bad advertising data. So NAK.
>>>
>>> Then we should find some alternative, perhaps only allow certain flags
> to
>>> be set this way or change the property to just Discoverable.
> 
>> I would just add a single property Discoverable that maps to General
> Discoverable Mode. If you want to get extra credits, then adding
> DiscoverableTimeout (like we have in adapter-api.txt) would allow to switch
> it back from discoverable to connectable.
> 
> Alright, perhaps we could maintain it as Flags and only allow certain flags
> to be set by the application since I do intend to use the same interface to
> expose device advertisement so we can remove the AdvertisingData and
> AdvertisingFlags from the Device interface. Regarding the Timeout we
> already have something similar with Duration, or do you think that there
> are use cases that we cannot fulfill? For bit 3-4 I guess we never set
> those, but BR/EDR Not Supported could be useful for the application to
> control the bearer it wants the remote to connect to.

As an end-user of this interface, I think I'd value it left as flags,
matching the format of the flags in the spec itself. Even if some of the
flags are read-only, having it match with the spec helps greatly in
understanding what's going on.

Although I fully appreciate the need to avoid allowing violations of the
spec, I'm afraid my understanding of it isn't good enough to contribute.
For my own use, I only need control over bit 0, but I can also
anticipate other situations in which controlling bits 1 and 2 would be
useful too. So I'd support Luiz's suggestion.

David
-- 
Website: http://www.flypig.co.uk

Re: [PATCH BlueZ 1/5] doc/advertising-api: Add Flags property

[Marcel Holtmann]

Hi Luiz,

> 
>>>>> This adds Flags which the application can use in case it want to set
>>>>> it own flags.
>>>>> 
>>>>> Note: This would allow for example an application to advertise as
>>>>> discoverable even if the adapter is not discoverable which may be
>>>>> required by dual-mode as it may not require BR/EDR to be discoverable.
>>>>> ---
>>>>> doc/advertising-api.txt | 15 +++++++++++++++
>>>>> 1 file changed, 15 insertions(+)
>>>>> 
>>>>> diff --git a/doc/advertising-api.txt b/doc/advertising-api.txt
>>>>> index eef98ad91..4e015f282 100644
>>>>> --- a/doc/advertising-api.txt
>>>>> +++ b/doc/advertising-api.txt
>>>>> @@ -78,6 +78,21 @@ Properties string Type
>>>>>                             <Transport Discovery> <Organization
>>> Flags...>
>>>>>                             0x26                   0x01
>>> 0x01...
>>>>> 
>>>>> +             array{byte} Flags [Experimental]
>>>>> +
>>>>> +                     Advertising Flags to include. When present this
>>> will
>>>>> +                     override existing flags such as Discoverable.
>>>>> +
>>>>> +                     Note: This property shall not be set when Type
> is
>>> set
>>>>> +                     to broadcast.
>>>>> +
>>>>> +                     Possible value:
>>>>> +                             bit 0 - LE Limited Discoverable Mode
>>>>> +                             bit 1 - LE General Discoverable Mode
>>>>> +                             bit 2 - BR/EDR Not Supported
>>>>> +                             bit 3 - Simultaneous LE and BR/EDR
>>> (Controller)
>>>>> +                             bit 4 - Simultaneous LE and BR/EDR
> (Host)
>>>>> +
>>> 
>>>> I think this is the wrong approach. Exposing this low-level part is
> just
>>> giving an API to circumvent and violate the Bluetooth spec. You are
>>> inviting apps to create bad advertising data. So NAK.
>>> 
>>> Then we should find some alternative, perhaps only allow certain flags
> to
>>> be set this way or change the property to just Discoverable.
> 
>> I would just add a single property Discoverable that maps to General
> Discoverable Mode. If you want to get extra credits, then adding
> DiscoverableTimeout (like we have in adapter-api.txt) would allow to switch
> it back from discoverable to connectable.
> 
> Alright, perhaps we could maintain it as Flags and only allow certain flags
> to be set by the application since I do intend to use the same interface to
> expose device advertisement so we can remove the AdvertisingData and
> AdvertisingFlags from the Device interface. Regarding the Timeout we
> already have something similar with Duration, or do you think that there
> are use cases that we cannot fulfill? For bit 3-4 I guess we never set
> those, but BR/EDR Not Supported could be useful for the application to
> control the bearer it wants the remote to connect to.

so I would do bool Discoverable and unit16 DiscoverableTimeout. The duration is for the advertising duration. Which means if the DiscoverableTimeout and duration is set then after DiscoverableTimeout it would switch back to being connectable only.

The BR/EDR Not Supported can not be in application control. You would need to ensure that a static address is used and the BR/EDR side is really separated from the LE side. As I said before, we do not want to turn this into something that leads to violating GAP and BlueZ would be the bad implemented device because of it.

It is also important to note that if we are LE only, then this is required to be always set. However that is main.conf master switch. So don’t go there. If we ever want to have a split-personality between BR/EDR and LE, then we do that with a total different and new interface. We don’t hack this into existing ones. Assume these are dual-mode most of the time.

Regards

Marcel

Re: [PATCH BlueZ 1/5] doc/advertising-api: Add Flags property

[Marcel Holtmann]

Hi David,

>> 
>>>>>> This adds Flags which the application can use in case it want to set
>>>>>> it own flags.
>>>>>> 
>>>>>> Note: This would allow for example an application to advertise as
>>>>>> discoverable even if the adapter is not discoverable which may be
>>>>>> required by dual-mode as it may not require BR/EDR to be discoverable.
>>>>>> ---
>>>>>> doc/advertising-api.txt | 15 +++++++++++++++
>>>>>> 1 file changed, 15 insertions(+)
>>>>>> 
>>>>>> diff --git a/doc/advertising-api.txt b/doc/advertising-api.txt
>>>>>> index eef98ad91..4e015f282 100644
>>>>>> --- a/doc/advertising-api.txt
>>>>>> +++ b/doc/advertising-api.txt
>>>>>> @@ -78,6 +78,21 @@ Properties string Type
>>>>>>                             <Transport Discovery> <Organization
>>>> Flags...>
>>>>>>                             0x26                   0x01
>>>> 0x01...
>>>>>> 
>>>>>> +             array{byte} Flags [Experimental]
>>>>>> +
>>>>>> +                     Advertising Flags to include. When present this
>>>> will
>>>>>> +                     override existing flags such as Discoverable.
>>>>>> +
>>>>>> +                     Note: This property shall not be set when Type
>> is
>>>> set
>>>>>> +                     to broadcast.
>>>>>> +
>>>>>> +                     Possible value:
>>>>>> +                             bit 0 - LE Limited Discoverable Mode
>>>>>> +                             bit 1 - LE General Discoverable Mode
>>>>>> +                             bit 2 - BR/EDR Not Supported
>>>>>> +                             bit 3 - Simultaneous LE and BR/EDR
>>>> (Controller)
>>>>>> +                             bit 4 - Simultaneous LE and BR/EDR
>> (Host)
>>>>>> +
>>>> 
>>>>> I think this is the wrong approach. Exposing this low-level part is
>> just
>>>> giving an API to circumvent and violate the Bluetooth spec. You are
>>>> inviting apps to create bad advertising data. So NAK.
>>>> 
>>>> Then we should find some alternative, perhaps only allow certain flags
>> to
>>>> be set this way or change the property to just Discoverable.
>> 
>>> I would just add a single property Discoverable that maps to General
>> Discoverable Mode. If you want to get extra credits, then adding
>> DiscoverableTimeout (like we have in adapter-api.txt) would allow to switch
>> it back from discoverable to connectable.
>> 
>> Alright, perhaps we could maintain it as Flags and only allow certain flags
>> to be set by the application since I do intend to use the same interface to
>> expose device advertisement so we can remove the AdvertisingData and
>> AdvertisingFlags from the Device interface. Regarding the Timeout we
>> already have something similar with Duration, or do you think that there
>> are use cases that we cannot fulfill? For bit 3-4 I guess we never set
>> those, but BR/EDR Not Supported could be useful for the application to
>> control the bearer it wants the remote to connect to.
> 
> As an end-user of this interface, I think I'd value it left as flags,
> matching the format of the flags in the spec itself. Even if some of the
> flags are read-only, having it match with the spec helps greatly in
> understanding what's going on.

that part I disagree. These flags are GAP details and not really application specific.

> Although I fully appreciate the need to avoid allowing violations of the
> spec, I'm afraid my understanding of it isn't good enough to contribute.
> For my own use, I only need control over bit 0, but I can also
> anticipate other situations in which controlling bits 1 and 2 would be
> useful too. So I'd support Luiz's suggestion.

See my response to Luiz since that is really not the case. If you operate as dual-mode device you have to do things correctly. Split-personality situations need a different interface. It can not be bolted onto this one.

Regards

Marcel

Re: [PATCH BlueZ 1/5] doc/advertising-api: Add Flags property

[David Llewellyn-Jones]

Hi Marcel,

On 11/05/18 16:34, Marcel Holtmann wrote:
> Hi David,
> 
>>>
>>>>>>> This adds Flags which the application can use in case it want to set
>>>>>>> it own flags.
>>>>>>>
>>>>>>> Note: This would allow for example an application to advertise as
>>>>>>> discoverable even if the adapter is not discoverable which may be
>>>>>>> required by dual-mode as it may not require BR/EDR to be discoverable.
>>>>>>> ---
>>>>>>> doc/advertising-api.txt | 15 +++++++++++++++
>>>>>>> 1 file changed, 15 insertions(+)
>>>>>>>
>>>>>>> diff --git a/doc/advertising-api.txt b/doc/advertising-api.txt
>>>>>>> index eef98ad91..4e015f282 100644
>>>>>>> --- a/doc/advertising-api.txt
>>>>>>> +++ b/doc/advertising-api.txt
>>>>>>> @@ -78,6 +78,21 @@ Properties string Type
>>>>>>>                             <Transport Discovery> <Organization
>>>>> Flags...>
>>>>>>>                             0x26                   0x01
>>>>> 0x01...
>>>>>>>
>>>>>>> +             array{byte} Flags [Experimental]
>>>>>>> +
>>>>>>> +                     Advertising Flags to include. When present this
>>>>> will
>>>>>>> +                     override existing flags such as Discoverable.
>>>>>>> +
>>>>>>> +                     Note: This property shall not be set when Type
>>> is
>>>>> set
>>>>>>> +                     to broadcast.
>>>>>>> +
>>>>>>> +                     Possible value:
>>>>>>> +                             bit 0 - LE Limited Discoverable Mode
>>>>>>> +                             bit 1 - LE General Discoverable Mode
>>>>>>> +                             bit 2 - BR/EDR Not Supported
>>>>>>> +                             bit 3 - Simultaneous LE and BR/EDR
>>>>> (Controller)
>>>>>>> +                             bit 4 - Simultaneous LE and BR/EDR
>>> (Host)
>>>>>>> +
>>>>>
>>>>>> I think this is the wrong approach. Exposing this low-level part is
>>> just
>>>>> giving an API to circumvent and violate the Bluetooth spec. You are
>>>>> inviting apps to create bad advertising data. So NAK.
>>>>>
>>>>> Then we should find some alternative, perhaps only allow certain flags
>>> to
>>>>> be set this way or change the property to just Discoverable.
>>>
>>>> I would just add a single property Discoverable that maps to General
>>> Discoverable Mode. If you want to get extra credits, then adding
>>> DiscoverableTimeout (like we have in adapter-api.txt) would allow to switch
>>> it back from discoverable to connectable.
>>>
>>> Alright, perhaps we could maintain it as Flags and only allow certain flags
>>> to be set by the application since I do intend to use the same interface to
>>> expose device advertisement so we can remove the AdvertisingData and
>>> AdvertisingFlags from the Device interface. Regarding the Timeout we
>>> already have something similar with Duration, or do you think that there
>>> are use cases that we cannot fulfill? For bit 3-4 I guess we never set
>>> those, but BR/EDR Not Supported could be useful for the application to
>>> control the bearer it wants the remote to connect to.
>>
>> As an end-user of this interface, I think I'd value it left as flags,
>> matching the format of the flags in the spec itself. Even if some of the
>> flags are read-only, having it match with the spec helps greatly in
>> understanding what's going on.
> 
> that part I disagree. These flags are GAP details and not really application specific.

Okay, that's fair enough.

I can only speak from my own experience writing application code. The
disassociation between the flags in the advertisement, and their control
through the adapter interface, was confusing.

I had to associate the flags as output by btmon with the data payload in
the advertisement, and the fact that the receiving bluez was filtering
out the advertisements as a result, before I could understand how to get
the result I needed.

But to be clear, ultimately, my only current concern is to be able to
control discoverability of the advertisement, so as long as I can do
this I'll be happy! The rest I'm sharing just to provide the context for
what I wrote.

>> Although I fully appreciate the need to avoid allowing violations of the
>> spec, I'm afraid my understanding of it isn't good enough to contribute.
>> For my own use, I only need control over bit 0, but I can also
>> anticipate other situations in which controlling bits 1 and 2 would be
>> useful too. So I'd support Luiz's suggestion.
> 
> See my response to Luiz since that is really not the case. If you operate as dual-mode device you have to do things correctly. Split-personality situations need a different interface. It can not be bolted onto this one.

Thanks for taking the time to reply. I'll have a read of your response
to Luiz.

David
-- 
Website: http://www.flypig.co.uk