fix: re-enable display after encoder-only writes by oveddan · Pull Request #13 · oveddan/mft-api

oveddan · 2026-08-13T01:32:01Z

Problem

The Twister firmware disables the LED display while it receives an encoder bulk-transfer write, and only re-enables it via the confirmation animation fired by a global-settings write (command 0x01) — or by a USB replug.

applyPatchPlan only sent a global-settings frame when a global tag actually changed. So any encoder-only plan (colors, mappings — the common case) finished with the device left dark until the user unplugged and replugged it.

Fix

After a plan containing any encoder write completes, send one extra no-op global-settings write built from the device's own current, unchanged global tag values. That triggers the firmware's setting_confirmation_animation() → display_enable() path without altering any global state.

Tests

test/applier.test.ts's fake device previously only modelled encoder writes and would have thrown on the new trailing frame. It now handles global writes and tracks its own global-tag state.

npm run check passes: build clean, 15/15 tests.

Not yet verified on hardware

This was verified by build + tests only. The behavioral claim — that the display comes back on immediately after a color/mapping apply — still needs a check against a real Twister before this is trusted in practice.

🤖 Generated with Claude Code

The Twister firmware disables the LED display when it receives an encoder bulk-transfer write and only re-enables it via the confirmation animation fired by a global-settings write (or a USB replug). apply() previously only sent globals when a global tag changed, so encoder-only plans (colors/mappings) left the device dark until unplugged and replugged. Send a trailing no-op global write to trigger the re-enable whenever a plan touches any encoder. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fe10722cef

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-13T01:35:25Z

+  // leave the display dark until the device is unplugged and replugged.
+  if (targets.some((target) => target !== "globals")) {
+    const policy = firmwarePolicy(device.identity.firmwareDate);
+    const [refreshFrame] = encodeGlobalDryRun(progressive.globals.rawTags, policy);


Preflight the refresh frame before applying encoder changes

When a 2026-07-02 device omits any expanded global tag 33–38, normalizeGlobals accepts the response with a warning and an encoder-only plan remains eligible, but encodeGlobalDryRun rejects that layout here. Because this runs after the target loop, the encoder changes have already been written and verified when the apply suddenly fails, leaving no complete journal entry or returned verified snapshot. Construct and validate the refresh frame before the first write so an unsupported global layout is rejected without partially applying the plan.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-08-13T01:35:25Z

+      frameHex: refreshFrame!.hex,
+      detail: "display re-enable after encoder write",
+    });
+    connection.sendConfigurationWrite(refreshFrame!.bytes);


Record failures from the trailing refresh write

If the MIDI backend throws while sending this added global frame, the exception bypasses the write error handling used for planned frames. The journal is left with a final pending entry rather than unknown, even though all requested encoder changes may already be present and the refresh outcome is ambiguous, which undermines journal-based recovery and produces an unclassified transport error. Wrap this send in the same unknown-outcome handling as the target writes.

Useful? React with 👍 / 👎.

oveddan · 2026-08-13T01:38:05Z

+  // leave the display dark until the device is unplugged and replugged.
+  if (targets.some((target) => target !== "globals")) {
+    const policy = firmwarePolicy(device.identity.firmwareDate);
+    const [refreshFrame] = encodeGlobalDryRun(progressive.globals.rawTags, policy);


A cosmetic refresh can fail an apply that already fully succeeded.

encodeGlobalDryRun calls assertExactTags, which throws Global record tag layout is not allowlisted whenever the device's global tag set is not exactly [0..9, 31, 32, 33..38]. That layout is not guaranteed here: normalizeGlobals only pushes a warning (Firmware did not return 2026 global tag N) when a 2026 tag is missing, so an export can legitimately carry a short/extra tag set and still produce a valid plan for encoder targets.

Scenario: a 2026-07-02 unit that omits (or adds) one global tag. All encoder frames are written and full-snapshot verified, then this line throws. applyPatchPlan rejects, complete is never journaled, the CLI never writes last-verified.json, and the user sees apply fail — for changes that already landed correctly — and may restore from the backup unnecessarily.

The display refresh is best-effort polish; it should not be able to turn a verified apply into a failure. Wrap the whole block (encode + send) in try/catch, journal the failure, and continue to complete.

oveddan · 2026-08-13T01:38:31Z

+      frameHex: refreshFrame!.hex,
+      detail: "display re-enable after encoder write",
+    });
+    connection.sendConfigurationWrite(refreshFrame!.bytes);


