Skip to content

Conversation

JoaquinTrinanes
Copy link

Description of changes

This PR adds an extra check to the logic that conditionally enabled tlp on laptops by default. Before, it was enabled if power-profiles-daemon.

With this change, we also check for services.tuned.enable, which also conflicts with tlp. Enabling tuned and loading the default laptop module resulted in an evaluation error of the config.

I have a couple of questions regarding the changes:

  • Looking at the NixOS tuneD module, auto-cpufreq also conflicts with the service. Should we add a check for it as well?
  • The original logic used a lib.version check before accessing the power-profiles-daemon to avoid evaluation error in older NixOS releases (pc/laptop: fix evaluation on 20.09 #270). I replaced that with an existence check for both options for consistency, but I can revert that part (tuned was released in 25.05 I believe).

In the end, this PR is superseded by #1474, but it is a nice QOL improvement while it doesn't land.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Looking at the NixOS tuneD module, auto-cpufreq also conflicts with the service. Should we add a check for it as well?

I am using /common/pc/laptop/default.nix and enable services.auto-cpufreq.enable, resulting in service.tlp.enable being implicitly enabled.

Although both services may conflict, they can apparently be used together. Personally, I just keep both enabled without bothering with fancy configurations on my end, hoping auto-cpufreq works reasonably well alongside tlp. Either way, my system never crashed due to this, if that is of interest.

Consequently, I do not know whether a warning or assertion would be appropriate.

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.

2 participants