greshake / i3status-rust

Very resourcefriendly and feature-rich replacement for i3status, written in pure Rust
GNU General Public License v3.0
2.88k stars 475 forks source link

Error in block 'battery': Failed to read UPower Type property #1405

Closed Noah-Kennedy closed 2 years ago

Noah-Kennedy commented 2 years ago

Just happened upon upgrading to 0.21.1 from 0.20.7. I am running arch linux. All packages are up-to-date on my system.

ammgws commented 2 years ago

So I'm guessing everyone affected is not using allow_missing in their block config?

@MaxVerevkin, @bim9262 I take it that your systems aren't affected by this? The only battery device I am using daily is my PS5 controller and I have no issues with the battery block..

Crandel commented 2 years ago

Yes, I have no allow_missing in my config, as it no required and I fine with default value

MaxVerevkin commented 2 years ago

@MaxVerevkin ... I take it that your systems aren't affected by this?

1) I don't have removable batteries. 2) I don't use UPower.

cfsmp3 commented 2 years ago

allow_missing seems to "protect" against the battery device itself being missing, not about a specific property being missing.

I can't test this much (I can't hit that message), but on my desktop (obviously, no battery) I need allow_missing to get the bar to work at all (otherwise I just get an error about the device not existing).

Of course the reason I have the battery block on my desktop in the first place is that I share the config with my laptop.

ammgws commented 2 years ago

https://github.com/greshake/i3status-rust/commit/fe5f2d7fcc49dca56beefbedd967f974cb2c49de#diff-8c7278300d3162b636edeeae47c136cab4273d1dc31c8eb3a9c850842fa6dfc7

From a quick look, before we would only hit that code path (.get("org.freedesktop.UPower.Device", "Type")) once whereas now it happens on every device refresh. So perhaps it's another timing issue with is_available inside refresh_device_info?

MaxVerevkin commented 2 years ago

Not a solution to this issue, but do we even need to check the type?

Crandel commented 2 years ago

I add allow_missing to my config and it stop panic, but show me no battery at all

ammgws commented 2 years ago

Not a solution to this issue, but do we even need to check the type?

If the comment below is no longer relevant, than I don't think so, since anything that's available on UPower should work, right?

https://github.com/greshake/i3status-rust/issues/421

This is a tricky issue. We use the battery_ prefix so that the block can use the same device name as sysfs, but it's possible we could add more clever logic to support other devices. What does your mouse battery look like in /sys/class/power_supply?

bim9262 commented 2 years ago

@cfsmp3 , @Crandel , @Noah-Kennedy , can you share the output from:

busctl introspect --system org.freedesktop.UPower /org/freedesktop/UPower/devices/$device org.freedesktop.UPower.Device

You can find $device from upower --enumerate.

Can you also share your config (just the battery block is fine)?

@ammgws , I don't know what the motivation was initially for not allowing 'Line Power' devices, perhaps they just have default values and aren't very interesting, but I don't think that displaying one is really a problem, but don't have one to test.

We don't use the battery_ prefix anymore. (I noted that the comment hasn't been updated to reflect that though). In #423 we started looking for devices (from the list of enumerated devices) that had a suffix of the device name, but since #1378 the device name must be an exact match.

MaxVerevkin commented 2 years ago

perhaps they just have default values and aren't very interesting

Yeah, exactly

❯ qdbus --system org.freedesktop.UPower /org/freedesktop/UPower/devices/line_power_ADP1 org.freedesktop.UPower.Device.Type
1

❯ qdbus --system org.freedesktop.UPower /org/freedesktop/UPower/devices/line_power_ADP1 org.freedesktop.UPower.Device.State
0

❯ qdbus --system org.freedesktop.UPower /org/freedesktop/UPower/devices/line_power_ADP1 org.freedesktop.UPower.Device.Percentage
0

❯ qdbus --system org.freedesktop.UPower /org/freedesktop/UPower/devices/line_power_ADP1 org.freedesktop.UPower.Device.TimeToFull
0

❯ qdbus --system org.freedesktop.UPower /org/freedesktop/UPower/devices/line_power_ADP1 org.freedesktop.UPower.Device.TimeToEmpty
0
Crandel commented 2 years ago
upower --enumerate
/org/freedesktop/UPower/devices/line_power_AC
/org/freedesktop/UPower/devices/battery_BAT0
/org/freedesktop/UPower/devices/line_power_ucsi_source_psy_USBC000o001
/org/freedesktop/UPower/devices/line_power_ucsi_source_psy_USBC000o002
/org/freedesktop/UPower/devices/line_power_ucsi_source_psy_USBC000o003
/org/freedesktop/UPower/devices/DisplayDevice
busctl introspect --system org.freedesktop.UPower /org/freedesktop/UPower/devices/battery_BAT0 org.freedesktop.UPower.Dev

