maniacx / Battery-Health-Charging

GNU General Public License v3.0
157 stars 14 forks source link

Force discharge #67

Closed maniacx closed 6 months ago

maniacx commented 8 months ago

Locke

I understand this issue to be for general discussions too, so I am posting it here instead of a new issue.

I am using this extension on my Lenovo ThinkPad L13 Yoga and are happy with it.

In addition to the charge thresholds, my device supports changes to the charge behavior:

$ cat /sys/class/power_supply/BAT0/charge_behaviour [auto] inhibit-charge force-discharge I am using these currently from the console, and would love to see support for that via gnome, just like the charge thresholds. I think it would be best to have it in the same extension, or do you think that should be a separate one?

My use case: I am using the device mostly as a desktop working from home. I have the peripherals on a KVM switch, which also delivers power over the same cable. I normally have the charge thresholds low to extend the battery life. When I anticipate to use it as a notebook/tablet, I charge it up (e.g. for an office day with meetings). When I connect it back to the KVM at home, I often have more charge left than the threshold I'd like to have at home. In this case, I need to drain the battery - either via above charge_behaviour or by not connecting the device to the KVM (I cannot simply unplug the power to the KVM as it powers the KVM and the USB devices).

Alternative use case: Some energy providers are offering a dynamic electricity pricing; in central Europe typically low (sometimes negative) on sunny or windy hours, and high on evenings. Some users may want to manually control when their devices are charging or discharging due to that.

@Locke Interesting. Let continue our discussion here. I can look into adding to the feature to the extension.

Just need some information on

  1. Can I apply charge_behaviour>force-discharge regardless of the charging threshold. or does charging threshold need to be at a certain/default level 95/100% to apply force-discharge
  2. If I set charge_behaviour>force-discharge and reboot the laptop, would it reset to charge_behaviour>auto
  3. If it does, Should I restore it back to force-discharge (I think it doesnt make sense to restore the charge_behavior to force-discharge on reboot)

Is this UI ok? You can just toggle the switch to enable force discharge. Screenshot from 2023-10-30 22-58-35

Locke commented 8 months ago

Thanks for picking this up! I will look into your questions and experiment a bit.

I believe the settings are independent, and force-discharge will drain below the charge thresholds, but want to confirm this and note the exact behavior down.

I will also investigate if there is a minimal charge level that stops the discharging - otherwise we should add a safeguard somehow.

Regarding the UI: I think for this extension simplicity should be favored, so I think it is ok to not consider the third option inhibit-charge and have the UI as suggested.

maniacx commented 8 months ago

Ok. Let me know what you find. Also mention your gnome version,

Locke commented 7 months ago

TLDR: I think we can implement it with the UI you suggested, but should require users to enable this toggle in the settings with a note like "Enable and use with caution. This may drain your battery below safe levels.".

My results and opinions:

I did not discover a minimal charge level which would have stopped the discharging - I manually stopped it at 14% charge level. If there is a failsafe, it is lower than what I am feeling comfortable for the average user.

The interface was added in Linux 5.17 with b55d416d48f5907f66218ae3d878e3bfb69ae4e6 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.17&id=b55d416d48f5907f66218ae3d878e3bfb69ae4e6 I do not see any thresholds / safeguards here.

To your questions:

  1. The charge_behaviour can be modified at any time. It does not matter if external power is connected or not, and it can be changed regardless of the battery level.
  2. In some cases it resets from force-discharge to auto, but not in all:
    1. with external power connected
      • suspend: force-discharge remains
      • hibernate: force-discharge remains
      • shutdown: resets to auto, starts charging once powered down.
        • boot: remains auto and keeps charging
    2. without external power
      • suspend: reset to auto, remains auto when power is connected again and starts charging
      • hibernate: reset to auto, remains auto when power is connected again and starts charging
      • reboot: not tested, assuming it resets to auto
  3. If we are above the charge threshold, I have no strong opinion. I would leave it to the device and as you suggested not add a logic to transition back from auto to force-discharge on reboot. However, when the battery level is below the charge thresholds, we may want to consider if we want to change it from force-discharge to auto, to not risk draining the battery too much.

With these results, we should discuss if and how we want to offer force-discharge with this extensions. I do not want to risk an average user draining their battery too low with this. Some options/alternatives I see:

