fix: start/stop thresholds not being set because of initial values

[CasperVM]

Background

I had this issue on my thinkpad where sometimes the start/stop thresholds of the battery charge were not being applied and failed with this;

write error: Invalid argument

This can happen when e.g. our start value is higher than the current stop value. In those cases we should lower the stop value to below our start.

What this change does

• Rewrites the battery scripts more cleanly to share more code.
• Adjusts the write to first set 0 and 100 for start/stop respectively.
• Sleeps 100ms before then applying actual settings.

Impact

If writes somehow fail user might end up with incorrect settings, but this was the case before anyway.
The 100ms sleep is to ensure settings are applied by the driver, but this might not be needed?

[AdnanHodzic]

Sounds good, please let me know once it's complete.

Thanks!

[CasperVM]

Sounds good, please let me know once it's complete.

Thanks!

Done 👍

Have a look when you can :)

[AdnanHodzic]

@PurpleWazard since you originally implemented this feature, please let me know if you have any comments.

In meantime, @CasperVM please give me some time to further review and test this.

[PurpleWazard]

@AdnanHodzic @CasperVM. this PR adds a batterydevice class which i like makes the code a bit cleaner. however, thinkpad_acpi and ideapad_acpi were removed for unknown reason and they dont seem to be implemented with the changes.

I had this issue on my thinkpad where sometimes the start/stop thresholds of the battery charge were not being applied and failed with this;

write error: Invalid argument
This can happen when e.g. our start value is higher than the current stop value. In those cases we should lower the stop value to below our start.

which is backwards the start shouldn't be higher then the stop. the reason for the error is becuase the kernel module wont work with those values.

the start value is what % at or under for the battery to start charging. the stop value is that value should charging at. ie. start: 70, stop: 80. the battery will maintain % between 70 and 80.

[CasperVM]

the start value is what % at or under for the battery to start charging. the stop value is that value should charging at. ie. start: 70, stop: 80. the battery will maintain % between 70 and 80.

Let me elaborate a little bit here, because I was probably a bit too unclear with my changes:

First of the class is an abstraction, both the thinkpad_acpi and ideapad_acpi had the exact same behavior, thus, we just use the parent class that implements the 'default'. I could add these back for clarity, but that just feels like unnecessary boilerplate to me.

For the battery percent, yes, the start shouldn't be higher than the stop. That was never in my config, but if you change the config it might become an issue:

• E.g. you initially set start/stop to 50-60 using the config.
• You then decide, hey, I want it to be 70-80 instead.
• The script tries to first set the start value to 70, which the kernel doesn't allow, because your old CURRENT stop value is 60.
• So this is not a config issue, but an error in how we handle config changes.

[CasperVM]

@AdnanHodzic @PurpleWazard

[Copilot]

Pull Request Overview

This PR refactors the battery threshold management scripts to fix a critical issue where start/stop thresholds fail to apply due to conflicting initial values. The solution sets temporary safe values (0/100) before applying the actual configured thresholds, with a 100ms delay to allow driver processing.

• Consolidates common battery management logic into a shared BatteryDevice base class
• Implements a two-step threshold setting process to avoid "Invalid argument" errors
• Refactors Ideapad and Asus battery scripts to use class-based inheritance

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
auto_cpufreq/battery_scripts/shared.py	New shared base class implementing common battery threshold management logic with two-step threshold setting
auto_cpufreq/battery_scripts/ideapad_laptop.py	Refactored to extend BatteryDevice class with Ideapad-specific conservation mode handling
auto_cpufreq/battery_scripts/asus.py	Refactored to extend BatteryDevice class with Asus-specific fallback paths
auto_cpufreq/battery_scripts/battery.py	Updated to instantiate device classes instead of calling standalone functions
auto_cpufreq/battery_scripts/thinkpad.py	Removed - functionality now provided by shared BatteryDevice class
auto_cpufreq/battery_scripts/ideapad_acpi.py	Removed - functionality now provided by shared BatteryDevice class

Comments suppressed due to low confidence (2)

auto_cpufreq/battery_scripts/ideapad_laptop.py:27

• Normal methods should have 'self', rather than 'value', as their first parameter.

    def set_conservation_mode(value):

auto_cpufreq/battery_scripts/asus.py:9

• This class does not call BatteryDevice.init during initialization. (AsusBatteryDevice.init may be missing a call to a base class init)

class AsusBatteryDevice(BatteryDevice):

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[AdnanHodzic]

Code changes (logic) generally make sense to me and from my testing everything worked as it should on ThinkPad X1 Carbon. I ran Copilot review also since there are a lot of changes and refactoring so please take a look at those, as it made some good remarks.

1. Things that could be improved and also addressed is that changes will not automatically picked up from config file:

# battery charging threshold
# reference: https://github.com/AdnanHodzic/auto-cpufreq/#battery-charging-thresholds
enable_thresholds = true
#start_threshold = 20
stop_threshold = 90  

instead I had to restart the auto-cpufreq daemon in order for them to be picked up (or simply auto-cpufreq --remove && auto-cpufreq --install)

2. As I suggested in a comment in code review, I think Battery charging thresholds of docs could be updated.

3. Changes in my case worked as intended, e.g: I purposely charged laptop to 91% and after I set the stop_threshold = 90 it didn't continue to charge. Same case is with charging stopping to charge at 90% if it was below that level.

