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: <20201111011745.2016-1-sonnysasaka@chromium.org>
 <20201111011745.2016-4-sonnysasaka@chromium.org> <CABBYNZJkyOBT63aAYEc4CtCJtqaQEW3bLyyrJ=XJQ_EpeDDmuQ@mail.gmail.com>
 <CAO271mkHCaQ0f+3PKjmnoAqszk1XuUFi_gF5rO9GvsgmqOOiVQ@mail.gmail.com> <CABBYNZKebbsGiQueqcE5svRbFON8hgL9wqTqtMFa9m=u+TsO4w@mail.gmail.com>
In-Reply-To: <CABBYNZKebbsGiQueqcE5svRbFON8hgL9wqTqtMFa9m=u+TsO4w@mail.gmail.com>
From:   Sonny Sasaka <sonnysasaka@chromium.org>
Date:   Tue, 17 Nov 2020 15:16:50 -0800
Message-ID: <CAO271m=CrnNwfWRg5NKUjTMyw-tHiw1xjHZDgzX9cUfJ9zWyHA@mail.gmail.com>
Subject: Re: [PATCH BlueZ v2 4/7] doc: Add Battery Provider API doc
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        Miao-chen Chou <mcchou@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Luiz,

On Tue, Nov 17, 2020 at 3:01 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Sonny,
>
> On Tue, Nov 17, 2020 at 2:37 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> >
> > Hi Luiz,
> >
> > Thanks for the feedback. Please find my responses below.
> >
> > On Mon, Nov 16, 2020 at 4:17 PM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi Sonny,
> > >
> > > On Tue, Nov 10, 2020 at 5:22 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> > > >
> > > > This patch add the documentation of the Battery Provider which lets
> > > > external clients feed battery information to BlueZ if they are able to
> > > > decode battery reporting via any profile. BlueZ UI clients can then use
> > > > the org.bluez.Battery1 API as a single source of battery information
> > > > coming from many different profiles.
> > > >
> > > > Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> > > >
> > > > ---
> > > >  doc/battery-api.txt | 55 +++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 55 insertions(+)
> > > >
> > > > diff --git a/doc/battery-api.txt b/doc/battery-api.txt
> > > > index dc7dbeda2..058bf0c6e 100644
> > > > --- a/doc/battery-api.txt
> > > > +++ b/doc/battery-api.txt
> > > > @@ -12,3 +12,58 @@ Object path  [variable prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX
> > > >  Properties     byte Percentage [readonly]
> > > >
> > > >                         The percentage of battery left as an unsigned 8-bit integer.
> > > > +
> > > > +               string Source [readonly, optional, experimental]
> > > > +
> > > > +                       Describes where the battery information comes from
> > > > +                       (e.g. "HFP 1.7", "HID").
> > > > +                       This property is informational only and may be useful
> > > > +                       for debugging purposes.
> > >
> > > We should probably tight this to UUID instead.
> > Ack. Will update the doc to suggest UUID in this source field.
> >
> > >
> > > > +
> > > > +
> > > > +Battery Provider Manager hierarchy
> > > > +==================================
> > > > +A battery provider starts by registering itself as a battery provider with the
> > > > +RegisterBatteryProvider method passing an object path as the provider ID. Then,
> > > > +it can start exposing org.bluez.BatteryProvider1 objects having the path
> > > > +starting with the given provider ID. It can also remove objects at any time.
> > > > +BlueZ will stop monitoring these exposed and removed objects after
> > > > +UnregisterBatteryProvider is called for that provider ID.
> > > > +
> > > > +Service                org.bluez
> > > > +Interface      org.bluez.BatteryProviderManager1 [experimental]
> > > > +Object path    /org/bluez/{hci0,hci1,...}
> > > > +
> > > > +Methods                void RegisterBatteryProvider(object provider)
> > > > +
> > > > +                       This registers a battery provider. A registered
> > > > +                       battery provider can then expose objects with
> > > > +                       org.bluez.BatteryProvider1 interface described below.
> > > > +
> > > > +               void UnregisterBatteryProvider(object provider)
> > > > +
> > > > +                       This unregisters a battery provider. After
> > > > +                       unregistration, the BatteryProvider1 objects provided
> > > > +                       by this client are ignored by BlueZ.
> > >
> > > Not sure if you were followinging the monitor patches, for registering
> > > objects we do prefer the ObjectManager style so multiple provider can
> > > be registered at once, also we may want to tight the control of
> > > battery provider with Profile API so we guarantee that the same entity
> > > that handles the profile connection is the one providing the battery
> > > status.
> > It is actually already in ObjectManager style. After the "root path"
> > is registered, the provider can expose many D-Bus objects at once and
> > bluetoothd can detect it.
> > About tying it with the Profile API, I will take a look at how it could be done.
> >
> > >
> > > > +
> > > > +
> > > > +Battery Provider hierarchy
> > > > +==========================
> > > > +
> > > > +Service                <client D-Bus address>
> > > > +Interface      org.bluez.BatteryProvider1 [experimental]
> > > > +Object path    {provider_root}/org/bluez/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX
> > > > +
> > > > +Properties     byte Percentage [readonly]
> > > > +
> > > > +                       The percentage of battery left as an unsigned 8-bit
> > > > +                       integer.
> > > > +
> > > > +               string Source [readonly, optional]
> > > > +
> > > > +                       Describes where the battery information comes from
> > > > +                       (e.g. "HFP 1.7", "HID").
> > > > +                       This property is informational only and may be useful
> > > > +                       for debugging purposes. The content of this property is
> > > > +                       a free string, but it is recommended to include the
> > > > +                       profile name and version to be useful.
> > > > --
> > > > 2.26.2
> > >
> > > Perhaps we should make this use the same interface as we use for the
> > > daemon itself (Battery1) and the Source there as well. Last but not
> > > least, have you explored the idea of exporting the battery status over
> > > uHID? If I got this right this would aggregate the status of different
> > > sources and then make the daemon expose them, which while it works for
> > > now it means that upper layer have different interfaces for handling a
> > > battery status of something plugged over USB and over Bluetooth.
> > I am okay with naming the interface Battery1, the same as the daemon.
> > Will make an update.
> > About the exporting battery status via UHID, it is possible to be
> > done, but it is an orthogonal problem that we plan to tackle
> > separately. Right now, this API is general enough for Chrome OS to
> > allow both HFP and HID batteries to be consolidated in BlueZ. Chrome
> > OS's powerd feeds only Bluetooth battery levels from
> > /sys/class/power_supply and filters out USB devices so the UI layer
> > does not need to worry about it: everything from BlueZ is tied to a
> > Bluetooth device.
>
> But how about devices pushing their battery status over HID reports
> instead of GATT? Afaik this is possible since the HID device may have
> support for USB (wired) as well where it exposes its battery status
> over HID it may just choose to continue doing so while connected over
> Bluetooth.
If the Bluetooth device reports battery status via HID, it will go
into sys/class/power_supply. In Chrome OS, powerd sends this back to
BlueZ because it knows the Bluetooth device address from
/sys/class/power_supply/ file name. This way the UI can get the
battery value from BlueZ since it's tied to a Bluetooth device. In
most Linux desktops, upower already exposes /sys/class/power_supply to
the UI directly so it's fine also.
>
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz