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: <20200406180331.891822-1-marcel@holtmann.org> <CANFp7mXe3QtkKL8+TgTp=jwvpydABhDkiXks=3h4+op20rWhGQ@mail.gmail.com>
 <AC887BAA-95ED-456B-84D2-2D1EF6A8339F@holtmann.org>
In-Reply-To: <AC887BAA-95ED-456B-84D2-2D1EF6A8339F@holtmann.org>
From:   Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Date:   Tue, 9 Jun 2020 10:43:12 -0700
Message-ID: <CANFp7mU2+Z3f1ZqR8RCsOUu6Crc_xng12gcgn0YCwjzfH9Y-nQ@mail.gmail.com>
Subject: Re: [PATCH] doc: Add commands and event for handling device flags
To:     Marcel Holtmann <marcel@holtmann.org>
Cc:     Bluez mailing list <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

Hey Marcel,

Coming back to this device flags idea, I think I would prefer the
original design over adding new management commands for each flag.
Bluez will just have to maintain the current flags and pending flags
(i.e. mgmt call) to decide when to emit property changed events for
the device WakeAllowed property.

0x0049 and 0x004A are now taken for experimental features but you have
my reviewed-by for the next available values for the original patch.

Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

Thanks
Abhishek

On Mon, Apr 6, 2020 at 11:36 AM Marcel Holtmann <marcel@holtmann.org> wrote=
:
>
> Hi Abhishek,
>
> >> ---
> >> doc/mgmt-api.txt | 96 ++++++++++++++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 96 insertions(+)
> >>
> >> diff --git a/doc/mgmt-api.txt b/doc/mgmt-api.txt
> >> index 39f23c456080..ac5b6c97d64a 100644
> >> --- a/doc/mgmt-api.txt
> >> +++ b/doc/mgmt-api.txt
> >> @@ -3138,6 +3138,74 @@ Read Security Information Command
> >>                                Invalid Index
> >>
> >>
> >> +Get Device Flags 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
> >> +
> >> +       Command Code:           0x0049
> >> +       Controller Index:       <controller id>
> >> +       Command Parameters:     Address (6 Octets)
> >> +                               Address_Type (1 Octet)
> >> +       Return Parameters:      Address (6 Octets)
> >> +                               Address_Type (1 Octet)
> >> +                               Supported_Flags (4 Octets)
> >> +                               Current_Flags (4 Octets)
> >> +
> >> +       This command is used to retrieve additional flags and settings
> >> +       for devices that are added via Add Device command.
> >> +
> >> +       Possible values for the Address_Type parameter:
> >> +               0       BR/EDR
> >> +               1       LE Public
> >> +               2       LE Random
> >> +
> >> +       The Flags parameters are a bitmask with currently the followin=
g
> >> +       available bits:
> >> +
> >> +               0       Remote Wakeup enabled
> >> +
> >> +       This command generates a Command Complete event on success
> >> +       or a Command Status event on failure.
> >> +
> >> +        Possible errors:       Invalid Parameters
> >> +                               Invalid Index
> >> +
> >> +
> >
> > Get device flags looks good to me.
> >
> >> +Set Device Flags 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
> >> +
> >> +       Command Code:           0x004a
> >> +       Controller Index:       <controller id>
> >> +       Command Parameters:     Address (6 Octets)
> >> +                               Address_Type (1 Octet)
> >> +                               Current_Flags (4 Octets)
> >
> > I would prefer to use a mask and value rather than current_flags here.
> >
> >> +       Return Parameters:      Address (6 Octets)
> >> +                               Address_Type (1 Octet)
> >
> > Prefer to also return an updated_mask and current_flags. This
> > simplifies completion for userspace. Otherwise, we would need to keep
> > a "pending flags" value on the device structure.
>
> I saw your =E2=80=9Cmask=E2=80=9D proposal and I am not a fan of that. I =
want to keep the design of the API somewhat consistent. Hence the Device Fl=
ags Changed event should be send after Add Device completed and also after =
Device Added has been sent out.
>
> One other option is to keep Get Device Flags as is and then instead of ad=
ding Set Device Flags, add one command per flag and rename Device Flags Cha=
nged to New Device Flags.
>
> Set Device Wakeable 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
>
>         Command Code:           0x004a
>         Controller Index:       <controller id>
>         Command Parameters:     Address (6 Octets)
>                                 Address_Type (1 Octet)
>                                 Wakeable (1 Octet)
>         Return Parameters:      Address (6 Octets)
>                                 Address_Type (1 Octet)
>                                 Current_Flags (4 Octets)
>
> Maybe this is not a bad idea either and is similar on how we handle setti=
ngs. I need to sleep about this.
>
> Regards
>
> Marcel
>