-
Notifications
You must be signed in to change notification settings - Fork 63
fix: Correct FC5/FC6 Modbus response to echo requested value #90
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -59,6 +59,93 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class OpenPLCDeviceContext(ModbusDeviceContext): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Custom Modbus device context that correctly handles FC5/FC6 response echo. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Per Modbus specification, FC5 (Write Single Coil) and FC6 (Write Single Register) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| responses should echo the requested value, not the current buffer state. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pymodbus implements FC5/FC6 by calling setValues() then getValues() and using | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| the read-back value in the response. However, OpenPLC uses a journal-based write | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| system where writes are queued and applied at the next PLC scan cycle. This means | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| getValues() may return the old value before the journal write is applied. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| This class caches values written via FC5/FC6 and returns them on the subsequent | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| getValues() call, ensuring the response correctly echoes the requested value. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| The cache uses asyncio task ID to isolate concurrent requests, preventing race | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| conditions when multiple FC5/FC6 requests arrive simultaneously. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Function codes that require echo response (write single operations) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _ECHO_FUNCTION_CODES = (5, 6) # FC5: Write Single Coil, FC6: Write Single Register | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def __init__(self, *args, **kwargs): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """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 = {} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def setValues(self, func_code, address, values): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Set values in the datastore and cache for FC5/FC6 echo response. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| For FC5/FC6, the written values are cached with the current asyncio task ID | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| so that the subsequent getValues() call can return the correct echo value. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Cache values for FC5/FC6 echo response | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if func_code in self._ECHO_FUNCTION_CODES: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| task_id = id(asyncio.current_task()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for i, val in enumerate(values): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cache_key = (task_id, func_code, address + i) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self._pending_writes[cache_key] = val | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except RuntimeError: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # No running event loop - skip caching (shouldn't happen in normal operation) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pass | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return super().setValues(func_code, address, values) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def getValues(self, func_code, address, count=1): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Get values from the datastore, using cached values for FC5/FC6 echo response. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| For FC5/FC6, if cached values exist for this task, they are returned and | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| removed from the cache. This ensures the response echoes the requested value | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| per Modbus specification. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # For FC5/FC6, return cached echo values if available | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if func_code in self._ECHO_FUNCTION_CODES: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| task_id = id(asyncio.current_task()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| results = [] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| all_cached = True | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for i in range(count): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cache_key = (task_id, func_code, address + i) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if cache_key in self._pending_writes: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+128
to
+138
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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): |
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.
The
_pending_writesdictionary is not thread-safe and could lead to race conditions if multiple asyncio tasks access it concurrently. Consider usingasyncio.Lock()to protect access to this shared state, or use a thread-safe data structure.