feat(vader5): present Elite-2 paddles via UHID HID descriptor (045e:0b00) [HARDWARE-GATED]#387
Closed
BANANASJIM wants to merge 2 commits into
Closed
feat(vader5): present Elite-2 paddles via UHID HID descriptor (045e:0b00) [HARDWARE-GATED]#387BANANASJIM wants to merge 2 commits into
BANANASJIM wants to merge 2 commits into
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds output backend selection (uinput/uhid) and present_output_id; implements optional uinput FF-only sidecar for rumble when UHID main-pad masquerade is used; updates DeviceInstance identity precedence and event-loop FF routing; extends UHID bindings to drain kernel→userspace events and adds tests and a Vader TOML enabling UHID masquerade. ChangesUHID Output Backend with FF Sidecar
Sequence DiagramsequenceDiagram
participant Config as Config
participant DeviceInit as DeviceInstance Init
participant UHID as UhidDevice
participant Sidecar as Uinput FF Sidecar
participant EventLoop as EventLoop
Config->>DeviceInit: backend="uhid", present_output_id=true
DeviceInit->>UHID: create primary UHID (effective_vid/pid/name)
alt uinput FF configured and not skipped
DeviceInit->>Sidecar: create reduced FF-only uinput device
DeviceInit->>EventLoop: addUinputFf(sidecar), pass sidecar.outputDevice() as ff_output
end
EventLoop->>UHID: loop reads primary UHID fd via drainEvent
UHID->>EventLoop: drainEvent returns .output => forward via output_cb
EventLoop->>EventLoop: pollFf uses ff_output when present, else ctx.output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/device_instance.zig (2)
388-395:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor
[output].nameon the primary UHID device.Line 389 still feeds
cfg.device.nameintoUHID_CREATE2, so the new UHID path ignores the configured virtual name. Withdevices/flydigi/vader5.toml, that means the device is still created asFlydigi Vader 5 Proinstead ofXbox Elite Series 2, even though this PR is explicitly relying on output identity masquerading.Suggested fix
const primary_cfg = uhid_mod.Config{ - .name = cfg.device.name, + .name = out_cfg.name orelse cfg.device.name, .uniq = std.mem.sliceTo(uniq_z, 0), .vid = effective_vid, .pid = effective_pid, .descriptor = primary_descriptor, .output = out_cfg.*, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/device_instance.zig` around lines 388 - 395, The primary uhid configuration is still using cfg.device.name for the UHID_CREATE2 name, ignoring the output identity; update the primary_cfg construction (uhid_mod.Config initialization) to use the output's configured name (e.g., out_cfg.name or out_cfg.*.name) instead of cfg.device.name so the primary UHID device honors [output].name; ensure you reference the same field used elsewhere for output identity masquerading and preserve existing fields (uniq, vid, pid, descriptor, output) when making this change.
442-450:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftReplace the
devices[0]PID-FFB heuristic with explicit interface selection.Lines 442-450 always bind
FfbForwardertodevices[0]. On multi-interface wheels, the first opened hidraw node can be a non-FFB control interface, so UHID PID output gets forwarded to the wrong fd and force feedback never reaches the actual wheel interface.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/device_instance.zig` around lines 442 - 450, The current PID-FFB heuristic always picks devices[0] for phys_fd which breaks multi-interface wheels; replace this by scanning the devices slice to pick the hidraw node that exposes the force-feedback interface (rather than devices[0]). Update the logic around phys_fd (and where FfbForwarder is bound) to iterate devices, detect the correct interface by checking each Device's reported interfaces/attributes (e.g., HID usage page/usage or a flag/method that indicates FFB capability) and choose that device's pollfd().fd (falling back to the existing test_physical_hidraw_fd or -1 if none found); update related code that constructs or passes into FfbForwarder to use the selected fd. Ensure you reference and modify the devices variable, phys_fd assignment, and any code that binds the FfbForwarder to the physical fd.
🧹 Nitpick comments (1)
.gitignore (1)
11-11: 💤 Low valueDuplicate gitignore pattern.
The
core.*pattern is already present at line 41. Gitignore rules apply globally regardless of position, so this entry is redundant.♻️ Proposed cleanup
-core.*🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.gitignore at line 11, Remove the duplicate gitignore pattern by deleting the redundant "core.*" entry so only the original "core.*" remains in .gitignore; locate the duplicate "core.*" token in the diff and remove that line (keep the earlier occurrence at line 41) to avoid redundant rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/test/supervisor_uhid_routing_test.zig`:
- Around line 517-531: initOutputUhid currently takes parsed by value and passes
&parsed.value into DeviceInstance.init which causes DeviceInstance.device_cfg to
point at stack memory that will be invalid after return; change the function
signature so callers pass a pointer to ParseResult (i.e., accept parsed:
*ParseResult or the same type as callers use) and forward that pointer directly
to DeviceInstance.init instead of &parsed.value; update call sites to pass
&parsed.value rather than the value, and ensure references to parsed in
initOutputUhid use the pointer type to avoid storing a pointer to stack data in
DeviceInstance.device_cfg.
---
Outside diff comments:
In `@src/device_instance.zig`:
- Around line 388-395: The primary uhid configuration is still using
cfg.device.name for the UHID_CREATE2 name, ignoring the output identity; update
the primary_cfg construction (uhid_mod.Config initialization) to use the
output's configured name (e.g., out_cfg.name or out_cfg.*.name) instead of
cfg.device.name so the primary UHID device honors [output].name; ensure you
reference the same field used elsewhere for output identity masquerading and
preserve existing fields (uniq, vid, pid, descriptor, output) when making this
change.
- Around line 442-450: The current PID-FFB heuristic always picks devices[0] for
phys_fd which breaks multi-interface wheels; replace this by scanning the
devices slice to pick the hidraw node that exposes the force-feedback interface
(rather than devices[0]). Update the logic around phys_fd (and where
FfbForwarder is bound) to iterate devices, detect the correct interface by
checking each Device's reported interfaces/attributes (e.g., HID usage
page/usage or a flag/method that indicates FFB capability) and choose that
device's pollfd().fd (falling back to the existing test_physical_hidraw_fd or -1
if none found); update related code that constructs or passes into FfbForwarder
to use the selected fd. Ensure you reference and modify the devices variable,
phys_fd assignment, and any code that binds the FfbForwarder to the physical fd.
---
Nitpick comments:
In @.gitignore:
- Line 11: Remove the duplicate gitignore pattern by deleting the redundant
"core.*" entry so only the original "core.*" remains in .gitignore; locate the
duplicate "core.*" token in the diff and remove that line (keep the earlier
occurrence at line 41) to avoid redundant rules.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05648ccc-8c7a-4fe7-9e0d-aad53d47dc6d
📒 Files selected for processing (6)
.gitignoredevices/flydigi/vader5.tomlsrc/config/device.zigsrc/device_instance.zigsrc/event_loop.zigsrc/test/supervisor_uhid_routing_test.zig
Comment on lines
+517
to
+531
| fn initOutputUhid( | ||
| allocator: std.mem.Allocator, | ||
| parsed: anytype, | ||
| mock: *MockDeviceIO, | ||
| primary_fd: posix.fd_t, | ||
| ) !DeviceInstance { | ||
| const devices = try allocator.alloc(DeviceIO, 1); | ||
| devices[0] = mock.deviceIO(); | ||
| var counter: u16 = 1; | ||
| return DeviceInstance.init(allocator, &parsed.value, null, null, &counter, .{ | ||
| .test_primary_uhid_fd = primary_fd, | ||
| .test_devices_override = devices, | ||
| .test_skip_ff_sidecar = true, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Don’t pass ParseResult by value in this helper.
Line 526 passes &parsed.value into DeviceInstance.init, but parsed is a by-value parameter. DeviceInstance stores that pointer in device_cfg, so after initOutputUhid() returns the instance points at stack memory from this helper. These tests mostly inspect owner, which masks it, but anything that reads inst.device_cfg is now using a dangling pointer.
Suggested fix
fn initOutputUhid(
allocator: std.mem.Allocator,
- parsed: anytype,
+ cfg: *const device_mod.DeviceConfig,
mock: *MockDeviceIO,
primary_fd: posix.fd_t,
) !DeviceInstance {
const devices = try allocator.alloc(DeviceIO, 1);
devices[0] = mock.deviceIO();
var counter: u16 = 1;
- return DeviceInstance.init(allocator, &parsed.value, null, null, &counter, .{
+ return DeviceInstance.init(allocator, cfg, null, null, &counter, .{
.test_primary_uhid_fd = primary_fd,
.test_devices_override = devices,
.test_skip_ff_sidecar = true,
});
}Call sites should pass &parsed.value.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn initOutputUhid( | |
| allocator: std.mem.Allocator, | |
| parsed: anytype, | |
| mock: *MockDeviceIO, | |
| primary_fd: posix.fd_t, | |
| ) !DeviceInstance { | |
| const devices = try allocator.alloc(DeviceIO, 1); | |
| devices[0] = mock.deviceIO(); | |
| var counter: u16 = 1; | |
| return DeviceInstance.init(allocator, &parsed.value, null, null, &counter, .{ | |
| .test_primary_uhid_fd = primary_fd, | |
| .test_devices_override = devices, | |
| .test_skip_ff_sidecar = true, | |
| }); | |
| } | |
| fn initOutputUhid( | |
| allocator: std.mem.Allocator, | |
| cfg: *const device_mod.DeviceConfig, | |
| mock: *MockDeviceIO, | |
| primary_fd: posix.fd_t, | |
| ) !DeviceInstance { | |
| const devices = try allocator.alloc(DeviceIO, 1); | |
| devices[0] = mock.deviceIO(); | |
| var counter: u16 = 1; | |
| return DeviceInstance.init(allocator, cfg, null, null, &counter, .{ | |
| .test_primary_uhid_fd = primary_fd, | |
| .test_devices_override = devices, | |
| .test_skip_ff_sidecar = true, | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/test/supervisor_uhid_routing_test.zig` around lines 517 - 531,
initOutputUhid currently takes parsed by value and passes &parsed.value into
DeviceInstance.init which causes DeviceInstance.device_cfg to point at stack
memory that will be invalid after return; change the function signature so
callers pass a pointer to ParseResult (i.e., accept parsed: *ParseResult or the
same type as callers use) and forward that pointer directly to
DeviceInstance.init instead of &parsed.value; update call sites to pass
&parsed.value rather than the value, and ensure references to parsed in
initOutputUhid use the pointer type to avoid storing a pointer to stack data in
DeviceInstance.device_cfg.
…b00)
Steam unlocks the Xbox Elite Series 2 paddle config UI only when it detects a
real Elite-2 HID device via hidapi. The Vader's virtual pad was presented
through uinput (BUS_VIRTUAL evdev caps, no HID report descriptor), so Steam saw
a descriptor-less pad at 045e:0b00 and applied a generic Xbox mapping with no
paddles.
Route the main pad to the UHID backend and present the [output] masquerade
identity so Steam sees a real HID device whose descriptor declares the four
back paddles (M1-M4 -> BTN_TRIGGER_HAPPY1-4) as Button usages.
- config: new [output] backend ("uinput"|"uhid") routes the main pad to UHID
independently of imu/ffb; new [output] present_output_id presents the
[output] vid/pid instead of the daemon identity 0xFADE:0xC001. Both validated
and default to current behavior, so existing UHID devices (IMU/PID, which
rely on 0xFADE:0xC001 for SDL pairing) are unaffected.
- device_instance: use_uhid honors [output] backend=uhid; identity precedence
present_output_id > clone_vid_pid > daemon default.
- rumble: a UHID gamepad descriptor has no FF output collection, so an FF-only
uinput sidecar is spawned (EV_FF, masquerade vid/pid, no input caps) to keep
rumble working when the main pad moves to UHID; FF polling routes to it.
- vader5.toml: opt in with backend=uhid + present_output_id=true.
- tests: routing-to-UHID, identity=045e:0b00 (not FADE:C001), descriptor
declares paddle usages 17-20, plus config validation cases.
UNVERIFIED in software: whether Steam's hidapi Elite-2 detection accepts a
generic-gamepad UHID descriptor at 045e:0b00 requires real-hardware validation.
BANANASJIM
force-pushed
the
fix/vader5-uhid-paddles
branch
from
2cb5463 to
5a77d90
Compare
…PORT The primary UHID fd was registered with the event loop only inside the PID-force-feedback block, so devices with plain rumble FFB (e.g. Flydigi Vader 5) never had /dev/uhid drained. After UHID_CREATE2 the kernel issues UHID_GET_REPORT / UHID_SET_REPORT during the HID probe; left unanswered they back-pressure the probe and the device never goes live, so zero input events reach the kernel (confirmed on real hardware). - Register primary_uhid.fd unconditionally for the UHID path; remove the now-duplicate registration in the PID block (FfbForwarder wiring stays). - Replace pollOutputReport in the loop drain with drainEvent, which: ignores lifecycle events (START/OPEN/CLOSE/STOP), answers GET_REPORT with UHID_GET_REPORT_REPLY and SET_REPORT with UHID_SET_REPORT_REPLY (empty, err=0, echoing the request id), forwards UHID_OUTPUT via output_cb, and loops to EAGAIN so the queue can't back up. - Tests: drainEvent GET_REPORT reply / lifecycle / OUTPUT; regression that a non-PID UHID main pad registers uhid_output_slot.
Owner
Author
|
Closing. Root cause of the Steam Elite-2 paddles is NOT a UHID/identity issue — it's a regression in the uinput paddle mapping (PR #353 moved M1-M4 from BTN_TRIGGER_HAPPY5-8, which the real Elite 2 / Steam read, to HAPPY1-4). Fix is a one-line config revert on the uinput path, no UHID needed. This branch (UHID Elite-2 emulation) also conflicts with P1/P2/ADR-015 (generic descriptor derivation, no device-specific protocol). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Presents the Vader as a real HID Elite-2 so Steam can unlock the paddle config UI. UNVERIFIED whether Steam accepts a generic-gamepad UHID descriptor at 045e:0b00 vs needing the real Elite-2 vendor descriptor — only real hardware confirms paddles appear. Gated on the checklist below.
Why
The Vader's main pad is presented via uinput, which exposes no HID report descriptor. Steam's Elite-2 paddle UI is gated on detecting a real Elite-2 HID device (hidapi), so a descriptor-less evdev pad at 045e:0b00 gets a generic Xbox mapping with no paddles (#355).
What changed
[output] backend = uinput|uhid(route main pad to UHID) +present_output_id(present the [output] masquerade VID/PID on UHID instead of the daemon id). Both default to current behavior — existing UHID/IMU devices unaffected.Test plan
refs #355
Summary by CodeRabbit
New Features
Chores