Re: [PATCH 2/3] profiles/battery: Add Bluetooth LE Battery service

[Luiz Augusto von Dentz <luiz.dentz@gmail.com>]

Hi Bastien,

On Thu, Oct 26, 2017 at 5:47 PM, Bastien Nocera <hadess@hadess.net> wrote:
> On Thu, 2017-10-26 at 16:36 +0300, Luiz Augusto von Dentz wrote:
>> Hi Bastien,
>>
>> On Thu, Oct 26, 2017 at 4:19 PM, Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>> > Hi Bastien,
>> >
>> > On Tue, Oct 24, 2017 at 3:30 PM, Bastien Nocera <hadess@hadess.net>
>> > wrote:
>> > > On Tue, 2017-10-24 at 14:58 +0300, Luiz Augusto von Dentz wrote:
>> > > > Hi Bastien,
>> > > >
>> > > > On Sat, Oct 21, 2017 at 2:03 AM, Bastien Nocera <hadess@hadess.
>> > > > net>
>> > > > wrote:
>> > > > > Only the Battery Level characteristic was tested with a
>> > > > > Microsoft Arc Touch Mouse SE.
>> > > > >
>> > > > > This patch however hopes to support both the Battery Level
>> > > > > and
>> > > > > the Battery Power State characteristics.
>> > > > > ---
>> > > > >  Makefile.plugins           |   3 +
>> > > > >  doc/battery-api.txt        |  36 +++
>> > > > >  profiles/battery/battery.c | 568
>> > > > > +++++++++++++++++++++++++++++++++++++++++++++
>> > > > >  3 files changed, 607 insertions(+)
>> > > > >  create mode 100644 doc/battery-api.txt
>> > > > >  create mode 100644 profiles/battery/battery.c
>> > > > >
>> > > > > diff --git a/Makefile.plugins b/Makefile.plugins
>> > > > > index 73377e532..1f3b5b552 100644
>> > > > > --- a/Makefile.plugins
>> > > > > +++ b/Makefile.plugins
>> > > > > @@ -100,6 +100,9 @@ builtin_sources += profiles/midi/midi.c \
>> > > > >  builtin_ldadd += @ALSA_LIBS@
>> > > > >  endif
>> > > > >
>> > > > > +builtin_modules += battery
>> > > > > +builtin_sources += profiles/battery/battery.c
>> > > > > +
>> > > > >  if SIXAXIS
>> > > > >  plugin_LTLIBRARIES += plugins/sixaxis.la
>> > > > >  plugins_sixaxis_la_SOURCES = plugins/sixaxis.c
>> > > > > diff --git a/doc/battery-api.txt b/doc/battery-api.txt
>> > > > > new file mode 100644
>> > > > > index 000000000..6d28f7b17
>> > > > > --- /dev/null
>> > > > > +++ b/doc/battery-api.txt
>> > > > > @@ -0,0 +1,36 @@
>> > > > > +BlueZ D-Bus Battery API description
>> > > > > +***********************************
>> > > > > +
>> > > > > +
>> > > > > +Battery hierarchy
>> > > > > +=================
>> > > > > +
>> > > > > +Service                org.bluez
>> > > > > +Interface      org.bluez.Battery1
>> > > > > +Object path    [variable
>> > > > > prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX
>> > > > > +
>> > > > > +Properties     boolean Present [readonly]
>> > > > > +
>> > > > > +                       Whether the device has a battery
>> > > > > present.
>> > > >
>> > > > Not sure why would we have this as a property, if the battery
>> > > > is not
>> > > > present then we should not even publish the interface.
>> > >
>> > > UPower has a "present" property as well, so this matches both the
>> > > service and the UPower properties. It avoids logic bugs where the
>> > > battery presence changes but we'd forget to export/unexport the
>> > > interface (or races). Finally, it's useful information for
>> > > devices
>> > > which have another means of power (I can imagine some devices
>> > > having
>> > > batteries, but also working wired, with a removable battery).
>> >
>> > Not sure what another power source has to do with it? Power and
>> > battery are 2 different things. About race conditions, if you do
>> > have
>> > the interface it means it is battery powered if we do set its
>> > presence
>> > to false all other properties should be invalidated as well and the
>> > same should go when battery is plugged again all properties have to
>> > be
>> > fetched once again, so imo it easier to add and remove the entire
>> > interface to avoid having properties in inconsistent state. Btw,
>> > forgetting to export/unexport may happen either way if the device
>> > decides to remove the entire service that is what should happen, in
>> > fact is what I would do if implementing the service so there is no
>> > chance the remote side would maintain a subscription for something
>> > that is not actually present anymore.
>>
>> Btw, the battery state don't seem to be even mentioned in the spec,
>> also the service declaration don't mention it only the battery level:
>>
>> https://www.bluetooth.com/specifications/gatt/viewer?attributeXmlFile
>> =org.bluetooth.service.battery_service.xml
>
> You're looking at the wrong spec. There's 2 different set of
> characteristics, one called "Battery Level", which pretty much only
> exports the percentage, and "Battery State" which has a presence value:
> https://www.bluetooth.com/specifications/gatt/viewer?attributeXmlFile=org.bluetooth.characteristic.battery_power_state.xml

But it is just a characteristic, it must belong to service, and
apparently, it doesn't belong to BAS as not even the qualification
tests mention it.

> I don't know whether it's actually an adopted spec, or whether it's
> used, and I'm fine simply removing the support for the time being. Or
> we could implement it and see whether it breaks anything :)

We may enable it if we find out there are devices device implementing
it, not sure what happened with it perhaps it got detached from BAS to
not make it mandatory, which would be quite confusing. If really means
it is optional attribute though then we should make sure the interface
don't have any hard dependency on it, but Im having a second thought
that there might be disagreements on how to handle such attribute
since things like presence may influence the level as well, since they
are 2 different characteristics what do we do when it say the battery
is not present.


-- 
Luiz Augusto von Dentz