Adds a method to add the TX Power attribute to the advertisements that are sent
through the D-Bus interface, and supports more than one advertisement based on
the maximum number reported by the kernel.
Michael Janssen (4):
doc: Add IncludeTXPower to LE Advertising API
core/advertising: Parse IncludeTXPower
core/advertising: Support more than one advertisement.
test: add IncludeTXPower to example-advertisement
doc/advertising-api.txt | 5 +++
src/advertising.c | 86 +++++++++++++++++++++++++++++++++++++++++-----
test/example-advertisement | 4 +++
3 files changed, 87 insertions(+), 8 deletions(-)
--
2.2.0.rc0.207.ga3a616c
Hi all,
> On Mon, Apr 13, 2015 at 11:27 AM, Michael Janssen <[email protected]> =
wrote:
> Hi Marcel:
>
> On Mon, Apr 13, 2015 at 11:16 AM, Marcel Holtmann <[email protected]> w=
rote:
>> Hi Michael,
>>
>>>>> Adds a method to add the TX Power attribute to the advertisements tha=
t are sent
>>>>> through the D-Bus interface, and supports more than one advertisement=
based on
>>>>> the maximum number reported by the kernel.
>>>>
>>>> That doesn't say why we should expose the IncludeTXPower, I thought we
>>>> would let bluetoothd figure if that should be included or not, we may
>>>> not have enough space or don't have this information from the
>>>> controller.
>>>>
>>>
>>> This just exposes the flag that is available on the MGMT API. If we
>>> don't have a reason to turn it on and off from user space, we should
>>> remove that flag too.
>>
>> the kernel side needs this flag. You can not emulate it from userspace. =
The reason is that userspace has no idea what the actual TX power is. Only =
the kernel knows. If you build the value in userspace by yourself, then you=
are plain faking / hardcoding it.
>
> We are not emulating it from userspace. This property signals to the
> kernel to include the power. It mirrors the MGMT API flag on the Add
> Advertising Command:
>
> 4 Add TX Power field to Adv_Data
>
> We do not build the value in userspace, but just set this flag when
> the property is set.
>
Michael is simply using this property to determine whether or not the
MGMT_ADV_FLAG_TX_POWER should passed to the mgmt command. I think such
an API makes sense, but did you have something different in mind like
always setting this flag from D-Bus?
>>
>> Regards
>>
>> Marcel
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth=
" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Somewhat unrelated to the discussion but I think there is a bug in how
register_advertisement determines to return "AlreadyExists" as an
error. It looks like you're simply comparing object paths but you
should really be comparing the object path AND the sender. Basically
object paths are unique to each external application so if two of
these want to use an identical object path that should be OK.
Thanks,
Arman
Hi Marcel:
On Mon, Apr 13, 2015 at 11:16 AM, Marcel Holtmann <[email protected]> wro=
te:
> Hi Michael,
>
>>>> Adds a method to add the TX Power attribute to the advertisements that=
are sent
>>>> through the D-Bus interface, and supports more than one advertisement =
based on
>>>> the maximum number reported by the kernel.
>>>
>>> That doesn't say why we should expose the IncludeTXPower, I thought we
>>> would let bluetoothd figure if that should be included or not, we may
>>> not have enough space or don't have this information from the
>>> controller.
>>>
>>
>> This just exposes the flag that is available on the MGMT API. If we
>> don't have a reason to turn it on and off from user space, we should
>> remove that flag too.
>
> the kernel side needs this flag. You can not emulate it from userspace. T=
he reason is that userspace has no idea what the actual TX power is. Only t=
he kernel knows. If you build the value in userspace by yourself, then you =
are plain faking / hardcoding it.
We are not emulating it from userspace. This property signals to the
kernel to include the power. It mirrors the MGMT API flag on the Add
Advertising Command:
4 Add TX Power field to Adv_Data
We do not build the value in userspace, but just set this flag when
the property is set.
>
> Regards
>
> Marcel
>
Hi Michael,
>>> Adds a method to add the TX Power attribute to the advertisements that are sent
>>> through the D-Bus interface, and supports more than one advertisement based on
>>> the maximum number reported by the kernel.
>>
>> That doesn't say why we should expose the IncludeTXPower, I thought we
>> would let bluetoothd figure if that should be included or not, we may
>> not have enough space or don't have this information from the
>> controller.
>>
>
> This just exposes the flag that is available on the MGMT API. If we
> don't have a reason to turn it on and off from user space, we should
> remove that flag too.
the kernel side needs this flag. You can not emulate it from userspace. The reason is that userspace has no idea what the actual TX power is. Only the kernel knows. If you build the value in userspace by yourself, then you are plain faking / hardcoding it.
Regards
Marcel
Hi Luiz,
On Mon, Apr 13, 2015 at 6:38 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Michael,
>
> On Fri, Apr 10, 2015 at 10:06 PM, Michael Janssen <[email protected]> wrote:
>> Adds a method to add the TX Power attribute to the advertisements that are sent
>> through the D-Bus interface, and supports more than one advertisement based on
>> the maximum number reported by the kernel.
>
> That doesn't say why we should expose the IncludeTXPower, I thought we
> would let bluetoothd figure if that should be included or not, we may
> not have enough space or don't have this information from the
> controller.
>
This just exposes the flag that is available on the MGMT API. If we
don't have a reason to turn it on and off from user space, we should
remove that flag too.
I would rather get an error when I want to include TX Power and there
isn't enough space to include it instead of having to discover by
testing that it is not there when I want to use it.
>> Michael Janssen (4):
>> doc: Add IncludeTXPower to LE Advertising API
>> core/advertising: Parse IncludeTXPower
>> core/advertising: Support more than one advertisement.
>> test: add IncludeTXPower to example-advertisement
>>
>> doc/advertising-api.txt | 5 +++
>> src/advertising.c | 86 +++++++++++++++++++++++++++++++++++++++++-----
>> test/example-advertisement | 4 +++
>> 3 files changed, 87 insertions(+), 8 deletions(-)
>>
>> --
>> 2.2.0.rc0.207.ga3a616c
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Luiz Augusto von Dentz
--
M Janssen
Hi Michael,
On Fri, Apr 10, 2015 at 10:06 PM, Michael Janssen <[email protected]> wrote:
> Adds a method to add the TX Power attribute to the advertisements that are sent
> through the D-Bus interface, and supports more than one advertisement based on
> the maximum number reported by the kernel.
That doesn't say why we should expose the IncludeTXPower, I thought we
would let bluetoothd figure if that should be included or not, we may
not have enough space or don't have this information from the
controller.
> Michael Janssen (4):
> doc: Add IncludeTXPower to LE Advertising API
> core/advertising: Parse IncludeTXPower
> core/advertising: Support more than one advertisement.
> test: add IncludeTXPower to example-advertisement
>
> doc/advertising-api.txt | 5 +++
> src/advertising.c | 86 +++++++++++++++++++++++++++++++++++++++++-----
> test/example-advertisement | 4 +++
> 3 files changed, 87 insertions(+), 8 deletions(-)
>
> --
> 2.2.0.rc0.207.ga3a616c
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Luiz Augusto von Dentz
Hi Michael,
On Friday 10 of April 2015 12:06:06 Michael Janssen wrote:
> Parse the IncludeTXPower property of the advertisement object, and
> pass the appropriate flag to MGMT if it is set.
>
> Uses MGMT Read Advertising Features Command to determine the maximum
> length allowed.
> ---
> src/advertising.c | 79
> +++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 74
> insertions(+), 5 deletions(-)
>
> diff --git a/src/advertising.c b/src/advertising.c
> index 2f8e539..8acd5b4 100644
> --- a/src/advertising.c
> +++ b/src/advertising.c
> @@ -46,6 +46,7 @@ struct btd_advertising {
> struct queue *ads;
> struct mgmt *mgmt;
> uint16_t mgmt_index;
> + uint8_t max_adv_len;
> };
>
> #define AD_TYPE_BROADCAST 0
> @@ -59,6 +60,7 @@ struct advertisement {
> GDBusProxy *proxy;
> DBusMessage *reg;
> uint8_t type; /* Advertising type */
> + bool include_tx_power;
> struct bt_ad *data;
> uint8_t instance;
> };
> @@ -361,6 +363,22 @@ fail:
> return false;
> }
>
> +static bool parse_advertising_include_tx_power(GDBusProxy *proxy,
> + bool *included)
> +{
> + DBusMessageIter iter;
> +
> + if (!g_dbus_proxy_get_property(proxy, "IncludeTXPower", &iter))
> + return true;
> +
> + if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_BOOLEAN)
> + return false;
> +
> + dbus_message_iter_get_basic(&iter, included);
Due to historical reasons dbus_bool_t is 4 bytes long (check dbus-types.h)
To be on a safe side I'd do:
dbus_bool_t b;
dbus_message_iter_get_basic(&iter, &b);
*included = b;
> +
> + return true;
> +}
> +
> static void add_advertising_callback(uint8_t status, uint16_t length,
> const void *param, void *user_data)
> {
> @@ -375,19 +393,44 @@ static void add_advertising_callback(uint8_t status,
> uint16_t length, ad->instance = rp->instance;
> }
>
> +static size_t calc_max_adv_len(struct advertisement *ad,
> + uint32_t flags)
nitpick: this should fit in single line
> +{
> + size_t max = ad->manager->max_adv_len;
> +
> + if (flags & MGMT_ADV_FLAG_TX_POWER)
> + max -= 3;
At least comment on where those 3/4 bytes came from.
> +
> + if (flags & (MGMT_ADV_FLAG_DISCOV | MGMT_ADV_FLAG_LIMITED_DISCOV |
> + MGMT_ADV_FLAG_MANAGED_FLAGS))
> + max -= 3;
> +
> + if (flags & MGMT_ADV_FLAG_APPEARANCE)
> + max -= 4;
> +
> + return max;
> +}
> +
> static DBusMessage *refresh_advertisement(struct advertisement *ad)
> {
> struct mgmt_cp_add_advertising *cp;
> uint8_t param_len;
> uint8_t *adv_data;
> size_t adv_data_len;
> + uint32_t flags = 0;
>
> DBG("Refreshing advertisement: %s", ad->path);
>
> + if (ad->type == AD_TYPE_PERIPHERAL)
> + flags = MGMT_ADV_FLAG_CONNECTABLE | MGMT_ADV_FLAG_DISCOV;
> +
> + if (ad->include_tx_power)
> + flags |= MGMT_ADV_FLAG_TX_POWER;
> +
> adv_data = bt_ad_generate(ad->data, &adv_data_len);
>
> - if (!adv_data) {
> - error("Advertising data couldn't be generated.");
> + if (!adv_data || (adv_data_len > calc_max_adv_len(ad, flags))) {
> + error("Advertising data too long or couldn't be generated.");
>
> return g_dbus_create_error(ad->reg, ERROR_INTERFACE
> ".InvalidLength",
> @@ -406,9 +449,7 @@ static DBusMessage *refresh_advertisement(struct
> advertisement *ad) return btd_error_failed(ad->reg, "Failed");
> }
>
> - if (ad->type == AD_TYPE_PERIPHERAL)
> - cp->flags = MGMT_ADV_FLAG_CONNECTABLE | MGMT_ADV_FLAG_DISCOV;
> -
> + cp->flags = flags;
> cp->instance = ad->instance;
> cp->adv_data_len = adv_data_len;
> memcpy(cp->data, adv_data, adv_data_len);
> @@ -457,6 +498,12 @@ static DBusMessage *parse_advertisement(struct
> advertisement *ad) goto fail;
> }
>
> + if (!parse_advertising_include_tx_power(ad->proxy,
> + &ad->include_tx_power)) {
> + error("Property \"IncludeTXPower\" failed to parse");
> + goto fail;
> + }
> +
> return refresh_advertisement(ad);
>
> fail:
> @@ -636,6 +683,20 @@ static void advertising_manager_destroy(void
> *user_data) free(manager);
> }
>
> +static void read_adv_features_callback(uint8_t status, uint16_t length,
> + const void *param, void *user_data)
> +{
> + struct btd_advertising *manager = user_data;
> + const struct mgmt_rp_read_adv_features *feat = param;
> +
> + if (status || !param) {
> + error("Failed to read advertising features");
I'd print status error here as well.
> + return;
> + }
Usually we check if daemon got enough bytes in response before accessing it:
if (length < sizeof(*rp)) {
error("Wrong size of read adv features response");
return;
}
> +
> + manager->max_adv_len = feat->max_adv_data_len;
> +}
> +
> static struct btd_advertising *
> advertising_manager_create(struct btd_adapter *adapter)
> {
> @@ -657,6 +718,14 @@ advertising_manager_create(struct btd_adapter *adapter)
>
> manager->mgmt_index = btd_adapter_get_index(adapter);
>
> + if (!mgmt_send(manager->mgmt, MGMT_OP_READ_ADV_FEATURES,
> + manager->mgmt_index, 0, NULL,
> + read_adv_features_callback, manager, NULL)) {
> + error("Cannot read advertising features, MGMT version too low");
I'm not sure if you get unsupported failure here since that needs to come from
kernel in response. So this error message doesn't seem right.
> + advertising_manager_destroy(manager);
> + return NULL;
> + }
> +
> if (!g_dbus_register_interface(btd_get_dbus_connection(),
> adapter_get_path(adapter),
> LE_ADVERTISING_MGR_IFACE,
--
BR
Szymon Janc
---
test/example-advertisement | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/test/example-advertisement b/test/example-advertisement
index 6e47391..1198f2e 100755
--- a/test/example-advertisement
+++ b/test/example-advertisement
@@ -51,6 +51,7 @@ class Advertisement(dbus.service.Object):
self.manufacturer_data = None
self.solicit_uuids = None
self.service_data = None
+ self.include_tx_power = None
dbus.service.Object.__init__(self, bus, self.path)
def get_properties(self):
@@ -68,6 +69,8 @@ class Advertisement(dbus.service.Object):
if self.service_data is not None:
properties['ServiceData'] = dbus.Dictionary(self.service_data,
signature='say')
+ if self.include_tx_power is not None:
+ properties['IncludeTXPower'] = dbus.Boolean(self.include_tx_power)
return {LE_ADVERTISEMENT_IFACE: properties}
def get_path(self):
@@ -117,6 +120,7 @@ class TestAdvertisement(Advertisement):
self.add_service_uuid('180F')
self.add_manufacturer_data(0xffff, [0x00, 0x01, 0x02, 0x03, 0x04])
self.add_service_data('9999', [0x00, 0x01, 0x02, 0x03, 0x04])
+ self.include_tx_power = True
def register_ad_cb():
--
2.2.0.rc0.207.ga3a616c
Use the Read Advertising Features response to determine the maximum
number of advertisements, and limit the number of advertisements based
on this.
---
src/advertising.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/advertising.c b/src/advertising.c
index 8acd5b4..c3e9a72 100644
--- a/src/advertising.c
+++ b/src/advertising.c
@@ -47,6 +47,7 @@ struct btd_advertising {
struct mgmt *mgmt;
uint16_t mgmt_index;
uint8_t max_adv_len;
+ uint8_t max_ads;
};
#define AD_TYPE_BROADCAST 0
@@ -610,9 +611,8 @@ static DBusMessage *register_advertisement(DBusConnection *conn,
if (queue_find(manager->ads, match_advertisement_path, path))
return btd_error_already_exists(msg);
- /* TODO: support more than one advertisement */
- if (!queue_isempty(manager->ads))
- return btd_error_failed(msg, "Already advertising");
+ if (queue_length(manager->ads) > manager->max_ads)
+ return btd_error_failed(msg, "Maximum advertisements reached");
dbus_message_iter_next(&args);
@@ -695,6 +695,7 @@ static void read_adv_features_callback(uint8_t status, uint16_t length,
}
manager->max_adv_len = feat->max_adv_data_len;
+ manager->max_ads = feat->max_instances;
}
static struct btd_advertising *
--
2.2.0.rc0.207.ga3a616c
Parse the IncludeTXPower property of the advertisement object, and
pass the appropriate flag to MGMT if it is set.
Uses MGMT Read Advertising Features Command to determine the maximum
length allowed.
---
src/advertising.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 74 insertions(+), 5 deletions(-)
diff --git a/src/advertising.c b/src/advertising.c
index 2f8e539..8acd5b4 100644
--- a/src/advertising.c
+++ b/src/advertising.c
@@ -46,6 +46,7 @@ struct btd_advertising {
struct queue *ads;
struct mgmt *mgmt;
uint16_t mgmt_index;
+ uint8_t max_adv_len;
};
#define AD_TYPE_BROADCAST 0
@@ -59,6 +60,7 @@ struct advertisement {
GDBusProxy *proxy;
DBusMessage *reg;
uint8_t type; /* Advertising type */
+ bool include_tx_power;
struct bt_ad *data;
uint8_t instance;
};
@@ -361,6 +363,22 @@ fail:
return false;
}
+static bool parse_advertising_include_tx_power(GDBusProxy *proxy,
+ bool *included)
+{
+ DBusMessageIter iter;
+
+ if (!g_dbus_proxy_get_property(proxy, "IncludeTXPower", &iter))
+ return true;
+
+ if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_BOOLEAN)
+ return false;
+
+ dbus_message_iter_get_basic(&iter, included);
+
+ return true;
+}
+
static void add_advertising_callback(uint8_t status, uint16_t length,
const void *param, void *user_data)
{
@@ -375,19 +393,44 @@ static void add_advertising_callback(uint8_t status, uint16_t length,
ad->instance = rp->instance;
}
+static size_t calc_max_adv_len(struct advertisement *ad,
+ uint32_t flags)
+{
+ size_t max = ad->manager->max_adv_len;
+
+ if (flags & MGMT_ADV_FLAG_TX_POWER)
+ max -= 3;
+
+ if (flags & (MGMT_ADV_FLAG_DISCOV | MGMT_ADV_FLAG_LIMITED_DISCOV |
+ MGMT_ADV_FLAG_MANAGED_FLAGS))
+ max -= 3;
+
+ if (flags & MGMT_ADV_FLAG_APPEARANCE)
+ max -= 4;
+
+ return max;
+}
+
static DBusMessage *refresh_advertisement(struct advertisement *ad)
{
struct mgmt_cp_add_advertising *cp;
uint8_t param_len;
uint8_t *adv_data;
size_t adv_data_len;
+ uint32_t flags = 0;
DBG("Refreshing advertisement: %s", ad->path);
+ if (ad->type == AD_TYPE_PERIPHERAL)
+ flags = MGMT_ADV_FLAG_CONNECTABLE | MGMT_ADV_FLAG_DISCOV;
+
+ if (ad->include_tx_power)
+ flags |= MGMT_ADV_FLAG_TX_POWER;
+
adv_data = bt_ad_generate(ad->data, &adv_data_len);
- if (!adv_data) {
- error("Advertising data couldn't be generated.");
+ if (!adv_data || (adv_data_len > calc_max_adv_len(ad, flags))) {
+ error("Advertising data too long or couldn't be generated.");
return g_dbus_create_error(ad->reg, ERROR_INTERFACE
".InvalidLength",
@@ -406,9 +449,7 @@ static DBusMessage *refresh_advertisement(struct advertisement *ad)
return btd_error_failed(ad->reg, "Failed");
}
- if (ad->type == AD_TYPE_PERIPHERAL)
- cp->flags = MGMT_ADV_FLAG_CONNECTABLE | MGMT_ADV_FLAG_DISCOV;
-
+ cp->flags = flags;
cp->instance = ad->instance;
cp->adv_data_len = adv_data_len;
memcpy(cp->data, adv_data, adv_data_len);
@@ -457,6 +498,12 @@ static DBusMessage *parse_advertisement(struct advertisement *ad)
goto fail;
}
+ if (!parse_advertising_include_tx_power(ad->proxy,
+ &ad->include_tx_power)) {
+ error("Property \"IncludeTXPower\" failed to parse");
+ goto fail;
+ }
+
return refresh_advertisement(ad);
fail:
@@ -636,6 +683,20 @@ static void advertising_manager_destroy(void *user_data)
free(manager);
}
+static void read_adv_features_callback(uint8_t status, uint16_t length,
+ const void *param, void *user_data)
+{
+ struct btd_advertising *manager = user_data;
+ const struct mgmt_rp_read_adv_features *feat = param;
+
+ if (status || !param) {
+ error("Failed to read advertising features");
+ return;
+ }
+
+ manager->max_adv_len = feat->max_adv_data_len;
+}
+
static struct btd_advertising *
advertising_manager_create(struct btd_adapter *adapter)
{
@@ -657,6 +718,14 @@ advertising_manager_create(struct btd_adapter *adapter)
manager->mgmt_index = btd_adapter_get_index(adapter);
+ if (!mgmt_send(manager->mgmt, MGMT_OP_READ_ADV_FEATURES,
+ manager->mgmt_index, 0, NULL,
+ read_adv_features_callback, manager, NULL)) {
+ error("Cannot read advertising features, MGMT version too low");
+ advertising_manager_destroy(manager);
+ return NULL;
+ }
+
if (!g_dbus_register_interface(btd_get_dbus_connection(),
adapter_get_path(adapter),
LE_ADVERTISING_MGR_IFACE,
--
2.2.0.rc0.207.ga3a616c
---
doc/advertising-api.txt | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/doc/advertising-api.txt b/doc/advertising-api.txt
index 7fb34ee..54ab106 100644
--- a/doc/advertising-api.txt
+++ b/doc/advertising-api.txt
@@ -61,6 +61,11 @@ Properties string Type
Service Data elements to include. The keys are the
UUID to associate with the data.
+ bool IncludeTXPower
+
+ Includes the TX Power in the advertising packet.
+ If missing, the TX Power is not included.
+
LE Advertising Manager hierarchy
================================
--
2.2.0.rc0.207.ga3a616c