feat(mcp266): WebUSB / Web Serial console example + web app - #761
Conversation
Add components/mcp266/webapp_example (companion to the scripted example, which is unchanged): an ESP32-S3 that runs the espp::Mcp266 driver and exposes it to a browser over native USB. Unlike the CAN bridge (raw CAN + in-browser CANopen), this runs the driver ON the device behind a small high-level protocol (stream_frame + Dispatcher, module id 6), so the web app needs no CANopen/DS402 knowledge. - Firmware: Twai + CanopenClient + Mcp266 + UsbDevice (vendor WebUSB + CDC Web Serial). A per-transport Dispatcher routes module-6 requests to a worker task (so blocking SDO calls never run in the TinyUSB callback), and all Mcp266 SDO access is serialized under one mutex (command handler + status streamer share the single SDO channel). Protocol: START / RESET_FAULTS / RESET_ESTOP / CONFIGURE_POSITION_LOOP / SET_POSITION_LIMITS / MOVE_TO_POSITION / DRIVE_SPEED / DRIVE_DUTY / GET_STATUS / SET_STATUS_STREAM / GET_DEVICE_INFO, with OK / ERROR / STATUS / DEVICE_INFO replies. STATUS streams per-axis position/velocity/statusword + battery/temperature. - Web app components/mcp266/web/mcp266_console.html: single-file, offline, WebUSB/Web Serial, reusing the DS402 panel's transport + stream_frame framing. Per-axis (M1/M2) status cards with DS402 state decode + target-reached, configure (position-loop clamp + fallback P, CiA 402 software limits, reset faults/e-stop), command (profile-position move with vel/accel/decel; the inert speed/duty mirrors), device telemetry, and a device-side live-status stream toggle. - CMakeLists mirror the can_bridge dual-mode setup (manager-on for registry users; manager-off + vendored esp_tinyusb/tinyusb submodules for CI). build.yml runs it on esp32s3 with IDF_COMPONENT_MANAGER=0; manifest lists the new example. Verified on IDF v6.0.1 (GCC 15.2): firmware builds clean (esp32s3, manager-off); web app node --check clean; a firmware<->web-app protocol round-trip test (status 25-byte layout + move/configure command encode/decode, incl. negatives) 8/8. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅Static analysis result - no issues found! ✅ |
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed protocol robustness and transport-selection concurrency issues in the new firmware example (plus a small docs mismatch) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds an ESP32-S3 firmware example and a companion single-file web console to control and monitor a Basicmicro MCP266 from a browser over WebUSB or Web Serial, using an on-device espp::Mcp266 driver behind a small framed protocol (module id 6).
Changes:
- New
components/mcp266/webapp_examplefirmware example: TWAI + CANopen client + MCP266 driver + USB vendor/CDC framed protocol with command handling and optional status streaming. - New offline web app
components/mcp266/web/mcp266_console.htmlimplementing the same framed protocol over WebUSB/Web Serial with per-axis UI and DS402 statusword decoding. - CI/docs wiring updates: add the new example to the mcp266 manifest examples list and build it in GitHub Actions.
File summaries
| File | Description |
|---|---|
| components/mcp266/webapp_example/sdkconfig.defaults | Pins target to esp32s3 and enables TinyUSB vendor+CDC configs sized for streaming bursts. |
| components/mcp266/webapp_example/README.md | Documents wiring, protocol (module 6), and build/flash steps for the webapp example. |
| components/mcp266/webapp_example/main/mcp266_webapp_example.cpp | Implements the USB-framed protocol, dispatch/worker tasking, and MCP266 command/status handling over CANopen. |
| components/mcp266/webapp_example/main/mcp266_protocol.hpp | Defines protocol constants and STATUS payload layout for host/device interoperability. |
| components/mcp266/webapp_example/main/CMakeLists.txt | Registers the example’s main component and its dependencies. |
| components/mcp266/webapp_example/CMakeLists.txt | Project-level CMake supporting manager-on (registry) and manager-off (CI) builds with vendored TinyUSB paths. |
| components/mcp266/web/mcp266_console.html | Adds the single-file browser UI and WebUSB/Web Serial transport + framing/parser implementation. |
| components/mcp266/idf_component.yml | Registers webapp_example as an additional example for the mcp266 component. |
| .github/workflows/build.yml | Adds CI build entry for the new webapp example (esp32s3, manager-off build command). |
Review details
Suppressed comments (1)
components/mcp266/webapp_example/main/mcp266_webapp_example.cpp:380
- To keep transport selection consistent for a given received chunk (and avoid races with the TinyUSB callbacks), set active_transport in the RX worker immediately before feeding that chunk into the corresponding dispatcher.
for (const auto &[source, chunk] : chunks)
(source == Transport::Vendor ? vendor_dispatcher : cdc_dispatcher).feed(chunk);
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…sport, meta - Validate the axis selector on axis-addressed requests (CONFIGURE / SET_LIMITS / MOVE / DRIVE_SPEED / DRIVE_DUTY): a new need_axis() rejects pl[0] > 1 with invalid_argument, so an unexpected value can no longer fall through axis_of() to M1 and command the wrong motor channel. - active_transport is now written only by the RX worker, immediately before it feeds each chunk to that chunk's dispatcher (removed the store in the TinyUSB receive callback). A frame arriving on the other endpoint between enqueue and dispatch can no longer retarget the reply/status for the frame being handled. - Web app meta description reworded: the device runs the CANopen driver and exposes a high-level protocol over USB (WebUSB / Web Serial) - not a "USB-to-CANopen bridge" (the text shows in the apps index). node --check clean; firmware rebuilds clean (esp32s3, manager-off, IDF v6.0.1). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the review comments in
Verified: |
There was a problem hiding this comment.
🟡 Changes recommended
Input coercion, stale streaming state, ignored offline status, and unbounded polling can produce unsafe or misleading behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
Previously missed (5) — in code that hasn't changed since the last review.
components/mcp266/web/mcp266_console.html:306
- These encoders silently coerce out-of-range values: for example, a user-entered velocity of
-1becomes4294967295, and malformed text is converted to0bynum()before encoding. That can command materially different motor motion than the value shown in the UI. Validate exact integer syntax, signed/unsigned wire ranges, and min/max relationships before sending, following the established pattern incomponents/canopen/web/ds402_panel.html:1000-1009.
components/mcp266/web/mcp266_console.html:399 - The firmware clears status byte 24 bit 0 when neither axis responds and fills the measurements with zeroes, but this handler ignores that flag and renders those zeroes as valid readings/“Not ready.” Check the online flag before updating the fields and visibly mark telemetry unavailable when it is clear.
components/mcp266/web/mcp266_console.html:435 - A previous session can leave device-side streaming enabled after an unplug or read-loop failure. The disconnected UI clears this checkbox, but reconnecting only sends
SET_STATUS_STREAMwhen it is checked, so the stale firmware setting remains active and unsolicited polling continues. Synchronize both states on every connection by explicitly sending the disabled state too.
.github/workflows/build.yml:84 - The CI matrix is required to stay alphabetically ordered, but this new
mcp266entry is inserted betweenbasicmicroandbdc_driver. Move the MCP266 entries into them...section so future additions and duplicate checks remain predictable.
components/mcp266/web/mcp266_console.html:327 - The generated motor-control inputs use sibling
<span>elements rather than associated labels, and none has anaria-label, so screen readers expose unlabeled edit fields. Give every input in this template an accessible name by wrapping it in a<label>or adding an explicit label association.
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Balanced
| stream_enabled.store(pl[0] != 0); | ||
| stream_period_ms.store(rd_u16(pl, 1) ? rd_u16(pl, 1) : 200); |
A STATUS snapshot issues eight blocking SDO reads while holding the shared MCP mutex, so the previously unbounded period_ms (down to 1 ms) let a client flood the CAN bus and starve command handling. Clamp it to [50, 10000] ms (0 => the 200 ms default), mirroring the streaming guard in the bldc_haptics example, and document the range in the protocol header. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the remaining review comment in
Verified: firmware rebuilds clean (esp32s3, manager-off, IDF v6.0.1). |
#761 (webapp) merged to main while this branch was in review; now that this branch rebases onto main, update the webapp for the mcp266 API changes here: - configure_position_loop: ec is now the last argument (fallback_p precedes it). - set_position_limits -> set_software_position_limits. - add the new motor_controller component (a transitive mcp266 dependency) to the manager-off component closure (EXTRA_COMPONENT_DIRS + COMPONENTS) so the firmware still builds with IDF_COMPONENT_MANAGER=0. Builds clean: webapp firmware (esp32s3, manager-off) + basicmicro/mcp266 examples on IDF v6.0.1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…s-form API (#764) * refactor(motor-control): add shared MotorController concept; align mcp266 Introduce espp::MotorAxis + the espp::MotorController compile-time concept (base_component/motor_controller.hpp): the common dual-channel surface both the serial (Basicmicro) and CANopen (Mcp266) drivers should present -- drive_duty/ drive_speed(Axis), read_encoder/read_speed(Axis, int32_t&), reset_estop, read_main_battery_voltage/read_temperature. Mcp266 now aliases Axis = MotorAxis and static_asserts conformance. Also the deferred mcp266 clarity/consistency tweaks: - configure_position_loop: ec moved to LAST (fallback_p now precedes it), with a 4-arg convenience overload using the default gain -- restoring the ec-trailing convention the rest of the codebase holds to. - set_position_limits -> set_software_position_limits, to disambiguate it from configure_position_loop's manufacturer PID clamp (docs cross-reference both). Basicmicro migration to the Axis-form surface + collision rename follows on this branch. Builds clean: mcp266 example on IDF v6.0.1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(basicmicro): Axis-form API; satisfy shared MotorController Migrate Basicmicro's per-channel commands from the drive_m1_*/drive_m2_* method pairs to a single Axis-form taking espp::MotorAxis, so it presents the same channel-selected surface as espp::Mcp266 and satisfies espp::MotorController (static_assert added). Generic code can now drive either transport by axis. Per-channel commands collapsed (M1/M2 pair -> one Axis method that selects the command byte from the axis): drive_duty, drive_speed, drive_speed_accel, buffered_drive_speed_distance, buffered_drive_speed_accel_distance, set/read_velocity_pid, set/read_position_pid, read_encoder, read_speed. This removes ~200 lines of near-verbatim duplication. Naming / correctness: - The both-channels commands, which previously shared the drive_duty/drive_speed names with the (new) single-channel meaning, are renamed drive_both_duty / drive_both_speed / drive_both_speed_accel / buffered_drive_both_* so the unqualified name always means "one channel" (matching Mcp266 and killing the cross-component name collision). - e_stop_reset -> reset_estop (matches Mcp266 / the concept). - read_encoder now returns a SIGNED int32 count (was uint32): a quadrature encoder run in reverse reads as negative, as callers expect. A status-byte overload is retained; read_speed likewise keeps a direction-byte overload. - Protected wire helpers that collided by name with the new public methods are renamed *_raw (read_count_raw, read_speed_raw, set/read_velocity_pid_raw, set/read_position_pid_raw). Wire format is unchanged: every Axis method emits the exact same command byte and payload the old M1/M2 method did. Example + docs updated. Builds clean: basicmicro example on IDF v6.0.1 (esp32); host test unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(motor-control): move MotorController to its own component; address review Two #764 review follow-ups: 1. Relocate the shared interface. Per review, base_component is the CRTP/logger base-class home, not a shared-interface bucket. Move MotorAxis + the MotorController concept out to a new header-only component `motor_controller` -- mirroring how `bldc_types` holds the shared concepts for the BLDC family -- so the two sibling drivers share the contract without either depending on the other. New components/motor_controller: motor_controller.hpp (MotorAxis + the concept + an fmt formatter for MotorAxis), CMakeLists (REQUIRES format), manifest, and a doc page; basicmicro & mcp266 now REQUIRE motor_controller (CMake + idf_component.yml) -- the include and static_asserts are unchanged. Doxyfile + motor_control doc index updated. 2. mcp266 example: check set_software_position_limits()'s return (it was dropped, so an SDO timeout / invalid arg would leave limits unset while the example commanded moves regardless) and bail with a logged error like the neighboring configure_position_loop() call. Builds clean: basicmicro + mcp266 examples on IDF v6.0.1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(basicmicro): signed read_speed; register + doc ordering (review) Address the #764 review round: - read_speed(Axis, qpps, ec): commands 18/19 report an unsigned magnitude plus a separate direction byte, so the no-direction overload returned reverse motion as positive -- violating the MotorController signed-speed contract. Fold the direction into the sign (negate when backward). The direction-byte overload still exposes the raw magnitude + flag. - upload_components.yml: list components/motor_controller ahead of its first-time dependents basicmicro / mcp266 so the registry can resolve it (per the documented dependency-order rule). - Doxyfile: move the motor_controller.hpp INPUT entry to its alphabetical spot (after monitor), per the file-order requirement. Builds clean: basicmicro example on IDF v6.0.1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(mcp266): update webapp example for the Axis-form API #761 (webapp) merged to main while this branch was in review; now that this branch rebases onto main, update the webapp for the mcp266 API changes here: - configure_position_loop: ec is now the last argument (fallback_p precedes it). - set_position_limits -> set_software_position_limits. - add the new motor_controller component (a transitive mcp266 dependency) to the manager-off component closure (EXTRA_COMPONENT_DIRS + COMPONENTS) so the firmware still builds with IDF_COMPONENT_MANAGER=0. Builds clean: webapp firmware (esp32s3, manager-off) + basicmicro/mcp266 examples on IDF v6.0.1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(motor-control): validate the axis selector; add motor_controller README Address the latest #764 review: - Axis validation (basicmicro + mcp266): the Axis-form API dispatches with `axis == Axis::M1 ? ... : ...`, so an out-of-range MotorAxis (e.g. one decoded from an untrusted byte) silently targeted M2 -- a footgun the old per-axis methods could not hit. Each public axis method now calls a check_axis() guard that rejects anything but M1/M2 with invalid_argument before any I/O; the guard also covers Mcp266::axis_state (every caller validates first). - Add components/motor_controller/README.md documenting the component. On the reviewer's example/CI-matrix request: motor_controller is a header-only interface/types component like bldc_types, which ships with no example or build.yml matrix entry -- a concept has nothing runnable to demonstrate beyond the drivers that implement it, which are already exercised by the basicmicro/mcp266 examples in CI. Added the README (good practice) but intentionally no example, matching the bldc_types precedent. Verified: basicmicro + mcp266 examples build clean on IDF v6.0.1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(motor-control): close the last two axis-safety gaps (review follow-up) - MotorAxis fmt formatter: format an out-of-range value as "UNKNOWN" (via an explicit switch) instead of "M2" -- the ternary made diagnostics mislabel malformed input as a valid channel. - Mcp266::drive(Axis): the public advanced-access accessor still routed an invalid selector through axis_state() to the M2 drive. It now returns a Ds402Drive* and yields nullptr for anything but M1/M2, so a decoded-byte axis cannot silently command the wrong drive. (No callers in-tree; pre-1.0.) Verified: MotorAxis formatter host test prints M1/M2/UNKNOWN and compiles clean under -Wall -Wextra; mcp266 example builds on IDF v6.0.1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds a companion
components/mcp266/webapp_example(the scriptedexampleis unchanged) that turns an ESP32-S3 into a browser front-end for a Basicmicro MCP266: configure, command, and view live status of both motor channels over WebUSB / Web Serial.Unlike the CAN bridge (raw CAN + in-browser CANopen), this runs the
espp::Mcp266driver on the device behind a small high-level protocol (stream_frame+Dispatcher, module id 6), so the web app needs zero CANopen/DS402 knowledge.Firmware
Twai+CanopenClient+Mcp266+UsbDevice(vendor WebUSB + CDC Web Serial). A per-transportDispatcherroutes module-6 requests to a worker task (so blocking SDO never runs in the TinyUSB callback), and allMcp266SDO access is serialized under one mutex (command handler + status streamer share the single SDO channel; the Twai RX task feedsprocess_framefrom a different task, per the client contract).START/RESET_FAULTS/RESET_ESTOP/CONFIGURE_POSITION_LOOP/SET_POSITION_LIMITS/MOVE_TO_POSITION/DRIVE_SPEED/DRIVE_DUTY/GET_STATUS/SET_STATUS_STREAM/GET_DEVICE_INFO; repliesOK/ERROR/STATUS/DEVICE_INFO.STATUSstreams per-axis position/velocity/statusword + battery/temperature.Web app
components/mcp266/web/mcp266_console.html— single-file, offline, WebUSB/Web Serial, reusing the DS402 panel’s transport +stream_frameframing. Per-axis (M1/M2) cards with DS402 state decode + target-reached, configure (position-loop clamp + fallback P, CiA 402 software limits, reset faults/e-stop, start node), command (profile-position move with vel/accel/decel; the inert speed/duty mirrors), device telemetry, and a device-side live-status stream toggle. Auto-listed in the apps index.Build / CI
CMakeLists mirror the
can_bridge_exampledual-mode setup (manager-on for registry users; manager-off + vendoredesp_tinyusb/tinyusbsubmodules for CI).build.ymlruns it on esp32s3 withIDF_COMPONENT_MANAGER=0; manifest lists the new example.Verified (IDF v6.0.1 / GCC 15.2)
managed_components).node --checkclean.Follow-up context: the separate design review flagged that surfacing
is_target_reached/get_stateonMcp266would let the firmware avoid decoding the raw statusword here — not needed for this PR (the web app decodes DS402 state itself, as the DS402 panel does).🤖 Generated with Claude Code