Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
MIME-Version: 1.0
References: <20231014074716.1866266-1-arkadiusz.bokowy@gmail.com>
 <e706186e-e64f-490f-9162-11bfdc589619@molgen.mpg.de> <CAGFh026-11qh0jRQ5=NS4SmH7JQy7BfNwJh9N=bs-K-aVNba-g@mail.gmail.com>
In-Reply-To: <CAGFh026-11qh0jRQ5=NS4SmH7JQy7BfNwJh9N=bs-K-aVNba-g@mail.gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 16 Oct 2023 10:34:08 -0700
Message-ID: <CABBYNZ+SYtZ48Rc5mife8bLV12ri4EvqL7QDJLnxuSNBmV46rg@mail.gmail.com>
Subject: Re: [PATCH BlueZ] btdev: Broadcast EXT_ADV packets every 200 ms
To:     Arkadiusz Bokowy <arkadiusz.bokowy@gmail.com>
Cc:     Paul Menzel <pmenzel@molgen.mpg.de>,
        linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Arkadiusz,

On Sat, Oct 14, 2023 at 11:58=E2=80=AFAM Arkadiusz Bokowy
<arkadiusz.bokowy@gmail.com> 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 curren=
t
> > > 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 wil=
l
> > > 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 20=
0
> > 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 =3D user_data;
> > > +     struct btdev *btdev =3D 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 !=3D INV_HANDLE || !remote->le_pa_en=
able)
> > > @@ -5440,7 +5411,6 @@ static int cmd_set_ext_scan_enable_complete(str=
uct btdev *dev, const void *data,
> > >               if (!btdev_list[i] || btdev_list[i] =3D=3D 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 anymor=
e?
>
> 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.

--=20
Luiz Augusto von Dentz