Re: [Bluez PATCH 02/10] advertising: Parse intervals and tx power from adv

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux