fix: Correct FC5/FC6 Modbus response to echo requested value#90
Conversation
Per Modbus specification, FC5 (Write Single Coil) and FC6 (Write Single Register) responses should echo the requested value. However, pymodbus implements these by calling setValues() then getValues(), using the read-back value in the response. OpenPLC uses a journal-based write system where writes are queued and applied at the next PLC scan cycle. This caused getValues() to return the old buffer value instead of the just-written value, making clients like libmodbus report errors due to request/response mismatch. This fix introduces OpenPLCDeviceContext that caches FC5/FC6 write values and returns them on the subsequent getValues() call. The cache uses asyncio task ID to isolate concurrent requests, preventing race conditions. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes Modbus FC5 (Write Single Coil) and FC6 (Write Single Register) responses to correctly echo the requested value per Modbus specification, resolving an issue where libmodbus reported errors due to response/request mismatches.
Changes:
- Introduces
OpenPLCDeviceContextclass that caches write values for FC5/FC6 operations - Uses asyncio task ID to isolate concurrent requests and prevent race conditions
- Replaces
ModbusDeviceContextwithOpenPLCDeviceContextin device initialization
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Initialize with an empty pending writes cache.""" | ||
| super().__init__(*args, **kwargs) | ||
| # Cache for FC5/FC6 echo values: (task_id, func_code, address) -> value | ||
| self._pending_writes = {} |
There was a problem hiding this comment.
The _pending_writes dictionary is not thread-safe and could lead to race conditions if multiple asyncio tasks access it concurrently. Consider using asyncio.Lock() to protect access to this shared state, or use a thread-safe data structure.
| results.append(self._pending_writes.pop(cache_key)) | ||
| else: | ||
| all_cached = False | ||
| break | ||
|
|
||
| if all_cached: | ||
| return results | ||
|
|
||
| # If not all values were cached, clean up any partial cache entries | ||
| # and fall through to normal read | ||
| for i in range(len(results), count): |
There was a problem hiding this comment.
Cache entries are removed immediately upon read, which could cause issues if getValues() is called multiple times for the same request. Consider whether the cache should persist until the request is complete or implement a more robust cache invalidation strategy.
| results.append(self._pending_writes.pop(cache_key)) | |
| else: | |
| all_cached = False | |
| break | |
| if all_cached: | |
| return results | |
| # If not all values were cached, clean up any partial cache entries | |
| # and fall through to normal read | |
| for i in range(len(results), count): | |
| # Read cached value without removing it yet; we'll clear the cache | |
| # for the full address range once we've confirmed all values exist. | |
| results.append(self._pending_writes[cache_key]) | |
| else: | |
| all_cached = False | |
| break | |
| if all_cached: | |
| # All values were cached: now invalidate the entire range for this task. | |
| for i in range(count): | |
| cache_key = (task_id, func_code, address + i) | |
| self._pending_writes.pop(cache_key, None) | |
| return results | |
| # If not all values were cached, clean up any partial cache entries | |
| # and fall through to normal read | |
| for i in range(count): |
| """Initialize with an empty pending writes cache.""" | ||
| super().__init__(*args, **kwargs) | ||
| # Cache for FC5/FC6 echo values: (task_id, func_code, address) -> value | ||
| self._pending_writes = {} |
There was a problem hiding this comment.
The _pending_writes cache grows unbounded and never clears stale entries if tasks fail or don't complete normally. Consider implementing cache expiration or cleanup logic to prevent memory leaks from orphaned task entries.
Summary
OpenPLCDeviceContextclass that caches write values for proper response echoProblem
When using libmodbus to write a coil via FC5, the coil was written successfully but libmodbus reported an error because the response didn't match the request:
0xFF00(ON)0x0000(the previous value)This happened because:
setValues()thengetValues()and using the read-back valuegetValues()read-back returned the old buffer value before the journal write was appliedSolution
Created
OpenPLCDeviceContextthat:setValues()is called with FC5 or FC6getValues()is called with FC5 or FC6This ensures FC5/FC6 responses correctly echo the requested value without introducing a double-buffer synchronization mechanism.
Test plan
🤖 Generated with Claude Code