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

NixOS TDP Control #78

Open
balintbarna opened this issue Jul 7, 2024 · 17 comments
Open

NixOS TDP Control #78

balintbarna opened this issue Jul 7, 2024 · 17 comments

Comments

@balintbarna
Copy link

This issue is meant as a feature request as well as a place to track info and progress on this topic.

I run NixOS on my ROG Ally and hhd works great except for the missing overlay and TDP controls. The web app works well enough, however the TDP controls are sorely missed. What's missing in order to include it in the NixOS package?

@antheas
Copy link
Collaborator

antheas commented Jul 7, 2024

Hi

Handheld Daemon is 3 projects: hhd, adjustor (TDP), and hhd-ui. Right now I think only hhd is packaged. The other two will need to be packaged as well. HHD-UI works great as an appimage for overlay support.

It just needs to be installed as an executable with one of these names: "hhd-ui.AppImage", "hhd-ui-dbg", "hhd-ui" in the system path.

As for adjustor, it has recently gained a bunch of dependencies for fixing the steam slider and acting as ppd. You will also need acpi_call as a kernel patch or dkms package.

@antheas
Copy link
Collaborator

antheas commented Jul 7, 2024

For hhd-ui specifically, the AUR version unpackages the asar electron file and is 5mb because it uses electron. But fedora does not package electron, so it places the appimage in /usr/bin I think. Both approaches are realistic.

@antheas
Copy link
Collaborator

antheas commented Jul 7, 2024

Tagging the maintainers @appsforartists @toast003

@appsforartists
Copy link
Contributor

I've got other priorities in my life right now, so I don't expect to get to this anytime soon. Happy to give feedback on PRs, although I'm new to Nix as well. (I got it working just-well-enough to make my Legion Go usable, and since then I've only used it for the occasional game.)

@appsforartists
Copy link
Contributor

If someone does package the other components, you'll likely need to update the paths to point at the right packages. @toast003 has an example here.

@toast003
Copy link

toast003 commented Jul 8, 2024

That was only needed for hhd itself, since the udev rules that hhd generates expect chmod to be in /bin/

@toast003
Copy link

toast003 commented Jul 8, 2024

Hhd-ui has a pr open (NixOS/nixpkgs#305027) but it got stuck cause I could not get it to build from source. It needs to be updated, and I also have more experience with nix now so I'll give it another shot

@appsforartists
Copy link
Contributor

Yeah, but whatever is looking for hhd-ui on the $PATH probably needs to be updated too.

@toast003
Copy link

No, thankfully adding hhd-ui (and lsof) to hhd's path with systemd.services.handheld-daemon.path = [pkgs.handheld-daemon-ui pkgs.lsof]; is enough

@Faupi
Copy link

Faupi commented Aug 19, 2024

I was able to include adjustor in handheld-daemon fine on NixOS (Legion Go) with the following configurations - adjustor package, integration under hhd.
I wasn't able to really look into setting up anything official for this yet, but maybe somebody could use this. It would be handy to have it included in the hhd NixOS module as an option too.

@appsforartists
Copy link
Contributor

You could probably port that to nixpkgs under a name like handheld-daemon-adjustor and then add a config flag to enable/disable it in services.handheld-daemon.

@harryaskham
Copy link

Huh there must be something in the air because I did this same thing a couple days ago :)

I think we need to go the buildPythonLibrary -> toPythonApplication route because HHD as-packaged does some on the fly importing of e.g. adjustor.drivers.amd... so needs the library itself either in its venv or PYTHONPATH, doesn't just orchestrate the adjustor binary

But then adjustor in turn imports from hhd.*; hhd is also currently installed package only. So to get this working (on a Win Mini 2024) I initially had to do this:

  • package handheld-daemon-ui using appimageTools
  • package pythonPackages.handheld-daemon-adjustor using buildPythonLibrary (otherwise identical to @Faupi version)
  • package a new pythonPackages.handheld-daemon using buildPythonLibrary with propagatedBuildInputs ++ [lsof, pythonPackages.handheld-daemon-adjustor, pkgs.handheld-daemon-ui] (otherwise identical to original handheld-daemon.nix)
  • package pkgs.adjustor as (toPythonApplication pythonPackages.handheld-daemon-adjustor)
    • Possibly optional - in my usage the adjustor CLI entrypoint is never used and looks to be an empty stub - is CLI adjustor ever used?
  • replace pkgs.handheld-daemon with (toPythonApplication pythonPackages.handheld-daemon)
    • overlaying the existing package with [lsof, py.hhd-adjustor, hhd-ui] also works

A hack here is that pythonPackages.handheld-daemon-adjustor doesn't depend on the pythonPackages.handheld-daemon lib it imports at runtime otherwise we get a circularity; it's enough that this is on the system PYTHONPATH. I wrote a small services.hhd.{daemon,ui,tdp}.enable module that ensure they are co-installed in the meantime.

If we're fine with the packages being individually incomplete then the hacked version I already have working would be fine with services.handheld-daemon responsible for having both pylibs co-installed. This is my first time looking at python packaging for nix so maybe there's another way to break the circularity but this implies to me we should have:

  • move pkgs.handheld-daemon to pythonPackages.handheld-daemon
    • Add pythonPackages.handheld-daemon.adjustor as a subpackage so hhd and adjustor can mutually import
  • pkgs.handheld-daemon now wraps pythonPackages.handheld-daemon
  • if needed, add pkgs.handheld-daemon.adjustor package that wraps only pythonPackages.handheld-daemon.adjustor

happy to take this on modulo a sanity check from folk above

@harryaskham
Copy link

harryaskham commented Aug 23, 2024

I took this opportunity to split out a public subset of my nix config, currently working with the latter setup of packaging pythonPackages.handheld-daemon.{hhd,adjustor} and wrapping as pkgs.handheld-daemon{-adjustor}.

https://github.com/harryaskham/collective-public/tree/main/pkgs

My main uncertainty stopping a nixpkgs PR is around the python codependency resolution and whether adjustor CLI is a useful artefact to expose

@appsforartists
Copy link
Contributor

Thanks for doing the legwork!

I'm not really a Nix developer. I learned just enough to make my Legion Go work, and then packaged handheld-daemon for the edification of my future self and other players.

I do think it's useful/important to consistently prefer handheld-daemon over hhd. I made that original choice to make it obvious to an unfamiliar reader what that block of config is doing. Now it has the additional importance of being consistent with the existing package, so people aren't left guessing which prefix to choose.

If/when you package this in a PR, please change the pnames and file names accordingly. Feel welcome to make yourself a maintainer of handheld-daemon too.

There's a chat room for handheld gamers that's frequented by Nix experts:
https://matrix.to/#/#Jovian-Experiments:matrix.org

That's where I got support as I was getting my system set up, and I expect you'd find great reviewers for your PR there too.

@harryaskham
Copy link

harryaskham commented Aug 25, 2024

ACK - agree about the verbosity. In my example I'd only abbreviated in the subpackage to avoid repetition so we have e.g. pythonPackages.handheld-daemon.hhd, pythonPackages.handheld-daemon.adjustor as fully qualified names, but I'm happy with pythonPackages.handheld-daemon.handheld-daemon too (.HHD would match the top level python import but I am not sure that's important). Most users should never see these as they'll be unexposed dependencies of pkgs.handheld-daemon{-ui} itself.

@toast003
Copy link

toast003 commented Oct 8, 2024

I'm making a PR to add adjustor to nixpgks, and so far I just needed to build adjustor and add it to handheld-daemon's propagatedBuildInputs and that seems to be enough for TDP changing.

@harryaskham did you have any issues that made you repackage handheld-daemon as a library? Since what I did seems to work so far I feel like I'm missing something

@harryaskham
Copy link

harryaskham commented Oct 8, 2024

Ah, funny timing, I was just refactoring my flakes and had just been looking at these.

I believe my intention was to have the adjustor binary from https://github.com/hhd-dev/adjustor as its own standalone package. adjustor imports from hhd.plugins, hence needed a pythonPackages.handheld-daemon-hhd or similar that adjustor could have in its buildInputs.

I'm not au fait with the HHD plugin system - I seem to recall that some adjustor usage was by HHD orchestrating the adjustor/adj entrypoint scripts by forking them (so needed the packages visible to the current-system), and

redacted edit

some was by HHD directly importing Python from the `adjustor` package (fixed either by making the `adjuster` python lib visible to the system, or adding it to `handheld-daemon`'s `propagatedBuildInputs).

^^^ (I can't find anywhere handheld-daemon pulls in from adjustor in the Python, so unless it's implicitly dependent I misremembered this)

Other distro packaging looks like the two binaries are separate, I assume due to the shell orchestration. On pypi hhd and adjustor are also packaged separately and neither pyproject depends on the other - likely to break the circularity?

Anyway having said all that: neither works without the other and it would make sense to me that the hhd/adjustor binaries/libraries were combined, with two outputs hhd and adjustor that could each see python3Packages.handheld-daemon and python3Packages.adjustor. Otherwise, if adjustor is to be separate, I think the kicker is it needs to have hhd.* in its PYTHONPATH when run as a system-level installed binary.

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

No branches or pull requests

6 participants