search

AdvancedClimateSystems / uModbus

Python implementation of the Modbus protocol.
Mozilla Public License 2.0
211 stars 82 forks source link

Read expected response size #59

Closed lutostag closed 6 years ago

lutostag commented 6 years ago

This should fix #49

It works by checking exactly how many bytes are expected to be returned, and waiting until we receive that many... or a timeout (where we will receive fewer bytes than requested).

This was a problem with us either on slow serial devices (when setting up the connection... parity) or tcp connections that were slow to respond (as the recv call will bail out early sometimes if there is nothing already in the buffer).

I haven't tested manually, so using your travis to see what falls out and we will fix it up.

Thanks!

OrangeTux commented 6 years ago

Many thanks for your PR! Later this weekend I'll look into it.

lutostag commented 6 years ago

Appreciate it. I know how I am planning to fix up the tests. I should be able to do so tomorrow.

Also wanted to say a big thank you for putting out this library. We use it to talk to several different pieces of hardware and it is the most non-confrontational python library for modbus.

Thanks again!

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.2%) to 96.3% when pulling d0d47fe58f9b4b2e60222fad16ba16b0b919f962 on mobilityhouse:read-expected-response-size into 02014a85898008d7f8e570cdaf0d75b2feca0995 on AdvancedClimateSystems:master.

OrangeTux commented 6 years ago

Great patch. I actually learned something new from the page you link in the docstring of recv_exactly.

But I've some placed some comments.

lutostag commented 6 years ago

Fixed the comment, if there is anything else let me know. More than happy to make any further changes needed.

Thanks for being so quick with the review. :)

OrangeTux commented 6 years ago

I've added 2 other comments about handling expection responses. Have you seen them?

lutostag commented 6 years ago

@OrangeTux, no I don't see them as comments against the diff. GitHub seems to drop them sometimes (if you comment as part of a review, but don't submit at the very bottom)

OrangeTux commented 6 years ago

I think my comments should be visible now.

lutostag commented 6 years ago

Indeed, thanks for checking that, I didn't know about the error handling.

The code is a bit more verbose now on the send_message side, but I think I made it as clear as possible. If you have a better idea on how to make it easier to read, I can take another attempt at it.

Hope that it covers your concerns.

Thanks for the eagle-eyed review!

OrangeTux commented 6 years ago

I've merged your changes into 1.0.1. Note that 1.0.1 isn't uploaded to Pypi. I forgot to update the version number in setup.py. Therefore I created a new release, 1.0.2, which is fixed this. 1.0.2 is on Pypi.

Again. thanks for your effort.