Initial Commit of the Autelis Pool Control binding for openHAB #2233
Conversation
|
openhab » openhab #2437 SUCCESS |
|
openhab » openhab #2438 SUCCESS |
|
@digitaldan thanks a lot for this contribution. Will come back to you once i've finished the Review. |
|
openhab » openhab #2445 FAILURE |
|
openhab » openhab #2446 SUCCESS |
| * run has been executed we only update an item if the value is different then what's in the | ||
| * cache. Not sure if there's a better way to do this. | ||
| */ | ||
| private Map<String, State> cachedItemState; |
There was a problem hiding this comment.
@teichsta , is there a better way of doing this? Is there a way of looking up an items state somewhere in openHAB. If I forgo the cache and just do a postUpdate I think it still sends it on the bus regardless if the state is the same or not.
There was a problem hiding this comment.
In general Bindings should be stateless since the Items' state can (potentially) be changed from other instances as well. Hence such caching should be avoided. See also this recent discussion about the usage of the ItemRegistry directly https://groups.google.com/d/msg/openhab/UeexB3MRLBU/HmmxrR4WUvoJ.
There was a problem hiding this comment.
Thanks @teichsta , that was my first thought as well, but I could not find any external bindings using the itemRegistry so I wasn't sure if that was appropriate or not.
There was a problem hiding this comment.
@digitaldan : In general, it is not ok. As I mentioned in the referened thread, whenever you have a "real" update available through polling, you should also send it and not check for differences in the state yourself. So you should normally not need any kind of general state caching. In the referenced thread there were some special situations though, where the use of the ItemRegistry seemed to be the only possible workaround. This does not make using the ItemRegistry a recommended architecture!
There was a problem hiding this comment.
The problem I have then is if we poll every 5 seconds, we will generate an awful lot of noise when nothing has changed, this noise finds its way to connected clients, and in my cause would cause a lot of unnecessary polling by the IOS and Android clients (yes I'm sensitive here). I would prefer then to leave it the way it is for OH 1.x and revisit this in 2.0 when I port the binding if thats OK?
There was a problem hiding this comment.
i'll double check, I could see the messages being added to the broadcast cache for clients ( just happened to have trace debugging on for the rest binding), so I'm assuming they were being delivered.
There was a problem hiding this comment.
So ResourceStateChangeListener.java in the rest binding is listening for state updated events and ignores state changed events, there was some code there but it has been commented out. Should it be the other way around?
There was a problem hiding this comment.
so I'm assuming they were being delivered.
So maybe this would be the right place to work on then :-)
There was a problem hiding this comment.
Actually I lied, both updated and changed are broadcasting, I will disable the call to broadcast on update, I'm not sure what the original intention was however.
There was a problem hiding this comment.
@kaikreuzer see pr #2269, its a one line change, my initial testing proved fine, I will continue to test on my system. Note, this one line change may be a significant improvement to users with "chatty" systems that we have recently addressed in the rest binding.
|
openhab » openhab #2466 SUCCESS |
|
openhab » openhab #2467 SUCCESS |
|
openhab » openhab #2488 SUCCESS |
|
openhab » openhab #2491 SUCCESS |
|
openhab » openhab #2664 SUCCESS |
|
Hi any chance to get this merged? I would like to cross this off my todo list ;-) |
|
i'll start the review now … so it will be part of the RC2 |
| org.osgi.service.component, | ||
| org.osgi.service.event, | ||
| org.slf4j | ||
| Export-Package: org.openhab.binding.autelis |
There was a problem hiding this comment.
remove this export unless you really need the package elsewhere
|
Hi @digitaldan, this PR looks quite good already. Please find my review comments inline. Please squash your commits into one once you've incorporated my feedback. Thanks, Thomas E.-E. |
|
@teichsta thanks so much for the thoughtful review, the 1.7 release looks fantastic! |
Signed-off-by: Dan Cunningham dan@digitaldan.com (github: digitaldan)
digitaldan
force-pushed
the
autelis-binding
branch
from
33dd2ac
to
c54fe3f
Compare
|
openhab » openhab #2898 SUCCESS |
|
@teichsta this should be good to merge, thanks. |
Initial Commit of the Autelis Pool Control binding for openHAB
|
Thanks @digitaldan! I am happy to also link the according Wiki page ;-) |
|
Thanks @teichsta! I just added the wiki page so I think we are good. |
|
yep, thanks! |
Signed-off-by: Dan Cunningham dan@digitaldan.com (github: digitaldan)