Skip to content

feat(bldc_haptics): WebUSB vendor control, OTA, and web console for the haptics example - #742

Open
finger563 wants to merge 10 commits into
mainfrom
feat/bldc-haptics-usb-webapp
Open

feat(bldc_haptics): WebUSB vendor control, OTA, and web console for the haptics example#742
finger563 wants to merge 10 commits into
mainfrom
feat/bldc-haptics-usb-webapp

Conversation

@finger563

@finger563 finger563 commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Summary

The BLDC haptics example is now controllable from a browser over USB: a TinyUSB vendor-class (WebUSB) interface, a small framed protocol, OTA over the same channel, and a self-contained web console with a live motor/detent visualization.

  • USB: reuses espp::UsbDevice's vendor function (WebUSB BOS + MS OS 2.0 descriptors already included, so Chrome claims the interface driver-free). VID 0x1209 / PID 0x0d34. The example is now persistent; RX is queued out of the TinyUSB task, telemetry streams from its own task, vendor TX is mutex-serialized.
  • Protocol (example/PROTOCOL.md, framing shared with the espp ota_stream format, CRC-32): GET_INFO/STATUS/MODES, SET_MODE (all 9 espp detent presets), SET_POSITION, SET_ENABLED, PLAY_HAPTIC, SET_STREAMING (5–1000 ms), and unsolicited TELEMETRY frames carrying a continuous detent-index+fraction knob value for smooth animation.
  • OTA: chunked begin/write/finish/abort via espp::Ota, SHA-256 validation, dual 3 MB slots with rollback (mark_app_valid after first-boot self-check), telemetry auto-paused during update. New OTA-capable partition table.
  • Webapp (example/webapp/index.html, no build step; also hosted via the docs site at the device's advertised WebUSB landing page): connect, live status tiles, canvas dial (range arc, detent ticks, current-detent highlight, eased needle, turns counter), preset dropdown, controls, firmware upload with progress. Dark/light aware.
  • All three existing hardware configs still supported and building (BLDC Test Stand / TinyS3 default, MotorGo Mini, MotorGo Axis — all ESP32-S3 native USB).

Hardware-validated (post-launch fixes included)

Verified on hardware: USB enumeration + WebUSB claim (Chrome/macOS), full init handshake, live status panels + telemetry dial, and OTA end-to-end from the web console. Two real bugs were found and fixed during bring-up:

  • usb_device: write_vendor() truncated any frame larger than the vendor TX FIFO (64-byte S3 default) instead of waiting for drain — the ~660-byte MODES reply always lost its tail, timing out the webapp's init. Now waits (1 ms polls, 250 ms bound, unmount-aware); the example also sizes both vendor FIFOs to a full frame (4200).
  • Telemetry task stack 4 KB → 8 KB: the TX-full warning path runs fmt formatting, which could overflow (= silent reboot) exactly when the host stopped reading.

Also added since launch: CDC-ACM interface carrying the system console (esp_tusb_init_console) since TinyUSB owns the S3's only USB PHY, plus panic core dumps to flash with a boot-time crash report surfaced on the CDC, the console, and a new GET_CRASH protocol command the web console requests automatically (partition table gained a 64K coredump entry — first flash of this branch needs full idf.py flash).

Still needs hardware verification

Windows MS OS 2.0 / WinUSB claim path, dial feel / continuous-position sign convention on a real motor across presets, play_haptic strength range.

Testing

  • idf.py build clean for all three board configs (esp32s3).
  • Webapp JS syntax-checked; DOM ids verified.

🤖 Generated with Claude Code

finger563 and others added 3 commits August 28, 2026 09:22
…ample

Upgrade the bldc_haptics example (ESP32-S3: MotorGo Mini / Axis / TinyS3 test
stand) from a 20-second demo into a persistent, USB-controllable haptic knob:

- Add a TinyUSB vendor-specific interface via espp::UsbDevice (WebUSB + MS OS
  2.0 BOS descriptors, VID 0x1209 / PID 0x0d34) so Chromium browsers can claim
  it driverlessly.
- Layer a small framed binary protocol on the espp ota_stream framing (magic
  "OT" + type + len + payload + CRC-32): status/info/mode enumeration reads,
  control commands (set mode / move-to-detent / enable-disable / play haptic
  click), and a configurable periodic telemetry stream (position, detent
  index, continuous knob value, shaft angle, velocity) for live visualization.
- Expose the protected detent center / config of espp::BldcHaptics through a
  thin HapticKnob subclass so telemetry can report the continuous knob value.
- Support firmware update over the same interface: the OTA message subset is
  byte-compatible with the espp ota example (espp::Ota engine, chunked
  transfer, validate + set boot partition + reboot, rollback-aware first
  boot), with an OTA-capable factory-less partition table (otadata + two 3MB
  app slots on the 8MB flash).
- RX bytes are queued out of the TinyUSB task and dispatched from a worker
  task (flash erase/validate must not block the USB stack); telemetry pauses
  while an OTA session is active.

Builds verified for all three hardware configs (test stand default, MotorGo
Mini, MotorGo Axis) on esp32s3.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Single-file, dependency-free web app (plain HTML/JS/CSS, dark-mode aware,
works from file:// / localhost / https in Chromium) that connects to the
example's vendor interface via WebUSB and provides:

- live status (mode, detent index, continuous value, velocity, fault state)
- an animated dial visualization: range arc, detent ticks (magnetic detents
  and end stops highlighted), current-detent marker and a needle eased toward
  the streamed continuous knob value
- controls: detent-preset dropdown (enumerated from the device), enable /
  disable, move-to-detent, haptic click with adjustable strength, telemetry
  rate selection
- firmware update over the same interface (chunked OTA with progress, rate,
  abort and restart handling)

The RX path is a perpetual bulk-IN pump that dispatches unsolicited telemetry
/ progress frames out-of-band while serialized command transactions await
their replies.

Also symlink the console into components/bldc_haptics/web/ so the docs CI
hosts it at esp-cpp.github.io/espp/apps/haptics_console.html — the WebUSB
landing page the firmware advertises.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- PROTOCOL.md: full wire spec for the haptics USB protocol (ota_stream
  framing, message tables, payload layouts, preset enumeration, telemetry
  format and OTA flow/semantics).
- README.md: updated usage — supported hardware, flashing (OTA partition
  table caveats), opening/using the web console, and the OTA update +
  rollback flow.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 28, 2026 16:19

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

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.

Adds a WebUSB (vendor-class) control channel, OTA update support, and a self-contained browser console for the bldc_haptics example on ESP32-S3 native USB.

Changes:

  • Introduces a single-file WebUSB console (example/webapp/index.html) implementing the framed protocol, live telemetry visualization, and OTA upload.
  • Implements the USB vendor-protocol + OTA handling in the example firmware, plus telemetry streaming and a custom OTA-capable partition table.
  • Updates example build/config plumbing (CMake, sdkconfig defaults) and documents the wire protocol + usage.

Reviewed changes

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

Show a summary per file
File Description
components/bldc_haptics/web/haptics_console.html Adds a docs-hosted entry point intended to reference the web console.
components/bldc_haptics/web/README.md Documents how the docs-hosted console is sourced and where to edit it.
components/bldc_haptics/example/webapp/index.html New offline WebUSB UI: connect, control, telemetry dial rendering, and OTA upload flow.
components/bldc_haptics/example/sdkconfig.defaults Enables TinyUSB vendor class + OTA rollback and custom partition table defaults.
components/bldc_haptics/example/partitions.csv Adds factory-less dual-slot OTA partition layout.
components/bldc_haptics/example/main/haptics_usb_protocol.hpp Defines message IDs and payload helpers sharing ota_stream framing.
components/bldc_haptics/example/main/bldc_haptics_example.cpp Adds USB vendor RX/TX, protocol worker task, OTA routing, and telemetry streaming.
components/bldc_haptics/example/main/CMakeLists.txt Adds required component dependencies for USB, OTA, timers, and tasks.
components/bldc_haptics/example/README.md Updates example docs for WebUSB control + OTA usage and troubleshooting.
components/bldc_haptics/example/PROTOCOL.md Documents the new framed USB protocol + message formats.
components/bldc_haptics/example/CMakeLists.txt Keeps IDF component manager enabled and narrows component dirs for esp_tinyusb.
Suppressed comments (1)

components/bldc_haptics/web/haptics_console.html:1

  • This file appears to be intended as a symlink target, but when served as a plain file (e.g., by a docs site), it will render as a single line of text rather than the console. GitHub Pages / static site pipelines can also ignore or mishandle symlinks depending on configuration. To make hosting robust, consider replacing this with an actual HTML redirect wrapper (meta refresh / location.replace) or duplicating/copying the authoritative index.html into the published web/ path as part of the docs build.
<!DOCTYPE html>

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

Comment thread components/bldc_haptics/example/main/bldc_haptics_example.cpp
Comment thread components/bldc_haptics/example/webapp/index.html
Comment thread components/bldc_haptics/example/main/bldc_haptics_example.cpp Outdated
finger563 and others added 3 commits August 28, 2026 12:38
…-full path)

The telemetry task's 4 KB stack could overflow (= silent reboot) when
UsbDevice::write_vendor hit its rate-limited TX-full warning - fmt-based log
formatting alone can use a few KB - which happens exactly when the host stops
draining the vendor IN endpoint.

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

TinyUSB owns the S3's only USB PHY, so the USB-Serial-JTAG console is
unavailable while the vendor interface runs - which made the reported random
reboots undiagnosable. This adds:

- A CDC-ACM interface on the composite with the SYSTEM console routed to it
  (esp_tusb_init_console): attach any serial terminal for live logs.
- Panic core dumps to a new 64K flash partition
  (CONFIG_ESP_COREDUMP_ENABLE_TO_FLASH); on the next boot the example
  summarizes the previous crash (reset reason, crashed task, PC, backtrace
  addresses + the addr2line command to decode them). Brownout/WDT resets are
  reported by reason (no core dump exists for those).
- The report is surfaced three ways: logged at boot (visible on the CDC
  console; re-logged when a terminal attaches, since it missed boot), and a
  new GET_CRASH (0x18) -> CRASH (0x94) protocol command that the web console
  requests automatically after connecting and prints in its log pane.

NOTE: the partition table changed (coredump partition) - reflash with a full
'idf.py flash'.

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

write_vendor() dropped the remaining bytes the moment tud_vendor_write()
returned 0 - but the vendor TX FIFO (CONFIG_TINYUSB_VENDOR_TX_BUFSIZE) is 64
bytes by default on the S3, SMALLER than one protocol frame, so any reply
larger than the FIFO was silently truncated mid-frame (observed live: the
haptics MODES reply, ~660 bytes, dropped its final 349 bytes -> the web
console's GET_MODES timed out on a CRC-incomplete frame while GET_INFO, which
fits, worked). A full FIFO mid-write is the NORMAL flow-control condition:
wait for the USB task to drain it (1 ms polls, 250 ms bound so an unplugged
or non-reading host cannot wedge the caller; unmounted exits immediately).

Also size the haptics example's vendor FIFOs to hold a full protocol frame
(4200 >= 4096 payload + framing) so common writes complete without waiting
and RX accepts a whole OTA chunk at once.

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

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown

✅Static analysis result - no issues found! ✅

- example: gate the USB protocol worker + telemetry tasks on successful
  usb.initialize() so a USB init failure leaves a clearly-logged,
  haptics-only device instead of tasks talking to a dead USB stack
- webapp: match device semantics for detent ticks -- an empty
  detent_positions list means EVERY position is a detent (when
  detent_strength > 0), not none; an explicit list marks only the
  'magnetic' positions

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

Copy link
Copy Markdown
Contributor Author

Re: the suppressed review note about components/bldc_haptics/web/haptics_console.html being a symlink (concern that static-site pipelines can mishandle symlinks) — checked the actual publishing pipeline, and it handles this correctly:

  • The docs workflow (.github/workflows/build_and_publish_docs.yml, the "Host the interactive single-file web apps" step) publishes hosted apps with a plain cp "${apps[@]}" ../docs/apps/ — no -R/-P. Per POSIX (and GNU coreutils on the ubuntu runner), cp dereferences symlinks named as source operands, so docs/apps/haptics_console.html is written as a regular file containing the full webapp content. Verified the behavior locally: cp link.html apps/ yields a regular file with the target's bytes.
  • actions/checkout on Linux materializes the mode-120000 git entry as a real symlink, and its relative target (../example/webapp/index.html) lives in the same repo, so the glob match resolves at copy time.
  • The subsequent Pages deploy (peaceiris/actions-gh-pages) only ever sees the already-dereferenced regular file in docs/.

So the hosted https://esp-cpp.github.io/espp/apps/haptics_console.html gets the real HTML, and the symlink keeps a single authoritative copy next to the example it speaks to (as documented in components/bldc_haptics/web/README.md). Leaving it as a symlink rather than adding a redirect wrapper or copy step.

@finger563
finger563 requested a balanced review from Copilot August 28, 2026 21:03

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

Suppressed comments (2)

components/bldc_haptics/web/haptics_console.html:1

  • The README states haptics_console.html is a symlink, but the diff shows it as a regular text file containing a path. If the docs site expects an actual HTML file, this will render as plain text instead of the console. Either commit a real symlink (if supported by the repo/CI) or replace this with an actual HTML redirect/loader page and update the README wording accordingly.
<!DOCTYPE html>

components/usb_device/src/usb_device.cpp:1

  • This change can block the calling thread for up to 250 ms when the host is slow/not reading. That’s a big latency hit for a low-level USB utility and can be problematic if write_vendor() is called from timing-sensitive tasks. Consider making the timeout configurable via UsbDevice config (or providing a non-blocking option that preserves the old behavior but reports no_buffer_space).
#include "usb_device.hpp"

Comment thread components/bldc_haptics/example/main/bldc_haptics_example.cpp
Comment thread components/bldc_haptics/example/main/bldc_haptics_example.cpp
Comment thread components/bldc_haptics/example/main/bldc_haptics_example.cpp
Comment thread components/bldc_haptics/example/main/bldc_haptics_example.cpp
Comment thread components/bldc_haptics/example/main/bldc_haptics_example.cpp
Comment thread components/bldc_haptics/example/main/bldc_haptics_example.cpp
Comment thread components/bldc_haptics/example/webapp/index.html Outdated
- example: add direct <algorithm> and <iterator> includes for std::clamp
  and std::make_move_iterator (were only transitively included)
- example: comment the SET_ENABLED 0 handler with the
  BldcHaptics::stop() -> BldcMotor::disable() -> BldcDriver::disable()
  chain and cite it in PROTOCOL.md (no behavior change)
- webapp: show real endpoint addresses (IN 0x81 / OUT 0x01) instead of
  presenting WebUSB endpoint *numbers* as addresses

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

Copy link
Copy Markdown
Contributor Author

Re: the two suppressed comments from the latest Copilot review:

components/bldc_haptics/web/haptics_console.html "is a regular text file containing a path" — that is exactly how git diffs render symlinks: a symlink is stored as an object with mode 120000 whose content is the target path, so every diff viewer shows it as a one-line "text file". The repo object is a real symlink:

$ git ls-files -s components/bldc_haptics/web/haptics_console.html
120000 7d59d0d 0  components/bldc_haptics/web/haptics_console.html
$ ls -l components/bldc_haptics/web/
lrwxr-xr-x  haptics_console.html -> ../example/webapp/index.html

And as verified in the previous review round, the docs publishing workflow (.github/workflows/build_and_publish_docs.yml, the cp "${apps[@]}" ../docs/apps/ step) uses plain cp, which dereferences symlink source operands — the hosted copy on GitHub Pages is the real HTML file, not a path string. Nothing renders as plain text.

usb_device.cpp write_vendor "can block up to 250 ms" — the wait only engages when the vendor TX FIFO is full and the host is mounted; the fast path is unchanged and non-blocking, and an unmounted device exits immediately. A full FIFO mid-frame is the normal flow-control condition here (the default FIFO is 64 B, smaller than one protocol frame), and the pre-fix behavior silently truncated mid-frame — observed live as the ~660 B GET_MODES reply losing its final 349 bytes, which is strictly worse than briefly waiting since the host parser discards a CRC-incomplete frame anyway. On timeout it does exactly what the comment suggests: gives up and reports no_buffer_space. The example also sizes its FIFOs to hold a full frame (sdkconfig.defaults) so common writes complete without waiting, and both in-repo callers run on dedicated tasks, not timing-sensitive paths. Making the bound configurable via UsbDevice::Config is reasonable — happy to add it in a follow-up, but I've kept this PR's API surface as-is.

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

Suppressed comments (11)

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

components/bldc_haptics/example/main/bldc_haptics_example.cpp:347

  • This advertised landing page will not be published: the docs workflow copies only components/*/web/*.html, but components/bldc_haptics/web contains only README.md; the claimed haptics_console.html symlink is absent. Consequently the WebUSB landing-page notification points to a 404. Add the symlink/copy at that exact path.
  vendor.landing_page_url = "esp-cpp.github.io/espp/apps/haptics_console.html";

components/bldc_haptics/example/main/bldc_haptics_example.cpp:555

  • BldcHaptics::set_position() only assigns current_position_; it does not change current_detent_center_, which is the target used to compute motor torque. Thus this command relabels the knob's current physical location and replies success, but the “Move to detent” control does not pull the motor toward the requested detent as documented. Implement a thread-safe physical target update, or change the protocol/UI semantics to describe reindexing only.
      haptic_motor.set_position(*position);
      reply_ok(static_cast<uint32_t>(static_cast<int32_t>(haptic_motor.get_position())));

components/bldc_haptics/example/main/bldc_haptics_example.cpp:588

  • An f32 payload can encode NaN or infinity, and std::clamp does not sanitize NaN, so a malformed WebUSB client can pass NaN directly into motor_->move() through play_haptic(). Reject non-finite strengths with std::isfinite before clamping to prevent invalid values propagating into motor/PWM calculations.
      const float clamped = std::clamp(*strength, 0.0f, 10.0f);
      //! [bldc_haptics_example_2]
      haptic_motor.play_haptic(espp::detail::HapticConfig{
          .strength = clamped,

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

  • The Abort button remains enabled while awaiting OTA_FINISH, but abortRequested is only checked in the preceding data loop. Clicking Abort during the potentially 60-second validation therefore logs that an abort was requested while the image is still activated and the device restarts. Disable Abort before sending FINISH (or implement actual finish cancellation).
        logLine("sys", "OTA FINISH: device is validating (SHA-256) + activating the image...");
        setNote("Validating image on device...");
        await transact(TYPE.OTA_FINISH, null, [TYPE.OK], FINISH_TIMEOUT_MS);

components/bldc_haptics/example/main/bldc_haptics_example.cpp:120

  • A flash core dump persists until erased or overwritten. Since this checks for any valid image without consuming it, every later clean reboot continues reporting the old crash and combines its task/PC with the current reset reason (for example, last reset: SW). After caching a successfully read summary in crash_report, erase/mark the flash dump consumed so GET_CRASH represents only the immediately preceding boot.
    if (esp_core_dump_image_check() == ESP_OK) {
      esp_core_dump_summary_t summary = {};
      if (esp_core_dump_get_summary(&summary) == ESP_OK) {
        crash_report = fmt::format("last reset: {} | crashed task '{}' PC=0x{:08x}", reason_name,

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

  • After a timeout the protocol has no request ID, yet the connection remains usable. A late reply to the timed-out command can therefore satisfy the next transaction when both accept OK; this is especially likely when an OTA erase/write exceeds its deadline and the failure path immediately sends OTA_ABORT. Treat a timeout as a desynchronized connection (close/reconnect or explicitly drain it), or add request correlation before allowing another command.
          pending.timer = setTimeout(() => {
            if (!pending) return;
            const p = pending; pending = null;
            p.reject(new Error(TYPE_NAME[type] + " response timeout"));
          }, timeoutMs);

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

  • At fractional device-pixel ratios, cssSize * dpr can be non-integral while assigning canvas.width converts it to an integer. The comparison then remains true on every animation frame, reallocating/resetting the canvas continuously. Round the backing size once before comparing and assigning it.
      const cssSize = canvas.clientWidth || 360;
      if (canvas.width !== cssSize * dpr) { canvas.width = cssSize * dpr; canvas.height = cssSize * dpr; }

components/bldc_haptics/example/README.md:71

  • This troubleshooting note now contradicts the implementation: the example instantiates CDC-ACM and routes the system console to it with esp_tusb_init_console, so runtime logs are not limited to UART pins. Document the TinyUSB CDC serial port here so users do not unnecessarily wire a UART adapter.
> On boards whose only USB connector is the ESP32-S3's native USB (e.g. MotorGo
> Mini), the connector is taken over by the TinyUSB vendor interface once the
> app starts, so serial logs are only available on the UART pins. Flashing
> still works over the same connector via the ROM bootloader (hold BOOT while
> resetting, or just use `webapp/index.html` for OTA updates after the first
> flash).

components/bldc_haptics/example/main/bldc_haptics_example.cpp:340

  • This comment says CDC is not a live console and logs remain on the default console, but the successful initialization path below calls esp_tusb_init_console(TINYUSB_CDC_ACM_0) specifically to route stdout/stderr and ESP logs to CDC. Update this description so it matches the actual system-console behavior.
  // CDC alongside the vendor interface: a plain serial port any terminal can
  // attach to (e.g. `screen /dev/tty.usbmodem*`). It carries the boot banner +
  // last-crash report on every connect - NOT a live console (espp logs go to
  // the default console, and a panic kills TinyUSB before it could print);
  // the flash core dump + next-boot report above is the backtrace path.

components/bldc_haptics/example/sdkconfig.defaults:35

  • A maximum OTA frame is 7 header bytes + 4096 payload bytes + 4 CRC bytes = 4107 bytes, not 4106. Correct the size stated here so future FIFO sizing is based on the actual wire format.

This issue also appears on line 40 of the same file.

# The vendor FIFOs default to 64 bytes on the S3 - smaller than one protocol
# frame (MODES is ~660 bytes; OTA data frames are 4106). Size them to hold a
# full frame so writes complete without waiting and RX can take a whole OTA

components/bldc_haptics/example/sdkconfig.defaults:43

  • This comment is stale: the example now instantiates a CDC function and routes the system console through it. Describing CDC as compile-only and claiming that no CDC interface exists can lead maintainers to remove required console configuration; document CDC as used and reserve the compile-only explanation for HID.
# The example itself only USES the vendor function, but the usb_device
# component's sources reference the TinyUSB CDC + HID class driver APIs, which
# esp_tinyusb gates behind these options -- keep them enabled (minimal counts)
# so the component compiles; no CDC / HID interface is instantiated here.

Comment thread components/usb_device/src/usb_device.cpp Outdated
Comment thread components/bldc_haptics/example/main/haptics_usb_protocol.hpp
finger563 added a commit that referenced this pull request Aug 29, 2026
…B FIFO sizing, IDF >=5.2, webapp label)

- CoreDump: serialize all flash-touching methods (has_core_dump / summary /
  format_report / image_size / read_image / erase) with an internal mutex so
  one CoreDump can be shared by several CoreDumpService instances /
  transports without a READ on one racing an ERASE on the other; documented
  on the class and in CoreDumpService's shared-CoreDump note.
- example sdkconfig.defaults: size the TinyUSB vendor RX/TX and CDC TX FIFOs
  (4096) to hold a complete DATA reply frame (2,063 bytes with the web
  console's 2 KiB read chunks) so the stock write path cannot truncate
  replies; PR #742 additionally makes write_vendor wait-for-drain.
- idf_component.yml: require idf >=5.2 (first release with
  esp_core_dump_get_panic_reason(), used for the ELF crash report).
- example: replace the now-stale knownConditionTrueFalse inline suppression
  (which CI reports as unmatched since b7871e9) with a
  CONFIG_ESP_COREDUMP_ENABLE_TO_FLASH guard, robust across cppcheck versions.
- CoreDumpService: make build_error() const (cppcheck functionConst).
- webapp: correct the download-panel label — core.elf drops the flash header
  before the ELF magic; the trailing checksum bytes are retained.

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

- usb_device: never sleep-wait for a vendor TX FIFO drain in TinyUSB-callback
  context. All TinyUSB callbacks record the TinyUSB task handle; write_vendor()
  called on that task (e.g. from a receive callback) now fails fast with
  no_buffer_space instead of stalling the very task that drains the FIFO and
  burning the 250 ms timeout. Contract documented on write_vendor().
- example: fix cppcheck duplicateCondition (merge the two if (usb_ok) blocks)
- example: reject non-finite PLAY_HAPTIC strengths (std::clamp passes NaN)
- example: erase the flash core dump after summarizing it so GET_CRASH only
  reports the immediately preceding boot
- example: haptics_usb_protocol.hpp directly includes <algorithm> (std::min)
- example: document SET_POSITION relabel-only semantics (PROTOCOL.md, webapp
  'Set detent index' control, handler comment) - it does not move the knob
- webapp: discard late replies to timed-out commands (in-order bulk pipe =>
  count them) instead of letting them satisfy the next transaction
- webapp: honor/disable Abort before OTA FINISH (finish cannot be cancelled)
- webapp: round the canvas backing size once (fractional DPR reallocated the
  canvas every animation frame)
- docs: CDC is the live system console (README troubleshooting, example
  comment, sdkconfig.defaults comments); OTA max frame is 4107 bytes

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

Copy link
Copy Markdown
Contributor Author

Triaged the 11 suppressed review comments; 9 fixed in 5cd99ac, 2 rebutted with evidence. Per item:

1. bldc_haptics_example.cpp:347 — landing page won't publish (symlink "absent")Rebutted. The symlink is present and tracked: git ls-files -s components/bldc_haptics/web/ shows 120000 ... haptics_console.html (→ ../example/webapp/index.html). And the docs workflow publishes it fine: build_and_publish_docs.yml uses a bash glob + cp (apps=(../components/*/web/*.html ...); cp "${apps[@]}" ../docs/apps/), not find -type f — a bash glob matches symlinks by name, and cp without -R/-P dereferences source operands, so the real HTML content lands in docs/apps/. Verified empirically with the same glob+cp on a symlinked .html (content copied, not a dangling link); the hosted apps/ index (same mechanism, e.g. apps/ota_console.html) serves 200 today.

2. bldc_haptics_example.cpp:555 — SET_POSITION doesn't move the motorCorrect observation; fixed by aligning the semantics (relabel-only), not by changing the component. BldcHaptics::set_position() indeed only re-labels the logical detent index (current_detent_center_ is untouched), so the knob does not physically move. Driving the motor to a different physical detent would be a bldc_haptics component feature change, out of scope for this example PR. Instead the protocol/UI now say what actually happens: PROTOCOL.md describes SET_POSITION as re-labeling the detent the knob currently rests in (no physical motion), the web console control is renamed "Set detent index" (with a tooltip) and logs "Detent index set to N", and the handler carries a comment citing the semantics.

3. bldc_haptics_example.cpp:588 — NaN passes through std::clamp into play_haptic()Fixed. std::clamp returns NaN for NaN, so the handler now rejects non-finite strengths with ERROR(invalid_argument) before clamping (std::isfinite, direct <cmath> include added).

4. webapp/index.html:1156 — Abort enabled during OTA FINISHFixed. Immediately before sending FINISH the app takes a last abortRequested check (covers a click during the final data chunk) and then disables the Abort button — once the device starts validating/activating, finish genuinely cannot be cancelled, so the button no longer pretends otherwise.

5. bldc_haptics_example.cpp:120 — stale core dump re-reported every bootFixed. After a successful esp_core_dump_get_summary() the example now erases the flash dump (esp_core_dump_image_erase()), so GET_CRASH / the CDC banner describe only the immediately preceding boot instead of pairing an old dump with the current reset reason. The summary (the advertised addr2line decode path) is already cached in RAM at that point.

6. webapp/index.html:686 — late reply to a timed-out command satisfies the next transactionFixed. The protocol is one-command-in-flight, the device processes commands serially, and the bulk IN pipe preserves ordering — so after a timeout the next solicited-type frame (if it ever arrives) belongs to the timed-out command. The app now counts timed-out commands (staleReplies), and the RX dispatcher discards exactly that many solicited frames (logged as late replies) before resuming normal matching; the counter resets on disconnect. This covers the OTA erase-timeout → immediate OTA_ABORT case without forcing a reconnect on every benign timeout.

7. webapp/index.html:856 — fractional DPR reallocates the canvas every frameFixed. The backing size is computed once as Math.round(cssSize * dpr) and compared/assigned as an integer, so the canvas.width !== check is stable at fractional device-pixel ratios.

8. README.md:71 — troubleshooting note says logs are UART-onlyFixed. The note now documents the CDC-ACM serial port + system console routing (screen /dev/tty.usbmodem*) as the live log path over the same cable; only the USB-Serial-JTAG idf.py monitor console goes away.

9. bldc_haptics_example.cpp:340 — comment says CDC is "NOT a live console"Fixed. The comment predated esp_tusb_init_console(); it now says CDC becomes the live system console (stdout/stderr, all espp/esp_log output) once USB is up, while keeping the (still true) caveat that panic backtraces can't appear live and go through the flash core dump path.

10. sdkconfig.defaults:35 — max OTA frame is 4107, not 4106Fixed. 7 header (magic u16 + type u8 + len u32) + 4096 payload + 4 CRC = 4107; both comment occurrences corrected (FIFO sizing at 4200 is unaffected).

11. sdkconfig.defaults:43 — comment claims no CDC interface is instantiatedFixed. The comment now documents CDC as instantiated and used (debug serial + system console) and reserves the compile-only rationale for HID (CONFIG_TINYUSB_HID_COUNT kept > 0 so the usb_device component's HID references compile).

Also in 5cd99ac: the static_analysis CI failure (cppcheck duplicateCondition on the back-to-back if (usb_ok) blocks at bldc_haptics_example.cpp:683/686) is fixed by merging them into one if/else; local cppcheck with the CI recipe is clean on the PR files, and the default-config example builds.

@finger563
finger563 requested a balanced review from Copilot August 29, 2026 02:53

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

Comment thread components/usb_device/src/usb_device.cpp
Comment thread components/bldc_haptics/example/main/haptics_usb_protocol.hpp Outdated
Comment thread components/bldc_haptics/example/webapp/index.html
…dback

- usb_device: replace the std::this_thread::sleep_for / std::chrono
  drain-wait in write_vendor() with FreeRTOS primitives (vTaskDelay +
  xTaskGetTickCount tick deadline, wraparound-safe); same 250 ms bound
  and unmounted / TinyUSB-task fast-exits, drops the <chrono>/<thread>
  includes
- haptics_usb_protocol: use std::bit_cast<int32_t> in get_i32_at() for a
  guaranteed bit-preserving u32->i32 conversion (static_cast is
  implementation-defined for values with the high bit set)

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

Suppressed comments (4)

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

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

  • acceptAllDevices is not a WebUSB USBDeviceRequestOptions member; requestDevice() requires filters, so selecting this checkbox rejects with TypeError instead of opening an unfiltered chooser. Remove this option, or use a supported broader filter (for example the vendor interface class) and relabel the checkbox accordingly.
        const options = els.anyDevice.checked
          ? { acceptAllDevices: true }
          : { filters: [{ vendorId: DEFAULT_VID, productId: DEFAULT_PID }] };

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

  • Discarding the next solicited frame assumes every timed-out command eventually replies. If it never does, the next command's valid response is discarded and every later command times out in turn. This occurs in the documented older-firmware path: a timed-out GET_CRASH increments this counter, then the following SET_STREAMING OK is dropped. Track stale replies by their expected type, or reset/reconnect the protocol after a timeout rather than continuing with an ambiguous stream.
      if (staleReplies > 0) {
        staleReplies--;
        logLine("sys", "Discarding late " + (TYPE_NAME[frame.type] || ("0x" + frame.type.toString(16))) +
                " reply to a previously timed-out command.");
        return;

components/bldc_haptics/example/main/bldc_haptics_example.cpp:131

  • The project is declared as bldc_haptics_example, so ESP-IDF generates build/bldc_haptics_example.elf. The reported decode command points to a nonexistent ELF and will fail when users try to decode a crash.
            "\ndecode with: xtensa-esp32s3-elf-addr2line -pfiaC -e build/bldc_haptics.elf <addrs>";

components/bldc_haptics/example/PROTOCOL.md:194

  • This decode example names build/bldc_haptics.elf, but project(bldc_haptics_example) produces build/bldc_haptics_example.elf; following the documented command therefore fails to open the ELF.
`xtensa-esp32s3-elf-addr2line -pfiaC -e build/bldc_haptics.elf <addrs>`);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants