search

hhd-dev / hhd

Handheld Daemon, a tool for configuring handheld devices.
GNU General Public License v3.0
121 stars 16 forks source link

NixOS TDP Control #78

Open balintbarna opened 3 months ago

balintbarna commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

Tagging the maintainers @appsforartists @toast003

appsforartists commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

toast003 commented 3 months ago

Hhd-ui has a pr open (https://github.com/NixOS/nixpkgs/pull/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 commented 3 months ago

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

toast003 commented 3 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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:

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:

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

harryaskham commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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.

balintbarna commented 2 weeks ago

I returned my ROG Ally so I'm not that interested in this issue anymore, however I like that others interested are picking it up. Should I keep it open?