Fix retries in ModbusBaseClient.async_execute() #1750
Conversation
The code in async_execute() in ModbusBaseClient appears to be written to retry, but the send is not in the loop, so the function never retries. To fix this put the transport_send() command inside the while loop.
test_client_protocol_execute() was being skipped because the test no longer worked. Add a mock transport that produces the appropriate response and ensure it gets passed through execute.
Add tests to ensure retries send data the appropriate number of times and a timeout with ModbusIOException occurs after sufficient retries are sent.
| raise ModbusIOException( | ||
| f"ERROR: No response received after {self.params.retries} retries" | ||
|
|
||
| count = 0 |
There was a problem hiding this comment.
This is not correct, retries is "retry to read" not to send. We cannot automatically send a request multiple time, at least not all requests.
Example: App sends a write request to step up a motor 1 step, if you repeat that it might be fatal. Some devices act on read requests and e.g. initiate an expensive remote request.
Only the app knows if it is safe to resend a request, so we have on purpose left that to the app.
|
An alternative would be to add a "request_retry=False" to the request calls, If true the code should repeat the send as well as build_response. It must be at request level, because a single device might have a mixture of "dangerous" and free commands. |
|
I personally find that quite non-intuitive. But it explains the confusion I had around retries in this code base. I really don't understand what the point of a retried "read" is. The same thing can be accomplished by multiplying the timeout by 3. I just thought I was fixing a bug when I was debugging some communications issues and the debug log said failed after 3 retries even though I only saw one request happen on the wire. I ended up fixing the underlying communications issue so I don't really need the changes in this PR. Seems to me like it would be more sensible to support automatic retries (including the send) and then just make the default to only send one and make it opt-in to retry. Dangerous applications need not apply. If someone needs per-request retries that would be another feature they should implement. Ultimately this is sounding like a larger conceptual change that would cut across the whole project. Sorry, but I don't really have time for such a project at the moment. Feel free to use my changes as part of a larger effort, if you like. Or, patch 2 fixes one of the skipped tests in a way that aided writing a test for retries, that might be useful. |
|
Your PR was sound which is why I took time to explain the reasoning. A change as you suggest is basically the same I suggested, all it takes is an extra parameter in base.py and then test it on in your patch. Might be I will take a look to amend your patch. I will take your tests and add them, thanks. |
2 participants
When using the asynchronous client and no response is received when a request is sent, the code sort of retries, but doesn't actually send the packets to retry. This PR fixes that issue, fixes the test for async_execute() and adds two additional tests to verify the behaviour of retries.