Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move battery options to non-asus specific folder #965

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fidgetingbits
Copy link
Contributor

Description of changes

The current asus/battery.nix file isn't actually asus-specific, and seems useful to be used by other non-asus laptops. This moves it into a generic folder common/pc/laptop/ and updates the current users.

Since there was also an option hardware.asus.battery, this deprecates the old option and changes to new hardware.battery instead.

I tested that the deprecation warning works if hardware.asus.battery.chargeUpTo = 50; is set somewhere. I also ran the test script against the three updated asus laptops to ensure they pass.

I have not actually tested the changes on a live machine.

Things done
  • Tested the changes in your own NixOS Configuration
  • Tested the changes end-to-end by using your fork of nixos-hardware and
    importing it via <nixos-hardware> or Flake input

@Mic92
Copy link
Member

Mic92 commented Jun 7, 2024

Let me know when you have tested this:

Currently we have an eval error for some laptops.

error: path '/home/runner/work/nixos-hardware/nixos-hardware/asus/battery.nix' does not exist

@ambroisie
Copy link

I'm curious why one would want to use this instead of the more full-featured tlp? Trying to reduce "bloat"?

@fidgetingbits
Copy link
Contributor Author

It's a good question. I was also recently pondering if just adding some module options for tlp package would be better.

I wasn't actually aware of tlp until this PR. Originally the main appeal of the nixos-hardware options was I could easily set them without having to look into how to configure anything and then just wanted it on my other non asus laptop.

Is there any particular reason similar tunable options haven't been added to the tlp package?

@ambroisie
Copy link

ambroisie commented Jun 13, 2024

@Mic92
Copy link
Member

Mic92 commented Jun 13, 2024

I was thinking the same. I don't think tlp is a particular big dependency either. Only thing is that it might conflict with power-profile-daemon which is in use, when you use most desktop environments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants