Skip to content
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

Merged
merged 1 commit into from
Jan 22, 2016
Merged

Conversation

agentmnm
Copy link
Contributor

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

… 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):
Copy link
Owner

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

@miccoli
Copy link
Owner

miccoli commented Jan 13, 2016

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: read(path, 20) now means read(path, size=20) and there is really no compelling reason to introduce a not backward compatible change.

@agentmnm
Copy link
Contributor Author

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.

@miccoli
Copy link
Owner

miccoli commented Jan 14, 2016

I will pull the proposed changes to a new branch and start working from there. Please stay tuned.

@agentmnm
Copy link
Contributor Author

Okey dokey, thanks for the heads up.

@miccoli
Copy link
Owner

miccoli commented Jan 14, 2016

Pulled (with modifications) in 526b5a7

@miccoli miccoli merged commit 0b52c1b into miccoli:dev Jan 22, 2016
@agentmnm
Copy link
Contributor Author

Cool! Thanks for improving my code and merging the timeout functionality on a per call basis.

@agentmnm agentmnm deleted the dev branch January 24, 2016 13:04
@miccoli
Copy link
Owner

miccoli commented Jan 24, 2016

@agentmnm you are welcome. Thanks for the pull requests: I think that the per call timeout is much better than a global one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants