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

[AdnanHodzic]

What does "(THIS SHOULD BE LEFT AS-IS IN MOST CASES)" mean in this case? Left uncommitted, or left as is it's in example, which is true?

If that's the case I would rephrase it like:

(THIS SHOULD BE LEFT AS-IS IN MOST CASES, e.g: true)

[AdnanHodzic]

Let's also make sure these changes are reflected everywhere else where it's written, e.g: auto-cpufreq.conf-example.nix

[AdnanHodzic]

Finally, let's make the example more how it was before

current:

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

Your proposed changes:

# experimental

# Add battery charging threshold (currently only available to Lenovo)
# checkout README.md for more info

# enable thresholds true or false
#enable_thresholds = true
#
# whether to check if thresholds are valid (true or false). (THIS SHOULD BE LEFT AS-IS IN MOST CASES)
#check_thresholds = true                                                   
#
# start threshold (0 is off ) can be 0-99
#start_threshold = 0
#
# stop threshold (100 is off) can be 1-100
#stop_threshold = 100

How I think it should be as it looks cleaner

# Add battery charging threshold 
# reference: https://github.com/AdnanHodzic/auto-cpufreq/#battery-charging-thresholds

# enable thresholds true or false
#enable_thresholds = true

# whether to check if thresholds are valid (true or false). Recommended in most cases "true"
#check_thresholds = true     

# start threshold (0 is off ) can be 0-99
#start_threshold = 0

# stop threshold (100 is off) can be 1-100
#stop_threshold = 100
`

Btw, regarding:
>  Add battery charging threshold 

You proposed:
> # Add battery charging threshold  (currently only available to Lenovo) 

do we want to limit it only to Lenovo, because there is other models, I mean we have asus.py ... it might be easier to leave that part so it's all listed [on Threshold part of readme.](https://github.com/AdnanHodzic/auto-cpufreq/#battery-charging-thresholds)

[CasperVM]

Left 'as-is' means as-is, so the example value. Generally speaking you wouldn't want to skip the check for invalid start/stop thresholds. I only added this option because of the exception with fixed thresholds. I didn't want to break existing functionality.

Would be good to find someone to test it however..

[AdnanHodzic]

I'm not sure where you got this information, but I never heard about this before. Have you also verified this works?

[CasperVM]

I have not, I do not have these devices. This was added to the readme quite a while ago: 7862a3f

[AdnanHodzic]

Thanks,

I added some more comments in documentation, and let's please get rid of auto-cpufreq.conf example contents under "Example config file contents" because user should refer to auto-cpufreq.conf file instead.

and let's make "Example config file contents" a link to auto-cpufreq.conf-example file

Also during my testing, setting following values in /etc/auto-cpufreq.conf file:

# enable thresholds true or false
enable_thresholds = true

# whether to check if thresholds are valid (true or false). (THIS SHOULD BE LEFT AS-IS IN MOST CASES)
check_thresholds = true

# start threshold (0 is off ) can be 0-99
start_threshold = 80
                                                                                                                                                       
# stop threshold (100 is off) can be 1-100
stop_threshold = 90

Even after as instructed in readme, systemctl restart auto-cpufreq.service and when that didn't change anything auto-cpufreq --remove && auto-cpufreq --install made no difference and values weren't picked up from auto-cpufreq.conf file and these values remained the same as set by system:

cat /sys/class/power_supply/BAT0/charge_stop_threshold
100
cat /sys/class/power_supply/BAT0/charge_start_threshold
0 

So not sure what happened but this feature is not working as it should anymore on my side.

[CasperVM]

I added some debug prints for further testing/investigation. I cannot test this myself, so help would be appreciated. I added some comments for clarification, I'll update the branch more once people can test/give some more feedback as to why this doesn't work in it's current state.

edit: I used to have a Thinkpad, where I experienced some issues that I'm trying to fix in this branch. But since a short while ago I do not have that anymore, so testing is difficult.

[CasperVM]

I have not, I do not have these devices. This was added to the readme quite a while ago: 7862a3f

[CasperVM]

Left 'as-is' means as-is, so the example value. Generally speaking you wouldn't want to skip the check for invalid start/stop thresholds. I only added this option because of the exception with fixed thresholds. I didn't want to break existing functionality.

Would be good to find someone to test it however..

[AdnanHodzic]

1. I started testing these changes again and I came across another weird issue.

Which is, if in /etc/auto-cpufreq.conf I leave these values commented, e.g:

# battery charging threshold
# reference: https://github.com/AdnanHodzic/auto-cpufreq/#battery-charging-thresholds


# enable thresholds true or false
#enable_thresholds = true                                                                                                                                                                                             

# whether to check if thresholds are valid (true or false). (THIS SHOULD BE LEFT AS-IS IN MOST CASES)
#check_thresholds = true

# start threshold (0 is off ) can be 0-99
#start_threshold = 0 

# stop threshold (100 is off) can be 1-100
#stop_threshold = 80

install auto-cpufreq using auto-cpufreq-installer from changes with this PR, I'll install auto-cpufreq daemon and --stats will work fine.

But, if I remove auto-cpufreq using auto-cpufreq-installer, install it again using the same, enable threshold values in: /etc/auto-cpufreq.conf`

