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

handheld-daemon: 1.1.0 -> 2.6.4 #303846

Merged
merged 14 commits into from
Apr 17, 2024
Merged

handheld-daemon: 1.1.0 -> 2.6.4 #303846

merged 14 commits into from
Apr 17, 2024

Conversation

toast003
Copy link
Contributor

Description of changes

Lots of changes since this is a pretty big update, but some of the bigger ones are support for more devices, support for tdp settings and gamescope overlay (using packages not in nixpkgs yet) as well as switching to the gpl v3 from the mit license.

hhd-dev/hhd@v1.1.0...v2.6.2

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@ofborg ofborg bot requested a review from appsforartists April 13, 2024 14:44
@toast003 toast003 changed the title Update hhd handheld-daemon: 1.1.0 -> 2.6.2 Apr 14, 2024
@toast003
Copy link
Contributor Author

Something that I have noticed that doesn't work is the controller hiding udev rules.
They run chmod 000 on the original controller so that apps only see the controller managed by hhd, however that fails for some reason. If you manually run chmod on the devices, they get hidden.

Here's an example of a udev rule generated by hhd:

# Hides device gamepad devices stemming from input33
# Managed by HHD, this file will be autoremoved during configuration changes.
SUBSYSTEMS=="input", KERNELS=="input33", ATTRS{id/vendor}=="045e", ATTRS{id/product}=="028e", GOTO="hhd_valid"
GOTO="hhd_end"
LABEL="hhd_valid"
KERNEL=="js[0-9]*|event[0-9]*", SUBSYSTEM=="input", MODE="000", GROUP="root", TAG-="uaccess", RUN+="/bin/chmod 000 /dev/input/%k"
LABEL="hhd_end"

@toast003
Copy link
Contributor Author

Didn't remember that /bin is empty in NixOS 😅

@appsforartists
Copy link
Contributor

Thanks for doing this!

I've fallen behind on the update train in large part because I know newer versions of handheld-daemon have iio rules that I haven't gotten around to testing. (Also, I've just been busy with life stuff and not doing much gaming/tinkering.) Here was the first blush at making them work.

What device(s) have you tested your upgrade on?

Please also include release notes, as linked in the PR template.

@pbek
Copy link
Contributor

pbek commented Apr 15, 2024

I've added the service with

  services.handheld-daemon = {
    enable = true;
    user = username;
    package = (pkgs.callPackage ../../apps/handheld-daemon/package.nix { }).override {
    };
  };

And the daemon runs...
But what should happen now, or how could I test it (on my ROG Ally)? 😊

@pbek
Copy link
Contributor

pbek commented Apr 15, 2024

https://hhd.dev/ can connect to the daemon.

@toast003
Copy link
Contributor Author

toast003 commented Apr 15, 2024

I've fallen behind on the update train in large part because I know newer versions of handheld-daemon have iio rules that I haven't gotten around to testing. (Also, I've just been busy with life stuff and not doing much gaming/tinkering.) Here was the first blush at making them work.

Haven't really tested iio anything since my device doesn't use it as far as I know.

What device(s) have you tested your upgrade on?

A 2023 gpd win max 2 with the 7840u

Please also include release notes, as linked in the PR template.

Isn't handheld daemon is already in the release notes?

@@ -37,6 +39,10 @@ python3.pkgs.buildPythonApplication rec {
hidapi=${ hidapi }/lib/
test -d $hidapi || { echo "ERROR: $hidapi doesn't exist, please update/fix this build expression."; exit 1; }
sed -i -e "s|libhidapi|$hidapi/libhidapi|" src/hhd/controller/lib/hid.py

# The generated udev rules point to /bin/chroot, which does not exist in NixOS
chmod=${ coreutils }/bin/chmod
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be updated to use toybox.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks to pointing that out, didn't realized it 😅

Btw are we using toolbox cause it's preferred by nixpkgs or cause it's already there? (or both idk)

Copy link
Contributor

Choose a reason for hiding this comment

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

IDK if it matters. Just suggesting to use it here because it was already in there.

(I think I chose it originally because it's a small dependency and I'd used it before to access POSIX commands.)

pkgs/by-name/ha/handheld-daemon/package.nix Show resolved Hide resolved
@appsforartists
Copy link
Contributor

Isn't handheld daemon is already in the release notes?

I guess it's only for major/breaking changes… 🤷‍♂️

Sorry, I'm new to this too…

@appsforartists
Copy link
Contributor

@pbek On a Legion Go, the most obvious signs that it's working are that the LED lights turn blue (the default PlayStation color) and both the top face buttons (Legion) and bottom face buttons (start+select) work.

I don't know enough about the ROG Ally to give similar advice, but if you open your Steam controller settings and see a DualShock, that's a good sign it's working.

Are you on unstable or testing this PR specifically?

@pbek
Copy link
Contributor

pbek commented Apr 15, 2024

@pbek On a Legion Go, the most obvious signs that it's working are that the LED lights turn blue (the default PlayStation color) and both the top face buttons (Legion) and bottom face buttons (start+select) work.

Hm, did LEDs didn't do anything and the start and select buttons already did work without hhd in steam and games run by steam. 🤔

I don't know enough about the ROG Ally to give similar advice, but if you open your Steam controller settings and see a DualShock, that's a good sign it's working.

I can still see the XBOX 360 controller, like without hhd.

Are you on unstable or testing this PR specifically?

Unstable, I just added the package.nix of this PR to my config, and I am using it in the service.

https://github.com/pbek/nixcfg/blob/516955f501644e14c7d0486b5bd1e090b9915c33/flake.nix#L157-L169

I was interested in making use of the additional two front facing buttons of the ROG Ally and something like brightness control...

@toast003
Copy link
Contributor Author

I don't know enough about the ROG Ally to give similar advice, but if you open your Steam controller settings and see a DualShock, that's a good sign it's working.

I can still see the XBOX 360 controller, like without hhd.

Have you set controller emulation to dualsense in hhd.dev?

@antheas
Copy link

antheas commented Apr 15, 2024

Toast's device activates the hrtrigger and creates the proper dir so that has been tested in fact

@antheas
Copy link

antheas commented Apr 15, 2024

ROG Ally also uses the hrtrigger, and there will be appropriate logs showing if it works

@appsforartists
Copy link
Contributor

appsforartists commented Apr 15, 2024

@toast003 I don't have scope to test this right now. If you tell me the current PR works as expected, I'm happy to merge this for you after you update that comment edit.

Also feel free to make yourself a maintainer of this lib if you so choose.

@antheas
Copy link

antheas commented Apr 15, 2024

@appsforartists the color of the leds no longer changes due to the playstation driver, the top buttons of the ally are start select

@pbek hhd fixes the bottom buttons of the ally to be guide and xbox + a in legacy mode (which opens the side menu in steam). However in the new version if you launch gamescope, it will detect that and connect to the X11 socket and send the command to open the side menu directly.

As for controlling the leds, if steam launches in the Deck UI, it will take control of them and change their colors, also allow setting them in the settings. In the legion go, the leds persist across reboots, in other devices they reset every boot. So if steam is not running you lose LED control for now.

A UI for the LEDs is planned.

@pbek
Copy link
Contributor

pbek commented Apr 15, 2024

Have you set controller emulation to dualsense in hhd.dev?

I have

ROG Ally also uses the hrtrigger, and there will be appropriate logs showing if it works

I can't see any of those logs with sudo journalctl -b -e.

I can only see lots of those:

Apr 15 20:17:16 ally2 hhd[12787]: /nix/store/5gldb5b71w2qk073xcvkpbzliwdwv29p-set-environment: line 15: warning: setlocale: LC_COLLATE: cannot change locale (de_AT.UTF-8): No such file or directory
Apr 15 20:17:16 ally2 hhd[12787]: /nix/store/5gldb5b71w2qk073xcvkpbzliwdwv29p-set-environment: line 16: warning: setlocale: LC_CTYPE: cannot change locale (en_US.UTF-8): No such file or directory
Apr 15 20:17:16 ally2 hhd[12787]: /nix/store/5gldb5b71w2qk073xcvkpbzliwdwv29p-set-environment: line 21: warning: setlocale: LC_NUMERIC: cannot change locale (de_AT.UTF-8): No such file or directory
Apr 15 20:17:16 ally2 hhd[12787]: /nix/store/5gldb5b71w2qk073xcvkpbzliwdwv29p-set-environment: line 24: warning: setlocale: LC_TIME: cannot change locale (de_AT.UTF-8): No such file or directory
Apr 15 20:17:18 ally2 hhd[12794]: /nix/store/5gldb5b71w2qk073xcvkpbzliwdwv29p-set-environment: line 15: warning: setlocale: LC_COLLATE: cannot change locale (de_AT.UTF-8): No such file or directory
Apr 15 20:17:18 ally2 hhd[12794]: /nix/store/5gldb5b71w2qk073xcvkpbzliwdwv29p-set-environment: line 16: warning: setlocale: LC_CTYPE: cannot change locale (en_US.UTF-8): No such file or directory

@antheas
Copy link

antheas commented Apr 15, 2024

Before merging, please bump the version that fixes this bug that was introduced by localization. And perhaps test that localization is read correctly from the system.

@pbek
Copy link
Contributor

pbek commented Apr 15, 2024

hhd fixes the bottom buttons of the ally to be guide

Ah, I see. Thank you!

Copy link
Contributor

@appsforartists appsforartists left a comment

Choose a reason for hiding this comment

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

redoing LG because the bot insisted, but @NickCao made the requests, so he should be the one doing the reviewing.

Copy link
Contributor

@appsforartists appsforartists left a comment

Choose a reason for hiding this comment

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

Undoing LG because the update looks like it breaks the order of commands in postPatch.

@ofborg ofborg bot requested a review from appsforartists April 16, 2024 19:02
@toast003
Copy link
Contributor Author

Wdym @appsforartists ?

It builds just fine

@appsforartists
Copy link
Contributor

As the comment states, this was originally cargo-culted from @AndersonTorres's postPatch in hid; however:

    # handheld-daemon contains a fork of the python module `hid`, so this hook
    # is borrowed from the `hid` derivation.
    substituteInPlace src/hhd/controller/lib/hid.py \
      --replace-fail libhidapi ${hidapi}/lib/libhidapi
    hidapi=${hidapi}/lib/
    test -d $hidapi || { echo "ERROR: $hidapi doesn't exist, please update/fix this build expression."; exit 1; }

uses hidapi, then defines it, then tests for it (after it's already been used).

The hidapi declaration should come first (and fixed-up as necessary so you don't end up with any weirdness like /lib/lib).

@appsforartists
Copy link
Contributor

Also, this whole refactoring thing feels like it's needlessly blocking the merge.

@NickCao, would you be amenable to merging this as it originally was with sed, and then @toast003 can break the substituteInPlace refactor into a new PR that includes both these sites, as well as the original in hid?

@toast003
Copy link
Contributor Author

substituteInPlace doesn't use the hidapi bash variable that gets declared later, but I can put it on top

@toast003
Copy link
Contributor Author

I don't mind, if that was the only issue I could do it here but whatever is best

@AndersonTorres
Copy link
Member

Wut??

@appsforartists
Copy link
Contributor

I misunderstood - I was reading that as a nix variable, not a bash one.

I do think we should change hid and this in sync, so the hooks continue to match, but I really don't like scope creep.

@NickCao
Copy link
Member

NickCao commented Apr 16, 2024

@NickCao, would you be amenable to merging this as it originally was with sed

Fine.

@toast003
Copy link
Contributor Author

Ok @NickCao undid the refactor, you can merge now

@NickCao NickCao merged commit bb1e65a into NixOS:master Apr 17, 2024
24 checks passed
@toast003 toast003 deleted the update-hhd branch April 18, 2024 07:52
@Faupi
Copy link

Faupi commented Apr 27, 2024

With the "not refactored" sed solution, substituting doesn't actually seem to work (paths still seem to map to /bin/chmod, etc.), switching to the substituteInPlace fully resolved it - and it can even be seen on the native controller hiding properly.
So from my side the reverted refactor looks fine. Could we get an update on it? Currently the package is half-broken due to it.

@toast003
Copy link
Contributor Author

I wanted to get #305027 ready before doing the refactor but I have been very busy lately (and will be until exams are done) so I haven't really had the energy to work on nix things 😅 .

I checked the package's results and paths are being changed, so I'm confused on why it isn't for you.

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

Successfully merging this pull request may close these issues.

8 participants