Skip to content

feat(dispatcher): stream_frame codec + Dispatcher multiplexer; OTA rides them - #747

Merged
finger563 merged 13 commits into
mainfrom
feat/stream-dispatcher
Sep 1, 2026
Merged

feat(dispatcher): stream_frame codec + Dispatcher multiplexer; OTA rides them#747
finger563 merged 13 commits into
mainfrom
feat/stream-dispatcher

Conversation

@finger563

@finger563 finger563 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

What

Two new dependency-free components — stream_frame (frame codec) and dispatcher (protocol multiplexer) — and every espp protocol that rode the old ota_stream framing is migrated onto a new v2 wire format.

  • stream_frame — the CRC-32 frame codec + incremental resynchronizing StreamParser, extracted out of ota/detail/ota_stream_protocol.hpp into a reusable, host-testable, ESP-free component.
  • dispatcherespp::Dispatcher parses one stream once and routes each frame to a per-module handler by the frame's module byte, so several framed protocols (OTA, crash-dump, a CAN bridge, haptics, app control) can share one USB / socket / UART link instead of each running its own StreamParser.

v2 wire format (breaking change)

The old v1 header packed module + message type + direction into a single type byte (module = the high nibble), which was too restrictive. v2 gives each its own field, and reserves flag bits for optional header fields:

[magic u16 = 0x4F54][flags u8][module u8][type u8]{[correlation u16] iff flags bit1}[len u32][payload][crc32]
  • flags: bit0 = reply — the request/response DIRECTION: 0 = request (initiator→responder), 1 = response/event (responder→initiator). Role-agnostic: a device can be the responder (answers a browser, as the examples do) or the initiator (sends a request to an external peer and reads the response). bit1 = correlation present (see below); bits 2–3 reserved for future optional header fields; bits 4–7 = version (=1).
  • module: full u8 (256 protocols) — the Dispatcher routing key.
  • type: full u8 message/transaction type within the module (a Transaction enum {Write, Read, WriteRead, Custom} gives recommended standard values; protocols may define their own).
  • correlation: optional u16, present only when flags bit1 is set — a protocol-defined correlation/sequence id carried in the header (CRC-covered, not in the payload) for matching a response to its request when several may be outstanding. Absent → byte-identical to a frame without it; more optional fields can be added under bits 2–3 without another breaking change.

This is a breaking change — there is no v1 compatibility path. All espp protocols and their web apps move to v2 together:

