Re: [Bluez PATCH v2 1/3] advertising: Generate advertising data earlier in pipeline

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

 



Hi Daniel,

On Thu, Mar 4, 2021 at 3:27 PM Daniel Winkler <danielwinkler@xxxxxxxxxx> wrote:
>
> Hi Luiz,
>
> Can you please clarify your suggestion? The issue here is that some
> properties (local name, for instance) aren't incorporated into the
> bt_ad object until generate_scan_rsp is called. I decided to move the
> generation of data/scan response to be earlier because otherwise the
> logic to determine if the scan response was empty would require some
> repetitive logic that already exists in generate_scan_rsp.
>
> Or are you suggesting that we not store adv_data_len and scan_rsp_len
> in the btd_adv_client, but instead store it in the bt_ad object? This
> seems possible to me, but it might require a bit more effort to keep
> the length property in sync. I'll wait for your clarification.

Yep, the included flags may have to be calculated separately, but I
thought the whole point here is to be able to tell if there is any
scan_rsp to be added so perhaps just return a bool would be enough to
then set the flag so we can continue to generate the data itself at
later stage.

> Thanks!
> Daniel
>
> On Thu, Mar 4, 2021 at 2:49 PM Luiz Augusto von Dentz
> <luiz.dentz@xxxxxxxxx> wrote:
> >
> > Hi Daniel,
> >
> > On Thu, Mar 4, 2021 at 12:25 PM Daniel Winkler <danielwinkler@xxxxxxxxxx> wrote:
> > >
> > > This change moves the advertising data generation to the beginning of
> > > the registration pipeline. This is necessary for the following patch,
> > > which will need to know whether the scan response data is existent so
> > > that the parameter request can be populated correctly.
> > >
> > > Reviewed-by: Alain Michaud <alainm@xxxxxxxxxxxx>
> > > Reviewed-by: Sonny Sasaka <sonnysasaka@xxxxxxxxxxxx>
> > >
> > > ---
> > >
> > > Changes in v2: None
> > >
> > >  src/advertising.c | 79 +++++++++++++++++++++++++----------------------
> > >  1 file changed, 42 insertions(+), 37 deletions(-)
> > >
> > > diff --git a/src/advertising.c b/src/advertising.c
> > > index 15a343e52..f3dc357a1 100644
> > > --- a/src/advertising.c
> > > +++ b/src/advertising.c
> > > @@ -80,6 +80,10 @@ struct btd_adv_client {
> > >         uint32_t flags;
> > >         struct bt_ad *data;
> > >         struct bt_ad *scan;
> > > +       uint8_t *adv_data;
> > > +       uint8_t *scan_rsp;
> > > +       size_t adv_data_len;
> > > +       size_t scan_rsp_len;
> >
> > I'm debating if we should really just encode it early or we could just
> > introduce something like bt_ad_length so we don't have to have copies
> > of the same data in 2 different formats since bt_ad already represents
> > that.
> >
> > >         uint8_t instance;
> > >         uint32_t min_interval;
> > >         uint32_t max_interval;
> > > @@ -141,6 +145,16 @@ static void client_free(void *data)
> > >         bt_ad_unref(client->data);
> > >         bt_ad_unref(client->scan);
> > >
> > > +       if (client->adv_data) {
> > > +               free(client->adv_data);
> > > +               client->adv_data = NULL;
> > > +       }
> > > +
> > > +       if (client->scan_rsp) {
> > > +               free(client->scan_rsp);
> > > +               client->scan_rsp = NULL;
> > > +       }
> > > +
> > >         g_dbus_proxy_unref(client->proxy);
> > >
> > >         if (client->owner)
> > > @@ -915,6 +929,22 @@ static int refresh_extended_adv(struct btd_adv_client *client,
> > >                 flags |= MGMT_ADV_PARAM_TX_POWER;
> > >         }
> > >
> > > +       client->adv_data = generate_adv_data(client, &flags,
> > > +                                               &client->adv_data_len);
> > > +       if (!client->adv_data ||
> > > +               (client->adv_data_len > calc_max_adv_len(client, flags))) {
> > > +               error("Advertising data too long or couldn't be generated.");
> > > +               return -EINVAL;
> > > +       }
> > > +
> > > +       client->scan_rsp = generate_scan_rsp(client, &flags,
> > > +                                               &client->scan_rsp_len);
> > > +       if (!client->scan_rsp && client->scan_rsp_len) {
> > > +               error("Scan data couldn't be generated.");
> > > +               free(client->adv_data);
> > > +               return -EINVAL;
> > > +       }
> > > +
> > >         cp.flags = htobl(flags);
> > >
> > >         mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_PARAMS,
> > > @@ -1222,11 +1252,6 @@ static void add_adv_params_callback(uint8_t status, uint16_t length,
> > >         const struct mgmt_rp_add_ext_adv_params *rp = param;
> > >         struct mgmt_cp_add_ext_adv_data *cp = NULL;
> > >         uint8_t param_len;
> > > -       uint8_t *adv_data = NULL;
> > > -       size_t adv_data_len;
> > > -       uint8_t *scan_rsp = NULL;
> > > -       size_t scan_rsp_len = -1;
> > > -       uint32_t flags = 0;
> > >         unsigned int mgmt_ret;
> > >         dbus_int16_t tx_power;
> > >
> > > @@ -1248,23 +1273,8 @@ static void add_adv_params_callback(uint8_t status, uint16_t length,
> > >
> > >         client->instance = rp->instance;
> > >
> > > -       flags = get_adv_flags(client);
> > > -
> > > -       adv_data = generate_adv_data(client, &flags, &adv_data_len);
> > > -       if (!adv_data || (adv_data_len > rp->max_adv_data_len)) {
> > > -               error("Advertising data too long or couldn't be generated.");
> > > -               goto fail;
> > > -       }
> > > -
> > > -       scan_rsp = generate_scan_rsp(client, &flags, &scan_rsp_len);
> > > -       if ((!scan_rsp && scan_rsp_len) ||
> > > -                       scan_rsp_len > rp->max_scan_rsp_len) {
> > > -               error("Scan data couldn't be generated.");
> > > -               goto fail;
> > > -       }
> > > -
> > > -       param_len = sizeof(struct mgmt_cp_add_advertising) + adv_data_len +
> > > -                                                       scan_rsp_len;
> > > +       param_len = sizeof(struct mgmt_cp_add_advertising) +
> > > +                       client->adv_data_len + client->scan_rsp_len;
> > >
> > >         cp = malloc0(param_len);
> > >         if (!cp) {
> > > @@ -1273,15 +1283,11 @@ static void add_adv_params_callback(uint8_t status, uint16_t length,
> > >         }
> > >
> > >         cp->instance = client->instance;
> > > -       cp->adv_data_len = adv_data_len;
> > > -       cp->scan_rsp_len = scan_rsp_len;
> > > -       memcpy(cp->data, adv_data, adv_data_len);
> > > -       memcpy(cp->data + adv_data_len, scan_rsp, scan_rsp_len);
> > > -
> > > -       free(adv_data);
> > > -       free(scan_rsp);
> > > -       adv_data = NULL;
> > > -       scan_rsp = NULL;
> > > +       cp->adv_data_len = client->adv_data_len;
> > > +       cp->scan_rsp_len = client->scan_rsp_len;
> > > +       memcpy(cp->data, client->adv_data, client->adv_data_len);
> > > +       memcpy(cp->data + client->adv_data_len, client->scan_rsp,
> > > +                       client->scan_rsp_len);
> > >
> > >         /* Submit request to update instance data */
> > >         mgmt_ret = mgmt_send(client->manager->mgmt, MGMT_OP_ADD_EXT_ADV_DATA,
> > > @@ -1305,12 +1311,6 @@ static void add_adv_params_callback(uint8_t status, uint16_t length,
> > >         return;
> > >
> > >  fail:
> > > -       if (adv_data)
> > > -               free(adv_data);
> > > -
> > > -       if (scan_rsp)
> > > -               free(scan_rsp);
> > > -
> > >         if (cp)
> > >                 free(cp);
> > >
> > > @@ -1454,6 +1454,11 @@ static struct btd_adv_client *client_create(struct btd_adv_manager *manager,
> > >         if (!client->scan)
> > >                 goto fail;
> > >
> > > +       client->adv_data = NULL;
> > > +       client->scan_rsp = NULL;
> > > +       client->adv_data_len = 0;
> > > +       client->scan_rsp_len = 0;
> > > +
> > >         client->manager = manager;
> > >         client->appearance = UINT16_MAX;
> > >         client->tx_power = ADV_TX_POWER_NO_PREFERENCE;
> > > --
> > > 2.30.1.766.gb4fecdf3b7-goog
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz



-- 
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