Add optional request params to reject truncated responses and error responses. #2230
Conversation
jameshilliard
force-pushed
the
error-truncation-request
branch
5 times, most recently
from
f1fc745 to
2a70190
Compare
There was a problem hiding this comment.
First of all it seems you forgot to cover the sync client.
The purpose of the clients in the library is to "send requests and return healthy responses with automatic retry" ! This PR starts to add an extra layer that changes the definition to "send requests and return expected responses with automatic retry", it might be better to that in a higher level client (which correspond to the discussion about managing a whole device).
| @@ -109,6 +109,14 @@ def getRegister(self, index): | |||
| """ | |||
| return self.registers[index] | |||
|
|
|||
| @property | |||
| def truncated(self) -> bool: | |||
| """Check if the response has been truncated.""" | |||
There was a problem hiding this comment.
You are missing a lot of requests that also have length
if implemented it must be implemented for all requests !
There was a problem hiding this comment.
Yeah, I just started with this one, still need to go through the others and add truncation properties.
pymodbus/pdu/pdu.py
Outdated
| @@ -102,12 +102,15 @@ class ModbusRequest(ModbusPDU): | |||
|
|
|||
| function_code = -1 | |||
|
|
|||
| def __init__(self, slave=0, **kwargs): # pylint: disable=useless-parent-delegation | |||
| def __init__(self, slave=0, error_responses=True, truncated_responses=True, **kwargs): # pylint: disable=useless-parent-delegation | |||
There was a problem hiding this comment.
Error response have nothing to do with truncate, and that is something the app need to handle.
There was a problem hiding this comment.
Error response have nothing to do with truncate
So I added that variable to allow tweaking the retry handler behavior for error responses similar to truncated responses since depending on desired behavior on may want to retry on different error conditions until successful.
that is something the app need to handle
I had added this since I wasn't seeing a good approach once could use to handle this app side due to the retry handler and counter being part of the client.
Yeah, the sync client code is rather...tricky IMO, trying to get it working there.
IMO this is just allowing the definition of a healthy response to be configured per request.
I'm not sure the higher level client layer would be where it makes sense to implement retry logic, that layer I'm thinking would be more for providing a register mapping layer AFAIU but the retry logic seems to me that it would need to be handled at a lower level. |
jameshilliard
force-pushed
the
error-truncation-request
branch
12 times, most recently
from
2161d65 to
c073a54
Compare
jameshilliard
force-pushed
the
error-truncation-request
branch
from
c073a54 to
c41a058
Compare
|
having a short read, is normal (I am sitting with a device right now that does it), f.x. if the app read registers non existent. As a consequence it is something the app should handle ! Many of the error responses, are not dynamic in nature, so adding them to the retry does not work, apart from that the app needs to check the error flag (as pr other discussion, it is a major change which we can think of for 4.0). So I do not see a lot of value in this PR (apart from the review comments). |
|
Still working on this one ? |
I mean, I made it a flag tied to the specific request because while truncation may be normal for some devices/registers, it may be not normal for others and indicate some other issue.
At the moment I'm using a wrapper that implements an additional layer of retry logic for both truncation and error flags(retries do seem to reliably mitigate the issue at least with the devices I'm working with), but having two retry loops(one in the app one in the library) effectively doing the same thing seems a bit weird.
Hmm, I'm guessing this is somewhat firmware dependent, I think having a retry on error capability is still seems somewhat useful in case apps want to delegate retries to the library.
Yes, but somewhat prioritizing the transaction and tests cleanup at the moment. |
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
This PR was closed because it has been stalled for 10 days with no activity. |
Adds tracking for truncated responses and the ability for the retry handler to reject truncated and error responses based on request flags.