Repository navigation
Report bad file record frames as undecodable - #3069
Conversation
The file record PDUs (read and write file record, read fifo queue) raise ModbusException when the byte or fifo count in a frame points past its end. DecodePDU.decode only turned ValueError, IndexError and struct.error into ModbusIOException, so these escaped the framer without transaction and device id, and the server could not answer them with an exception response like any other undecodable request. Convert ModbusException there as well, and keep ModbusIOException as is.
janiversen
left a comment
janiversen
left a comment
There was a problem hiding this comment.
But ModbusException is a legal and documented exception, that is also raised f.x. by mixin.py
We do not want to convert ModbusException to ModbusIOException, as they are used to indicate 2 different types of problems.
|
if ModbusException is raises in a place (like decode) it should be changed to ModbusIOException....to signal it is a communication problem and e.g. a problem with the use of the API. The ModbusException in file_message.py is actually being changed with #3068 (not sure if this change have been uploaded yet). |
ModbusException signals a problem with the use of the API, so decode() is back to converting only ValueError, IndexError and struct.error. The three checks that reject a frame while decoding (the read and write file record byte counts and the read fifo queue count) now raise ModbusIOException themselves, with the function code. The constructor checks in file_message.py still raise ModbusException.
|
makes sense, thanks. reworked it that way in 5133651: decode() is back to unchanged, and the three decode-time checks in file_message.py (both byte counts and the fifo count) now raise ModbusIOException with the function code, while the FileRecord constructor checks keep ModbusException. the tests are the same and still fail on dev. if #3068 lands first i'll rebase onto it. |
DecodePDU.decodeturnsValueError,IndexErrorandstruct.errorintoModbusIOException. The framer tags that with the transaction and device id, and the request handler answers it with an exception response (#3057). The file record PDUs raised a plainModbusExceptioninstead when the byte count (read and write file record) or the fifo count (read fifo queue response) points past the end of the frame (pymodbus/pdu/file_message.py). That exception leaves the framer without the ids, so the handler'sModbusIOExceptionpath never sees it.These three decode checks now raise
ModbusIOException, with the function code.ModbusExceptionstays for problems with the use of the API, such as the checks in theFileRecordconstructor, anddecodeitself is unchanged.Tests:
test_decode_bad_file_record_framecovers a bad read file record and write file record request on the server decoder and a bad read fifo queue response on the client decoder, andtest_bad_file_record_request_for_framer_socketchecks that the transaction and device id reach the exception. Ondevall four fail withModbusException: Modbus Error: Invalid byte count: 16orInvalid fifo count: 5; here they pass.test/pduandtest/framer: 965 passed.ruff check,ruff format --checkandpylint(10.00/10) are clean on the changed file.AI disclosure: written with help from AI coding tools (Claude Code); I reviewed the change and checked the behaviour above before submitting.