Generate glue vars #13
Conversation
WalkthroughAdds a GlueGenerator module using Jinja2 to render glueVars.c from LOCATED_VARIABLES.h entries, a Jinja2 template for the generated C glue, and a CLI integration in xml2st.py to generate and write glueVars.c from a LOCATED_VARIABLES.h file. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant CLI as xml2st.py (CLI)
participant FS as Filesystem
participant GG as GlueGenerator
participant T as Jinja2 Template
U->>CLI: run --generate-gluevars LOCATED_VARIABLES.h
CLI->>FS: read LOCATED_VARIABLES.h
FS-->>CLI: located_vars_lines
CLI->>GG: generate_glue_variables(lines)
GG->>GG: parse lines, compute glue_code (warn/skip non-matches)
GG->>T: render templates/glueVars.c.j2 with vars
T-->>GG: rendered glueVars.c
GG-->>CLI: return glue source
CLI->>FS: write glueVars.c (same dir)
FS-->>CLI: write OK
CLI-->>U: success / error message
sequenceDiagram
autonumber
participant App as Host App
participant GV as glueVars.c (generated)
App->>GV: setBufferPointers(...) %% bind external buffers
App->>GV: glueVars() %% execute generated wiring
App->>GV: updateTime() %% advance __CURRENT_TIME
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
xml2st.py (4)
9-9: Import path may break when installed as a package; consider relative importIf xml2st.py and GlueGenerator.py live in the same package, absolute import without a package prefix can break when installed or executed from a different CWD. Prefer a relative import.
Apply this diff if xml2st.py is inside a package:
-from GlueGenerator import GlueGenerator +from .GlueGenerator import GlueGenerator
101-128: Return the output path and write with explicit encoding and Unix newlinesThe helper currently prints but doesn’t return the generated file path. Returning it improves composability/tests. Also, read/write with explicit UTF-8 and normalize line endings for deterministic builds.
Apply this diff:
-def generate_gluevars(located_vars_file): +def generate_gluevars(located_vars_file) -> str: @@ - with open(located_vars_file, "r") as f: + with open(located_vars_file, "r", encoding="utf-8") as f: located_vars = f.readlines() @@ - with open(glue_vars_file, "w") as f: - f.write(glueVars) + with open(glue_vars_file, "w", encoding="utf-8", newline="\n") as f: + f.write(glueVars) @@ - print(f"Glue variables saved to {glue_vars_file}") + print(f"Glue variables saved to {glue_vars_file}") + return glue_vars_file
122-124: Consider guarding against partial writesIf the process is interrupted while writing, glueVars.c may be left truncated. Writing to a temp file and atomic rename reduces risk.
I can provide a temp-write + atomic replace snippet if you want to harden this path.
191-199: Propagate the generated path and fail fast on NoneIf
generate_gluevars()returns None due to earlier validation, the CLI still prints “Generating glue variables...” then exits. Capture and validate the return for clearer UX.Apply this diff after adopting the return value:
- print("Generating glue variables...") - generate_gluevars(args.generate_gluevars) + print("Generating glue variables...") + out = generate_gluevars(args.generate_gluevars) + if not out: + raise RuntimeError("No glue file produced") + print(f"Done: {out}")GlueGenerator.py (5)
13-15: Template loader path: OK; consider trimming blocks for cleaner outputThe loader root is correct. You can reduce extraneous whitespace in generated C by enabling
trim_blocksandlstrip_blockson the Environment.Apply this diff (paired with the env creation below):
- self.__loader = FileSystemLoader(os.path.join(paths.AbsDir(__file__), "templates")) + self.__loader = FileSystemLoader(os.path.join(paths.AbsDir(__file__), "templates"))And later:
- env = Environment(loader=self.__loader) + env = Environment(loader=self.__loader, trim_blocks=True, lstrip_blocks=True)
21-29: Preserve original traceback using exception chainingRe-raising without chaining drops the original stack, making debugging harder. Chain the exception to keep context. Also tighten the parsing to guard against unexpected prefixes.
Apply this diff:
- try: - parts = varName.split("_") - pos1 = int(parts[2][2:]) # number after QX0 or QW0 - pos2 = int(parts[3]) if len(parts) > 3 else 0 - except Exception as e: - raise Exception(f"Error parsing variable name '{varName}': {e}") + try: + parts = varName.split("_") + # Expect names like __QX0_0 or __QW10 + if len(parts) < 3 or not varName.startswith("__") or len(parts[2]) < 3: + raise ValueError(f"Unexpected located var shape: '{varName}'") + pos1 = int(parts[2][2:]) # number after QX0 or QW0 + pos2 = int(parts[3]) if len(parts) > 3 else 0 + except Exception as e: + raise Exception(f"Error parsing variable name '{varName}': {e}") from e
30-65: Collapse nested if/elif into a data-driven mapA lookup table reduces branching and future maintenance errors, and aligns with the Ruff hint (SIM116).
Apply this diff:
- kind = varName[2] # I, Q, M - sub = varName[3] # X, B, W, D, L - - if kind == 'I': - if sub == 'X': - return f"bool_input_ptr[{pos1}][{pos2}] = (IEC_BOOL *){varName};" - elif sub == 'B': - return f"byte_input_ptr[{pos1}] = (IEC_BYTE *){varName};" - elif sub == 'W': - return f"int_input_ptr[{pos1}] = (IEC_UINT *){varName};" - elif sub == 'D': - return f"dint_input_ptr[{pos1}] = (IEC_UDINT *){varName};" - elif sub == 'L': - return f"lint_input_ptr[{pos1}] = (IEC_ULINT *){varName};" - - elif kind == 'Q': - if sub == 'X': - return f"bool_output_ptr[{pos1}][{pos2}] = (IEC_BOOL *){varName};" - elif sub == 'B': - return f"byte_output_ptr[{pos1}] = (IEC_BYTE *){varName};" - elif sub == 'W': - return f"int_output_ptr[{pos1}] = (IEC_UINT *){varName};" - elif sub == 'D': - return f"dint_output_ptr[{pos1}] = (IEC_UDINT *){varName};" - elif sub == 'L': - return f"lint_output_ptr[{pos1}] = (IEC_ULINT *){varName};" - - elif kind == 'M': - if sub == 'W': - return f"int_memory_ptr[{pos1}] = (IEC_UINT *){varName};" - elif sub == 'D': - return f"dint_memory_ptr[{pos1}] = (IEC_UDINT *){varName};" - elif sub == 'L': - return f"lint_memory_ptr[{pos1}] = (IEC_ULINT *){varName};" - - raise Exception(f"Unhandled variable type: {varName}") + kind = varName[2] # I, Q, M + sub = varName[3] # X, B, W, D, L + + table = { + 'I': { + 'X': "bool_input_ptr[{p1}][{p2}] = (IEC_BOOL *){vn};", + 'B': "byte_input_ptr[{p1}] = (IEC_BYTE *){vn};", + 'W': "int_input_ptr[{p1}] = (IEC_UINT *){vn};", + 'D': "dint_input_ptr[{p1}] = (IEC_UDINT *){vn};", + 'L': "lint_input_ptr[{p1}] = (IEC_ULINT *){vn};", + }, + 'Q': { + 'X': "bool_output_ptr[{p1}][{p2}] = (IEC_BOOL *){vn};", + 'B': "byte_output_ptr[{p1}] = (IEC_BYTE *){vn};", + 'W': "int_output_ptr[{p1}] = (IEC_UINT *){vn};", + 'D': "dint_output_ptr[{p1}] = (IEC_UDINT *){vn};", + 'L': "lint_output_ptr[{p1}] = (IEC_ULINT *){vn};", + }, + 'M': { + 'W': "int_memory_ptr[{p1}] = (IEC_UINT *){vn};", + 'D': "dint_memory_ptr[{p1}] = (IEC_UDINT *){vn};", + 'L': "lint_memory_ptr[{p1}] = (IEC_ULINT *){vn};", + }, + } + try: + fmt = table[kind][sub] + return fmt.format(p1=pos1, p2=pos2, vn=varName) + except KeyError: + raise Exception(f"Unhandled variable type: {varName}")
72-77: Pre-compile the regex for speed and clarity; allow leading whitespaceMinor perf/readability win; avoids recompiling per line and tolerates indentation in headers.
Apply this diff:
- m = re.match(r"__LOCATED_VAR\(([^,]+),([^,]+),.*\)", line.strip()) + # Precompiled at module scope: + # LOCATED_RE = re.compile(r"\s*__LOCATED_VAR\(([^,]+),([^,]+),.*\)") + m = LOCATED_RE.match(line)And at the top-level:
+LOCATED_RE = re.compile(r"\s*__LOCATED_VAR\(([^,]+),([^,]+),.*\)")
79-92: Environment construction: enable block trimming and consider stable orderingGood use of Jinja2. If deterministic output is desired, sort
parsedby name to avoid diff churn across runs with different header orders.Apply this diff:
- parsed = [] - for line in located_vars_lines: - entry = self.__parse_line(line) - if entry is not None: - parsed.append(entry) + parsed = [] + for line in located_vars_lines: + entry = self.__parse_line(line) + if entry is not None: + parsed.append(entry) + # Keep output stable + parsed.sort(key=lambda e: e["name"]) @@ - env = Environment(loader=self.__loader) + env = Environment(loader=self.__loader, trim_blocks=True, lstrip_blocks=True)templates/glueVars.c.j2 (3)
15-16: Make BUFFER_SIZE configurable or documentedA hardcoded 1024 may not match target hardware layouts. Consider parameterizing via a Jinja variable with a sensible default or document the assumption in the header.
Example:
-#define BUFFER_SIZE 1024 +#ifndef BUFFER_SIZE +#define BUFFER_SIZE 1024 +#endif
21-44: Pointer declarations are correct but non-idiomatic for non-boolean channelsThe
IEC_BYTE *(*byte_input_ptr)style is equivalent toIEC_BYTE **byte_input_ptrbut less readable. Consider using the double-pointer form for non-boolean channels for consistency.Apply this diff:
-static IEC_BYTE *(*byte_input_ptr) = NULL; -static IEC_BYTE *(*byte_output_ptr) = NULL; +static IEC_BYTE **byte_input_ptr = NULL; +static IEC_BYTE **byte_output_ptr = NULL; @@ -static IEC_UINT *(*int_input_ptr) = NULL; -static IEC_UINT *(*int_output_ptr) = NULL; +static IEC_UINT **int_input_ptr = NULL; +static IEC_UINT **int_output_ptr = NULL; @@ -static IEC_UDINT *(*dint_input_ptr) = NULL; -static IEC_UDINT *(*dint_output_ptr) = NULL; +static IEC_UDINT **dint_input_ptr = NULL; +static IEC_UDINT **dint_output_ptr = NULL; @@ -static IEC_ULINT *(*lint_input_ptr) = NULL; -static IEC_ULINT *(*lint_output_ptr) = NULL; +static IEC_ULINT **lint_input_ptr = NULL; +static IEC_ULINT **lint_output_ptr = NULL; @@ -static IEC_UINT *(*int_memory_ptr) = NULL; -static IEC_UDINT *(*dint_memory_ptr) = NULL; -static IEC_ULINT *(*lint_memory_ptr) = NULL; +static IEC_UINT **int_memory_ptr = NULL; +static IEC_UDINT **dint_memory_ptr = NULL; +static IEC_ULINT **lint_memory_ptr = NULL;This keeps assignments in
setBufferPointersvalid due to parameter decay rules.
52-73: API surface looks coherent; consider basic null checks in debug buildsIf any buffer pointer is NULL, subsequent glue code may dereference them. Optional runtime guards (even under a DEBUG macro) can help catch integration issues early.
Example:
void setBufferPointers(IEC_BOOL *input_bool[BUFFER_SIZE][8], IEC_BOOL *output_bool[BUFFER_SIZE][8], @@ { +#ifdef DEBUG + if (!input_bool || !output_bool || !input_byte || !output_byte || + !input_int || !output_int || !input_dint || !output_dint || + !input_lint || !output_lint || !int_memory || !dint_memory || !lint_memory) { + // Intentional crash in debug to surface wiring mistakes early + __builtin_trap(); + } +#endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
GlueGenerator.py(1 hunks)templates/glueVars.c.j2(1 hunks)xml2st.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
xml2st.py (1)
GlueGenerator.py (2)
GlueGenerator(11-91)generate_glue_variables(79-91)
GlueGenerator.py (1)
util/paths.py (1)
AbsDir(36-37)
🪛 Ruff (0.12.2)
GlueGenerator.py
28-28: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
34-43: Use a dictionary instead of consecutive if statements
(SIM116)
46-55: Use a dictionary instead of consecutive if statements
(SIM116)
58-63: Use a dictionary instead of consecutive if statements
(SIM116)
🔇 Additional comments (5)
xml2st.py (2)
133-138: Argparse metavar usage is correct; nice UX improvementUsing a single-string
metavarimproves--helpoutput without changing behavior. All good here.
146-151: Good: isolates glue generation as its own CLI entryClear help text and argument name. Consider adding a brief example in README in a follow-up.
templates/glueVars.c.j2 (3)
45-51: Double-include pattern looks good; verify include pathRelying on a quoted relative include assumes glueVars.c and LOCATED_VARIABLES.h sit side by side, which
xml2st.generate_gluevars()satisfies. LGTM.
75-80: Emitting glue code: ensure trailing semicolons are present in all variantsCurrent generator returns statements with
;. If future glue snippets ever omit;, the template may generate invalid C. Safe as-is; just a note for future edits.Would you like me to add a simple CI check that compiles the rendered glue with a stub to catch such regressions?
82-92: Time update logic is correct; add comments on time units
common_ticktime__appears to be in nanoseconds; the normalization is correct. Adding a brief comment helps future maintainers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
templates/glueVars.c.j2 (3)
24-61: Risk of missing memory buffers on tiny AVR targets; add safe 1-length arrays to keep glue uniform.On the AVR subset (Line 27), MAX_MEMORY_* are 0 and no memory arrays are declared. If any located %M* variables appear, generated glue will reference missing symbols and fail to link. Add zero-safe dims so arrays always exist.
Proposed patch:
#ifdef ARDUINO //OpenPLC Buffers + // Avoid zero-length arrays on tiny AVRs when MAX_MEMORY_* == 0 + #define DIM_OR_ONE(n) ((n) > 0 ? (n) : 1) #if defined(__AVR_ATmega328P__) || defined(__AVR_ATmega168__) || defined(__AVR_ATmega32U4__) || defined(__AVR_ATmega16U4__) @@ - #define MAX_MEMORY_WORD 0 - #define MAX_MEMORY_DWORD 0 - #define MAX_MEMORY_LWORD 0 + #define MAX_MEMORY_WORD 0 + #define MAX_MEMORY_DWORD 0 + #define MAX_MEMORY_LWORD 0 @@ - IEC_UINT *int_output[MAX_ANALOG_OUTPUT]; + IEC_UINT *int_output[MAX_ANALOG_OUTPUT]; + // Provide memory arrays even if size is 0 to keep glue code uniform + IEC_UINT *int_memory[DIM_OR_ONE(MAX_MEMORY_WORD)]; + IEC_UDINT *dint_memory[DIM_OR_ONE(MAX_MEMORY_DWORD)]; + IEC_ULINT *lint_memory[DIM_OR_ONE(MAX_MEMORY_LWORD)];
62-93: Unify symbol names across platforms to simplify glue generation.Arduino path exposes bool_input/int_input/... while non-Arduino exposes *_ptr. Alias the non-Arduino pointers to Arduino-style names so a single glue emission works for both.
Proposed patch:
#else #define BUFFER_SIZE 1024 @@ static IEC_ULINT *(*lint_memory_ptr) = NULL; + // Alias non-Arduino pointer variables to Arduino-style names + #define bool_input bool_input_ptr + #define bool_output bool_output_ptr + #define byte_input byte_input_ptr + #define byte_output byte_output_ptr + #define int_input int_input_ptr + #define int_output int_output_ptr + #define dint_input dint_input_ptr + #define dint_output dint_output_ptr + #define lint_input lint_input_ptr + #define lint_output lint_output_ptr + #define int_memory int_memory_ptr + #define dint_memory dint_memory_ptr + #define lint_memory lint_memory_ptr
94-116: Document call order and consider basic pointer guards in setBufferPointers.glueVars() assumes these are set; a brief comment and light sanity checks prevent hard-to-debug NULL derefs.
Proposed patch:
- void setBufferPointers(IEC_BOOL *input_bool[BUFFER_SIZE][8], IEC_BOOL *output_bool[BUFFER_SIZE][8], + // Must be called once at startup before glueVars() is ever invoked. + void setBufferPointers(IEC_BOOL *input_bool[BUFFER_SIZE][8], IEC_BOOL *output_bool[BUFFER_SIZE][8], IEC_BYTE *input_byte[BUFFER_SIZE], IEC_BYTE *output_byte[BUFFER_SIZE], @@ - { + { + // minimal guards in debug builds + #ifdef DEBUG + if (!input_bool || !output_bool || !input_int || !output_int) { return; } + #endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
templates/glueVars.c.j2(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-windows-arm
🔇 Additional comments (3)
templates/glueVars.c.j2 (3)
11-18: Two-pass __LOCATED_VAR include pattern looks correct.Macro varargs and the two-pass trick to define storage then pointers is sound.
125-135: Time accumulator logic is correct.Carry handling and 64-bit math look good.
118-124: Confirm generated glueVars.c references only supported symbolsThe initial grep check failed because
glueVars.cwasn’t present—this template must first be rendered by the GlueGenerator. Please generate the C output and then verify that no byte-input/output or memory array symbols appear unguarded:• Render the Jinja2 template (e.g. via your build or a standalone script) to produce
glueVars.c.
• Run:rg -nP '(\bbyte_input(_ptr)?\b|\bbyte_output(_ptr)?\b|\bint_memory(_ptr)?\b|\bdint_memory(_ptr)?\b|\blint_memory(_ptr)?\b)' path/to/generated/glueVars.c -n• Confirm the output is empty (i.e., no unsupported symbols). If any matches remain, ensure they’re wrapped by the alias macros (non-Arduino) or use the DIM_OR_ONE fix (Arduino AVR) so that both AVR tiny and standard AVR targets compile cleanly.
• If you encounter any unresolved references afterward, update the generator or template accordingly.
2 participants
Summary by CodeRabbit