You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(desktop): keep TextArea appends as appends once the byte bound is reached - #841
Once the Log Viewer's console reached ~16 KiB the desktop stopped syncing ("transport lost" / "Link lost", requests timing out) and only unplugging the board helped. No crash, no coredump.
Root causes (three, found with the board + a pyusb probe)
1. The browser's USB read length.desktop.html asked transferIn() for MAX_FRAME (4111 B). A bulk transfer ends on a short packet or when the buffer is full, so once the device streams several max-size frames back-to-back (the WINDOW_OPEN snapshot of a 16 KiB log, a busy Log Viewer) every packet is full and the one straddling byte 4111 overruns the buffer. The OS reports an overflow ("babble") and the pipe then errors out on every read: libusb shows Overflow then Pipe error; the page sees "USB read error" and declares the transport lost. This alone explains "it breaks once the log reaches 16k" and it did not depend on the firmware.
probe read length
result with a 19 KB log open
4111 B (the page's)
overflow on the first snapshot, pipe errors after
8192 B
1.3 MB in 26 s, replies in 17 ms, 0 errors
2. Appends became full Text replacements at the bound. Only the device applied max_text_bytes; when it trimmed, every append was replaced by a full Text of the bounded text, so each 200 ms Log Viewer pump sent ~16.4 KB (five max-size frames) instead of a ~400 B append. On the board: 3 replacements/s, ~50 KB/s, all max-size frames (which also feeds problem 1).
3. A host that went away kept its sink active. After a page close/reload the device kept flushing to the sink: every frame blocked the desktop task for the 250 ms USB drain timeout, and UsbDevice's "Vendor TX FIFO full, dropping a 126-byte frame" warning was appended to the Log Viewer, producing the next frame to drop (4 lines/s; the user's 16 KiB log was full of them). This is also the likely cause of the "Link lost" burst on reload.
Fix
Browser: bulk reads are 16 KiB rounded to the IN endpoint's packet size (USB_READ_BYTES), so a full packet can never straddle the buffer's end.
Both sides apply the same byte bound with the firmware's exact cut rule (cut = size − max, moved to the next line start when a newline follows, then past UTF-8 continuation bytes), in console and edit modes; an append stays a TextAppend on the wire. Only a pending append larger than the whole bound still becomes a Text. A mid-line cut keeps the ANSI state the removed prefix produced; edit mode follows MaxLines trims unless the user is mid-edit.
Device-initiated resync:send() stops at the first refused frame, pauses the sink (needs_resync), and the desktop re-sends the full snapshot by itself (unsolicited DESKTOP with the complete window list + snapshot WINDOW_OPENs, which the browser already treats as authoritative) once the transport takes frames again, retrying every 1 s with back-off to 8 s; GET_DESKTOP resyncs immediately as before; detach() clears a pending resync.
The example runs FreeRTOS at 1 kHz (CONFIG_FREERTOS_HZ=1000) so a waiting USB write polls every 1 ms instead of 10 ms.
desktop_host_test ALL TESTS PASSED (new bound cases incl. UTF-8); desktop_codec_test.js 17 passed; resolve_module_id_test.js + apps_registry_test.js pass; smoke.sh SELFTEST PASS (line-start cut, mid-line cut with re-render, ANSI colour across a cut, edit-mode trims).
esp32s3 IDF_COMPONENT_MANAGER=0 idf.py build of components/desktop/example passes; cppcheck clean on the changed headers.
Hardware (ESP32-S3 board, 19 KB log, Log Viewer + I2C + CANopen open): with the fixed firmware and aligned reads the link streams ~640 B/s of appends, snapshots (4+ max-size frames) arrive intact, GET_DESKTOP answers in 17 ms, 0 USB errors.
Hardware, host-gone scenario (CDC transport, System Monitor + Log Viewer open, host away 12 s): 1.3 s after the host came back the device sent exactly one unsolicited DESKTOP + two snapshot WINDOW_OPENs and resumed deltas (108 WIDGET_SETs in the next 11 s); GET_DESKTOP answered in 9.8 ms. The board was reflashed over OTA through the framed stream (module 0) between iterations.
Desktop page: the babble transfer status is now recovered like a stall (defense in depth; it cannot occur with aligned reads).
Review rounds 3-5 hardened the resync against races (a successful snapshot reactivates the sink; a per-sink generation makes a stale success or a stale failure a no-op after a detach / re-attach / concurrent refusal).
Protocol: espp.desktop is now v2 (negotiated through discovery): the host applies MaxTextBytes to appends and honours the new DESKTOP flag bit2 Snapshot (the GET_DESKTOP reply and the automatic resync set it; the browser drops its dialogs and, with a complete list, unlisted windows before the snapshot re-creates them). v1 was never released; message layouts and golden vectors are unchanged.
User-confirmed in the browser with the PR-head firmware on the board: the desktop keeps syncing past 16 KiB of console log.
… reached
Once a TextArea held `max_text_bytes` (16 KiB by default, the Log Viewer's
console), every further append replaced the pending TextAppend with a full
`Text` of the bounded text, because only the device applied the byte bound.
Each 200 ms Log Viewer pump then sent ~16.4 KB (five max-size frames) instead
of one ~400 B append; max-size frames need a fully drained 4096-byte USB FIFO
and the vendor write path polls once per tick (10 ms at 100 Hz), so writes
timed out, were dropped, the "host must resync" warnings fed back into the
captured log, and the desktop task stalled until the device was unplugged.
- the browser now applies the same byte bound (DESKTOP record MaxTextBytes)
with the firmware's exact cut rule (cut = size - max, moved to the next
line start when a newline follows, then past UTF-8 continuation bytes), in
both console and edit modes, so an append stays a TextAppend on the wire;
only a pending append larger than the whole bound still becomes a Text
- `boundTextBytes` in the pure block, codec-test vectors mirroring the host
test, and a selftest case (line-start cut, mid-line cut with re-render)
- host test: an append past the bound stays an append; UTF-8 continuation
cut; a pending append larger than the bound becomes a Text
- the example runs FreeRTOS at 1 kHz so a waiting USB write polls every
1 ms instead of 10 ms
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
…ws MaxLines trims
Review round 1:
- a mid-line byte cut now renders the kept suffix from the ANSI state the
removed prefix left behind (ansiSplit over the prefix), and the first
line's startState is no longer reset to null after a trim: surviving
console nodes were rendered from the state the removed text really
produced, so they stay valid without a full re-render
- edit mode: the <textarea> follows the device's copy after EITHER bound
(a MaxLines trim used to shorten the lines but not the value), unless the
user is mid-edit (dirty), whose text is sent whole and bounded + echoed
by the device
- selftest: ANSI mid-line cut keeps the colour, edit-mode MaxLines trim
syncs the value, a pending edit survives a MaxLines trim
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
…napshot it; packet-aligned WebUSB reads
Found with the board: once the console held more than 16 KiB the browser
died with "transport lost" even on the fixed firmware, and the log filled
with "Vendor TX FIFO full, dropping a 126-byte frame" at 4 lines/s.
1. Browser read length. desktop.html asked transferIn() for MAX_FRAME
(4111 B). A bulk transfer ends on a short packet or a full buffer, so
when the device streams several max-size frames back-to-back (the
WINDOW_OPEN snapshot of a 16 KiB log, a busy Log Viewer) every packet is
full and the one straddling byte 4111 overruns the buffer: the OS reports
an overflow ("babble") and the pipe then errors out on every read
(libusb: Overflow, then Pipe error; the page: "USB read error" ->
transport lost). Reads are now 16 KiB rounded to the IN endpoint's packet
size. Reproduced with a pyusb probe: 4111-byte reads fail on the first
snapshot, 8192-byte reads move 1.3 MB in 26 s with 0 errors.
2. Device-initiated resync. When a host went away (page closed / reloaded)
its sink stayed active: every flush blocked the desktop task for the
transport's 250 ms drain timeout per frame, and UsbDevice's warning was
appended to the Log Viewer, which produced the next frame to drop. Now
send() stops at the first refused frame, the sink is paused (inactive +
needs_resync) and the desktop re-sends the full snapshot by itself
(unsolicited DESKTOP with the complete window list, then the snapshot
WINDOW_OPENs -- which the browser already treats as authoritative) once
the transport takes frames again, retrying every 1 s with back-off to
8 s; a GET_DESKTOP resyncs immediately as before. detach() clears a
pending resync (no host to heal while unplugged).
Docs: send_fn / needs_resync() / README flow-control paragraph.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
…babble recovery in the page
Review round 3 (Copilot): resync_if_due() never cleared needs_resync after
the snapshot was taken, so the sink stayed paused and -- resync_due not
being re-armed on success -- the retry fired on every task iteration: on
the board the device streamed whole snapshots 80 times a second as soon as
the host accepted frames. send_snapshot() now returns whether every frame
was taken; on success the flag is cleared and the sink reactivated, unless a
concurrent detach() cleared the flag first (then it stays inactive).
desktop.html: a "babble" transferIn status is recovered like a stall
(clearHalt + continue), the way the OTA / ODrive consoles do, instead of
the read loop dying -- defense in depth behind the packet-aligned reads.
Hardware (CDC, host gone 12 s, System Monitor + Log Viewer open): 1.3 s
after the host came back the device sent one unsolicited DESKTOP + two
snapshot WINDOW_OPENs and resumed deltas (108 WIDGET_SETs in 11 s, no
repeat snapshot); GET_DESKTOP answered in 9.8 ms.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
The retry deadline is not guaranteed to wake the desktop task. send() is also called directly from submit() when the command queue is full; if that caller's transport write fails after the desktop task has computed no deadline and gone to sleep, this new resync_due is never observed until unrelated activity wakes the task. Wake the task whenever a failure arms or moves the retry deadline.
Accepted edits can be overwritten by stale lines
components/desktop/web/desktop.html:3115
!wd.st.dirty does not guarantee that wd.st.lines contains the accepted edit. The idle callback sends wd.inner.value and clears dirty, but never copies that value into lines; therefore the next TextAppend calls trim() and this assignment can replace the textarea with the stale pre-edit model, discarding an edit the device already accepted. Update lines when an edit is sent (while preserving the pending-edit behavior), then apply subsequent bounds to that synchronized value.
…ange meanwhile
Review round 4 (Copilot): between the snapshot send (sink mutex released)
and the reactivation (model mutex taken) another task may refuse a direct
send to the same sink (e.g. the queue-full ERROR reply in submit()) or the
transport may detach; the flag was already true, so the older successful
snapshot would have masked the newer failure. Each sink now carries a
generation bumped by every state change (refused frame, attach, detach);
resync_if_due() captures it with the due sink and reactivates only when it
is unchanged after the snapshot.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
Update desktop documentation for automatic snapshot retries
components/desktop/README.md:183
The published Sphinx page still states that a dropped transport remains in resync until the host's next GET_DESKTOP (doc/en/desktop/desktop.rst:48-51). That now contradicts the automatic snapshot retry documented here. Update that page as part of this behavior change so generated documentation describes the new flow control.
Restore Doxygen comment placement above Config
components/desktop/include/desktop.hpp:165
Configuration for the Desktop. now attaches to kResyncRetryMin because the constants were inserted between that Doxygen line and struct Config. Move that line back immediately above the struct so the generated API docs describe the intended symbol.
Review round 5 (Copilot): the mirror image of the previous race. send()
releases the sink mutex before reporting a refused frame, so a detach
(set_sink_active(false)) or a new GET_DESKTOP attach could land first and
the old failure then re-armed retries against the detached transport or
paused the new session. send() now captures the sink generation before the
transport callback and on_send_failed() applies the failure only while that
generation is current.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
The new paused-sink state breaks the documented attached() semantics, and published documentation remains inconsistent with automatic resynchronization.
Pausing after a refused frame now sets the sink's active flag to false, so DesktopService::attached() also becomes false even though detach() was never called and an automatic resync is still scheduled. This contradicts that accessor's documented meaning (“asked for the desktop … and was not detached”) and makes a transient backpressure event look like transport detachment to API consumers. Keep attachment/session state separate from the streaming-paused state, for example by retaining active as attachment and filtering broadcasts/retries using needs_resync.
Update Desktop guide for automatic snapshot retry behavior
components/desktop/include/desktop.hpp:106
The Sphinx Desktop guide still says a dropped transport remains flagged “until the host's next GET_DESKTOP” (doc/en/desktop/desktop.rst:48-51). That now contradicts this public contract because the device can clear the flag after an unsolicited automatic snapshot. Update the guide alongside the README so published documentation describes the new retry/backoff behavior.
Remove unmatched closing parenthesis from API documentation
…ync in the guide
Review round 6 (Copilot, summary-only): a refused frame used to clear the
sink's `active` flag, so DesktopService::attached() reported a transient
backpressure event as a detach. `active` is now purely attachment (GET_DESKTOP
/ detach), `needs_resync` is the pause: broadcasts go to attached sinks that
are not paused, the automatic resync retries attached paused sinks, and a
successful snapshot only clears the pause. The Sphinx guide and the service
header now describe the retry/back-off behaviour instead of "until the next
GET_DESKTOP"; the send_fn sentence lost its nested parenthesis.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
The reason will be displayed to describe this comment to others. Learn more.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
…iction helper for both bounds
Review round 7 (Copilot): wd.trim() rebuilt and UTF-8-encoded the whole
text on every append. The widget now keeps `bytes` (the UTF-8 size of the
lines joined by "\n", what the device counts) up to date on append /
setText / eviction and only joins + encodes when the bound is exceeded.
Line eviction (model + console DOM) lives in one dropFirstLine() helper
used by the MaxLines and the MaxTextBytes paths; the edit-mode textarea is
re-synced only when a trim changed something. Selftest asserts the counter
matches the text after the cuts.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
Capturing the generation before acquiring sink->mutex still permits a newer failure to be discarded. A snapshot send and a concurrent queue-full reply can both capture generation G; if the snapshot succeeds first and the reply then fails, resync_if_due() may clear the flag and increment to G+1 before on_send_failed() runs, causing that later refusal to be treated as stale. Order the generation token/failure update with the serialized send operation so every refusal that occurs after a successful snapshot wins.
…ive for dialogs
Review round 8 (Copilot): the automatic resync (and a same-session
GET_DESKTOP) could not heal a refused DIALOG_CLOSE -- Model::snapshot()
only re-sends open dialogs and the browser never dropped the absent ones,
so a stale modal survived. The snapshot DESKTOP (GET_DESKTOP reply and the
device-initiated resync, never the apps / records change broadcast) now
carries flags bit2 Snapshot; the browser drops every local dialog when it
sees it (or when the DESKTOP answers its own request) before the DIALOG
frames that follow re-create the live ones. Windows keep the existing rule
(reconciled against a complete window list). Selftest: a dialog survives
a plain broadcast and is dropped by a snapshot DESKTOP. Old hosts ignore
the bit.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
Retry scheduling can leave the task asleep indefinitely
components/desktop/include/desktop.hpp:1414
A retry scheduled from the queue-full path can be missed indefinitely. submit() calls send() on the caller thread; while its transport callback blocks, the desktop task can drain the queue and go to sleep. If that send then fails, this updates resync_due but never notifies the sleeping task, so no automatic resync runs until some unrelated event wakes it. Wake the task after arming the retry.
Full renders discard retained ANSI state
components/desktop/web/desktop.html:3118
The saved ANSI state is lost on the next full console render. renderAll() starts from null and overwrites every line's startState, and any later Flags update (or an edit→console mode switch) calls it. Thus a suffix kept after cutting off an SGR prefix initially remains colored, but changing an unrelated flag such as auto-scroll immediately renders it with the default state. Persist the retained prefix state separately, or seed renderAll() from the first surviving line's retained state.
…pends, Snapshot flag)
Review round 9 (Copilot): the host-side trimming and the DESKTOP Snapshot
flag change what a host must do, so the version negotiated through
discovery is now 2 (kProtocolVersion, the page's espp-protocols meta and
DESKTOP_PROTOCOL_VERSION, the docs and module tables). v1 was never
released (the component is unpublished and postdates the last tag); a v1
host would keep appending without trimming and could keep a stale dialog
after a resync. Every message layout is unchanged, so the DESKTOP
payload's own proto byte (kDesktopProto) stays 1 and the golden vectors
are untouched.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Once the Log Viewer's console reached ~16 KiB the desktop stopped syncing ("transport lost" / "Link lost", requests timing out) and only unplugging the board helped. No crash, no coredump.
Root causes (three, found with the board + a pyusb probe)
1. The browser's USB read length.
desktop.htmlaskedtransferIn()forMAX_FRAME(4111 B). A bulk transfer ends on a short packet or when the buffer is full, so once the device streams several max-size frames back-to-back (the WINDOW_OPEN snapshot of a 16 KiB log, a busy Log Viewer) every packet is full and the one straddling byte 4111 overruns the buffer. The OS reports an overflow ("babble") and the pipe then errors out on every read: libusb showsOverflowthenPipe error; the page sees "USB read error" and declares the transport lost. This alone explains "it breaks once the log reaches 16k" and it did not depend on the firmware.2. Appends became full Text replacements at the bound. Only the device applied
max_text_bytes; when it trimmed, every append was replaced by a fullTextof the bounded text, so each 200 ms Log Viewer pump sent ~16.4 KB (five max-size frames) instead of a ~400 B append. On the board: 3 replacements/s, ~50 KB/s, all max-size frames (which also feeds problem 1).3. A host that went away kept its sink active. After a page close/reload the device kept flushing to the sink: every frame blocked the desktop task for the 250 ms USB drain timeout, and
UsbDevice's "Vendor TX FIFO full, dropping a 126-byte frame" warning was appended to the Log Viewer, producing the next frame to drop (4 lines/s; the user's 16 KiB log was full of them). This is also the likely cause of the "Link lost" burst on reload.Fix
USB_READ_BYTES), so a full packet can never straddle the buffer's end.TextAppendon the wire. Only a pending append larger than the whole bound still becomes aText. A mid-line cut keeps the ANSI state the removed prefix produced; edit mode follows MaxLines trims unless the user is mid-edit.send()stops at the first refused frame, pauses the sink (needs_resync), and the desktop re-sends the full snapshot by itself (unsolicited DESKTOP with the complete window list + snapshot WINDOW_OPENs, which the browser already treats as authoritative) once the transport takes frames again, retrying every 1 s with back-off to 8 s; GET_DESKTOP resyncs immediately as before;detach()clears a pending resync.CONFIG_FREERTOS_HZ=1000) so a waiting USB write polls every 1 ms instead of 10 ms.Docs: README flush + flow-control sections,
Config::max_text_bytes,append_text,send_fn,needs_resync().Verification
desktop_host_testALL TESTS PASSED (new bound cases incl. UTF-8);desktop_codec_test.js17 passed;resolve_module_id_test.js+apps_registry_test.jspass;smoke.shSELFTEST PASS (line-start cut, mid-line cut with re-render, ANSI colour across a cut, edit-mode trims).IDF_COMPONENT_MANAGER=0 idf.py buildofcomponents/desktop/examplepasses; cppcheck clean on the changed headers.babbletransfer status is now recovered like a stall (defense in depth; it cannot occur with aligned reads).espp.desktopis now v2 (negotiated through discovery): the host appliesMaxTextBytesto appends and honours the new DESKTOP flag bit2Snapshot(the GET_DESKTOP reply and the automatic resync set it; the browser drops its dialogs and, with a complete list, unlisted windows before the snapshot re-creates them). v1 was never released; message layouts and golden vectors are unchanged.🤖 Generated with Claude Code
https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU