fix(client/android): rumble survives a vibrator fault, and an unplug stops leaking #35

Merged

enricobuehler merged 2 commits from worktree-haptics-m5-android into main 2026-08-04 21:02:18 +00:00

Conversation 0 Commits 2 Files Changed 7

+361 -56

enricobuehler commented 2026-08-04 16:15:03 +00:00

Owner

Copy Link

Copy Source

Workstream M5 of the haptics sweep — the Android feedback path. Closes B16, B24, B25, R15.

Every one of these fails silently, which is why none of them showed up as a bug report.

B16 — one throw and rumble is gone for the session

The pf-rumble poll thread called Vibrator.cancel() unguarded while every call around it was already wrapped. An unchecked throw — DeadSystemRuntimeException, or the RuntimeException a dying service wraps a RemoteException in — unwound the thread. running stayed true, so nothing noticed and nothing restarted it.

Guarding the two bare cancel() calls is not sufficient on its own: the binder calls that bind a vibrator (vibratorManager, vibratorIds, hasAmplitudeControl) can throw exactly the same way. So the loop itself now survives a failed render, and the same guard covers the pf-hidout thread, which had the identical shape.

B24 — a stop that was never written, treated as one that landed

The DualSense capture disarmed its backstop timer before the stop write, on a queue that discarded failed submits without saying so. A dropped stop left the motors running with nothing scheduled to retry — and a USB pad holds its last level until told zero.

Writes now report whether they were accepted; the backstop is disarmed only once the stop is on its way, and re-arms if its own write is refused.

B25 — the eviction rule dropped one-shots

Overflow policy was "drop the oldest". Right for rumble (re-sent continuously, a lost frame returns in milliseconds), wrong for lightbar colours, player-LED masks and trigger effects, which the host sends once on change and never repeats.

Eviction is now driven by an explicit key from the caller. Not by report id — that was the trap: every DualSense output report carries the same id and differs only in its valid_flag bytes, so an id-keyed rule would let a rumble supersede a lightbar, which is this same bug by another route.

R15 — an unplug leaked the connection and the receiver

