Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

exit (crash) immediately to shell if -b is passed #15

Closed
xxxserxxx opened this issue Feb 15, 2020 · 10 comments
Closed

exit (crash) immediately to shell if -b is passed #15

xxxserxxx opened this issue Feb 15, 2020 · 10 comments
Labels
bug Something isn't working
Milestone

Comments

@xxxserxxx
Copy link
Owner

xxxserxxx commented Feb 15, 2020

Starting gotop -b exits to shell immediately (see error log below).
It seems to try to read files in /sys/class/power_supply/hidpp_battery_0 that do not exist (although that directory exists).

Required information:

  • gotop version (gotop -v):
    2.0.0

  • The output of uname -a:
    Linux p72 4.20.2-1-default Add ability to send different signals to processes #1 SMP PREEMPT Sun Jan 13 12:37:46 UTC 2019 (036c5c2) x86_64 x86_64 x86_64 GNU/Linux

  • Terminal shell (e.g. zsh or bash):
    bash

  • Terminal emulator (e.g. iTerm or gnome terminal):
    urxvt

  • Any relevant hardware info:
    Lenovo ThinkPad P72

Also please copy or attach the following file if it exists and contains logs:

22:20:11 battery.go:30: failed to get battery info from system: [{Current:open /sys/class/power_supply/hidpp_battery_0/voltage_now: no such file or directory Full:open /sys/class/power_supply/hidpp_battery_0/voltage_now: no such file or directory Design:open /sys/class/power_supply/hidpp_battery_0/voltage_min_design: no such file or directory ChargeRate:open /sys/class/power_supply/hidpp_battery_0/voltage_now: no such file or directory Voltage:open /sys/class/power_supply/hidpp_battery_0/voltage_now: no such file or directory DesignVoltage:open /sys/class/power_supply/hidpp_battery_0/voltage_min_design: no such file or directory}]
22:20:11 battery.go:56: failed to get battery info from system: [{Current:open /sys/class/power_supply/hidpp_battery_0/voltage_now: no such file or directory Full:open /sys/class/power_supply/hidpp_battery_0/voltage_now: no such file or directory Design:open /sys/class/power_supply/hidpp_battery_0/voltage_min_design: no such file or directory ChargeRate:open /sys/class/power_supply/hidpp_battery_0/voltage_now: no such file or directory Voltage:open /sys/class/power_supply/hidpp_battery_0/voltage_now: no such file or directory DesignVoltage:open /sys/class/power_supply/hidpp_battery_0/voltage_min_design: no such file or directory}]
panic: runtime error: index out of range

goroutine 1 [running]:
github.com/cjbassi/gotop/src/termui.(*LineGraph).Draw(0xc000090540, 0xc000104150)
	/home/cjbassi/playground/gotop/src/termui/linegraph.go:82 +0xdfe
github.com/gizak/termui.(*Grid).Draw(0xc000464000, 0xc000104150)
	/home/cjbassi/.local/share/go/pkg/mod/github.com/gizak/termui@v0.0.0-20190114080300-043950fed066/grid.go:154 +0x1a3
github.com/gizak/termui.Render(0xc0004efec8, 0x1, 0x1)
	/home/cjbassi/.local/share/go/pkg/mod/github.com/gizak/termui@v0.0.0-20190114080300-043950fed066/render.go:22 +0x1c0
main.main()
	/home/cjbassi/playground/gotop/main.go:468 +0x3a5

Original cjbassi#100
Submitter @bubbleguuum

@xxxserxxx xxxserxxx added the bug Something isn't working label Feb 15, 2020
@xxxserxxx
Copy link
Owner Author

Can you paste the output of ls -Al /sys/class/power_supply/hidpp_battery_0?

@xxxserxxx
Copy link
Owner Author

ls -Al /sys/class/power_supply/hidpp_battery_0
lrwxrwxrwx 1 root root 0 Jan 28 11:46 /sys/class/power_supply/hidpp_battery_0 -> ../../devices/pci0000:00/0000:00:14.0/usb1/1-3/1-3:1.2/0003:046D:C52B.0003/0003:046D:4041.0005/power_supply/hidpp_battery_0

And the content of the whole directory:

ls -l /sys/class/power_supply/hidpp_battery_0/
total 0
-r--r--r-- 1 root root 4096 Jan 28 11:47 capacity_level
lrwxrwxrwx 1 root root    0 Jan 28 11:47 device -> ../../../0003:046D:4041.0005
-r--r--r-- 1 root root 4096 Jan 28 11:47 manufacturer
-r--r--r-- 1 root root 4096 Jan 28 11:47 model_name
-r--r--r-- 1 root root 4096 Jan 28 11:47 online
drwxr-xr-x 2 root root    0 Jan 28 11:47 power
lrwxrwxrwx 1 root root    0 Jan 28 11:47 powers -> ../../../0003:046D:4041.0005
-r--r--r-- 1 root root 4096 Jan 28 11:47 scope
-r--r--r-- 1 root root 4096 Jan 28 11:47 serial_number
-r--r--r-- 1 root root 4096 Jan 28 11:47 status
lrwxrwxrwx 1 root root    0 Jan 28 11:47 subsystem -> ../../../../../../../../../../class/power_supply
-r--r--r-- 1 root root 4096 Jan 28 11:47 type
-rw-r--r-- 1 root root 4096 Jan 28 11:47 uevent

@xxxserxxx
Copy link
Owner Author

I have the explanation. I also have the Logitech USB unifying receiver connected to my laptop.
It reports battery level of the wireless mouse using it, and /sys/class/power_supply/hidpp_battery_0/ is associated to it. The real laptop battery is in /sys/class/power_supply/BAT0. Distro is openSUSE Tumbleweed if that matters. If I unplug the USB receiver, gotop -b works fine and picks BAT0.

@xxxserxxx
Copy link
Owner Author

Glad you figured that out, that's an interesting situation.

I just pushed a commit that should make sure gotop doesn't panic anymore with the battery widget by correctly handling an error we were receiving, but I still need to fix the error.

So the deal is that we're using distatus/battery to fetch battery information, and it tries to gather information like voltage etc, in addition to current usage. But voltage info isn't provided for your mouse. We don't need that info anyway tho, so I'm thinking we can just fork that project and remove the voltage stuff.

@xxxserxxx
Copy link
Owner Author

Great !
This USB battery device with no voltage should probably be discarded, because the real one we want is the laptop battery (BAT0). In other terms, I would not want the battery widget to display the mouse battery rather than the laptop battery.

@xxxserxxx
Copy link
Owner Author

Looks like there's already an open issue at distatus/battery: distatus/battery#8.

@xxxserxxx
Copy link
Owner Author

Hey! The behaviour you're experiencing is, technically, correct. There's a battery(-like) device, but it lacks most of the battery info, hence it is reported, but mostly with errors. To workaround it, you should use granular error checking. I have just pushed an update for it to the CLI part: distatus/battery@c96c646.
Feel free to ping me, if you have any questions!

[BTW: The reason why most errors talk about voltage is because if battery fails to find info in Watts, it falls back to trying Amps*Volts, hence voltage error comes up as the last one. Just FYI ;-).]

@xxxserxxx
Copy link
Owner Author

@KenjiTakahashi FYI in case you feel inclined to check the code using your API. I'm not sure exactly how to test this.

@KenjiTakahashi
Copy link

It does looks reasonable to me. Would probably be a bit simpler, if you could check partial errors while iterating over the bottom loop, but I don't know how this could affect whatever makeId is doing, so just a thought.

@xxxserxxx
Copy link
Owner Author

@KenjiTakahashi Good suggestion, thanks. Since this is all (well, majority) inherited code, there are still areas where I need to better understand the reasoning behind the original logic -- and also the underlying library APIs -- when I'm making code changes, and solutions, like this one, tend to be course-grained. It'll improve over time, we can hope :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants