Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
MIME-Version: 1.0
References: <20231014074716.1866266-1-arkadiusz.bokowy@gmail.com> <e706186e-e64f-490f-9162-11bfdc589619@molgen.mpg.de>
In-Reply-To: <e706186e-e64f-490f-9162-11bfdc589619@molgen.mpg.de>
From:   Arkadiusz Bokowy <arkadiusz.bokowy@gmail.com>
Date:   Sat, 14 Oct 2023 20:52:11 +0200
Message-ID: <CAGFh026-11qh0jRQ5=NS4SmH7JQy7BfNwJh9N=bs-K-aVNba-g@mail.gmail.com>
Subject: Re: [PATCH BlueZ] btdev: Broadcast EXT_ADV packets every 200 ms
To:     Paul Menzel <pmenzel@molgen.mpg.de>
Cc:     linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Paul,

> > Real BLE devices transmit LE advertisement report packages in given
> > intervals (typically in range between 20 ms and 10.24 s). With current
> > kernel module Bluetooth stack implementation it is possible that the
> > first LE meta packet just after enabling scanning will be lost. It is
> > not an issue for real devices, because more advertisement reports will
> > be delivered later (in given interval time).
> >
> > This patch changes optimistic implementation of sending only one LE
> > meta packets just after enabling scanning to sending LE meta packets
> > in 200 ms intervals. Such behavior will better emulate real HCI and
> > will workaround the issue of dropping the very first LE meta packet
> > by the kernel.
>
> Could you please describe your test setup? I guess you optimized the 200
> ms for your setup, and that is the reason you did not choose an even
> lower value like 100 ms?

No, it's not an optimization for my particular setup, but more
generally for CPU load. I thought that it might be better not to run
advertisement code too frequently. But I guess that lower values
should also be OK, e.g. 100 ms or 50 ms. There is one "issue" with
that, though.... Now, the advertisement packet will be sent one
interval after the advertisement was enabled. If that's indeed an
issue, it can be fixed by calling the callback function in the moment
when the timer is armed.

> > -static void le_set_ext_adv_enable_complete(struct btdev *btdev,
> > -                                             struct le_ext_adv *ext_adv)
> > +static bool ext_adv_broadcast(void *user_data)
> >   {
> > +     struct le_ext_adv *ext_adv = user_data;
> > +     struct btdev *btdev = ext_adv->dev;
>
> Are these used?
>
> Why rename the function?

In the previous implementation, the le_set_ext_adv_enable_complete()
function was called after advertisement was enabled in order to send
LE meta packages to devices which have enabled scanning. But this code
was run only once. I thought that we can reuse this logic to send LE
meta packages every interval time. That's why I've renamed this
function, so the name will be more descriptive (it's not enable
complete callback anymore). And in order to provide compatibility with
new sygnature, I've added ext_adv and btdev variables.

> >   static void scan_pa(struct btdev *dev, struct btdev *remote)
> >   {
> >       if (dev->le_pa_sync_handle != INV_HANDLE || !remote->le_pa_enable)
> > @@ -5440,7 +5411,6 @@ static int cmd_set_ext_scan_enable_complete(struct btdev *dev, const void *data,
> >               if (!btdev_list[i] || btdev_list[i] == dev)
> >                       continue;
> >
> > -             scan_ext_adv(dev, btdev_list[i]);
> >               scan_pa(dev, btdev_list[i]);
> >       }
>
> Excuse my ignorance, but the remove code is really not necessary anymore?

Maybe I will briefly describe the previous implementation. There are
two possible cases for sending and receiving advertisements: a) device
A (B, C, ...) enables advertisement and then device Z enables
scanning; b) device Z enables scanning and then other devices enable
advertisement. So, in previous implementation, the LE meta packets
have been sent to devices with enabled scanning from devices which
have enabled advertisement either when the scan was enabled (in device
Z) or when the advertisement was enabled (in device A, B, C ...). The
removed code corresponds to the case of enabling scanning. It's not
required any more, because now advertising is governed by the devices
A, B, C by running advertisement logic every interval time (in the
original patch, every 200 ms, but that can be changed to lover
values).

To be honest I'm not sure what about scan_pa(). Maybe that logic
should also be moved to some timer callback and should be governed by
the A, C, B, ... devices? I would have to read more about sync in the
BLE spec.

Regards,
Arek