search

favalex / modbus-cli

Command line tool to access Modbus devices
Mozilla Public License 2.0
175 stars 33 forks source link

Modbus response containing exception code leads to uncaught Python exception #15

Open svet-b opened 2 years ago

svet-b commented 2 years ago

Although I appreciate the parallel, I assume this isn't the desired behavior?

In a situation where a ModbusTCP server returns exception code 4 (maybe also others - I haven't tested), the code crashes with the following:

Traceback (most recent call last):
  File "/home/pi/.local/bin/./modbus", line 109, in <module>
    main()
  File "/home/pi/.local/bin/./modbus", line 102, in main
    connect_to_device(args).perform_accesses(parse_accesses(args.access, definitions, args.byte_order), definitions).close()
  File "/home/pi/.local/lib/python3.9/site-packages/modbus_cli/modbus_tcp.py", line 63, in perform_accesses
    access.read_registers_receive(self)
  File "/home/pi/.local/lib/python3.9/site-packages/modbus_cli/access.py", line 140, in read_registers_receive
    words = modbus.receive(self.request)
  File "/home/pi/.local/lib/python3.9/site-packages/modbus_cli/modbus_tcp.py", line 38, in receive
    return self.protocol.parse_response_adu(response, request)
  File "/home/pi/.local/lib/python3.9/site-packages/umodbus/client/tcp.py", line 235, in parse_response_adu
    function = create_function_from_response_pdu(resp_pdu, req_adu)
  File "/home/pi/.local/lib/python3.9/site-packages/umodbus/functions.py", line 132, in create_function_from_response_pdu
    function_code = pdu_to_function_code_or_raise_error(resp_pdu)
  File "/home/pi/.local/lib/python3.9/site-packages/umodbus/functions.py", line 118, in pdu_to_function_code_or_raise_error
    raise error_code_to_exception_map[error_code]
umodbus.exceptions.ServerDeviceFailureError: An unrecoverable error occurred.

It seems preferable to catch (maybe even parse) the umodbus exception and display a corresponding error message.

svet-b commented 2 years ago

Actually I now seem to be getting the above also in a situation where the response is not an error code (as read via a different Python library), so am no longer sure what's going on.

favalex commented 2 years ago

I agree it's not very user-friendly for a command line tool to exit in this way. The upside however is that we get more info about unexpected errors. In this case it looks like umodbus getting an unexpected error code. Corrupt packet? Non-standard modbus implementation on the other side? The underlying problem is probably better solved outside of the modbus tool. Do you think it would be useful to just print something like "Unexpected internal error" in cases like this? Personally I prefer seeing the stack trace, but I'm open to discuss other solutions. Maybe show the stack trace only in verbose mode?

svet-b commented 2 years ago

In this case it looks like umodbus getting an unexpected error code. Corrupt packet? Non-standard modbus implementation on the other side? The underlying problem is probably better solved outside of the modbus tool.

Maybe I wasn't clear about the nature of the exception in question here: I'm talking about a situation where the client receives a standard error code from the Modbus server (one of these: https://en.wikipedia.org/wiki/Modbus#Exception_responses). In the case above it's a "server/slave device failure" (Modbus code 04). The actual error (umodbus.exceptions.ServerDeviceFailureError) is printed on the last line, but is basically buried by the stack trace, so most users will look at this and conclude that the code just crashed. The umodbus package has a module that defines these exceptions (exceptions.py)

It's the Modbus equivalent of getting an HTTP 404 when you try to load a website. Anyway, I hope that clarifies things. It's of course up to you whether this is a case you're interested in addressing; I do also get the philosophy that you want to have a wrapper that's as transparent as possible. In that case my main recommendation would be to not print stack traces for known umodbus exceptions, since in those cases the stack traces don't add much.

favalex commented 2 years ago

Got it. Yes, I agree, no reason to print a stack trace in this case. Do you by any chance have time to write a PR?

svet-b commented 2 years ago

Yeah would love to contribute. Not much free time currently, but will try to throw something together over the next week or two. In principle should be fairly straightforward.