Resubmit: Don't close/reopen tcp connection on single modbus message timeout #2350
Conversation
pymodbus/client/base.py
Outdated
|
|
||
| return resp # type: ignore[return-value] | ||
|
|
||
| def no_message_response_after_retries(self, retries) -> None: |
There was a problem hiding this comment.
Is there a reason to single out tcp ? tls and udp also use socket, so they should at the very least be included, and I cannot see it hurts to include serial as well, and thus make a general solution (which is much less confusing).
There was a problem hiding this comment.
I was being cautious and not changing behaviour I can't test. The thing about TCP is that we do have a connection protocol and hence the lower layers will tell us if the connection needs to be closed and reopened. (i.e. there's a FIN or RST packet, or a request is not ack'ed at the transport layer.) This isn't true of UDP - though it is for TLS.
Happy to be guided by you though - many thanks for comments.
There was a problem hiding this comment.
This is very true for UDP as well, in the way we use it.
For serial it is the same since we have the device drivers to tell us if there are problems.
We do not want special solutions for different transport, that confuses the end users, and makes maintenance more difficult.
pymodbus/client/tcp.py
Outdated
| @@ -83,6 +83,13 @@ def __init__( # pylint: disable=too-many-arguments | |||
| on_connect_callback, | |||
| ) | |||
|
|
|||
| def no_message_response_after_retries(self, retries) -> None: | |||
| """Take action after a message has not been responded to after retries.""" | |||
| """Just log exception - no other action needed on TCP connections.""" | |||
There was a problem hiding this comment.
Why make a string, if you do not use it ?
Fixed. It should have been a comment not a string. |
|
Please remember to mark as ready for review when you have addressed the requested changes. |
janiversen
force-pushed
the
tcprtu-multi2
branch
from
3a68953
to
f146b89
Compare
Github closed my pull request #2346 during my resync of the upstream changes.
Have applied your feedback to a new branch. Apologies that the comments trail has been disconnected.
This update preserves the normal function of close() as you recommend, and instead modifies async_execute. The intent is that TCP modbus connections are not torn down on the lack of response to a request which maybe aimed at an inactive slave id.
Updated PR to make it work.
Each request is retried X times, if not successful ExceptionResponse is returned.
If 3 requests do not receive a response, the connection is terminated.
Every request with a response, resent the connection disconnect counter.