Page MenuHomeFreeBSD

Battery capacity remaining percent in top(1)
ClosedPublic

Authored by antranigv_freebsd.am on Dec 19 2019, 12:09 AM.

Details

Reviewers
philip
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rS355978: top: display battery capacity remaining

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Owners added a reviewer: Restricted Owners Package.Dec 19 2019, 12:09 AM
philip requested changes to this revision.Dec 19 2019, 8:41 AM
philip added a subscriber: philip.

Except for some very minor style issues, this looks like a good change! Thanks for the submission.

usr.bin/top/display.c
1328 ↗(On Diff #65799)

Follow the style of the rest of the file please.

usr.bin/top/display.h
27 ↗(On Diff #65799)

This should be in alphabetical order like the other variables (style).

This revision now requires changes to proceed.Dec 19 2019, 8:41 AM
antranigv_freebsd.am marked an inline comment as done.

Update display.c to follow style guidelines.

This looks generally good, one minor comment and I think it's ready to go.

usr.bin/top/machine.c
384 ↗(On Diff #65853)

It would be nice to have a quick comment here saying that only batteries reported via ACPI are counted. Anybody still running APM won't have these numbers, but that's totally fine: APM is likely to be deleted soon anyway as laptops that require you use APM aren't really viable anymore as laptops... I'm also unsure how the 'pinebook' reports this information, but that too is a niche thing that's kinda weird still.... So just a quick comment will suffice.

  1. I know someone in the city who has a Pinebook, should I try running on it? 2) So now I should just set a comment that says this is ACPI only? 😄
usr.bin/top/machine.c
384 ↗(On Diff #65853)

Should I try running this on a Pinebook? I know someone in the city who has one.

  1. I know someone in the city who has a Pinebook, should I try running on it? 2) So now I should just set a comment that says this is ACPI only? 😄

1: sure, why not. I wouldn't hold this up for that testing, it's more of a good to know read.
2: Yup.

Note in the comments that only ACPI reported batteries are supported

This looks good now. I'll commit it.

This revision is now accepted and ready to land.Dec 21 2019, 3:28 AM
This revision was automatically updated to reflect the committed changes.