fix: show online debug values for structured data types and FB instances#614
Conversation
…ces (#591) Fix three issues preventing debug values from displaying for struct and FB instance variables in textual (ST/IL) programs: 1. Extend variable reclassification to all POUs on project load, not just graphical (LD/FBD). The text parser lacks project context to correctly classify FB instances as 'derived', so re-parsing with full context is needed for ST/IL programs too. 2. Expand top-level user-data-type variables in debug polling so structs and any unresolved FB instances get their leaf fields registered for value polling. 3. Use fallback path resolution for struct fields in the debug tree builder. The xml2st compiler converts structs to FBs, so debug.c may use FB-style paths (no .value.) instead of struct-style paths (.value.). Try both. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds top-level user-data-type (UDT) variable processing in the workspace screen, performs full-project variable reclassification before graphical-node sync in the store, and implements dual-path debug-field resolution (FB-style vs struct-style) in debug-tree traversal to improve variable debug mapping and online visibility. (<=50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Workspace as WorkspaceScreen
participant Store as SharedStore
participant Parser as IECParser
participant Debug as DebugTraversal
participant UI as GraphicalPOUNodes
Workspace->>Store: Trigger full-context reclassification
Store->>Parser: Re-parse POUs with pous, dataTypes, libraries
Parser-->>Store: Reclassified variables, dataTypes, libraries
Store->>UI: Sync graphical POU nodes using reclassified variables
Workspace->>Debug: Process top-level UDTs (derive inner vars, ensure ENO)
Debug->>Debug: Resolve field paths (FB-style preferred, else struct-style)
Debug-->>Workspace: Mapped debug-variable entries for UI monitoring
UI-->>Workspace: Visualize updated variable tree and online values
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/utils/debug-tree-traversal.ts (1)
227-258: Consider extracting the duplicated FB-vs-struct path resolution into a helper.Lines 228–235 (for
user-data-typefields) and 251–258 (forarrayfields) contain identical path-resolution logic: build both paths, scandebugVariablesfor an FB-style prefix match, and pick the winner. A small helper (similar tofindDebugVariableForFieldfor base-type fields) would reduce duplication and make future path-style changes a single-point edit.♻️ Example helper
+function resolveFieldPath( + debugVariables: DebugVariableEntry[], + parentPath: string, + fieldName: string, +): string { + const fbStylePath = `${parentPath}.${fieldName.toUpperCase()}` + const structStylePath = `${parentPath}.value.${fieldName.toUpperCase()}` + const hasFbMatch = debugVariables.some( + (dv) => + dv.name.toUpperCase().startsWith(fbStylePath.toUpperCase() + '.') || + dv.name.toUpperCase() === fbStylePath.toUpperCase(), + ) + return hasFbMatch ? fbStylePath : structStylePath +}Then both sites become:
- const fbStylePath = `${fullPath}.${field.name.toUpperCase()}` - const structStylePath = `${fullPath}.value.${field.name.toUpperCase()}` - const hasFbMatch = debugVariables.some(...) - const fieldFullPath = hasFbMatch ? fbStylePath : structStylePath + const fieldFullPath = resolveFieldPath(debugVariables, fullPath, field.name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/debug-tree-traversal.ts` around lines 227 - 258, Extract the duplicated FB-vs-struct path resolution (building fbStylePath and structStylePath, scanning debugVariables for a startsWith or exact match, and choosing the winner) into a single helper (e.g., resolveFbOrStructPath or findDebugVariablePath) and call it from both the user-data-type branch that invokes traverseNestedNode and the array branch; follow the existing pattern used by findDebugVariableForField for base-type fields so the helper accepts (fullPath, field.name, debugVariables) and returns the selected fieldFullPath to replace the duplicated blocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/store/slices/shared/index.ts`:
- Around line 1223-1250: Wrap the per-POU reclassification block in a try/catch
so a single failing POU won't abort handleOpenProjectRequest: inside the
pous.forEach callback around calls to generateIecVariablesToString and
parseIecStringToVariables, catch any error, log it (including pou.data.name and
error.message) and continue to the next POU; on success call
getState().projectActions.setPouVariables with the reparsedVariables as before,
using reclassDataTypes and reclassLibraries from the outer scope.
---
Nitpick comments:
In `@src/utils/debug-tree-traversal.ts`:
- Around line 227-258: Extract the duplicated FB-vs-struct path resolution
(building fbStylePath and structStylePath, scanning debugVariables for a
startsWith or exact match, and choosing the winner) into a single helper (e.g.,
resolveFbOrStructPath or findDebugVariablePath) and call it from both the
user-data-type branch that invokes traverseNestedNode and the array branch;
follow the existing pattern used by findDebugVariableForField for base-type
fields so the helper accepts (fullPath, field.name, debugVariables) and returns
the selected fieldFullPath to replace the duplicated blocks.
…tree The `canForceVariable` function used nullish coalescing (`??`) instead of logical OR (`||`) for its fallback checks. Since `Map.has()` returns a boolean (not null/undefined), `false ?? nextCheck` returns `false` without evaluating `nextCheck`. This prevented the compositeKey lookup from ever running when the fullPath lookup failed. For struct field nodes in the debug tree, `debugIndex` may not be set by the tree builder, so the function needs the compositeKey fallback to find the variable in `debugVariableIndexes` (populated by the polling setup). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Wrap the reclassification loop in a try/catch so that a single malformed POU doesn't abort the entire project load. If reclassification fails for one POU, it keeps its original type classifications and the rest of the project loads normally. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Fixes #591 — Online values not visible for structured data types and FB instances in textual (ST/IL) programs.
user-data-typeinstead ofderived.user-data-typevariables in debug polling so structs and any unresolved FB instances get their leaf fields registered for online value polling..value.) instead of struct-style paths (.value.). The tree builder now tries both.Test plan
Structure.Parameter1,.Parameter2,.Parameter3show online valuesfbAddition.Par1,.Par2,.Par3,.RetVal,.LocalVarshow online valuesnpm run test— all 76 tests pass🤖 Generated with Claude Code
Summary by CodeRabbit