Propagate known request identity on ModbusIOException - #2993
Merged
janiversen merged 4 commits intoJul 28, 2026
Merged
Conversation
After pymodbus-dev#2992, attach function_code, transaction_id, and dev_id wherever the outstanding request (or PDU) already knows them: client execute timeouts/mismatches, register decode failures, and invalid device id. Suggested in review of pymodbus-dev#2992. Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com>
Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com>
Critical review of pymodbus-dev#2992/pymodbus-dev#2993 found register_message identity was dead because DecodePDU swallowed ModbusIOException. Re-raise payload decode errors, enrich framing tid/dev_id in the framer, use known fcode on the server exception response when present, and forward the UDP peer addr. Also lean the helper docstring, drop a private-method unit test, and assert identity through handleFrame + sync execute paths. Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com>
janiversen
requested changes
Jul 28, 2026
janiversen
left a comment
janiversen
left a comment
Collaborator
There was a problem hiding this comment.
Mainly a problem with comments, please remove.
And a couple of changes.
Ps. I have not reviewed the tests.
| try: | ||
| pdu = self.decoder.decode(frame_data) | ||
| except ModbusIOException as exc: | ||
| # Framing ids are authoritative; payload decode never has them yet. |
Collaborator
There was a problem hiding this comment.
Please remove comments, we have few comments in the code, as the code is self documenting (and comments tend not to be maintained).
| exc.dev_id = dev_id | ||
| raise | ||
| if pdu is None: | ||
| # Unknown/garbage FC: do not invent function_code from noise (#2990). |
| str(pdu), | ||
| ) | ||
| return pdu | ||
| except ModbusIOException: |
Collaborator
There was a problem hiding this comment.
This is wrong, it is already caught as ModbusException.
| ) | ||
| return pdu | ||
| except ModbusIOException: | ||
| # Payload decode already attached known identity (e.g. function_code). |
| """Decode a register response packet.""" | ||
| self.registers = [] | ||
| if (data_len := int(data[0])) >= len(data): | ||
| # function_code is the class/request FC; framing tid/dev_id are filled |
| # is cleared before framing runs, so identity comes from the framer | ||
| # exception attributes when available. Use function code 0x00 rather | ||
| # than a hardcoded unrelated value (was 40 / 0x28) — see #2990. | ||
| # Undecodable / corrupt PDUs land here. last_pdu is cleared before |
Collaborator
|
Tests are ok. |
Remove narrative comments per maintainer style. Drop the DecodePDU ModbusIOException re-raise (already covered by ModbusException). Keep framing identity on the None path and client request identity helper. Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com>
Contributor
Author
|
Addressed in a5280f1 — removed the narrative comments and dropped the DecodePDU |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #2992 / review note from @janiversen: attach known identity on
ModbusIOExceptionwhere transaction id and/or function code are already known.Client execute paths (
TransactionManager.sync_execute/execute):_io_exception_from_requestattachesfunction_code,transaction_id,dev_idon timeouts, cancel, and device/TID mismatches.Wire-path payload decode (review fix — previous register kwargs were dead):
DecodePDUre-raisesModbusIOExceptionfrom payload decode (e.g. bad register byte_count) instead of swallowing it.FramerBaseenriches framingtransaction_id/dev_idon that exception; unknown/garbage FC still raises without inventing a function code (Server returns hardcoded function code (0x28) and transaction_id=0 for any undecodable function code #2990).ReadHoldingRegistersResponse.decodeattaches knownfunction_codeonly.Server exception path residuals from #2992 review:
exc.fcodewhen payload decode supplied it; otherwise0x00for noise/unknown FC.addronserver_sendso UDP peers receive the exception (was hardcoded0whilelast_addris cleared).Also:
ModbusPDUinvalid device id carries constructortransaction_id/dev_id.Validation
AI/LLM disclosure