Hi Daniel,
On Wed, Sep 16, 2020 at 4:53 PM Daniel Winkler <danielwinkler@xxxxxxxxxx> wrote:
>
> Hello Luiz,
>
> The patch allows the client to set an arbitrary TxPower, different from the include property.
Does the controller honor this setting? Can we really set a TxPower
per instance? If it doesn't then this can possibly break proximity
logic on the scanner side.
> Best,
> Daniel
>
> On Wed, Sep 16, 2020 at 4:52 PM Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx> wrote:
>>
>> Hi Daniel,
>>
>> On Wed, Sep 16, 2020 at 4:25 PM Daniel Winkler <danielwinkler@xxxxxxxxxx> wrote:
>> >
>> > This change adds parsers for the advertising intervals and tx power
>> > properties of the LEAdvertisement1 object. It validates that each field
>> > adheres to the 5.2 spec, and that min and max intervals are compatible
>> > with each other, i.e. that min interval is less than max interval.
>> >
>> > A note here for maintainers: The tx power that is sent in the hci
>> > parameter command is an int8_t, but as far as I can tell, there is no
>> > clean way to use a signed 8-bit integer in dbus. The dbus byte type
>> > seems incompatible with negative values in high-level languages (python)
>> > without awkward usage manipulation on the client side. For this reason,
>> > I chose to use an int16_t type for the tx power dbus field, which is
>> > then downcasted to the int8_t in bluetoothd, which at least makes the
>> > signed-ness of the type crystal clear to the dbus client that uses it.
>> >
>> > This change is manually verified by ensuring the intervals and tx power
>> > parameters are correctly parsed from the LEAdvertisement1 object, and
>> > that the parse fails if the parameters are incorrect or not compatible
>> > with each other.
>> >
>> > Reviewed-by: Sonny Sasaka <sonnysasaka@xxxxxxxxxxxx>
>> > ---
>> >
>> > src/advertising.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++
>> > 1 file changed, 89 insertions(+)
>> >
>> > diff --git a/src/advertising.c b/src/advertising.c
>> > index 172a83907..82ee87313 100644
>> > --- a/src/advertising.c
>> > +++ b/src/advertising.c
>> > @@ -63,6 +63,11 @@ struct btd_adv_manager {
>> > #define AD_TYPE_BROADCAST 0
>> > #define AD_TYPE_PERIPHERAL 1
>> >
>> > +/* BLUETOOTH SPECIFICATION Version 5.2 | Vol 4, Part E, page 2585
>> > + * defines tx power value indicating no preference
>> > + */
>> > +#define ADV_TX_POWER_NO_PREFERENCE 0x7F
>> > +
>> > struct btd_adv_client {
>> > struct btd_adv_manager *manager;
>> > char *owner;
>> > @@ -83,6 +88,9 @@ struct btd_adv_client {
>> > struct bt_ad *data;
>> > struct bt_ad *scan;
>> > uint8_t instance;
>> > + uint32_t min_interval;
>> > + uint32_t max_interval;
>> > + int8_t tx_power;
>> > };
>> >
>> > struct dbus_obj_match {
>> > @@ -946,6 +954,74 @@ static bool parse_secondary(DBusMessageIter *iter,
>> > return false;
>> > }
>> >
>> > +static bool parse_min_interval(DBusMessageIter *iter,
>> > + struct btd_adv_client *client)
>> > +{
>> > + if (!iter)
>> > + return false;
>> > +
>> > + if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_UINT32)
>> > + return false;
>> > +
>> > + dbus_message_iter_get_basic(iter, &client->min_interval);
>> > +
>> > + /* BLUETOOTH SPECIFICATION Version 5.2 | Vol 4, Part E, page 2584
>> > + * defines acceptable interval range
>> > + */
>> > + if (client->min_interval < 0x20 || client->min_interval > 0xFFFFFF) {
>> > + client->min_interval = 0;
>> > + return false;
>> > + }
>> > +
>> > + return true;
>> > +}
>> > +
>> > +static bool parse_max_interval(DBusMessageIter *iter,
>> > + struct btd_adv_client *client)
>> > +{
>> > + if (!iter)
>> > + return false;
>> > +
>> > + if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_UINT32)
>> > + return false;
>> > +
>> > + dbus_message_iter_get_basic(iter, &client->max_interval);
>> > +
>> > + /* BLUETOOTH SPECIFICATION Version 5.2 | Vol 4, Part E, page 2584
>> > + * defines acceptable interval range
>> > + */
>> > + if (client->max_interval < 0x20 || client->max_interval > 0xFFFFFF) {
>> > + client->max_interval = 0;
>> > + return false;
>> > + }
>> > +
>> > + return true;
>> > +}
>> > +
>> > +static bool parse_tx_power(DBusMessageIter *iter,
>> > + struct btd_adv_client *client)
>> > +{
>> > + int16_t val;
>> > +
>> > + if (!iter)
>> > + return false;
>> > +
>> > + if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_INT16)
>> > + return false;
>> > +
>> > + dbus_message_iter_get_basic(iter, &val);
>> > +
>> > + /* BLUETOOTH SPECIFICATION Version 5.2 | Vol 4, Part E, page 2585
>> > + * defines acceptable tx power range
>> > + */
>> > + if (val < -127 || val > 20)
>> > + return false;
>> > +
>> > + client->tx_power = val;
>> > +
>> > + return true;
>> > +}
>> > +
>> > static struct adv_parser {
>> > const char *name;
>> > bool (*func)(DBusMessageIter *iter, struct btd_adv_client *client);
>> > @@ -964,6 +1040,9 @@ static struct adv_parser {
>> > { "Discoverable", parse_discoverable },
>> > { "DiscoverableTimeout", parse_discoverable_timeout },
>> > { "SecondaryChannel", parse_secondary },
>> > + { "MinInterval", parse_min_interval },
>> > + { "MaxInterval", parse_max_interval },
>>
>> We will need to these to the D-Bus documentation if you want to extend it.
>>
>> > + { "TxPower", parse_tx_power },
>>
>> TxPower is already part of the include, or you want the ability to set
>> an arbitrary TxPower?
>>
>> > { },
>> > };
>> >
>> > @@ -1092,6 +1171,13 @@ static DBusMessage *parse_advertisement(struct btd_adv_client *client)
>> > goto fail;
>> > }
>> >
>> > + if (client->min_interval > client->max_interval) {
>> > + /* Min interval must not be bigger than max interval */
>> > + error("MinInterval must be less than MaxInterval (%lu > %lu)",
>> > + client->min_interval, client->max_interval);
>> > + goto fail;
>> > + }
>> > +
>> > err = refresh_adv(client, add_adv_callback, &client->add_adv_id);
>> > if (!err)
>> > return NULL;
>> > @@ -1167,6 +1253,9 @@ static struct btd_adv_client *client_create(struct btd_adv_manager *manager,
>> >
>> > client->manager = manager;
>> > client->appearance = UINT16_MAX;
>> > + client->tx_power = ADV_TX_POWER_NO_PREFERENCE;
>> > + client->min_interval = 0;
>> > + client->max_interval = 0;
>> >
>> > return client;
>> >
>> > --
>> > 2.28.0.618.gf4bc123cb7-goog
>> >
>>
>>
>> --
>> Luiz Augusto von Dentz
--
Luiz Augusto von Dentz