My opinion: Implement only the first point, i.e. offer the plain functionality with no safeguard, to advanced users, who enable this knowingly. The third point needs effort for the implementation and testing. Although my device resets to auto on reboot or without external power, I see some risks that other devices / firmware versions may behave differently. Also I don't know how safe it would be to rely on a Gnome extension that is running as a user / only once logged in, for critical system functionality.

My System:

If needed, I can easily re-test with Debian testing (Gnome 44) and/or another older kernel (e.g. 6.1 shipped by Debian).

maniacx commented 7 months ago

If needed, I can easily re-test with Debian testing (Gnome 44) and/or another older kernel (e.g. 6.1 shipped by Debian).

Not needed. I just wanted info to build a test version first so that you can test on your system.

Thanks for the insight. Based on your finding, I have implementation the following.

  1. Force-discharge toggle will only available if it is enabled in extension preference.
  2. Activating the toggle will switch force discharge only if battery level > end charging threshold value.
  3. When extension is disabled the it will switch charging_behaviours to auto.
  4. On enabling the extension it will switch back to force-discharge mode as long battery level > end charging threshold value.
  5. This way if use lockscreens the charge_behavior will switch to auto and battey cannot be monitores as the extension is disabled in lockscreen mode.

Test tthis branch and let me know how it goes. https://github.com/maniacx/Battery-Health-Charging/tree/thinkpad-force-discharge

Locke commented 7 months ago

charge_behaviour returns the active option in square brackets like this: [auto] inhibit-charge force-discharge

I have pushed one commit to https://github.com/Locke/Battery-Health-Charging/commits/thinkpad-force-discharge that parses this active option. Please note: I haven't really written JavaScript in years, so I am not sure if I chose the best approach.

With that, the new toggle seems to work :partying_face:

I have not tested this much and will continue testing until Tuesday evening.

maniacx commented 7 months ago

Oh. I didn't realize that it actually output the exact content. Thanks for the correction. I forgot to mention, I added .replace(/\r?\n|\r/g, '') but I think that is not required. (Unless it returns a new line at the end). Take your time to test, no rush.

maniacx commented 7 months ago

Also the switching to charge_behavior to auto wont work when, suspend/hibernate and might completely drain the battery, if user leave in on suspend for a long time OR user wakes-up from suspend/hibernate but doesnt log in. Also I didnt consider the fact the charging-behavior switches to auto when charger is disconnected.

I feel i should just make force-discharge entirely users responsibility, (no automatic switch of force-discharge to auto when charging threshold is reached) than presenting a half working feature. Let me know what you think, after testing everything specially the supend/hibernate. if I should remove it completely or implement it differently. Thanks

Locke commented 7 months ago

I have not tested as much as I wanted, and due to personal circumstances will have reduced capacity.

I first tended to have more automation, but given that there remain scenarios we cannot cover, agree to switch to a full manual approach. Otherwise, a user may be custom to the extension stopping the discharge, but then getting bitten if it doesn't e.g. on the login screen.

To compensate, I suggest to add notifications, which should train the user to pay attention:

I had two errors today, each when going into standby:

Nov 06 11:11:34 l13yg2a gnome-shell[71054]: JS ERROR: Exception in callback for signal: updated: TypeError: this._proxy is undefined
                                            _disableBatteryCapacityMonitoring@/home/locke/.local/share/gnome-shell/extensions/Battery-Health-Charging@maniacx.github.com/devices/Thinkpad.js:377:13
                                            destroy@/home/locke/.local/share/gnome-shell/extensions/Battery-Health-Charging@maniacx.github.com/devices/Thinkpad.js:386:14
                                            destroy@/home/locke/.local/share/gnome-shell/extensions/Battery-Health-Charging@maniacx.github.com/lib/driver.js:176:33
                                            enable/this._sessionId<@/home/locke/.local/share/gnome-shell/extensions/Battery-Health-Charging@maniacx.github.com/extension.js:51:34
                                            _emit@resource:///org/gnome/gjs/modules/core/_signals.js:114:47
                                            _sync@resource:///org/gnome/shell/ui/sessionMode.js:204:14
                                            pushMode@resource:///org/gnome/shell/ui/sessionMode.js:163:14
                                            activate@resource:///org/gnome/shell/ui/screenShield.js:620:34
                                            lock@resource:///org/gnome/shell/ui/screenShield.js:666:14
                                            _prepareForSleep@resource:///org/gnome/shell/ui/screenShield.js:250:22
                                            _emit@resource:///org/gnome/gjs/modules/core/_signals.js:114:47
                                            _prepareForSleep@resource:///org/gnome/shell/misc/loginManager.js:205:14
                                            _emit@resource:///org/gnome/gjs/modules/core/_signals.js:114:47
                                            _convertToNativeSignal@resource:///org/gnome/gjs/modules/core/overrides/Gio.js:152:19

