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

battery: Add {cycles}, {health} format replacements #3130

Merged
merged 10 commits into from
Apr 18, 2024
Merged

battery: Add {cycles}, {health} format replacements #3130

merged 10 commits into from
Apr 18, 2024

Conversation

kiriDevs
Copy link
Contributor

@kiriDevs kiriDevs commented Apr 10, 2024

This PR adds the following format replacements to the battery module (currently for Linux only):

  • {cycles} - Displays the amount of charge cycles the selected1 battery has seen.
  • {health} Displays a percentage representing the selected1 batteries max charge, relative to it's design (original) max charge.

Battery selection1

Currently, both of the new format replacements select the battery candidate with the highest charge_full_design. This seemed the most intuitive for {cycles}, and I have applied the same logic to {health} to stay consistent with this. Should multiple batteries have the same charge_full_design, the "worse" value (i.e. higher cycle count, lower health percentage) is displayed.

However, I could also imagine using a charge_full_design weighted average of all eligible batteries' percentages for {health}, so it could represent the health of a full battery array.

@kiriDevs kiriDevs marked this pull request as ready for review April 10, 2024 16:20
I even did this originally, then got confused when my battery in particular showed 102% and, instead of checking the values I calculate with, just decided to do the stupid thing and do maths the wrong around
was here before, but is an easy fix for a clang-tidy warning
@kiriDevs
Copy link
Contributor Author

kiriDevs commented Apr 17, 2024

About the remaining clang-tidy warnings:

floating point literal has suffix 'f', which is not uppercase

These warnings are (as far as I can tell) all caused by pre-existing code (the one float this PR introduces uses an uppercase F). Should I change them anyways?

function 'getInfos' has cognitive complexity of 224 (threshold 25)

This was far over threshold before, and I would prefer to not refactor (rewrite) that thing. Would it be acceptable to just add to it in this case?

function 'update' has cognitive complexity of 26 (threshold 25)

That one I could probably manage, if required.

@Alexays Alexays merged commit f26efae into Alexays:master Apr 18, 2024
7 of 9 checks passed
@Alexays
Copy link
Owner

Alexays commented Apr 18, 2024

Don't worry about clang-tidy, we need to adjust it :)
Thanks!
Can you also update the Github wiki?

@kiriDevs
Copy link
Contributor Author

Thank you for merging, and maintaining this project in general :)

Can you also update the Github wiki?

Done!

@nebulosa2007
Copy link

I've updated waybar to 0.10.1, updated config, reloaded waybar:

...
"format-alt": "{time} {health} {cycles} {icon}",
... 

but got this error:

[2024-04-23 11:48:31.588] [error] battery: argument not found

with just {health} or {cycles} either.
What should I check on my Arch Linux?

@kiriDevs
Copy link
Contributor Author

kiriDevs commented Apr 24, 2024

with just {health} or {cycles} either.

Did you try each one independently, or both at once?

After looking at the code again, it does appear like I did, in fact, never add {health} for the regular formatting, only the hover-tooltip (oops). However, {cycles} should already be registered for the "main" label.

Edit: Since it's not actually registered, I've commented out {health} from the wiki again for now; I'll change that later today (and hopefully fix @nebulosa2007's issue in the process).

@kiriDevs
Copy link
Contributor Author

I've opened #3167 to address {health} not being registered, along with any other issues that could be related here.

@kiriDevs kiriDevs deleted the kiridevs/battery-cycles branch April 24, 2024 09:00
@nebulosa2007
Copy link

Did you try each one independently, or both at once?

I tried them independently and both:

..
"format-alt": "{time} {health} {icon}",
..
"format-alt": "{time} {cycles} {icon}",
..
"format-alt": "{time} {health} {cycles} {icon}",
..

only the hover-tooltip

Ok then, I've updated config to:

"tooltip-format": "{health} {cycles} {capacity}%",

There are no errors, but tooltip gives me only 0 0 95%

Also, I compiled 1.10.2 + fix above. Now it works as format-alt and tooltip-format both, but shows 0 0 only.

I should I do next?

@nebulosa2007
Copy link

nebulosa2007 commented Apr 24, 2024

Btw, tried run inxi to get info about my battery it showed me:

$ sudo inxi -B -v8 | grep Battery -A3
Battery:
  ID-1: BAT1 charge: 26.0 Wh (100.0%) condition: 26.0/47.5 Wh (54.7%)
    volts: 12.4 min: 10.8 model: Sony Corporation type: Li-ion serial: N/A
    status: full

There is no cycles count but shows the condition, so theoretically waybar can show {health}, am I right?

@kiriDevs
Copy link
Contributor Author

kiriDevs commented Apr 24, 2024

There is no cycles count but shows the condition, so theoretically waybar can show {health}, am I right?

Well, theoretically, yes. Practically is a different story.

I do not know how inxi gets the information about the battery. waybar uses the capacity_full and capacity_full_design values from sysfs (/sys/class/power_supply/<batteryName>/) to calculate health. If you don't see any health value, the information is probably not available there. I believe, depending on battery, the values might be named differently? If you could ls the directory of your battery you/we might be able to see if the values are

  • there, but not detected / read correctly for some reason
  • there, but under a different name
  • not available at all in sysfs

@nebulosa2007
Copy link

nebulosa2007 commented Apr 24, 2024

I have this files, not capacity but energy:

$ ls -la /sys/class/power_supply/*/*_full{,_design}
-r--r--r-- 1 root root 4096 Apr 24 15:10 /sys/class/power_supply/BAT1/energy_full
-r--r--r-- 1 root root 4096 Apr 24 15:10 /sys/class/power_supply/BAT1/energy_full_design

$ ls -la /sys/class/power_supply/*/cycle_count
-r--r--r-- 1 root root 4096 Apr 24 15:10 /sys/class/power_supply/BAT1/cycle_count

$ cat /sys/class/power_supply/*/*_full{,_design}
26000000
47520000

$ cat /sys/class/power_supply/*/cycle_count
0

@kiriDevs
Copy link
Contributor Author

Thank you. I will implement checking for energy_full and energy_full_design in the new PR, so that {health} should work. I don't think there's anything we can do about {cycles} though, unfortunately :(

@nebulosa2007
Copy link

nebulosa2007 commented Apr 24, 2024

Compiled again with fixes
image
Now it shows in text-alt and in tooltip too.

As for {cycles}- file exists and contain a number, I think it's a battery issue, not the waybar.

Thanks a lot!

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.

3 participants