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

fix battery prompt on pinebook pro #1020

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wimh
Copy link

@wimh wimh commented Sep 19, 2020

On my pinebook pro the battery level indication did not work, this PR contains the changes I had to make to fix this;

  • The device is called /sys/class/power_supply/cw2015-battery, therefore I replaced battery with *battery twice.
  • charge_counter contains an unusable value, so I removed that.

As the last change might cause problems for others, I checked https://www.kernel.org/doc/html/latest/power/power_supply_class.html which explains:

CHARGE_COUNTER
the current charge counter (in µAh). This could easily be negative; there is no empty or full value. It is only useful for relative, time-based measurements.

Based on this description, it should not be useful to calculate the battery level. The usage of charge_counter was added in commit 6a1f065 for #88. I don't see a reason for the change there. In that ticket you also ask for some diagnostics, I'll also provide that, but a slightly modified version;

() {
  emulate -L zsh && setopt xtrace null_glob
  echo OS=$OS
  uname
  uname -o
  uname -a
  ls /sys/class/power_supply/(BAT*|*battery)/
  cat /dev/null /sys/class/power_supply/(BAT*|*battery)/(energy|charge)_now
  cat /dev/null /sys/class/power_supply/(BAT*|*battery)/(energy|charge)_full
  cat /dev/null /sys/class/power_supply/(BAT*|*battery)/(power|current)_now
  cat /dev/null /sys/class/power_supply/(BAT*|*battery)/status
  cat /dev/null /sys/class/power_supply/(BAT*|*battery)/capacity
}

+(anon):2> echo 'OS=Linux'
OS=Linux
+(anon):3> uname
Linux
+(anon):4> uname -o
GNU/Linux
+(anon):5> uname -a
Linux pine1 5.5.0-1-pinebookpro-arm64 #1 SMP PREEMPT Wed Aug 12 04:14:48 UTC 2020 aarch64 GNU/Linux
+(anon):6> ls /sys/class/power_supply/cw2015-battery/
capacity	charge_full_design  health  present    technology	  type	       wakeup7
charge_counter	current_now	    hwmon5  status     temp		  uevent
charge_full	device		    power   subsystem  time_to_empty_now  voltage_now
+(anon):7> cat /dev/null
+(anon):8> cat /dev/null /sys/class/power_supply/cw2015-battery/charge_full
9800000
+(anon):9> cat /dev/null /sys/class/power_supply/cw2015-battery/current_now
1189674
+(anon):10> cat /dev/null /sys/class/power_supply/cw2015-battery/status
Discharging
+(anon):11> cat /dev/null /sys/class/power_supply/cw2015-battery/capacity
87

(charge_counter contains 3 right now)

Let me know if you need any further information.

@romkatv
Copy link
Owner

romkatv commented Sep 21, 2020

I cannot merge the PR as is because it breaks battery on WSL. I'll address it within a week. Please wait. There is no need for you to do anything.

@romkatv
Copy link
Owner

romkatv commented Oct 1, 2020

Status update: I'll have to postpone this for the time being. Hopefully will get to it in October but no guarantees.

By the way, why do you want to display battery in prompt? It's not a property of a shell session but rather of the machine. System tray or its equivalent is a better fit to display machine-level information. Every OS I know of display battery level by default.

@wimh
Copy link
Author

wimh commented Oct 3, 2020

No worries. It is fixed on my laptop, I just wanted to share it in case anybody else has the same problem. But if you consider WSL compatibility more important, I would not mind if you close this PR without merging.

And yes, I can find the same information in the system tray. And the system tray information is even more up to date. So maybe I will remove it some day.

@romkatv
Copy link
Owner

romkatv commented Oct 3, 2020

if you consider WSL compatibility more important

Yes, I consider not breaking existing working code more important than enabling features that never worked to begin with.

I would not mind if you close this PR without merging.

Noted.

And yes, I can find the same information in the system tray. And the system tray information is even more up to date. So maybe I will remove it some day.

Indeed.

Battery segment is useless. It's not the only one of this kind. There are plenty of other segments that serve purely decorative function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants