Skip to content

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

Merged
finger563 merged 10 commits into
mainfrom
feat/canopen-can-bridge
Sep 2, 2026
Merged

feat(canopen): USB<->CAN bridge example + WebUSB/Web Serial CAN console#748
finger563 merged 10 commits into
mainfrom
feat/canopen-can-bridge

Conversation

@finger563

@finger563 finger563 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

What

A new example that turns an ESP32-S3 into a WebUSB / Web Serial CAN interface, plus the hosted CAN console web app that drives it. The existing DS402 canopen_example is left unchanged.

  • Send CAN frames as a normal, ACK-ing bus participant ("master").
  • Inspect the bus — stream every received frame; in listen-only mode a passive sniffer that never ACKs/transmits.

How it works

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/logs stay on the separate 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] (flags: bit0 extended, bit1 RTR). The bus starts stopped; the host sets baudrate/mode then STARTs it.

Web app

components/canopen/web/can_console.html — self-contained single file, auto-hosted at apps/can_console.html:

  • dual WebUSB / Web Serial connect,
  • bus config (baud + Normal/Listen-only) with live status + RX/TX/err counters,
  • send-frame panel with full id / extended / RTR / DLC / hex validation,
  • a capped (3000-row, drop-oldest) live RX+TX monitor table with pause / clear / autoscroll.

node --check clean; CRC/framing/encode layouts verified.

Wiring

TWAI TX=GPIO17, RX=GPIO16 by default (change in the source) → a CAN transceiver on a 120 Ω-terminated bus.

Test

Example builds clean (esp32s3) and is added to the CI build matrix. On-device bus traffic is the real validation.

Depends on

Stacked on #747 (stream_frame + dispatcher) — this PR is branched off it and targets it as its base. Benefits from #746's blocking vendor write for lossless RX streaming once that merges (currently uses the non-blocking write).

🤖 Generated with Claude Code

Copilot AI lite review requested due to automatic review settings September 1, 2026 03:09

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 adds a new CANopen-adjacent example that turns an ESP32-S3 into a USB↔CAN (TWAI) bridge speaking the shared stream_frame protocol via espp::Dispatcher, plus a hosted single-file WebUSB/Web Serial CAN console web app to drive it from a browser.

Changes:

  • Add can_bridge_example (ESP32-S3) bridging TWAI↔USB with a module-5 Dispatcher protocol.
  • Add components/canopen/web/can_console.html Web app (WebUSB/Web Serial) implementing the bridge protocol with live monitoring + send/config UI.
  • Document the new bridge in CANopen docs and add the example to the CI build matrix.

Reviewed changes

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

Show a summary per file
File Description
doc/en/buses/canopen.rst Adds documentation section pointing to the CAN bridge example and hosted console.
components/canopen/web/can_console.html New self-contained WebUSB/Web Serial console implementing stream_frame parsing and CAN bridge UI.
components/canopen/can_bridge_example/sdkconfig.defaults ESP32-S3 + TinyUSB configuration defaults for vendor + CDC transports.
components/canopen/can_bridge_example/README.md Example-level usage, wiring, and protocol documentation.
components/canopen/can_bridge_example/main/CMakeLists.txt Registers the example’s main component and dependencies.
components/canopen/can_bridge_example/main/can_bridge_protocol.hpp Defines the bridge wire protocol types and CAN frame encoding helpers.
components/canopen/can_bridge_example/main/can_bridge_example.cpp Implements USB vendor/CDC transport + Dispatcher + TWAI bridge logic.
components/canopen/can_bridge_example/CMakeLists.txt ESP-IDF project CMake for the example (narrowed EXTRA_COMPONENT_DIRS).
.github/workflows/build.yml Adds can_bridge_example to the CI build matrix.
Suppressed comments (2)

components/canopen/can_bridge_example/main/can_bridge_example.cpp:200

  • SET_CONFIG accepts any mode byte and stores it directly, but start_bus() treats any value other than kModeListenOnly as NORMAL. That can lead to STATUS reporting an unsupported mode value while the bus actually runs in normal mode. Reject unknown mode values to keep behavior consistent with the protocol contract.
            baudrate = sf::get_u32(payload);
            mode = payload[4];

components/canopen/can_bridge_example/main/can_bridge_example.cpp:259

  • If usb.initialize() fails, the example only logs an error but continues running and later claims the bridge is “ready”. Subsequent reads/writes will fail with not_connected, and the RX task will still start. It’s safer to abort app_main early when USB can’t be initialized.
  std::error_code usb_ec;
  if (!usb.initialize(usb_ec))
    logger.error("Failed to initialize USB device: {}", usb_ec.message());

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

Comment thread components/canopen/can_bridge_example/main/can_bridge_protocol.hpp Outdated
Comment thread components/canopen/can_bridge_example/main/can_bridge_example.cpp Outdated
Comment thread .github/workflows/build.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.

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 9 out of 9 changed files in this pull request and generated 10 comments.

Suppressed comments (2)

components/canopen/web/can_console.html:876

  • This adds a TX row after only the USB write succeeds, before the device returns OK. When the bus is stopped, listen-only, bus-off, or transmission fails, the monitor still reports a frame that was never queued on CAN and only logs the later error separately. Correlate CAN_TX requests with their OK/ERROR response and add the row only after OK.
          data = padded;
        }
      } else {

.github/workflows/build.yml:105

  • The build matrix is maintained alphabetically by path. components/canopen/can_bridge_example sorts before components/canopen/example, so these entries should be reversed.
        - path: 'components/canopen/example'
          target: esp32

Comment thread components/canopen/can_bridge_example/main/can_bridge_example.cpp Outdated
Comment thread components/canopen/can_bridge_example/main/can_bridge_example.cpp
Comment thread components/canopen/can_bridge_example/main/can_bridge_example.cpp Outdated
Comment thread components/canopen/web/can_console.html Outdated
Comment thread components/canopen/web/can_console.html
Comment thread components/canopen/can_bridge_example/main/can_bridge_protocol.hpp Outdated
Comment thread components/canopen/can_bridge_example/main/can_bridge_example.cpp Outdated
Comment thread components/canopen/can_bridge_example/main/can_bridge_example.cpp Outdated
Comment thread components/canopen/web/can_console.html Outdated
Comment thread doc/en/buses/canopen.rst Outdated
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

✅Static analysis result - no issues found! ✅

finger563 added a commit that referenced this pull request Sep 1, 2026
…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>
Base automatically changed from feat/stream-dispatcher to main September 1, 2026 21:12
finger563 added a commit that referenced this pull request Sep 1, 2026
…des them (#747)

* feat(dispatcher): stream_frame codec + Dispatcher multiplexer; OTA rides 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>

* refactor(stream_frame): v2 framing — full-byte module + type + flags

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>

* feat(coredump): migrate to v2 stream_frame (module 4) + Dispatcher; dispatcher 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>

* feat(dispatcher): Python (xplat) bindings, C++ + Python examples, host 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>

* fix(dispatcher): static-analysis + python-lint follow-ups

- 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>

* fix(dispatcher): re-entrancy safety, binding lifetimes, v2 doc corrections, 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>

* fix(bldc_haptics): migrate the haptics USB protocol to v2 stream_frame (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>

* fix(dispatcher): v2 direction hardening + finish v1->v2 doc sweep

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>

* feat(stream_frame): optional correlation/sequence id + role-agnostic 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>

* perf(dispatcher): zero-copy re-entrancy-safe dispatch (defer mutations)

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>

* chore(dispatcher): make manifests release-ready (drop stream_frame override_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>

* fix(dispatcher): exception-safe dispatch depth + upload order + stale 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>

* fix(dispatcher): suppress cppcheck throwInEntryPoint false positive in 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>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@finger563
finger563 force-pushed the feat/canopen-can-bridge branch from 782ac22 to 049577c Compare September 1, 2026 21:12
@finger563 finger563 self-assigned this Sep 1, 2026
@finger563 finger563 added enhancement New feature or request can canopen labels Sep 1, 2026
@finger563
finger563 force-pushed the feat/canopen-can-bridge branch from 049577c to 835e440 Compare September 1, 2026 21:14
@finger563
finger563 requested a balanced review from Copilot September 2, 2026 00:47

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.

🟡 Changes recommended

The CI IDF version mismatch, CDC frame truncation, and cross-transport reply race can prevent reliable operation.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (5)

components/canopen/can_bridge_example/main/can_bridge_example.cpp:266

  • Updating the global route when bytes are enqueued does not bind replies to the chunk being dispatched. If both interfaces receive data before the worker runs, active_transport points at the newest arrival while the worker may first handle an older request, so that request's synchronous OK/ERROR/STATUS reply is sent to the wrong interface. Select the tagged source immediately before feeding each chunk (or pass the source through the handler) rather than only here.
    active_transport.store(source); // reply on the transport the host is using

components/canopen/web/can_console.html:1047

  • When the summary is visible, every CAN frame sorts the map and deletes/recreates up to 1,024 DOM rows. A normal busy CAN bus can deliver thousands of frames per second, so this path will monopolize the browser main thread despite the entry cap. Update the map per frame, but throttle/batch renderSummary() (for example, once per animation frame or a few times per second).
      if (els.showSummary.checked) renderSummary();

components/canopen/web/can_console.html:268

  • The adjacent <span> does not provide an accessible name for this select. Associate a label with the control or add an aria-label.
          <select id="mode">

components/canopen/web/can_console.html:300

  • This DLC input has no programmatically associated label; the preceding <span> does not contribute to its accessible name. Add an accessible label.
          <input type="text" id="txDlc" size="3" placeholder="auto">

components/canopen/web/can_console.html:304

  • This data input also lacks an accessible name because its visible <span> is not a <label>. Add a programmatic label for screen-reader users.
          <input type="text" id="txData" value="DE AD BE EF" placeholder="DEADBEEF">
  • Files reviewed: 10/10 changed files
  • Comments generated: 8
  • Review effort level: Balanced

Comment thread .github/workflows/build.yml
Comment thread components/canopen/can_bridge_example/main/can_bridge_example.cpp
Comment thread components/canopen/web/can_console.html Outdated
Comment thread components/canopen/web/can_console.html Outdated
Comment thread components/canopen/web/can_console.html Outdated
Comment thread components/canopen/can_bridge_example/README.md Outdated
Comment thread components/canopen/can_bridge_example/README.md Outdated
Comment thread components/canopen/web/can_console.html Outdated
@finger563

Copy link
Copy Markdown
Contributor Author

Addressed the latest review comments in a3df5cb, a910519, 49b8814:

Correctness

  • write_cdc truncation under backpressure (can_bridge_example.cpp:82) — write_cdc now has the same all-or-nothing contract as write_vendor (bounded 250 ms drain-wait; all-or-nothing in TinyUSB-callback context; not_connected vs full-FIFO distinguished), using the raw tud_cdc_n_* API. No more truncated frames on any transport. This is the fix closed fix(usb_device): give write_cdc the same bounded-blocking backpressure as write_vendor #746 carried, folded in here since the bridge exposes CDC/Web Serial. The example's (previously false) "all-or-nothing" comment is now accurate.
  • Build matrix runs twai examples on the wrong IDF (build.yml:103) — twai needs the IDF ≥6.0 esp_driver_twai node API, but the matrix hardcoded v5.5.1, so twai/example and canopen/example (already on main) were silently failing under continue-on-error. Added an optional matrix.test.idf_version (default v5.5.1) and pinned the three twai-based examples to v6.0. A repo-wide default bump to v6 will follow as a separate PR.
  • Summary "+N more IDs" counted frames, not IDs (can_console.html:1101) — now tracks distinct omitted IDs in a Set, so the count is accurate under repeated traffic from one omitted ID.

Docs

  • v2 wire-format omits the correlation field (can_console.html:18) — documented the optional flags bit1 u16 correlation id, that len is at offset 5 (base) vs 7 (correlated), and the 9- vs 11-byte header.
  • README base-header order incomplete (README.md:33) — now shows the full magic/flags/module/type/len/payload/crc32 order, module 5, and the reply/version flag bits (plus the optional correlation field).
  • README RTR data field (README.md:50) — clarified that RTR frames carry no data bytes (6-byte payload even when dlc is nonzero), so clients must not append data for RTR.

Accessibility

  • Unlabeled controls (can_console.html:255/268/294/300/304) — added aria-label to the baud/mode selects and the ID/DLC/data inputs.

Verified: can_bridge_example builds clean for esp32s3 on IDF v6.0.1 (exercises the write_cdc change); node --check on the web app; build.yml override applied to all three twai entries.

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.

🟡 Changes recommended

Partial USB writes can corrupt framed streams, while transport routing and web-console performance issues remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

components/canopen/can_bridge_example/main/can_bridge_example.cpp:269

  • A later chunk on the other interface can overwrite active_transport before an earlier queued request is dispatched, causing that request's OK/ERROR/STATUS reply to be sent to the wrong host. Separate parsers do not bind replies to their source; pass the queued source through the handler/send path for request replies, and reserve last-used routing for unsolicited CAN_RX events.
  auto enqueue_rx = [&](Transport source, std::span<const uint8_t> data) {
    active_transport.store(source); // reply on the transport the host is using

components/canopen/web/can_console.html:1056

  • When the summary is visible, every CAN frame sorts up to 1024 entries, deletes the entire summary DOM, and recreates every row. At normal bus rates this can monopolize the browser main thread; update the affected row incrementally or throttle summary rendering to a fixed refresh interval.
      if (els.showSummary.checked) renderSummary();
  • Files reviewed: 12/12 changed files
  • Comments generated: 5
  • Review effort level: Balanced

Comment thread components/canopen/web/can_console.html
Comment thread components/usb_device/src/usb_device.cpp
Comment thread components/canopen/can_bridge_example/main/can_bridge_example.cpp
Comment thread components/canopen/web/can_console.html Outdated
Comment thread components/usb_device/include/usb_device.hpp 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.

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 13 out of 13 changed files in this pull request and generated 5 comments.

Comment thread components/canopen/can_bridge_example/main/can_bridge_example.cpp
Comment thread components/canopen/web/can_console.html Outdated
Comment thread components/usb_device/src/usb_device.cpp Outdated
Comment thread components/usb_device/src/usb_device.cpp
Comment thread doc/en/buses/canopen.rst
@finger563

Copy link
Copy Markdown
Contributor Author

Addressed the final review comments in 441c1ab0d:

  • Duplicated 250 ms write timeout (usb_device.cpp:890/997) — factored the timeout and the drain-poll interval into shared file-scope constants kUsbWriteTimeoutTicks / kUsbWriteDrainPollTicks, used by both write_cdc() and write_vendor().
  • Invalid <meta> (can_console.html:7) — the literal USB<->CAN (unescaped <) reworded to USB-to-CAN.
  • rst interpreted text (canopen.rst:44) — Twai now renders as an inline literal ``Twai`` to match the surrounding literals.
  • GPIO doc mismatch (can_bridge_example.cpp:43) — updated the PR description to TX=GPIO17/RX=GPIO16, matching the code + README.

Builds clean for esp32s3 on IDF v6.0.1. Should be good to merge.

finger563 and others added 6 commits September 1, 2026 23:04
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>
…dress 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>
…n 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>
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>
…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>
…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>
finger563 and others added 4 commits September 1, 2026 23:05
…cs, 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>
… 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>
…ded 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>
…a, 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>
@finger563
finger563 force-pushed the feat/canopen-can-bridge branch from 441c1ab to 58e12ce Compare September 2, 2026 04:05
@finger563
finger563 merged commit 2c3055c into main Sep 2, 2026
144 of 152 checks passed
@finger563
finger563 deleted the feat/canopen-can-bridge branch September 2, 2026 04:06
finger563 added a commit that referenced this pull request Sep 2, 2026
…on (#753)

The docs build hosts every components/*/web/*.html by flattening them into
docs/apps/ (cp -L in build_and_publish_docs.yml). #748 added a second
can_console.html (components/canopen/web/) while components/twai/web/ already had
one ("CAN Bus Console (slcan)"), so the copy failed:

  cp: will not overwrite just-created ../docs/apps/can_console.html
      with ../components/twai/web/can_console.html

Rename the newer canopen bridge console to can_bridge_console.html (the twai
console keeps its published apps/can_console.html URL) and update its three
references: the example README link, the WebUSB landing_page_url baked into the
firmware descriptor, and the canopen.rst doc link. The apps index globs the
directory, so the renamed file is listed automatically.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

can canopen enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants