Fix exception error message for decoding response #2618
Conversation
Szewcson
force-pushed
the
exception_error_msg_fix
branch
from
ce98e31 to
9cbab36
Compare
Signed-off-by: Adam Szewczyk <a.szewczyk@cthings.co>
Szewcson
force-pushed
the
exception_error_msg_fix
branch
from
9cbab36 to
3c3afcc
Compare
|
Also maybe exception code and function code should be added to str? |
pymodbus/pdu/pdu.py
Outdated
| @@ -118,6 +118,20 @@ class ExceptionResponse(ModbusPDU): | |||
| GATEWAY_PATH_UNAVIABLE = 0x0A | |||
| GATEWAY_NO_RESPONSE = 0x0B | |||
|
|
|||
| exception_code_dict = { | |||
| 0x00: "None", | |||
There was a problem hiding this comment.
Please use the constants defined above, to avoid mismatch between constant and value.
There was a problem hiding this comment.
This is a constant meaning the name should be capitalized
pymodbus/pdu/decoders.py
Outdated
| @@ -102,7 +102,7 @@ def decode(self, frame: bytes) -> base.ModbusPDU | None: | |||
| """Decode a frame.""" | |||
| try: | |||
| if (function_code := int(frame[0])) > 0x80: | |||
| pdu_exp = base.ExceptionResponse(function_code & 0x7F) | |||
| pdu_exp = base.ExceptionResponse(function_code & 0x7F, int(frame[1])) | |||
There was a problem hiding this comment.
please do not convert to int, if a conversion is needed it should be done where needed.
pymodbus/pdu/pdu.py
Outdated
| @@ -137,3 +152,4 @@ def encode(self) -> bytes: | |||
| def decode(self, data: bytes) -> None: | |||
| """Decode a modbus exception response.""" | |||
| self.exception_code = int(data[0]) | |||
| self.exception_name = self.exception_code_dict.get(self.exception_code, f"Unknown Exception value: {self.exception_code}") | |||
There was a problem hiding this comment.
Not sure what this does, it really seems not needed.
pymodbus/pdu/pdu.py
Outdated
| @@ -128,7 +142,8 @@ def __init__( | |||
| super().__init__(transaction_id=transaction, dev_id=device_id) | |||
| self.function_code = function_code | 0x80 | |||
| self.exception_code = exception_code | |||
| Log.error(f"Exception response {self.function_code} / {self.exception_code}") | |||
| self.exception_name = self.exception_code_dict.get(self.exception_code, f"Unknown Exception value: {self.exception_code}") | |||
There was a problem hiding this comment.
no need to make an object variable, remove self.
pymodbus/pdu/pdu.py
Outdated
| @@ -128,7 +142,8 @@ def __init__( | |||
| super().__init__(transaction_id=transaction, dev_id=device_id) | |||
| self.function_code = function_code | 0x80 | |||
| self.exception_code = exception_code | |||
| Log.error(f"Exception response {self.function_code} / {self.exception_code}") | |||
| self.exception_name = self.exception_code_dict.get(self.exception_code, f"Unknown Exception value: {self.exception_code}") | |||
| Log.error(f"Exception response FC{self.function_code & 0x7F} / {self.exception_name}") | |||
There was a problem hiding this comment.
no need to add 0x7f just use the function_code parameter.
Just showing the name, makes it more difficult for log parsers so the code should still be presented.
Showing the exception name makes sense for people who do not know the modbus standard, but the same people to not know the function codes either....so it would be better to either use names for both or not at all.
There was a problem hiding this comment.
in general instead of logging, it would be better as __str__ allowing the app to do what it want.
There was a problem hiding this comment.
Ok, so which approach here will be the best?
There was a problem hiding this comment.
no, pymodbus normally do not log responses....apart from that in the eyes of pymodbus this is not an error.
But I am flexible with that.....
Szewcson
force-pushed
the
exception_error_msg_fix
branch
from
f7baa94 to
9f65a77
Compare
Signed-off-by: szewcu <szewcson@gmail.com>
Szewcson
force-pushed
the
exception_error_msg_fix
branch
from
9f65a77 to
d24dddf
Compare
|
But this log was there probably for opposite situation - generation of the exception. Maybe we need some flag then. |
|
the generation of the exception is used in the server. |
Signed-off-by: szewcu <szewcson@gmail.com>
Signed-off-by: szewcu <szewcson@gmail.com>
When decoding exception frame Error message is showing always exception code as 0. Also in error message I think showing Exception code in human readable format will be more helpful, as well as showing failing FC instead FC with exception bit set.