The link only signalled the drop; neither capture released anything. Interfaces stayed claimed (so the pad could not return to Android's own input stack) and a re-plug overwrote the field holding the detach receiver, stranding one live for the rest of the process.

Captures now release the transport; stop() is safe to call from the callback it arrives on (the reader thread must not join itself); a close is reported exactly once however many detectors see it; and a reader that could not queue a single request reports itself down instead of leaving the owner waiting on a capture that never streams.

---

Verification

• :kit:testDebugUnitTest — 46 pass / 0 fail (39 baseline + 7 new), :app:compileDebugKotlin clean.
• Non-vacuity proven: restoring the old blind newest-wins policy fails 4 of the 7 new tests, including the headline "one-shot survives a rumble storm".
• Second commit wires :kit:testDebugUnitTest into android.yml — it was running nowhere. The workflow only assembled, and the screenshot workflow runs :app's tests, so the kit's 39 existing tests could go red unnoticed.

Not verified

No on-glass run. B16's fault needs a dying system_server, and R15's needs a physical unplug/re-plug cycle — neither is reachable from a JVM unit test, and the kit module has no Robolectric. The queue policy (B25) is the part that is genuinely covered.

One fix caught during self-review rather than by a test: Sc2Capture.onLinkClosed is shared by both the USB and BLE links, so an unconditional usb.stop() would have torn down USB when BLE dropped. It now reads which link was live before clearing it.

Workstream **M5** of the haptics sweep — the Android feedback path. Closes **B16, B24, B25, R15**. Every one of these fails silently, which is why none of them showed up as a bug report. ### B16 — one throw and rumble is gone for the session The `pf-rumble` poll thread called `Vibrator.cancel()` unguarded while every call around it was already wrapped. An unchecked throw — `DeadSystemRuntimeException`, or the `RuntimeException` a dying service wraps a `RemoteException` in — unwound the thread. `running` stayed `true`, so nothing noticed and nothing restarted it. Guarding the two bare `cancel()` calls is **not sufficient on its own**: the binder calls that *bind* a vibrator (`vibratorManager`, `vibratorIds`, `hasAmplitudeControl`) can throw exactly the same way. So the loop itself now survives a failed render, and the same guard covers the `pf-hidout` thread, which had the identical shape. ### B24 — a stop that was never written, treated as one that landed The DualSense capture disarmed its backstop timer *before* the stop write, on a queue that discarded failed submits without saying so. A dropped stop left the motors running with nothing scheduled to retry — and a USB pad holds its last level until told zero. Writes now report whether they were accepted; the backstop is disarmed only once the stop is on its way, and re-arms if its own write is refused. ### B25 — the eviction rule dropped one-shots Overflow policy was "drop the oldest". Right for rumble (re-sent continuously, a lost frame returns in milliseconds), wrong for lightbar colours, player-LED masks and trigger effects, which the host sends once on change and never repeats. Eviction is now driven by an explicit key from the caller. **Not by report id** — that was the trap: every DualSense output report carries the *same* id and differs only in its `valid_flag` bytes, so an id-keyed rule would let a rumble supersede a lightbar, which is this same bug by another route. ### R15 — an unplug leaked the connection and the receiver The link only *signalled* the drop; neither capture released anything. Interfaces stayed claimed (so the pad could not return to Android's own input stack) and a re-plug overwrote the field holding the detach receiver, stranding one live for the rest of the process. Captures now release the transport; `stop()` is safe to call from the callback it arrives on (the reader thread must not join itself); a close is reported exactly once however many detectors see it; and a reader that could not queue a single request reports itself down instead of leaving the owner waiting on a capture that never streams. --- ### Verification - `:kit:testDebugUnitTest` — **46 pass / 0 fail** (39 baseline + 7 new), `:app:compileDebugKotlin` clean. - **Non-vacuity proven**: restoring the old blind newest-wins policy fails 4 of the 7 new tests, including the headline "one-shot survives a rumble storm". - Second commit wires `:kit:testDebugUnitTest` into `android.yml` — **it was running nowhere**. The workflow only assembled, and the screenshot workflow runs `:app`'s tests, so the kit's 39 existing tests could go red unnoticed. ### Not verified No on-glass run. B16's fault needs a dying `system_server`, and R15's needs a physical unplug/re-plug cycle — neither is reachable from a JVM unit test, and the kit module has no Robolectric. The queue policy (B25) is the part that is genuinely covered. One fix caught during self-review rather than by a test: `Sc2Capture.onLinkClosed` is shared by **both** the USB and BLE links, so an unconditional `usb.stop()` would have torn down USB when BLE dropped. It now reads which link was live before clearing it.

enricobuehler added 2 commits 2026-08-04 16:15:06 +00:00

fix(client/android): rumble survives a vibrator fault, and an unplug stops leaking ... e2faecfd42

Four faults in the Android feedback path, all of them silent.

Rumble stopped for the rest of the session if one vibrator call threw. The
poll thread called cancel() unguarded while every call around it was already
wrapped, so an unchecked throw — DeadSystemRuntimeException, or the
RuntimeException a dying service wraps a RemoteException in — unwound the
thread. `running` stayed true, so nothing noticed it was gone and nothing
restarted it. Guarding the two bare cancels is not enough on its own: the
binder calls that bind a vibrator can throw just the same, so the loop itself
now survives a failed render, and the same guard covers the hidout thread.

A rumble stop that was never written was treated as one that landed. The
DualSense capture disarmed its backstop timer *before* the write, on a queue
that discarded failed submits without saying so, so a dropped stop left the
motors running with nothing scheduled to try again — and a USB pad holds its
last level until told zero. Writes now report whether they were accepted, the
backstop is disarmed only once the stop is actually on its way, and the
backstop re-arms rather than giving up if its own write is refused.

A full write queue dropped lightbar colours, player-LED masks and trigger
effects. Its overflow rule was "drop the oldest", which is right for rumble —
re-sent continuously, so a lost frame returns milliseconds later — and wrong
for everything else, which the host sends once on change and never repeats.
Eviction is now driven by an explicit key from the caller rather than by
inspecting the bytes: rumble supersedes the pending rumble in place, and a
one-shot is discarded only if the queue holds nothing but one-shots. The key
cannot be recovered from the report itself, which is why this is not keyed by
report id — every DualSense output report carries the *same* id and differs
only in its valid_flag bytes, so an id-keyed rule would let a rumble
supersede a lightbar, which is this bug again by another route.

An unplug leaked the USB connection and the detach receiver. The link only
signalled the drop; neither capture released anything, so the interfaces
stayed claimed (the pad could not return to Android's own input stack) and a
re-plug overwrote the field holding the receiver, stranding one live for the
rest of the process. The captures now release the transport, stop() is safe
to call from the callback it arrives on — the reader thread must not join
itself — and a close is reported exactly once however many detectors see it.
A reader that could not queue a single request now reports itself down too,
instead of leaving the owner waiting on a capture that never streams.

ci(android): run the kit's unit tests ... 66a28d5abb

They were running nowhere. This workflow only assembled, and the screenshot
workflow runs the :app module's tests, so nothing enforced :kit's — the pure
parsers, migrations and feedback policies could go red without anyone
noticing. A couple of seconds against a module the build already produces.

enricobuehler referenced this pull request 2026-08-04 18:37:56 +00:00

fix(feedback): the pad stops keeping a game's trigger effect after the stream ends #44

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

enricobuehler merged commit ec288d64d3 into main 2026-08-04 21:02:18 +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#35