NAME TYPE SIGNATURE RESULT/VALUE FLAGS

The same empty output for DisplayDevice

bim9262 commented 2 years ago

looks like you are missing the ice on Device at the end of your busctl command.

Crandel commented 2 years ago

@bim9262 Yes, you are right

busctl introspect --system org.freedesktop.UPower /org/freedesktop/UPower/devices/battery_BAT0 org.freedesktop.UPower.Device
NAME                          TYPE      SIGNATURE RESULT/VALUE            FLAGS
.GetHistory                   method    suu       a(udu)                  -
.GetStatistics                method    s         a(dd)                   -
.Refresh                      method    -         -                       -
.BatteryLevel                 property  u         1                       emits-change
.Capacity                     property  d         100                     emits-change
.Energy                       property  d         48.184                  emits-change
.EnergyEmpty                  property  d         0                       emits-change
.EnergyFull                   property  d         51.9992                 emits-change
.EnergyFullDesign             property  d         51.9992                 emits-change
.EnergyRate                   property  d         7.3188                  emits-change
.HasHistory                   property  b         true                    emits-change
.HasStatistics                property  b         true                    emits-change
.IconName                     property  s         "battery-full-symbolic" emits-change
.IsPresent                    property  b         true                    emits-change
.IsRechargeable               property  b         true                    emits-change
.Luminosity                   property  d         0                       emits-change
.Model                        property  s         "DELL G8VCF6C"          emits-change
.NativePath                   property  s         "BAT0"                  emits-change
.Online                       property  b         false                   emits-change
.Percentage                   property  d         92                      emits-change
.PowerSupply                  property  b         true                    emits-change
.Serial                       property  s         "2870"                  emits-change
.State                        property  u         2                       emits-change
.Technology                   property  u         2                       emits-change
.Temperature                  property  d         0                       emits-change
.TimeToEmpty                  property  x         23700                   emits-change
.TimeToFull                   property  x         0                       emits-change
.Type                         property  u         2                       emits-change
.UpdateTime                   property  t         1642626840              emits-change
.Vendor                       property  s         "SMP"                   emits-change
.Voltage                      property  d         8.446                   emits-change
.WarningLevel                 property  u         1                       emits-change
bim9262 commented 2 years ago

@Crandel do you have device = "battery_BAT0" or device = "BAT0" in your config?

Crandel commented 2 years ago

@bim9262 No, I use DisplayDevice. Output the same as battery_BAT0

Celti commented 2 years ago

I can confirm this problem is happening on Arch Linux after upgrading to 0.21.1. It happens even if I rebuild the package locally, or if I build i3status-rs manually from git.

My battery block is as minimal as possible, specifying only block, driver, and the device as DisplayDevice. If I change the device to battery_BAT0 the problem remains. If I add allow_missing then the bar loads, but the battery is reported as disconnected even when the battery is present and I am in fact running on battery.

Inspecting the bus manually via busctl or d-feet shows the Type property is showing 2, as it should be.

bim9262 commented 2 years ago

I found the error. See the linked PR for the fix.

Celti commented 2 years ago

You beat me to it by moments! :+1:

ammgws commented 2 years ago

As mentioned here we should select battery devices first - I think that's what we did previously?

bim9262 commented 2 years ago

@FlorianFranzen pointed out two issues.

  1. I changed how we matched the device to the device_path.
  2. DisplayDevice does no longer work with allow_missing

First we can restore the original logic that was in place for how a device to the device_path. Second the DisplayDevice is not in the EnumerateDevices so we need to account for that in is_available.

I have made a draft commit ready, but we should talk about if there needs to be a new option for allowing rescanning devices as mentioned in #1362

ammgws commented 2 years ago

What are the disadvantages of applying your commit without considering rescanning devices for the moment?

bim9262 commented 2 years ago

@ammgws, if we apply the commit as it stands then we must scan for a new device_path each time is_available and refresh_device_info. #1362 discusses calling refresh_device_info before is_available in update which will remove one additional dbus call each update. By adding a rescan_if_missing option you can avoid rescanning all together.

ammgws commented 2 years ago

If it's just about reducing dbus calls then we should apply the commit now and optimise later.

bim9262 commented 2 years ago

PR opened @ammgws