refactor(haptics): one copy of each thing every rumble path was transcribing #51

Merged

enricobuehler merged 2 commits from worktree-haptics-m12-dry into main 2026-08-04 21:11:50 +00:00

Conversation 0 Commits 2 Files Changed 24

+705 -221

enricobuehler commented 2026-08-04 20:53:19 +00:00

Owner

Copy Link

Copy Source

Haptics sweep M12 — DRY, docs & dead code (design/haptics-sweep-2026-08-03.md). Closes S1–S9, S11, T2, T3.

Filed as cleanup. Three of the duplications were hiding real defects.

The three that were actually broken

S4 — the UHID event ABI existed five times, and one copy was wrong. Every UHID gamepad backend (DualSense, DualShock 4, Switch Pro, Steam Controller, Steam Controller 2) carried a verbatim copy of the kernel's constants plus its own put_cstr. They had already drifted: switch_pro lacks the SET_REPORT pair entirely, and steam_controller read a fixed 16-byte SET_REPORT window instead of the event's size field.

That is wrong in both directions. A longer report is silently truncated — and a shorter one leaves the parser reading whatever the reused event buffer still holds past the payload, so a rumble report acts on numbers the game never wrote. Every sibling that parses SET_REPORT already read the size field.

S11 (first half) — a dead FF-id fallback. ff-core's input_ff_upload picks a free effect slot and writes it into the effect before uinput forwards the request, so id == -1 cannot arrive. The fallback allocated from a local counter, which is also the wrong answer — the kernel owns that id space. Removed, debug_assert in its place.

S7 — Apple silently dropped weak rumble. hidByte took the top byte with no non-zero floor, so every amplitude below 0x0100 rendered as exactly nothing. Android has floored it at 1 forever; this was the odd one out. The same converter also existed twice byte-identically inside one Gradle module.

The rest

S2	DS5 layout gets named offsets (dualsense_proto::out_report) documenting all three transport bases — USB 0, SDL payload −1, Bluetooth +2. See the caveat below.
S3	PUNKTFUNK_HID_EFFECT_MAX now sizes the struct it describes; the header emits uint8_t effect[PUNKTFUNK_HID_EFFECT_MAX] instead of a second literal 11.
S5	No in-tree caller sets min_pulse_ms; and the macOS-DualSense-over-BT keepalive the doc cited cannot be served by keepalive_ms — that renderer skips writes whose levels are unchanged, so it would swallow the engine's re-emit. It keeps its own 0.9 s keepalive instead. Docs now say so.
S6	Three ×257-vs-<<8 doc comments corrected. The scaling is fine — both round-trip to 255 — the docs were wrong in both directions.
S8	backstop_ms.max(160) deleted: the engine's backstop() floors at 500, so it was unreachable.
S1	TrackpadHaptic marked staged scaffolding — the tag is on a shipped wire, so deleting the variant would not reclaim it, only lose the decoder.
S9	RumbleRenderer's header described a Policy type, a staleness watchdog and a ttlMs that no longer exist; policy was stored and never read. Removed.
T2	Ds5Feedback had no tests at all, despite being the writer that hand-shifts every offset.
T3	Android rumble JNI packing now tested on both sides, and MAX_PADS <= 16 is a compile-time assertion rather than a comment.

Two corrections to the findings

S11's second half is not a defect. The "unreachable forwarding branch" is reachable and load-bearing: clients/session/src/main.rs:274 calls set_forwarding unconditionally on every params-build — its own comment explains why (browse mode reuses one service across launches) — so Ctl::Forwarding routinely arrives unchanged, and that early-out is what prevents a redundant sync_open + Valve-HIDAPI cycle every launch. Deleting it would have added churn. Left alone.

S2 cannot be fully single-sourced, and I have not pretended otherwise. The finding says to "index both Rust sites off" shared offsets. pf-inject and pf-client-core do not depend on each other, and their only shared crate is punktfunk-core — which states outright that platform-specific code lives outside it. A DualSense report layout does not belong there. So the parser owns the named module, and Ds5Feedback derives its offsets by explicit subtraction (11 - REPORT_ID_LEN) with a test pinning the −1 relationship. Worth knowing: getting that shift wrong doesn't just fail the test, it fails to compile (unconditional_panic, index 47 into a 47-byte array).

Verification

pf-inject	clippy -D warnings 0, 91 tests
pf-client-core + punktfunk-core	clippy 0, 437 tests (amd64 container)
punktfunk-client-android	7 tests
Android :kit:	6 tests
Apple	swift build clean, 189 tests / 0 failures
Rust	cargo fmt --all --check clean

Every new test probed by reverting its fix: restoring the fixed SET_REPORT window fails 3, breaking the pack shift fails 3, dropping the amplitude floor fails 1, a wrong DS5 offset fails the pin (or refuses to compile).

Note for whoever merges

PR #45 (M10) adds TRIGGER_EFFECT_MAX to quic/datagram.rs — the wire-side 11. This PR's S3 work uses PUNKTFUNK_HID_EFFECT_MAX, the C-ABI-side 11. They are different layers and both are legitimate, but after both land it is worth one look to decide whether they should reference each other.

Not verified

On-glass. The SET_REPORT fix in particular wants a real Steam Controller: it changes which bytes reach parse_steam_output, which is exactly the path a physical pad exercises.

Haptics sweep **M12 — DRY, docs & dead code** (`design/haptics-sweep-2026-08-03.md`). Closes **S1–S9, S11, T2, T3**. Filed as cleanup. Three of the duplications were hiding real defects. ## The three that were actually broken **S4 — the UHID event ABI existed five times, and one copy was wrong.** Every UHID gamepad backend (DualSense, DualShock 4, Switch Pro, Steam Controller, Steam Controller 2) carried a verbatim copy of the kernel's constants plus its own `put_cstr`. They had already drifted: `switch_pro` lacks the SET_REPORT pair entirely, and `steam_controller` read a **fixed 16-byte** SET_REPORT window instead of the event's `size` field. That is wrong in both directions. A longer report is silently truncated — and a *shorter* one leaves the parser reading whatever the reused event buffer still holds past the payload, so a rumble report acts on numbers the game never wrote. Every sibling that parses SET_REPORT already read the size field. **S11 (first half) — a dead FF-id fallback.** ff-core's `input_ff_upload` picks a free effect slot and writes it into the effect *before* uinput forwards the request, so `id == -1` cannot arrive. The fallback allocated from a local counter, which is also the wrong answer — the kernel owns that id space. Removed, `debug_assert` in its place. **S7 — Apple silently dropped weak rumble.** `hidByte` took the top byte with no non-zero floor, so every amplitude below `0x0100` rendered as *exactly nothing*. Android has floored it at 1 forever; this was the odd one out. The same converter also existed twice byte-identically inside one Gradle module. ## The rest | | | |---|---| | **S2** | DS5 layout gets named offsets (`dualsense_proto::out_report`) documenting all three transport bases — USB `0`, SDL payload `−1`, Bluetooth `+2`. See the caveat below. | | **S3** | `PUNKTFUNK_HID_EFFECT_MAX` now sizes the struct it describes; the header emits `uint8_t effect[PUNKTFUNK_HID_EFFECT_MAX]` instead of a second literal `11`. | | **S5** | No in-tree caller sets `min_pulse_ms`; and the macOS-DualSense-over-BT keepalive the doc cited **cannot** be served by `keepalive_ms` — that renderer skips writes whose levels are unchanged, so it would swallow the engine's re-emit. It keeps its own 0.9 s keepalive instead. Docs now say so. | | **S6** | Three ×257-vs-`<<8` doc comments corrected. The scaling is fine — both round-trip to 255 — the docs were wrong in *both* directions. | | **S8** | `backstop_ms.max(160)` deleted: the engine's `backstop()` floors at 500, so it was unreachable. | | **S1** | `TrackpadHaptic` marked staged scaffolding — the tag is on a shipped wire, so deleting the variant would not reclaim it, only lose the decoder. | | **S9** | `RumbleRenderer`'s header described a `Policy` type, a staleness watchdog and a `ttlMs` that no longer exist; `policy` was stored and never read. Removed. | | **T2** | `Ds5Feedback` had no tests at all, despite being the writer that hand-shifts every offset. | | **T3** | Android rumble JNI packing now tested on **both** sides, and `MAX_PADS <= 16` is a compile-time assertion rather than a comment. | ## Two corrections to the findings **S11's second half is not a defect.** The "unreachable forwarding branch" is reachable and load-bearing: `clients/session/src/main.rs:274` calls `set_forwarding` *unconditionally* on every params-build — its own comment explains why (browse mode reuses one service across launches) — so `Ctl::Forwarding` routinely arrives unchanged, and that early-out is what prevents a redundant `sync_open` + Valve-HIDAPI cycle every launch. Deleting it would have added churn. Left alone. **S2 cannot be fully single-sourced, and I have not pretended otherwise.** The finding says to "index both Rust sites off" shared offsets. `pf-inject` and `pf-client-core` do not depend on each other, and their only shared crate is `punktfunk-core` — which states outright that platform-specific code lives outside it. A DualSense report layout does not belong there. So the parser owns the named module, and `Ds5Feedback` **derives** its offsets by explicit subtraction (`11 - REPORT_ID_LEN`) with a test pinning the `−1` relationship. Worth knowing: getting that shift wrong doesn't just fail the test, it fails to *compile* (`unconditional_panic`, index 47 into a 47-byte array). ## Verification | | | |---|---| | `pf-inject` | clippy `-D warnings` **0**, **91 tests** | | `pf-client-core` + `punktfunk-core` | clippy **0**, **437 tests** (amd64 container) | | `punktfunk-client-android` | **7 tests** | | Android `:kit:` | **6 tests** | | Apple | `swift build` clean, **189 tests / 0 failures** | | Rust | `cargo fmt --all --check` clean | Every new test probed by reverting its fix: restoring the fixed SET_REPORT window fails 3, breaking the pack shift fails 3, dropping the amplitude floor fails 1, a wrong DS5 offset fails the pin (or refuses to compile). ## Note for whoever merges **PR #45 (M10) adds `TRIGGER_EFFECT_MAX` to `quic/datagram.rs`** — the wire-side 11. This PR's S3 work uses `PUNKTFUNK_HID_EFFECT_MAX`, the C-ABI-side 11. They are different layers and both are legitimate, but after both land it is worth one look to decide whether they should reference each other. ## Not verified On-glass. The SET_REPORT fix in particular wants a real Steam Controller: it changes which bytes reach `parse_steam_output`, which is exactly the path a physical pad exercises.

enricobuehler added 1 commit 2026-08-04 20:53:20 +00:00

refactor(haptics): one copy of each thing every rumble path was transcribing ... 42a0dd52be

Twelve findings from the sweep's DRY/docs/dead-code tail. Most are small; three found
real defects hiding behind the duplication.

**The UHID event ABI existed five times.** Every UHID gamepad backend — DualSense,
DualShock 4, Switch Pro, Steam Controller, Steam Controller 2 — carried its own verbatim
copy of the kernel's constants plus its own `put_cstr`, and they had already drifted:
`switch_pro` was missing the SET_REPORT pair entirely, and `steam_controller` read a
FIXED 16-byte SET_REPORT window instead of the event's own `size`. That last one is a
bug in both directions — a longer report was truncated, and a shorter one had the parser
reading whatever the reused event buffer still held past the payload, i.e. acting on
rumble values the game never wrote. Now one `uhid_abi` module owns the numbers plus the
two accessors that are easy to get subtly wrong, with tests on exactly that.

