exception raising on incomplete message

[kjetilmjos]

Had a problem with incomplete messages received and return causing strange values to appear. Added an exception raise to not return register if messages are unvalid.

[dhoomakethu]

@kjetilmjos Sorry for looking in to this late and thanks for the PR. The issue you faced with strange values was it applicable for Modbus TCP or Serial communication ? Do you have any logs for the said issue ?

[kjetilmjos]

No worries. This was with Modbus TCP using a USR-TCP converter. The log message I got was:

Traceback (most recent call last):
File "/usr/local/lib/python3.7/site-packages/pymodbus/transaction.py", line 168, in execute
request.unit_id)
File "/usr/local/lib/python3.7/site-packages/pymodbus/framer/socket_framer.py", line 165, in processIncomingPacket
self._process(callback, error=True)
File "/usr/local/lib/python3.7/site-packages/pymodbus/framer/socket_framer.py", line 175, in _process
raise ModbusIOException("Unable to decode request")
pymodbus.exceptions.ModbusIOException: Modbus Error: [Input/Output] Unable to decode request
2019-03-24 14:08:48,321 - ERROR - pymodbus.factory - unpack requires a buffer of 7 bytes
2019-03-24 14:08:48,322 - ERROR - pymodbus.transaction - Modbus Error: [Input/Output] Unable to decode request

Found out the code was only printing a debug log message so I was unable to catch the error if an exception was not raised.

[kjetilmjos]

@dhoomakethu have you had any chance to look more into this?

[dhoomakethu]

Unfortunately no, this somehow slipped from 2.3.0rc1 release. Are you still having this issue ? Can you please give it a try on the latest 2.3.0rc1 and see if it still an issue .thanks

[kjetilmjos]

@dhoomakethu I don't have access to the equipment any more which frequently gave these errors. But If you look at transactions.py line 295, only a debug message is printed when an incomplete message is received. Shouldn't the code instead raise a InvalidMessageReceivedException
As it looks now the code is just continuing even if the message received is incomplete.
Line 259 in the same file also looks for invalid message but if it's found it raises and exception not printing debug.

[dhoomakethu]

@kjetilmjos We are not raising the error when received data length is less than expected to ensure that further retries can add the missing the data to the buffer. The error would still be raised during processing when the frame doesn't meet the expected frame format. This is still debatable and I will keep this open for some more time. Sorry for the dealy.

[kjetilmjos]

No worries, thanks for looking into this.

[thormick]

I've run into this issue as well, which manifests when there's an incomplete message. Ideally the behavior should be to either return a correct, complete response, or raise an exception, but not "strange values".

In lieu of that, for us it's fine to just ignore incomplete messages, wait and retry, but a way to know there's an issue right away would be very preferable. Is there any way to return information back to the caller that the ModbusTransactionManager._recv method found an issue? Introduce a flag that can be set to trigger an exception if something is detected to be off? Being able to provide an error handler callback would be sufficient, but I imagine introducing that would be excessively complicated here.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.1% Duplication

[janiversen]

After having tested the issue, raising an exception is not the solution. The problem with incomplete messages is badly handled, but raising an exception is breaking the code (please remember that especially on SERIAL receiving incomplete messages happens quite often).

There are a couple of other PRs to the same theme, that I will try next.

[thormick]

Note that this PR is 3 years old, so it's against code that's much older, and kjetilmjos didn't understand the issue to begin with, partially because the error messages back then were misleading (it used to say "incomplete message received" in cases where no message had been received at all because it had timed out). I took over looking into this on our end from kjetilmjos, investigated this issue and reported this as #538 , and fixed it with #575. Which isn't to say that it can't be improved, but this PR should have been closed long ago.

[janiversen]

Well it was not closed, maybe because someone thought the idea was not that bad !

And of course I rebased it to be against the newest dev (after all at the moment I am the most active developer), after having rebased it, it still made sense to me, hence the approval.

I have in the same sense looked through all other open PR´s relating to problems with incomplete messages and done exactly the same.

This is all part of a larger scheme to refactor the transport/protocol layer and make it common for the whole library, and not as today, where we have multiple implementations of the same theme, but with different solutions.

Have a look at the current dev, and if there are things you are unhappy with, please submit a PR.

[thormick]

Yep, I gathered as much, I just wanted to point out that the specific issue that the PR submitter tried to fix has been resolved in the meantime. So if this PR made sense, well good, but as it is it's fine to close this.

From what I remember the sort of refactoring you mention would make sense, so good luck with that. I doubt I'll have time to look much at it myself though, even though we (Gapit) have a lot of pymodbus based microservices in production, it hasn't been the focus going forward.

[janiversen]

Is there a reason why you have changed focus ? something we can do ?

I started on Pymodbus because I use it in home assistant (Modbus integration), and my aim is to make the HA integration a lot faster (currently it is sync based and far too slow).

At least I think you can see there have been quite a number of changes during the last months, making the code PEP8 and using python 3.8+ features a lot more.

[thormick]

It's mostly that Gapit wants to see if using Node.js/node-red with node-red-contrib-modbus is suitable for our needs. That project hasn't gotten far enough to say so for certain, though there seems to be a lot more performance/scalability limitations to that whole approach. So there isn't really anything on the Pymodbus side that you can address.

But that there's progress is good to hear. I'm quite certain that we'll still be having Pymodbus in production for the foreseeable future, so perhaps I can get back to checking it out again. Gapit only uses sync based TCP connections, which is sufficient for a microservice approach for data fetching, so as long as it works there isn't a big reason to touch our stack.