Support local vars in C/C++ function blocks#707
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe PR changes which PLC variables are exposed to generated C/C++ blocks by replacing class-based filtering with ChangesC/C++ Variable Exposure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/cpp/generateSTCode.ts (1)
77-102:⚠️ Potential issue | 🔴 CriticalCopy local arrays back to
data__after the C/C++ call.
localarrays now go through the__flat_*staging path, but the copy-back still runs only foroutputvariables. Any mutation to a local array insidesetup()orloop()is discarded at the end of the cycle, so FB state is not actually preserved for that class of variable.💡 Minimal fix
const inputVariables = exposedVariables.filter((v) => v.class === 'input') const outputVariables = exposedVariables.filter((v) => v.class === 'output') const localVariables = exposedVariables.filter((v) => v.class === 'local') + const statefulVariables = [...outputVariables, ...localVariables] @@ - const outputCopyBack = generateOutputArrayCopyBack(outputVariables) + const outputCopyBack = generateOutputArrayCopyBack(statefulVariables)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/cpp/generateSTCode.ts` around lines 77 - 102, Local arrays that go through the __flat_* staging path are not being copied back into data__ after the C/C++ call because only outputVariables are passed to generateOutputArrayCopyBack; as a result mutations to local arrays are lost. Update the code that builds outputCopyBack (the variable named outputCopyBack) to include localVariables as well (e.g., pass [...outputVariables, ...localVariables] into generateOutputArrayCopyBack or call a helper that merges both sets) so that local array contents are copied back into data__ after setup() / loop().
🧹 Nitpick comments (1)
src/utils/cpp/generateCppBridge.test.ts (1)
53-88: Add a local-array regression case.These assertions only exercise scalar
localvariables. Array locals take the separate__flat_*path ingenerateSTCode, so a dedicated case is needed to catch state-persistence regressions there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/cpp/generateCppBridge.test.ts` around lines 53 - 88, The tests only cover scalar locals; add a regression test that defines a local array variable (similar to localVar but with an array dimension) and include it in the inputs to generateSTCode, generateCBlocksHeader, and generateCBlocksCode to exercise the __flat_* array handling path. In that new test assert that the ST output contains the array-flat mapping pattern used by generateSTCode (search for the generated __flat_ symbol that maps the array into vars), and assert the header/code outputs include the array pointer/define forms (the __flat_* symbol and array-specific declarations) and still do not include scalar-only names like HASBEENINITIALIZED or hasBeenInitialized; add this case alongside the existing scalar tests to catch regressions in array local persistence handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/utils/cpp/generateSTCode.ts`:
- Around line 77-102: Local arrays that go through the __flat_* staging path are
not being copied back into data__ after the C/C++ call because only
outputVariables are passed to generateOutputArrayCopyBack; as a result mutations
to local arrays are lost. Update the code that builds outputCopyBack (the
variable named outputCopyBack) to include localVariables as well (e.g., pass
[...outputVariables, ...localVariables] into generateOutputArrayCopyBack or call
a helper that merges both sets) so that local array contents are copied back
into data__ after setup() / loop().
---
Nitpick comments:
In `@src/utils/cpp/generateCppBridge.test.ts`:
- Around line 53-88: The tests only cover scalar locals; add a regression test
that defines a local array variable (similar to localVar but with an array
dimension) and include it in the inputs to generateSTCode,
generateCBlocksHeader, and generateCBlocksCode to exercise the __flat_* array
handling path. In that new test assert that the ST output contains the
array-flat mapping pattern used by generateSTCode (search for the generated
__flat_ symbol that maps the array into vars), and assert the header/code
outputs include the array pointer/define forms (the __flat_* symbol and
array-specific declarations) and still do not include scalar-only names like
HASBEENINITIALIZED or hasBeenInitialized; add this case alongside the existing
scalar tests to catch regressions in array local persistence handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ea9156d2-b7af-42aa-9423-0ca49a8306dd
📒 Files selected for processing (7)
src/renderer/components/_molecules/variables-table/selectable-cell.tsxsrc/renderer/components/_organisms/variables-editor/index.tsxsrc/utils/cpp/generateCBlocksCode.tssrc/utils/cpp/generateCBlocksHeader.tssrc/utils/cpp/generateCppBridge.test.tssrc/utils/cpp/generateSTCode.tssrc/utils/cpp/shared.ts
|
Quick follow-up on the CodeRabbit findings: both points are already covered in the current PR head
So the remaining visible CodeRabbit notes look historical relative to the current head, not pending code changes. |
|
I updated PR #707 by merging the latest development branch and resolving the merge conflicts. The branch is now mergeable again from a code/conflict standpoint. At this point, the remaining blockers are approval for the pending GitHub Actions workflows and 2 approving reviews from reviewers with write access. Could a maintainer please approve the pending workflows and review the PR? |
1 participant
Pull request info
References
If applicable substitute the issue reference to the one that this PR refers
This PR resolve the N/A.
Link to Jira task
N/A
Description of the changes proposed
localvariables in the generated C/C++ FB bridgehasBeenInitializedout of the exposed bridge__flat_*staging/copy-back path in the generated ST wrapperlocalas a selectable class forcppvariables in the table editorcppvariables tolocalDOD checklist
Summary by CodeRabbit
Bug Fixes
User Interface
Tests