shdown / luastatus

universal status bar content generator
GNU General Public License v3.0
295 stars 12 forks source link

Reduce the number of options of the `battery-linux` plugin #54

Closed cdlscpmv closed 5 years ago

cdlscpmv commented 5 years ago

The plugin exposed too much of its implementation details to the user. Some of the options were very specific to the platform on which the plugin is running, but which could be dealt with without user intervention.

The changes are as follows:

  1. The underlying implementation is backed by udev now, so that plugging/unplugging events are displayed immediately.

  2. The property naming schemas are removed. We can determine the units when the battery becomes available (thanks to udev).

  3. No global variables used.

  4. Add the consumption field to the returned table. The values is always in Watts irrespective of the units used in the underlying driver. This is what i3status does. And I think it is a good way to abstract away such minor platform differences.

  5. The file capacity is not parsed. Instead the ratio of energy_now to energy_full is used. I also plan to add an option to calculate the capacity relative to energy_full_design (in which case the capacity file is useless anyway).

  6. All necessary files are unconditionally read. And the estimated remaining time is always calculated (if it can be). The overhead of computing it is so small that it doesn't make a difference.

  7. The option period is added and the timer_opts is removed.

  8. Some obsolete options and functions are removed.

I did some benchmarks. The new version is about as fast as the old version with no_uevent set, which is 50% slower than the uevent version. The time for computing battery stats is around 80 microseconds on my rather average machine. Given that the default polling period is set to 2 seconds, this results in occupying 0.004% of the whole CPU time (on one core).

cdlscpmv commented 5 years ago

There is a number of places where the plugin is used in the codebase. I decided not to touch those until these changes are accepted.

shdown commented 5 years ago

OK, overall, I like it. I am sorry for not responding in #52 in time.

By the way, according to https://lwn.net/Articles/646617/, on modern Linux, uevent is always present if the device directory is present. i3status also relies on the uevent file. So it’s probably OK do to the same in luastatus.

This PR will be merged after:

cdlscpmv commented 5 years ago

Done.

shdown commented 5 years ago

Merged, thanks!