**A dead force-feedback id fallback.** ff-core's `input_ff_upload` picks a free effect
slot and writes it into the effect BEFORE uinput forwards the request, so the `id == -1`
branch could never run — and allocating from a local counter would have been the wrong
answer anyway, since the kernel owns that id space. Removed, with a `debug_assert` where
it stood.

**Apple's HID path silently dropped weak rumble.** `hidByte` took the top byte with no
non-zero floor, so every amplitude below 0x0100 rendered as exactly nothing. Android has
always floored it at 1; this was the odd one out. That converter also existed twice
byte-identically inside one Gradle module — now one `wireAmplitudeToByte`.

Also: the DS5 output-report layout gets named offsets (`dualsense_proto::out_report`)
documenting all three transport bases — USB 0, SDL payload −1, Bluetooth +2 — since the
differing bases are transport-forced, not drift. `pf-client-core` cannot import them (it
and `pf-inject` do not depend on each other, and a DualSense layout has no business in
`punktfunk-core`, their only shared crate), so its copy now DERIVES its offsets by
explicit subtraction and a test pins the relationship. `PUNKTFUNK_HID_EFFECT_MAX` sizes
the struct it describes instead of a second literal 11 — the header now emits
`uint8_t effect[PUNKTFUNK_HID_EFFECT_MAX]`. The rumble policy engine's `min_pulse_ms`
and `keepalive_ms` docs stop naming cases nothing implements: no in-tree caller sets
`min_pulse_ms`, and the macOS DualSense-over-BT keepalive the doc cited CANNOT be served
by the quirk, because that renderer skips writes whose levels are unchanged and would
swallow the engine's re-emit — it keeps its own keepalive instead. `TrackpadHaptic` is
marked as staged scaffolding (the tag is on a shipped wire; removing the variant would
not reclaim it). Three ×257-vs-`<<8` doc comments corrected — the scaling itself is fine,
both round-trip to 255. `backstop_ms.max(160)` deleted as unreachable (the engine floors
at 500). New tests for `Ds5Feedback` and for the Android rumble JNI packing on BOTH sides,
with `MAX_PADS <= 16` now a compile-time assertion rather than a comment.

Closes S1-S9, S11, T2, T3 (design/haptics-sweep-2026-08-03.md M12).

S11's second half is NOT a defect and was left alone: `clients/session/src/main.rs`
calls `set_forwarding` unconditionally on every params-build (its own comment explains
why — browse mode reuses one service across launches), so `Ctl::Forwarding` routinely
arrives unchanged and that early-out is what stops a redundant `sync_open` + Valve-HIDAPI
cycle each launch.

Verified: pf-inject clippy -D warnings 0 / 91 tests; pf-client-core + punktfunk-core
clippy 0 / 437 tests (amd64 container); punktfunk-client-android 7 tests; Android :kit:
6 tests; Apple swift build + 189 tests / 0 failures; cargo fmt --all --check clean. Each
new test probed by reverting its fix — the fixed SET_REPORT window fails 3, a broken pack
shift fails 3, dropping the amplitude floor fails 1, and a wrong DS5 offset either fails
the pin or refuses to compile.

enricobuehler added 1 commit 2026-08-04 21:11:32 +00:00

Merge remote-tracking branch 'origin/main' into worktree-haptics-m12-dry ... 92f617a989

# Conflicts:
#	clients/android/kit/src/main/kotlin/io/unom/punktfunk/kit/GamepadFeedback.kt
#	crates/pf-client-core/src/gamepad.rs

enricobuehler marked the pull request as ready for review 2026-08-04 21:11:36 +00:00

enricobuehler merged commit 2d223274fc into main 2026-08-04 21:11:50 +00:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

No items

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

FernandoMX enricobuehler (Enrico Bühler) kszlim nannku neoney sassycrown

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: unom/punktfunk#51