Add journal buffer system for race-condition-free plugin writes - #74
Conversation
Add systemd support
Fix user retention
Development improvements on logging an plugins
Improve package installation on several distros
This document describes a proposed journaling system for plugin writes to OpenPLC image tables. Key features: - All plugin writes go to a journal buffer instead of direct buffer access - Journal is applied atomically at cycle_start with "last writer wins" - Solves race conditions between asynchronous plugins (Python) and synchronous plugins (native with cycle hooks) - Eliminates the "zero vs uninitialized" ambiguity - No priority configuration needed - temporal ordering is deterministic The document includes: - Detailed architecture diagrams and data flows - API specifications for both C and Python - Thread safety model and mutex ordering - Integration points with existing runtime - Implementation phases with effort estimates - Comparison with alternative approaches Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…e 1) Implement core journal buffer infrastructure: - journal_buffer.h: Public API with write functions for all buffer types - journal_buffer.c: Implementation with mutex-protected journal entries Key features: - Static buffer with 1024 max entries (~20KB memory) - Sequence-numbered entries for last-writer-wins conflict resolution - Thread-safe write functions for bool, byte, int, dint, lint types - journal_apply_and_clear() for atomic application at cycle_start - Emergency flush when buffer is full (respects lock ordering) - Diagnostic functions for monitoring pending entries This addresses race conditions between plugins writing to shared image tables by routing all writes through a journal that is applied atomically at the start of each PLC scan cycle. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Initialize journal buffer in plc_cycle_thread after image tables setup - Call journal_apply_and_clear() at start of each PLC cycle (after acquiring buffer_mutex, before plugin cycle_start hooks) - Cleanup journal buffer in unload_plc_program before clearing image tables This ensures all plugin writes from the journal are atomically applied at the start of each scan cycle, before plugins and the PLC program run. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ecture docs: Add journal buffer architecture specification
Add journal buffer API for Python plugins: - Add journal write wrapper functions in plugin_driver.c - Set journal function pointers in plugin_runtime_args_t - Add ctypes bindings in PluginRuntimeArgs Python class - Modify GenericBufferAccessor to use journal writes - Keep read operations unchanged (direct buffer access) All Python plugin writes now go through the journal system for race-condition-free buffer access. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove unnecessary mutex from simple setValues methods since journal writes are internally thread-safe - Keep mutex for OpenPLCSegmentedHoldingRegistersDataBlock.setValues because it requires read-modify-write for partial DINT/LINT updates - Add documentation explaining journal-based thread safety - Update batch_processor.py with thread safety notes Affected setValues methods: - OpenPLCCoilsDataBlock: mutex removed (simple bool writes) - OpenPLCHoldingRegistersDataBlock: mutex removed (simple int writes) - OpenPLCSegmentedCoilsDataBlock: mutex removed (simple bool writes) - OpenPLCSegmentedHoldingRegistersDataBlock: mutex kept (RMW needed) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Implement S7Comm plugin for Siemens S7 communication using Snap7 library. Uses journal buffer system for race-condition-free writes to OpenPLC. Key features: - On-demand data sync via Snap7 RWArea callback (no cycle hooks needed) - S7 client reads: acquire OpenPLC mutex, copy fresh data, release mutex - S7 client writes: use journal_write functions (thread-safe, no mutex) - Support for PE/PA/MK system areas and configurable data blocks - JSON-based configuration for flexible I/O mapping - Big-endian to little-endian conversion for S7 protocol The S7 server thread runs independently without requiring cycle_start/ cycle_end hooks for data synchronization, similar to Python Modbus plugins. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Resolve merge conflicts in s7comm_plugin.cpp by keeping the journal-based implementation. The S7Comm plugin now uses on-demand data synchronization via the RWArea callback instead of cycle-based double-buffering. Key changes from development: - S7Comm plugin added to plugins.conf - Install script updates Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The callback receives S7 protocol area codes (S7AreaPE=0x81, S7AreaPA=0x82, S7AreaMK=0x83, S7AreaDB=0x84) but we were comparing against server area codes (srvAreaPE=0, srvAreaPA=1, srvAreaMK=2, srvAreaDB=5). This caused find_area_runtime() to always return NULL, resulting in the callback returning 0 without filling any data - hence all reads returned zeros. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Added detailed logging to trace: - Callback invocation with area code, operation, size - Area/DB lookup results - Buffer read operations with pointer values - Read result data This will help diagnose why reads are returning zeros. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The rm -rf /var/lib/apt/lists/* was a Docker-style optimization that's counterproductive on regular systems - it forces users to redownload the entire apt cache on every subsequent apt operation. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Docker-specific optimizations like apt cache cleanup belong in the Dockerfile, not the install script. This keeps install.sh generic for all platforms while still optimizing Docker image size. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
S7Comm plugin with journal-based writes is now working correctly. Removed temporary debug logging that was added for troubleshooting. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 23 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * pending entries and make room for new ones. | ||
| */ | ||
| #define JOURNAL_MAX_ENTRIES 1024 |
There was a problem hiding this comment.
The chosen value of 1024 entries may not be sufficient for all use cases. Consider adding a comment explaining the rationale behind this buffer size (e.g., expected maximum writes per cycle, memory constraints) and whether it should be configurable.
| * pending entries and make room for new ones. | |
| */ | |
| #define JOURNAL_MAX_ENTRIES 1024 | |
| * pending entries and make room for new ones. | |
| * | |
| * Rationale: | |
| * - Typical plugin workloads are expected to issue far fewer than 1024 writes | |
| * per PLC scan cycle, so this value provides a safety margin while keeping | |
| * memory usage bounded and predictable. | |
| * - With journal_entry_t sized for the largest supported value type, this | |
| * limit keeps the total journal buffer size acceptable for typical embedded | |
| * targets. | |
| * | |
| * Configuration: | |
| * - The default value is 1024, but it can be overridden at compile time by | |
| * defining JOURNAL_MAX_ENTRIES (e.g., via compiler flag | |
| * -DJOURNAL_MAX_ENTRIES=4096) before including this header. | |
| * - Increasing this value allows more plugin writes per cycle at the cost of | |
| * higher memory usage. | |
| */ | |
| #ifndef JOURNAL_MAX_ENTRIES | |
| #define JOURNAL_MAX_ENTRIES 1024 | |
| #endif |
| static journal_entry_t *add_entry_locked(void) | ||
| { | ||
| /* Check if buffer is full */ | ||
| if (g_count >= JOURNAL_MAX_ENTRIES) { | ||
| /* Emergency flush: apply all entries and clear */ | ||
| emergency_flush_locked(); | ||
| } | ||
|
|
||
| /* Add new entry */ | ||
| journal_entry_t *entry = &g_entries[g_count]; | ||
| entry->sequence = g_next_sequence++; | ||
| g_count++; | ||
|
|
||
| return entry; | ||
| } |
There was a problem hiding this comment.
After emergency_flush_locked() at line 124, g_count is reset to 0 inside emergency_flush_locked(). However, the function continues to use g_count without rechecking if the emergency flush succeeded. If emergency_flush_locked() fails to acquire the image_mutex or encounters an error, g_count might still be at JOURNAL_MAX_ENTRIES, causing an out-of-bounds array access at line 128. Add error handling or verification after emergency flush.
| return 0; | ||
| } | ||
| type = db->type; | ||
| start_buffer = db->start_buffer + (PTag->Start / get_type_size(type)); |
There was a problem hiding this comment.
Integer division PTag->Start / get_type_size(type) may not correctly handle byte-offset addressing. For example, if PTag->Start is a byte offset that doesn't align with the type size, this calculation will truncate and potentially access the wrong buffer index. Consider adding validation that PTag->Start is properly aligned or handling partial offsets.
| buffer_ptr[buffer_idx].contents.value = value | ||
| # Handle byte operations | ||
| elif buffer_type_obj.name == "byte": | ||
| result = self.args.journal_write_byte(journal_type, buffer_idx, int(value) & 0xFF) |
There was a problem hiding this comment.
The value masking with '& 0xFF' happens after int() conversion, but if value is negative, int() will preserve the sign and the mask won't produce the expected unsigned byte value. For example, int(-1) & 0xFF = 255, which may be intended, but int(value) where value is a large number could overflow. Consider validating the value range or using ctypes.c_uint8 for proper unsigned conversion.
| result = self.args.journal_write_byte(journal_type, buffer_idx, int(value) & 0xFF) | |
| result = self.args.journal_write_byte( | |
| journal_type, | |
| buffer_idx, | |
| ctypes.c_uint8(int(value)).value, | |
| ) |
| # Handle byte operations | ||
| elif buffer_type_obj.name == "byte": | ||
| result = self.args.journal_write_byte(journal_type, buffer_idx, int(value) & 0xFF) | ||
| if result != 0: | ||
| return False, f"Journal write failed with code {result}" | ||
| return True, "Success" | ||
|
|
||
| # Handle int operations (16-bit) | ||
| elif buffer_type_obj.name == "int": | ||
| result = self.args.journal_write_int(journal_type, buffer_idx, int(value) & 0xFFFF) | ||
| if result != 0: | ||
| return False, f"Journal write failed with code {result}" | ||
| return True, "Success" | ||
|
|
||
| # Handle dint operations (32-bit) | ||
| elif buffer_type_obj.name == "dint": | ||
| result = self.args.journal_write_dint(journal_type, buffer_idx, int(value) & 0xFFFFFFFF) | ||
| if result != 0: | ||
| return False, f"Journal write failed with code {result}" | ||
| return True, "Success" | ||
|
|
||
| # Handle lint operations (64-bit) | ||
| elif buffer_type_obj.name == "lint": | ||
| result = self.args.journal_write_lint(journal_type, buffer_idx, int(value)) |
There was a problem hiding this comment.
Similar to the byte write issue, these int conversions followed by masking may not handle negative values or out-of-range values correctly. Python's int() can handle arbitrarily large numbers, and masking a large negative number could produce unexpected results. Add explicit range validation before the journal write calls.
| # Handle byte operations | |
| elif buffer_type_obj.name == "byte": | |
| result = self.args.journal_write_byte(journal_type, buffer_idx, int(value) & 0xFF) | |
| if result != 0: | |
| return False, f"Journal write failed with code {result}" | |
| return True, "Success" | |
| # Handle int operations (16-bit) | |
| elif buffer_type_obj.name == "int": | |
| result = self.args.journal_write_int(journal_type, buffer_idx, int(value) & 0xFFFF) | |
| if result != 0: | |
| return False, f"Journal write failed with code {result}" | |
| return True, "Success" | |
| # Handle dint operations (32-bit) | |
| elif buffer_type_obj.name == "dint": | |
| result = self.args.journal_write_dint(journal_type, buffer_idx, int(value) & 0xFFFFFFFF) | |
| if result != 0: | |
| return False, f"Journal write failed with code {result}" | |
| return True, "Success" | |
| # Handle lint operations (64-bit) | |
| elif buffer_type_obj.name == "lint": | |
| result = self.args.journal_write_lint(journal_type, buffer_idx, int(value)) | |
| # Handle byte operations (0..255) | |
| elif buffer_type_obj.name == "byte": | |
| int_value = int(value) | |
| if int_value < 0 or int_value > 0xFF: | |
| raise ValueError(f"Value out of range for byte (0..255): {int_value}") | |
| result = self.args.journal_write_byte(journal_type, buffer_idx, int_value) | |
| if result != 0: | |
| return False, f"Journal write failed with code {result}" | |
| return True, "Success" | |
| # Handle int operations (16-bit signed, -32768..32767) | |
| elif buffer_type_obj.name == "int": | |
| int_value = int(value) | |
| if int_value < -0x8000 or int_value > 0x7FFF: | |
| raise ValueError(f"Value out of range for int (-32768..32767): {int_value}") | |
| result = self.args.journal_write_int(journal_type, buffer_idx, int_value) | |
| if result != 0: | |
| return False, f"Journal write failed with code {result}" | |
| return True, "Success" | |
| # Handle dint operations (32-bit signed, -2147483648..2147483647) | |
| elif buffer_type_obj.name == "dint": | |
| int_value = int(value) | |
| if int_value < -0x80000000 or int_value > 0x7FFFFFFF: | |
| raise ValueError( | |
| f"Value out of range for dint (-2147483648..2147483647): {int_value}" | |
| ) | |
| result = self.args.journal_write_dint(journal_type, buffer_idx, int_value) | |
| if result != 0: | |
| return False, f"Journal write failed with code {result}" | |
| return True, "Success" | |
| # Handle lint operations (64-bit signed, -9223372036854775808..9223372036854775807) | |
| elif buffer_type_obj.name == "lint": | |
| int_value = int(value) | |
| if int_value < -0x8000000000000000 or int_value > 0x7FFFFFFFFFFFFFFF: | |
| raise ValueError( | |
| f"Value out of range for lint (-9223372036854775808..9223372036854775807): {int_value}" | |
| ) | |
| result = self.args.journal_write_lint(journal_type, buffer_idx, int_value) |
| # Handle byte operations | ||
| elif buffer_type_obj.name == "byte": | ||
| result = self.args.journal_write_byte(journal_type, buffer_idx, int(value) & 0xFF) | ||
| if result != 0: | ||
| return False, f"Journal write failed with code {result}" | ||
| return True, "Success" | ||
|
|
||
| # Handle int operations (16-bit) | ||
| elif buffer_type_obj.name == "int": | ||
| result = self.args.journal_write_int(journal_type, buffer_idx, int(value) & 0xFFFF) | ||
| if result != 0: | ||
| return False, f"Journal write failed with code {result}" | ||
| return True, "Success" | ||
|
|
||
| # Handle dint operations (32-bit) | ||
| elif buffer_type_obj.name == "dint": | ||
| result = self.args.journal_write_dint(journal_type, buffer_idx, int(value) & 0xFFFFFFFF) | ||
| if result != 0: | ||
| return False, f"Journal write failed with code {result}" | ||
| return True, "Success" | ||
|
|
||
| # Handle lint operations (64-bit) | ||
| elif buffer_type_obj.name == "lint": | ||
| result = self.args.journal_write_lint(journal_type, buffer_idx, int(value)) |
There was a problem hiding this comment.
Similar to the byte write issue, these int conversions followed by masking may not handle negative values or out-of-range values correctly. Python's int() can handle arbitrarily large numbers, and masking a large negative number could produce unexpected results. Add explicit range validation before the journal write calls.
| # Handle byte operations | |
| elif buffer_type_obj.name == "byte": | |
| result = self.args.journal_write_byte(journal_type, buffer_idx, int(value) & 0xFF) | |
| if result != 0: | |
| return False, f"Journal write failed with code {result}" | |
| return True, "Success" | |
| # Handle int operations (16-bit) | |
| elif buffer_type_obj.name == "int": | |
| result = self.args.journal_write_int(journal_type, buffer_idx, int(value) & 0xFFFF) | |
| if result != 0: | |
| return False, f"Journal write failed with code {result}" | |
| return True, "Success" | |
| # Handle dint operations (32-bit) | |
| elif buffer_type_obj.name == "dint": | |
| result = self.args.journal_write_dint(journal_type, buffer_idx, int(value) & 0xFFFFFFFF) | |
| if result != 0: | |
| return False, f"Journal write failed with code {result}" | |
| return True, "Success" | |
| # Handle lint operations (64-bit) | |
| elif buffer_type_obj.name == "lint": | |
| result = self.args.journal_write_lint(journal_type, buffer_idx, int(value)) | |
| # Handle byte operations (8-bit unsigned) | |
| elif buffer_type_obj.name == "byte": | |
| int_value = int(value) | |
| if int_value < 0 or int_value > 0xFF: | |
| raise OverflowError("Value out of range for 8-bit unsigned byte") | |
| result = self.args.journal_write_byte(journal_type, buffer_idx, int_value) | |
| if result != 0: | |
| return False, f"Journal write failed with code {result}" | |
| return True, "Success" | |
| # Handle int operations (16-bit signed) | |
| elif buffer_type_obj.name == "int": | |
| int_value = int(value) | |
| if int_value < -0x8000 or int_value > 0x7FFF: | |
| raise OverflowError("Value out of range for 16-bit signed int") | |
| result = self.args.journal_write_int(journal_type, buffer_idx, int_value) | |
| if result != 0: | |
| return False, f"Journal write failed with code {result}" | |
| return True, "Success" | |
| # Handle dint operations (32-bit signed) | |
| elif buffer_type_obj.name == "dint": | |
| int_value = int(value) | |
| if int_value < -0x80000000 or int_value > 0x7FFFFFFF: | |
| raise OverflowError("Value out of range for 32-bit signed dint") | |
| result = self.args.journal_write_dint(journal_type, buffer_idx, int_value) | |
| if result != 0: | |
| return False, f"Journal write failed with code {result}" | |
| return True, "Success" | |
| # Handle lint operations (64-bit signed) | |
| elif buffer_type_obj.name == "lint": | |
| int_value = int(value) | |
| if int_value < -0x8000000000000000 or int_value > 0x7FFFFFFFFFFFFFFF: | |
| raise OverflowError("Value out of range for 64-bit signed lint") | |
| result = self.args.journal_write_lint(journal_type, buffer_idx, int_value) |
Summary
This PR implements a journal buffer system that provides race-condition-free writes to OpenPLC image tables from multiple plugins. The journal buffer is applied atomically at the start of each PLC scan cycle with "last writer wins" semantics.
Key Changes
Phase 1: Core journal buffer C implementation (
journal_buffer.c/.h)Phase 2: Runtime integration
Phase 3: Plugin API extension
plugin_runtime_args_tSafeBufferAccessautomaticallyPhase 4: S7Comm native plugin migration
Phase 5: Python plugin optimization
Architecture
Test plan
🤖 Generated with Claude Code