Hi,
I need your opinion if reporting battery condition/changing reporting
speed of a bluetooth device through sysfs is an acceptable practice.
[PATCH] Add sysfs battery & speed attributes for wacom bluetooth tablet
The patch creates 2 sysfs attributes:
The battery attribute is read-only and it appears in:
/sys/bus/hid/devices/{btaddr}/battery
/sys/class/bluetooth/hci*:*/{btaddr}/battery
/sys/class/hidraw/hidraw*/device/battery
Capacity values are in %, zero value means AC plug is connected.
The speed attribute allows to poke reporting speed of wacom tablet.
This attribute is RW, valid values are: 5 for low speed, 6 is high
speed. High speed is the default value. Using low speed is
a workaround if you experience big delay between move of stylus on
the tablet and move of cursor on screen.
--
Kind regards,
Przemo
Dnia 2010-03-16, wto o godzinie 11:49 +0100, Jiri Kosina pisze:
[..]
> I have applied it, thanks.
Thanks!
> One question still though ..
>
> > + case POWER_SUPPLY_PROP_CAPACITY:
> > + /* show 100% battery capacity when charging */
> > + if (power_state == 0)
> > + val->intval = 100;
> > + else
> > + val->intval = power_state;
> > + break;
>
> Why is it not possible to show the actual percentage in the charging state
> as well?
The device doesn't report capacity during charging as far as I can tell.
We could keep last reported value (bad idea IMHO), set it to 0 (even
worse) or set it to 100% - a bit misleading, but I can't see any other
option.
cheers,
Przemo
On Mon, 15 Mar 2010, Przemo Firszt wrote:
> > > Does it make sense to add a line to Kconfig to explain that
> > > CONFIG_POWER_SUPPLY/CONFIG_POWER_SUPPLY_MODULE is required to enable
> > > monitoring battery/ac state?
> >
> > Either that, or introducing separate CONFIG sub-option for the Wacom
> > driver might be reasonable option as well.
> Hi Jiri,
> Thanks for help - next time I'll know how to do it :-)
> I went the CONFIG sub-option way - see attached.
I have applied it, thanks.
One question still though ..
> + case POWER_SUPPLY_PROP_CAPACITY:
> + /* show 100% battery capacity when charging */
> + if (power_state == 0)
> + val->intval = 100;
> + else
> + val->intval = power_state;
> + break;
Why is it not possible to show the actual percentage in the charging state
as well?
Thanks,
--
Jiri Kosina
SUSE Labs, Novell Inc.
Dnia 2010-03-15, pon o godzinie 14:54 +0100, Jiri Kosina pisze:
> On Wed, 10 Mar 2010, Przemo Firszt wrote:
[..]
> > Does it make sense to add a line to Kconfig to explain that
> > CONFIG_POWER_SUPPLY/CONFIG_POWER_SUPPLY_MODULE is required to enable
> > monitoring battery/ac state?
>
> Either that, or introducing separate CONFIG sub-option for the Wacom
> driver might be reasonable option as well.
Hi Jiri,
Thanks for help - next time I'll know how to do it :-)
I went the CONFIG sub-option way - see attached.
thanks,
Przemo
On Wed, 10 Mar 2010, Przemo Firszt wrote:
> > Anyway, you'll have to sort out the new dependency on CONFIG_POWER_SUPPLY
> > somehow (compiling the battery code out from the driver if
> > CONFIG_POWER_SUPPLY is unset, or selecting it directly from Kconfig).
> Thanks for checking the patch.
> See attached updated version - is it OK?
> Does it make sense to add a line to Kconfig to explain that
> CONFIG_POWER_SUPPLY/CONFIG_POWER_SUPPLY_MODULE is required to enable
> monitoring battery/ac state?
Either that, or introducing separate CONFIG sub-option for the Wacom
driver might be reasonable option as well.
Otherwise the patch looks good.
Thanks,
--
Jiri Kosina
SUSE Labs, Novell Inc.
Dnia 2010-03-09, wto o godzinie 22:22 +0100, Jiri Kosina pisze:
Hi,
[..]
> Anyway, you'll have to sort out the new dependency on CONFIG_POWER_SUPPLY
> somehow (compiling the battery code out from the driver if
> CONFIG_POWER_SUPPLY is unset, or selecting it directly from Kconfig).
Thanks for checking the patch.
See attached updated version - is it OK?
Does it make sense to add a line to Kconfig to explain that
CONFIG_POWER_SUPPLY/CONFIG_POWER_SUPPLY_MODULE is required to enable
monitoring battery/ac state?
Cheers,
Przemo
On Tue, 9 Mar 2010, Przemo Firszt wrote:
> Please ignore previous patch
Hi Prezemo,
thanks for basing the patch on power_supply infrastructure.
Anyway, you'll have to sort out the new dependency on CONFIG_POWER_SUPPLY
somehow (compiling the battery code out from the driver if
CONFIG_POWER_SUPPLY is unset, or selecting it directly from Kconfig).
Thanks,
--
Jiri Kosina
SUSE Labs, Novell Inc.
Please ignore previous patch
--
Przemo
Dnia 2010-03-02, wto o godzinie 12:11 +0000, Bastien Nocera pisze:
[..]
> > A couple of comments:
> <snip>
> > - isn't there a more kernel-y way to export that data, so that it's
> > automatically picked up by things like upower (né DeviceKit-power)?
>
> I've been told it should use the power_supply class, so it would work
> pretty much out-of-the-box with things like upower and
> gnome-power-manager.
Thanks for the comments - it was exactly that what I was looking
for! :-)
Please find attached revamped patch - this time battery & ac patch only.
Speed switching isn't yet ready for release.
For some reason gnome-power-manager isn't updating the values properly,
despite of that they are OK in:
/sys/class/power_supply/wacom_{ac,battery}/uevent
I'm not sure if it's a result of lack of .external_power_changed (it's
missing in some drivers using power_supply) or something else. Any
hints?
Cheers,
Przemo
Dnia 2010-03-08, pon o godzinie 12:07 +0100, Jiri Kosina pisze:
> On Sun, 28 Feb 2010, Przemo Firszt wrote:
>
> > Hi,
> > I need your opinion if reporting battery condition/changing reporting
> > speed of a bluetooth device through sysfs is an acceptable practice.
>
> > [PATCH] Add sysfs battery & speed attributes for wacom bluetooth tablet
> >
> > The patch creates 2 sysfs attributes:
> > The battery attribute is read-only and it appears in:
> > /sys/bus/hid/devices/{btaddr}/battery
> > /sys/class/bluetooth/hci*:*/{btaddr}/battery
> > /sys/class/hidraw/hidraw*/device/battery
> > Capacity values are in %, zero value means AC plug is connected.
> >
> > The speed attribute allows to poke reporting speed of wacom tablet.
> > This attribute is RW, valid values are: 5 for low speed, 6 is high
> > speed. High speed is the default value. Using low speed is
> > a workaround if you experience big delay between move of stylus on
> > the tablet and move of cursor on screen.
>
> Hi Przemo,
>
> do you see any obstacle to using CONFIG_POWER_SUPPLY (see drivers/power)
> infrastructure for this?
No, I don't. I'm very sorry for the delay, but I've been rather busy in
last few days. I've got patch implementing usage of power_supply ready -
I just wanted to send it together with switching of reporting speed.
I'll clean it and send it ASAP (but not today :-( ).
--
Cheers,
Przemo
On Sun, 28 Feb 2010, Przemo Firszt wrote:
> Hi,
> I need your opinion if reporting battery condition/changing reporting
> speed of a bluetooth device through sysfs is an acceptable practice.
> [PATCH] Add sysfs battery & speed attributes for wacom bluetooth tablet
>
> The patch creates 2 sysfs attributes:
> The battery attribute is read-only and it appears in:
> /sys/bus/hid/devices/{btaddr}/battery
> /sys/class/bluetooth/hci*:*/{btaddr}/battery
> /sys/class/hidraw/hidraw*/device/battery
> Capacity values are in %, zero value means AC plug is connected.
>
> The speed attribute allows to poke reporting speed of wacom tablet.
> This attribute is RW, valid values are: 5 for low speed, 6 is high
> speed. High speed is the default value. Using low speed is
> a workaround if you experience big delay between move of stylus on
> the tablet and move of cursor on screen.
Hi Przemo,
do you see any obstacle to using CONFIG_POWER_SUPPLY (see drivers/power)
infrastructure for this?
Thanks,
--
Jiri Kosina
SUSE Labs, Novell Inc.
On Tue, 2010-03-02 at 12:02 +0000, Bastien Nocera wrote:
> Hey Przemo,
>
> On Sun, 2010-02-28 at 20:39 +0000, Przemo Firszt wrote:
> > Hi,
> > I need your opinion if reporting battery condition/changing reporting
> > speed of a bluetooth device through sysfs is an acceptable practice.
> >
> > [PATCH] Add sysfs battery & speed attributes for wacom bluetooth tablet
> >
> > The patch creates 2 sysfs attributes:
> > The battery attribute is read-only and it appears in:
> > /sys/bus/hid/devices/{btaddr}/battery
> > /sys/class/bluetooth/hci*:*/{btaddr}/battery
> > /sys/class/hidraw/hidraw*/device/battery
> > Capacity values are in %, zero value means AC plug is connected.
>
> A couple of comments:
<snip>
> - isn't there a more kernel-y way to export that data, so that it's
> automatically picked up by things like upower (n? DeviceKit-power)?
I've been told it should use the power_supply class, so it would work
pretty much out-of-the-box with things like upower and
gnome-power-manager.
Cheers
Hey Przemo,
On Sun, 2010-02-28 at 20:39 +0000, Przemo Firszt wrote:
> Hi,
> I need your opinion if reporting battery condition/changing reporting
> speed of a bluetooth device through sysfs is an acceptable practice.
>
> [PATCH] Add sysfs battery & speed attributes for wacom bluetooth tablet
>
> The patch creates 2 sysfs attributes:
> The battery attribute is read-only and it appears in:
> /sys/bus/hid/devices/{btaddr}/battery
> /sys/class/bluetooth/hci*:*/{btaddr}/battery
> /sys/class/hidraw/hidraw*/device/battery
> Capacity values are in %, zero value means AC plug is connected.
A couple of comments:
- battery status and adapter status should probably be 2 separate
values. I don't know what the hardware actually exports, but I hope to
have access to the specs of the device soon, which should answer that
question.
- isn't there a more kernel-y way to export that data, so that it's
automatically picked up by things like upower (n? DeviceKit-power)?
> The speed attribute allows to poke reporting speed of wacom tablet.
> This attribute is RW, valid values are: 5 for low speed, 6 is high
> speed. High speed is the default value. Using low speed is
> a workaround if you experience big delay between move of stylus on
> the tablet and move of cursor on screen.
5 and 6 are magic values. I'd rather have a boolean "high_speed"
attribute which would switch between high-speed and low-speed.
Finally, the patch should be split in at least 2 parts, one to add the
sysfs infrastructure and one of the properties, and another one adding
the property itself (maybe even 3, if we were to split the sysfs
enablement from the properties themselves).
But that's a minor problem, and if the maintainers are happy taking a
single patch for the features, then I'm happy as well :)
Cheers