Add memory location support (%MX, %MW, %MD, %ML) to Modbus slave plugin#63
Conversation
- Add bool_memory buffer to C runtime (image_tables.c/h) for %MX support - Update plugin_driver.c/h to pass bool_memory to plugins - Update Python PluginRuntimeArgs struct to match C struct alignment - Add bool_memory support to buffer_types.py and buffer_accessor.py - Add read_bool_memory/write_bool_memory methods to SafeBufferAccess - Implement OpenPLCSegmentedCoilsDataBlock for %QX and %MX address segmentation - Implement OpenPLCSegmentedHoldingRegistersDataBlock for %QW, %MW, %MD, %ML - Add word-order handling (big-endian by default) for 32-bit and 64-bit values - Add parse_buffer_mapping_config() for backward-compatible JSON config parsing - Update init() to read buffer_mapping from JSON and use segmented data blocks - Add -DOPENPLC_V4 flag to compile.sh for conditional compilation - Add setBufferPointers_v4 symbol lookup with bool_memory support JSON config format now supports: - Legacy format: max_coils, max_discrete_inputs, max_holding_registers, max_input_registers - New segmented format: holding_registers.qw_count/mw_count/md_count/ml_count, coils.qx_bits/mx_bits, discrete_inputs.ix_bits, input_registers.iw_count Address mapping (matching v3): - Holding registers: 0-1023 (%QW), 1024-2047 (%MW), 2048-4095 (%MD), 4096-8191 (%ML) - Coils: 0 to qx_bits-1 (%QX), qx_bits to qx_bits+mx_bits-1 (%MX) The runtime now tries setBufferPointers_v4 first (with bool_memory), falling back to setBufferPointers for older programs compiled without -DOPENPLC_V4. Co-Authored-By: Thiago Alves <thiagoralves@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
There was a problem hiding this comment.
Pull request overview
This PR adds support for IEC 61131-3 memory locations (%MX, %MW, %MD, %ML) to the Modbus slave plugin, providing feature parity with OpenPLC v3. The changes introduce a new bool_memory buffer for %MX support across the runtime and enhance the Modbus plugin with segmented data blocks for all memory location types.
Key Changes:
- Added runtime infrastructure for
bool_memory[BUFFER_SIZE][8]buffer with v4 symbol lookup and backward compatibility - Implemented segmented Modbus data blocks supporting %QX/%MX coils and %QW/%MW/%MD/%ML holding registers with configurable word ordering
- Updated Python plugin infrastructure to expose bool_memory through SafeBufferAccess API
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/compile.sh | Added -DOPENPLC_V4 flag for conditional compilation of memory location support |
| core/src/plc_app/image_tables.h | Declared bool_memory buffer and ext_setBufferPointers_v4 function pointer |
| core/src/plc_app/image_tables.c | Implemented bool_memory buffer initialization and v4/v1 fallback logic |
| core/src/drivers/plugins/python/shared/safe_buffer_access_refactored.py | Added read_bool_memory/write_bool_memory methods |
| core/src/drivers/plugins/python/shared/plugin_runtime_args.py | Added bool_memory field to PluginRuntimeArgs ctypes structure |
| core/src/drivers/plugins/python/shared/buffer_types.py | Added bool_memory mapping to buffer types registry |
| core/src/drivers/plugins/python/shared/buffer_accessor.py | Added bool_memory field mapping in buffer pointer resolution |
| core/src/drivers/plugins/python/modbus_slave/simple_modbus.py | Implemented segmented coils and holding registers data blocks with parse_buffer_mapping_config |
| core/src/drivers/plugin_driver.h | Added bool_memory field to plugin_runtime_args struct |
| core/src/drivers/plugin_driver.c | Added bool_memory buffer initialization in structured args generation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Try to load v4 version with bool_memory support (optional - only present in v4 programs) | ||
| *(void **)(&ext_setBufferPointers_v4) = | ||
| plugin_manager_get_func(pm, void (*)(unsigned long), "setBufferPointers_v4"); |
There was a problem hiding this comment.
The function signature cast void (*)(unsigned long) is incorrect for setBufferPointers_v4. This function takes 14 pointer parameters, not an unsigned long. This type mismatch could lead to undefined behavior. The cast should match the actual function signature declared in image_tables.h or use a generic function pointer type.
| plugin_manager_get_func(pm, void (*)(unsigned long), "setBufferPointers_v4"); | |
| plugin_manager_get_func(pm, void (*)(void), "setBufferPointers_v4"); |
| if error_msg == "Success": | ||
| values.append(1 if value else 0) | ||
| else: | ||
| print(f"[MODBUS] Error reading coil %MX{coil_addr - self.qx_bits}: {error_msg}") |
There was a problem hiding this comment.
The error message incorrectly references %MX{coil_addr - self.qx_bits} when it should reference the actual MX address, which is mx_offset (or coil_addr - self.qx_bits). The variable coil_addr in the message calculation should consistently use the offset value to match the address space being accessed. Consider using mx_offset directly for clarity.
| buffer_idx, bit_idx, bool(value), thread_safe=False | ||
| ) | ||
| if error_msg != "Success": | ||
| print(f"[MODBUS] Error setting coil %MX{coil_addr - self.qx_bits}: {error_msg}") |
There was a problem hiding this comment.
Same issue as Comment 3 - the error message calculation coil_addr - self.qx_bits should use the mx_offset variable that was already calculated in _get_segment_info for consistency and clarity.
| "qw_count": min( | ||
| hr_config.get("qw_count", DEFAULT_HOLDING_REG_CONFIG["qw_count"]), BUFFER_SIZE | ||
| ), | ||
| "mw_count": min( | ||
| hr_config.get("mw_count", DEFAULT_HOLDING_REG_CONFIG["mw_count"]), BUFFER_SIZE | ||
| ), | ||
| "md_count": min( | ||
| hr_config.get("md_count", DEFAULT_HOLDING_REG_CONFIG["md_count"]), BUFFER_SIZE | ||
| ), | ||
| "ml_count": min( | ||
| hr_config.get("ml_count", DEFAULT_HOLDING_REG_CONFIG["ml_count"]), BUFFER_SIZE | ||
| ), |
There was a problem hiding this comment.
The repetitive pattern of min(hr_config.get(...), BUFFER_SIZE) could be extracted into a helper function to reduce duplication and improve maintainability. This same pattern appears multiple times in the function for different configuration sections.
…x function signature cast Co-Authored-By: Thiago Alves <thiagoralves@gmail.com>
fee1732 to
7deda6a
Compare
Add memory location support (%MX, %MW, %MD, %ML) to Modbus slave plugin
Summary
This PR adds support for IEC 61131-3 memory locations (%MX, %MW, %MD, %ML) to the Modbus slave plugin, matching the functionality available in OpenPLC v3. The changes include:
Runtime infrastructure:
bool_memory[BUFFER_SIZE][8]buffer to image_tables.c/h for %MX supportsetBufferPointers_v4symbol lookup with bool_memory parameter (falls back to v1 for older programs)-DOPENPLC_V4flag to compile.sh for conditional compilation in glueVars.cModbus plugin enhancements:
OpenPLCSegmentedCoilsDataBlockclass supporting %QX (0 to qx_bits-1) and %MX (qx_bits to qx_bits+mx_bits-1)OpenPLCSegmentedHoldingRegistersDataBlockclass with address segmentation matching v3:parse_buffer_mapping_config()for backward-compatible JSON config parsingPython struct updates:
Updates since last revision
Copilot review fixes:
setBufferPointers_v4lookupReview & Testing Checklist for Human
Verify C/Python struct alignment: The
bool_memoryfield inplugin_runtime_args.pymust be in the exact same position as inplugin_driver.h. Field order must match: lint_memory -> bool_memory -> mutex_take. Misalignment causes memory corruption.Rebuild runtime before testing: The C runtime must be rebuilt after pulling these changes. The plugin and runtime are distributed together and must match.
Test Modbus holding register segmentation: Connect a Modbus client and verify that reading/writing to addresses 0-1023 affects %QW, 1024-2047 affects %MW, 2048-4095 affects %MD (32-bit), and 4096-8191 affects %ML (64-bit).
Test word order for 32-bit/64-bit values: Write a known 32-bit value (e.g., 0x12345678) to %MD via Modbus and verify the high word (0x1234) is at the lower address and low word (0x5678) is at the higher address.
Recommended test plan:
Notes
This PR provides the runtime infrastructure for %MX support. A separate PR for xml2st is required to update the glueVars.c.j2 template to actually glue %MX variables using
#ifdef OPENPLC_V4. Without that change, %MX addresses will work in Modbus but won't be connected to PLC located variables.Link to Devin run: https://app.devin.ai/sessions/a39db7c299fb417481ba9800b03995c3
Requested by: Thiago Alves (thiago.alves@autonomylogic.com) / @thiagoralves