Hi Arkadiusz,
On Sat, Oct 14, 2023 at 11:58 AM Arkadiusz Bokowy
<arkadiusz.bokowy@xxxxxxxxx> wrote:
>
> 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.
I'd keep the initial logic of sending the advertisement immediately
since the likes of mgmt-tester, and other testers, do have a number of
tests that connect so adding a delay can probably cause us to spend
more time waiting.
> > > -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.
Btw, make sure you run the testers to confirm you are not breaking any
kernel testers.
--
Luiz Augusto von Dentz
