Adding battery monitoring support

[WiserOrb]

So I wrote a week ago about adding battery support. Today I found time to to do that.

It still lacks preferences setting, I've not looked into that yet. It's my first time coding Javascript/gnome extension/ui so the code quality may be lacking.

Changes: I've started from the net.js file. Then import UPowerGlib to query battery values. The graph shows power and battery percentage overlapped. Dark blue shows battery discharging, light blue battery charging. The white line shows battery percentage over time. Added a bolt icon. Added color settings in the schema file.

[fflewddur]

Awesome, glad to see you're working on this! I should have some time to review it later today or tomorrow, thanks for the contribution!

[WiserOrb]

So from the first post you can see the graph is quite blocky. That's because upower updates the values only after 30 seconds. To solve this I directly read voltage and current ecc. readings from the battery files under /sys/class/power_supply. With this metod I get much finer resolution.

[fflewddur]

Looks like your work is coming along well! I noticed you modified metadata.json to rename the extension. You shouldn't need to do this for development, you can just link your forked repo from your extension directory like this: ln -s [path to tophat repository] ~/.local/share/gnome-shell/extensions/tophat@fflewddur.github.io. I also recommend using eslint prior to any commits, or setting up your editor to run it automatically on save, which should help clear up the errors from GitHub Actions. You can install eslint with the command npm install eslint from your repo directory, and then run it with the command ./node_modules/eslint/bin/eslint.js . --ext .js.

I tried running this on my desktop, but it crashes immediately because there are no batteries present. My only laptop is an M1-based Mac, and I'm still working to get a Linux install running reliably in a VM on it, so I haven't been able to test this with a battery-powered device yet.

[WiserOrb]

Ok now it's managing the absence of a battery,now it simply displays N/A. Also added the eslink to my ide, thanks for the tip.

[WiserOrb]

Ok, update. I think the changes are complete. I've added a setting to enable/disable the panel and a color picker for the secondary color. When you have time can you review the commits?

The settings:

[fflewddur]

Very cool, from what I can see, this is looking really nice! Unfortunately I still haven't found a way to test it myself--none of the virtualization software I've tried supports passing power state to the guest on an ARM64 Mac. Now I'm trying kernel mods for my x86 desktop to see if I can emulate a battery device that way, but no luck yet.

While I can't run it, I've looked over the code and have a few questions and suggestions:

1. To design the three existing monitors, I started with the question "What is the most important thing to know about this resource?", with the answer (in my opinion) being "how much of the resource is currently being consumed?". That's what the top bar widgets show, with other useful details kept in the drop-down menu. From this code, it looks like the battery monitor is displaying a textual representation of both current power draw and battery charge remaining. Would one or the other suffice for the top bar? I'd think the most critical information for a battery monitor would be the charge left on the battery itself, which could be more easily visualized as a bar chart, perhaps similar to memory usage. Would the power draw in watts be more appropriate for the drop-down menu instead of the top bar widget? Or if the GNOME top bar already shows a power monitor (I don't know, I've not had a Linux-based laptop for a long time, but I'd be surprised if this isn't available by default), maybe a visualization of just power draw instead of battery charge would make sense?
2. Do we need a secondary color setting in the preferences dialog? In fact, the config schema shows three color settings for this monitor. Would it be possible to redesign it to only use one? If secondary/tertiary colors are needed, could they be computed based on the primary meter color (e.g., by darkening or lightening that color, or picking its complement from the color wheel)? Long term, I'm hoping to use the GNOME accent color for all of the meters, which is why I'm wary of adding more color options.
3. If no battery is detected, the whole monitor should be disabled and hidden. I don't think end users should need to manually disable it to get that top bar space back when there is no battery present. As a short term fix, hiding the power monitor by default and letting users manually enable it would also work.
4. The metadata.json file still needs to be reverted to the unmodified version; it's fine to change this for local development, but the changes shouldn't be included in the PR.
5. If I'm unable to figure out how to test this using my own hardware, we could try merging it but keeping it disabled by default, so that end users need to manually turn it on. If we do that for a release cycle or two and don't receive any bug reports from people, I'll feel much better about enabling it for everyone. Would that approach work for you?

Again, thanks for your work on this, I think it'll be a valuable addition to TopHat after some refinement!

[WiserOrb]

1. Yes gnome already ships with a battery level. If we drop the battery level monitor, we could also drop the relative color.
2. I use the secondary color to distinguish charging and discharging states. I think is necessary to show ti information. If only one color is necessary how this two states can be distinguished. I tried this two approaches, but neither are satisfying.
3. I agree. It shouldn't be too difficult to do.
4. Ops, fixing this now.

5. Completely fine with that.

   Also it would be nice to add the scale on the graph, however I'll wait to see how you're going to do it in the other monitors.

Really thanks for the guidance.

[fflewddur]

Since GNOME already includes a battery level indicator, I think it's better to avoid duplicating the information that it displays (at least in the top bar). This would also solve another problem: without a legend, it's not clear what the two chart colors represent. Simplifying the menu to only show power usage would solve that issue, too. Alternatively, you could use two charts, one showing power use over time and one showing battery charge. E.g., something like:

         Power usage
Power consumption:                   -5W
[Chart of power consumption over time]

Battery level:                       91%
Estimated time remaining:        8 hours
[Chart of battery level over time]

Also, I agree, it'd be very helpful to show a scale for each graph. I'll file an issue for that so I don't forget. Thanks!

[zenitraM]

Hey, been testing this PR locally, but I had to make a couple tweaks as on my laptop the power variables are represented differently:

❯ find /sys/class/power_supply/BAT1/
/sys/class/power_supply/BAT1/
/sys/class/power_supply/BAT1/uevent
/sys/class/power_supply/BAT1/serial_number
/sys/class/power_supply/BAT1/technology
/sys/class/power_supply/BAT1/power_now
/sys/class/power_supply/BAT1/present
/sys/class/power_supply/BAT1/power
/sys/class/power_supply/BAT1/power/runtime_active_time
/sys/class/power_supply/BAT1/power/runtime_active_kids
/sys/class/power_supply/BAT1/power/runtime_usage
/sys/class/power_supply/BAT1/power/runtime_status
/sys/class/power_supply/BAT1/power/autosuspend_delay_ms
/sys/class/power_supply/BAT1/power/async
/sys/class/power_supply/BAT1/power/runtime_suspended_time
/sys/class/power_supply/BAT1/power/runtime_enabled
/sys/class/power_supply/BAT1/power/control
/sys/class/power_supply/BAT1/hwmon4
/sys/class/power_supply/BAT1/hwmon4/uevent
/sys/class/power_supply/BAT1/hwmon4/in0_input
/sys/class/power_supply/BAT1/hwmon4/power
/sys/class/power_supply/BAT1/hwmon4/power/runtime_active_time
/sys/class/power_supply/BAT1/hwmon4/power/runtime_active_kids
/sys/class/power_supply/BAT1/hwmon4/power/runtime_usage
/sys/class/power_supply/BAT1/hwmon4/power/runtime_status
/sys/class/power_supply/BAT1/hwmon4/power/autosuspend_delay_ms
/sys/class/power_supply/BAT1/hwmon4/power/async
/sys/class/power_supply/BAT1/hwmon4/power/runtime_suspended_time
/sys/class/power_supply/BAT1/hwmon4/power/runtime_enabled
/sys/class/power_supply/BAT1/hwmon4/power/control
/sys/class/power_supply/BAT1/hwmon4/device
/sys/class/power_supply/BAT1/hwmon4/subsystem
/sys/class/power_supply/BAT1/hwmon4/name
/sys/class/power_supply/BAT1/manufacturer
/sys/class/power_supply/BAT1/device
/sys/class/power_supply/BAT1/energy_now
/sys/class/power_supply/BAT1/type
/sys/class/power_supply/BAT1/capacity
/sys/class/power_supply/BAT1/cycle_count
/sys/class/power_supply/BAT1/voltage_now
/sys/class/power_supply/BAT1/subsystem
/sys/class/power_supply/BAT1/status
/sys/class/power_supply/BAT1/alarm
/sys/class/power_supply/BAT1/model_name
/sys/class/power_supply/BAT1/voltage_min_design
/sys/class/power_supply/BAT1/capacity_level
/sys/class/power_supply/BAT1/energy_full_design
/sys/class/power_supply/BAT1/energy_full

so instead of having current_now I have power_now which I'm fairly sure is measured on Watts (so no need to divide current/voltage) and instead of charge_now/full I have energy_now/full (not sure about the units but dividing them still gives out the expected % value).

Not sure about what is the logic behind which values get exposed (maybe dependent on what the specific charge controller exposes?) but possibly the extension may need some additional logic to support those different paths based on what is available. Here's how I made it work:

diff --git a/lib/battery.js b/lib/battery.js
index af42f9a..a58025b 100644
--- a/lib/battery.js
+++ b/lib/battery.js
@@ -118,8 +118,9 @@ var PowerMonitor = GObject.registerClass({
             const basePath = `/sys/class/power_supply/${path}`;
             this.currentPath = `${basePath}/current_now`;
             this.voltagePath = `${basePath}/voltage_now`;
-            this.chargePath = `${basePath}/charge_now`;
-            this.chargeFullPath = `${basePath}/charge_full`;
+            this.powerPath = `${basePath}/power_now`;
+            this.chargePath = `${basePath}/energy_now`;
+            this.chargeFullPath = `${basePath}/energy_full`;
             this.statusPath = `${basePath}/status`;
         }
         this.history = new Array(0);
@@ -260,6 +261,7 @@ var PowerMonitor = GObject.registerClass({
     getPowerValues() {
         const current = this.parseValue(this.currentPath) * 1e-6;
         const voltage = this.parseValue(this.voltagePath) * 1e-6;
+        const power = this.parseValue(this.powerPath) * 1e-6;
         const charge = this.parseValue(this.chargePath);
         const chargeFull = Math.max(this.parseValue(this.chargeFullPath), 1); // avoid division by zero
         const status = this.parseStatus(this.statusPath).replace(/[\n\r]/g, '').toUpperCase();
@@ -267,7 +269,8 @@ var PowerMonitor = GObject.registerClass({

         return new PowerUse(
-            voltage * current,
+            power,
             charge / chargeFull * 100,
             state
         );

[WiserOrb]

I did not foresee this.

Looking how Upower does it, they subtract the previous energy level to the current, here.

To compute the energy they try to use charge, and if not present the percentage.

if (values->units == UP_BATTERY_UNIT_CHARGE) {
        values->units = UP_BATTERY_UNIT_ENERGY;
        values->energy.cur = up_device_battery_charge_to_energy (self, values->charge.cur);
        values->energy.rate = up_device_battery_charge_to_energy (self, values->charge.rate);
    }

    /* QUIRK: Discard weird measurements (like a 300W power usage). */
    if (values->energy.rate > 300)
        values->energy.rate = 0;

    /* Infer current energy if unknown */
    if (values->energy.cur < 0.01 && values->percentage > 0)
        values->energy.cur = priv->energy_full * values->percentage / 100.0;

    /* QUIRK: Increase energy_full if energy.cur is higher */
    if (values->energy.cur > priv->energy_full) {
        priv->energy_full = values->energy.cur;
        g_object_set (self,
                      /* How healthy the battery is (clamp to 100% if it can hold more charge than expected) */
                      "capacity", MIN (priv->energy_full / priv->energy_design * 100.0, 100),
                      "energy-full", priv->energy_full,
                      NULL);
    }

Then to go from charge to energy they use the design_voltage, witch they do admit is not great, and I agree.

static gdouble
up_device_battery_charge_to_energy (UpDeviceBattery *self, gdouble charge)
{
    UpDeviceBatteryPrivate *priv = up_device_battery_get_instance_private (self);

    /* We want to work with energy internally.
     * Note that this is a pretty bad way of estimating the energy,
     * we just assume that the voltage is always the same, which is
     * obviously not true. The voltage depends on at least:
     *  - output current
     *  - temperature
     *  - charge
     * The easiest way to improve this would likely be "machine learning",
     * i.e. statistics through which we can calculate the actual
     * performance based on the factors we have.
     */
    return priv->voltage_design * charge;
}

To be as robust as possible, we should use this as a fallback, since is what they do.

[WiserOrb]

Also do your system as a BAT0? Is it rechargeable but not the main one? Because if yes we should add a menu to select the correct battery if multiple are present. I prototype a solution, but I didn't submit the changes to not over-engineer the extension.

[WiserOrb]

Not sure about what is the logic behind which values get exposed (maybe dependent on what the specific charge controller exposes?)

From the comments they do in the upower source it seems so.

[zenitraM]

Looking how Upower does it, they subtract the previous energy level to the current, here. To compute the energy they try to use charge, and if not present the percentage.

They seem to only do this if they find the energy rate (I assume coming from power_now) to be unreliable: https://gitlab.freedesktop.org/upower/upower/-/blob/master/src/up-device-battery.c#L335-344

Not sure if it may be possible to just read the energy-rate value using UPower itself instead of reimplementing this logic in the extension though?

Also do your system as a BAT0? Is it rechargeable but not the main one? Because if yes we should add a menu to select the correct battery if multiple are present. I prototype a solution, but I didn't submit the changes to not over-engineer the extension.

I only have a single battery, it just appears as BAT1 instead of BAT0.

[zenitraM]

Nevermind, just reread from your previous comments that you aren't reading those values from upower directly because of the frequency - i agree on that :sweat_smile:

I think a fallback route at least for power may be to try to read power_now first (seems it should be the most accurate when it is present), if not present, fall back to voltage*charge, and, if still not present or some option is chosen, fall back to non-fine-grained UPower?

[WiserOrb]

Before making the final design, I would like to collect a bit more information on what kind of configuration we could find in the wild. I couldn't find any database or discussion. Maybe we could ask people on r/Gnome to post what they have inside */BAT0

[WiserOrb]

Before that I did a search through GitHub. There are some useful information: linux driver rust-battery lxpanel plugin