-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Gardena: Optimized refresh after command #2353
Conversation
Signed-off-by: Gerhard Riegler <[email protected]>
@gerrieg, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kaikreuzer to be a potential reviewer. |
@@ -342,9 +337,10 @@ public void sendCommand(Device device, GardenaSmartCommandName commandName, Obje | |||
} | |||
|
|||
if (command != null) { | |||
stopRefreshThread(); |
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.
Is that really the best solution? To cancel a scheduled job and reschedule it again, just to execute a command in between?
Shouldn't at least a running task be allowed to end gracefully first instead of being interrupted?
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.
The best solution would be to delay the task at least 6 seconds. If the next execution < 6 seconds, the values are refreshed but are wrong, because the gardena cloud has not yet updated the values from the command.
So i have to cancel the refresh thread and reschedule it with a 6 seconds delay. Or do you know a better way?
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.
What was wrong with your previous "intermediate" refresh?
So i have to cancel the refresh thread
You currently do this with the true
parameter, which means that it is interrupted, even if the job is executed at that moment. I'd think you should give any existing job the chance to finish and wait for its termination first.
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.
The intermediate refresh was to fast, it was after 3 seconds. OK, i can do it in 6 seconds BUT, if the normal refreshThread runs in < 6 seconds, the values are wrong.
e.g if i switch the watering computer valve to ON, it jumps to OFF and then again to ON within some seconds. That breaks rules with multiple watering computers.
OK, i removed the true parameter now
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.
OK, i removed the true parameter now
But this now applies for all occasions, e.g. also for the handler disposal - which should be as quick as possible and there it was fully ok to interrupt potentially long running HTTP requests...
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.
You are right, i changed it.
It's late, i should go to bed now, it was a long day for me 😫
Signed-off-by: Gerhard Riegler <[email protected]>
Signed-off-by: Gerhard Riegler <[email protected]>
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.
Thanks for your patience :-)
* master: [senseBox] Forgot to add binding to feature.xml (openhab#2371) [Kodi] Fix audio sink (openhab#2301) (openhab#2303) Initial contribution of a Niko Home Control binding. (openhab#1589) [keba] fixed typo (openhab#2370) Removed utilisation of org.apache.http.client (openhab#2369) [RFXCOM] Make LWRF mood buttons work (openhab#2366) [senseBox] Switch caching to ESH ExpiringCacheMap (openhab#2361) Fixed problems found by AuthorTagCheck in the cometvisu bundle (openhab#2364) Added author tag where missing. (openhab#2351) [RFXCOM] Fix chill temperature (openhab#2362) initial contribution Windcentrale binding (openhab#1535) Tesla : reset the Even Stream connection to the Tesla Back-end if the event time stamps differ too much from the current system time (openhab#2358) Homematic: Some updates (openhab#2346) [RFXCOM] Support all data from wind sensors (openhab#2329) Gardena: Optimized refresh after command (openhab#2353) Fix some javadocs. (openhab#2349) Some small non-standard copyright banners which we missed while reviewing (openhab#2347)
* Optimized refresh after command Signed-off-by: Gerhard Riegler <[email protected]>
* Optimized refresh after command Signed-off-by: Gerhard Riegler <[email protected]>
* Optimized refresh after command Signed-off-by: Gerhard Riegler <[email protected]>
* Optimized refresh after command Signed-off-by: Gerhard Riegler <[email protected]>
* Optimized refresh after command Signed-off-by: Gerhard Riegler <[email protected]>
* Optimized refresh after command Signed-off-by: Gerhard Riegler <[email protected]>
* Optimized refresh after command Signed-off-by: Gerhard Riegler <[email protected]>
* Optimized refresh after command Signed-off-by: Gerhard Riegler <[email protected]>
* Optimized refresh after command Signed-off-by: Gerhard Riegler <[email protected]>
…2353) Signed-off-by: Christoph Weitkamp <[email protected]>
Signed-off-by: Gerhard Riegler [email protected]