feat: CODESYS-compatible SoftMotion (CiA 402 EtherCAT axes) - #928
Merged
Conversation
…web) Byte-identical mirror of the shared SoftMotion surface from openplc-web (PR to be released together): - cia402.ts: recognize CiA 402 drives + resolve objects to IEC addresses - generate-softmotion.ts: compile-time AXIS_REF_SM3 globals + PDO scalars + per-scan SM_Drive_GenericDS402 bridge + VAR_EXTERNAL injection - enrichDeviceData tags recognized drives; open-plc/esi-types carry the Cia402AxisConfig; preprocessPous runs the codegen in every compile path - shared tests (cia402, generate-softmotion, preprocess-pous) pass under the editor's jest - binary-versions: strucpp v0.5.13 -> v0.5.14 (ships the SM3 library + FB inout copy-back + composite shared globals) The editor loads plcopen-softmotion.stlib via its bundled-libs directory scan (no bundled-stlibs.ts change needed). Requires strucpp release v0.5.14 (STruCpp PR #194). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds CiA 402 servo recognition, persisted SoftMotion axis configuration, PLC bridge generation, preprocessing and language-server integration, editor controls, project-tree presentation, IEC axis-name validation, and tests. It also updates the strucpp version. ChangesCiA 402 SoftMotion Feature
Dependency Version Update
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant EtherCATEditor
participant DeviceStore
participant preprocessPous
participant generateSoftMotionArtifacts
participant ProjectSync
participant PLCProject
EtherCATEditor->>DeviceStore: update CiA 402 axis configuration
DeviceStore->>preprocessPous: provide configured project
preprocessPous->>generateSoftMotionArtifacts: generate axis artifacts
generateSoftMotionArtifacts->>PLCProject: add globals, bridge POU, and task instance
ProjectSync->>PLCProject: synthesize SoftMotion globals document
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/backend/shared/utils/PLC/__tests__/preprocess-pous.test.ts (2)
313-359: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test for the
isSimulator = true+ CiA402 device combination.This new test only exercises
preprocessPous(project, false, logger.log). Given the concern raised inpreprocess-pous.tsabout whether SoftMotion generation should differ under simulator mode, a companion test withisSimulator = truewould help pin down/verify the intended behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/shared/utils/PLC/__tests__/preprocess-pous.test.ts` around lines 313 - 359, The SoftMotion CiA 402 coverage in preprocess-pous.test only verifies the non-simulator path; add a companion test for the same X_Axis/ethercat setup with preprocessPous called in simulator mode to lock down the intended behavior. Reuse the existing preprocessPous, makeProjectData, and collectLog setup, but pass isSimulator=true and assert whether the __sm3_bridge, AXIS_REF_SM3 global, and main interface external injection should still be generated or omitted based on the expected simulator behavior.
317-346: 📐 Maintainability & Code Quality | 🔵 Trivial
as anycast bypasses strict typing on the EtherCAT remote device fixture.The test object is cast wholesale with
as any(plus an eslint-disable) rather than conforming to the typedPLCRemoteDevice/ConfiguredEtherCATDeviceshapes. This bypasses compile-time verification that the fixture matches the real contract consumed bycollectAxes/resolveCia402Objects, and violates the strict-mode "avoid any types" rule for files undersrc/**/*.{ts,tsx}.Consider building the fixture as a properly-typed
PLCRemoteDevice/ConfiguredEtherCATDeviceobject instead.
[medium_effort_and_high_reward]As per coding guidelines, "Keep TypeScript in strict mode and avoid
anytypes."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/shared/utils/PLC/__tests__/preprocess-pous.test.ts` around lines 317 - 346, The EtherCAT remote device fixture is bypassing strict typing by casting the object to any, so update the test data to conform to the real PLCRemoteDevice and ConfiguredEtherCATDevice shapes instead. Remove the wholesale as any cast and the eslint-disable, and adjust the fixture in preprocess-pous.test.ts so collectAxes and resolveCia402Objects receive a fully typed remoteDevices entry with all required fields present.Source: Coding guidelines
src/backend/shared/ethercat/generate-softmotion.ts (1)
118-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilently dropped duplicate/unresolvable axes give no diagnostic signal.
When a sanitized axis name collides with a previously-seen one, the later device is silently skipped (comment: "later devices lose — surfaced by compile if referenced"). Relying on a downstream "undefined variable" compile error to surface this is a poor diagnostic experience — the actual root cause (two EtherCAT devices sanitizing to the same identifier) won't be obvious to the user.
Consider threading a
logcallback through togenerateSoftMotionArtifacts(similar to the one already used inpreprocessPous) to warn explicitly when an axis is skipped due to a name collision.♻️ Suggested approach
-export function generateSoftMotionArtifacts(project: PLCProjectData): PLCProjectData { - const axes = collectAxes(project) +export function generateSoftMotionArtifacts(project: PLCProjectData, log?: (level: 'info' | 'warn', msg: string) => void): PLCProjectData { + const axes = collectAxes(project, log)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/shared/ethercat/generate-softmotion.ts` around lines 118 - 122, The duplicate-axis handling in generateSoftMotionArtifacts silently skips later devices when sanitizeAxisName produces a collision, so add an explicit warning instead of relying on downstream compile failures. Thread a log callback into generateSoftMotionArtifacts, similar to preprocessPous, and use it where the seen set check currently continues. Include the original device name and the sanitized axis name in the warning so users can identify the collision source quickly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/shared/ethercat/cia402.ts`:
- Around line 78-87: The docstring for normalizeObjectIndex is misleading
because it lists “24640” as a parsed form even though the implementation always
interprets the stripped input as hex. Update the comment on normalizeObjectIndex
to remove or clarify that example so it only describes supported input forms,
and make sure the wording matches the actual parseInt(s, 16) behavior for raw
string inputs.
In `@src/backend/shared/ethercat/generate-softmotion.ts`:
- Around line 149-231: The generated SoftMotion identifiers can collide with
existing project declarations because generate-softmotion.ts appends newGlobals,
bridgePou, bridgeInstance, and per-axis names without checking for duplicates.
Add collision detection in the bridge generation flow (around the loops building
newGlobals/bridgeVars and the final project merge) against existing
resource.globalVariables, project.pous, and resource.instances, and either
prefix generated names or fail fast with a clear diagnostic naming the
conflicting axis/PDO/device and the reserved symbol such as __sm3_bridge or
__sm3_bridge_inst.
- Around line 207-216: The bridge instance is currently bound to the first task
in generate-softmotion, which may be an Interrupt or otherwise unsuitable cycle.
Update the task selection logic near SM3_BRIDGE_INSTANCE_NAME to choose the
fastest Cyclic task from resource.tasks, and fall back to a dedicated motion
task or SM3_FALLBACK_TASK only when no appropriate cyclic task exists. Keep the
bridge attached to the chosen task when building bridgeInstance so scheduling is
deterministic and suitable for PDO/axis updates.
In `@src/backend/shared/types/PLC/open-plc.ts`:
- Around line 716-721: Tighten the Cia402AxisConfigSchema validation so invalid
axis scaling values are rejected before SoftMotion generation. Update the schema
in open-plc.ts to make scaleNum and scaleDenom integer-only, and require
scaleDenom to be greater than zero while keeping scaleFactor as-is. This should
align the schema with generate-softmotion.ts, which already truncates these
fields, and prevent invalid axis configs from reaching the generated code.
---
Nitpick comments:
In `@src/backend/shared/ethercat/generate-softmotion.ts`:
- Around line 118-122: The duplicate-axis handling in
generateSoftMotionArtifacts silently skips later devices when sanitizeAxisName
produces a collision, so add an explicit warning instead of relying on
downstream compile failures. Thread a log callback into
generateSoftMotionArtifacts, similar to preprocessPous, and use it where the
seen set check currently continues. Include the original device name and the
sanitized axis name in the warning so users can identify the collision source
quickly.
In `@src/backend/shared/utils/PLC/__tests__/preprocess-pous.test.ts`:
- Around line 313-359: The SoftMotion CiA 402 coverage in preprocess-pous.test
only verifies the non-simulator path; add a companion test for the same
X_Axis/ethercat setup with preprocessPous called in simulator mode to lock down
the intended behavior. Reuse the existing preprocessPous, makeProjectData, and
collectLog setup, but pass isSimulator=true and assert whether the __sm3_bridge,
AXIS_REF_SM3 global, and main interface external injection should still be
generated or omitted based on the expected simulator behavior.
- Around line 317-346: The EtherCAT remote device fixture is bypassing strict
typing by casting the object to any, so update the test data to conform to the
real PLCRemoteDevice and ConfiguredEtherCATDevice shapes instead. Remove the
wholesale as any cast and the eslint-disable, and adjust the fixture in
preprocess-pous.test.ts so collectAxes and resolveCia402Objects receive a fully
typed remoteDevices entry with all required fields present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2b811131-ae34-49e3-8db6-c6ac92ea988b
📒 Files selected for processing (11)
binary-versions.jsonsrc/backend/shared/ethercat/__tests__/cia402.test.tssrc/backend/shared/ethercat/__tests__/fixtures/cia402-servo-esi.xmlsrc/backend/shared/ethercat/__tests__/generate-softmotion.test.tssrc/backend/shared/ethercat/cia402.tssrc/backend/shared/ethercat/enrich-device-data.tssrc/backend/shared/ethercat/generate-softmotion.tssrc/backend/shared/types/PLC/open-plc.tssrc/backend/shared/utils/PLC/__tests__/preprocess-pous.test.tssrc/backend/shared/utils/PLC/preprocess-pous.tssrc/middleware/shared/ports/esi-types.ts
Comment on lines
+78
to
+87
| /** Parse a hex object index in any ESI form (`#x6040`, `0x6040`, `6040`, `24640`). */ | ||
| export function normalizeObjectIndex(raw: string | number): number { | ||
| if (typeof raw === 'number') return raw | ||
| const s = raw.trim().replace(/^#x/i, '').replace(/^0x/i, '') | ||
| // ESI indices are hex; a bare token like "6040" is hex, not decimal. Reject | ||
| // anything that isn't a pure hex string (parseInt would leniently read a | ||
| // leading hex prefix like "b" out of "bogus"). | ||
| if (!/^[0-9a-f]+$/i.test(s)) return -1 | ||
| return parseInt(s, 16) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Docstring example doesn't match implementation.
The docstring says this parses 24640 as a valid form, but the function always treats the (stripped) input as hex. normalizeObjectIndex('24640') parses "24640" with parseInt(s, 16), yielding 149056, not 24640 (0x6040). If "24640" is meant to illustrate the decimal equivalent, it's misleading as written in a "forms this function parses" list and could lead a future caller to assume decimal strings are supported.
📝 Suggested docstring fix
-/** Parse a hex object index in any ESI form (`#x6040`, `0x6040`, `6040`, `24640`). */
+/**
+ * Parse a hex object index in any ESI form (`#x6040`, `0x6040`, `6040`).
+ * Note: the input is always interpreted as hex; e.g. `6040` here means `0x6040` (decimal 24640).
+ */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** Parse a hex object index in any ESI form (`#x6040`, `0x6040`, `6040`, `24640`). */ | |
| export function normalizeObjectIndex(raw: string | number): number { | |
| if (typeof raw === 'number') return raw | |
| const s = raw.trim().replace(/^#x/i, '').replace(/^0x/i, '') | |
| // ESI indices are hex; a bare token like "6040" is hex, not decimal. Reject | |
| // anything that isn't a pure hex string (parseInt would leniently read a | |
| // leading hex prefix like "b" out of "bogus"). | |
| if (!/^[0-9a-f]+$/i.test(s)) return -1 | |
| return parseInt(s, 16) | |
| } | |
| /** | |
| * Parse a hex object index in any ESI form (`#x6040`, `0x6040`, `6040`). | |
| * Note: the input is always interpreted as hex; e.g. `6040` here means `0x6040` (decimal 24640). | |
| */ | |
| export function normalizeObjectIndex(raw: string | number): number { | |
| if (typeof raw === 'number') return raw | |
| const s = raw.trim().replace(/^`#x/i`, '').replace(/^0x/i, '') | |
| // ESI indices are hex; a bare token like "6040" is hex, not decimal. Reject | |
| // anything that isn't a pure hex string (parseInt would leniently read a | |
| // leading hex prefix like "b" out of "bogus"). | |
| if (!/^[0-9a-f]+$/i.test(s)) return -1 | |
| return parseInt(s, 16) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/shared/ethercat/cia402.ts` around lines 78 - 87, The docstring
for normalizeObjectIndex is misleading because it lists “24640” as a parsed form
even though the implementation always interprets the stripped input as hex.
Update the comment on normalizeObjectIndex to remove or clarify that example so
it only describes supported input forms, and make sure the wording matches the
actual parseInt(s, 16) behavior for raw string inputs.
Comment on lines
+149
to
+231
| const newGlobals: PLCVariable[] = [] | ||
| const bridgeVars: PLCVariable[] = [] | ||
| const bodyLines: string[] = [] | ||
|
|
||
| for (const axis of axes) { | ||
| // AXIS_REF_SM3 instance (the name used in MC_*(Axis := ...)) — a config | ||
| // global; the bridge reaches it via VAR_EXTERNAL. | ||
| newGlobals.push(global(axis.axisName, 'AXIS_REF_SM3', 'derived', '')) | ||
| bridgeVars.push(external(axis.axisName, 'AXIS_REF_SM3', 'derived')) | ||
|
|
||
| bodyLines.push(`(* ---- SoftMotion axis ${axis.axisName} ---- *)`) | ||
| // Apply configured scaling each scan (device config is authoritative). | ||
| bodyLines.push(`${axis.axisName}.iRatioTechUnitsNum := DINT#${Math.trunc(axis.scaleNum)};`) | ||
| bodyLines.push(`${axis.axisName}.dwRatioTechUnitsDenom := DWORD#${Math.trunc(axis.scaleDenom)};`) | ||
| bodyLines.push(`${axis.axisName}.fScalefactor := ${lrealLiteral(axis.scaleFactor)};`) | ||
|
|
||
| const inBinds: string[] = [] | ||
| const outBinds: string[] = [] | ||
| for (const obj of axis.objects) { | ||
| const iecType = obj.binding.iecType.toLowerCase() | ||
| // located scalar global bound to the drive PDO address... | ||
| newGlobals.push(global(obj.scalarName, iecType, 'base-type', obj.iecLocation)) | ||
| // ...and the bridge's VAR_EXTERNAL view of it. | ||
| bridgeVars.push(external(obj.scalarName, iecType, 'base-type')) | ||
| if (obj.binding.pinKind === 'input') inBinds.push(`${obj.binding.pin} := ${obj.scalarName}`) | ||
| else outBinds.push(`${obj.binding.pin} => ${obj.scalarName}`) | ||
| } | ||
|
|
||
| const fbInstance = `${axis.axisName}_drive` | ||
| bridgeVars.push(local(fbInstance, 'SM_Drive_GenericDS402', 'derived')) | ||
| const binds = [`Axis := ${axis.axisName}`, ...inBinds, 'bOnline := TRUE', ...outBinds] | ||
| bodyLines.push(`${fbInstance}(`) | ||
| bodyLines.push(`\t${binds.join(',\n\t')});`) | ||
| } | ||
|
|
||
| const bridgePou: PLCPou = { | ||
| name: SM3_BRIDGE_POU_NAME, | ||
| pouType: 'program', | ||
| interface: { variables: bridgeVars }, | ||
| body: { language: 'st', value: bodyLines.join('\n') }, | ||
| documentation: 'Auto-generated SoftMotion drive bridge — do not edit; regenerated each compile.', | ||
| } | ||
|
|
||
| // Inject a VAR_EXTERNAL for each axis into user programs that reference it, so | ||
| // `MC_*(Axis := X_Axis)` resolves without the user declaring the global. | ||
| const patchedPous = project.pous.map((pou) => { | ||
| if (pou.pouType !== 'program') return pou | ||
| const declared = new Set((pou.interface?.variables ?? []).map((v) => v.name.toUpperCase())) | ||
| const toAdd = axes | ||
| .filter((a) => !declared.has(a.axisName.toUpperCase()) && bodyReferences(pou.body.value, a.axisName)) | ||
| .map((a) => external(a.axisName, 'AXIS_REF_SM3', 'derived')) | ||
| if (toAdd.length === 0) return pou | ||
| return { | ||
| ...pou, | ||
| interface: { ...pou.interface, variables: [...(pou.interface?.variables ?? []), ...toAdd] }, | ||
| } | ||
| }) | ||
|
|
||
| const resource = project.configurations.resource | ||
| // Ensure a task exists to run the bridge, then attach the bridge instance at | ||
| // the FRONT of the instance list so it runs before user POUs each scan | ||
| // (fresh PDO feedback in, commands out). | ||
| const tasks = resource.tasks.length > 0 ? resource.tasks : [SM3_FALLBACK_TASK] | ||
| const bridgeInstance: PLCInstance = { | ||
| name: SM3_BRIDGE_INSTANCE_NAME, | ||
| task: tasks[0].name, | ||
| program: SM3_BRIDGE_POU_NAME, | ||
| } | ||
|
|
||
| return { | ||
| ...project, | ||
| pous: [...patchedPous, bridgePou], | ||
| configurations: { | ||
| ...project.configurations, | ||
| resource: { | ||
| ...resource, | ||
| tasks, | ||
| globalVariables: [...resource.globalVariables, ...newGlobals], | ||
| instances: [bridgeInstance, ...resource.instances], | ||
| }, | ||
| }, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
No collision detection for generated identifiers against pre-existing project data.
newGlobals, bridgeVars, bridgePou (__sm3_bridge), and bridgeInstance (__sm3_bridge_inst) are unconditionally appended/concatenated onto resource.globalVariables, project.pous, and resource.instances without checking whether a name collision already exists (e.g. a user-declared global variable named the same as an axis/PDO scalar, or an existing POU/instance already named __sm3_bridge/__sm3_bridge_inst). Since axis names come directly from user-editable EtherCAT device names with no reserved-namespace prefix, a collision is plausible and would silently produce duplicate declarations, which will likely surface as a confusing downstream compiler error rather than a clear diagnostic pointing at the actual cause.
Consider validating/prefixing generated names, or failing fast with a clear message identifying the offending device when a collision with existing project data is detected.
[medium_effort_and_high_reward]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/shared/ethercat/generate-softmotion.ts` around lines 149 - 231,
The generated SoftMotion identifiers can collide with existing project
declarations because generate-softmotion.ts appends newGlobals, bridgePou,
bridgeInstance, and per-axis names without checking for duplicates. Add
collision detection in the bridge generation flow (around the loops building
newGlobals/bridgeVars and the final project merge) against existing
resource.globalVariables, project.pous, and resource.instances, and either
prefix generated names or fail fast with a clear diagnostic naming the
conflicting axis/PDO/device and the reserved symbol such as __sm3_bridge or
__sm3_bridge_inst.
Comment on lines
+207
to
+216
| const resource = project.configurations.resource | ||
| // Ensure a task exists to run the bridge, then attach the bridge instance at | ||
| // the FRONT of the instance list so it runs before user POUs each scan | ||
| // (fresh PDO feedback in, commands out). | ||
| const tasks = resource.tasks.length > 0 ? resource.tasks : [SM3_FALLBACK_TASK] | ||
| const bridgeInstance: PLCInstance = { | ||
| name: SM3_BRIDGE_INSTANCE_NAME, | ||
| task: tasks[0].name, | ||
| program: SM3_BRIDGE_POU_NAME, | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file around the reported lines
sed -n '180,240p' src/backend/shared/ethercat/generate-softmotion.ts
printf '\n---\n'
# Find task-related types and any logic that determines task ordering/selection
rg -n "interface .*Task|type .*Task|tasks\\[0\\]|SM3_FALLBACK_TASK|trigger|Cyclic|Interrupt|interval" src/backend/shared src -g '!**/dist/**' -g '!**/build/**'
printf '\n---\n'
# Inspect the source of the task definitions if present
fd -a "task" src/backend/shared src | head -n 50Repository: Autonomy-Logic/openplc-editor
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- src/backend/shared/types/PLC/open-plc.ts ---'
sed -n '150,190p' src/backend/shared/types/PLC/open-plc.ts
printf '\n%s\n' '--- src/backend/shared/ethercat/generate-softmotion.ts ---'
sed -n '1,80p' src/backend/shared/ethercat/generate-softmotion.ts
printf '\n%s\n' '--- src/backend/shared/ethercat/__tests__/generate-softmotion.test.ts ---'
sed -n '1,320p' src/backend/shared/ethercat/__tests__/generate-softmotion.test.ts
printf '\n%s\n' '--- task parsing / generation helpers ---'
sed -n '1,180p' src/frontend/utils/parse-resource-string-to-configuration.ts
sed -n '1,120p' src/frontend/utils/parse-resource-configuration-to-string.tsRepository: Autonomy-Logic/openplc-editor
Length of output: 22315
Don’t bind the bridge to the first declared task. resource.tasks can include Interrupt or slow tasks, and the bridge is attached to tasks[0] with no scheduling check. That can run the PDO/axis bridge on an unsuitable cycle and hurt motion tracking. Pick the fastest Cyclic task, or create a dedicated motion task when none exists.
[medium_effort_and_high_reward]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/shared/ethercat/generate-softmotion.ts` around lines 207 - 216,
The bridge instance is currently bound to the first task in generate-softmotion,
which may be an Interrupt or otherwise unsuitable cycle. Update the task
selection logic near SM3_BRIDGE_INSTANCE_NAME to choose the fastest Cyclic task
from resource.tasks, and fall back to a dedicated motion task or
SM3_FALLBACK_TASK only when no appropriate cyclic task exists. Keep the bridge
attached to the chosen task when building bridgeInstance so scheduling is
deterministic and suitable for PDO/axis updates.
Comment on lines
+716
to
+721
| const Cia402AxisConfigSchema = z.object({ | ||
| enabled: z.boolean(), | ||
| scaleNum: z.number(), | ||
| scaleDenom: z.number(), | ||
| scaleFactor: z.number(), | ||
| }) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the schema and its surrounding types.
ast-grep outline src/backend/shared/types/PLC/open-plc.ts --view expanded
# Find all references to the axis config fields and the SoftMotion generator.
rg -n "scaleNum|scaleDenom|scaleFactor|Cia402AxisConfigSchema|generate-softmotion" src -S
# Read the relevant sections around the schema definition.
sed -n '680,760p' src/backend/shared/types/PLC/open-plc.ts
# Read the generator implementation if present.
fd -a "generate-softmotion.ts" srcRepository: Autonomy-Logic/openplc-editor
Length of output: 13309
Constrain CiA 402 scaling values
generate-softmotion.ts truncates these fields to integers, but the schema still accepts floats and a zero/negative denominator. Tighten scaleNum/scaleDenom to integer validation and require scaleDenom > 0 so invalid axis configs don’t reach generated SoftMotion code.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/shared/types/PLC/open-plc.ts` around lines 716 - 721, Tighten the
Cia402AxisConfigSchema validation so invalid axis scaling values are rejected
before SoftMotion generation. Update the schema in open-plc.ts to make scaleNum
and scaleDenom integer-only, and require scaleDenom to be greater than zero
while keeping scaleFactor as-is. This should align the schema with
generate-softmotion.ts, which already truncates these fields, and prevent
invalid axis configs from reaching the generated code.
…or of openplc-web) Byte-identical mirror of the openplc-web SoftMotion axis UI: - SoftMotion icon (teal rotary-motion tile) marking a recognized CiA 402 SoftMotion drive in the project tree. - Cia402AxisTab: CODESYS-style axis config — enable toggle, increments↔units scaling, the CiA 402 object→IEC-address mapping table, and a real-time feedback panel. The device name is the axis name used in MC_*(Axis := …). - project tree + explorer wire the `softMotionDrive` leaf lang when cia402.enabled; the EtherCAT device editor gains a "SoftMotion Axis" tab. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/components/_molecules/project-tree/index.tsx (1)
536-536: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
isEthercatDevicedoesn't match'softMotionDrive', breaking rename and delete.
isEthercatDeviceonly checksleafLang === 'ethercatDevice'. When a CiA 402 drive is enabled,project.tsx(line 448) setsleafLangto'softMotionDrive', soisEthercatDeviceisfalse. This causes:
- Rename (line 561 guard): fails with "Only POU, datatype, server, or remote device files can be renamed."
- Delete (line 693 guard): fails with "Only POU, datatype, server, or remote device files can be deleted."
The popover (line 811) still shows Rename and Delete options for
softMotionDrive, making the errors confusing. SoftMotion drives havebusNameanddeviceIdprops, so the handlers would work correctly after the fix.🐛 Proposed fix
- const isEthercatDevice = useMemo(() => leafLang === 'ethercatDevice', [leafLang]) + const isEthercatDevice = useMemo( + () => leafLang === 'ethercatDevice' || leafLang === 'softMotionDrive', + [leafLang], + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/components/_molecules/project-tree/index.tsx` at line 536, Update the isEthercatDevice useMemo in the project-tree component to return true for both 'ethercatDevice' and 'softMotionDrive'. This ensures the existing rename and delete guards and the popover actions correctly support CiA 402 drives while preserving EtherCAT device behavior.
🧹 Nitpick comments (1)
src/frontend/components/_features/[workspace]/editor/device/ethercat/components/cia402-axis-tab.tsx (1)
58-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract default
Cia402AxisConfigto a shared constant.The default config
{ enabled: false, scaleNum: 1, scaleDenom: 1, scaleFactor: 1 }is duplicated here and inethercat-device-editor.tsx(lines 197–202). If one copy changes, the UI would display different defaults than what gets persisted. Export aDEFAULT_CIA402_AXIS_CONFIGconstant fromesi-types.ts(whereCia402AxisConfigis defined) and reference it in both places.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/components/_features/`[workspace]/editor/device/ethercat/components/cia402-axis-tab.tsx around lines 58 - 63, Define and export a shared DEFAULT_CIA402_AXIS_CONFIG constant in esi-types.ts alongside Cia402AxisConfig, then replace the inline fallback objects in cia402-axis-tab.tsx and ethercat-device-editor.tsx with references to that constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/frontend/components/_features/`[workspace]/editor/device/ethercat/components/cia402-axis-tab.tsx:
- Around line 33-41: parseFloatInput treats an empty value as zero, allowing
scaleFactor to be set to an unusable value. Update parseFloatInput, or the
scaleFactor onChange handler, to reject empty input and zero by returning
undefined while preserving valid positive finite values; ensure the relevant
scaleFactor update in the component does not call onUpdate with 0.
---
Outside diff comments:
In `@src/frontend/components/_molecules/project-tree/index.tsx`:
- Line 536: Update the isEthercatDevice useMemo in the project-tree component to
return true for both 'ethercatDevice' and 'softMotionDrive'. This ensures the
existing rename and delete guards and the popover actions correctly support CiA
402 drives while preserving EtherCAT device behavior.
---
Nitpick comments:
In
`@src/frontend/components/_features/`[workspace]/editor/device/ethercat/components/cia402-axis-tab.tsx:
- Around line 58-63: Define and export a shared DEFAULT_CIA402_AXIS_CONFIG
constant in esi-types.ts alongside Cia402AxisConfig, then replace the inline
fallback objects in cia402-axis-tab.tsx and ethercat-device-editor.tsx with
references to that constant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2d8dd2de-67da-49a4-b0e1-427e2cfbee30
📒 Files selected for processing (5)
src/frontend/assets/icons/interface/SoftMotion.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/cia402-axis-tab.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/ethercat-device-editor.tsxsrc/frontend/components/_molecules/project-tree/index.tsxsrc/frontend/components/_organisms/explorer/project.tsx
Comment on lines
+33
to
+41
| function parseFloatInput(value: string): number | undefined { | ||
| const n = Number(value) | ||
| return Number.isFinite(n) ? n : undefined | ||
| } | ||
|
|
||
| function parseIntInput(value: string, min: number): number | undefined { | ||
| const n = parseInt(value, 10) | ||
| return Number.isNaN(n) || n < min ? undefined : n | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
parseFloatInput allows 0 for scaleFactor when input is cleared.
Number('') returns 0 (not NaN), so clearing the scale factor input calls onUpdate({ scaleFactor: 0 }). A zero scale factor makes the axis unusable. In contrast, parseIntInput returns undefined for empty strings (since parseInt('', 10) is NaN), so scaleNum/scaleDenom can't be zeroed out by clearing. Add a minimum check to parseFloatInput or guard against 0 in the onChange handler.
🛡️ Proposed fix
function parseFloatInput(value: string): number | undefined {
const n = Number(value)
- return Number.isFinite(n) ? n : undefined
+ return Number.isFinite(n) && n !== 0 ? n : undefined
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function parseFloatInput(value: string): number | undefined { | |
| const n = Number(value) | |
| return Number.isFinite(n) ? n : undefined | |
| } | |
| function parseIntInput(value: string, min: number): number | undefined { | |
| const n = parseInt(value, 10) | |
| return Number.isNaN(n) || n < min ? undefined : n | |
| } | |
| function parseFloatInput(value: string): number | undefined { | |
| const n = Number(value) | |
| return Number.isFinite(n) && n !== 0 ? n : undefined | |
| } | |
| function parseIntInput(value: string, min: number): number | undefined { | |
| const n = parseInt(value, 10) | |
| return Number.isNaN(n) || n < min ? undefined : n | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/frontend/components/_features/`[workspace]/editor/device/ethercat/components/cia402-axis-tab.tsx
around lines 33 - 41, parseFloatInput treats an empty value as zero, allowing
scaleFactor to be set to an unusable value. Update parseFloatInput, or the
scaleFactor onChange handler, to reject empty input and zero by returning
undefined while preserving valid positive finite values; ensure the relevant
scaleFactor update in the component does not call onUpdate with 0.
…channel mappings (mirror of openplc-web) Byte-identical mirror of the openplc-web SoftMotion UX fixes: 1. Rename works for SoftMotion drives (isEthercatDevice now covers the softMotionDrive leaf lang). 2. Drive names are enforced to valid IEC identifiers — sanitized at add-time, validated on rename (isValidIecIdentifier), since the name is the axis variable in generated code. 3. Channel Mappings tab hidden for SoftMotion drives (PDO mappings are internal); editor defaults to the SoftMotion Axis tab. 4. SoftMotion Axis tab simplified (dropped the enable toggle and the application-code reference blurb). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… openplc-web) Byte-identical mirror. The LSP now synthesizes a `VAR_GLOBAL <axis> : AXIS_REF_SM3` document and injects the same axis VAR_EXTERNAL the compiler generates (via the shared injectAxisExternals, now covering function blocks too), so editor code that names a SoftMotion axis — `MC_Power(Axis := X_Axis)` — resolves instead of flagging the axis as undeclared. Also fixes the compile gap where axis references inside function blocks didn't get their VAR_EXTERNAL. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…RNAL injection (mirror of openplc-web) Byte-identical mirror. The LSP now surfaces each axis as a bare top-level VAR_GLOBAL block (ambient global — resolves with no VAR_EXTERNAL), POUs are serialised verbatim (no injected declarations shifting line numbers, so go-to-definition stays correct), and go-to-definition on an axis redirects to the owning drive's device editor. VAR_EXTERNAL injection remains compile-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/frontend/services/st-lsp/project-sync.ts (1)
112-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow
serializeSoftMotionAxisGlobalsToSTto the fields it actually uses
{ remoteDevices } as neversuppresses type checking here. Since this helper only readsremoteDevices, acceptPick<PLCProjectData, 'remoteDevices'>(or a dedicated input type) instead of casting an incomplete object.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/services/st-lsp/project-sync.ts` at line 112, Update serializeSoftMotionAxisGlobalsToST to accept Pick<PLCProjectData, 'remoteDevices'> or an equivalent dedicated input type, then remove the as never cast at its call site in the project-sync flow while preserving the existing remoteDevices serialization behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/frontend/services/st-lsp/project-sync.ts`:
- Line 112: Update serializeSoftMotionAxisGlobalsToST to accept
Pick<PLCProjectData, 'remoteDevices'> or an equivalent dedicated input type,
then remove the as never cast at its call site in the project-sync flow while
preserving the existing remoteDevices serialization behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c238f60d-321f-40ea-9250-1015e7ad0a0b
📒 Files selected for processing (5)
src/backend/shared/ethercat/__tests__/generate-softmotion.test.tssrc/backend/shared/ethercat/generate-softmotion.tssrc/frontend/services/st-lsp/__tests__/goto-definition-redirect.test.tssrc/frontend/services/st-lsp/goto-definition-redirect.tssrc/frontend/services/st-lsp/project-sync.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/backend/shared/ethercat/generate-softmotion.ts
- src/backend/shared/ethercat/tests/generate-softmotion.test.ts
…olve (mirror of openplc-web) Byte-identical mirror. The LSP now sends the project's configuration-level globals as a CONFIGURATION VAR_GLOBAL doc (the level the compiler emits at, and the only form strucpp matches a VAR_EXTERNAL against), so a POU's VAR_EXTERNAL no longer falsely errors. Extracted a shared reconcileSyntheticDoc engine used by all three synthesized docs (data types, resource globals, softmotion axes), and go-to-definition on a user global opens the Resource editor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
An FB/program VAR_EXTERNAL is a reference to a single CONFIGURATION VAR_GLOBAL, but the debugger keyed each reference per-POU/instance. A global watched from a function block therefore showed `-` (no value) and appeared as duplicate rows (e.g. `test_global` + `main.MANUAL_OVERRIDE0.test_global`). Give every external reference one canonical, POU-independent identity (`Config0:<name>`, displayed `Config0.<name>`) across all four paths that treated externals per-POU: - debug tree: surface FB VAR_EXTERNAL members (findFunctionBlockExternalVariables) and resolve them (plus program externals) to the canonical global key; drop the now-redundant external special-case in buildDebugTree. - poller: poll external watches by the canonical key, instance-independent. - watch panel (allDebugVariables): canonical key/display + dedup by key. - store: sync the debug (watch) flag across the global definition and every VAR_EXTERNAL reference so the debug icon toggles everywhere at once. Externals now resolve to the shared global's value and dedup to a single `Config0.<name>` entry regardless of where they are referenced. Validated in-browser (simulator) + web vitest / editor jest (544 each), 100% coverage on gated files. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 tasks
dcoutinho1328
added a commit
that referenced
this pull request
Jul 16, 2026
…ed/utils PR #928 introduced imports of generate-softmotion.ts's pure axis-naming helpers (sanitizeAxisName, softMotionAxisNames, serializeSoftMotionAxisGlobalsToST, isValidIecIdentifier) and cia402.ts directly from three frontend/services/ and frontend/store/ files, plus two frontend/components/ call sites (missed by the checker's @root/ alias blind spot) — all violating the services/store/components -> backend-shared layer rule and failing validate:arch on development for every PR since. Rather than special-casing the violation with a KNOWN_EXCEPTIONS entry, relocated the pure logic to middleware/shared/utils/ethercat/ — the existing home for domain logic reachable from every layer including the compile pipeline (same relationship target-capabilities/ and library/ already have to backend/shared/compile/pipeline.ts). backend/shared/ethercat/ generate-softmotion.ts now only owns the actual codegen (generateSoftMotionArtifacts/injectAxisExternals), importing collectAxes from the new utils module. Also fixed a pre-existing prettier violation from the same PR (preprocess-pous.test.ts) that was failing the format check on development. (openplc-web's matching softmotion-e2e.test.ts fix does not apply here — that test is web-adapter-specific and doesn't exist in this repo.) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
dcoutinho1328
added a commit
that referenced
this pull request
Jul 16, 2026
compare-surfaces.py required byte-identical test files across both repos, same as program files. This is stricter than necessary and already broken in practice: openplc-web's backend/shared/ethercat/__tests__/softmotion-e2e.test.ts (added by #928) is genuinely web-adapter-specific (imports middleware/adapters/web/transpile-from-port + bundled-stlibs, neither of which exists in this repo) and has never had an editor counterpart — this has been failing the required Shared Surface Sync check on every PR since #928 merged, independent of anything in this branch. Test files are allowed to diverge (platform-specific mocks, adapter-only e2e suites) without indicating a real program-file drift — the same exclusion the architecture validator (__architecture__/validate.ts) already applies to its own scan. Now only program files are compared. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
dcoutinho1328
added a commit
that referenced
this pull request
Jul 16, 2026
…xis helpers (#937) * fix(architecture): document backend-shared exception for SoftMotion axis helpers PR #928 introduced imports of generate-softmotion.ts's pure axis-naming helpers (sanitizeAxisName, softMotionAxisNames, serializeSoftMotionAxisGlobalsToST, isValidIecIdentifier) from three services/store files, violating the services/store -> backend-shared layer rule and failing `validate:arch` on development for every PR since. Documented as KNOWN_EXCEPTIONS entries, mirroring the existing ladder-slice/fbd-slice precedent: the pure discovery logic isn't duplicated, only reached across a layer boundary those files can't move. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Revert "fix(architecture): document backend-shared exception for SoftMotion axis helpers" This reverts commit b1a60bf. * fix(architecture): relocate SoftMotion axis-naming to middleware/shared/utils PR #928 introduced imports of generate-softmotion.ts's pure axis-naming helpers (sanitizeAxisName, softMotionAxisNames, serializeSoftMotionAxisGlobalsToST, isValidIecIdentifier) and cia402.ts directly from three frontend/services/ and frontend/store/ files, plus two frontend/components/ call sites (missed by the checker's @root/ alias blind spot) — all violating the services/store/components -> backend-shared layer rule and failing validate:arch on development for every PR since. Rather than special-casing the violation with a KNOWN_EXCEPTIONS entry, relocated the pure logic to middleware/shared/utils/ethercat/ — the existing home for domain logic reachable from every layer including the compile pipeline (same relationship target-capabilities/ and library/ already have to backend/shared/compile/pipeline.ts). backend/shared/ethercat/ generate-softmotion.ts now only owns the actual codegen (generateSoftMotionArtifacts/injectAxisExternals), importing collectAxes from the new utils module. Also fixed a pre-existing prettier violation from the same PR (preprocess-pous.test.ts) that was failing the format check on development. (openplc-web's matching softmotion-e2e.test.ts fix does not apply here — that test is web-adapter-specific and doesn't exist in this repo.) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(sync): exclude test files from shared-surface comparison compare-surfaces.py required byte-identical test files across both repos, same as program files. This is stricter than necessary and already broken in practice: openplc-web's backend/shared/ethercat/__tests__/softmotion-e2e.test.ts (added by #928) is genuinely web-adapter-specific (imports middleware/adapters/web/transpile-from-port + bundled-stlibs, neither of which exists in this repo) and has never had an editor counterpart — this has been failing the required Shared Surface Sync check on every PR since #928 merged, independent of anything in this branch. Test files are allowed to diverge (platform-specific mocks, adapter-only e2e suites) without indicating a real program-file drift — the same exclusion the architecture validator (__architecture__/validate.ts) already applies to its own scan. Now only program files are compared. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Byte-identical mirror of the shared SoftMotion surface added in openplc-web#593 — release together. Adds CODESYS-SoftMotion-compatible CiA 402 EtherCAT axis support so CODESYS SoftMotion projects port to OpenPLC with minimal source changes.
Companion PRs (release together):
plcopen-softmotionlibrary + compiler enablers (strucpp v0.5.14)What's here (openplc-editor)
Mirror of the shared backend surface (
src/backend/shared/**,src/middleware/shared/ports/**), which is kept byte-identical between the two apps:cia402.ts— CiA 402 drive recognition + object→IEC-address resolution.generate-softmotion.ts— compile-timeAXIS_REF_SM3global + located PDO scalars + per-scanSM_Drive_GenericDS402bridge +VAR_EXTERNALinjection; wired intopreprocessPous.enrichDeviceDatatags recognized drives;open-plc/esi-typescarryCia402AxisConfig.v0.5.13 → v0.5.14.The editor picks up
plcopen-softmotion.stlibthrough its existing bundled-libs directory scan — nobundled-stlibs.ts(that file is web-specific).Validation
Shared tests (
cia402,generate-softmotion,preprocess-pous) pass under the editor's jest; new code 100% lines/statements/functions.Requires
strucpp release v0.5.14 (STruCpp#194). Follow-up (not in this PR): device-tree soft-motion icon + CODESYS-style axis config/feedback screen.
🤖 Generated with Claude Code
Summary by CodeRabbit