# battery charging threshold
# reference: https://github.com/AdnanHodzic/auto-cpufreq/#battery-charging-thresholds


# enable thresholds true or false
enable_thresholds = true                                                                                                                                                                                             

# whether to check if thresholds are valid (true or false). (THIS SHOULD BE LEFT AS-IS IN MOST CASES)
check_thresholds = true

# start threshold (0 is off ) can be 0-99
start_threshold = 0 

# stop threshold (100 is off) can be 1-100
stop_threshold = 80

and install auto-cpufreq daemon, running stats will return the following:

sudo auto-cpufreq --stats              

------------------------ auto-cpufreq not running ------------------------------

ERROR: auto-cpufreq is not running in daemon mode.

Make sure to run "sudo auto-cpufreq --install" first

-------------------------------------------------------------------------------

2. Besides that new issue, as I said in my latest comment, these values no longer work for me unlike with initial commit. So I can't even test anything anymore.

3. Furthermore, I also noticed that my laptop is no longer charging when connected to my external monitor via usb-c (which was always the case before). Not sure if this is related to testing these changes, but with all this said as much as I appreciate the effort, I cannot merge this PR and its changes in this state.

Update: reboot fixed the issue and my laptop is charging again when connected to external monitor with changes on master branch.

Especially since I wan't to make a big new release in next couple of weeks.

If anyone would like to further test these changes or new ones are made, and is in working state again I'm also willing to test it again. But right now I'm facing these 3 major issues.

[be-west]

Hi @AdnanHodzic,

I would be willing to contribute to this PR. I own a ThinkPad, so I could run a few tests.
After taking a look at the current code, I identified a couple of potential issues:

1. The main problem you're having, as mentioned in your first point, seems to be a type error while parsing the config, which makes the daemon crash.

2. The --monitor option enables the configured battery thresholds. I think this should be fixed, considering the README mentions No changes are made to the system.

3. Instead of the charge_ {start/end}_threshold, maybe the charge_control_{start,end}_threshold VFS files should be used if they exist. I had a very quick look at how TLP does this, and they use the former only if the latter wasn't found. Plus, I could only find official documentation about the charge_control_* attributes.

[CasperVM]

Hi @AdnanHodzic,

I would be willing to contribute to this PR. I own a ThinkPad, so I could run a few tests. After taking a look at the current code, I identified a couple of potential issues:

1. The main problem you're having, as mentioned in your first point, seems to be a type error while parsing the config, which makes the daemon crash.

2. The `--monitor` option enables the configured battery thresholds. I think this should be fixed, considering the README mentions `No changes are made to the system.`

3. Instead of the `charge_ {start/end}_threshold`, maybe the `charge_control_{start,end}_threshold` VFS files should be used if they exist. I had a very quick look at how TLP does this, and they use the former only if the latter wasn't found. Plus, I could only find official documentation about the `charge_control_*` attributes.

@be-west I wouldn't mind to collaborate. I'd be completely fine if you finish it up, as I don't have the hardware to test this change.

[AdnanHodzic]

@be-west and @CasperVM please give it a try, especially since @CasperVM is already an existing contributor, if you need to sync you can use auto-cpufreq Discord community.

As always, you will be credited for your work as part of future release :)

[be-west]

@CasperVM
Hi. I've made some changes, and now the thresholds are working as expected on my ThinkPad.
Main changes:

• The --monitor flag no longer makes changes to the thresholds
• The type errors are fixed.
• Moved some code around for easier test writing (I wrote some unit tests for myself)
• The charge_control_{start,end}_threshold files are now preferred over the charge_{start,stop}_threshold files

Should I commit this to your branch, or should I create my own fork?

Edit: you can also reach me on the Discord: bene_w

[AdnanHodzic]

Have you checked if your ThinkPad charges when you set your start value above your current charge level?

Setting:

enable_thresholds = true
start_threshold = 80
stop_threshold = 100                                                                                               

While my battery was on 80%, started charging it to 100%

After that I even tried setting it to:

enable_thresholds = true
start_threshold = 80
stop_threshold = 85

While battery was on 82% and it still kept charging.

Then I let it discharge to 75% and set:

enable_thresholds = true
start_threshold = 20
stop_threshold = 80

and it was charging, but then I let it discharge to 70% with same settings, and I had same problem as above where it wouldn't charge.

This is where I figured what the issue was, in background it was still set to use GNOME Battery charging and "Preserve Battery Health" was last selected:

Which if clicked (with or without auto-cpufreq daemon running), it'll set values to:

cat /sys/class/power_supply/BAT0/charge_control_end_threshold
80
cat /sys/class/power_supply/BAT0/charge_control_start_threshold
75

So, although with auto-cpufreq.conf these values were overwritten to 80/20 respectively, it seems like it still had above values cached somewhere and they were overriding what was defined with auto-cpufreq.

So once I switch to "Maximize Charge" ...

it started charging because it also reset:

cat /sys/class/power_supply/BAT0/charge_control_end_threshold
0
cat /sys/class/power_supply/BAT0/charge_control_start_threshold
100

although, these were still set to:

enable_thresholds = true
start_threshold = 20
stop_threshold = 80

in auto-cpufreq.conf

Switching back to "Preserve Battery Health" also reset them again to 75/80:

Running sudo systemctl restart auto-cpufreq.service set to pick up values defined in auto-cpufreq.conf and everything was working as it's supposed to:

If battery was charged above defined threshold, it would not be further charged which is good:

So, in a nutshell code changes in this PR are working as expected, only (major) problem is that GNOME Battery charging is interfering, and should be disabled completely if battery threshold values from auto-cpufreq.conf are about to picked up properly.

Same happened after GNOME Power profiles were introduced, they had to be disabled by auto-cpufreq otherwise they would conflict with each other.

[be-west]

Ah great, you found the issue.
Do you want an automatic solution for the GNOME battery charging interference?
Is this a standalone systemd service that can be disabled like the power profiles daemon?

[AdnanHodzic]

Do you want an automatic solution for the GNOME battery charging interference?

What exactly do you by "automatic solution for the GNOME battery charging interference?"

Is this a standalone systemd service that can be disabled like the power profiles daemon?

Not sure, quick search is not as conclusive, but ultimately you/we would have to look into a way to have it disabled.

[be-west]

Yeah, that's what I meant, disabling it like it is done with the power profiles daemon.
But I'm wondering why someone would try to use both features at once.
I don't have a GNOME environment, so I wouldn't mind someone posting the steps to disable it.

[AdnanHodzic]

But I'm wondering why someone would try to use both features at once.

I see value in someone wanting to use GNOME Battery Charging feature along with auto-cpufreq (in case you're not using battery thresholds). So ideally this feature would be disabled only if auto-cpufreq battery threshold is used, but ...

I don't have a GNOME environment, so I wouldn't mind someone posting the steps to disable it.

There's no way to disable it like e.g GNOME Power Profiles, it's not a systemd service or anything like that. It works on setting and picking up the same values in:

/sys/class/power_supply/BAT0/charge_control_start_threshold
/sys/class/power_supply/BAT0/charge_control_end_threshold

There could be an aggressive way to disable it with:

gsettings set org.gnome.settings-daemon.plugins.power active false

But then, this would This prevents GNOME from:

• showing Battery Charging UI
• writing thresholds
• interfering

with side effects we don't want:

• No lid handling
• No brightness key handling
• No low battery notifications

Hence, what I propose it that we simple leave it there, and add to README something like "Please note: if you're using auto-cpufreq battery threshold features, ignore settings defined in "GNOME Battery Charging" as they will be overwritten with values defined in auto-cpufreq.conf".

In addition to that, I would add additional check, once start/stop thresholds are set by auto-cpufreq, to periodically check (once set, and every few hours?) if they match values defined in auto-cpufreq.conf file, if not overwrite them. This way we would avoid what happened to me before and what's the source of our problem.

[be-west]

@AdnanHodzic
hey, I now made the daemon regularly apply (1 hour) the threshold settings. Is that fine?
I also added the note you suggested to the README and removed the remaining debug statements.
Maybe one last test, and it should be good to go.

[AdnanHodzic]

@be-west it looks good now, values are picked up correctly, but with charger connected/power plugged in my laptop is not charging ...

In meantime, I requested Copilot review, please take a look at comments/suggestions it made and add your comments or actions on top of them.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.

---

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

[Copilot]

IdeapadBatteryDevice.apply_threshold_settings_to_bat() only toggles conservation mode and never applies start/stop thresholds when conservation mode is off. This is a functional regression vs the previous ideapad_laptop_setup() behavior where thresholds were set when conservation mode was disabled. Consider: (1) only changing conservation mode when the config option is explicitly set, and (2) when conservation mode is off, delegating to super().apply_threshold_settings_to_bat(...) to apply thresholds.

[AdnanHodzic]

@be-west another one to consider.

[Copilot]

This example config (start_threshold = 20, stop_threshold = 1) violates the new validation in BatteryDevice._parse_threshold_values() which requires start_threshold < stop_threshold. With the current parsing order, it will disable thresholds_enabled entirely, meaning conservation mode/threshold behavior described here won't be applied. Update the doc example to match the accepted config, or adjust parsing/validation to support the documented ideapad conservation-mode workflow.

[Copilot]

_get_config() calls conf.items("battery") unconditionally, which raises configparser.NoSectionError when the config file lacks a [battery] section. That will make the battery daemon spam errors (caught in the thread wrapper) and prevents any thresholds from being applied even when thresholds are simply disabled. Handle a missing section by returning {} (e.g., if not conf.has_section("battery")) or catching NoSectionError.

Suggested change

	conf = config.get_config()
	conf = config.get_config()
	if not conf.has_section("battery"):
	# No [battery] section present; return empty config so callers can use defaults
	return {}

[AdnanHodzic]

@be-west this one also seems like a good idea

[be-west]

@AdnanHodzic As long as it is not discharging, that is expected behaviour. It will only start charging once it goes below the start threshold.

[AdnanHodzic]

@AdnanHodzic As long as it is not discharging, that is expected behaviour. It will only start charging once it goes below the start threshold.

It didn't further discharge.

But how is this expected behavior? If charging stop threshold is 80, and start threshold is 20, then I'm expecting it to keep charging until 80%?

start_threshold = 20
stop_threshold = 80

In this case if it's >=20% it should start charging, and stop <=80%

The only way I managed to get it to start charging is by changing start charging threshold to 0. Which kind of defaults the purpose of even having ability to edit start_threshold.

enable_thresholds = true
start_threshold = 0
stop_threshold = 80

This is more what I'm expecting (for it to be charging) until it's <=80%. But I was expecting this to also be the case, if start_threshold = 20 or start_threshold = 50.

[be-west]

If it would always charge when level >= start_threshold, then there would be no point in having a start value. Some laptops/vendors only have stop values but if you want a similar behaviour with ThinkPads, then you would need to set start_threshold=end_threshold-1

[be-west]

@AdnanHodzic I applied the Copilot suggestions and removed the misleading section in the README.

[AdnanHodzic]

Great, thanks!

start_threshold=end_threshold-1 did the trick.

In this case one last thing, can you please add mention of start_threshold=end_threshold-1 in README for ThinkPad users, as other users might be trying to do what I was trying to do.

I also ran another Copilot review with couple of comments which should be looked into, after which we're ready to finally merge these changes 🙂

[AdnanHodzic]

I think it would make more sense to suggest to set the start_threshold to be 0.

Any objections?

[Copilot]

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 7 comments.

---

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

[Copilot]

The _parse_ideapad_conservation_mode method is called with the string "ideapad_laptop_conservation_mode" but never actually reads the config value from the battery configuration. The parameter is ignored (named _), and the method always returns False. This means the ideapad_laptop_conservation_mode configuration option will never be read or applied, even for IdeapadBatteryDevice instances. The method should retrieve the actual config value using self._get_config().get(config_key) and parse it to return True if it's "true" and False otherwise.

Suggested change

	parsed_config["ideapad_conservation_mode"] = (
	self._parse_ideapad_conservation_mode(
	"ideapad_laptop_conservation_mode"
	)
	ideapad_value = config.get("ideapad_laptop_conservation_mode")
	parsed_config["ideapad_conservation_mode"] = (
	isinstance(ideapad_value, str)
	and ideapad_value.strip().lower() == "true"

[AdnanHodzic]

@be-west another good idea

[Copilot]

The battery daemon applies threshold settings immediately in an infinite loop without an initial delay. This means the first application happens at daemon start (good), but then it sleeps for 1 hour. However, the daemon thread is started but never joins or provides any feedback about whether the initial application succeeded. Consider applying settings once immediately before starting the daemon thread, or at least logging when the daemon successfully starts and applies settings for the first time.

Suggested change

	def battery_daemon():
	while True:
	try:
	dev.apply_threshold_settings()
	except Exception as e:
	print(
	f"ERROR: An error occurred while applying battery thresholds: {e}"
	)
	sleep(BATTERY_APPLY_INTERVAL)
	# Apply thresholds once synchronously before starting the background daemon.
	try:
	dev.apply_threshold_settings()
	print("Battery thresholds applied successfully; starting battery daemon.")
	except Exception as e:
	print(
	f"ERROR: An error occurred while applying initial battery thresholds: {e}"
	)
	def battery_daemon():
	while True:
	sleep(BATTERY_APPLY_INTERVAL)
	try:
	dev.apply_threshold_settings()
	except Exception as e:
	print(
	f"ERROR: An error occurred while applying battery thresholds: {e}"
	)

[AdnanHodzic]

@be-west this suggestion makes sense.

[Copilot]

The code sets the stop threshold to 100, then sleeps for 100ms before setting the actual start threshold. However, the comment says this is to avoid 'invalid argument' errors when start >= stop, but the sleep happens AFTER setting stop to 100 (which should already resolve the constraint). The sleep might be unnecessary, or it should happen between setting stop=100 and setting start. The PR description mentions uncertainty about whether the sleep is needed. Consider testing if the sleep can be removed, or add a comment explaining why the sleep is specifically needed after setting stop=100.

[AdnanHodzic]

@be-west think this one can be ignored.

[Copilot]

The documentation states that ideapad conservation mode "works only with laptops that have the ideapad_laptop kernel module", but the code in battery.py also creates an IdeapadBatteryDevice for the ideapad_laptop module (line 50-51). However, there's a mismatch: the conservation mode file path is hardcoded to use "ideapad_acpi" in its path (/sys/bus/platform/drivers/ideapad_acpi/...). This could be confusing. Either clarify in the documentation which kernel modules support conservation mode, or ensure the code comment in ideapad_laptop.py (line 6-7) adequately explains this discrepancy.

Suggested change

	This works only with laptops that have the `ideapad_laptop` kernel module.
	add `ideapad_laptop_conservation_mode = true` to your `auto-cpufreq.conf` file
	This works only on Lenovo Ideapad laptops where the kernel exposes the battery conservation mode interface (typically via the `ideapad_acpi` / `ideapad_laptop` drivers).
	Add `ideapad_laptop_conservation_mode = true` to your `auto-cpufreq.conf` file:

[AdnanHodzic]

@be-west am I wrong or this "misleading" part was already addressed?

[Copilot]

There's a grammatical error in the docstring. "So see valid values" should be "To see valid values".

Suggested change

	So see valid values and more info about different devices,
	To see valid values and more info about different devices,

[AdnanHodzic]

@be-west made commit suggestion for this one already since it's a minor thing.

[be-west]

@AdnanHodzic I went over most suggestions - only skipped the one with the initial application, as we don't want the log line in live mode, and the one about the threshold values not being written for Ideapads. Only the conservation mode applies to them.

[AdnanHodzic]

@be-west thank you for perseverance and dedication to act on all the requested changes, much appreciated.

Thank you for your contribution, besides you @be-west I'll also credit @CasperVM your work on this PR as part of future release.