Unguarded send: unlike every other write in this function, a transport failure here is never journaled as unknown.

The main loop wraps sendConfigurationWrite in try/catch and records outcome: "unknown" before rethrowing (applier.ts:63-66). This send has no such guard, so if the MIDI output errors (port closed, rtmidi throw), the journal is left with a pending globals entry, no terminal outcome, no complete, and the caller gets a raw rtmidi error message rather than the write-outcome language the rest of the applier uses. docs/write-safety.md invariant 6 states every pending is followed by verified, failed, or unknown.

oveddan · 2026-08-13T01:38:32Z

+  if (targets.some((target) => target !== "globals")) {
+    const policy = firmwarePolicy(device.identity.firmwareDate);
+    const [refreshFrame] = encodeGlobalDryRun(progressive.globals.rawTags, policy);
+    await appendJournal(options.journalPath, {


This pending entry never gets a terminal outcome, even on the happy path.

docs/write-safety.md invariant 6: "Before each send, an append-only journal records pending. Outcomes are verified, failed, or unknown." After this change, every successful encoder apply leaves a pending globals record with no matching outcome, immediately followed by complete. Journal-recovery tooling (explicitly listed as required future coverage in the same doc) and any human auditing the file cannot distinguish this normal, intentional no-op from a global write whose fate is genuinely unknown — i.e. exactly the ambiguous case the journal exists to flag. Emit a terminal entry (verified, or a distinct non-pending record) for this frame.

oveddan · 2026-08-13T01:38:33Z

+  // write and only re-enables it from the confirmation animation fired by a
+  // global-settings write (or a USB replug). Without this, encoder-only plans
+  // leave the display dark until the device is unplugged and replugged.
+  if (targets.some((target) => target !== "globals")) {


A global write now happens after the final verification, so postSnapshot describes a state that was never read back.

The frame is sent after the last exportConfiguration, and progressive is returned (and written to last-verified.json) unchanged. The no-op claim rests entirely on the assumption that every global tag round-trips read→write identically. The codebase already documents one asymmetry of exactly this kind on the encoder side (tag 24 shiftedMidiChannel, semanticRaw/desiredRaw), and the frame includes identity/topology globals — tag 0 is the device MIDI channel, tags 2-7 the side-button actions — which docs/write-safety.md invariant 8 says must be isolated, ordered last, and gated behind a fresh discovery/confirmation cycle. If any tag does not round-trip, apply silently changes global device state and reports a verified snapshot that does not match the hardware. Re-export after the refresh and compare against progressive, so a non-no-op is caught rather than assumed away.

oveddan · 2026-08-13T01:38:34Z

 class FakeWritableTwister implements ConfigurationWriteConnection {
  private readonly handlers = new Set<MessageHandler>();
  private readonly encoders = new Map<number, number[]>();
+  private globals: number[] = [...GLOBALS];


Nothing here asserts the behavior the PR adds.

The fake now accepts and merges global writes, but the test makes no assertion that a trailing global frame was sent, that it carried the device's current values, or that this.globals is byte-identical to GLOBALS afterwards. Both regressions the change is meant to prevent pass silently: deleting the whole new block, or having it write mutated global values. Given the PR explicitly ships unverified on hardware, the fake is the only line of defense — assert (a) exactly one bytes[4] === 1 write was received, and (b) this.globals deep-equals GLOBALS at the end.

oveddan · 2026-08-13T01:38:34Z

    progressive = observed;
  }
+
+  // Firmware disables the LED display while receiving an encoder bulk-transfer


The display stays dark on exactly the paths where the user most needs to see the device.

The refresh only runs after every target verifies. If an encoder write fails verification or the read-back times out (applier.ts:68-75), the function throws before reaching this block, so the display remains disabled — and the CLI is at that moment printing a manual-restore instruction that the user has to act on with a dark device. The unknown/failed paths are the ones that most warrant the extra frame, not the least.

oveddan · 2026-08-13T01:39:49Z

Claude Code review — 6 findings (inline)

Reviewed fe10722. Build clean, 15/15 tests pass. Findings are posted inline; summary here for context alongside the Codex pass.

The change is correct in intent — the firmware really does gate display_enable() behind the global-settings confirmation animation. The issues are all about where in applyPatchPlan the refresh was placed: after the final verification, outside the error handling that guards every other write.

#	Location	Issue
P1	`src/applier.ts:88`	`assertExactTags` can throw after all planned writes landed and verified — `apply` reports failure for changes that actually succeeded, possibly prompting a needless restore. `normalizeGlobals` only warns on absent 2026 tags (33-38), so a non-allowlisted tag layout is reachable.
P2	`src/applier.ts:96`	Send isn't wrapped in try/catch, unlike the main write loop. A transport error leaves a dangling `pending` entry and a raw rtmidi error.
P2	`src/applier.ts:89`	That `pending` journal record never gets a terminal outcome even on success — violates `docs/write-safety.md` invariant 6. Recovery tooling can't distinguish an intentional no-op from a genuinely unknown global write.
P2 (plausible)	`src/applier.ts:86`	The no-op claim is assumed, never checked: the write lands after final verification and `progressive` is returned unchanged. The frame carries tag 0 (device MIDI channel) and tags 2-7 (side buttons) — the identity/topology globals invariant 8 says must be isolated and confirmation-gated. Depends on every global tag round-tripping read→write identically, which this codebase already knows is false for encoder tag 24.
P2	`test/applier.test.ts:20`	Nothing asserts a global write was sent or that globals are unchanged — deleting the entire new fake-device block still passes all 15 tests.
P3	`src/applier.ts:82`	On verification-failure and read-back-timeout paths the function throws before the refresh, so the display stays dark exactly while the CLI prints a manual-restore instruction the user must act on.

Suggested shape

Preflight the refresh frame (encode + assertExactTags) before the first encoder write, so an unencodable global layout fails fast instead of after the fact; wrap the send in the same journal-then-rethrow guard as the write loop; and either re-read globals after the refresh or add a test asserting they're byte-identical.

Still unverified on hardware

Worth repeating from the PR body: the fake device in the test suite models the firmware behavior this PR assumes. If the command 0x01 → setting_confirmation_animation() → display_enable() assumption is wrong, green tests won't reveal it. Finding #4 above is the same risk from the other direction — a "no-op" write that isn't actually a no-op would silently alter device identity settings.

🤖 Generated with Claude Code

Review of the previous commit found the refresh write was placed after final verification and outside the error handling guarding every other write in applyPatchPlan. Three consequences, all addressed here: - encodeGlobalDryRun's tag-layout check ran after every encoder frame had landed and verified, so a device whose globals omit an expanded tag (normalizeGlobals only warns) failed the apply for changes that actually succeeded. Validate the frame before the first write. - A transport error on the refresh send bypassed the journal, leaving a dangling `pending` entry and an unclassified error. Wrap it in the same unknown-outcome handling as the planned writes. - The frame rewrites every global tag verbatim, including the identity and topology tags invariant 8 isolates, and nothing confirmed it was a no-op. Read back and compare, and journal a terminal outcome so the entry resolves on every path (invariant 6). Tests now assert the refresh is sent, is the final frame, leaves every global tag byte-identical, journals `unknown` on transport failure, and writes nothing at all when the layout is unsupported. Each fails if the corresponding behavior is removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

oveddan · 2026-08-13T01:48:50Z

Fixes pushed — `19ebb1f`

Addressed both Codex findings and three of mine. Build clean, 18/18 tests pass (was 15).

Fixed

Finding	Raised by	Fix
`applier.ts:88` — preflight the refresh frame (P1)	Codex + Claude	`encodeGlobalDryRun` now validates the global tag layout before the first write, using `before.globals.rawTags`. An encoder write doesn't change the tag layout, only values, so validating early is sound while the actual frame is still built from the latest snapshot. Unsupported layout now fails with the device untouched.
`applier.ts:96` — journal refresh transport failure (P2)	Codex + Claude	Send wrapped in the same journal-`unknown`-then-rethrow guard as the planned writes. Error message states the requested changes did verify, so the failure isn't mistaken for a lost patch.
`applier.ts:89` — `pending` never resolves (P2)	Claude	Terminal outcome journaled on every path — `verified` on success, `unknown` on transport/read-back failure, `failed` on state change. Satisfies invariant 6.
`applier.ts:86` — "no-op" assumed, never checked (P2)	Claude	Refresh is now followed by a read-back and full snapshot comparison against the last verified state. If the frame alters any global — including the identity/topology tags invariant 8 isolates — the apply fails loudly and points at the backup. `postSnapshot` is the re-read snapshot, so it describes state actually observed.
`test/applier.test.ts:20` — nothing asserted the new behavior	Claude	Three new tests, below.

Tests

Previously the entire fake-device global-write block could be deleted with all 15 tests still green. Now:

encoder-only writes end with a global frame that leaves globals byte-identical — asserts exactly one global frame, that it's the final frame, and that every global tag is unchanged afterward.
a failed display re-enable is journaled as unknown after the changes verified — simulated transport failure; asserts the unknown entry and the absence of complete.
an unsupported global tag layout is refused before any encoder frame is sent — device with a truncated global layout; asserts connection.writes is empty.

Mutation-checked, since "tests pass" was exactly the weak claim last time:

removing the preflight → the layout test fails
removing the refresh block → the other two fail

Not fixed, deliberately: `applier.ts:82` (P3)

The display stays dark when apply throws on verification failure or read-back timeout. I'm leaving this. Those are precisely the ambiguous-outcome paths, and pushing an additional global frame when the device state is already unknown cuts against invariant 7's rule that nothing is auto-corrected after an ambiguous failure. A dark display is a legitimate signal that the apply did not complete. Better addressed in CLI guidance than by writing more frames into an unknown state — happy to be overruled.

Still unverified on hardware

Unchanged from the original PR body, and the reason I'm not self-merging: the tests validate this against a fake device encoding the assumption under test. The new read-back does convert the invariant-8 risk from silent to loud — if command 0x01 round-trips any global tag asymmetrically, apply now fails instead of quietly rewriting device identity. But whether the display actually re-enables on a real Twister still needs a physical check.

🤖 Generated with Claude Code

chatgpt-codex-connector Bot reviewed Aug 13, 2026

View reviewed changes

oveddan commented Aug 13, 2026

View reviewed changes

oveddan merged commit 910c316 into main Aug 14, 2026

oveddan deleted the fix/display-refresh-after-encoder-writes branch August 14, 2026 04:11

This was referenced Aug 14, 2026

Publish the CLI as mft-config, with apply withheld pending #14 #15

Merged

Close the apply boundary: forgeable eligibility, replayable plans, cwd-relative state #14

Open

Navigation Menu

fix: re-enable display after encoder-only writes - #13

fix: re-enable display after encoder-only writes#13
oveddan merged 2 commits into
mainfrom
fix/display-refresh-after-encoder-writes

oveddan commented Aug 13, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Aug 13, 2026

Uh oh!

chatgpt-codex-connector Bot Aug 13, 2026

Uh oh!

oveddan Aug 13, 2026

Uh oh!

oveddan Aug 13, 2026

Uh oh!

oveddan Aug 13, 2026

Uh oh!

oveddan Aug 13, 2026

Uh oh!

oveddan Aug 13, 2026

Uh oh!

oveddan Aug 13, 2026

Uh oh!

oveddan commented Aug 13, 2026

Uh oh!

oveddan commented Aug 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

oveddan commented Aug 13, 2026

Problem

Fix

Tests

Not yet verified on hardware

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

oveddan Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

oveddan Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

oveddan Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

oveddan Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

oveddan Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

oveddan Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

oveddan commented Aug 13, 2026

Claude Code review — 6 findings (inline)

Suggested shape

Still unverified on hardware

Uh oh!

oveddan commented Aug 13, 2026

Fixes pushed — 19ebb1f

Fixed

Tests

Not fixed, deliberately: applier.ts:82 (P3)

Still unverified on hardware

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Fixes pushed — `19ebb1f`

Not fixed, deliberately: `applier.ts:82` (P3)