Relevant code:

    _disableBatteryCapacityMonitoring() {
        this._disableForceDischarge();
        if (this._proxy !== null)
            this._proxy.disconnect(this._proxyId); // line 377
        this._proxyId = null;
        this._proxy = null;
    }

I think undefined might not be null?

Nov 06 12:35:51 l13yg2a gnome-shell[71054]: JS ERROR: Exception in callback for signal: updated: TypeError: this._notifier is null
                                            destroy@/home/locke/.local/share/gnome-shell/extensions/Battery-Health-Charging@maniacx.github.com/lib/driver.js:173:9
                                            enable/this._sessionId<@/home/locke/.local/share/gnome-shell/extensions/Battery-Health-Charging@maniacx.github.com/extension.js:51:34
                                            _emit@resource:///org/gnome/gjs/modules/core/_signals.js:114:47
                                            _sync@resource:///org/gnome/shell/ui/sessionMode.js:204:14
                                            pushMode@resource:///org/gnome/shell/ui/sessionMode.js:163:14
                                            activate@resource:///org/gnome/shell/ui/screenShield.js:620:34
                                            lock@resource:///org/gnome/shell/ui/screenShield.js:666:14
                                            _prepareForSleep@resource:///org/gnome/shell/ui/screenShield.js:250:22
                                            _emit@resource:///org/gnome/gjs/modules/core/_signals.js:114:47
                                            _prepareForSleep@resource:///org/gnome/shell/misc/loginManager.js:205:14
                                            _emit@resource:///org/gnome/gjs/modules/core/_signals.js:114:47
                                            _convertToNativeSignal@resource:///org/gnome/gjs/modules/core/overrides/Gio.js:152:19

Relevant code:

    destroy() {
        if (this._thresholdPanel)
            this._thresholdPanel.destroy();
        this._thresholdPanel = null;
        this._notifier.destroy(); // line 173
        this._notifier = null;
        if (this._currentDevice)
            this._currentDevice.destroy();
        this._currentDevice = null;
        this._settings.disconnectObject(this);
        this._settings = null;
    }

Maybe caused by the previous error; maybe should have a check anyway?

maniacx commented 7 months ago

Hmm. Does this happen everytime of only sometime? I think the frequent enable/disable call done while rebasing, propbably triggers this. Suprisingly nobody reported about the notfier

I think undefined might not be null?

You might be correct. For now I think better to use != instead of !==,

        if (this._proxy != null)
            this._proxy.disconnect(this._proxyId);

Same for notiifer as well, add a check/

if(this._notifier != null)
    this._notifier.destroy();
maniacx commented 7 months ago

I had some time today and did some testing with suspend and the extension does complete disable() during suspend thereby setting charging_behavior to auto. Offcourse I do not have a thinkpad, and I have tested by changing sysfs path to a local file on my harddrive. Unfortunately I cannot test for hibernate as I dont have swap partition.

If this works for you too than I will keep the current behavior of automatically switching to auto

So i updated the branch with checks for notifier, along with your commit. https://github.com/maniacx/Battery-Health-Charging/tree/thinkpad-force-discharge So please test this version. take your time and report to me anytime, if all goes well, I will update in gnome extension after a month. Do report if there are any issue.

Note: I have removed the removal of new line while reading charge_behavior. If removal of new line is still required, revert the last commit

Thanks

Locke commented 7 months ago

Status update: I've been running the branch with your commit 7114cc289cd4 since that day or the next, and had only one problem [1], that I think is unrelated to this extension and might be firmware bug.

I had many sleep / hibernate / reboot cycles and no more crashes and did not encounter an unexpected behavior.

I do see sometimes commands being executed twice, but don't mind that, as the action is the same, e.g. when I just resumed from hibernation:

Nov 21 09:50:49 l13yg2a pkexec[14844]: pam_unix(polkit-1:session): session opened for user root(uid=0) by (uid=1000)
Nov 21 09:50:49 l13yg2a pkexec[14844]: locke: Executing command [USER=root] [TTY=unknown] [CWD=/home/locke] [COMMAND=/usr/local/bin/batteryhealthchargingctl-locke CHECKINSTALLATION /home/locke/.local/share/gnome-shell/extensions/Battery-Health-Charging@maniacx.github.com/resources locke]
Nov 21 09:50:49 l13yg2a pkexec[14923]: pam_unix(polkit-1:session): session opened for user root(uid=0) by (uid=1000)
Nov 21 09:50:49 l13yg2a pkexec[14923]: locke: Executing command [USER=root] [TTY=unknown] [CWD=/home/locke] [COMMAND=/usr/local/bin/batteryhealthchargingctl-locke FORCE_DISCHARGE_BAT0 force-discharge]
Nov 21 09:50:49 l13yg2a pkexec[14926]: pam_unix(polkit-1:session): session opened for user root(uid=0) by (uid=1000)
Nov 21 09:50:49 l13yg2a pkexec[14926]: locke: Executing command [USER=root] [TTY=unknown] [CWD=/home/locke] [COMMAND=/usr/local/bin/batteryhealthchargingctl-locke FORCE_DISCHARGE_BAT0 force-discharge]
Nov 21 09:50:49 l13yg2a pkexec[14930]: pam_unix(polkit-1:session): session opened for user root(uid=0) by (uid=1000)
Nov 21 09:50:49 l13yg2a pkexec[14930]: locke: Executing command [USER=root] [TTY=unknown] [CWD=/home/locke] [COMMAND=/usr/local/bin/batteryhealthchargingctl-locke BAT0_START_END 80 75]

Regarding the behavior: as on suspend/hibernation the value is changed to auto, I don't see a problem anymore with the automatic approach.

I see that you committed several changes the last few days to the thinkpad-force-discharge branch. Did you just rebase it on-top of the latest release, or is there any change I should be aware of?

[1] I've had the power connected for several days and did many sleep cycles and maybe a few reboots, not sure. After resuming on once instance, switching from force-discharge to auto changed the reported value, but the system kept discharging. The system continued to discharge after a reboot, even when I manually echo'ed between auto and force-discharge, yet reporting auto. I re-plugged the power cable and the system started charging.

maniacx commented 7 months ago

I see that you committed several changes the last few days to the thinkpad-force-discharge branch. Did you just rebase it on-top of the latest release, or is there any change I should be aware of?

I released a new version on Gnome extension, so the extension manager/ extension app may automatically update extension to version 37. After release, Yes, I did rebase yesterday. Sorry about that, i should have created a new branch, instead of rebasing the current branch. There are no changes in thinkpad ,settings or quicksettings. Updated changes where related to icons, extension preferences (About page), and polkit installation and removal (No longer need to logout/login when polkit is installed, updated or removed)...

[1] I've had the power connected for several days and did many sleep cycles and maybe a few reboots, not sure. After resuming on once instance, switching from force-discharge to auto changed the reported value, but the system kept discharging. The system continued to discharge after a reboot, even when I manually echo'ed between auto and force-discharge, yet reporting auto. I re-plugged the power cable and the system started charging.

Yeah, You are correct, Seems like a firmware bug

I do see sometimes commands being executed twice, but don't mind that, as the action is the same, e.g. when I just resumed from hibernation:

Yes, it in the current code when extension is enabled, force-discharge applies twice, here and here

And this could the fix https://github.com/maniacx/Battery-Health-Charging/commit/06274bc15ddb0c9ba6b5d87b0d0511aae07d5d3e

maniacx commented 7 months ago

Hello @Locke . Do you think the thinkpad-force-discharge branch is good and functional enough? Can I merge it the main branch for release?

Locke commented 6 months ago

Hi @maniacx, sory for the late reply, I somehow procrastinated this too long.

Yes, I have been using 06274bc1 since then and did not notice any more issues.

I see you have released this by now, I have upgraded to version 39 just now and it seems to work fine.

maniacx commented 6 months ago

Great. I will close the issue.