feat(canopen): DS402 drive panel web app (in-browser SDO over the CAN bridge) - #752
Conversation
There was a problem hiding this comment.
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
Adds a single-file, offline DS402 commissioning web app that communicates with CANopen nodes via SDO over the existing USB↔CAN bridge, and documents it in the CANopen bus docs.
Changes:
- Document DS402 commissioning workflow and link to the hosted DS402 panel app.
- Add
ds402_panel.html: WebUSB/Web Serial transport, stream_frame framing, CANopen SDO client, DS402 UI, and generic object dictionary read/write.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| doc/en/buses/canopen.rst | Adds documentation + link to the new DS402 drive panel web app. |
| components/canopen/web/ds402_panel.html | Introduces the in-browser SDO client and DS402 commissioning UI over the existing raw-CAN bridge. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
441c1ab to
58e12ce
Compare
162dfc1 to
094c881
Compare
|
Addressed the review comments in
Verified: |
|
✅Static analysis result - no issues found! ✅ |
There was a problem hiding this comment.
🟡 Changes recommended
The new web app has confirmed UI/state handling and performance issues (config buttons not reflecting running state; unconditional per-frame logging) that can lead to invalid operations and UI degradation on real CANopen traffic.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
… CAN bridge)
A single-file, offline, dependency-free browser front-end that commissions a
CANopen CiA 402 (DS402) drive through the existing USB<->CAN bridge
(can_bridge_example). It reuses the CAN console's transport + stream_frame
framing to move raw CAN frames and layers all CANopen/DS402 logic in the browser
("architecture option A") - the firmware stays a dumb raw-CAN pipe, no firmware
change needed.
Includes:
- An in-browser CANopen SDO client: expedited + segmented upload (read) and
download (write) on the default SDO channel (0x600/0x580 + nodeId), with abort
decoding and one-transaction-at-a-time serialization.
- A DS402 panel: statusword (0x6041) -> power-drive-system state decode,
controlword (0x6040) command buttons + enable sequence / quick-stop /
fault-reset, mode of operation (0x6060/0x6061), target and actual
velocity/position/torque, and an opt-in live poll.
- Node identity (0x1000/0x1008/0x1018) and NMT controls.
- A generic object-dictionary panel to read/write ANY index:subindex with a
chosen data type (u8..i32/string/raw hex), for vendor-specific objects.
Because it drives the node over SDO (not cyclic PDOs) it is a commissioning /
bring-up tool; the bus must be in Normal mode so the bridge ACKs the node.
The apps index auto-lists it from its <title>/<meta description>; canopen.rst
links it next to the CAN console. Verified: node --check clean, and a runtime
test of the SDO expedited/segmented up/download codec + DS402 state decode
against a mock CANopen server (13/13 round-trips).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The <meta name="description"> content had a literal "USB<->CAN"; an unescaped < makes the tag invalid HTML. Reword to "USB-to-CAN" (same fix as can_console). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… poll re-entrancy
Address the review on the DS402 web app:
- Disconnect / link-loss now abort any outstanding SDO transaction: new
abortSdo() clears the waiter timer, rejects its pending Promise, and resets
sdoTxnActive. Without this a link drop mid-SDO left the channel wedged ("SDO
channel busy" / "another SDO transaction in progress") after reconnect and a
Promise pending until timeout. Called from disconnect() and onLinkLost().
- Segmented SDO download now validates the server echoed the expected toggle bit
(like the upload path) and throws on mismatch, so a desynced channel cannot
silently corrupt a write.
- refreshDs402() is guarded by a refreshInFlight flag around the whole multi-read
sequence: sdoTxnActive is released between individual reads, so a poll tick or
manual refresh could otherwise start a second refresh mid-sequence and
interleave reads. The manual button no longer needs its own sdoTxnActive check.
- Clarified the NMT comment: the UI always targets the selected node (1-127,
shared with the SDO client); it does not expose CANopen node-0 broadcast.
Verified: node --check clean, and the SDO codec round-trip test still passes
(segmented upload + download with the new toggle check).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e logging - setStatusLine() now reflects the bus run state in the controls (only while connected): Apply + Start are disabled while the bus is running (SET_CONFIG is rejected by the firmware then) and Stop is disabled while stopped - matching the CAN bridge console, so Apply is not clickable with its "only while stopped" tooltip during a run. - onIncoming() logged every non-SDO CAN frame to the DOM unconditionally; on a live CANopen bus (heartbeats / PDOs / EMCY) that floods the DOM and freezes the UI. Gate it behind a new "log bus frames" toggle (default off), mirroring the SDO-frame toggle. - Update the can_console.html references to can_bridge_console.html (renamed on main in #753). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
d26ba7b to
3bcbbcb
Compare
|
Addressed the review comments (and rebased onto current
|
There was a problem hiding this comment.
🟡 Changes recommended
SDO matching, validation, and command sequencing contain correctness issues that can return stale data or send unintended drive commands.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
components/canopen/web/ds402_panel.html:994
- Invalid or empty node input silently falls back to node 1. A user who entered 0, 128, or cleared the field can therefore send a controlword/NMT command to the wrong drive. Reject the action and surface validation feedback instead of substituting another node ID.
function currentNode() {
const n = parseInt(els.nodeId.value, 10);
return (n >= 1 && n <= 127) ? n : 1;
components/canopen/web/ds402_panel.html:1203
guardedreturnsundefinedboth for successful void writes and for failures, so the controlword, mode, and target handlers always emit a success log after an SDO timeout/abort. Return an explicit success result (or rethrow) and only log success/refresh after a confirmed write.
async function guarded(label, fn) {
// Run an SDO-using action, surfacing errors to the log without throwing.
try { return await fn(); }
catch (e) { logLine("err", label + ": " + (e && e.message ? e.message : e)); return undefined; }
components/canopen/web/ds402_panel.html:1242
- The first
sendControlwordstartsrefreshDs402()before resolving. That refresh acquiressdoTxnActive, so the second call immediately fails as “another SDO transaction is in progress”; bit 7 is never sent. Perform both writes directly as one sequence, then refresh once.
// Fault reset is a rising edge on bit 7: clear it, then set it.
await sendControlword(0x0000, "clear (pre fault-reset)");
await sendControlword(CW.faultReset, "fault reset");
components/canopen/web/ds402_panel.html:1319
- The write address is accepted without range checks, but the SDO encoder masks it to 16/8 bits. For example, index
10000(hex) or subindex256silently targets0x0000:0, potentially writing a different object than the UI reports. Validate index0..0xFFFFand subindex0..0xFF; apply the same check to the read handler.
try { index = parseIntFlexible(els.odIndex.value.startsWith("0x") ? els.odIndex.value : "0x" + els.odIndex.value); sub = parseIntFlexible(els.odSub.value); }
catch (e) { els.odResult.textContent = "error: " + e.message; return; }
components/canopen/web/ds402_panel.html:1247
- Masking the raw controlword silently aliases out-of-range input; for example
10000is displayed as a valid parse but sends0x0000(Disable Voltage). Reject values outside0x0000..0xFFFFand pass the validated value unchanged.
let v; try { v = parseIntFlexible(els.cwRaw.value.startsWith("0x") ? els.cwRaw.value : "0x" + els.cwRaw.value); }
catch (_) { logLine("err", "bad controlword hex"); return; }
sendControlword(v & 0xFFFF, "raw");
- Files reviewed: 2/2 changed files
- Comments generated: 7
- Review effort level: Balanced
…le sequence Address the SDO/DS402 correctness review on the DS402 web app: - SDO response matching: sdoDeliver() now requires an EXACT 8-byte standard (non-RTR, non-extended) frame and, for phases whose response echoes the multiplexer (expedited/initiate/abort), that index:subindex matches the pending transfer. A late response for an earlier index can no longer complete the next transaction with stale data / a false ack, and stray bus traffic on the response COB-ID is no longer zero-padded into a fake response. Segment phases carry no multiplexer and stay matched by phase + toggle. - Node snapshot: withSdo() captures the node id ONCE and threads it through the whole transaction, so a node-field change mid segmented transfer cannot send the initiate and later segments to different servers. - Segmented upload rejects a short transfer (declared size > delivered bytes) instead of returning truncated data as a successful read. - Scalar writes are range-validated before encoding (u8=-1 / i16=65535 / oversized 32-bit no longer silently wrap); scalar reads reject an undersized object instead of zero-extending it into a valid-looking value. Zero-length writes are rejected (no valid SDO expedited encoding). Target/controlword handlers only log the value as sent when the write actually succeeded. - The "Enable operation" button now performs the real state-aware enable sequence (Shutdown -> Switch On -> Enable Operation, with fault reset and quick-stop exit), polling the statusword between transitions - it no longer just writes 0x000F, which fails from Switch-on-disabled. Relabeled "Enable sequence". Verified: node --check clean + a runtime test of the range/undersized/truncation/ empty-write/segmented-codec logic (15/15). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the SDO/DS402 correctness review (
Verified: |
There was a problem hiding this comment.
🟡 Changes recommended
Drive-control races, unsafe address wrapping, and misleading success handling must be corrected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
components/canopen/web/ds402_panel.html:1036
- Invalid or out-of-range node IDs silently become node 1. Because the number input's min/max are not automatically enforced by these button handlers, entering
128or leaving it blank can send SDO/NMT control commands to the wrong drive. Reject the value and surface a validation error instead of defaulting to a valid node.
function currentNode() {
const n = parseInt(els.nodeId.value, 10);
return (n >= 1 && n <= 127) ? n : 1;
}
components/canopen/web/ds402_panel.html:1259
- Each read in this refresh independently calls
currentNode(). With live polling enabled, changing the node selector during the five SDO round trips can leave the panel showing a statusword from one drive and mode/actual values from another. Pin one node for the whole refresh (and discard results if selection changes).
const sw = await guarded("read statusword", () => readObject(OD.statusword[0], OD.statusword[1], "u16"));
if (sw != null) setDs402State(sw);
const md = await guarded("read mode display", () => readObject(OD.modeDisplay[0], OD.modeDisplay[1], "i8"));
if (md != null) els.modeDisplay.textContent = md + " (" + (MODE_OP_NAME[md] || "?") + ")";
const pos = await guarded("read position actual", () => readObject(OD.positionActual[0], OD.positionActual[1], "i32"));
- Files reviewed: 2/2 changed files
- Comments generated: 9
- Review effort level: Balanced
…, address bounds Address the remaining SDO/DS402 robustness review: - Node pinning across multi-transaction actions: withSdo() accepts an explicit node (default = the field value). The enable sequence and the identity read now snapshot the node ONCE and pass it to every readObject/sdoDownload, so editing the node field mid-action can no longer read state from one drive and command another. The enable sequence also locks the node field while it runs. - Enable sequence is cancellable: Quick stop sets a cancel flag (and waits for the sequence to release the SDO channel) before issuing quick stop, checked after every awaited SDO op - so quick stop can no longer be undone by the sequence driving the state machine back to Operation enabled. - Fault reset performs both controlword writes (clear -> 0x80) DIRECTLY instead of via sendControlword, whose trailing refreshDs402() would grab the SDO channel and fail the second write, so bit 7 now gets its rising edge. - Object address validation: sdoUpload/sdoDownload reject index outside 0x0000..0xFFFF and subindex outside 0..255 before packing the low bits, so a typo like hex 16040 (0x16040) can no longer be silently sent as 0x6040. - Segmented uploads are bounded (SDO_MAX_TRANSFER = 64 KiB) for both a declared oversized total and a node that never sets the last-segment bit; on overflow we send a client SDO abort and fail, so a faulty node cannot grow browser memory. - Serial read loop treats a clean EOF while still reading as a link failure (not a silent normal return), so a port that closes under us tears the UI down instead of leaving it "Connected". Normal cancellation during close() is unaffected. - Mode-of-operation set logs success only inside the successful write path (no more success message after an SDO abort/timeout/busy error). Verified: node --check clean + runtime tests of the address-validation and upload-bound logic. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the remaining SDO/DS402 robustness comments (
Verified: |
…elated fixes A focused self-review (plus an independent adversarial pass) surfaced one real bug and several hardening items: - HIGH: Quick stop could be silently dropped. The enable sequence's finally ran refreshDs402() which re-acquired the SDO channel exactly as the cancelled sequence handed control back to Quick stop, so the quick-stop write failed with "another SDO transaction in progress" and was never sent. Root cause: the SDO channel failed fast on contention. Replace the fail-fast mutex with a QUEUE (withSdo enqueues; pumpSdoQueue runs one transaction at a time in FIFO order), so a poll tick, the enable sequence and manual commands serialize instead of the loser being dropped. abortSdo() now also drains the queue on disconnect. Additionally the enable sequence skips its trailing refresh when cancelled, so the quick-stop write runs immediately. - Manual commands issued during a live-poll burst (or a trailing refresh) are no longer dropped with a "busy" error - they queue and run in order. - The enable sequence now locks the competing command controls (controlword / mode / target / OD-write / NMT / node field) while it runs; Quick stop stays enabled (it cancels the sequence) and reads stay enabled. - currentNode() rejects an out-of-range node id (1..127) instead of silently clamping to 1, so a command / identity read can no longer target a different drive than the field shows. The enable-sequence and identity handlers surface the error. - disconnect()/onLinkLost() set the sequence cancel flag (matching the comment) and the sequence's finally no longer refreshes after a disconnect (was spamming "not connected"). - OD read shows scalar hex at the object's own width (i16 -1 -> 0xffff, not 0xffffffff). Connecting with Live poll pre-ticked now actually starts polling. Verified: node --check clean + a runtime test of the queue (serialization, FIFO order, abort-drain), currentNode validation, and hex width (13/13). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A purpose-built, single-file, offline web app for commissioning CiA 402 (DS402) drives through the existing USB<->CAN bridge (
can_bridge_example), using architecture option A: all CANopen/DS402 logic runs in the browser over raw CAN — the firmware stays a dumb raw-CAN pipe, so no firmware change.Reuses the CAN console's transport +
stream_frameframing; adds:0x600/0x580+ nodeId), abort-code decoding, one-transaction-at-a-time.0x6041) → power-drive-system state decode; controlword (0x6040) command buttons + enable sequence / quick-stop / fault-reset; mode of operation (0x6060/0x6061); target & actual velocity/position/torque; opt-in live poll.0x1000/0x1008/0x1018) and NMT controls.u8..i32/string/raw hex), for vendor-specific objects.Since it drives the node over SDO (not cyclic PDOs) it is a commissioning / bring-up tool; the bus must be in Normal mode so the bridge ACKs the node.
Notes
<title>/<meta description>;canopen.rstlinks it beside the CAN console.node --checkclean, plus a runtime test of the SDO expedited/segmented up/download codec + DS402 state decode against a mock CANopen server (13/13 round-trips). Hardware test against a real DS402 drive still pending.can_bridge_examplefirmware). Base will retarget tomainonce feat(canopen): USB<->CAN bridge example + WebUSB/Web Serial CAN console #748 merges.🤖 Generated with Claude Code