Bug 86144 - upower does not update /org/freedesktop/UPower/devices/DisplayDevice state
Summary: upower does not update /org/freedesktop/UPower/devices/DisplayDevice state
Status: RESOLVED FIXED
Alias: None
Product: upower
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: Richard Hughes
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-11-11 11:03 UTC by Eugenio
Modified: 2014-11-18 18:41 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
output (14.43 KB, text/plain)
2014-11-12 16:44 UTC, Eugenio
Details
daemon: Split out updating on_battery and warning_level (1.75 KB, patch)
2014-11-13 18:21 UTC, Bastien Nocera
Details | Splinter Review
daemon: Update display device when battery is removed (981 bytes, patch)
2014-11-13 18:21 UTC, Bastien Nocera
Details | Splinter Review
daemon: Split out updating on_battery and warning_level (2.16 KB, patch)
2014-11-14 22:04 UTC, Bastien Nocera
Details | Splinter Review
daemon: remove unused variable (771 bytes, patch)
2014-11-18 09:32 UTC, Peter Wu
Details | Splinter Review

Description Eugenio 2014-11-11 11:03:01 UTC
/org/freedesktop/UPower/devices/DisplayDevice is not updated after plugging and unplugging the battery in my laptop.

If I boot my laptop without the battery upower -d reports:

Device: /org/freedesktop/UPower/devices/line_power_ACAD
  native-path:          ACAD
  power supply:         yes
  updated:              mar 11 nov 2014 09:25:11 CET (6782 seconds ago)
  has history:          no
  has statistics:       no
  line-power
    warning-level:       none
    online:              yes
    icon-name:          'ac-adapter-symbolic'

Device: /org/freedesktop/UPower/devices/DisplayDevice
  power supply:         no
  updated:              gio 01 gen 1970 01:00:00 CET (1415701093 seconds ago)
  has history:          no
  has statistics:       no
  unknown
    warning-level:       none
    icon-name:          ''

Daemon:
  daemon-version:  0.99.1
  on-battery:      no
  lid-is-closed:   no
  lid-is-present:  yes
  critical-action: HybridSleep

----------------------------------------------------------------
After connecting the battery:
Device: /org/freedesktop/UPower/devices/line_power_ACAD
  native-path:          ACAD
  power supply:         yes
  updated:              mar 11 nov 2014 09:25:11 CET (6797 seconds ago)
  has history:          no
  has statistics:       no
  line-power
    warning-level:       none
    online:              yes
    icon-name:          'ac-adapter-symbolic'

Device: /org/freedesktop/UPower/devices/battery_BAT1
  native-path:          BAT1
  vendor:               LENOVO
  model:                PABAS0241231
  serial:               41167
  power supply:         yes
  updated:              mar 11 nov 2014 11:18:28 CET (0 seconds ago)
  has history:          yes
  has statistics:       yes
  battery
    present:             yes
    rechargeable:        yes
    state:               charging
    warning-level:       none
    energy:              34,86 Wh
    energy-empty:        0 Wh
    energy-full:         40,38 Wh
    energy-full-design:  40,4 Wh
    energy-rate:         0 W
    voltage:             11,875 V
    percentage:          86%
    capacity:            99,9505%
    technology:          lithium-ion
    icon-name:          'battery-full-charging-symbolic'
  History (charge):
    1415701107  86,000  charging
    1415701107  0,000   unknown
  History (rate):
    1415701107  0,000   unknown

Device: /org/freedesktop/UPower/devices/DisplayDevice
  power supply:         yes
  updated:              gio 01 gen 1970 01:00:00 CET (1415701108 seconds ago)
  has history:          no
  has statistics:       no
  battery
    present:             yes
    state:               charging
    warning-level:       none
    energy:              34,86 Wh
    energy-full:         40,38 Wh
    energy-rate:         0 W
    percentage:          86%
    icon-name:          'battery-full-charging-symbolic'

Daemon:
  daemon-version:  0.99.1
  on-battery:      no
  lid-is-closed:   no
  lid-is-present:  yes
  critical-action: HybridSleep
-----------------------------------------------------------------
Disconnecting the battery:
Device: /org/freedesktop/UPower/devices/line_power_ACAD
  native-path:          ACAD
  power supply:         yes
  updated:              mar 11 nov 2014 09:25:11 CET (6815 seconds ago)
  has history:          no
  has statistics:       no
  line-power
    warning-level:       none
    online:              yes
    icon-name:          'ac-adapter-symbolic'

Device: /org/freedesktop/UPower/devices/DisplayDevice
  power supply:         yes
  updated:              gio 01 gen 1970 01:00:00 CET (1415701126 seconds ago)
  has history:          no
  has statistics:       no
  battery
    present:             yes
    state:               charging
    warning-level:       none
    energy:              34,87 Wh
    energy-full:         40,38 Wh
    energy-rate:         0 W
    percentage:          86%
    icon-name:          'battery-full-charging-symbolic'

Daemon:
  daemon-version:  0.99.1
  on-battery:      no
  lid-is-closed:   no
  lid-is-present:  yes
  critical-action: HybridSleep

It seems that DisplayDevice is not updated even if there is not a battery present.
acpi_listen reports battery events after connect and disconnect (2 battery events each).
Reading some upower bug report I tried to trigger a refresh of the AC with: "qdbus --system  org.freedesktop.UPower /org/freedesktop/UPower/devices/line_power_ACAD org.freedesktop.UPower.Device.Refresh".
Refreshing the /org/freedesktop/UPower/devices/DisplayDevice does not work.
and then upower -d report:

Device: /org/freedesktop/UPower/devices/line_power_ACAD
  native-path:          ACAD
  power supply:         yes
  updated:              mar 11 nov 2014 11:26:00 CET (1 seconds ago)
  has history:          no
  has statistics:       no
  line-power
    warning-level:       none
    online:              yes
    icon-name:          'ac-adapter-symbolic'

Device: /org/freedesktop/UPower/devices/DisplayDevice
  power supply:         yes
  updated:              gio 01 gen 1970 01:00:00 CET (1415701561 seconds ago)
  has history:          no
  has statistics:       no
  unknown
    warning-level:       none
    icon-name:          'battery-missing-symbolic'

Daemon:
  daemon-version:  0.99.1
  on-battery:      no
  lid-is-closed:   no
  lid-is-present:  yes
  critical-action: HybridSleep

And this is correct. This behaviour lead to this bug:
https://bugzilla.gnome.org/show_bug.cgi?id=731948

Other events are reported correctly (AC plug and unplug).
acpi -ab reports correctly the status with no visible delay.

Maybe the problem is that this laptop does not send AC events when battery is unplugged?
I don't know what should be the proper behavior..
Comment 1 Bastien Nocera 2014-11-12 10:13:46 UTC
If the battery is removed from the list, it's probably a simple bug in the creation of the aggregate battery device.
Comment 2 Bastien Nocera 2014-11-12 10:28:57 UTC
Can you run:
killall -9 upowerd ; /usr/libexec/upowerd -v
and capture the output when the battery is present and you unplug it?

This should help check where the problem is exactly.
Comment 3 Eugenio 2014-11-12 16:44:02 UTC
Created attachment 109358 [details]
output

here is the output of "killall -9 upowerd ; /usr/lib/upower/upowerd -v".
Comment 4 Bastien Nocera 2014-11-13 18:21:43 UTC
Created attachment 109433 [details] [review]
daemon: Split out updating on_battery and warning_level

So that we can reuse this code.
Comment 5 Bastien Nocera 2014-11-13 18:21:46 UTC
Created attachment 109434 [details] [review]
daemon: Update display device when battery is removed

When removing a battery, make sure to go through the batteries, and
update the display device status.
Comment 6 Bastien Nocera 2014-11-13 18:24:10 UTC
Can you please test those 2 patches on top of upower 1.0?
Comment 7 Eugenio 2014-11-14 10:49:06 UTC
I did not find upower 1.0 so I tested those patches on top of upower git.
In the first patch I think you forgot to move

gboolean ret;
UpDaemonPrivate *priv = daemon->priv;
UpDeviceLevel warning_level

from up_daemon_device_changed_cb, so i added those lines to up_daemon_update_warning_level in order to compile.

And now it works! DisplayDevice is updated and I get "battery-missing-symbolic".
Thank you!
Comment 8 Bastien Nocera 2014-11-14 22:04:29 UTC
Created attachment 109494 [details] [review]
daemon: Split out updating on_battery and warning_level

So that we can reuse this code.
Comment 9 Bastien Nocera 2014-11-14 22:10:42 UTC
Thanks for testing!

commit 2e87407eb91407e64f3d23de07b9dc91735a6a49
Author: Bastien Nocera <hadess@hadess.net>
Date:   Fri Nov 14 23:07:25 2014 +0100

    daemon: Use new warning update helper function
    
    Rather than the copy/paste code in commit:
    b9bd275890387fb35c185f37ec0ea1f2aa857818
    
    This makes sure that the AC status is updated.
    
    https://bugs.freedesktop.org/show_bug.cgi?id=86144

commit 83ebd3eccbff589af0dc6d2f1978338cdfa10833
Author: Bastien Nocera <hadess@hadess.net>
Date:   Thu Nov 13 19:20:12 2014 +0100

    daemon: Update display device when battery is removed
    
    When removing a battery, make sure to go through the batteries, and
    update the display device status.
    
    https://bugs.freedesktop.org/show_bug.cgi?id=86144

commit 4fdb9cee21d0d862ed3b551bebb48cf90d645de4
Author: Bastien Nocera <hadess@hadess.net>
Date:   Thu Nov 13 19:19:30 2014 +0100

    daemon: Split out updating on_battery and warning_level
    
    So that we can reuse this code.
    
    https://bugs.freedesktop.org/show_bug.cgi?id=86144
Comment 10 Peter Wu 2014-11-18 09:32:53 UTC
Created attachment 109661 [details] [review]
daemon: remove unused variable

An unused variable was left.
Comment 11 Bastien Nocera 2014-11-18 10:34:47 UTC
(In reply to Peter Wu from comment #10)
> Created attachment 109661 [details] [review] [review]
> daemon: remove unused variable
> 
> An unused variable was left.

Pushed. Please use full commit-id in the commit messages, don't add Signed-off-by, and either file new bugs or reopen the existing ones, if you don't want patches to be lost.
Comment 12 Peter Wu 2014-11-18 12:21:58 UTC
(In reply to Bastien Nocera from comment #11)
> Pushed. Please use full commit-id in the commit messages,

Thanks, I learned about using the `git describe` output from the git people. A full commit hash is not human readable, and the git describe output contains both the human readable tag (base) and an abbreviated git hash. I do not mind either way, this is my motivation.

> don't add Signed-off-by, and either file new bugs or
> reopen the existing ones, if you don't want patches to be lost.

This was a quick fix, I'll sent others to the devkit-devel list.
Comment 13 Bastien Nocera 2014-11-18 12:25:49 UTC
(In reply to Peter Wu from comment #12)
> (In reply to Bastien Nocera from comment #11)
> > Pushed. Please use full commit-id in the commit messages,
> 
> Thanks, I learned about using the `git describe` output from the git people.
> A full commit hash is not human readable, and the git describe output
> contains both the human readable tag (base) and an abbreviated git hash. I
> do not mind either way, this is my motivation.
> 
> > don't add Signed-off-by, and either file new bugs or
> > reopen the existing ones, if you don't want patches to be lost.
> 
> This was a quick fix, I'll sent others to the devkit-devel list.

Bugzilla is better actually.
Comment 14 Peter Wu 2014-11-18 12:34:36 UTC
(In reply to Bastien Nocera from comment #13)
> (In reply to Peter Wu from comment #12)
[..]
> > This was a quick fix, I'll sent others to the devkit-devel list.
> 
> Bugzilla is better actually.

Okay, should I open a single "fix memleaks" bug with all patches attached? The patches are already availble at
https://git.lekensteyn.nl/upower/log/?h=memleak-fixes

About the S-o-b line, I was under the impression that this was preferred Richard Hughes (UPower maintainer) does this (git log).
Comment 15 Bastien Nocera 2014-11-18 18:41:33 UTC
(In reply to Peter Wu from comment #14)
> (In reply to Bastien Nocera from comment #13)
> > (In reply to Peter Wu from comment #12)
> [..]
> > > This was a quick fix, I'll sent others to the devkit-devel list.
> > 
> > Bugzilla is better actually.
> 
> Okay, should I open a single "fix memleaks" bug with all patches attached?
> The patches are already availble at
> https://git.lekensteyn.nl/upower/log/?h=memleak-fixes

Please. You can use "git-bz" to do that, it makes it easier.

> About the S-o-b line, I was under the impression that this was preferred
> Richard Hughes (UPower maintainer) does this (git log).

Right. I don't, and a lot of other contributors don't, which makes its usage moot.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.