fix(core/wire): a truncated trigger datagram stops cancelling the effect it should carry #45

Merged

enricobuehler merged 2 commits from worktree-haptics-m10-wire into main 2026-08-04 21:07:56 +00:00

Conversation 0 Commits 2 Files Changed 6

+452 -156

enricobuehler commented 2026-08-04 18:53:34 +00:00

Owner

Copy Link

Copy Source

Workstream M10 of the haptics sweep — wire & ABI hardening. Closes R10, R11, R20, R21, T4.

Two commits. The second is a breaking C-header change and is deliberately separate so it can be dropped or landed on its own.

R10 — an out-of-range pad reached one rumble consumer, not the other

It skipped the reorder gate (the per-pad seq cursor has no slot for it) and was handed to the legacy queue, while the policy engine discarded it on its own bounds check. The comment right there promised "both consumers are fed" — false for exactly these. An embedder draining the queue could be handed an index it would use to subscript its own per-pad array.

The host never emits one, so it is malformed or hostile either way. Both consumers now agree by dropping it before either sees it.

R11 + T4 — the trigger effect was bounded on neither side

Encode appended whatever it was handed; decode took the entire tail. Its sibling HidRaw had been bounded both ways all along. There is now one constant (TRIGGER_EFFECT_MAX) both sides clamp to.

Worse than the missing bound was the empty case. A body with no effect bytes decoded as an empty effect — and downstream an empty block is written as an all-zero trigger report, which is mode 0x00: release. So a truncated datagram could silently cancel the trigger effect a game was holding. That shape is now rejected outright; a genuine release is a full-length zero block and still decodes.

T4's tests pin both halves plus HidRaw, so the pair cannot drift apart again.

R20 — a hole in the ABI history, and a symbol nobody versioned

Two separate gaps:

• v11 shipped without its history line (it was connect_ex9 + client_caps, documented inline at the function but missing from the list).
• The rumble policy engine's C surface was added while ABI_VERSION still read 7, with no bump — verified by tracing the introducing commit. Every core since has exported those symbols while advertising a number that never promised them.

A shipped binary says what it says, so that cannot be corrected backwards. v15 instead establishes the floor that guarantees the surface: at or above it the symbols are present; below it an embedder must probe. No code changed for the bump, and nothing moved on the wire.

R21 — 149 unprefixed macros in every embedder's namespace ⚠️ BREAKING

MAX_PADS, TAG_LEN, ABI_VERSION, INPUT_MAGIC, the whole BTN_/AXIS_ family. The rename table already stated this rule and already carried the handful someone had noticed — and its own comment spells out the danger: a clashing #define silently takes the last definition rather than failing to compile, so the failure mode is a wrong value, not a build error.

Associated constants (ColorInfo_CP_BT709, ClockResync_ROUNDS, …) are deliberately left: cbindgen already qualifies those by type name, which is precisely the property whose absence makes a bare MAX_PADS dangerous.

Scheduled deliberately. The sweep flagged this for a release boundary and 0.24.0 has shipped. I checked every in-repo consumer: nothing used the unprefixed spellings except one Swift test, which sat directly beside lines already using the prefixed form — its constant simply had never been added to the table. Updated here. External C embedders must add the prefix; there is no silent breakage, since the old names stop existing.

---

Verification

• cargo clippy -p punktfunk-core --features quic --all-targets -- -D warnings — exit 0
• cargo test -p punktfunk-core --features quic — 331 passed / 0 failed
• Downstream on amd64 Linux after cargo clean: clippy -D warnings exit 0; pf-client-core 82/0, pf-inject 86/0
• C harness against the regenerated header (tests/c/run.sh, what CI runs) — PASS, reporting abi_version=15
• Header: unprefixed #defines 149 → 0 (excluding the 9 type-qualified associated constants); total count unchanged
• cargo fmt --all --check — clean

Known-failing, pre-existing and unrelated

