Initial contribution of a Niko Home Control binding. #1589

mherwege · 2016-12-19T21:35:42Z

Signed-off-by: Mark Herwege [email protected]

kubawolanin

Great piece of work @mherwege, congratulations!
Thank you for this binding.

I've caught a few things worth improving.
Please read through the Code Guidelines and see what else could be missing.
Additionally:

Please run code formatter on your files ("Format selection" or "Cleanup")
Please add about.html file to the binding (without 3rd party license or with 3rd party license info)

Thanks again! 👍 🥇

kubawolanin · 2017-02-15T11:01:06Z

...ns/binding/org.openhab.binding.nikohomecontrol/ESH-INF/i18n/nikohomecontrol_xx_XX.properties

@@ -0,0 +1,11 @@
+# binding


Please remove this file

File removed.

kubawolanin · 2017-02-15T11:01:21Z

addons/binding/org.openhab.binding.nikohomecontrol/ESH-INF/thing/thing-types.xml

+        </config-description>
+    </bridge-type>
+
+	<thing-type id="onOff">


Please reformat this file using the provided code formatter or clean up settings,

I ran the code formatter on all files.

kubawolanin · 2017-02-15T11:01:51Z

addons/binding/org.openhab.binding.nikohomecontrol/README.md

@@ -0,0 +1,79 @@
+# Niko Home Control Binding
+The openHAB2 Niko Home Control binding integrates with a [Niko Home Control](http://www.nikohomecontrol.be/) system through a Niko Home Control IP-interface.


Please add a newline after each heading

Add blank lines.

kubawolanin · 2017-02-15T11:07:20Z

addons/binding/org.openhab.binding.nikohomecontrol/pom.xml

+  <parent>
+    <groupId>org.openhab.binding</groupId>
+    <artifactId>pom</artifactId>
+    <version>2.0.0-SNAPSHOT</version>


Please change version to 2.1.0-SNAPSHOT

Changed snapshot version.

kubawolanin · 2017-02-15T11:07:23Z

addons/binding/org.openhab.binding.nikohomecontrol/pom.xml

+
+  <groupId>org.openhab.binding</groupId>
+  <artifactId>org.openhab.binding.nikohomecontrol</artifactId>
+  <version>2.0.0-SNAPSHOT</version>


Please change version to 2.1.0-SNAPSHOT

mherwege · 2017-02-15T22:45:04Z

@kubawolanin Thank you for your review and suggestions for improvement. I believe I have done all the changes you were requesting. I had another look at the coding guidlines and did a few changes to the logging levels based on that. I hope I have done everything right. Java development, using git and Eclipse is a new experience for me. So be critical if anything is not what it should be. I can only learn from it.
As I have not stopped working on this binding, I have added some functionality in the mean time. The current version of this extra functionality has also been committed. Current feedback from testers is that the provided functionality is perfectly usable though.
What are the next steps now? Should I keep on committing new development or stop for now and reserve that for future enhancement issues and pull requests?

ThomDietrich · 2017-03-22T23:03:41Z

A nuisance, please put the code blocks in your README in code blocks rather than indenting them.

Thanks for contributing!

mherwege · 2017-03-23T07:58:07Z

@ThomDietrich Formatting changes have been done. I believe used 4 spaces in front of the lines, which I assumed also identify code. Unless I do something wrong, the backtick blocks don't render well in the Eclipse preview.

ThomDietrich · 2017-03-23T14:59:12Z

@mherwege I don't know about the Eclipse preview. Normally I'd prefer the backticks style, as does the rest of the openHAB documentation. It's easier to manage and less error prone. One benefit is, that you can provide the language to syntax highlight with, otherwise the syntax will be auto detected (some markdown engines). The latter is not true for intended code blocks.

Correction: Just tested (docs.openhab.org) and the four spaces version is also syntax highlighted after syntax autodetection. Still the backticks version should be the preferred choice.

martinvw

Hello @mherwege thanks for your contribution! i did an initial review, can you take a look at my remarks?

martinvw · 2017-04-19T11:16:28Z

addons/binding/org.openhab.binding.nikohomecontrol/ESH-INF/thing/thing-types.xml

+                <advanced>true</advanced>
+            </parameter>
+        </config-description>
+	</thing-type>


It does not seem to be properly formatted, note that XML files should be formatted using tabs.

Should be OK now.

martinvw · 2017-04-19T13:47:13Z

addons/binding/org.openhab.binding.nikohomecontrol/META-INF/MANIFEST.MF

+Service-Component: OSGI-INF/*.xml
+Export-Package: org.openhab.binding.nikohomecontrol;uses:="org.eclipse.smarthome.core.thing",
+ org.openhab.binding.nikohomecontrol.handler;
+  uses:="org.eclipse.smarthome.core.thing,


The uses should not be there

martinvw · 2017-04-19T13:47:34Z

addons/binding/org.openhab.binding.nikohomecontrol/OSGI-INF/NikoHomeControlHandlerFactory.xml

+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+    Copyright (c) 2014-2016 by the respective copyright holders.


It seems that this file has an outdated copyright header, can you fix all of them by running

mvn license:format

Please commit only the ones for your binding.

I started development in 2016. I manually changed dates in all of them as mvn license:format does not seem to work in my environment.

martinvw · 2017-04-19T18:43:42Z

.../src/main/java/org/openhab/binding/nikohomecontrol/handler/NikoHomeControlBridgeHandler.java

+
+    private Logger logger = LoggerFactory.getLogger(NikoHomeControlBridgeHandler.class);
+
+    private NikoHomeControlCommunication nhcComm = null;


a reason you initialise this one and not for example the refreshTimer, note that I prefer not to initialize fields to their defaults.

Removed all initialize to default.

martinvw · 2017-04-19T18:44:58Z

.../src/main/java/org/openhab/binding/nikohomecontrol/handler/NikoHomeControlBridgeHandler.java

+            if (config.get(CONFIG_HOST_NAME) != null) {
+                logger.debug("Niko Home Control: bridge handler host {}", config.get(CONFIG_HOST_NAME).toString());
+                addr = InetAddress.getByName(config.get(CONFIG_HOST_NAME).toString());
+            }


if this not true (ie. config.get(CONFIG_HOST_NAME) == null) addr will remain null it feels incorrect?

A null hostname is allowed downstream as there is a mechanism to discover one. I have changed it a bit and put in comments to make this clearer.

martinvw · 2017-04-20T15:40:30Z

...java/org/openhab/binding/nikohomecontrol/internal/protocol/NikoHomeControlCommunication.java

+            // if that JSON cannot be converted, it should be an array of hashmaps in the data field
+            nhcCmdAck = new NHCCmdAck();
+            nhcCmdAck = gson.fromJson(nhcMessage, NHCCmdAck.class);
+            if (nhcCmdAck.cmd != null) {


martinvw · 2017-04-20T15:41:31Z

...java/org/openhab/binding/nikohomecontrol/internal/protocol/NikoHomeControlCommunication.java

+                    listThermostatHVAC(nhcCmdAck);
+                } else if (nhcCmdAck.cmd.equals("readtariffdata")) {
+                    readTariffData(nhcCmdAck);
+                } else if (nhcCmdAck.cmd.equals("getalarms")) {


You could consider to use a switch for this

I kept it, as the second block does look at cmd and event, making a switch more difficult. I did put it in a separate method.

martinvw · 2017-04-23T06:41:36Z

.../src/main/java/org/openhab/binding/nikohomecontrol/handler/NikoHomeControlBridgeHandler.java

+                    thing.getConfiguration().put(CONFIG_HOST_NAME, nhcComm.getAddr().getHostAddress());
+                    thing.getConfiguration().put(CONFIG_PORT, port);
+                    HashMap<String, String> properties = new HashMap<String, String>();
+                    properties.put("IP Address", nhcComm.getAddr().getHostAddress());


Is it wise to store it with spaces, should you maybe use camelCase?

I was in doubt, but followed your suggestion. I also put this all in a separate method, so I can easily call it on restart of the communication as well.

martinvw · 2017-04-23T06:43:15Z

.../src/main/java/org/openhab/binding/nikohomecontrol/handler/NikoHomeControlBridgeHandler.java

+                logger.debug("Niko Home Control: restart communication at scheduled time");
+                updateStatus(ThingStatus.OFFLINE);
+                nhcComm.stopCommunication(true);
+                thing.setProperty("IP Address", nhcComm.getAddr().getHostAddress());


Better use a constant instead of duplicating it here and on line 126

Separated the whole properties block out in a separate method, so no duplication anymore.

martinvw · 2017-04-23T06:44:20Z

.../src/main/java/org/openhab/binding/nikohomecontrol/handler/NikoHomeControlBridgeHandler.java

+            public void run() {
+                logger.debug("Niko Home Control: restart communication at scheduled time");
+                updateStatus(ThingStatus.OFFLINE);
+                nhcComm.stopCommunication(true);


Maybe this method should publicly be called restartCommunication

Agreed and changed.

mherwege · 2017-04-25T19:22:59Z

@martinvw many thanks for this review. I appreciate the time you spent on reviewing my code. I can't hide I am not a programmer by trade and Java is fairly new to me. I learned a lot from your feedback.
I have already been doing most of the adjustments you requested and will respond to each of your remarks when I have fully tested the changes. That may take some time though.
I do struggle with one specific point about your feedback on discovery. I know I should be able to register it in with an XML, but in my understanding that assumes the discovery can be self-contained and does not need an existing bridge. In my case the discovery relies on the bridge being created manually before. Discovery uses the bridge to get access the the information about the things that can be linked to the bridge. I used the milight binding as an example when I created this and it is implemented in exactly the same way there, with one difference. In the milight binding the bridge is also discovered and that is through a discovery service registering through the xml file. But the individual lights discovery is a second discovery registered the way I do it myself.
I could probably refactor the bridge creation to also allow discovery, but I am not sure it adds a lot of value. I would still need a few parameters which I now set in the bridge and would have to move these to the binding level. And the discovery service for the individual things connected to the bridge could still not be created in advance.
Is there a strong reason to have to register all discovery services through the xml? Or is my alternative of registering it when creating hte bridge handler feasible?

martinvw · 2017-04-26T18:10:50Z

@mherwege you are welcome thanks for your time to do the contribution!

I also have to take a look at the discovery, I'll be back :-)

martinvw · 2017-04-26T18:33:16Z

@mherwege I fear that you are completely right, maybe @SJKA or @kaikreuzer can tell us whether this is indeed the best way to do thing discovery for items on a bridge?

kaikreuzer · 2017-04-27T14:04:38Z

I didn't look too closely into the code yet, but it indeed seems that the ThingDiscoveryService (btw, a very awkward name here, should be better adapted to have the binding name in it as well) is nowhere registered as an OSGi service, is that right?

@mherwege is correct that it cannot be done through XML as you will effectively need to register a ThingDiscoveryService per bridge, so you might end up with multiple instances.
Therefore, the suggestion is to create an instance and register it as a service within the ThingFactory at the same place where the bridge handler is instantiated (and also unregister it again, when the bridge handler is removed). The best example to look at is the one of the Philips hue bridge here: https://github.com/eclipse/smarthome/blob/master/extensions/binding/org.eclipse.smarthome.binding.hue/src/main/java/org/eclipse/smarthome/binding/hue/internal/HueThingHandlerFactory.java#L88

mherwege · 2017-04-27T20:37:04Z

@martinvw I think I have covered all suggestions and questions. Thank you again for your time, effort and guidance on this.

@kaikreuzer My implementation borrowed the discovery piece from the Milight binding. The discovery service is not registered in the thing handler factory, but in the thing handler initialize method.

kaikreuzer · 2017-04-28T06:50:40Z

Ok, I found the registration here: https://github.com/openhab/openhab2-addons/pull/1589/files#diff-cab28c130e927102c8fbb50465c1b3c8R57, which means that the service registers itself in its code - that's a very uncommon construct.

Please note that the bundleContext is a remainder from an earlier version of the framework and that it should not be used by handler implementations - I have just created eclipse-archived/smarthome#3327 to have it removed. So my advice from above still holds: Register the discovery service within the handler factory.

mherwege · 2017-04-29T10:15:54Z

@kaikreuzer Thank you for the information. I have done as requested and moved the discovery registration to the handler factory.

mherwege · 2017-04-29T10:22:00Z

@martinvw Thank you again for all your effort reviewing this. I believe I have now covered all your requests and suggestions, as well as the issue identified by you and Kai. I am happy to follow up on any further requests or suggestions.

martinvw

Thanks for the changes, its a great improvement. I spotted some additional things. Nothing big mainly small improvements.

martinvw · 2017-05-01T16:11:00Z

addons/binding/org.openhab.binding.nikohomecontrol/pom.xml

+  <name>Niko Home Control Binding</name>
+  <packaging>eclipse-plugin</packaging>
+
+  <properties>


You could leave this out.

martinvw · 2017-05-01T18:23:18Z

.../src/main/java/org/openhab/binding/nikohomecontrol/handler/NikoHomeControlBridgeHandler.java

+                        logger.debug("Niko Home Control: cannot discover, discovery service not started");
+                    }
+
+                } catch (IOException e) {


Is this only thrown from line 111 or can it also come from other places?

Only from that line.

I need to correct myself here. It is thrown from current line 123 (was 111) and line 119. Both of them are communication issues, so that's why I catch both of them together. The debug logs would show the underlying difference.

martinvw · 2017-05-01T18:23:53Z

.../src/main/java/org/openhab/binding/nikohomecontrol/handler/NikoHomeControlBridgeHandler.java

+
+        // This timer will restart the bridge connection periodically
+        logger.debug("Niko Home Control: restart bridge connection every {} min", refreshInterval);
+        refreshTimer = scheduler.scheduleAtFixedRate(new Runnable() {


Please use scheduleWithFixedDelay which is preferred over scheduleAtFixedRate, see Coding Guidelines D.3

martinvw · 2017-05-01T18:25:48Z

.../src/main/java/org/openhab/binding/nikohomecontrol/handler/NikoHomeControlBridgeHandler.java

+            @Override
+            public void run() {
+
+                logger.debug("Niko Home Control: restart communication at scheduled time");


Do you really need to restart it by marking it offline and trying to reconnect? Should this method maybe start with a check:

if (nhcComm.communicationActive()) { return; }

I ran into issues when just checking for the connection to still be up. It looks like it was disconnecting automatically at the Niko controller side after a while whatever I did. So I fear that just checking for the connection and not doing anything will get it disconnected after a while. OH will then stop receiving channel updates without knowing anything is wrong until it sends something. I got over it by this periodic restart. I made it configurable. So it could be shortened or switched off if someone sees a different behaviour than I have.

martinvw · 2017-05-01T18:28:10Z

.../src/main/java/org/openhab/binding/nikohomecontrol/handler/NikoHomeControlBridgeHandler.java

+     */
+    private void updateProperties() {
+
+        HashMap<String, String> properties = new HashMap<>();


Its best practice to use only the interface type for the declaration:

Map<String, String> properties = new HashMap<>();

martinvw · 2017-05-01T18:34:49Z

...java/org/openhab/binding/nikohomecontrol/internal/protocol/NikoHomeControlCommunication.java

+    private String lastEnergyErase = "";
+    private String lastConfig = "";
+
+    private final ArrayList<Integer> locations = new ArrayList<>();


Consider to add the interface types here, so List, Map

martinvw · 2017-05-01T18:36:40Z

...java/org/openhab/binding/nikohomecontrol/internal/protocol/NikoHomeControlCommunication.java

+        broadcastaddr = InetAddress.getByAddress(ipaddr);
+
+        DatagramSocket datagramSocket = null;
+        try {


You can use the try with resources on the DatagramSocket then you can get rid of the finally:
https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html

martinvw · 2017-05-01T18:37:53Z

...java/org/openhab/binding/nikohomecontrol/internal/protocol/NikoHomeControlCommunication.java

+            } else if ("listactions".equals(nhcCmdAck.cmd)) {
+                listActions(nhcCmdAck);
+            } else if ("listactions".equals(nhcCmdAck.event)) {
+                for (HashMap<String, String> action : nhcCmdAck.data) {


Use interface instead of type.

martinvw · 2017-05-01T18:39:31Z

...java/org/openhab/binding/nikohomecontrol/internal/protocol/NikoHomeControlCommunication.java

+            try {
+                nhcSocket.close();
+            } catch (IOException e) {
+            }


Most of the times its better to make explicit that you ignore an exception. For this location it makes sense to ignore, but better do that explicit. You could name the exception ignore and add a comment why you ignored it.

martinvw · 2017-05-01T18:40:58Z

features/openhab-addons/src/main/feature/feature.xml

@@ -200,6 +200,12 @@
        <bundle start-level="80">mvn:org.openhab.binding/org.openhab.binding.network/${project.version}</bundle>
    </feature>

+    <feature name="openhab-binding-nikohomecontrol" description="Niko Home Control Binding" version="${project.version}">
+        <feature>openhab-runtime-base</feature>
+        <feature>esh-model-script</feature>


Do you need this one? I see only one binding who has this?

I don't think so. Probably slipped in through copy/paste.

mherwege · 2017-05-02T17:33:39Z

@martinvw Thank you once again for the review. I have done the changes, but will need to test again when I get home. I will push when it all works and let you know then.

mherwege · 2017-05-03T07:34:35Z

@martinvw The updated code has now been pushed.

mherwege · 2017-05-03T07:59:25Z

@martinvw I didn't encounter any issue when running locally. All changes have been pushed. I did notice that my local maven check build suddenly gave 'missing eol at line 0' errors on all my files, while I did not touch most of them. It is building and running fine, so I ignored it for now. My local check profile is still v1, so pretty old by now I believe.

mherwege · 2017-05-11T14:21:01Z

@martinvw I have made some extensive changes to my code base. These changes implement a custom gson deserializer, split the largest class in multiple classes (hopefully making the code easier to maintain) and implement auto-discovery for the bridge. As the changes touch many parts of the code, I am not sure I what the best way to proceed is. Should I push it? It will probably require another detailed review. Or should I stick with what is aleady here an wait for further feedback?

martinvw · 2017-05-11T14:33:23Z

Please push it, I'll do another review :-)

mherwege · 2017-05-11T19:51:01Z

@martinvw Thank you for your patience and understanding. I have pushed the changes. Let me know what you think.

martinvw

Thanks for all the changes, a great improvement can you look at my remarks/questions?

martinvw · 2017-05-15T15:53:43Z

addons/binding/org.openhab.binding.nikohomecontrol/ESH-INF/thing/thing-types.xml

+				<default>8000</default>
+				<advanced>true</advanced>
+			</parameter>
+            <parameter name="BROADCASTADDR" type="text" required="false">


XML files should be indented using tabs according to our code formatter, the github view suggests that it is a mix now.

martinvw · 2017-05-15T15:54:03Z

addons/binding/org.openhab.binding.nikohomecontrol/ESH-INF/thing/thing-types.xml

+			<parameter name="ADDR" type="text" required="false">
+				<label>IP or Host Name</label>
+				<description>Address will be resolved on connection if not configured</description>
+				<advanced>true</advanced>


Is every config advanced?

Yes, as these are all optional in a typical home network and when running openHab on a machine connected to only one network segment. The documentation explains when these parameters are required. E.g. I don't need these when running openHab from my laptop, but I do when running from my Pi that doubles as a VPN server. On the Pi, discovery wrongly picks the VPN network interface to look for a broadcast address. There is no way to know what network to use unless the user configures one or more of these advanced parameters.

This sounds as if you are heavily mixing discovery with configuration here.
The configuration should really enable the handler to reach out to the (correct!) device immediately - so I would clearly expect the IP address being configured here. Otherwise you will be in big trouble if there ARE multiple gateways on the same network (which you should never rule out).
The job of the discovery service is to find devices on the network and provide their IP addresses for the thing configuration; note that they can even update existing Things in case their IP addresses change (this is e.g. how all the UPnP-discovered devices are handled).

@kaikreuzer I am afraid I will need more help here. I can move the broadcastAddr parameter to the binding. But I don't know how to access it from the bridge discovery service then. From what I have seen, I can easily access it from the handler factory by overriding the activate method and adding componentContext.getProperties(). But that does not seem to help me with the bridge discovery.
I agree I would be in trouble if there are 2 gateways on the local network. However, I think this is highly unlikely. I even suspect the whole Niko system would not work if that was the case. However, I did run into the issue I have 2 separate network interfaces on my openHAB system, therefore making it impossible to reliably find the proper broadcast address automatically. I need to provide it. Once I have that, it is stable. The specific IP address is not stable though and my router does not allow me to fix it. So I cannot provide a fixed IP.
I am open to any guidance on how to do this better and indeed move the broadcast parameter to the binding level. Would that mean I have to register the discovery service from the handler factory, rather than through the declaration?

That's a very valid question, I didn't think of that. In general, it would be possible to directly access the OSGi Configuration Admin and get the configuration for the binding out of it in the DiscoveryService code - but that isn't really nice.
Actually, there are quite some other bindings that have the requirement to figure out the correct network interface under which the openHAB instance is accessible. Would it make sense to have the user configure it and make the value available through a new method in NetUtil?
Another option is to do it similarly to UPnP and mDNS: Simply do the broadcast on ALL available interfaces :-)

martinvw · 2017-05-15T15:54:43Z

addons/binding/org.openhab.binding.nikohomecontrol/OSGI-INF/NikoHomeControlBridgeDiscovery.xml

+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+
+    Copyright (c) 2014-2017 by the respective copyright holders.


It seems that this file has a non-default copyright header, can you fix all of them by running

mvn license:format

Please commit only the ones for your binding.

martinvw · 2017-05-15T15:55:38Z

...ntrol/src/main/java/org/openhab/binding/nikohomecontrol/NikoHomeControlBindingConstants.java

+ *
+ * @author Mark Herwege
+ */
+


Remove empty line

martinvw · 2017-05-15T15:56:15Z

.../src/main/java/org/openhab/binding/nikohomecontrol/handler/NikoHomeControlBridgeHandler.java

+        if (config.get(CONFIG_HOST_NAME) != null) {
+            try {
+                // If hostname or address was provided in the configuration, try to use this to for bridge and give
+                // error


the line breaks became a little bit funny here

Too many edits. I cleaned it up.

martinvw · 2017-05-20T18:48:08Z

...java/org/openhab/binding/nikohomecontrol/internal/protocol/NikoHomeControlCommunication.java

+
+        for (HashMap<String, String> action : data) {
+
+            int id = Integer.valueOf(action.get("id"));


You should use valueOf if you want an integer if you want an int call parseInt

Thank you for the info. I wasn't aware of that.

martinvw · 2017-05-20T18:49:03Z

...java/org/openhab/binding/nikohomecontrol/internal/protocol/NikoHomeControlCommunication.java

+            logger.debug("Niko Home Control: event execute action {} with state {}", id, state);
+            try {
+                this.actions.get(id).setState(state);
+            } catch (Exception e) {


do you really need to catch the Exception-class or are there more specific exceptions you could catch like the IOException or maybe only RuntimeException's (which is not that good either). The current code and also catching RuntimeException's might 'hide' programming errors like NullPointerExceptions occurring.

https://www.google.nl/search?q=java%20why%20not%20to%20catch%20exception

No, I don't need to. Something is very wrong when I would get here. It would be a programming error. So I removed the try/catch.

martinvw · 2017-05-20T18:49:59Z

...rg/openhab/binding/nikohomecontrol/internal/protocol/NikoHomeControlMessageDeserializer.java

+
+                    message = new NhcMessageListMap();
+
+                    List<HashMap<String, String>> dataList = new ArrayList<>();


martinvw · 2017-05-20T18:50:50Z

...rg/openhab/binding/nikohomecontrol/internal/protocol/NikoHomeControlMessageDeserializer.java

+                        for (Entry<String, JsonElement> entry : jsonDataObject.entrySet()) {
+                            data.put(entry.getKey(), entry.getValue().getAsString());
+                        }
+                        dataList.add((HashMap<String, String>) data);


If you define dataList correct you don't need this cast

martinvw · 2017-05-20T18:51:22Z

...rg/openhab/binding/nikohomecontrol/internal/protocol/NikoHomeControlMessageDeserializer.java

+
+            return message;
+
+        } catch (IllegalStateException | ClassCastException e) {


Why/when do we get these?

I don't want the binding to completely fail when what I get back from the controller is different from what I expect. Debug logging will show me exactly what came back and will tell me if it cannot be interpreted. IllegalStateException can be thrown from the nested getAsJsonObject() calls when the inner element would be a json array or primitive, rather then an object. Similarly ClassCastException would occur if the getAsString() call is not on a primite, but an object or array.
A number of messages I get from the controller, but do not interpret at the moment, have formats that cannot be deserialized with the current logic yet. The nested logic would have to be refined. Also, I am not 100% sure all versions of the controller software use exactly the same json formats. This also shields from that. The debug log will show an 'Unsupported json type' message.
I learned the structure from this: http://www.javacreed.com/gson-deserialiser-example/. As the nesting can be complex and unpreditable, I only went with what I know for know, and will extend this part if I implement extra functionality.

mherwege · 2017-05-20T21:34:01Z

@martinvw Thank you very much for the review. I think I made all changes. I have added some questions and remarks in the review. Could you have a look? I will already push what I have done.

kaikreuzer · 2017-06-07T15:09:52Z

My understanding can now be totally wrong, but I don't think it will currently work like described above. And I don't see how it could be the binding's responsibility to take care of this logic. While it might be possible, I think it is a more general problem that should work at a level above the bindings.

You've got it EXACTLY! That was exactly the discussion I had around wall buttons in KNX. And our conclusion was that - although we actually need to react on state updates - we will deprecate the handleUpdate() method and instead introduce some mechanism on the channel link through which the user can define that the device should behave like a "slave", i.e. the state is actually managed somewhere else. Setting this flag would make the framework convert state updates to commands that are then passed to the handleCommand() method - which would then tell NHC that this action is turned on. So I would hope that this idea would be applicable in a similar way for KNX and NHC (and other systems in future).

The activity is represented in openHAB as a thing.

This is something I actually missed in the review: The correct granularity for such an activity would actually be a channel, not a thing. Having tens of Things with a single channel does not make much sense. For KNX we decided to have a basic thing with no fixed set of channels. This can then be either "discovered and populated with channels" or manually defined in a *.things file. Using the manual configuration, users would also be free to define multiple things for structuring their channels in logical groups (in KNX e.g. having a Thing for an 8-blinds actuator with 8 channels). Would that also make sense for NHC in order to reduce the number of Things?

mherwege · 2017-06-07T15:55:30Z

@kaikreuzer I am not sure yet the slave concept will solve it completely in my case. The issue is that if you define the NHC device as the slave, what will you do with updates coming from it? The NHC device and its defined 'activity' are both an input, an output and a state. The communication is always 2-way.

push an NHC button -> changes activity state in NHC -> sends update to openHAB -> Hue does need to get a command
use Hue app to switch Hue -> sends update to openHAB -> NHC needs to get a command to change activity state in NHC -> NHC will send an update to openHAB
use an openHAB UI to change the state of the Hue bulb -> NHC and Hue get command -> NHC will send update to openHAB

Actuators (inputs) and outputs are not separate in NHC at the level I am integrating. So it should act as a slave at one point in time, and as a master at another.

The activity is represented in openHAB as a thing.
For the common case, an activity represents a light, or other output. This, to me, is most natural to be represented as a thing. I could imagine that certain of these things have additional channels beyond what I have right now. The alternative is to have no bridge, 1 thing (representing the gateway) and x channels representing each one light. To me, that looks much less natural than what I have now, but I am open for discussion on this. The exception cases are where I have a complete sphere as an output represented as one thing, or where my output is an all-off function. But even there, representing as a thing feels more natural to me. Also, these things have locations I, by default, copy from the NHC system setup, where I also have a location concept.
So would like to keep the current structure if OK with you.

Signed-off-by: MHERWEGE <[email protected]>

kaikreuzer · 2017-06-08T12:38:09Z

what will you do with updates coming from it?

Discard them. In slave mode, updates are not passed on to the items, only commands are. So yes, you will need to find some way for the user to configure that a certain channel is merely a button, which should send commands, so that in such a case your handler sends commands and not state updates.

representing as a thing feels more natural to me. Also, these things have locations

Be careful not to mix up the item/functional and thing/physical levels here (see this explanation of "things" in openHAB. All what you describe make sense on an item level: Every light, shutter, etc. is naturally one ITEM (let's call if object). Each of this has a location. But the "Thing" (actuator) can have a different location and combine multiple channels (->items). For NHC, these are your things and their location is very likely the electrical cabinet. So the situation is very similar to KNX actually.

mherwege · 2017-06-08T14:30:25Z

@kaikreuzer

So yes, you will need to find some way for the user to configure that a certain channel is merely a button, which should send commands, so that in such a case your handler sends commands and not state updates.

But then I lose all possibility of having the LED work, as I won't have a correct state in the NHC system anymore. If the state is changed outside of NHC, it still needs to get a command to update its state. It will always reply with an update message that is indistinguishable from a message sent when the button was pressed on the NHC side. So the slave mechanism will not resolve the issue for the NHC binding.

For NHC, these are your things and their location is very likely the electrical cabinet. So the situation is very similar to KNX actually.

I do understand the background to this and I do appreciate your advice on this. I just don't think this is a black or white situation.

I based my original decision on how to structure this on the smarthome doc FAQ (question 1) and on the Hue binding which, I believe, also uses a bridge and thing model where things represent individual bulbs.
I don't agree fully with your statement on the similarity with KNX, as the only thing that is visible is a logical representation through the IP interface in the controller (and even then limited to what is exposed in the NHC system to the IP interface). That interface works through actions that have inputs, outputs and state. I cannot map on a lower level, because there is no interface to talk to at that lower level. None of the other components can be distinguished in the communication with the controller.
Also, the communication with the controller allows me to do discovery of things (actions in NHC) if I model as a bridge. If the setup in NHC changes, discovery would easily allow me to update. I know channels could be dynamic, but that would mean kind of writing my own discovery mechanism to update the channel list if setup changes on the NHC side.
For the communication back with the system, I need at least one property (the action Id in the controller), which I can easily store in a thing property. Extra properties I read from the controller (name, location, type) make it very transparent to a user. So far, I have not seen how to make that easily visible to the user when this would be channels. A channel number or name mapped to an NHC action Id is pretty meaningless, as these are nowhere visible in any of the standard NHC software, so the user would not know what it is and would not be able to map it to what it physically represents.

I do believe this is certainly a boundary situation that could go either way. I don't feel comfortable changing it at this time for the reasons listed above.

mherwege · 2017-06-08T14:40:37Z

@kaikreuzer I am looking into bringing the broadcastAddr parameter on the binding level, rather than bridge level. This indeed is a more logical structure. I have a few questions on it though:

I need this parameter in the bridge discovery then. Is my assumption correct that I need to register the bridge discovery service from the handler factory instead of defining it in the osgi xml for this to work?
When doing this I am running into the same problem as issue [AllPlay] Defined configuration values are ignored by the binding #2335. I don't see the property being returned. Any guidance on this?

Thank you for your guidance and patience.

mherwege · 2017-06-08T21:44:31Z

@kaikreuzer I have done the easy changes so far.
I am currently working on improving the discovery with broadcast mechanism. It looks like I can indeed broadcast to all interfaces without issue. So I am removing the broadcast address search from the code. I will still move the method to org.eclipse.smarthome.core.net.NetUtil once I am through with changes in this binding, but will need more time for that.

kaikreuzer · 2017-06-09T10:59:54Z

the only thing that is visible is a logical representation

This clearly shows that the correct modelling would be a single thing with many channels, which are the exposed actions. This is how the ESH concepts are meant to be used. I understand your reasoning, though, as there isn't really nice support for dynamic channels (yet) in the UIs (but actually for textual configuration it would be easier to go that way). So let's leave it as is, but please keep in mind that it is rather considered as a workaround than as an example of how to ideally model things. :-)

It looks like I can indeed broadcast to all interfaces without issue.

Cool, that's than definitely the easiest way to move forward!

Removed broadcastaddr parameter, used bridgediscovery to find IP address and use background discovery service to update if required. Don't change or discover IP address in bridge handler. Signed-off-by: MHERWEGE <[email protected]>

mherwege · 2017-06-12T16:07:24Z

@kaikreuzer I believe I have now done all requested changes. Please let me know if you have any further feedback.

Signed-off-by: MHERWEGE <[email protected]>

kaikreuzer · 2017-06-14T16:36:50Z

Cool, thank you so much @mherwege and congrats to have it finally merged 👍

* 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)

* Initial contribution of Niko Home Control binding. Signed-off-by: Mark Herwege <[email protected]>

Signed-off-by: Schrotti <[email protected]>

kaikreuzer added the new binding If someone has started to work on a binding. For a new binding PR. label Dec 25, 2016

mherwege mentioned this pull request Jan 5, 2017

Child things missing handler when removing bridge eclipse-archived/smarthome#2757

Closed

kubawolanin requested changes Feb 15, 2017

View reviewed changes

martinvw self-assigned this Apr 19, 2017

martinvw self-requested a review April 22, 2017 18:57

martinvw requested changes Apr 23, 2017

View reviewed changes

martinvw reviewed May 1, 2017

View reviewed changes

martinvw removed their assignment May 1, 2017

martinvw reviewed May 20, 2017

View reviewed changes

martinvw requested a review from kaikreuzer May 20, 2017 21:40

martinvw approved these changes May 20, 2017

View reviewed changes

First corrections based on review from Kai.

19eb8cb

Signed-off-by: MHERWEGE <[email protected]>

Separated discovery from bridge parameters.

56fab19

Removed broadcastaddr parameter, used bridgediscovery to find IP address and use background discovery service to update if required. Don't change or discover IP address in bridge handler. Signed-off-by: MHERWEGE <[email protected]>

Fix for old listener not stopping when restarting communication.

20fa330

Signed-off-by: MHERWEGE <[email protected]>

kaikreuzer removed the awaiting feedback Awaiting feedback from the pull request author label Jun 14, 2017

kaikreuzer approved these changes Jun 14, 2017

View reviewed changes

kaikreuzer merged commit 1da019f into openhab:master Jun 14, 2017

kaikreuzer modified the milestone: 2.1 Jun 24, 2017

mherwege mentioned this pull request Jun 26, 2017

Added methods to find broadcastAddress(es), useful in bindings for discovery. eclipse-archived/smarthome#3747

Merged

falkena pushed a commit to falkena/openhab-addons that referenced this pull request Jun 29, 2017

Initial contribution of a Niko Home Control binding. (openhab#1589)

125934a

* Initial contribution of Niko Home Control binding. Signed-off-by: Mark Herwege <[email protected]>

qvistgaard pushed a commit to qvistgaard/openhab2-addons that referenced this pull request Jun 30, 2017

Initial contribution of a Niko Home Control binding. (openhab#1589)

1e4a50d

* Initial contribution of Niko Home Control binding. Signed-off-by: Mark Herwege <[email protected]>

jsjames pushed a commit to jsjames/openhab-addons that referenced this pull request Jul 1, 2017

Initial contribution of a Niko Home Control binding. (openhab#1589)

793c8eb

* Initial contribution of Niko Home Control binding. Signed-off-by: Mark Herwege <[email protected]>

reyem pushed a commit to reyem/openhab2-addons that referenced this pull request Jul 1, 2017

Initial contribution of a Niko Home Control binding. (openhab#1589)

0a3161c

* Initial contribution of Niko Home Control binding. Signed-off-by: Mark Herwege <[email protected]>

Markinus pushed a commit to Markinus/openhab2-addons that referenced this pull request Jul 2, 2017

Initial contribution of a Niko Home Control binding. (openhab#1589)

5fb12c9

* Initial contribution of Niko Home Control binding. Signed-off-by: Mark Herwege <[email protected]>

ppieczul pushed a commit to ppieczul/openhab2-addons that referenced this pull request Jul 2, 2017

Initial contribution of a Niko Home Control binding. (openhab#1589)

01bd4f1

* Initial contribution of Niko Home Control binding. Signed-off-by: Mark Herwege <[email protected]>

reyem pushed a commit to reyem/openhab2-addons that referenced this pull request Jul 3, 2017

Initial contribution of a Niko Home Control binding. (openhab#1589)

d35484c

* Initial contribution of Niko Home Control binding. Signed-off-by: Mark Herwege <[email protected]>

aogorek pushed a commit to aogorek/openhab2-addons that referenced this pull request Jul 5, 2017

Initial contribution of a Niko Home Control binding. (openhab#1589)

86b1180

* Initial contribution of Niko Home Control binding. Signed-off-by: Mark Herwege <[email protected]>

Markinus pushed a commit to Markinus/openhab2-addons that referenced this pull request Sep 8, 2017

Initial contribution of a Niko Home Control binding. (openhab#1589)

9a58df6

* Initial contribution of Niko Home Control binding. Signed-off-by: Mark Herwege <[email protected]>

hillmanr pushed a commit to hillmanr/openhab2-addons-pollytts that referenced this pull request Dec 6, 2017

Initial contribution of a Niko Home Control binding. (openhab#1589)

65d9d2f

* Initial contribution of Niko Home Control binding. Signed-off-by: Mark Herwege <[email protected]>

markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Aug 12, 2023

Change OH versions from 2.5.0 to 3.1.0 in doc files (openhab#1589)

3f4a263

Signed-off-by: Schrotti <[email protected]>

		@@ -0,0 +1,79 @@
		# Niko Home Control Binding
		The openHAB2 Niko Home Control binding integrates with a [Niko Home Control](http://www.nikohomecontrol.be/) system through a Niko Home Control IP-interface.


		private Logger logger = LoggerFactory.getLogger(NikoHomeControlBridgeHandler.class);

		private NikoHomeControlCommunication nhcComm = null;


		for (HashMap<String, String> action : data) {

		int id = Integer.valueOf(action.get("id"));


		message = new NhcMessageListMap();

		List<HashMap<String, String>> dataList = new ArrayList<>();


		return message;

		} catch (IllegalStateException \| ClassCastException e) {

Initial contribution of a Niko Home Control binding. #1589

Initial contribution of a Niko Home Control binding. #1589

Conversation

mherwege commented Dec 19, 2016

kubawolanin left a 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

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

mherwege commented Feb 15, 2017

ThomDietrich commented Mar 22, 2017

mherwege commented Mar 23, 2017

ThomDietrich commented Mar 23, 2017 • edited Loading

martinvw left a 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

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

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

Choose a reason for hiding this comment

mherwege commented Apr 25, 2017

martinvw commented Apr 26, 2017

martinvw commented Apr 26, 2017

kaikreuzer commented Apr 27, 2017

mherwege commented Apr 27, 2017

kaikreuzer commented Apr 28, 2017

mherwege commented Apr 29, 2017

mherwege commented Apr 29, 2017

martinvw left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinvw May 1, 2017 • edited Loading

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

mherwege commented May 2, 2017

mherwege commented May 3, 2017

mherwege commented May 3, 2017

mherwege commented May 11, 2017

martinvw commented May 11, 2017

mherwege commented May 11, 2017

martinvw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomDietrich commented Mar 23, 2017 •

edited

Loading

martinvw May 1, 2017 •

edited

Loading

mherwege Jun 7, 2017 •

edited

Loading