Retry receiving response if response is behind request#41
Conversation
NModbus4/IO/ModbusIpTransport.cs
Outdated
There was a problem hiding this comment.
Since OnShouldRetryResponse is internal what means it's implementation detail, we should on debug stage determine that we pass wrong arguments here.
I think we should replace
if (request is IModbusRequest)
with
Debug.Assert(request is IModbusRequest, "Expected IModbusRequest")
Thoughts?
There was a problem hiding this comment.
But I didn't noticed that we have ModbusMaster.ExecuteCustomMessage so we could
a) assert
b) throw
c) nothing
Interesting your opinion.
There was a problem hiding this comment.
I just used the style of the existing code so that the change would be minimal, rather than doing things how I would normally do. I dislike Debug.Assert, especially when in this case since things are internal you would be better off changing the data type of the request parameter to be IModbusRequest.
I'm not sure what purpose ExecuteCustomMessage serves - it's not referenced within the codebase, covered by a test, documented or covered by a sample. Personally I would either change the signature so it takes IModbusRequest, or remove it altogether. Probably changing it is better.
Let me know what your opinion is, and you can either do this as a separate task since it's unrelated to this PR, or I can bundle it up in here too.
|
@richardlawley I've left a couple comments but otherwise LGTM. |
richardlawley
force-pushed
the
RetryOnResponseBehindRequest
branch
from
b6b2246 to
d0a60ed
Compare
|
Updated to reflect your comments, and so the tests point at the correct methods. |
Retry receiving response if response is behind request
|
Thanks, merged. |
2 participants
This introduces the ability for a master to retry receiving a response if the one it picked up was from an earlier request (which timed out according to us). We use this when communicating with a device which sometimes responds slowly. Without it, it is possible for a master to be unable to recover - once one response arrives late, there is always one queued until the connection is closed.
The default behaviour is unaffected by this PR (default is off). To enable it:
Tests have been added to cover the provided functionality.
This is a function I've had in my own fork and been using in production for a couple of years now.