-
Notifications
You must be signed in to change notification settings - Fork 58
feat: FB_INIT
user code initialization
#1458
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1458 +/- ##
==========================================
+ Coverage 93.67% 94.13% +0.46%
==========================================
Files 166 173 +7
Lines 50097 56801 +6704
==========================================
+ Hits 46928 53470 +6542
- Misses 3169 3331 +162 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a6ea592
to
7096598
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the initialization logic for FUNCTION_BLOCKs by introducing user-defined and project-wide initialization functions, updating tests and code generation, and expanding the documentation. Key changes include:
- Updates to test expectations and assertions for initialization function bodies.
- New logic in the lowering phase to generate and integrate user-defined initialization calls.
- Documentation updates in multiple markdown files to explain FUNCTION_BLOCK initialization and interoperability considerations.
Reviewed Changes
Copilot reviewed 27 out of 34 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/resolver/tests/resolve_and_lower_init_functions.rs | Updated test assertions to expect an additional call statement for user-defined initialization. |
src/lowering/initializers.rs | Introduced create_user_init_units and integrated user-defined init calls with implicit initializer assignments. |
src/lowering.rs | Added candidate collection for user initialization methods and integrated global user initialization calls. |
src/codegen/tests/* | Added definitions of _user_init* functions across various test cases for inheritance and debugging. |
compiler/plc_project/src/project.rs | Updated get_init_symbol_name to support dashes in project names. |
Documentation (book/src/using_rusty.md, pous.md, api_lib_guide.md) | Expanded documentation for project-wide and FUNCTION_BLOCK initialization, including interoperability with C implementations. |
Files not reviewed (7)
- compiler/plc_driver/src/tests/snapshots/plc_driver__tests__multi_files__multiple_files_in_different_locations_with_debug_info.snap: Language not supported
- compiler/plc_driver/src/tests/snapshots/plc_driver__tests__multi_files__multiple_files_with_debug_info.snap: Language not supported
- compiler/plc_driver/src/tests/snapshots/plc_driver__tests__multi_files__multiple_source_files_generated.snap: Language not supported
- src/codegen/tests/debug_tests/snapshots/rusty__codegen__tests__debug_tests__expression_debugging__external_impl_is_not_added_as_external_subroutine.snap: Language not supported
- src/codegen/tests/snapshots/rusty__codegen__tests__code_gen_tests__external_program_global_var_is_external.snap: Language not supported
- src/codegen/tests/snapshots/rusty__codegen__tests__debug_tests__global_var_nested_struct_added_to_debug_info.snap: Language not supported
- src/codegen/tests/snapshots/rusty__codegen__tests__debug_tests__global_var_struct_added_to_debug_info.snap: Language not supported
Comments suppressed due to low confidence (1)
src/lowering/initializers.rs:283
- [nitpick] Consider aligning the naming of this initialization call with the user-defined initialization pattern by using get_user_init_fn_name (e.g. yielding "_user_init") or add a clarifying comment to distinguish this call from those generated for user-defined init functions.
let op = create_member_reference("fb_init", id_provider.clone(), Some(base));
{ | ||
// add the struct to potential `STRUCT_INIT` candidates | ||
if let Some(name) = user_type.data_type.get_name() { | ||
self.user_inits.insert(name.to_string(), false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ghaith If we want to support user-defined struct-init functions for FFI purposes, their addition should be trivial if we use a naming-scheme here. We can then just query the index if such a function is present and then add it to the hashmap to be generated/called.
@@ -33,6 +35,10 @@ impl InitVisitor { | |||
init_symbol_name: &'static str, | |||
) -> Vec<CompilationUnit> { | |||
let mut visitor = Self::new(index, unresolvables, id_provider); | |||
// before visiting, we need to collect all candidates for user-defined init functions | |||
units.iter().for_each(|unit| { | |||
visitor.collect_user_init_candidates(unit); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done outside of/before the regular visitor since the visitor might not pick up candidates in different compilation unit (the order of visiting would matter).
@@ -0,0 +1,7 @@ | |||
FUNCTION main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a similar test in codegen?
I would like to see the case where we have a function using a function block, and that functionblock has a user_init be tested.
I expect the function, while initializing the function block call to (implicitly) call fb_init. I'm fine with adding this as a followup if we know the case works, but it would be good to express this in codegen tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a test for this in #1471
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expecting a test in the follow up PR, but otherwise looks good.
This pull request introduces enhancements to the initialization logic for
FUNCTION_BLOCK
s and project-wide initialization. It also includes updates to unit tests, code generation and documentation to support these changes. Below is a summary of the most important changes grouped by theme:Enhancements to
FUNCTION_BLOCK
InitializationFB_INIT
methods forFUNCTION_BLOCK
s, including examples for both IEC61131-3 and external C implementations. This includes naming conventions, initialization patterns, and interoperability considerations. (book/src/libraries/api_lib_guide.md
- [1]book/src/pous.md
- [2]book/src/using_rusty.md
- [3])Updates to Code Generation
__user_init_<FunctionBlockName>
) forFUNCTION_BLOCK
instances, ensuring proper initialization of nested structures and base classes in object-oriented scenarios.__init___<projectname>
) to invoke the corresponding user-defined initialization functions.Test and Snapshot Updates
__user_init_<FunctionBlockName>
function definitions and their integration into the project-wide initializer.Code Refinements
get_init_symbol_name
method to handle project names with both dots and dashes, ensuring consistent symbol naming. (compiler/plc_project/src/project.rs
- compiler/plc_project/src/project.rsL305-R305)