cargo test --test c_abi fails locally with ld: library 'opus' not found. That test hardcodes -lopus on macOS and libopus is not installed on this machine — a macro rename cannot make a library disappear, and CI exercises the C ABI through run.sh, which asks rustc for the native libs dynamically and passes.

Not verified

The Swift test edit was not compiled: this worktree has no PunktfunkCore.xcframework (the checked-in one is stale — see M4), so the Apple package cannot build here. The change is a one-identifier rename and PUNKTFUNK_MAX_PADS is confirmed present in the regenerated header, but it has not been through a Swift compile.

Workstream **M10** of the haptics sweep — wire & ABI hardening. Closes **R10, R11, R20, R21, T4**. Two commits. The second is a **breaking C-header change** and is deliberately separate so it can be dropped or landed on its own. ### R10 — an out-of-range pad reached one rumble consumer, not the other It skipped the reorder gate (the per-pad seq cursor has no slot for it) and was handed to the legacy queue, while the policy engine discarded it on its own bounds check. The comment right there promised *"both consumers are fed"* — false for exactly these. An embedder draining the queue could be handed an index it would use to subscript its own per-pad array. The host never emits one, so it is malformed or hostile either way. Both consumers now agree by dropping it before either sees it. ### R11 + T4 — the trigger effect was bounded on neither side Encode appended whatever it was handed; decode took the entire tail. Its sibling `HidRaw` had been bounded both ways all along. There is now one constant (`TRIGGER_EFFECT_MAX`) both sides clamp to. **Worse than the missing bound was the empty case.** A body with no effect bytes decoded as an *empty* effect — and downstream an empty block is written as an all-zero trigger report, which is mode `0x00`: release. So a truncated datagram could silently cancel the trigger effect a game was holding. That shape is now rejected outright; a genuine release is a full-length zero block and still decodes. T4's tests pin both halves plus `HidRaw`, so the pair cannot drift apart again. ### R20 — a hole in the ABI history, and a symbol nobody versioned Two separate gaps: - **v11 shipped without its history line** (it was `connect_ex9` + `client_caps`, documented inline at the function but missing from the list). - **The rumble policy engine's C surface was added while `ABI_VERSION` still read 7, with no bump** — verified by tracing the introducing commit. Every core since has exported those symbols while advertising a number that never promised them. A shipped binary says what it says, so that cannot be corrected backwards. **v15 instead establishes the floor that guarantees the surface**: at or above it the symbols are present; below it an embedder must probe. No code changed for the bump, and nothing moved on the wire. ### R21 — 149 unprefixed macros in every embedder's namespace ⚠️ BREAKING `MAX_PADS`, `TAG_LEN`, `ABI_VERSION`, `INPUT_MAGIC`, the whole `BTN_`/`AXIS_` family. The rename table already stated this rule and already carried the handful someone had noticed — and its own comment spells out the danger: **a clashing `#define` silently takes the last definition rather than failing to compile**, so the failure mode is a wrong value, not a build error. Associated constants (`ColorInfo_CP_BT709`, `ClockResync_ROUNDS`, …) are deliberately left: cbindgen already qualifies those by type name, which is precisely the property whose absence makes a bare `MAX_PADS` dangerous. **Scheduled deliberately.** The sweep flagged this for a release boundary and 0.24.0 has shipped. I checked every in-repo consumer: nothing used the unprefixed spellings **except one Swift test**, which sat directly beside lines already using the prefixed form — its constant simply had never been added to the table. Updated here. External C embedders must add the prefix; there is no silent breakage, since the old names stop existing. --- ### Verification - `cargo clippy -p punktfunk-core --features quic --all-targets -- -D warnings` — **exit 0** - `cargo test -p punktfunk-core --features quic` — **331 passed / 0 failed** - Downstream on amd64 Linux after `cargo clean`: clippy `-D warnings` **exit 0**; `pf-client-core` **82/0**, `pf-inject` **86/0** - **C harness against the regenerated header** (`tests/c/run.sh`, what CI runs) — **PASS**, reporting `abi_version=15` - Header: unprefixed `#define`s **149 → 0** (excluding the 9 type-qualified associated constants); total count unchanged - `cargo fmt --all --check` — clean ### Known-failing, pre-existing and unrelated `cargo test --test c_abi` fails locally with `ld: library 'opus' not found`. That test hardcodes `-lopus` on macOS and **libopus is not installed on this machine** — a macro rename cannot make a library disappear, and CI exercises the C ABI through `run.sh`, which asks rustc for the native libs dynamically and passes. ### Not verified The Swift test edit was not compiled: this worktree has no `PunktfunkCore.xcframework` (the checked-in one is stale — see M4), so the Apple package cannot build here. The change is a one-identifier rename and `PUNKTFUNK_MAX_PADS` is confirmed present in the regenerated header, but it has not been through a Swift compile.

