-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
added timeout parameter to the functions dir, ping, present, read, write #6
Conversation
… write communication with owserver gets aborted when the given timeout is exceeded due to inner workings of owserver the granularity is in 1s steps only e.g. if 0.5s is given as a timeout, the abortion will happen after 1s has elapsed usage examples: owproxy.read('sensA/temperature',2) # reading a temperature with timeout of 2s owproxy.dir(timeout=1) # reading directory with timeout of 1s
@@ -509,19 +532,20 @@ def dir(self, path='/', slash=True, bus=False): | |||
else: | |||
return [] | |||
|
|||
def read(self, path, size=MAX_PAYLOAD, offset=0): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it is better not to break the current API: timeout=0
should always be the last argument. (Major changes should be delayed to forthcoming version 1.0.0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
Thanks for the pull request: I will look at the changes. I'm convinced that having timeouts is a very good idea, but I'm unsure if it is necessary to have a timeout argument for each method, instead of a global or per proxy instance constant value. For sure I will not change the current API: |
As for the API: I agree, changing it, was not a good idea. I was not aware that the method read can get called with the size parameter. Backward compatibility is more important than using timeout without labelling it in a call. But for individual timeout per call basis vs. global: I think it could be possible to have both. There could be a max communication time an interaction with owserver can take per default - like 10s. At the same time there could be a timeout - set by the user e.g. me - on a per call basis that overwrites the global max com time. In my case I read 25 sensors in a row. A per call base timeout of 1s would ensure that, in case there are delays with the communication to one or two sensors, that the communication to the rest of the sensor would take place in a timely manner. Let me know if I should work over this again and what you think of the changes in req and if it needs changes too. |
I will pull the proposed changes to a new branch and start working from there. Please stay tuned. |
Okey dokey, thanks for the heads up. |
Pulled (with modifications) in 526b5a7 |
Cool! Thanks for improving my code and merging the timeout functionality on a per call basis. |
@agentmnm you are welcome. Thanks for the pull requests: I think that the per call timeout is much better than a global one |
communication with owserver gets aborted when the given timeout is exceeded
due to inner workings of owserver the granularity is in 1s steps only
e.g. if 0.5s is given as a timeout, the abortion will happen after 1s has elapsed
usage examples:
owproxy.read('sensA/temperature',2) # reading a temperature with timeout of 2s
owproxy.dir(timeout=1) # reading directory with timeout of 1s