Module Protocol Firmware Web app
0 OTA ota example ota_console.html
2 haptics bldc_haptics example haptics_console.html
4 crash dump coredump example coredump_console.html
5 CAN bridge canopen/can_bridge_example (#748) can_console.html

ota_stream_protocol.hpp stays as a thin facade re-exporting the stream_frame codec under the historical espp::detail::ota_stream namespace and keeping the OTA MessageType enum + make_*/parse_* helpers, so the OTA/haptics/coredump code that layers its own protocol on the framing keeps working (each now sets its own module + reply flag).

Migrated in this PR

  • OTA (module 0) — facade + example + ota_console.html.
  • coredump (module 4, merged to main) — CoreDumpService builds/parses via stream_frame directly, example uses the Dispatcher, coredump_console.html updated.
  • haptics (module 2, merged to main) — protocol + example + haptics_console.html + PROTOCOL.md updated (it relied on OTA make_ok coinciding with its own Ok type in v1).

Request handlers reject reply-flagged frames (echo/loopback safety); the web apps require the reply flag on inbound frames.

xplat / Python

stream_frame + Dispatcher are exposed to the espp Python package (lib/python_bindings/dispatcher_bindings.cpp): espp.stream_frame (crc32 / build_frame / Frame / StreamParser / Transaction, incl. the optional correlation) + espp.Dispatcher. python/dispatcher_test.py (chained into the wheel test-command) + python/dispatcher.py demo.

Examples / tests

  • C++ examples: components/dispatcher/example and components/stream_frame/example (both in the build matrix + Doxygen).
  • Host tests: components/stream_frame/test (incl. correlation round-trip), components/dispatcher/test (incl. a re-entrancy test, ASan-clean), and the OTA helper test. Python test mirrors them.

Note — publish prerequisite

The ota / dispatcher / coredump manifests depend on the published espp/stream_frame: '>=1.0' (no override_path). Because the IDF component manager resolves this namespaced dependency from the registry (and does NOT fall back to a local EXTRA_COMPONENT_DIRS component for it), espp/stream_frame and espp/dispatcher must be published to the registry before this merges — the manager-ON example builds (ota / coredump / bldc_haptics; can_bridge on #748) won't resolve the dependency until then. Both new components are already listed in upload_components.yml. The manager-OFF examples (dispatcher, stream_frame) build regardless via CMake REQUIRES.

🤖 Generated with Claude Code

Copilot AI lite review requested due to automatic review settings September 1, 2026 02:55
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

✅Static analysis result - no issues found! ✅

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces two new reusable, dependency-free components—stream_frame (a framed stream codec with CRC32 + incremental resynchronizing parser) and dispatcher (a module-based frame multiplexer)—and migrates the OTA stream protocol and OTA example to build on them, with accompanying docs and host-side tests.

Changes:

  • Added new components/stream_frame header-only codec and documentation.
  • Added new components/dispatcher header-only multiplexer and host tests.
  • Refactored OTA stream protocol to re-export the generic codec and updated OTA example/tests to use Dispatcher + uint8_t frame types.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
doc/en/stream_frame/stream_frame.rst New Stream Frame component documentation page.
doc/en/stream_frame/index.rst New Stream Frame docs index/toctree entry.
doc/en/ota/ota.rst OTA docs updated to reference stream_frame/dispatcher layering.
doc/en/index.rst Top-level docs index updated to include new components.
doc/en/dispatcher/index.rst New Dispatcher docs index/toctree entry.
doc/en/dispatcher/dispatcher.rst New Dispatcher component documentation page.
doc/Doxyfile Adds Dispatcher/Stream Frame headers to Doxygen INPUT list.
components/stream_frame/README.md New Stream Frame component README.
components/stream_frame/include/stream_frame.hpp New Stream Frame codec API (header-only).
components/stream_frame/idf_component.yml New Stream Frame component manifest.
components/stream_frame/CMakeLists.txt New Stream Frame component registration (header-only).
components/ota/test/ota_protocol_host_test.cpp Updates host OTA protocol tests for uint8_t frame type + include path.
components/ota/include/detail/ota_stream_protocol.hpp Refactors OTA protocol to re-export stream_frame and keep OTA helpers.
components/ota/idf_component.yml OTA manifest updated to depend on stream_frame via local override.
components/ota/example/main/ota_example.cpp OTA example migrated from StreamParser to Dispatcher module routing.
components/ota/example/main/CMakeLists.txt OTA example main now REQUIRES dispatcher.
components/ota/example/CMakeLists.txt OTA example project includes dispatcher/stream_frame in component dirs + list.
components/ota/CMakeLists.txt OTA component now REQUIRES stream_frame.
components/dispatcher/test/dispatcher_host_test.cpp New Dispatcher host tests (routing/coexistence/reset).
components/dispatcher/README.md New Dispatcher component README.
components/dispatcher/include/dispatcher.hpp New Dispatcher API (header-only) built on stream_frame.
components/dispatcher/idf_component.yml New Dispatcher component manifest (depends on stream_frame override).
components/dispatcher/CMakeLists.txt New Dispatcher component registration (header-only).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread doc/en/stream_frame/stream_frame.rst Outdated
Comment thread components/stream_frame/README.md Outdated
Comment thread components/stream_frame/include/stream_frame.hpp Outdated
Comment thread doc/Doxyfile
Comment thread components/dispatcher/idf_component.yml Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

…des them

Extracts the frame codec that lived in ota/detail/ota_stream_protocol.hpp into a
new dependency-free 'stream_frame' component (magic/type/len/crc framing, CRC-32,
put/get helpers, incremental resynchronizing StreamParser; Frame::type is now a
generic uint8_t). ota_stream_protocol.hpp becomes a thin facade that re-exports
those symbols under the historical espp::detail::ota_stream namespace and keeps
the OTA MessageType enum + make_*/parse_* helpers, so existing OTA (and the open
coredump branch) keep compiling unchanged.

Adds a new 'dispatcher' component: espp::Dispatcher parses one stream once and
routes each frame to a per-module handler by the type byte's high-nibble module
id (module_of = type >> 4). This lets several framed protocols share one USB /
socket / UART link instead of running a StreamParser per protocol. The module id
math is backward compatible with the deployed wire codes: OTA opcodes 0x0X/0x8X
-> module 0, coredump 0x4X/0xCX -> module 4 (the 0x80 reply bit sits in the high
nibble, so requests and replies map to disjoint module ids and a device-side
dispatcher only ever sees the request modules).

The ota example now feeds a Dispatcher with OTA registered on module 0 (instead
of a bare StreamParser), demonstrating the pattern and cleanly ignoring other
protocols' frames rather than replying 'unknown message type'.

Host tests: components/ota/test (codec, updated for uint8_t Frame::type) and a
new components/dispatcher/test (routing / coexistence / reset) both pass. ota
example builds clean (esp32s3). Docs + Doxygen inputs added for both components.

The ota + dispatcher manifests point espp/stream_frame at the local source via
override_path until it is published (mirroring lilygo-t5-47's bq27220/pca9535).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@finger563
finger563 force-pushed the feat/stream-dispatcher branch from 571738c to bac571f Compare September 1, 2026 03:40
finger563 and others added 2 commits August 31, 2026 22:41
Addresses review feedback that the v1 nibble module id (module = type>>4) was
too restrictive and conflated module id, message type and direction in one byte.

New v2 wire format (breaking; all espp protocols + web apps move to it):
  [magic u16][flags u8][module u8][type u8][len u32][payload][crc32]
- flags: bit0 = reply (request vs device->host reply/event); bits4-7 = version
  (=1); reserved bits leave room to extend payload semantics later.
- module: full u8 (256 protocols) — the Dispatcher routing key.
- type: full u8 message/transaction type within the module. A Transaction enum
  {Write, Read, WriteRead, Custom} gives recommended standard values; protocols
  may define their own type values and carry a finer opcode in the payload.

Dispatcher now routes by the module byte (not type>>4); handlers receive the
whole Frame (module/type/flags/payload). Registration is a small (module ->
handler) set rather than a 256-entry table.

OTA migrated to v2: module 0, request types Begin/Data/Finish/Abort = 0x01-0x04,
reply types Ok/Error/Progress = 0x05/0x06/0x07 with the frame reply flag set
(previously 0x81/0x82/0x83). ota_console.html updated to the v2 header + reply
opcodes.

Also addresses PR review comments:
- stream_frame docs/README/header no longer imply StreamParser filters unknown
  types — it yields every CRC-verified frame; routing/ignoring is the
  Dispatcher's job.
- Doxyfile INPUT: dispatcher/stream_frame moved to their alphabetical positions.
- upload_components.yml: dispatcher + stream_frame added.
- Raw-framing host tests moved to components/stream_frame/test; the ota test now
  covers the OTA make_*/parse_* helpers. Both + the dispatcher test pass on host.

ota example builds clean (esp32s3); all three host tests pass; webapp
node --check clean and BEGIN-frame bytes verified.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ispatcher STL cleanup

Migrates the now-merged coredump component onto the v2 framing so #747 does not
break it: CoreDumpService builds/parses via stream_frame directly on MODULE 4
(reply Msg values' high bit maps to the frame reply flag), and feed() ignores
frames for other modules. Drops the coredump->ota dependency (it only needed the
framing) in favor of stream_frame.

The coredump example now routes each stream through an espp::Dispatcher (module
4 = core-dump service, module 1 = the example's WebUSB crash-trigger) instead of
running two StreamParsers over the same bytes with hand-rolled reset bookkeeping.
coredump_console.html updated to the v2 header (module 4 requests/replies; crash
buttons now send module 1 / type 0x00 / [CrashKind]).

Also fixes the static-analysis failure the v2 dispatcher introduced: the
registry raw loops now use std::find_if / std::any_of / std::erase_if and
dispatch() is const (cppcheck useStlAlgorithm / functionConst).

coredump example builds clean (esp32s3); host tests + webapp node --check pass;
cppcheck clean on the changed headers.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

…t example

Exposes the stream_frame codec and the Dispatcher to the espp Python package and
adds interaction examples/tests in both languages:

- lib/python_bindings/dispatcher_bindings.cpp: pybind11 bindings for
  espp.stream_frame (crc32/make_flags/build_frame/Transaction/Frame/StreamParser
  + constants) and espp.Dispatcher (register_module with a Python callback,
  feed/dispatch/reset/module_of). Wired into module.cpp and lib/espp.cmake
  (include dirs + source); both headers are dependency-free so they bind cleanly.
- python/dispatcher_test.py: mirrors the C++ host tests (codec round-trip, split
  delivery, module routing, unregister, reset). Chained into the cibuildwheel
  test-command. python/dispatcher.py: a runnable multiplexing demo.
- components/dispatcher/example: a dedicated C++ example multiplexing two toy
  protocols over one in-memory stream (buildable for esp32; added to build.yml,
  Doxyfile EXAMPLE_PATH, the component manifest examples, and referenced from the
  docs via a snippet).

Validated the bindings at runtime by building a standalone pybind11 module from
dispatcher_bindings.cpp (header-only deps) and running dispatcher_test.py +
dispatcher.py against it — all pass. C++ example builds clean (esp32).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread python/dispatcher.py Fixed
Comment thread python/dispatcher_test.py Fixed
- ota_stream_protocol.hpp: give ErrorInfo/ProgressInfo members in-class
  initializers (cppcheck uninitMemberVarNoCtor on the changed lines).
- ota host test: const Case array + const loop ref (cppcheck constVariable /
  constVariableReference).
- python/dispatcher{,_test}.py: reference espp.stream_frame instead of mixing
  'import espp' with 'from espp import ...' (code-quality bot).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 44 out of 44 changed files in this pull request and generated 13 comments.

Suppressed comments (1)

components/ota/include/detail/ota_stream_protocol.hpp:85

  • Changing these public wire values leaves components/ota/README.md documenting the old 7-byte header and 0x810x83 replies, while the updated web console uses the new format. Update the component README in the same migration so users do not implement the obsolete protocol.
  Ok = 0x05,       ///< device -> host: success reply (payload: u32 bytes_received so far)
  Error = 0x06,    ///< device -> host: failure reply (payload: u32 code + utf8 message)
  Progress = 0x07, ///< device -> host: optional progress (payload: u32 written, u32 total)

Comment thread components/stream_frame/include/stream_frame.hpp Outdated
Comment thread components/dispatcher/include/dispatcher.hpp Outdated
Comment thread components/ota/example/main/ota_example.cpp
Comment thread components/ota/include/detail/ota_stream_protocol.hpp
Comment thread components/stream_frame/CMakeLists.txt
Comment thread components/dispatcher/README.md Outdated
Comment thread components/dispatcher/idf_component.yml Outdated
Comment thread components/ota/include/detail/ota_stream_protocol.hpp Outdated
Comment thread doc/en/dispatcher/dispatcher.rst Outdated
Comment thread doc/en/dispatcher/index.rst Outdated
finger563 and others added 2 commits September 1, 2026 09:22
…tions, stream_frame example

Addresses the fresh review round on the v2 changes:
- Dispatcher::dispatch() copies the handler out before invoking it, so a handler
  that (re-entrantly) register/unregister_module()s cannot destroy the
  std::function mid-call (use-after-free). Covered by a new host test (passes
  under ASan) + a Python re-entrancy test.
- Python bindings: register_module now takes py::object so None actually
  unregisters (a py::function arg can't be None); and the Frame is passed to the
  handler as a return_value_policy::copy, so a handler that retains it owns an
  independent object rather than a wrapper over feed()'s temporary. Regression
  tests added.
- ota example: ignore reply-flagged frames (OTA replies share module 0).
- Doc corrections: dispatcher index/rst/README no longer say routing is by the
  type high nibble or that replies are 'unregistered/ignored' — routing is the
  explicit module byte, and replies share their protocol's module (distinguish
  with is_reply()); dispatcher manifest description updated; ota_stream header
  comment updated to the v2 layout + reply opcodes 0x05-0x07; coredump README
  updated to v2 (module 4).
- Added components/stream_frame/example (+ build.yml, Doxyfile, manifest
  examples, doc snippet) so stream_frame has its own build coverage / registry
  discoverability.

Host + python tests pass; ota + stream_frame examples build clean; cppcheck clean.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e (module 2)

The haptics example rode the ota_stream framing and (in v1) relied on the OTA
make_ok/make_error opcodes coinciding with its own Ok/Error types. v2 gives each
frame an explicit module byte, so leaving it unmigrated would emit module-0 OTA
frames and its v1 web app would misparse the new 9-byte header. Migrate it:

- haptics_usb_protocol.hpp: build() now uses stream_frame::build_frame on
  MODULE 2, deriving the reply flag from the type's high bit; message-type
  values are unchanged.
- example: reply_ok/reply_error build via proto::build(Msg::Ok/Error) (module 2)
  instead of the OTA make_ok/make_error (module 0); the RX loop ignores frames
  for other modules; the overflow path reuses reply_errc.
- webapp (example/webapp/index.html, symlinked as web/haptics_console.html):
  v2 9-byte header + module 2 gating.
- PROTOCOL.md: v2 framing (9 header bytes, flags/module fields, module 2); drops
  the now-false 'byte-compatible with the ota example' claim.
- CMake: add stream_frame to the example EXTRA_COMPONENT_DIRS / COMPONENTS / main
  REQUIRES.

Example builds clean (esp32s3); webapp node --check clean and the GetStatus / Ok
frames verified; cppcheck clean.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@finger563 finger563 self-assigned this Sep 1, 2026
@finger563 finger563 added enhancement New feature or request dispatcher labels Sep 1, 2026
@finger563
finger563 requested a balanced review from Copilot September 1, 2026 14:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 55 out of 55 changed files in this pull request and generated 8 comments.

Suppressed comments (3)

components/coredump/example/main/coredump_example.cpp:257

  • This dispatcher callback discards flags and forwards every module-4 frame as a request. Since handle_frame(type, payload) cannot inspect direction, a reply-flagged request type can still execute a flash operation. Preserve the device-side request-only check at this boundary.
  vendor_dispatcher.register_module(
      espp::CoreDumpService::kModule,
      [&](const espp::stream_frame::Frame &f) { vendor_service.handle_frame(f.type, f.payload); });

components/coredump/example/main/coredump_example.cpp:261

  • The CDC dispatcher also strips the reply flag before calling the service, so reply-flagged request types are processed as commands. Filter to request frames before forwarding, matching the protocol's host-to-device direction.
  cdc_dispatcher.register_module(
      espp::CoreDumpService::kModule,
      [&](const espp::stream_frame::Frame &f) { cdc_service.handle_frame(f.type, f.payload); });

components/coredump/include/coredump_service.hpp:49

  • The generated Sphinx page doc/en/coredump/coredump.rst still says CoreDumpService uses the OTA facade and coexists through a dedicated type range. After this dependency change, routing is by stream_frame module 4, so the published documentation contradicts the service and component README. Update that page to describe the v2 module/flags layout.
#include <system_error>
#include <vector>

#include "stream_frame.hpp"

Comment thread components/coredump/example/main/coredump_example.cpp Outdated
Comment thread components/bldc_haptics/example/main/bldc_haptics_example.cpp Outdated
Comment thread components/bldc_haptics/example/webapp/index.html Outdated
Comment thread components/coredump/include/coredump_service.hpp Outdated
Comment thread components/stream_frame/include/stream_frame.hpp Outdated
Comment thread components/bldc_haptics/example/PROTOCOL.md Outdated
Comment thread components/bldc_haptics/example/main/haptics_usb_protocol.hpp
Comment thread components/ota/include/detail/ota_stream_protocol.hpp
Latest review round: request-side handlers now reject reply-flagged frames
(and the host web app requires them), so an echoed/loopback reply can't re-enter
a device's request handler:
- CoreDumpService::feed() skips reply-flagged frames (module-routed request
  responder); the coredump example's dispatcher handlers and crash-trigger
  handler gate on !is_reply().
- bldc_haptics example RX loop dispatches only module-2 REQUESTS (!is_reply()),
  so a haptics reply type (0x81) can't hit the default -> ERROR path and loop on
  an echoing transport.
- haptics_console.html only consumes module-2 frames with the reply flag set.

Docs finished onto v2:
- components/ota/README.md: v2 9-byte header (flags/module) + reply opcodes
  0x05/0x06/0x07 (was the old 7-byte / 0x81-0x83).
- bldc_haptics README + PROTOCOL.md: point at the stream_frame codec as the
  authoritative spec and drop the now-false 'byte-compatible with the ota
  example' claim (haptics is module 2).

coredump + haptics examples build clean (esp32s3); webapp node --check clean.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@finger563
finger563 requested a balanced review from Copilot September 1, 2026 15:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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.

Pull request overview

Copilot reviewed 57 out of 57 changed files in this pull request and generated 1 comment.

Suppressed comments (7)

components/stream_frame/include/stream_frame.hpp:1

  • make_flags() currently shifts the full version byte into the high nibble and then truncates to uint8_t, which implicitly aliases versions > 15 (only the low 4 bits survive). Consider explicitly masking to 4 bits (and keeping reserved bits 1–3 clear) so the encoding matches the documented “bits 4–7” semantics and avoids surprising truncation.
    components/stream_frame/include/stream_frame.hpp:1
  • StreamParser::reset() clears only buffer_ but leaves dropped_bytes_ untouched. That’s a reasonable choice, but it’s ambiguous: callers may expect reset() to reset all parser state (including diagnostics) after a reconnect/overflow. Either reset dropped_bytes_ as well, or document clearly that it is lifetime-cumulative (or provide a separate reset_stats()/clear_dropped_bytes() API).
    lib/python_bindings/dispatcher_bindings.cpp:1
  • These bindings accept raw stream data as std::string, which commonly allows Python str inputs and can silently UTF-8 encode/transform data that is intended to be binary. For a framing codec/router, it’s safer to require a bytes-like input (py::bytes, py::buffer, py::memoryview, etc.) and extract raw bytes explicitly, which also makes it easier to avoid extra copies for large payloads.
    lib/python_bindings/dispatcher_bindings.cpp:1
  • These bindings accept raw stream data as std::string, which commonly allows Python str inputs and can silently UTF-8 encode/transform data that is intended to be binary. For a framing codec/router, it’s safer to require a bytes-like input (py::bytes, py::buffer, py::memoryview, etc.) and extract raw bytes explicitly, which also makes it easier to avoid extra copies for large payloads.
    lib/python_bindings/dispatcher_bindings.cpp:1
  • These bindings accept raw stream data as std::string, which commonly allows Python str inputs and can silently UTF-8 encode/transform data that is intended to be binary. For a framing codec/router, it’s safer to require a bytes-like input (py::bytes, py::buffer, py::memoryview, etc.) and extract raw bytes explicitly, which also makes it easier to avoid extra copies for large payloads.
    lib/python_bindings/dispatcher_bindings.cpp:1
  • These bindings accept raw stream data as std::string, which commonly allows Python str inputs and can silently UTF-8 encode/transform data that is intended to be binary. For a framing codec/router, it’s safer to require a bytes-like input (py::bytes, py::buffer, py::memoryview, etc.) and extract raw bytes explicitly, which also makes it easier to avoid extra copies for large payloads.
    lib/python_bindings/dispatcher_bindings.cpp:1
  • These bindings accept raw stream data as std::string, which commonly allows Python str inputs and can silently UTF-8 encode/transform data that is intended to be binary. For a framing codec/router, it’s safer to require a bytes-like input (py::bytes, py::buffer, py::memoryview, etc.) and extract raw bytes explicitly, which also makes it easier to avoid extra copies for large payloads.

Comment thread components/dispatcher/include/dispatcher.hpp Outdated
…direction wording

Since v2 is already a breaking change, add an OPTIONAL header field for
request/response correlation (per review discussion), and generalize the
direction flag's docs so both device roles are first-class.

- flags bit0 (reply) is now documented as the request/response DIRECTION:
  0 = request (initiator->responder), 1 = response/event (responder->initiator)
  — not the host/device-specific phrasing. A device can be the responder (answers
  a browser, as the coredump/haptics/CAN examples do) or the initiator (sends a
  request to an external peer and reads the response); same flag, mirror-image
  handling.
- flags bit1 (kFlagCorrelation) gates an OPTIONAL u16 correlation/sequence id in
  the header (after `type`, CRC-covered, not in the payload) for matching a
  response to its request when several may be outstanding. Absent -> byte-
  identical to before; bits 2..3 are reserved so more optional fields can be
  added later WITHOUT another breaking change. Frame gains
  optional<uint16_t> correlation + has_correlation(); build_frame() gains an
  optional correlation arg; the parser derives header size from the flag.
- Python bindings expose Frame.correlation + build_frame(..., correlation=);
  the three web-app StreamParsers (ota/coredump/haptics) are now correlation-
  aware (forward-compatible — the firmware never sets the bit, so received
  frames are unchanged).

Host tests (incl. a correlation round-trip, ASan-clean) + python test + the
stream_frame/dispatcher examples build/pass; web apps node --check clean.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 57 out of 57 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

components/ota/README.md:88

  • The new stream_frame dependency makes this README's documented host-test command fail: the command at lines 132–133 only adds components/ota/include, so stream_frame.hpp cannot be found. Add -I components/stream_frame/include to keep the published test instructions runnable.
This is the shared [`stream_frame`](../stream_frame) v2 codec: `flags` bit0 =
reply (0 = request, 1 = device→host reply) and bits 4-7 = version (1); `module`
is the routing id (OTA is **module 0**). OTA layers its message types on it.

components/coredump/include/coredump_service.hpp:10

  • The published doc/en/coredump/coredump.rst still says CoreDumpService reuses OTA framing and coexists by dedicated type ranges. This change instead uses stream_frame v2 and routes module 4, so update that RST page as part of this breaking protocol migration.
// handler layered on the espp `stream_frame` codec (magic "OT" + flags u8 +
// module u8 + type u8 + len u32 + payload + CRC-32, all little-endian; see
// components/stream_frame/include/stream_frame.hpp for the authoritative
// framing spec). The core-dump protocol owns dispatcher MODULE 4, so it can
// share one byte stream with other espp protocols (OTA on module 0, an
// application protocol, or free-form console text): frames for other modules
// are simply ignored (route with espp::Dispatcher, or call handle_frame()

Comment thread components/stream_frame/include/stream_frame.hpp
finger563 and others added 2 commits September 1, 2026 15:03
Review: dispatch() copied the handler std::function on every frame to stay safe
against a handler that (un)registers a module mid-dispatch — non-trivial for
high-throughput streams with large captures.

Replace the copy with deferred mutation: register_module()/unregister_module()
called while a handler is executing are queued and applied when the outermost
dispatch unwinds (a dispatch-depth counter supports a handler that itself
feed()s). handlers_ is therefore never reallocated and the running handler is
never destroyed while on the stack, so dispatch() can find the handler and call
it in place with no copy. unregister_module() is now register_module(id, null).

Same re-entrancy guarantees; ASan test (self-unregister mid-dispatch) + the
Python re-entrancy test pass; cppcheck clean; dispatcher example builds.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…erride_path)

Point ota / dispatcher / coredump at the published dependency
'espp/stream_frame: >=1.0' instead of an override_path to the local source.

PREREQUISITE: espp/stream_frame (and espp/dispatcher) must be published to the
component registry before these merge — the IDF component manager resolves the
namespaced 'espp/stream_frame' dependency from the registry and does NOT fall
back to a local EXTRA_COMPONENT_DIRS component for it (verified), so the
manager-ON example builds (ota / coredump / bldc_haptics; can_bridge on #748)
will not resolve it until it is released. Both new components are already listed
in upload_components.yml. The manager-OFF examples (dispatcher, stream_frame)
build regardless via CMake REQUIRES.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 57 out of 57 changed files in this pull request and generated 3 comments.

Suppressed comments (5)

Previously missed (5) — in code that hasn't changed since the last review.

components/bldc_haptics/example/webapp/index.html:19

  • The framing summary omits the flag-gated correlation u16, even though this app's parser handles it, and consequently says CRC always covers a 9-byte header. Add the optional field so this protocol document agrees with stream_frame and with the implementation below.
    components/coredump/web/coredump_console.html:29
  • This embedded wire specification calls the header unconditionally 9 bytes, but the parser below supports the flag-bit-1 correlation field, which makes it 11 bytes and shifts len. Include that optional field and describe CRC coverage over the full variable-size header.
    components/dispatcher/idf_component.yml:10
  • The new component advertises an example, but components/dispatcher/example/README.md is absent. Add the example's build/usage README and its documentation include page so registry and docs users get instructions rather than only source code.
    components/ota/web/ota_console.html:19
  • This embedded wire specification omits the optional correlation field that the parser below already accepts when flags bit 1 is set, and therefore also states the CRC header length incorrectly for correlated frames. Document the flag-gated u16 so implementers do not place len at the wrong offset.
    components/stream_frame/idf_component.yml:10
  • The new component advertises an example, but components/stream_frame/example/README.md is absent. Add the example's build/usage README and its documentation include page so registry and docs users get instructions rather than only source code.

Comment thread components/dispatcher/include/dispatcher.hpp Outdated
Comment thread .github/workflows/upload_components.yml Outdated
Comment thread components/dispatcher/test/dispatcher_host_test.cpp Outdated
finger563 and others added 2 commits September 1, 2026 15:59
… comment

Addresses the latest review:
- dispatch(): if a handler throws (e.g. a Python callback raising), an RAII
  guard now restores dispatch_depth_ on every path, so the dispatcher is not
  wedged (previously the skipped decrement left depth nonzero forever and all
  later registrations were deferred and never applied). The guard destructor
  only touches an int, so it cannot throw during unwinding; pending ops queued
  before a throw are applied on the next dispatch. Covered by new C++ (ASan) +
  Python throwing-handler tests.
- upload_components.yml: move components/stream_frame ahead of its first-time
  dependents (coredump / dispatcher / ota) so the initial publish can resolve it
  (per the workflow's own ordering note).
- dispatcher host test: fix the stale 're-entrancy = copy the handler' comment
  to describe the actual deferred-mutation mechanism.

Host + Python tests pass (incl. the new exception cases); cppcheck clean;
dispatcher example builds.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…n the test

The new throwing-handler test raises inside a handler and catches it around
feed(); cppcheck cannot trace the throw through the std::function / feed()
indirection and flags main() with throwInEntryPoint. The exception never escapes
the test, so suppress it inline (with a note).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@finger563
finger563 merged commit c20cc36 into main Sep 1, 2026
154 of 158 checks passed
@finger563
finger563 deleted the feat/stream-dispatcher branch September 1, 2026 21:12
finger563 added a commit that referenced this pull request Sep 1, 2026
…onal field)

The merged stream_frame v2 codec (#747) gained an optional u16 correlation id
in the header (flags bit1). Update the CAN console StreamParser to match the
other espp web apps: read flags first, derive the header size (9 or 11 bytes),
read len at offset 5+ext, size the frame/CRC window off the dynamic header, and
expose frame.correlation (u16 when present, else null). Added FLAG_CORRELATION /
CORRELATION_SIZE constants. The CAN firmware never sets the bit, so received
frames are unchanged — this is forward-compatible robustness.

node --check clean; correlation + plain frames verified round-trip.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
finger563 added a commit that referenced this pull request Sep 2, 2026
…onal field)

The merged stream_frame v2 codec (#747) gained an optional u16 correlation id
in the header (flags bit1). Update the CAN console StreamParser to match the
other espp web apps: read flags first, derive the header size (9 or 11 bytes),
read len at offset 5+ext, size the frame/CRC window off the dynamic header, and
expose frame.correlation (u16 when present, else null). Added FLAG_CORRELATION /
CORRELATION_SIZE constants. The CAN firmware never sets the bit, so received
frames are unchanged — this is forward-compatible robustness.

node --check clean; correlation + plain frames verified round-trip.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
finger563 added a commit that referenced this pull request Sep 2, 2026
…le (#748)

* feat(canopen): USB<->CAN bridge example + WebUSB/Web Serial CAN console

Adds a new can_bridge_example that turns an ESP32-S3 into a WebUSB / Web Serial
CAN interface, and the hosted CAN console web app that drives it.

- Firmware bridges the Twai (CAN 2.0) controller to the host over USB using the
  stream_frame framing and an espp::Dispatcher (this example owns module id 5).
  The same framed protocol is exposed on BOTH the vendor interface (WebUSB) and
  a CDC interface (Web Serial); device->host frames go to whichever transport
  the host last used. The system console stays on USB-Serial-JTAG.
- Protocol (can_bridge_protocol.hpp): CAN_TX / SET_CONFIG / START / STOP /
  GET_STATUS requests (0x5X) and CAN_RX / OK / ERROR / STATUS replies (0xDX); a
  CAN frame encodes as [id u32][flags u8][dlc u8][data]. Supports normal
  (master, ACK) and listen-only (passive sniff) modes; the bus starts stopped
  and the host configures baudrate/mode then starts it.
- Web app (components/canopen/web/can_console.html): dual WebUSB/Web Serial
  connect, bus config + live status/counters, a send-frame panel with full
  id/ext/rtr/dlc/hex validation, and a capped live RX+TX monitor table with
  pause/clear/autoscroll. Self-contained single file; node --check clean.

The DS402 canopen_example is unchanged. Example builds clean (esp32s3) and is
added to the CI build matrix; the web app is auto-hosted from components/*/web/.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* refactor(canopen): migrate CAN bridge to v2 stream_frame framing + address review

Rebased onto the v2 stream_frame/dispatcher branch and migrated the CAN bridge:
- send_frame() now builds v2 frames on module 5, deriving the reply flag from
  the type's high bit (0xD_ replies); the dispatcher handler takes the whole
  stream_frame::Frame (module already routed).
- Protocol header comment corrected: every frame is module 5 with the type in
  the type byte and the reply flag set for 0xD_ replies (the old 'high nibble'
  wording described the retired v1 nibble scheme — the PR comment).
- can_console.html migrated to the v2 9-byte header (module 5).

Also addresses the other review comments:
- top comment fixed: both vendor (WebUSB) and CDC (Web Serial) carry the framed
  protocol; the console/logs go to USB-Serial-JTAG (not CDC).
- build.yml: can_bridge_example ordered before canopen/example (alphabetical).

can_bridge_example builds clean (esp32s3); webapp node --check clean and the
GET_STATUS/CAN_RX frames verified.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(canopen): address CAN bridge review comments + register example in manifest

Firmware:
- Vendor and CDC are independent byte streams, so each now gets its OWN
  Dispatcher (chunks are tagged by transport) — a frame split across reads on
  one transport can never be stitched onto the other's bytes.
- SET_CONFIG validates the mode byte (0=normal / 1=listen-only) instead of
  silently accepting any value.
- send() surfaces a rate-limited warning when a frame is dropped (USB TX
  backpressure / disconnect) rather than discarding the write result; the
  writes are all-or-nothing so no truncated frame reaches the host.
- init failure no longer announces the bridge as 'ready'.
- Strengthened the protocol-header comment: the v2 frame has a dedicated module
  byte set to 5 for every frame (requests AND replies) — 0x5X/0xD_ are the
  type values, not modules (the retired v1 nibble scheme is gone).

Web app (can_console.html):
- Web Serial teardown now releases the reader/writer locks before port.close()
  (a held writer lock made close() reject and leave the port open).
- The per-ID summary is bounded (MAX_SUMMARY_IDS = 1024, with a '+N more' note).
- DLC input is parsed strictly so '2abc' / '1.5' are rejected.

Docs/manifest:
- canopen.rst: fixed the section title/underline (was HTML-escaped + too short).
- canopen idf_component.yml: registered can_bridge_example so it is discoverable
  and usable from the component registry.

can_bridge_example builds clean (esp32s3); webapp node --check clean.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(canopen): CAN bridge handler ignores reply-flagged frames

Same v2 direction hardening as the coredump/haptics examples: the CAN bridge is
a device request responder (it SENDS the 0xD_ replies), so handle_can_frame now
returns early on frame.is_reply() — an echoed/loopback reply can no longer
re-enter the request switch and emit a spurious 'unknown CAN bridge message'
error. (can_console.html already gates on module 5 + the reply flag.)

Builds clean (esp32s3).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(canopen): make can_console.html parser correlation-aware (v2 optional field)

The merged stream_frame v2 codec (#747) gained an optional u16 correlation id
in the header (flags bit1). Update the CAN console StreamParser to match the
other espp web apps: read flags first, derive the header size (9 or 11 bytes),
read len at offset 5+ext, size the frame/CRC window off the dynamic header, and
expose frame.correlation (u16 when present, else null). Added FLAG_CORRELATION /
CORRELATION_SIZE constants. The CAN firmware never sets the bit, so received
frames are unchanged — this is forward-compatible robustness.

node --check clean; correlation + plain frames verified round-trip.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(usb_device): give write_cdc the same all-or-nothing backpressure as write_vendor

write_cdc() advanced its offset and flushed each chunk, then could break out of
the loop when a stalled FIFO made no progress - leaving a truncated frame on the
wire and corrupting framing for every subsequent message under sustained CDC
backpressure. (The can_bridge example even documented it as "all-or-nothing",
which was false.)

Port write_vendor()'s contract to write_cdc() using the raw tud_cdc_n_* API (so
the whole frame can be sized up front, mirroring tud_vendor_*): bounded (250 ms)
sleep-wait for the TinyUSB task to drain the FIFO, and ALL-OR-NOTHING when called
from TinyUSB-callback context (fail fast with no_buffer_space without enqueueing
a partial frame). Distinguishes host-disconnect (not_connected) from a full
FIFO. This is the fix that closed PR #746 carried; folded in here since the CAN
bridge exposes write_cdc over Web Serial.

Benefits every write_cdc caller (coredump, usb_cdc example, can bridge). Builds
clean for esp32s3 on IDF v6.0.1.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* docs(canopen): address CAN console review - a11y labels, v2 header docs, distinct-ID count

- can_console.html: add aria-labels to the baud/mode selects and the ID/DLC/data
  inputs (were announced without an accessible name); document the optional v2
  correlation field in the wire-format comment (len is at offset 5 for a base
  frame, 7 when flags bit1 is set; header 9 or 11 bytes).
- Summary "+N more IDs not shown" counted dropped FRAMES, so repeated traffic
  from one omitted ID could read "+1000 more IDs". Track DISTINCT omitted IDs in
  a Set so the count is accurate.
- README: document the complete stream_frame base-header order (magic/flags/
  module/type/len/payload/crc32, module 5) instead of an abbreviated list; and
  clarify that RTR CAN frames carry NO data bytes (6-byte payload even when dlc
  is nonzero), so third-party clients must not append data for RTR.
- can_bridge_example.cpp: correct the send() comment now that write_cdc is
  all-or-nothing with a bounded drain-wait.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(usb_device): make write_cdc/write_vendor truly all-or-nothing for framed writes

The previous streaming loop could still leave a truncated prefix on the wire:
tud_*_write() enqueues and flushes a chunk (advancing offset), and a later
iteration can hit the 250 ms timeout or a disconnect and return false after
those bytes are already on the stream - poisoning the host-side framing parser.
That contradicted the documented all-or-nothing contract.

Now a frame that fits in the TX FIFO (CFG_TUD_CDC_TX_BUFSIZE / 512,
CFG_TUD_VENDOR_TX_BUFSIZE / 2048) is written atomically: bounded-wait for room
for the WHOLE frame, then enqueue it in a single write, so a timeout/disconnect
returns false without enqueueing anything. TinyUSB-callback context still fails
fast when the frame does not already fit. Only frames LARGER than the FIFO are
streamed (inherently non-atomic - documented). Applied symmetrically to both
write paths and updated the header docs + component README to match.

Builds clean for esp32s3 on IDF v6.0.1.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(canopen): CAN console review round 2 - WebUSB partial write, bounded dropped-ID set, GPIO doc

- can_console.html WebUSB send(): transferOut may complete with status "ok" but
  bytesWritten < length; loop over the unsent suffix until every byte is
  acknowledged so a truncated frame can never desync the device parser.
- The summary "+N more IDs" set (summaryDroppedIds) was unbounded - a flood of
  distinct 29-bit IDs would grow it without limit, defeating the memory cap. Cap
  it at MAX_SUMMARY_IDS and show "N+" once it overflows.
- README: the firmware defaults are TX=GPIO17/RX=GPIO16; the wiring table said
  5/4. Align the doc to the code so the example works as documented.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(canopen): final review nits - shared USB write timeout, valid meta, rst literal

- usb_device: the 250 ms write timeout (and the drain poll interval) were
  hard-coded in both write_cdc() and write_vendor(); factor them into shared
  file-scope constants kUsbWriteTimeoutTicks / kUsbWriteDrainPollTicks so the two
  TX paths stay consistent and future tuning is one edit.
- can_console.html: the <meta name="description"> content had a literal "USB<->CAN"
  (an unescaped '<' makes the tag invalid); reworded to "USB-to-CAN".
- canopen.rst: render Twai as an inline literal (``Twai``) to match the
  surrounding ``can_bridge_example`` / ``stream_frame`` literals and avoid a
  single-backtick interpreted-text role.

(PR description GPIO defaults corrected to TX=17/RX=16 to match the code + README.)

Builds clean for esp32s3 on IDF v6.0.1.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dispatcher enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants