Isy binding #109

HentschelT · 2017-11-16T05:45:47Z

pull request for:

venstar thermostat isy bindings
insteon smoke detector isy bindings
EZX10-RF insteon-X10 RF bridge (doesn't do much, just shows up)
various other switches/inline/outlet devices that mapped into existing devices
some NPE bug fixes

Venstar Thermostat has all functionality, just need to find a way to get a choice (selection) thing going in paper UI. For now, just set thermostat mode + fan setting with the same strings as the drop-downs in the UDI isy994 management console.
The venstar has a problem in that it may take a few tries to get all the states updated. This is even visible in the UDI management console, sometimes some of the states there are blank.
I don't have one of the newer smarthome thermostats, but I believe the venstar functionality should be adaptable (need type UID for those)

Thanks,
-Th

: also fix one more instance of list of variable being empty

adding older style inline lync's to definition

Support for Insteon Smoke bridge to Onelink Smoke Alarms https://www.smarthome.com/insteon-2982-222-smoke-bridge.html

initial cut for venstar thermostat (potentially others too)

venstar thermostat

craigham · 2017-11-17T22:20:52Z

...org.openhab.binding.isy/src/main/java/org/openhab/binding/isy/handler/IsyHandlerBuilder.java


 public class IsyHandlerBuilder {

    IsyDeviceHandler handler;

    protected IsyHandlerBuilder(Thing thing) {
-        this.handler = new IsyDeviceHandler(thing);
+        // for thermostat, return a specialized handler based on type UID
+        if (IsyBindingConstants.VENSTAR_THERMOSTAT_THING_TYPE.equals(thing.getThingTypeUID())) {


I think it would be better to not add the thermo support to this builder. I created this builder so we could use a generic handler for most of the isy items. However thermostats are quite the beast on their own, and you underestandably you created the specific handler. In the handler factory I would just instantiate the handler you wrote, and then you could embed all that channel mapping right into the handler class, instead of creating it and passing in to the builder. Make sense?

Yea, I wasn't too happy about this choice myself - it was basically the fastest way to get that handler created. I'll change it to what you say here, just give me a couple days.

craigham · 2017-11-17T22:22:12Z

...hab.binding.isy/src/main/java/org/openhab/binding/isy/discovery/IsyRestDiscoveryService.java

+        // (TH) POTENTIAL HACK
+        // it would be nice to restart the web socket to the ISY after discovery is done
+        // the web socket is started to early, before any isy "things" are created, thus the initial ws update from the
+        // isy is missed


TH, I have done quite a bit of work on a couple other bindings since I started this one. There is a channelLinked method (I think in thing handler chain of hierarchy) which can be overridden and we can query the ISY for initial updates when items are discovered. I spend a good bit of the year away from my house which has the isy so I have fallen behind on refactoring this binding until I get back there. I will be returning to that house early jan so can get started soon.

specifically, you can override channelLinked within the handler. This method gets called whenever a channel is linked to an item, so then you get the info from the isy.

One thing I noticed on the ISY is that the websocket (on initial connect) spits out device information that I cannot get on the REST interface. I'm not sure if that's intended or a bug on the ISY side.

Just throwing this in: If the web service handler could keep the information from the initial contact (organized by insteon address, perhaps) and then forward that information as a update to the device on "channelLinked", that would be the best option I think (but probably also the hardest to implement)

That’s interesting. Never thought about all that other info maybe because I hadn’t got to wanting to use it. Do you mean things like ramp rate, on level, etc for lights for example? I haven’t used my isy now in 6 mos so a little rusty.

One thing I noticed is a field called "UOM" (you may see some commented references in the code), which is only send on the initial websocket connect. I believe that's the current unit setting for the thermostat, which I should probably consider (need to read some more on the thermo doc's). I may have to do a different conversion based on that setting, but for now it's lost and I don't see it in the (browseable) REST interface. Not sure what else is sent that isn't part of what can be seen on the REST side

IsyHandlerBuilder see discussion: QuailAutomation#109 (comment)

craigham · 2017-11-20T01:10:55Z

...org.openhab.binding.isy/src/main/java/org/openhab/binding/isy/handler/IsyHandlerFactory.java

@@ -118,7 +119,7 @@ protected ThingHandler createHandler(Thing thing) {
                    .addChannelforDeviceId(CHANNEL_SMOKEDETECT_LOWBAT, 6)
                    .addChannelforDeviceId(CHANNEL_SMOKEDETECT_MALFUNCTION, 7).build();
        } else if (thingTypeUID.equals(VENSTAR_THERMOSTAT_THING_TYPE)) {
-            return IsyHandlerBuilder.builder(thing).addChannelforDeviceId(CHANNEL_VENSTAR_MAIN, 1)
+            return VenstarThermostatHandlerBuilder.builder(thing).addChannelforDeviceId(CHANNEL_VENSTAR_MAIN, 1)


Thomas. I may have confused you with my suggestion before. I don't think we need a builder for the Thermo handler. I think within the handler factory you should just instantiate the thermostat handler directly.

eg. return new VenstarThemostatHandler(thing)

I just used that builder pattern because for a lot of the isy devices, it is really just handling on/off commands, so I went the route of creating generic code. For the thermostat, I think you need a custom handler as you have written.

Here is an example of the factory for another binding I worked on with one of the more prolific committers for Openhab. Check out how we just directly instantiate the handlers based on the thing type. this would eliminate your thermo builder class, and all that channel mapping which is done for the more generic isy handlers would just be encapsulated within the thermostat handler.

https://github.com/QuailAutomation/openhab2-addons/blob/digitaldan-omnilink-binding-thing-types/addons/binding/org.openhab.binding.omnilink/src/main/java/org/openhab/binding/omnilink/internal/OmnilinkHandlerFactory.java

Let me know what you think.

Well, the problem is that the Venstar is a mix between the two - one channel is special, the remaining 3 are handled like any other you already implemented. One reason I followed your pattern is that I don't know if any other Thermos use the same channel id's for channels handled the standard way (which then could be adapted in that factory for that new device) Alternatively, we could just create another class for any new thermo device.

But I guess you're right, we could worry about that when that actually comes up. I'll make a change to just set everything up in the handler ctor for now..

ok, 3rd is a charm (I hope :) )
Commit 7a8f194

craigham · 2017-11-20T01:38:47Z

Thanks Thomas. I realize this is more difficult on you because there isn’t a clear pattern used for the more sophisticated handlers. That omnilink binding is a really good reference that I will be refactoring this binding to look more like when I get back to my isy, which will be within 2 months. Cheers, Craig

…

On Sun, Nov 19, 2017 at 5:24 PM Thomas Hentschel ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In addons/binding/org.openhab.binding.isy/src/main/java/org/openhab/binding/isy/handler/IsyHandlerFactory.java <#109 (comment)> : > @@ -118,7 +119,7 @@ protected ThingHandler createHandler(Thing thing) { .addChannelforDeviceId(CHANNEL_SMOKEDETECT_LOWBAT, 6) .addChannelforDeviceId(CHANNEL_SMOKEDETECT_MALFUNCTION, 7).build(); } else if (thingTypeUID.equals(VENSTAR_THERMOSTAT_THING_TYPE)) { - return IsyHandlerBuilder.builder(thing).addChannelforDeviceId(CHANNEL_VENSTAR_MAIN, 1) + return VenstarThermostatHandlerBuilder.builder(thing).addChannelforDeviceId(CHANNEL_VENSTAR_MAIN, 1) Well, the problem is that the Venstar is a mix between the two - one channel is special, the remaining 3 are handled like any other you already implemented. One reason I followed your pattern is that I don't know if any other Thermos use the same channel id's for channels handled the standard way (which then could be adapted in that factory for that new device) Alternatively, we could just create another class for any new thermo device. But I guess you're right, we could worry about that when that actually comes up. I'll make a change to just set everything up in the handler ctor for now.. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#109 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AK5qXronOJxRW_wLKnAFJvsBBc7eb0dWks5s4NTJgaJpZM4Qf_fN> .

HentschelT · 2017-11-20T01:47:30Z

Craig, yea, no problem, I think I can see your intend of trying to keep that handler factory from becoming too unwieldy (esp. for cases where it's not really needed).

https://github.com/QuailAutomation/openhab2-addons into QuailAutomation-isy-binding

HentschelT · 2017-11-20T03:01:19Z

merged upstream, so should be able to apply pull request

craigham · 2017-11-20T22:45:07Z

Thanks for the changes Thomas. Please test this build, I can't.

craigham · 2017-11-20T22:45:18Z

I will advise when the build is complete

craigham · 2017-11-20T23:07:10Z

build is done, is avail via marketplace or s3 download link

HentschelT · 2017-11-21T01:25:43Z

Ok, installed via marketplace, everything looks good, poked around in some of the devices after install. Discovery works well here, and actuation of items as well.

One snag I ran into (and that doesn't have to do anything with the build): I had created a prior add-on build for testing on my RPi via 'mvn clean package' and dropped that build into the 'add-on' directory of ohab. It appears that the marketplace add-on install and the install via 'add-on' doesn't work too well together yet. As a result I saw a similar error as someone had posted on the UDI forum (https://forum.universal-devices.com/topic/21026-isy-binding-for-openhab-2/?p=207562), which could explain the issue seen back then.

Anyway, stopping the service, removing the build in 'add-ons' and restarting the service got things going again with the build installed from the marketplace.

HentschelT added 6 commits November 13, 2017 15:27

fixed NPE when discovering empty var's list

2407032

: Add outletlinc to ISY bindings

5bebdb2

: also fix one more instance of list of variable being empty

: Add inline switch to ISY bindings

da9e584

adding older style inline lync's to definition

: Add smoke detector to ISY bindings

94052df

Support for Insteon Smoke bridge to Onelink Smoke Alarms https://www.smarthome.com/insteon-2982-222-smoke-bridge.html

: Add Venstar thermostat to ISY bindings

abcfab4

initial cut for venstar thermostat (potentially others too)

: Add X10 receiver to ISY bindings, add 6 way switchlinc, debugging

aaf07ec

venstar thermostat

craigham reviewed Nov 17, 2017

View reviewed changes

: Fix builder for thermostat, create own builder, remove from

869446a

IsyHandlerBuilder see discussion: QuailAutomation#109 (comment)

craigham reviewed Nov 20, 2017

View reviewed changes

Remove builder, instantiate thermo directly

7a8f194

HentschelT added 2 commits November 19, 2017 18:55

Merge branch 'isy-binding' of

7a56477

https://github.com/QuailAutomation/openhab2-addons into QuailAutomation-isy-binding

merging upstream commit

d00de60

craigham merged commit 5c50f45 into QuailAutomation:isy-binding Nov 20, 2017

HentschelT deleted the isy-binding branch November 23, 2017 20:42

HentschelT restored the isy-binding branch November 27, 2017 07:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Isy binding #109

Isy binding #109

HentschelT commented Nov 16, 2017

craigham Nov 17, 2017

HentschelT Nov 18, 2017

craigham Nov 17, 2017

craigham Nov 17, 2017

HentschelT Nov 20, 2017

craigham Nov 20, 2017

HentschelT Nov 20, 2017

craigham Nov 20, 2017 •

edited

HentschelT Nov 20, 2017

HentschelT Nov 20, 2017

craigham commented Nov 20, 2017 via email

HentschelT commented Nov 20, 2017

HentschelT commented Nov 20, 2017

craigham commented Nov 20, 2017

craigham commented Nov 20, 2017

craigham commented Nov 20, 2017

HentschelT commented Nov 21, 2017

Isy binding #109

Isy binding #109

Conversation

HentschelT commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

craigham Nov 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

craigham commented Nov 20, 2017 via email

HentschelT commented Nov 20, 2017

HentschelT commented Nov 20, 2017

craigham commented Nov 20, 2017

craigham commented Nov 20, 2017

craigham commented Nov 20, 2017

HentschelT commented Nov 21, 2017

craigham Nov 20, 2017 •

edited