feat(phase-13-wave-6): HID PID FFB passthrough — closes ADR-015 Stage 3 path (issue #82)#166
Merged
BANANASJIM merged 4 commits intomainfrom Apr 30, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Wave 6 UHID PID force‑feedback passthrough: UHID OUTPUT parsing, PID descriptor build/validation, FfbForwarder to forward reports to a physical hidraw FD, event-loop/UHID wiring, clone_vid_pid config/udev changes, exported test modules, docs, and CI step adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant EventLoop
participant UhidDevice as UHID Device
participant FfbForwarder
participant Hidraw as Physical Hidraw
EventLoop->>EventLoop: poll UHID output slot
EventLoop->>UhidDevice: pollOutputReport()
UhidDevice-->>EventLoop: OutputReport(report_id, data)
EventLoop->>FfbForwarder: forwarderCallback(report)
FfbForwarder->>FfbForwarder: check state (active/disabled)
alt active
FfbForwarder->>Hidraw: write(report bytes)
alt write success
FfbForwarder->>FfbForwarder: increment writes_total
else EAGAIN
FfbForwarder->>FfbForwarder: increment drops_eagain (stay active)
else EACCES/ENODEV/EBADF
FfbForwarder->>FfbForwarder: log & transition to disabled
end
else disabled
FfbForwarder->>FfbForwarder: short-circuit (no write)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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: 6
♻️ Duplicate comments (1)
openspec/changes/phase-13-wave-6-pidff/test-plan.md (1)
34-41:⚠️ Potential issue | 🟡 MinorSame mandatory-set drift as
tasks.md— TP3 and TP5 reference the wrong required set.TP3 ("11-of-12 descriptor") and TP5 (
{1,2,3,4,5,6,10,11,12,13,14,15}) inherit the same stale assumption flagged ontasks.md§T1c: the implementation validates 8 kernel-mandatory IDs ({1, 7, 10, 11, 12, 13, 14, 15}), and IDs 2–6 are emitted but optional. As-written, TP3's "strip one report ID" strategy can yield false negatives — stripping any of IDs 2-6 will NOT triggererror.IncompletePidDescriptor. Suggest adjusting these to match the actual 8-ID set, or strip specifically ID 7 / 10 / 11 / etc. for TP3.No separate action needed beyond the fix on
tasks.md; calling it out here so the test-plan stays in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/changes/phase-13-wave-6-pidff/test-plan.md` around lines 34 - 41, TP3 and TP5 in the test-plan reference the wrong mandatory Report ID set; update them to match the actual kernel-mandatory IDs from tasks.md §T1c ({1, 7, 10, 11, 12, 13, 14, 15}): change TP3 so the "strip one report ID" case removes one of those kernel-mandatory IDs (e.g., ID 7) and assert that validateMandatoryReports returns error.IncompletePidDescriptor, and change TP5’s byte-scan assertion to require presence of each NN ∈ {1, 7, 10, 11, 12, 13, 14, 15} instead of the stale set.
🧹 Nitpick comments (1)
src/io/uhid_descriptor.zig (1)
508-559: buildForPid: clean, kernel-aligned descriptor with end-of-build validation. LGTM.The end-of-build
validateMandatoryReports(buf.items)call (line 557) is the right gate — it fail-closes witherror.IncompletePidDescriptorbefore any malformed bytes reachUHID_CREATE2, which is the exact regression class probe Run 2 hit (kernel OOPS on a 7-of-12 descriptor). The deferredcfg/ffb_cfgparameters are documented at lines 502-507 as intentional shape preservation for the call sites insrc/device_instance.zig:266, so the unused-binding_ = cfg; _ = ffb_cfg;reads cleanly rather than as dead code.One tiny nit (purely optional):
hidItemSizeat lines 2031-2041 — used by tests at lines 1787, 1835, and 2074 — doesn't special-case the long-form prefix0xFEthe wayvalidateMandatoryReports(lines 993-999) does. Today no PID emitter uses long-form items so this never trips, but if anyone reuseshidItemSizeagainst a third-party descriptor it would silently misread long-form items assize=2.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/io/uhid_descriptor.zig` around lines 508 - 559, hidItemSize currently doesn't handle the HID long-item prefix 0xFE, so it can miscompute lengths for long-form items; update hidItemSize to detect a prefix byte 0xFE and, after bounds-checking the buffer, read the following length byte and return the full long-item length (header bytes + payload), mirroring how validateMandatoryReports special-cases long-form items; ensure you handle buffer overrun safety and update/keep the existing tests (they reference hidItemSize) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openspec/changes/phase-13-wave-6-pidff/design.md`:
- Around line 36-43: The spec and validator should require only the 8
kernel-required PID report IDs (not all 12 emitted reports) when kind == "pid";
update the design text and any validator references so pidff_find_reports() and
downstream checks (evdev_open / hid_hw_open) consider those 8 IDs mandatory and
only return -ENODEV if any of those 8 are missing, while allowing the other four
reports to be optional, and adjust the described acceptance criteria and tests
to match this 8-of-8 requirement (also update wording in the sections referenced
around lines 271-286 to reflect the change).
In `@openspec/changes/phase-13-wave-6-pidff/tasks.md`:
- Around line 102-156: Update the spec to match the implementation: change the
“12 PID mandatory reports” wording to reflect the kernel-mandatory count of 8,
add Block Free Report (ID 7 / usage 0x90) to the T1b checklist and remove IDs
2–6 from the mandatory list, update the T1c pseudocode REQUIRED set to
[_]u8{1,7,10,11,12,13,14,15}, and change any “8-of-12” / 12-report wording in
T1d to the corrected 8-report phrasing so the document aligns with the code
constant PID_MANDATORY_REPORT_IDS and the validateMandatoryReports / buildForPid
validation behavior.
In `@src/cli/install.zig`:
- Around line 1859-1860: The dedupe logic for UdevEntry is currently only
preserving block_kernel_drivers; it must also treat clone_vid_pid as additive so
a later override with clone_vid_pid=true isn't lost. Update the merge/dedup code
(the routine invoked by collectAllDeviceEntries() that collapses duplicate
VID/PID entries) to set the resulting UdevEntry.clone_vid_pid = true if any of
the merged entries have clone_vid_pid == true (i.e., OR the flag across
entries), while still preserving block_kernel_drivers as before.
In `@src/device_instance.zig`:
- Around line 331-340: The code currently guesses the physical hidraw write fd
using devices[0].pollfd().fd when out_cfg.force_feedback -> pid_ffb expects a
supervisor-owned physical writer, which can be wrong for multi-interface
devices; change the logic in the FFB setup (where pid_ffb,
opts.test_physical_hidraw_fd, devices, FfbForwarder.init, primary_uhid.fd, and
loop.addUhidOutput are referenced) to require an explicit physical hidraw fd:
prefer opts.test_physical_hidraw_fd, then a configured physical writer field on
pid_ffb (add one if needed), and only as a last resort attempt to locate a
matching device by inspecting device descriptors/paths (not devices[0]) to find
a supervisor-owned hidraw writer; remove or disable the naive devices[0]
fallback so PID data is always forwarded to the correct physical hidraw fd
before calling FfbForwarder.init and loop.addUhidOutput.
In `@src/io/uhid.zig`:
- Around line 459-476: The code in pollOutputReport reads into buf and then
directly reads UhidOutputReq.size and slices req.data without ensuring the read
returned enough bytes for those fields; to fix, after posix.read and after
checking n >= 4 compute payload_len = n - 4 and if payload_len < the offset of
UhidOutputReq.size + 2 (i.e. the bytes needed to safely read the u16 size)
return null; then compute sz = `@min`(payload_len, `@as`(usize, req.size),
UHID_DATA_MAX) and use buf[4..4+sz] (or req.data[0..sz]) when constructing
OutputReport so you never index uninitialized bytes from UhidOutputReq.size or
req.data; reference symbols: pollOutputReport, posix.read, UhidOutputReq,
UhidOutputReq.size, UHID_EVENT_SIZE, UHID_DATA_MAX, OutputReport.
---
Duplicate comments:
In `@openspec/changes/phase-13-wave-6-pidff/test-plan.md`:
- Around line 34-41: TP3 and TP5 in the test-plan reference the wrong mandatory
Report ID set; update them to match the actual kernel-mandatory IDs from
tasks.md §T1c ({1, 7, 10, 11, 12, 13, 14, 15}): change TP3 so the "strip one
report ID" case removes one of those kernel-mandatory IDs (e.g., ID 7) and
assert that validateMandatoryReports returns error.IncompletePidDescriptor, and
change TP5’s byte-scan assertion to require presence of each NN ∈ {1, 7, 10, 11,
12, 13, 14, 15} instead of the stale set.
---
Nitpick comments:
In `@src/io/uhid_descriptor.zig`:
- Around line 508-559: hidItemSize currently doesn't handle the HID long-item
prefix 0xFE, so it can miscompute lengths for long-form items; update
hidItemSize to detect a prefix byte 0xFE and, after bounds-checking the buffer,
read the following length byte and return the full long-item length (header
bytes + payload), mirroring how validateMandatoryReports special-cases long-form
items; ensure you handle buffer overrun safety and update/keep the existing
tests (they reference hidItemSize) accordingly.
🪄 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: 0c953343-73ac-48ff-a2b8-9981a801fc2f
📒 Files selected for processing (16)
build.zigopenspec/changes/phase-13-wave-6-pidff/brief.mdopenspec/changes/phase-13-wave-6-pidff/design.mdopenspec/changes/phase-13-wave-6-pidff/tasks.mdopenspec/changes/phase-13-wave-6-pidff/test-plan.mdsrc/cli/install.zigsrc/config/device.zigsrc/device_instance.zigsrc/event_loop.zigsrc/io/ffb_forwarder.zigsrc/io/uhid.zigsrc/io/uhid_descriptor.zigsrc/main.zigsrc/test/supervisor_uhid_routing_test.zigsrc/test/uhid_output_dispatch_test.zigsrc/test/wave6_pidff_e2e_test.zig
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/device_instance.zig (1)
351-364:⚠️ Potential issue | 🟠 MajorDon't bind the forwarder to
devices[0]'s current fd.This captures an init-time interface fd as the FFB target. Besides picking the wrong interface on multi-interface wheels, it also goes stale after
closeDeviceIO()/rebindDeviceIO():self.devicesgets replaced, butself.ffb_forwarder.physical_fdstill points at the closed original fd, so post-reconnect PID writes will fail or disable forwarding. Please source a stable physical hidraw writer here, or refresh the forwarder whenever the device fds are rebound.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device_instance.zig` around lines 351 - 364, The forwarder is being initialized with a transient fd from devices[0].pollfd().fd (phys_fd) which can be wrong for multi-interface wheels and goes stale after closeDeviceIO()/rebindDeviceIO(); instead either (A) derive and hold a stable physical writer (e.g., open the hidraw device path once and use that fd or a small wrapper that re-opens on demand) and pass that stable writer to FfbForwarder.init, or (B) keep FfbForwarder.init lightweight and update/rehydrate it whenever device IO is rebound by calling a refresh method from rebindDeviceIO()/closeDeviceIO() to replace ffb_forwarder.physical_fd; change the initialization in device_instance where phys_fd is chosen (currently using devices[0] and opts.test_physical_hidraw_fd) and add a refresh hook in closeDeviceIO/rebindDeviceIO to ensure ffb_fwd/ffb_forwarder never holds a closed/stale fd.openspec/changes/phase-13-wave-6-pidff/tasks.md (1)
576-580:⚠️ Potential issue | 🟡 MinorUse a 7-of-8 mandatory descriptor for the negative case.
This spec now defines the required set as
{1, 7, 10, 11, 12, 13, 14, 15}invalidateMandatoryReports, so an “11-of-12” fixture can drop an optional report and still pass. Update this to “omit one mandatory report / 7-of-8” so the test contract matches the validator.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/changes/phase-13-wave-6-pidff/tasks.md` around lines 576 - 580, The negative test "wave6: incomplete PID descriptor → error.IncompletePidDescriptor" currently constructs an 11-of-12 fixture which can still satisfy validateMandatoryReports; change the fixture to omit one of the mandatory reports so it is a 7-of-8 mandatory case: update the helper (e.g., buildForPidIncomplete) or the direct call site to produce a byte sequence missing one element from the mandatory set {1,7,10,11,12,13,14,15} and assert error.IncompletePidDescriptor when validateMandatoryReports is invoked; ensure the test explicitly documents which mandatory ID was removed.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
39-42: LGTM — sequential check target avoids the documented build-cache deadlock.Switching both the matrix and distro-check jobs to
check-all-serialmatches the new ordered step chain inbuild.zig(test → safe → fmt) and the inline comment clearly captures the rationale for future maintainers. One minor consideration worth keeping in mind: serializing the three checks will increase wall-clock time for every matrix cell, so once the Zig incremental-cache contention is resolved upstream it would be worth re-evaluating whethercheck-allcan be restored to recover parallelism.Also applies to: 138-138
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 39 - 42, The workflow was updated to run "zig build ... check-all-serial" in one place but the same change must be mirrored for the other job(s); update the other occurrences (the matrix job and the distro-check job) to use the serial target instead of check-all so both run lines invoke "check-all-serial" and keep the explanatory comment about avoiding the Zig build-cache deadlock; verify the run lines referencing matrix.zig_args and any job names like matrix and distro-check are changed consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openspec/changes/phase-13-wave-6-pidff/design.md`:
- Line 231: Update the sentence that reads "the kernel needs a HID Physical
Interface Device collection with all 12" to distinguish between mandatory vs
total report counts: explicitly state how many total report IDs buildForPid
emits (the descriptor table lists 13 IDs: 1,2,3,4,5,6,7,10,11,12,13,14,15) and
call out that only 8 of those are kernel-mandatory while 5 are optional (match
the earlier mentions of 8 mandatory reports). Ensure the wording removes "all 12
mandatory" and instead specifies "X total reports (13), of which 8 are
kernel-mandatory and 5 are optional" or the correct totals if buildForPid emits
a different number.
In `@openspec/changes/phase-13-wave-6-pidff/test-plan.md`:
- Around line 23-36: Update the test-plan items TP1, TP2, and TP3 to assert the
8-kernel-mandatory reports instead of 12: change TP1 to call buildForPid(alloc,
cfg_with_one_axis, ffb_cfg{kind=pid}) and assert
validateMandatoryReports(result) succeeds for the 8 specific report IDs
(1,7,10,11,12,13,14,15); change TP2’s golden-reference expectation to be
byte-equal for a descriptor containing those 8 mandatory reports (plus optional
ones as defined) rather than assuming 12; and change TP3 to feed
validateMandatoryReports an artificially stripped descriptor with 7-of-8
mandatory reports and expect error.IncompletePidDescriptor. Ensure references to
validateMandatoryReports, buildForPid, and error.IncompletePidDescriptor are
updated accordingly.
- Around line 37-42: Update test-plan.md to explicitly state which PID report
IDs are present in the normal 12-report descriptor versus the 13-report stress
case: list the 8 kernel-mandatory IDs {1,7,10,11,12,13,14,15} (per
PID_REQUIRED_REPORTS in drivers/hid/usbhid/hid-pidff.c) and then enumerate the
five optional IDs {2,3,4,5,6}, clarifying which one of those optional IDs is
omitted in the normal 12-report run and that TP4 (stress) includes all five
optional IDs for a total of 13 reports; adjust TP1/TP2/TP3/TP31 wording to say
“12 reports (8 mandatory + 4 optional: specify which optional IDs)” and change
TP4 to reference the 13-report stress case (8 mandatory + 5 optional) so the
document and the descriptor table in design.md are consistent.
In `@src/cli/install.zig`:
- Around line 2394-2397: The parser for clone_vid_pid (inside the
isFieldKey(trimmed, "clone_vid_pid") branch) currently trims the RHS into val
and compares it directly to "true", which fails when an inline TOML comment
follows (e.g., `clone_vid_pid = true # opt-in`). Change the logic that computes
val (the trimmed RHS slice) to strip any inline comment: if std.mem.indexOf(u8,
val, "#") returns a position, re-slice val to val[0..hashPos] and trim again
before comparing; then perform std.mem.eql(u8, cleaned_val, "true") to set
clone_vid_pid. Ensure this update is applied where val is created and compared.
---
Duplicate comments:
In `@openspec/changes/phase-13-wave-6-pidff/tasks.md`:
- Around line 576-580: The negative test "wave6: incomplete PID descriptor →
error.IncompletePidDescriptor" currently constructs an 11-of-12 fixture which
can still satisfy validateMandatoryReports; change the fixture to omit one of
the mandatory reports so it is a 7-of-8 mandatory case: update the helper (e.g.,
buildForPidIncomplete) or the direct call site to produce a byte sequence
missing one element from the mandatory set {1,7,10,11,12,13,14,15} and assert
error.IncompletePidDescriptor when validateMandatoryReports is invoked; ensure
the test explicitly documents which mandatory ID was removed.
In `@src/device_instance.zig`:
- Around line 351-364: The forwarder is being initialized with a transient fd
from devices[0].pollfd().fd (phys_fd) which can be wrong for multi-interface
wheels and goes stale after closeDeviceIO()/rebindDeviceIO(); instead either (A)
derive and hold a stable physical writer (e.g., open the hidraw device path once
and use that fd or a small wrapper that re-opens on demand) and pass that stable
writer to FfbForwarder.init, or (B) keep FfbForwarder.init lightweight and
update/rehydrate it whenever device IO is rebound by calling a refresh method
from rebindDeviceIO()/closeDeviceIO() to replace ffb_forwarder.physical_fd;
change the initialization in device_instance where phys_fd is chosen (currently
using devices[0] and opts.test_physical_hidraw_fd) and add a refresh hook in
closeDeviceIO/rebindDeviceIO to ensure ffb_fwd/ffb_forwarder never holds a
closed/stale fd.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 39-42: The workflow was updated to run "zig build ...
check-all-serial" in one place but the same change must be mirrored for the
other job(s); update the other occurrences (the matrix job and the distro-check
job) to use the serial target instead of check-all so both run lines invoke
"check-all-serial" and keep the explanatory comment about avoiding the Zig
build-cache deadlock; verify the run lines referencing matrix.zig_args and any
job names like matrix and distro-check are changed consistently.
🪄 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: f5245d35-44b4-42f1-843b-cf55189911b5
📒 Files selected for processing (9)
.github/workflows/ci.ymlbuild.zigopenspec/changes/phase-13-wave-6-pidff/brief.mdopenspec/changes/phase-13-wave-6-pidff/design.mdopenspec/changes/phase-13-wave-6-pidff/tasks.mdopenspec/changes/phase-13-wave-6-pidff/test-plan.mdsrc/cli/install.zigsrc/device_instance.zigsrc/io/uhid.zig
🚧 Files skipped from review as they are similar to previous changes (2)
- src/io/uhid.zig
- build.zig
Comment on lines
+23
to
+36
| - [ ] **TP1**: `buildForPid(alloc, cfg_with_one_axis, ffb_cfg{kind=pid})` | ||
| returns a non-empty descriptor; `validateMandatoryReports(result)` | ||
| returns success (12 reports present). | ||
| - [ ] **TP2**: Output bytes are byte-equal to an inline golden reference | ||
| (Moza R5 case, pinned by implementer on first successful real-hardware | ||
| run when kernel logs show `hid-universal-pidff` FFB init without | ||
| `Error initialising force feedback`). Probe Run 2 evidence covers | ||
| bustype acceptance + driver binding ONLY — it does not supply a | ||
| working descriptor (probe Run 2's 7-of-12 descriptor caused FFB init | ||
| failure + kernel NULL-deref; the implementer must build a complete | ||
| 12-report descriptor per USB HID PID 1.01 §4.x). | ||
| - [ ] **TP3**: `validateMandatoryReports` on a hand-crafted 11-of-12 | ||
| descriptor (one report ID stripped) returns | ||
| `error.IncompletePidDescriptor`. |
There was a problem hiding this comment.
Correct test specifications to align with 8-mandatory-report design.
TP1, TP2, and TP3 reference "12 reports" but the design specification (lines 36-43, 268, 286, 844 in design.md) clearly states that only 8 reports are kernel-mandatory. The validator validateMandatoryReports checks for these 8 specific report IDs (1, 7, 10, 11, 12, 13, 14, 15).
These test cases should validate the 8 mandatory reports, not 12 total reports:
- TP1 should verify the 8 mandatory reports are present
- TP3's incomplete descriptor test should use 7-of-8 mandatory reports (not 11-of-12)
Note: TP4 mentions "8-mandatory + 5-optional" which would total 13 reports, further highlighting the confusion about the total count.
📝 Proposed corrections
- [ ] **TP1**: `buildForPid(alloc, cfg_with_one_axis, ffb_cfg{kind=pid})`
returns a non-empty descriptor; `validateMandatoryReports(result)`
- returns success (12 reports present).
+ returns success (8 kernel-mandatory reports present).
- [ ] **TP2**: Output bytes are byte-equal to an inline golden reference
(Moza R5 case, pinned by implementer on first successful real-hardware
run when kernel logs show `hid-universal-pidff` FFB init without
`Error initialising force feedback`). Probe Run 2 evidence covers
bustype acceptance + driver binding ONLY — it does not supply a
- working descriptor (probe Run 2's 7-of-12 descriptor caused FFB init
+ working descriptor (probe Run 2's incomplete descriptor caused FFB init
failure + kernel NULL-deref; the implementer must build a complete
- 12-report descriptor per USB HID PID 1.01 §4.x).
+ descriptor with 8 kernel-mandatory reports per USB HID PID 1.01 §4.x).
-- [ ] **TP3**: `validateMandatoryReports` on a hand-crafted 11-of-12
- descriptor (one report ID stripped) returns
+- [ ] **TP3**: `validateMandatoryReports` on a hand-crafted incomplete
+ descriptor (with 7-of-8 mandatory report IDs) returns
`error.IncompletePidDescriptor`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openspec/changes/phase-13-wave-6-pidff/test-plan.md` around lines 23 - 36,
Update the test-plan items TP1, TP2, and TP3 to assert the 8-kernel-mandatory
reports instead of 12: change TP1 to call buildForPid(alloc, cfg_with_one_axis,
ffb_cfg{kind=pid}) and assert validateMandatoryReports(result) succeeds for the
8 specific report IDs (1,7,10,11,12,13,14,15); change TP2’s golden-reference
expectation to be byte-equal for a descriptor containing those 8 mandatory
reports (plus optional ones as defined) rather than assuming 12; and change TP3
to feed validateMandatoryReports an artificially stripped descriptor with 7-of-8
mandatory reports and expect error.IncompletePidDescriptor. Ensure references to
validateMandatoryReports, buildForPid, and error.IncompletePidDescriptor are
updated accordingly.
Comment on lines
+2394
to
+2397
| if (isFieldKey(trimmed, "clone_vid_pid")) { | ||
| if (std.mem.indexOf(u8, trimmed, "=")) |eq| { | ||
| const val = std.mem.trim(u8, trimmed[eq + 1 ..], " \t"); | ||
| clone_vid_pid = std.mem.eql(u8, val, "true"); |
There was a problem hiding this comment.
Handle inline TOML comments when reading clone_vid_pid.
clone_vid_pid = true # opt-in is valid TOML, but this parser compares the whole RHS to "true", so the flag is read as false and the cloned-VID/PID udev rule is skipped.
💡 Suggested fix
if (isFieldKey(trimmed, "clone_vid_pid")) {
if (std.mem.indexOf(u8, trimmed, "=")) |eq| {
- const val = std.mem.trim(u8, trimmed[eq + 1 ..], " \t");
+ const raw = std.mem.trim(u8, trimmed[eq + 1 ..], " \t");
+ const end = std.mem.indexOfScalar(u8, raw, '#') orelse raw.len;
+ const val = std.mem.trim(u8, raw[0..end], " \t");
clone_vid_pid = std.mem.eql(u8, val, "true");
}
}📝 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
| if (isFieldKey(trimmed, "clone_vid_pid")) { | |
| if (std.mem.indexOf(u8, trimmed, "=")) |eq| { | |
| const val = std.mem.trim(u8, trimmed[eq + 1 ..], " \t"); | |
| clone_vid_pid = std.mem.eql(u8, val, "true"); | |
| if (isFieldKey(trimmed, "clone_vid_pid")) { | |
| if (std.mem.indexOf(u8, trimmed, "=")) |eq| { | |
| const raw = std.mem.trim(u8, trimmed[eq + 1 ..], " \t"); | |
| const end = std.mem.indexOfScalar(u8, raw, '#') orelse raw.len; | |
| const val = std.mem.trim(u8, raw[0..end], " \t"); | |
| clone_vid_pid = std.mem.eql(u8, val, "true"); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/install.zig` around lines 2394 - 2397, The parser for clone_vid_pid
(inside the isFieldKey(trimmed, "clone_vid_pid") branch) currently trims the RHS
into val and compares it directly to "true", which fails when an inline TOML
comment follows (e.g., `clone_vid_pid = true # opt-in`). Change the logic that
computes val (the trimmed RHS slice) to strip any inline comment: if
std.mem.indexOf(u8, val, "#") returns a position, re-slice val to
val[0..hashPos] and trim again before comparing; then perform std.mem.eql(u8,
cleaned_val, "true") to set clone_vid_pid. Ensure this update is applied where
val is created and compared.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/release.yml (1)
54-86: Release-path-j1will measurably slow release builds.Forcing
-j1on both the native musl build (line 54) and the cross-target matrix build (line 86) means each release tag triggers serialReleaseSafebuilds forx86_64-linux-muslandaarch64-linux-musl. This is fine as a short-term workaround, but unlike CI test runs, release pipelines are user-visible (publish latency) — please ensure the underlying parallel-build issue is tracked so this can be lifted. Same observation as.github/workflows/e2e.yml.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 54 - 86, The workflow forces serial Zig builds by passing "-j1" in the zig build invocations (seen in the "Generate install artifacts" step that runs "./zig-out/bin/padctl install" and the "Build" step in the "build" job), which slows release builds; remove the explicit "-j1" flags (or replace with a parallel value such as "-j$(nproc)" or omit "-j" to let Zig choose) in both places (the padctl install invocation and the zig build run in the build matrix) and if needed add a short TODO comment referencing the underlying parallel-build issue ID so it can be reverted when fixed..github/workflows/e2e.yml (1)
36-41: Add inline comment referencing Zig issue#22453to explain the-j1workaround.The
-j1flag is consistently applied across all workflows to address Zig cache lock contention (Zig issue#22453). This root cause is already documented inci.yml, but adding the same explanatory comment toe2e.ymland other workflows would improve maintainability and help future contributors understand the trade-off without searching elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e.yml around lines 36 - 41, Add an inline comment above the Zig invocations in this workflow (the lines containing "run: zig build -j1 test-e2e" and "run: zig build -j1 test-integration") that explains the use of the "-j1" workaround and references Zig issue `#22453`; the comment should mirror the explanation already in ci.yml (briefly stating it avoids Zig cache lock contention) so future maintainers understand why parallel builds are disabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/e2e.yml:
- Around line 36-41: Add an inline comment above the Zig invocations in this
workflow (the lines containing "run: zig build -j1 test-e2e" and "run: zig build
-j1 test-integration") that explains the use of the "-j1" workaround and
references Zig issue `#22453`; the comment should mirror the explanation already
in ci.yml (briefly stating it avoids Zig cache lock contention) so future
maintainers understand why parallel builds are disabled.
In @.github/workflows/release.yml:
- Around line 54-86: The workflow forces serial Zig builds by passing "-j1" in
the zig build invocations (seen in the "Generate install artifacts" step that
runs "./zig-out/bin/padctl install" and the "Build" step in the "build" job),
which slows release builds; remove the explicit "-j1" flags (or replace with a
parallel value such as "-j$(nproc)" or omit "-j" to let Zig choose) in both
places (the padctl install invocation and the zig build run in the build matrix)
and if needed add a short TODO comment referencing the underlying parallel-build
issue ID so it can be reverted when fixed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5e09596-3caa-41ac-b440-9eccfb51c559
📒 Files selected for processing (8)
.github/workflows/ci.yml.github/workflows/docs.yml.github/workflows/e2e.yml.github/workflows/install-flow.yml.github/workflows/release.yml.github/workflows/validate.ymlbuild.zigscripts/coverage.sh
✅ Files skipped from review due to trivial changes (3)
- .github/workflows/validate.yml
- scripts/coverage.sh
- .github/workflows/install-flow.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci.yml
- build.zig
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/ci.yml (1)
131-149: Nit: stale step name indistro-check.The step is still named
Run check-all in container, but the container body no longer invokes thecheck-alltarget — it now runs the same three split processes ascheck-matrix. Consider renaming for accuracy and to mirror thecheck-matrixstep name.✏️ Proposed rename
- - name: Run check-all in container + - name: Run all checks in container (serial via separate processes)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 131 - 149, The GitHub Actions step named "Run check-all in container" is stale because the docker command now runs the three split builds like the `check-matrix` step; rename the step to accurately reflect its behavior (e.g. "Run check-matrix in container" or "Run split checks in container") in the `.github/workflows/ci.yml` job `distro-check` so the step label matches the commands that call /tmp/zig... zig build -Dlibusb=false test/test-safe/check-fmt; update the step name string to the new, clearer label.src/test/wave6_pidff_e2e_test.zig (1)
367-379: EAGAIN trigger relies on pipe buffer being ≤128 KiB.The fill loop caps at 128 KiB, which is comfortably above Linux's default 64 KiB pipe buffer, so this works today. But it silently breaks if the runtime ever sees a larger default (custom
pipe-default-size, sysctl tweaks, or a non-default builder env): the loop returns withfilled == 128*1024but the pipe still has free space, the subsequent 2-byte write succeeds, andexpect(fwd.drops_eagain >= 1)fails for a non-bug reason.Filling unconditionally until EAGAIN removes the assumption entirely.
♻️ Proposed refactor
- // Fill the hidraw pipe buffer to capacity (Linux default 64 KiB). + // Fill the hidraw pipe buffer to capacity (write until EAGAIN, regardless of + // the kernel's pipe-default-size). const chunk = [_]u8{0xFF} ** 4096; - var filled: usize = 0; - while (filled < 128 * 1024) { - const n = posix.write(hidraw_fds[1], &chunk) catch break; - filled += n; - } + while (true) { + _ = posix.write(hidraw_fds[1], &chunk) catch |err| switch (err) { + error.WouldBlock => break, + else => return err, + }; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/wave6_pidff_e2e_test.zig` around lines 367 - 379, The test currently assumes the pipe buffer is <=128KiB by looping up to filled < 128*1024; change the fill loop that writes chunk via posix.write to instead loop until the write fails with EAGAIN (i.e., keep writing and accumulate filled using the return value of posix.write, breaking only when posix.write returns an error with errno == EAGAIN or when write returns 0), so that the pipe is genuinely filled regardless of system pipe-default-size; update the code around the chunk, filled, posix.write and hidraw_fds loop so the subsequent writeUhidOutputEvent/drainAndForward and the expect(fwd.drops_eagain >= 1) assertion no longer rely on a fixed 128KiB cap.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 131-149: The GitHub Actions step named "Run check-all in
container" is stale because the docker command now runs the three split builds
like the `check-matrix` step; rename the step to accurately reflect its behavior
(e.g. "Run check-matrix in container" or "Run split checks in container") in the
`.github/workflows/ci.yml` job `distro-check` so the step label matches the
commands that call /tmp/zig... zig build -Dlibusb=false
test/test-safe/check-fmt; update the step name string to the new, clearer label.
In `@src/test/wave6_pidff_e2e_test.zig`:
- Around line 367-379: The test currently assumes the pipe buffer is <=128KiB by
looping up to filled < 128*1024; change the fill loop that writes chunk via
posix.write to instead loop until the write fails with EAGAIN (i.e., keep
writing and accumulate filled using the return value of posix.write, breaking
only when posix.write returns an error with errno == EAGAIN or when write
returns 0), so that the pipe is genuinely filled regardless of system
pipe-default-size; update the code around the chunk, filled, posix.write and
hidraw_fds loop so the subsequent writeUhidOutputEvent/drainAndForward and the
expect(fwd.drops_eagain >= 1) assertion no longer rely on a fixed 128KiB cap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52ab7f93-391e-4565-bdea-59a2c79d72b5
📒 Files selected for processing (2)
.github/workflows/ci.ymlsrc/test/wave6_pidff_e2e_test.zig
BANANASJIM
pushed a commit
that referenced
this pull request
Apr 27, 2026
…flock deadlock workaround) build.zig was registering uhid_output_dispatch_tests and wave6_pidff_e2e_tests as separate b.addTest artifacts sharing src_mod. zig issue #22453 causes a flock AB/BA deadlock between artifacts on the cache manifest, which manifested as a 100+ minute CI hang on PR #166. Inline both test files into src/main.zig's pub const ..._test = @import(...) namespace following the pattern of interpreter_e2e_test, mapper_e2e_test, etc. This compiles them into the main test artifact, eliminating cross-artifact flock contention. Convert @import("src") module-alias imports to relative path imports matching the existing test file convention. Update test assertions to align with the simplified device_instance.zig which no longer carries FfbForwarder or test_physical_hidraw_fd seam (FFB wiring moved to event_loop, Wave 6 T4). Bisected over 4 rounds; T3 commit 0698fb1 (uhid_output_dispatch_test artifact) confirmed as root cause.
BANANASJIM
force-pushed
the
spec/phase-13-wave-6-pidff
branch
from
93a544b to
b47e7d3
Compare
BANANASJIM
pushed a commit
that referenced
this pull request
Apr 27, 2026
…flock deadlock workaround) build.zig was registering uhid_output_dispatch_tests and wave6_pidff_e2e_tests as separate b.addTest artifacts sharing src_mod. zig issue #22453 causes a flock AB/BA deadlock between artifacts on the cache manifest, which manifested as a 100+ minute CI hang on PR #166. Inline both test files into src/main.zig's pub const ..._test = @import(...) namespace following the pattern of interpreter_e2e_test, mapper_e2e_test, etc. This compiles them into the main test artifact, eliminating cross-artifact flock contention. Convert @import("src") module-alias imports to relative path imports matching the existing test file convention. Update test assertions to align with the simplified device_instance.zig which no longer carries FfbForwarder or test_physical_hidraw_fd seam (FFB wiring moved to event_loop, Wave 6 T4). Bisected over 4 rounds; T3 commit 0698fb1 (uhid_output_dispatch_test artifact) confirmed as root cause.
BANANASJIM
pushed a commit
that referenced
this pull request
Apr 27, 2026
build.zig was registering uhid_output_dispatch_tests and wave6_pidff_e2e_tests as separate b.addTest artifacts sharing src_mod. Zig issue #22453 causes a flock AB/BA deadlock between artifacts on the cache manifest, manifesting as a 100+ minute CI hang on PR #166. Inline both test files into src/main.zig's pub const ..._test = @import(...) namespace following the existing convention (interpreter_e2e_test, etc.). Convert the test files' @import("src") module-alias imports to relative path imports matching src/test/interpreter_e2e_test.zig. This change is scoped to ONLY 4 files: build.zig (remove 2 addTest blocks), src/main.zig (add 2 namespace entries), and the 2 test files (rewrite import header). All Wave 6 production code (FfbForwarder wiring, clone_vid_pid parsing, supervisor_uhid_routing_test, etc.) is preserved.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/device_instance.zig (1)
351-361:⚠️ Potential issue | 🟠 MajorDon't infer the physical PID target from
devices[0].Line 360 still falls back to the first opened interface fd. In this file, Lines 35-57 show that interface 0 can be a
UsbrawDeviceor the wrong HID interface, so multi-interface wheels can forward PID bytes to the wrong fd and silently lose FFB. This path should fail closed unless an explicit physical hidraw writer has been resolved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device_instance.zig` around lines 351 - 361, The code currently falls back to using devices[0] for phys_fd which can pick the wrong interface; change the logic so phys_fd does NOT silently fall back to devices[0]. Set phys_fd to opts.test_physical_hidraw_fd orelse -1 and treat -1 as a hard "no physical hidraw" condition (return an error or disable FFB startup) rather than using devices[0]. Update any callers that expect a valid fd from phys_fd to handle the -1 sentinel and fail closed when no explicit physical hidraw writer is provided; reference symbols: devices, phys_fd, opts.test_physical_hidraw_fd.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openspec/changes/phase-13-wave-6-pidff/brief.md`:
- Line 49: The brief incorrectly claims a rate ceiling for the FfbForwarder;
update the T4 line in brief.md so it does not assert an implemented rate ceiling
— either remove "rate ceiling" from the description or append " (future work)" /
"not yet implemented" to the phrase for `FfbForwarder` in the T4 summary,
referencing the new `src/io/ffb_forwarder.zig` and related
`src/supervisor.zig`/`src/io/hidraw.zig` entries to make it clear the rate limit
is not currently enforced.
In `@openspec/changes/phase-13-wave-6-pidff/tasks.md`:
- Around line 511-603: Update T6 to describe the actual pipe-based Layer‑1 test
harness: remove or de-emphasize the /dev/uhid path and UHID gating, and replace
UHID-specific steps with the pipe2 fixtures used by
src/test/wave6_pidff_e2e_test.zig; explicitly state that the test uses
physical_hidraw pipe descriptors passed to DeviceInstance.init (the write-end
used to simulate device output) rather than
UhidSimulator.injectOutput/UHID_CREATE2, update the test registration in
build.zig accordingly, and delete the PADCTL_TEST_REQUIRE_UHID=1 requirement so
the acceptance criteria match the implemented pipe-based tests.
In `@src/io/ffb_forwarder.zig`:
- Around line 82-97: The test hangs because the writer end fds[1] remains open
while draining the pipe, so a subsequent posix.read can block; after performing
the three forwards (FfbForwarder.init / fwd.forward and checking
fwd.writes_total) explicitly close the writer descriptor (posix.close(fds[1]) or
call FfbForwarder.close/cleanup if provided) before doing the multiple
posix.read calls so the reader sees EOF and reads return without blocking;
ensure defer close calls are adjusted (remove double-close) or guard against
closing twice.
In `@src/io/uhid_descriptor.zig`:
- Around line 502-558: buildForPid currently ignores the device.OutputConfig and
emits only a single 1-byte X axis, causing a mismatch with encodeReport; change
buildForPid (notably the function named buildForPid and its current single-axis
preamble) to actually build the same input layout based on the passed
cfg/OutputConfig (buttons, hats, axes, touchpad) instead of discarding cfg:
remove the `_ = cfg`/`_ = ffb_cfg` stubs and either call or extract the existing
input-descriptor generation logic used by encodeReport (or replicate its
button/axis/hat/touchpad emission) while preserving the PID output reports and
INPUT_REPORT_ID; ensure report ID, sizes, and ordering match encodeReport's
expectations so validateMandatoryReports and HID_MAX_DESCRIPTOR checks remain
valid.
---
Duplicate comments:
In `@src/device_instance.zig`:
- Around line 351-361: The code currently falls back to using devices[0] for
phys_fd which can pick the wrong interface; change the logic so phys_fd does NOT
silently fall back to devices[0]. Set phys_fd to opts.test_physical_hidraw_fd
orelse -1 and treat -1 as a hard "no physical hidraw" condition (return an error
or disable FFB startup) rather than using devices[0]. Update any callers that
expect a valid fd from phys_fd to handle the -1 sentinel and fail closed when no
explicit physical hidraw writer is provided; reference symbols: devices,
phys_fd, opts.test_physical_hidraw_fd.
🪄 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: 15838cee-1015-4f56-82ae-c2e7a0bb21cb
📒 Files selected for processing (17)
.github/workflows/ci.ymlbuild.zigopenspec/changes/phase-13-wave-6-pidff/brief.mdopenspec/changes/phase-13-wave-6-pidff/design.mdopenspec/changes/phase-13-wave-6-pidff/tasks.mdopenspec/changes/phase-13-wave-6-pidff/test-plan.mdsrc/cli/install.zigsrc/config/device.zigsrc/device_instance.zigsrc/event_loop.zigsrc/io/ffb_forwarder.zigsrc/io/uhid.zigsrc/io/uhid_descriptor.zigsrc/main.zigsrc/test/supervisor_uhid_routing_test.zigsrc/test/uhid_output_dispatch_test.zigsrc/test/wave6_pidff_e2e_test.zig
✅ Files skipped from review due to trivial changes (2)
- build.zig
- src/main.zig
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/ci.yml
- src/test/uhid_output_dispatch_test.zig
- src/cli/install.zig
| | T1 | `UhidDescriptorBuilder.buildForPid` — PID output collection, 8 kernel-mandatory + 5 optional reports, defensive 8-mandatory validator, byte-exact golden test | `src/io/uhid_descriptor.zig` | ~250 | | ||
| | T2 | VID/PID cloning — `OutputConfig.force_feedback.clone_vid_pid` opt-in; primary UHID card vendor/product overridden from `[device]` when true; udev rules emit per-cloned-VID/PID match | `src/config/device.zig`, `src/device_instance.zig`, `src/cli/install.zig` | ~150 | | ||
| | T3 | `UHID_OUTPUT` event handler — `src/io/uhid.zig` adds `pollOutputReport()` + `onOutputReport(report_id, payload)` callback; payload-parse unit tests | `src/io/uhid.zig`, `src/io/uhid_types.zig` | ~120 | | ||
| | T4 | `FfbForwarder` — write FFB bytes to physical hidraw fd, EAGAIN/EACCES/ENODEV classification, rate ceiling + dropped-packet counter | `src/io/ffb_forwarder.zig` (new), `src/supervisor.zig`, `src/io/hidraw.zig` | ~250 | |
There was a problem hiding this comment.
Don't claim a rate ceiling that isn't implemented yet.
The T4 summary says the forwarder ships with a rate ceiling, but this PR's stated limitations still say there is no active FFB rate ceiling. Please either drop that phrase here or mark it as future work so the brief doesn't overstate the current behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openspec/changes/phase-13-wave-6-pidff/brief.md` at line 49, The brief
incorrectly claims a rate ceiling for the FfbForwarder; update the T4 line in
brief.md so it does not assert an implemented rate ceiling — either remove "rate
ceiling" from the description or append " (future work)" / "not yet implemented"
to the phrase for `FfbForwarder` in the T4 summary, referencing the new
`src/io/ffb_forwarder.zig` and related `src/supervisor.zig`/`src/io/hidraw.zig`
entries to make it clear the rate limit is not currently enforced.
Comment on lines
+82
to
+97
| const fds = try posix.pipe2(.{}); | ||
| defer posix.close(fds[0]); | ||
| defer posix.close(fds[1]); | ||
|
|
||
| var fwd = FfbForwarder.init(fds[1]); | ||
| const payload = [_]u8{0xAA}; | ||
| fwd.forward(.{ .report_id = 0xAA, .data = &payload }); | ||
| fwd.forward(.{ .report_id = 0xAA, .data = &payload }); | ||
| fwd.forward(.{ .report_id = 0xAA, .data = &payload }); | ||
| try testing.expectEqual(@as(u64, 3), fwd.writes_total); | ||
|
|
||
| // Drain pipe | ||
| var buf: [64]u8 = undefined; | ||
| _ = posix.read(fds[0], &buf) catch {}; | ||
| _ = posix.read(fds[0], &buf) catch {}; | ||
| _ = posix.read(fds[0], &buf) catch {}; |
There was a problem hiding this comment.
This test can still hang on a blocking pipe.
After three 1-byte writes, the first read() can consume all buffered bytes. The second read then blocks because fds[1] stays open until scope exit. That makes this test flaky/hanging in the same way the PR notes call out elsewhere.
Suggested fix
- const fds = try posix.pipe2(.{});
+ const fds = try posix.pipe2(.{ .NONBLOCK = true });
@@
- // Drain pipe
- var buf: [64]u8 = undefined;
- _ = posix.read(fds[0], &buf) catch {};
- _ = posix.read(fds[0], &buf) catch {};
- _ = posix.read(fds[0], &buf) catch {};
+ var buf: [64]u8 = undefined;
+ while (true) {
+ _ = posix.read(fds[0], &buf) catch |err| switch (err) {
+ error.WouldBlock => break,
+ else => return err,
+ };
+ }📝 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
| const fds = try posix.pipe2(.{}); | |
| defer posix.close(fds[0]); | |
| defer posix.close(fds[1]); | |
| var fwd = FfbForwarder.init(fds[1]); | |
| const payload = [_]u8{0xAA}; | |
| fwd.forward(.{ .report_id = 0xAA, .data = &payload }); | |
| fwd.forward(.{ .report_id = 0xAA, .data = &payload }); | |
| fwd.forward(.{ .report_id = 0xAA, .data = &payload }); | |
| try testing.expectEqual(@as(u64, 3), fwd.writes_total); | |
| // Drain pipe | |
| var buf: [64]u8 = undefined; | |
| _ = posix.read(fds[0], &buf) catch {}; | |
| _ = posix.read(fds[0], &buf) catch {}; | |
| _ = posix.read(fds[0], &buf) catch {}; | |
| const fds = try posix.pipe2(.{ .NONBLOCK = true }); | |
| defer posix.close(fds[0]); | |
| defer posix.close(fds[1]); | |
| var fwd = FfbForwarder.init(fds[1]); | |
| const payload = [_]u8{0xAA}; | |
| fwd.forward(.{ .report_id = 0xAA, .data = &payload }); | |
| fwd.forward(.{ .report_id = 0xAA, .data = &payload }); | |
| fwd.forward(.{ .report_id = 0xAA, .data = &payload }); | |
| try testing.expectEqual(`@as`(u64, 3), fwd.writes_total); | |
| var buf: [64]u8 = undefined; | |
| while (true) { | |
| _ = posix.read(fds[0], &buf) catch |err| switch (err) { | |
| error.WouldBlock => break, | |
| else => return err, | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/io/ffb_forwarder.zig` around lines 82 - 97, The test hangs because the
writer end fds[1] remains open while draining the pipe, so a subsequent
posix.read can block; after performing the three forwards (FfbForwarder.init /
fwd.forward and checking fwd.writes_total) explicitly close the writer
descriptor (posix.close(fds[1]) or call FfbForwarder.close/cleanup if provided)
before doing the multiple posix.read calls so the reader sees EOF and reads
return without blocking; ensure defer close calls are adjusted (remove
double-close) or guard against closing twice.
Merged
3 tasks
BANANASJIM
pushed a commit
that referenced
this pull request
Apr 28, 2026
…diagnostics Wave 6 PR #166's `check` matrix hits a 6-hour runner-level timeout without producing log artifacts (GHA strips logs from in-progress cancelled steps). Wrap each `zig build test*` invocation in `timeout 600` so the step finalizes at 10min with logs uploaded; add `--summary all` so Zig's test runner prints every passing test name, making the last test before hang visible. Forensic-only — no production behavior change.
BANANASJIM
added a commit
that referenced
this pull request
Apr 28, 2026
…cs (#182) * fix(ci): wrap test steps in `timeout 600` + `--summary all` for hang diagnostics Wave 6 PR #166's `check` matrix hits a 6-hour runner-level timeout without producing log artifacts (GHA strips logs from in-progress cancelled steps). Wrap each `zig build test*` invocation in `timeout 600` so the step finalizes at 10min with logs uploaded; add `--summary all` so Zig's test runner prints every passing test name, making the last test before hang visible. Forensic-only — no production behavior change. * fix(ci): use zig build check-all instead of split sub-steps The 3-step split tried zig build fmt --check, but fmt is not a build step (check-fmt is) and --check is not a valid zig build flag. Result: 5 CI jobs failed with "unrecognized argument: '--check'". zig build check-all already chains test + test-safe + check-fmt per build.zig:165-168. Wrap the single invocation with timeout 600 + --summary all instead. Single source of truth in build.zig; no drift risk between workflow and build script. --------- Co-authored-by: openplan-implementer <openplan-implementer@openplan.local>
BANANASJIM
force-pushed
the
spec/phase-13-wave-6-pidff
branch
from
b47e7d3 to
47a784e
Compare
BANANASJIM
pushed a commit
that referenced
this pull request
Apr 28, 2026
…flock deadlock workaround) build.zig was registering uhid_output_dispatch_tests and wave6_pidff_e2e_tests as separate b.addTest artifacts sharing src_mod. zig issue #22453 causes a flock AB/BA deadlock between artifacts on the cache manifest, which manifested as a 100+ minute CI hang on PR #166. Inline both test files into src/main.zig's pub const ..._test = @import(...) namespace following the pattern of interpreter_e2e_test, mapper_e2e_test, etc. This compiles them into the main test artifact, eliminating cross-artifact flock contention. Convert @import("src") module-alias imports to relative path imports matching the existing test file convention. Update test assertions to align with the simplified device_instance.zig which no longer carries FfbForwarder or test_physical_hidraw_fd seam (FFB wiring moved to event_loop, Wave 6 T4). Bisected over 4 rounds; T3 commit 0698fb1 (uhid_output_dispatch_test artifact) confirmed as root cause.
BANANASJIM
pushed a commit
that referenced
this pull request
Apr 28, 2026
build.zig was registering uhid_output_dispatch_tests and wave6_pidff_e2e_tests as separate b.addTest artifacts sharing src_mod. Zig issue #22453 causes a flock AB/BA deadlock between artifacts on the cache manifest, manifesting as a 100+ minute CI hang on PR #166. Inline both test files into src/main.zig's pub const ..._test = @import(...) namespace following the existing convention (interpreter_e2e_test, etc.). Convert the test files' @import("src") module-alias imports to relative path imports matching src/test/interpreter_e2e_test.zig. This change is scoped to ONLY 4 files: build.zig (remove 2 addTest blocks), src/main.zig (add 2 namespace entries), and the 2 test files (rewrite import header). All Wave 6 production code (FfbForwarder wiring, clone_vid_pid parsing, supervisor_uhid_routing_test, etc.) is preserved.
5 tasks
BANANASJIM
added a commit
that referenced
this pull request
Apr 28, 2026
…ns (#185) Y1's timeout 600 instrumentation works but Zig's test runner buffers stdout; SIGTERM doesn't flush, so the 10min-killed process produces zero log output (PR #166 commit 47a784e first run had timeout=124 with empty log between command-start and SIGTERM). Pipe each instrumented invocation through tee <name>.log so output accumulates incrementally and survives SIGTERM. Pattern matches the tsan step which has used 2>&1 | tee tsan-build.log since Wave 6. set -eo pipefail ensures timeout's exit code propagates correctly through the pipe. Co-authored-by: openplan-implementer <openplan-implementer@openplan.local>
BANANASJIM
force-pushed
the
spec/phase-13-wave-6-pidff
branch
from
47a784e to
0cc174c
Compare
BANANASJIM
pushed a commit
that referenced
this pull request
Apr 28, 2026
…flock deadlock workaround) build.zig was registering uhid_output_dispatch_tests and wave6_pidff_e2e_tests as separate b.addTest artifacts sharing src_mod. zig issue #22453 causes a flock AB/BA deadlock between artifacts on the cache manifest, which manifested as a 100+ minute CI hang on PR #166. Inline both test files into src/main.zig's pub const ..._test = @import(...) namespace following the pattern of interpreter_e2e_test, mapper_e2e_test, etc. This compiles them into the main test artifact, eliminating cross-artifact flock contention. Convert @import("src") module-alias imports to relative path imports matching the existing test file convention. Update test assertions to align with the simplified device_instance.zig which no longer carries FfbForwarder or test_physical_hidraw_fd seam (FFB wiring moved to event_loop, Wave 6 T4). Bisected over 4 rounds; T3 commit 0698fb1 (uhid_output_dispatch_test artifact) confirmed as root cause.
BANANASJIM
pushed a commit
that referenced
this pull request
Apr 28, 2026
build.zig was registering uhid_output_dispatch_tests and wave6_pidff_e2e_tests as separate b.addTest artifacts sharing src_mod. Zig issue #22453 causes a flock AB/BA deadlock between artifacts on the cache manifest, manifesting as a 100+ minute CI hang on PR #166. Inline both test files into src/main.zig's pub const ..._test = @import(...) namespace following the existing convention (interpreter_e2e_test, etc.). Convert the test files' @import("src") module-alias imports to relative path imports matching src/test/interpreter_e2e_test.zig. This change is scoped to ONLY 4 files: build.zig (remove 2 addTest blocks), src/main.zig (add 2 namespace entries), and the 2 test files (rewrite import header). All Wave 6 production code (FfbForwarder wiring, clone_vid_pid parsing, supervisor_uhid_routing_test, etc.) is preserved.
4 tasks
… e2e) Bundles T1-T7 of openspec/changes/phase-13-wave-6-pidff: - T1: PID HID descriptor builder + 12-of-12 mandatory validator - T2: VID/PID cloning + per-device udev permission rule (clone_vid_pid) - T3: parse UHID_OUTPUT events + FfbForwarder dispatch hook - T4: FfbForwarder hidraw write + EAGAIN drop + dispatch wiring - T5: [output.force_feedback] schema + validate matrix - T6: wave6_pidff_e2e_test (descriptor -> UHID_OUTPUT -> forwarder) - CI: TP33 NONBLOCK, 3-process check-all, Y1 timeout, tee logs refs: openspec/changes/phase-13-wave-6-pidff/tasks.md
BANANASJIM
force-pushed
the
spec/phase-13-wave-6-pidff
branch
from
0cc174c to
3da1684
Compare
4 tasks
BANANASJIM
added a commit
that referenced
this pull request
May 1, 2026
* docs: comprehensive feature-coverage audit + close gaps for 2026-04-28..2026-05-01 Closes the doc backlog for features merged since the last user-facing drift sync (PR #184, 2026-04-28): - Wave 6 HID PID FFB passthrough (PR #166): [output.force_feedback] backend/kind/clone_vid_pid reference + validation matrix + hid-pidff kernel requirement note + racing-wheel TOML example - padctl status active mapping (PR #199): document the STATUS device=... state=... mapping=... response triple - padctl --validate auto-detect (PR #204): document mapping vs device detection by [device]-header heuristic + repeatable flag - button_group size limit (PR #177): document source.size 1..=8 and bit_index < size*8 constraints in device-config.md - layer tap=macro reject (PR #176): document error.LayerTapCannotBeMacro constraint in mapping-config.md - chord_index cross-link: surface chord_switch from mapping-config.md for users looking up the field name - code-contributions.md: 'fixtures are SSOT, e2e tests must use parseFile/@embedfile' rule (drift-proofing carry-over from PR #193) chord_switch (PR #201) docs are tracked in sibling PR docs/chord-switch-example-and-docs. * docs: fix H1+M1 from PR #209 reviewer H1 [Option B]: retire last inline vader5_toml literal in interpreter_e2e_test.zig — remove parseString-based test that duplicated the existing parseFile-based test; delete the 28-line vader5_toml const. code-contributions.md updated to reflect that no inline device literals remain in the test suite (PR #193 began the cleanup, this PR finishes it). M1: chord_index doc said 'integer (>=1)' but schema accepts u8 with no >=1 validation. Relax to 'integer (0-255)'; clarify that 0 or omit means unselectable via chord, matching actual schema behavior. * docs: fix Qodo F3 — button_group size is runtime warn-skip not compile reject Doc claimed 'rejected at compile time'; src/core/interpreter.zig compileReport emits std.log.warn and returns the report unprocessed. Reword to match actual runtime behavior. --------- Co-authored-by: openplan-implementer <openplan-implementer@openplan.local>
4 tasks
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.
Summary
Implements Phase 13 Wave 6 — HID PID force-feedback passthrough through padctl's UHID virtual device path. Builds on Wave 3 (UHID routing) + Wave 5 (canary) and closes ADR-015 Stage 3 architectural path. Targets racing-wheel FFB support via kernel
hid-universal-pidffmodalias binding.Architecture
Kernel
hid-universal-pidffbinds to padctl's UHID device (whenclone_vid_pidadvertises a wheel-class VID/PID) and emitsUHID_OUTPUTevents with FFB command bytes. padctl forwards those bytes byte-for-byte to the physical wheel's hidraw fd. No FFB remapping or interpretation — pure passthrough per ADR-015 Option (a) constraint.Tasks landed
Probe-derived empirical foundation
Wave 6 probe (separate research at `research/wave6-pidff-probe`, parallel PR #161) ran on kernel 6.18.9 with two virtual-UHID variants:
Probe Run 2's incomplete descriptor caused `pidff_find_reports -ENODEV` and a kernel NULL-deref. T1's descriptor includes all 8 mandatory reports per kernel mainline to avoid this.
Test plan
Known limitations / non-goals
References
Summary by CodeRabbit
New Features
Tests
Documentation
Chores