Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200521145757.245774-1-alainm@chromium.org> <CABBYNZKU4JVHxL5NEaMFgf6-aAOrG4re=T0Yat9b=viz57PcsQ@mail.gmail.com>
 <CALWDO_UQhU5QxZ1eRNWZtAtkcghWEBM5eVeujxhROSr7PG_P=w@mail.gmail.com>
 <CABBYNZ+8ed4qXarr647Q2SKkvqdT3N5AccP4L5a35e--9ybzgg@mail.gmail.com>
 <CALWDO_WT1DmS02jSJSa8GyiPwFz7cEnOjqhXMJsDO=6=MjyGyw@mail.gmail.com>
 <CABBYNZJzSaEvFTt6b6vuEoM3Eof225=bszVvM0ZtyJ0NsqVdGg@mail.gmail.com> <CALWDO_XX-xKJ4Se6HwfyroD9uQHj32xf5ZAFkEEA_QjV5MsVNg@mail.gmail.com>
In-Reply-To: <CALWDO_XX-xKJ4Se6HwfyroD9uQHj32xf5ZAFkEEA_QjV5MsVNg@mail.gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Thu, 21 May 2020 17:14:11 -0700
Message-ID: <CABBYNZ+-29UPnXy8Z_w4VyripQAKzXmZ_Vy-8LZB91ZSmQ7WEQ@mail.gmail.com>
Subject: Re: [BlueZ PATCH v3] doc:adding definitions for load default params
 mgmt op
To:     Alain Michaud <alainmichaud@google.com>
Cc:     Alain Michaud <alainm@chromium.org>,
        BlueZ <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Alain,

On Thu, May 21, 2020 at 4:44 PM Alain Michaud <alainmichaud@google.com> wro=
te:
>
> Hi Luiz
>
> On Thu., May 21, 2020, 7:19 p.m. Luiz Augusto von Dentz, <luiz.dentz@gmai=
l.com> wrote:
>>
>> Hi Alain,
>>
>> On Thu, May 21, 2020 at 3:28 PM Alain Michaud <alainmichaud@google.com> =
wrote:
>> >
>> > Hi Luiz
>> >
>> >
>> > On Thu, May 21, 2020 at 6:05 PM Luiz Augusto von Dentz
>> > <luiz.dentz@gmail.com> wrote:
>> > >
>> > > Hi Alain,
>> > >
>> > > On Thu, May 21, 2020 at 1:12 PM Alain Michaud <alainmichaud@google.c=
om> wrote:
>> > > >
>> > > > Hi Luiz,
>> > > >
>> > > >
>> > > > On Thu, May 21, 2020 at 12:26 PM Luiz Augusto von Dentz <luiz.dent=
z@gmail.com> wrote:
>> > > >>
>> > > >> Hi Alain,
>> > > >>
>> > > >> On Thu, May 21, 2020 at 8:05 AM Alain Michaud <alainm@chromium.or=
g> wrote:
>> > > >> >
>> > > >> > This change adds the definition for the load default parameter =
command.
>> > > >> > In particular, this command is used to load default parameters =
for
>> > > >> > various operations in the kernel. This mechanism is also expand=
able to
>> > > >> > future values that may be necessary.
>> > > >> >
>> > > >> > This will allow bluetoothd to load parameters from a conf file =
that may
>> > > >> > be customized for the specific requirements of each platforms.
>> > > >> >
>> > > >> > ---
>> > > >> > rebase against current master
>> > > >> >
>> > > >> >  doc/mgmt-api.txt | 62 ++++++++++++++++++++++++++++++++++++++++=
++++++++
>> > > >> >  1 file changed, 62 insertions(+)
>> > > >> >
>> > > >> > diff --git a/doc/mgmt-api.txt b/doc/mgmt-api.txt
>> > > >> > index 6ee01fed8..c6575e654 100644
>> > > >> > --- a/doc/mgmt-api.txt
>> > > >> > +++ b/doc/mgmt-api.txt
>> > > >> > @@ -3223,6 +3223,68 @@ Set Experimental Feature Command
>> > > >> >                                 Invalid Index
>> > > >> >
>> > > >> >
>> > > >> > +Load Default Controller Parameter Command
>> > > >> > +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D
>> > > >> > +
>> > > >> > +       Command Code:           0x004b
>> > > >> > +       Controller Index:       <controller id>
>> > > >> > +       Command Parameters:     Parameter_Count (2 Octets)
>> > > >> > +                               Parameter1 {
>> > > >> > +                                       Parameter_Type (2 Octet=
)
>> > > >> > +                                       Value_Length (1 Octet)
>> > > >> > +                                       Value (0-255 Octets)
>> > > >> > +                               }
>> > > >> > +                               Parameter2 { }
>> > > >> > +                               ...
>> > > >> > +       Return Parameters:
>> > > >> > +
>> > > >> > +       This command is used to feed the kernel a list of defau=
lt controller
>> > > >> > +       parameters.
>> > > >> > +
>> > > >> > +       Currently defined Parameter_Type values are:
>> > > >> > +
>> > > >> > +               0x0000  BR/EDR Page Scan Type
>> > > >> > +               0x0001  BR/EDR Page Scan Interval
>> > > >> > +               0x0002  BR/EDR Page Scan Window
>> > > >> > +               0x0003  BR/EDR Inquiry Scan Type
>> > > >> > +               0x0004  BR/EDR Inquiry Scan Interval
>> > > >> > +               0x0005  BR/EDR Inquiry Scan Window
>> > > >> > +               0x0006  BR/EDR Link Supervision Timeout
>> > > >> > +               0x0007  BR/EDR Page Timeout
>> > > >> > +               0x0008  BR/EDR Min Sniff Interval
>> > > >> > +               0x0009  BR/EDR Max Sniff Interval
>> > > >> > +               0x000a  LE Advertisement Min Interval
>> > > >> > +               0x000b  LE Advertisement Max Interval
>> > > >> > +               0x000c  LE Multi Advertisement Rotation Interva=
l
>> > > >> > +               0x000d  LE Scanning Interval for auto connect
>> > > >> > +               0x000e  LE Scanning Window for auto connect
>> > > >> > +               0x000f  LE Scanning Interval for HID only
>> > > >> > +               0x0010  LE Scanning Window for HID only
>> > > >>
>> > > >> I remember commenting that we don't have profile information on t=
he
>> > > >> kernel so Im not sure how you are planning to detect the when the
>> > > >> device is HID or not?
>> > > >
>> > > > I expect we'll need to add an Add Device flag to carry that inform=
ation down into the kernel.  The hid profile already sends an op, so it's r=
elatively trivial to carry that information in so we can apply specific sca=
nning requirements for that case (which are less aggressive and therefore h=
ave less of an impact to Wifi and BT performance).
>> > >
>> > > Well if we will need to feed that information of the profile type we
>> > > might as well just feed the intervals directly, that way any profile
>> > > that needs some different parameters than the default can all use th=
e
>> > > same command and we don't have to grow this list when a new profile
>> > > want to use different parameters.
>> > >
>> > That might be another option indeed.  I wonder how far you'd want to
>> > take this to have the profile provide all the values.  At some point,
>> > you'd need some logic that would pick the value that meets all the
>> > profile requirements so if one wants w:100, i:200 and another wants
>> > w:200: i400, you may need to scan at 100% to meet the requirements of
>> > all profiles...  I think that with the current proposal, while less
>> > flexible, may avoid this complexity/condition..
>>
>> Not sure I follow you here, what I had in mind was that the profile
>> provides its own settings for scanning but perhaps you are saying that
>> in case there are multiple profiles involved we would have to
>> aggregate the parameters, well that has to be done either way even if
>> we use simple types, in which case there can be multiple defaults (one
>> for each profile) that the kernel must aggregate or we do aggregate
>> them at userspace and just provide the end result. In the end I guess
>> this means that the connect scanning parameters should probably be per
>> device rather than a using a globals with per profile exceptions, much
>> like connection parameters are per device as well.
>
>
> You got the idea. Aggregation would be needed, but with simple types, we =
can have simple logic such as if only hid is added (via the flag, we use th=
e smaller duty cycle provided by the hid parameters defined in this patch),=
 else, we use the passive scanning parameters as we do today. In essence, w=
