TamtamHero / fw-fanctrl

A simple systemd service to better control Framework Laptop's fan(s)
BSD 3-Clause "New" or "Revised" License
178 stars 33 forks source link

Add NixOS Flake #26

Closed Svenum closed 2 months ago

Svenum commented 4 months ago

I try to add the nixos flake, but I am not very experienced. So if you want to help, fell free. :smile:

mdvmeijer commented 4 months ago

Hello, thank you for the PR. I also tried my hand at creating a Nix package for fw-fanctrl a while ago, but without the flake.

Using your package + flake, if I run nix build and run the resulting executable, I get the following repeated output:

Could not start dynamically linked executable: ectool
NixOS cannot run dynamically linked executables intended for generic
linux environments out of the box. For more information, see:
https://nix.dev/permalink/stub-ld

Are you able to successfully run fw-fanctrl using your flake? If so, how?

Svenum commented 4 months ago

Ok, I am able to build it just like you sad nix build and then there is the "result/bin" folder with the needed executables in it.

Svenum commented 4 months ago

I run my system on unstable branch of NixOS maybe that is something?

dantefromhell commented 3 months ago

I'm running into the same issue as @mdvmeijer reported, running on nixpkgs-unstable@2748d22b - although a slightly outdated (2 weeks old).

❯ cat /etc/os-release
ANSI_COLOR="1;34"
BUG_REPORT_URL="https://github.com/NixOS/nixpkgs/issues"
BUILD_ID="24.05.20240413.90055d5"
DOCUMENTATION_URL="https://nixos.org/learn.html"
HOME_URL="https://nixos.org/"
ID=nixos
IMAGE_ID=""
IMAGE_VERSION=""
LOGO="nix-snowflake"
NAME=NixOS
PRETTY_NAME="NixOS 24.05 (Uakari)"
SUPPORT_URL="https://nixos.org/community.html"
VERSION="24.05 (Uakari)"
VERSION_CODENAME=uakari
VERSION_ID="24.05"
❯ nix build 'github:TamtamHero/fw-fanctrl/68a710cd3ac6ed0000cbb0bd147b6de4e0bd6689' && result/bin/ectool
Could not start dynamically linked executable: result/bin/ectool
NixOS cannot run dynamically linked executables intended for generic
linux environments out of the box. For more information, see:
https://nix.dev/permalink/stub-ld
Svenum commented 3 months ago

Ok, just to be sure you did this:

  1. git clone https://github.com/Svenum/fw-fanctrl
  2. cd fw-fanctrl
  3. git checkout add-flake
  4. nix build .
mdvmeijer commented 3 months ago

Yes, I followed those four steps. I'm wondering, are you using NixOS or just the Nix package manager on another distro? It seems like the package should just not work on NixOS since it uses the ./bin/ectool binary directly from this repo without any changes done for NixOS.

My guess for fixing this:

I would try this myself, but https://github.com/ssddq/fw-ectool doesn't seem to be updated to support AMD yet, so I'll first have to make a PR for that. I don't have easy access to my Intel FW for now.

Svenum commented 3 months ago

Ok, weird. I use NixOS. I dont understand why I could build this and you dont. Yes to add it as build input could work. I'll try. I have an framework 16 I hope it works if you say it does not work on amd.

mdvmeijer commented 3 months ago

Hmm okay, then I'm not sure. Could be that you have ectool already installed globally? But if not then I'm also out of ideas.

Svenum commented 3 months ago

Not that I know.... But does not matter. I add it to the build inputs that it work for everyone 😉

Svenum commented 3 months ago

Sorry for the late respond @mdvmeijer @dantefromhell. I added the built input now.

Svenum commented 3 months ago

@mdvmeijer @dantefromhell @TamtamHero:

I have got it working, but now I got this errors:

Missing Chromium EC memory map.
Cannot find I2C adapter
Unable to establish host communication
Couldn't find EC
Missing Chromium EC memory map.
Cannot find I2C adapter
Unable to establish host communication
Couldn't find EC
model: Framework laptop 13, cpu_type: AMD, fan_count: 0
Missing Chromium EC memory map.
Cannot find I2C adapter
Unable to establish host communication
Couldn't find EC
speed: 15% temp: 0°C movingAverage: 0.0°C
Missing Chromium EC memory map.
Cannot find I2C adapter
Unable to establish host communication
Couldn't find EC

And I have an Framework 16 but it says: model: Framework laptop 13, cpu_type: AMD, fan_count: 0

Svenum commented 3 months ago

@mdvmeijer @dantefromhell @TamtamHero:

I have got it working, but now I got this errors:

Missing Chromium EC memory map.
Cannot find I2C adapter
Unable to establish host communication
Couldn't find EC
Missing Chromium EC memory map.
Cannot find I2C adapter
Unable to establish host communication
Couldn't find EC
model: Framework laptop 13, cpu_type: AMD, fan_count: 0
Missing Chromium EC memory map.
Cannot find I2C adapter
Unable to establish host communication
Couldn't find EC
speed: 15% temp: 0°C movingAverage: 0.0°C
Missing Chromium EC memory map.
Cannot find I2C adapter
Unable to establish host communication
Couldn't find EC

And I have an Framework 16 but it says: model: Framework laptop 13, cpu_type: AMD, fan_count: 0

If this is fixed it should work. So please help me. :)

dantefromhell commented 3 months ago

After manually adding a config file due to #11 I can build and run

❯ nix build 'github:Svenum/fw-fanctrl/add-flake' --refresh
❯ sudo result/bin/fw-fanctrl
model: Framework laptop 13, cpu_type: Intel, fan_count: 1
speed: 15% temp: 44.75°C movingAverage: 44.75°C
speed: 15% temp: 43.0°C movingAverage: 44.69°C
speed: 15% temp: 43.0°C movingAverage: 44.63°C
^C
❯ sudo result/bin/fw-fanctrl --strategy lazyest
model: Framework laptop 13, cpu_type: Intel, fan_count: 1
speed: 0% temp: 46.0°C movingAverage: 46.0°C
speed: 0% temp: 45.25°C movingAverage: 45.98°C
speed: 0% temp: 44.25°C movingAverage: 45.94°C
speed: 0% temp: 44.5°C movingAverage: 45.9°C
speed: 0% temp: 45.25°C movingAverage: 45.88°C
speed: 0% temp: 44.5°C movingAverage: 45.84°C
speed: 0% temp: 44.5°C movingAverage: 45.81°C
^C

👍🏽

Svenum commented 3 months ago

After manually adding a config file due to #11 I can build and run

❯ nix build 'github:Svenum/fw-fanctrl/add-flake' --refresh
❯ sudo result/bin/fw-fanctrl
model: Framework laptop 13, cpu_type: Intel, fan_count: 1
speed: 15% temp: 44.75°C movingAverage: 44.75°C
speed: 15% temp: 43.0°C movingAverage: 44.69°C
speed: 15% temp: 43.0°C movingAverage: 44.63°C
^C
❯ sudo result/bin/fw-fanctrl --strategy lazyest
model: Framework laptop 13, cpu_type: Intel, fan_count: 1
speed: 0% temp: 46.0°C movingAverage: 46.0°C
speed: 0% temp: 45.25°C movingAverage: 45.98°C
speed: 0% temp: 44.25°C movingAverage: 45.94°C
speed: 0% temp: 44.5°C movingAverage: 45.9°C
speed: 0% temp: 45.25°C movingAverage: 45.88°C
speed: 0% temp: 44.5°C movingAverage: 45.84°C
speed: 0% temp: 44.5°C movingAverage: 45.81°C
^C

👍🏽

That looks great: :+1: Then it seems like my problems are related to the fact, that I own a Framework 16....

Could you also try to add the flake to your system like that:

{
  inputs = {
    nixpkgs.url = "github:NixOS/nixpkgs/nixos-23.11"; # You could try unstable as well
    fw-fanctrl = {
      url = "github:Svenum/fw-fanctrl/add-flake"; # This is NOT a stable tested version
      inputs.nixpkgs.follows = "nixpkgs";
    };
  };
  outputs = {nixpkgs, fw-fanctrl}: {
    nixosConfigurations.foo = nixpkgs.lib.nixosSystem {
      system = "x86_64-linux";
      modules = [
          fw-fanctrl.nixosModules.default
          configuration.nix
      ];
    };
  }
}

and then in your configuration.nix:

# Enable fw-fanctrl
programs.fw-fanctrl.enable = true;

# Add a custom config
programs.fw-fanctrl.config = ''
    {
     ...
    }
'';

If you know how the tool works, you could try to add a new custom config like I told above with programs.fw-fanctrl.config.

If you added the config, could you check if evvery thing works?

TamtamHero commented 3 months ago

