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

Fixed battery reporting #536

Closed
wants to merge 2 commits into from

Conversation

overhacked
Copy link

The sed script was having some issues with the way pmset -g batt output looks on macOS 10.12 Sierra and later. I rewrote the sed commands to an awk script, which is a bit more resistant to any future format changes.

@augmentedfourth
Copy link
Contributor

#476 was requested over a year ago and never got pulled. Good luck with this one.

Copy link

@cvigo cvigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 1317 should "return 2 + ..."

@@ -1301,7 +1301,7 @@ case "$LP_OS" in
{
(( LP_ENABLE_BATT )) || return 4
local percent batt_status
eval "$(pmset -g batt | sed -n 's/^ -InternalBattery[^ ]*[ ]\([0-9]*[0-9]\)%; \([^;]*\).*$/percent=\1 batt_status='\'\\2\'/p)"
eval "$(pmset -g batt | awk '/InternalBattery/{gsub(/[^0-9]/,"",$3);gsub(/[;]/,"",$4);printf "percent=%d batt_status=%s", $3, $4}')"
case "$batt_status" in
charged | "")
return 4
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 1317 should "return 2 + ..."
return $(( 2 + ( percent > LP_BATTERY_THRESHOLD ) ))

@overhacked
Copy link
Author

This change is more robust than the change in #476, because it doesn't depend on the specific formatting of numbers and whitespace of the InternalBattery line, only that the third and fourth fields (space-delimited) are percent and status. This is the same on older versions, so this should be a backwards-compatible change.

@augmentedfourth
Copy link
Contributor

If you're going to fix this, the same off-by-one error also exists in line 1295 (on the Linux code for this same battery check).

@Rycieos
Copy link
Collaborator

Rycieos commented May 8, 2018

@overhacked I disagree that this change is more robust.

dolmen has said before that using awk is frowned upon since it is much slower than sed or other alternatives. The change from sed to awk here isn't needed.

Your point that "it doesn't depend on the specific formatting of numbers and whitespace of the InternalBattery line" is false, since your usage of $3 and $4 is directly dependent on whitespace. According to this StackOverflow answer for how awk parses whitespace to separate fields:

any run of spaces and/or tabs and/or newlines is treated as a field separator with leading and trailing runs ignored.

The other PR has solved this in the same way, using the POSIX [[:space:]], which according to the manual:

in the 'C' locale, this is tab, newline, vertical tab, form feed, carriage return, and space.

The real issue is exactly how many fields there are, since that is what broke before. We don't have a record of what pmset output before, but now it looks like this:

Now drawing from 'AC Power'
 -InternalBattery-0 (id=2228323)    100%; charged; 0:00 remaining present: true

Based on what the old sed command was, my guess is that the line used to look like:

-InternalBattery-0    100%; charged; 0:00 remaining present: true

and they added the ID field, which changed what number the percent and status fields were. If they add another field again, your awk script would break. The sed script in the other PR would not break, since it is looking for the percent sign (%).

Of course there are a multitude of things that Apple could change in the output, and they are hard to prepare for. A simple one (that could already be possible) would be for them to output a line for each physical battery:

Now drawing from 'AC Power'
 -InternalBattery-0 (id=2228323)    100%; charged; 0:00 remaining present: true
 -InternalBattery-1 (id=2228324)    100%; charged; 0:00 remaining present: true

The sed script still works:

percent=100 batt_status='charged'
percent=100 batt_status='charged'

But will only show the stats for the last listed battery.

The awk script:

percent=100 batt_status=chargedpercent=100 batt_status=charged

Still kinda works, but will show the percent of the first battery and the status of the last battery.

I would of course love for this feature to become more robust, but that is difficult, and I'm not sure it's worth anyone's time, since we don't know what changes will or could be coming.

@overhacked overhacked closed this Feb 4, 2019
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.

None yet

4 participants