ith this logic, no aggregation of settings is necessary since it is up to t=
he platform who set the various values via their .conf file.

Well you will need to know when to apply the hid params, so to me it
still seems a per device policy, what Im missing?

> Scan parameters are global, they can't be per connection like the connect=
ion parameters.

Not when you are scanning to connect to specifica device, perhaps you
want to expand on where each scanning parameter set takes place then.

>>
>>
>> > > >>
>> > > >>
>> > > >> > +               0x0012  LE Scanning Interval for wake scenarios
>> > > >> > +               0x0013  LE Scanning Window for wake scenarios
>> > > >> > +               0x0014  LE Scanning Interval for discovery
>> > > >> > +               0x0015  LE Scanning Window for discovery
>> > > >> > +               0x0016  LE Scanning Interval for adv monitoring
>> > > >> > +               0x0017  LE Scanning Window for adv monitoring
>> > > >> > +               0x0018  LE Scanning Interval for connect
>> > > >> > +               0x0019  LE Scanning Window for connect
>> > > >> > +               0x001a  LE Min Connection Interval
>> > > >> > +               0x001b  LE Max Connection Interval
>> > > >> > +               0x001c  LE Connection Connection Latency
>> > > >> > +               0x001d  LE Connection Supervision Timeout
>> > > >> > +
>> > > >> > +       This command can be used when the controller is not pow=
ered and
>> > > >> > +       all settings will be programmed once powered.  Note tha=
t these only
>> > > >> > +       control the default parameters.  Higher level Apis may =
influence the
>> > > >> > +       effective value used.
>> > > >> > +
>> > > >> > +       This command generates a Command Complete event on succ=
ess or
>> > > >> > +       a Command Status event on failure.
>> > > >> > +
>> > > >> > +       Possible errors:        Invalid Parameters
>> > > >> > +                               Invalid Index
>> > > >> > +
>> > > >> > +
>> > > >> >  Command Complete Event
>> > > >> >  =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D
>> > > >> >
>> > > >> > --
>> > > >> > 2.26.2.761.g0e0b3e54be-goog
>> > > >> >
>> > > >>
>> > > >>
>> > > >> --
>> > > >> Luiz Augusto von Dentz
>> > >
>> > >
>> > >
>> > > --
>> > > Luiz Augusto von Dentz
>>
>>
>>
>> --
>> Luiz Augusto von Dentz


--=20
Luiz Augusto von Dentz