I'm following this very remotely since I don't know anything about Nix, but: @Svenum "Couldn't find EC" is symptomatic of an outdated ectool binary. The one available on main on this repo (https://github.com/TamtamHero/fw-fanctrl/blob/main/bin/ectool) is up to date and supports the 16" model

Svenum commented 3 months ago

Ok, then I will try to create a second package definition qnd use your ectool binary. Thanks 👍

Svenum commented 3 months ago

I have added the ectool as an extra package, please try the module and the package

Svenum commented 3 months ago

I have updated the flake, so that you could edit the config like that:

programs.fw-fanctrl.config = {
  defaultStrategy = "lazy";
  strategies = {
    "lazy" = {
      fanSpeedUpdateFrequency = 5;
      movingAverageInterval = 30;
      speedCurve = [
        { temp = 0; speed = 15; }
        { temp = 50; speed = 15; }
        { temp = 65; speed = 25; }
        { temp = 70; speed = 35; }
        { temp = 75; speed = 50; }
        { temp = 85; speed = 100; }
      ];
    };
  };
};
Svenum commented 3 months ago

This PR is read for review and would fix: https://github.com/TamtamHero/fw-fanctrl/issues/19 and https://github.com/TamtamHero/fw-fanctrl/issues/11

Svenum commented 3 months ago

@thomaseizinger I have updated the descriptions, but on some options I do not know for what they are, maybe you know. :smile:

thomaseizinger commented 3 months ago

@thomaseizinger I have updated the descriptions, but on some options I do not know for what they are, maybe you know. 😄

Looks all good from what I can see!

Svenum commented 3 months ago

Ok, some descriptions still have only "". Maybe you could add some text here. If they got filled the PR is ready I think.

thomaseizinger commented 3 months ago

Ok, some descriptions still have only "". Maybe you could add some text here. If they got filled the PR is ready I think.

I don't think I understand ectool well enough to actually fill those in unfortunately! But I think it would be valuable to add those descriptions :)

leopoldhub commented 3 months ago

Also, big changes should soon be merged and some of your configurations will need to change along with them. I will do my best to help you make the transition but will need your help for the Nix stuff.

leopoldhub commented 3 months ago

I would also advise against merging packaging-specific files into the main branch as more of them could be added in the future and it would quickly become a mess (and no guarantee that they will be maintained/updated as new commits are made). I think a separate branch /nix_packaging (or similar) would be more appropriate.

What are your thoughts @TamtamHero ?

Svenum commented 3 months ago

I would also advise against merging packaging-specific files into the main branch as more of them could be added in the future and it would quickly become a mess (and no guarantee that they will be maintained/updated as new commits are made). I think a separate branch /nix_packaging (or similar) would be more appropriate.

What are your thoughts @TamtamHero ?

Thats sound valid. But I think its better to create a stable branch witch is tested at a hole. So main is for dev work and stable is the stable one.

leopoldhub commented 3 months ago

I would also advise against merging packaging-specific files into the main branch as more of them could be added in the future and it would quickly become a mess (and no guarantee that they will be maintained/updated as new commits are made). I think a separate branch /nix_packaging (or similar) would be more appropriate. What are your thoughts @TamtamHero ?

Thats sound valid. But I think its better to create a stable branch witch is tested at a hole. So main is for dev work and stable is the stable one.

I'm on board with this, the main branch for the work in progress version, a latest/stable branch for the most recent version and tags/branches for previous ones

leopoldhub commented 3 months ago

*and the nix / packaging ones based on the latest/stable branch

thomaseizinger commented 3 months ago

I would also advise against merging packaging-specific files into the main branch as more of them could be added in the future and it would quickly become a mess (and no guarantee that they will be maintained/updated as new commits are made). I think a separate branch /nix_packaging (or similar) would be more appropriate. What are your thoughts @TamtamHero ?

Thats sound valid. But I think its better to create a stable branch witch is tested at a hole. So main is for dev work and stable is the stable one.

YMMV but a separate branch is even harder to maintain, no? It is common for the tip of main to not be stable. That is what tags are for. They mark stable releases within a branch.

thomaseizinger commented 3 months ago

The convention for files added to a repo but not necessarily have them up-to-date is to put them in a contrib/ directory.

leopoldhub commented 3 months ago

YMMV but a separate branch is even harder to maintain, no? It is common for the tip of main to not be stable. That is what tags are for. They mark stable releases within a branch.

I agree that more branches can mean more work and that tags may be more appropriate here

The convention for files added to a repo but not necessarily have them up-to-date is to put them in a contrib/ directory.

I disagree.

First of all, some packaging solutions may require files in the root directory of the project, in which case you are cooked.

More than that, it means that you will have every single packaging solution configuration whether you want it or not (imagine installing the package for nix and having AUR configs in the project you pull?).

Lastly, you run the risk of having outdated/broken configs every time someone contributes. If someone wants to contribute to the project, they will have to go through every single packaging solution and make sure their changes do not break them (and most likely won't be able to test them all). In my opinion, this is not their responsibility, but one of the maintainers of that solution. So in this case, the contributor will add the new feature without updating the solutions (and rightly so), which means that you will have outdated/broken solution config most of the time. It will also cause problems with releases, you need to be sure that every single packaging solution are done and working at the exact same time and that they won't need changes before the next stable release.

In the end, I believe that branching is the most appropriate way to do this in our case. Packaging solutions are separate, updated as needed and always in sync with the version they are shipping.

thomaseizinger commented 3 months ago

Lastly, you run the risk of having outdated/broken configs every time someone contributes. If someone wants to contribute to the project, they will have to go through every single packaging solution and make sure their changes do not break them (and most likely won't be able to test them all).

That very much depends on what policy is being set. In the best case, nothing breaks and people just get the new features. In the worst case, somebody updating their flakes (speaking for NixOS here) will receive a broken package and make a bug report. Then the maintainer of the nix packaging (or somebody else) can come along and fix things.

If everything is on a separate branch, people have to actively monitor every push to main and update the respective branches. That is a lot more work for everyone for IMO little benefit. It is much harder to detect outdated packages compared to broken ones.

leopoldhub commented 3 months ago

That very much depends on what policy

That's true, however I still think that active updates for packaging solutions are better than the implicit way.

We should do our best not to break things, and as you said, in most cases it updating them would not require any specific manipulation, just testing that there are no obvious side effects.

This would also improve the security side of things. If, for whatever reason, a commit had a dangerous side effect on some of the packaging solutions, it would have a better chance of being caught early.

thomaseizinger commented 3 months ago

I guess we could build some automation to automatically open a PR to the respective branch for each push to main. That way, whoever are the maintainers of the nix packaging stuff can at least get notified.

Svenum commented 3 months ago

I think that is a great idea. And for issues templates would be nice, so that the user know that he has to mark us (@thomaseizinger @leopoldhub and @Svenum as long as you want) when its a Nix related problem.

leopoldhub commented 3 months ago

I think that is a great idea. And for issues templates would be nice, so that the user know that he has to mark us (@thomaseizinger @leopoldhub and @Svenum as long as you want) when its a Nix related problem.

OK with me, I will work on the project globally for the time being.

leopoldhub commented 3 months ago

Hi @Svenum , could you please rebase your branch onto the updated main? I will start to adapt this branch as soon as it is done. Have a great day

Svenum commented 3 months ago

I am going to do this tomorrow 👍

Svenum commented 3 months ago

Branch is up to date now

thomaseizinger commented 3 months ago

I think that is a great idea. And for issues templates would be nice, so that the user know that he has to mark us (@thomaseizinger @leopoldhub and @Svenum as long as you want) when its a Nix related problem.

Works for me too! My Nix-fu is still pretty bad but happy to help!

This PR should target a different branch then, right?

leopoldhub commented 3 months ago

Works for me too! My Nix-fu is still pretty bad but happy to help!

This PR should target a different branch then, right?

Thanks, yes, I belive so

Svenum commented 3 months ago

Ok, on witch branch should I push then?

thomaseizinger commented 3 months ago

Ok, on witch branch should I push then?

We need @TamtamHero to create a new branch and then we can re-direct this PR to it. Something like nixos-flake?

leopoldhub commented 3 months ago

@Svenum I just created the packaging/nix branch, could you target this one?

Svenum commented 3 months ago

Is changed

leopoldhub commented 3 months ago

I am currently working on a pr related to this one #37 that should make our work easier. I will start working on this one as soon as all is well over there. Have a nice day

Svenum commented 3 months ago

The deletion of the requirements.txt borke the nix package

leopoldhub commented 3 months ago

The deletion of the requirements.txt borke the nix package

Hi, yes I will look into it. I haven't had much time in the last 3 days, I will be more active this week. I will keep you informed 👍

Svenum commented 3 months ago

Ok, I fixed the build Issue but now I get this after typing fw-fanctrl:

Traceback (most recent call last):
  File "/nix/store/pq5jxxsp2iqwan95fyj3aasc8m01r4kp-python3.11-fw-fanctrl-20-04-2024/bin/.fw-fanctrl-wrapped", line 322, in <module>
    main()
  File "/nix/store/pq5jxxsp2iqwan95fyj3aasc8m01r4kp-python3.11-fw-fanctrl-20-04-2024/bin/.fw-fanctrl-wrapped", line 303, in main
    client_socket.connect(COMMANDS_SOCKET_FILE_PATH)
FileNotFoundError: [Errno 2] No such file or directory

Looks like the same error as in https://github.com/TamtamHero/fw-fanctrl/issues/35.

The problem is maybe that line: https://github.com/TamtamHero/fw-fanctrl/blob/176d34bedafc3540edd9261629ebf47f673252b7/fanctrl.py#L16C1-L16C91

In NixOS you dont have the right to write in /etc. I would recomend to do the socket under /run/fw-fanctrl/

leopoldhub commented 3 months ago

Hi @Svenum , If the root user cannot write to /etc, then the socket does not exist. You are right about using /run instead of /etc as it is in the standard. I will make the change in #37 as well.