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

Add optional timeout parameter #47

Merged
merged 1 commit into from
Jan 18, 2021
Merged

Conversation

vinnyfuria
Copy link
Contributor

This is a perquisite to fixing an issue in the radiotherm component of HASS

The problem is, the radiotherms have pretty unreliable networking. Resulting in timeouts in about 1% of requests. By default though, the urllib has a really long timeout, which results in the HASS component crapping itself. So we need a way to override the default tiemout, which requires this fix.

For some further context, I did some timing tests against my CT50, and saw the following:

Count:    17121
Min:      2.238989
Max:      61.574224
Mean:     2.549727
dSquared: 27086.792889
Variance: 1.582080
Stdev:    1.257808

4 seconds seemed a reasonable default for use on local networks; happy to leave that at urllib's default if consensus is that makes more sense.

@vinnyfuria
Copy link
Contributor Author

@mhrivnak Are you, or someone else, actively maintaining this library? This fix, or something similar, is needed to fix this issue in Home Assistant.

@mhrivnak mhrivnak merged commit b2230c7 into mhrivnak:master Jan 18, 2021
@cjkrolak
Copy link

cjkrolak commented Sep 5, 2021

@vinnyfuria I have two 3m50 thermostats and found that the 4 second timeout introduced here is too short, it results in random timeouts on both thermostats during the get method, so I am using a 10 second timeout instead, a 6 sigma limit so that the request never times out just due to chance.

Your statistics above result in a z-score of 1.17, [i.e. (4 - 2.549727) / 1.257808] or the 88th percentile, meaning you should be timing out with a 4 second limit about 12% of the time just due to chance. Are you seeing any timeouts with this 4 second limit imposed?

@vinnyfuria
Copy link
Contributor Author

@cjkrolak I'm logging about 10 timeouts a day using the 4 second timeout and sampling once a minute. I noticed diminishing returns increasing this value as I saw that the thermostat would often just not respond at all in many cases.

The value is configurable in the radiotherm library now -- if you're having problems in home assistant, it would be pretty easy to add a configuration to override the default.

(I realized long after the fact that my statistics above were skewed by the tool I was using having an unaccounted timeout of 60 seconds, so non-responsive queries appeared as responses that took about 60 seconds)

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

Successfully merging this pull request may close these issues.

3 participants