Skip to content

Fix BLE connection-robustness: abort in-flight state machines and dedupe notification listener#32

Merged
jonasniesner merged 1 commit into
OpenDisplay:mainfrom
balloob:fix/ble-connection-robustness
Jul 6, 2026
Merged

Fix BLE connection-robustness: abort in-flight state machines and dedupe notification listener#32
jonasniesner merged 1 commit into
OpenDisplay:mainfrom
balloob:fix/ble-connection-robustness

Conversation

@balloob

@balloob balloob commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Two connection-robustness fixes in httpdocs/js/ble-common.js.

1. State machines don't abort on disconnect

resetState() (called on both user disconnect and GATT gattserverdisconnected) cleared only configReadState. directWriteState, dfuState, and firmwareVersionState were left active with their pending callbacks never invoked and no timeout. Consequences:

  • A disconnect mid-transfer left "Read Toolbox" / firmware-version read / DFU upload hung silently — the caller's onComplete/onError never fired.
  • directWriteState.active staying true made the next sendCanvasToDisplay throw 'Direct write already in progress' until a page reload.

Fix: a new abortInFlightOperations(err) helper, invoked from resetState(), settles each pending callback with a clear "Disconnected" error and clears configReadState/directWriteState/dfuState/firmwareVersionState so a later operation starts fresh. The direct-write path reuses the existing _abortDirectWrite(err) so cleanup stays consistent with that state machine.

2. Duplicate notification listeners on auto-reconnect

connectToGATT() added a fresh anonymous characteristicvaluechanged listener on every call. Auto-reconnect re-runs connectToGATT() against Chrome's cached characteristic object, so after one reconnect every notification was processed twice — double-counting the config-read length (→ truncated config) and double-driving the direct-write chunk pump.

Fix: the handler is now a single bound reference (this._onNotification, created once in the constructor) that is removeEventListener'd before being re-added, mirroring the existing this._onDisconnect pattern already in the file. Exactly one listener per characteristic, so notifications are processed once after a reconnect.

Verification

  • node --check httpdocs/js/ble-common.js passes.
  • A Node vm harness (14 assertions, all passing):
    • Simulated disconnect asserts pending configRead/directWrite/dfu/firmwareVersion callbacks are each settled with the error and their state cleared (directWriteState === null); resetState() with nothing in flight is a safe no-op.
    • Simulated connect → reconnect against a shared (Chrome-cached) characteristic object asserts exactly one listener is attached and each notification fires the handler once, not twice.

Overlap with #28 (fix/direct-write-ack-timeout)

This branches off main and touches the same direct-write / resetState area as #28. Per the team this textual overlap is expected and accepted ("fix later") — #28 adds a direct-write ack timer and expands _abortDirectWrite (clearing that timer); this PR calls _abortDirectWrite from the disconnect path. The two are semantically compatible (if #28's ack-timer field exists, its _abortDirectWrite already clears it), and any textual conflict at merge time will be reconciled then. This PR deliberately does not touch the CRC (#27) or encoding (#26/#30) areas.

🤖 Generated with Claude Code

https://claude.ai/code/session_012g2e8mr132vcizx92WsgiR

Abort all in-flight state machines on disconnect. resetState() previously
cleared only configReadState, leaving directWriteState, dfuState, and
firmwareVersionState active with their pending callbacks never invoked. A
disconnect mid-transfer left "Read Toolbox"/firmware upload hung silently, and
a lingering directWriteState.active made the next sendCanvasToDisplay throw
'Direct write already in progress' until page reload. A new
abortInFlightOperations() settles each pending callback with a "Disconnected"
error and clears the state so a later operation starts fresh, mirroring
_abortDirectWrite for the direct-write path.

Attach the characteristicvaluechanged listener exactly once. connectToGATT()
added a fresh anonymous listener every call; auto-reconnect re-ran it against
Chrome's cached characteristic object, so after one reconnect every
notification was processed twice (double-counting config-read length and
double-driving the chunk pump). The handler is now a bound reference removed
before it is re-added, mirroring the existing _onDisconnect pattern.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012g2e8mr132vcizx92WsgiR
@jonasniesner jonasniesner merged commit 4d5e82f into OpenDisplay:main Jul 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants