Add Support for Battery Field

[srithon]

I added a "Battery" field to the output that mirrors that of neofetch. I wrote a shell script to find the directory of the computer's battery by finding the first entry in /sys/class/power_supply that starts with "bat." I looked online and saw that some people's battery directories are named "battery" and not "BAT," so this was done to make the battery functionality compatible with those systems as well. The script is invoked at compile time in the Makefile, so the user does not have to run the script themselves. The directory is echo'd into a separate config header which defines "BATTERY_DIRECTORY" as the path to the battery. This is then used in the source file.

The output differs from neofetch in that the battery field in the output is labelled "Battery" regardless of the name of the actual battery. This could easily be changed by using sed to replace "Battery" in config.h with the name of the battery.

[ss7m]

Do you only malloc BUF_SIZE / 2 because you know that the returned string will never be that large?

[ss7m]

Is this ready to pull right now?

[Titaniumtown]

It works for me

[srithon]

Is this ready to pull right now?

Yes, it's ready to pull

[srithon]

Do you only malloc BUF_SIZE / 2 because you know that the returned string will never be that large?

Yea, based on the research I've done, the longest string that it can possibly be is "100% [Discharging]", which is 19 bytes counting the null character. I kept the size at BUF_SIZE / 2 just to be safe, but I believe that it can be reduced to 19 without any risk.

[allisio]

Just a couple nitpicks:

• I don't think we need to introduce new files for this. Getting the battery directory in the Makefile is great, but we can contain it to there and just pass it to the compilation with a -D flag.
• ls -1 is implied if the output isn't going to a terminal (read: being piped into some other process)
• The fastidious buffer-tracking and memory micro-management does seem to get the right results, but a scanf/sprintf approach really seems the better way to go.

[srithon]

* I don't think we need to introduce new files for this. Getting the battery directory in the `Makefile` is great, but we can contain it to there and just pass it to the compilation with a `-D` flag.

Oh cool, I didn't know about that! I just added a commit that got rid of the extra header file, but I kept the shell script as is because I felt that it was too long for the Makefile. I moved it into a config_scripts folder for cleanliness, so that it can potentially be grouped with other compile-time scripts in the future.

Do you think the script should be inlined?

* `ls -1` is implied if the output isn't going to a terminal (read: being piped into some other process)

I also did not know about this! I added a commit getting rid of the "-1" argument.

* The fastidious buffer-tracking and memory micro-management _does_ seem to get the right results, but [a `scanf/sprintf` approach](https://gist.github.com/allisio/1e850b93c81150124c2634716fbc4815) really seems the better way to go.

I had a feeling I was overcomplicating this.

One of the reasons for why I did all of the micro-management was because when I used time to see how long the executable took compared to the original, it was averaging around 30 ms rather than the ~10 ms for the original project. After seeing this, I was really confused because my code was already fairly lean. I optimized everything I could see but the time stayed the same. In hindsight, time isn't a profiler, so maybe my optimizations were a little misguided. However, I feel that if this version of the function takes 20 ms already, it may be necessary to keep all of the optimizations we can make.

I'd look to hear your thoughts on this, though.

Do you think that the performance bottleneck that time showed is accurate?

Regardless, I will revert the optimization commits and use the scanf/sprintf approach that you detailed, as it is significantly cleaner than my approach.

Thanks for the feedback!

[allisio]

I moved it into a config_scripts folder for cleanliness, so that it can potentially be grouped with other compile-time scripts in the future.

I think this makes a lot of sense. :+1: No need to inline it, seeing as how it'd be a bit much to be interested in the performance of the Makefile.

You weren't necessarily over-complicating, and I figured you had good intentions, but—as you point out—time has the potential to lead one astray where performance is concerned. Cases in point:

$ thyme 1000 ./paleofetch_master
     42 0.003
    724 0.004
    222 0.005
      7 0.006
      5 0.007
$ thyme 1000 ./paleofetch_srithon
     23 0.003
    721 0.004
    237 0.005
     12 0.006
      5 0.007
      1 0.008
      1 0.009
$ thyme 1000 ./paleofetch_simpler_battery
      7 0.003
    728 0.004
    251 0.005
     12 0.006
      1 0.007
      1 0.008

thyme is just a little utility I use to run time n times in the hopes of not being misguided by outliers, and I think it shows a pretty clear picture here: we don't get quite as many runs at the lower end of the curve, but we still get some, and in any case we're certainly within tolerance whichever method we use for this feature. It's hard to say what caused such a comparatively massive spike during your testing, but I'm sure the tripling in runtime you observed was anomalous.

Your code certainly does the job and doesn't incur any noteworthy performance penalty (at least on my machine), so it could totally be merged as-is, but the "cleaner" approach would make it a little easier to modify in future. We might, for instance, want to use /BAT*/energy_{now,full} in order to provide a more granular reading. There's also the matter of devices with multiple batteries, but I'll pretend I've never heard of such a thing. 😅

In any event, thanks for taking the initiative in seeing this added to paleofetch!

[srithon]

You weren't necessarily over-complicating, and I figured you had good intentions, but—as you point out—time has the potential to lead one astray where performance is concerned. Cases in point:

$ thyme 1000 ./paleofetch_master
     42 0.003
    724 0.004
    222 0.005
      7 0.006
      5 0.007
$ thyme 1000 ./paleofetch_srithon
     23 0.003
    721 0.004
    237 0.005
     12 0.006
      5 0.007
      1 0.008
      1 0.009
$ thyme 1000 ./paleofetch_simpler_battery
      7 0.003
    728 0.004
    251 0.005
     12 0.006
      1 0.007
      1 0.008

thyme is just a little utility I use to run time n times in the hopes of not being misguided by outliers, and I think it shows a pretty clear picture here: we don't get quite as many runs at the lower end of the curve, but we still get some, and in any case we're certainly within tolerance whichever method we use for this feature. It's hard to say what caused such a comparatively massive spike during your testing, but I'm sure the tripling in runtime you observed was anomalous.

Thanks for your explanation! I will definitely install thyme for future benchmarks.

Your code certainly does the job and doesn't incur any noteworthy performance penalty (at least on my machine), so it could totally be merged as-is, but the "cleaner" approach would make it a little easier to modify in future.

That makes a lot of sense. I'll make sure to keep that in mind in the future.

We might, for instance, want to use /BAT*/energy_{now,full} in order to provide a more granular reading.

That's an interesting idea! Maybe we could have this as a config option?

There's also the matter of devices with multiple batteries, but I'll pretend I've never heard of such a thing. sweat_smile

Hmm, I hadn't considered that. I have some ideas on how we could potentially make that work, but they may not be optimal. I'll open up an issue for it after these changes are pulled and we can talk about it there.

In any event, thanks for taking the initiative in seeing this added to paleofetch

Once again, thanks for the feedback!

[ss7m]

This looks to have been a very productive discussion- I'll go ahead and pull this.