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>
In-Reply-To: <CABBYNZJkyOBT63aAYEc4CtCJtqaQEW3bLyyrJ=XJQ_EpeDDmuQ@mail.gmail.com>
From:   Sonny Sasaka <sonnysasaka@chromium.org>
Date:   Tue, 17 Nov 2020 14:37:33 -0800
Message-ID: <CAO271mkHCaQ0f+3PKjmnoAqszk1XuUFi_gF5rO9GvsgmqOOiVQ@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,

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.

>
>
> --
> Luiz Augusto von Dentz