With that said I think, after mentioned things above are addressed I think we're good to merge this, unless you have any other major concerns @PurpleWazard

[AdnanHodzic]

In meantime, I ran into a major issue, after I disabled stop_threshold and enable_thresholds = true and removed auto-cpufreq in meantime, and reverted everything with changes that are on master branch.

# battery charging threshold
# reference: https://github.com/AdnanHodzic/auto-cpufreq/#battery-charging-thresholds
#enable_thresholds = true
#start_threshold = 20
#stop_threshold = 90 

I realized that my laptop was still not charging above 90% because permanent changes were made to my /sys/class/power_supply/BAT0/charge_control_end_threshold file which still returns 90 ...

After I manually edited it back to 100, it charges past 90%.

[CasperVM]

@AdnanHodzic Fixed some of the issues that the auto-review caught. Should this also perhaps be tested before merge?

Added some clarification in the Readme as well

As for the thresholds not resetting, we could add that to the --remove option when uninstalling auto-cpufreq. But perhaps in another PR?

[CasperVM]

One small oversight still, but unsure what would be the proper way to go around it here:

If we only have e.g. the stop threshold, the start will be at 0. Meaning we wont start charging until the battery is dead.

We should probably keep some sane defaults instead? Or just ignore the config if only one of the values is set?

Edit: made it so we actually validate now, and both start/stop are needed. I don't think it makes sense to apply anything if either one is not set.

[AdnanHodzic]

Fixed some of the issues that the auto-review caught. Should this also perhaps be tested before merge?
Added some clarification in the Readme as well

Thanks, I can also test this if changes are finalized, just please note it might not happen until end of the week.

As for the thresholds not resetting, we could add that to the --remove option when uninstalling auto-cpufreq. But perhaps in another PR?

Option 1: Ideally, changes would be updated immediately as soon as they are written to config file. Some kind of hook could be created to trigger it, but would need to think more about this. As this will be the problem with enabling and disabling the changes.

Option 2: Alternatively, we could update config part (and README) and below # battery charging threshold state that after each change to fields:

#enable_thresholds = true
#start_threshold = 20
#stop_threshold = 90

auto-cpufreq daemon should be restart or simply removed and enabled again (`auto-cpufreq --remove && auto-cpufreq --install). It's not pretty, but it's the simplest thing I can think of now.

As for the thresholds not resetting, we could add that to the --remove option when uninstalling auto-cpufreq. But perhaps in another PR?

If we're going for Option 2, then it can be part of this PR.

One small oversight still, but unsure what would be the proper way to go around it here:

If we only have e.g. the stop threshold, the start will be at 0. Meaning we wont start charging until the battery is dead.

We should probably keep some sane defaults instead? Or just ignore the config if only one of the values is set?

Edit: made it so we actually validate now, and both start/stop are needed. I don't think it makes sense to apply anything if either one is not set.

Not sure if I understand this part, as during my testing I used following config:

# battery charging threshold
# reference: https://github.com/AdnanHodzic/auto-cpufreq/#battery-charging-thresholds
enable_thresholds = true
#start_threshold = 20
stop_threshold = 90

and charging worked fine?

[CasperVM]

I think that option 2 for now would be the best and a proper hook should be implemented later. So we just add more documentation.

It's odd that your charging worked fine, as in the previous commit the default for start = 0, and stop = 100.

As per the kernel docs:
https://docs.kernel.org/admin-guide/laptops/thinkpad-acpi.html#battery-charge-control

charge_control_start_threshold accepts an integer between 0 and 99 (inclusive); this value represents a battery percentage level, below which charging will begin.

and

charge_control_end_threshold accepts an integer between 1 and 100 (inclusive); this value represents a battery percentage level, above which charging will stop.

I'll make one last commit to properly enforce this + add documentation/comments. Again, I don't think it make sense to have only 1 of the values set up. It might just end up in someone having issues and not understanding why. Additionally, it's difficult to determine 'sane' defaults.

[AdnanHodzic]

I think that option 2 for now would be the best and a proper hook should be implemented later. So we just add more documentation.

Agreed.

It's odd that your charging worked fine, as in the previous commit the default for start = 0, and stop = 100.

As per the kernel docs: https://docs.kernel.org/admin-guide/laptops/thinkpad-acpi.html#battery-charge-control

charge_control_start_threshold accepts an integer between 0 and 99 (inclusive); this value represents a battery percentage level, below which charging will begin.

and

charge_control_end_threshold accepts an integer between 1 and 100 (inclusive); this value represents a battery percentage level, above which charging will stop.

I think it worked because by not setting #start_threshold = 20 it left it at 0 by default and didn't change anything?

I'll make one last commit to properly enforce this + add documentation/comments. Again, I don't think it make sense to have only 1 of the values set up. It might just end up in someone having issues and not understanding why. Additionally, it's difficult to determine 'sane' defaults.

Sounds good, better to relate them to avoid unnecessary new issues and questions.

[AdnanHodzic]

@CasperVM Once you're done making all the changes and testing from your side, please let me know so I know when I can do the final review and test. Thanks

P.S: reference from #898 how users setup battery charging thresholds.

sudo modprobe thinkpad_acpi force_load=true
echo 85 > /sys/class/power_supply/BAT0/charge_control_end_threshold

[CasperVM]

@AdnanHodzic I'm happy with the current changes, you can do the final review and test when you have the time