enricobuehler added 2 commits 2026-08-04 18:53:35 +00:00

fix(core/wire): a truncated trigger datagram stops cancelling the effect it should carry ... 77ddd05b13

Three wire and ABI faults.

An out-of-range pad index reached one rumble consumer and not the other. It
skipped the reorder gate — the per-pad seq cursor has no slot for it — and was
handed to the legacy queue, while the policy engine discarded it on its own
bounds check, so the comment promising both consumers are fed was false for
exactly these. An embedder draining the queue could be handed an index it would
use to subscript its own per-pad array. The host never emits one, so it is
malformed or hostile either way; both consumers now agree by dropping it before
either sees it.

The adaptive-trigger effect was the only variable-length wire field bounded on
neither side. Encode appended whatever it was handed and decode took the whole
tail, while its sibling raw-report field had been bounded both ways all along;
there is now one constant both sides clamp to. Worse than the missing bound was
the empty case: a body with no effect bytes decoded as an EMPTY effect, and
downstream an empty block is written as an all-zero trigger report, which is
mode 0x00 — release. A truncated datagram could therefore silently cancel the
trigger effect a game was holding. That shape is now rejected outright; a
genuine release is a full-length zero block and still decodes.

The C ABI history had a hole and a symbol nobody versioned. v11 shipped without
its line, and the rumble policy engine's C surface was added while the version
constant still read 7, with no bump at all — so every core since has exported
those symbols while advertising a number that never promised them. A shipped
binary says what it says, so that cannot be corrected backwards; v15 instead
establishes the floor that guarantees the surface, and the v11 line is written
down. No code changed for the bump and nothing moved on the wire.

fix(core/abi)!: stop exporting 149 unprefixed macros into every embedder's namespace ... 2d43275fcb

BREAKING (C header only): constants such as MAX_PADS, TAG_LEN, ABI_VERSION,
INPUT_MAGIC and the whole BTN_/AXIS_ family are now PUNKTFUNK_-prefixed.

cbindgen emits a bare #define per `pub const`, so those names landed in the
namespace of every C program that includes the header. The rename table already
said this was the rule and already carried the handful someone had noticed —
and its own comment spells out why it matters: a clashing #define silently
takes the last definition rather than failing to compile, so the failure mode
is a wrong value, not a build error. This is the remaining 149.

Associated constants are deliberately left alone. cbindgen already qualifies
those with their type name, which is the very property whose absence makes a
bare MAX_PADS dangerous — they are namespaced, just not by us.

Nothing in this repository consumed the unprefixed spellings except one Swift
test, which sat next to lines already using the prefixed form because its
constant happened never to have been added to the table; it is updated here.
The C harness links and runs against the regenerated header.

Scheduled deliberately: the sweep flagged this for a release boundary, and
0.24.0 has shipped. External C embedders using the old spellings must add the
prefix; there is no silent breakage, since the old names simply stop existing.

enricobuehler referenced this pull request 2026-08-04 20:53:19 +00:00

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

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

enricobuehler merged commit 62d35bc4b6 into main 2026-08-04 21:07:56 +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#45