[vera] Vera Binding initial contribution #2283

MakeSimpleOrg · 2017-05-19T10:59:11Z

No description provided.

lolodomo · 2017-05-21T05:29:24Z

@mrguessed : as the author of the 1.x binding, could you please review and comment on this new binding for OH2 ?

MakeSimpleOrg · 2017-05-21T07:25:28Z

The main purpose was to make binding that do not require settings. Controller and devices are added in 2 clicks.
Everything is implemented, except thermostats and colored lamps. I do not have these devices for the test.
Based on Z-Way binding, but more simple.

How to correctly sign-off the first commit?

martinvw · 2017-05-21T08:34:48Z

If you squash both commits you can also change the message, for squashing / rebasing see:
http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html

martinvw · 2017-05-21T08:35:51Z

Thanks for your PR!

Please include the build in the parent pom.xml in that case it will already be tested and analysed by travis.

Please also check:

http://docs.openhab.org/developers/development/bindings.html
http://docs.openhab.org/developers/development/guidelines.html

MakeSimpleOrg · 2017-05-26T03:02:36Z

Something else? :)

martinvw

Thanks for this binding, I think a lot people could profit from it.

I added some review remarks please take a look at them.

martinvw · 2017-05-26T04:55:13Z

addons/binding/org.openhab.binding.vera2/ESH-INF/binding/binding.xml

+
+    <name>Vera Binding</name>
+    <description>
+       <![CDATA[


Can you add a useful description and do not add in a CDATA if there is nothing to escape.

martinvw · 2017-05-26T04:55:49Z

addons/binding/org.openhab.binding.vera2/ESH-INF/config/bridge-config.xml

+		<parameter-group name="veraController">
+            <label>Vera controller</label>
+            <description>The configuration of the Vera. All the information can be detected during the discovery.</description>
+        </parameter-group>


XML files should be indented using tabs according to our code formatter.

Please format using the ESH profile in eclipse

martinvw · 2017-05-26T04:56:32Z

addons/binding/org.openhab.binding.vera2/ESH-INF/config/bridge-config.xml

+		    <context>network-address</context>
+		    <label>IP address</label>
+		    <description>The IP address or hostname of the Vera controller.</description>
+		    <default>localhost</default>


This does not feel like a sane default or can you run the vera controller as software I thought it was some box.

martinvw · 2017-05-26T04:59:06Z

addons/binding/org.openhab.binding.vera2/ESH-INF/i18n/vera2.properties

@@ -0,0 +1,78 @@
+# binding
+binding.vera2.name = Vera Binding
+binding.vera2.description = description


Put the English texts just in place, use I18N for non-english only
@ThomDietrich @kaikreuzer @SJKA does this assumption comply with what others think about this subject?

martinvw · 2017-05-26T05:04:06Z

addons/binding/org.openhab.binding.vera2/ESH-INF/thing/bridge.xml

+		<label>Vera Controller</label>
+		<description>
+            <![CDATA[
+            The Vera Controller


Please only escape if needed, or is there another reason you did this?

martinvw · 2017-05-26T21:21:00Z

...2/src/main/java/org/openhab/binding/vera2/internal/discovery/VeraBridgeDiscoveryService.java

+                    ThingUID thingUID = new ThingUID(VeraBindingConstants.THING_TYPE_BRIDGE,
+                            ipAddress.replaceAll("\\.", "_"));
+
+                    // Attention: if is already present as thing in the ThingRegistry


You can avoid this by an additional interface ExtendedDiscoveryService just search for it.

martinvw · 2017-05-26T21:21:10Z

...2/src/main/java/org/openhab/binding/vera2/internal/discovery/VeraBridgeDiscoveryService.java

+                            .withLabel("Vera controller " + ipAddress).build();
+                    thingDiscovered(discoveryResult);
+                }
+            } 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

martinvw · 2017-05-26T21:21:41Z

...2/src/main/java/org/openhab/binding/vera2/internal/discovery/VeraDeviceDiscoveryService.java

+
+        List<Device> deviceList = mBridgeHandler.getData().devices;
+        for (Device device : deviceList) {
+            if (device.category.equals("0")) {


Position literals first in String comparisons - that way if the String is null you won't get a NullPointerException, it'll just return false. source: PMD

martinvw · 2017-05-26T21:22:13Z

...2/src/main/java/org/openhab/binding/vera2/internal/discovery/VeraDeviceDiscoveryService.java

+            }
+            ThingUID thingUID = new ThingUID(THING_TYPE_DEVICE, mBridgeHandler.getThing().getUID(), device.id);
+            DiscoveryResult discoveryResult = DiscoveryResultBuilder.create(thingUID).withLabel(device.uName)
+                    .withBridge(bridgeUID).withProperty(DEVICE_CONFIG_ID, device.id)


allign them such that the . get at a newline with one call per line.

formatter do that

martinvw · 2017-05-26T21:22:39Z

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

@@ -0,0 +1,19 @@
+<?xml version="1.0" encoding="UTF-8"?>


It is missing from the parent pom.xml

To have the binding being picked up by the distro, you furthermore need to add it to the feature.xml, again at the alphabetically correct position. If you have a dependency on some transport bundle (e.g. upnp, mdns or serial), make sure to add a line for this dependency as well (see the other bindings as an example).

MakeSimpleOrg · 2017-05-27T06:57:33Z

Great work @martinvw. I would not be able to find so many mistakes in someone else's code.
I'm glad that you did not see the first version of the code :)

I'll do everything as I find the time.

martinvw · 2017-05-30T17:52:08Z

I'll do everything as I find the time.

Thanks for the first changes, just give a shout if you are ready.

MakeSimpleOrg · 2017-05-31T18:15:55Z

I want to make 2 another bindings for Fibaro and Zipato controllers. They have a very similar api. No difference in binding code, only in api part. All devices for these controllers are the same.
Maybe, i need make a one binding for all?

MakeSimpleOrg · 2017-06-01T02:21:02Z

Only README, rename and TODO remains only.
Maybe I'll write readme tomorrow. But can't implement any TODO items now, maybe other people can do that later.

MakeSimpleOrg · 2017-06-03T00:43:29Z

@martinvw i am ready :)

Give Kai a clean slate

Signed-off-by: Dmitriy Ponomarev <diamond5170@gmail.com>

include the build in the parent project add copyrights Signed-off-by: Dmitriy Ponomarev <diamond5170@gmail.com>

Add to pom and feature. Fix equals. Fix typo. Change replaceAll to regexp. Remove cdata. Fix default ip. Signed-off-by: Dmitriy Ponomarev <diamond5170@gmail.com>

enum for CategoryType SerializedName for gson format for all xml files remove unused imports from manifest.mf getters, setters, private fields reuse gson handle config change for bridge remove unused code and cleanup Signed-off-by: Dmitriy Ponomarev <diamond5170@gmail.com>

Unnecessary code Signed-off-by: Dmitriy Ponomarev <diamond5170@gmail.com>

Descriptions. Cleanup unused channels and code. Remove i18n. Signed-off-by: Dmitriy Ponomarev <diamond5170@gmail.com>

Signed-off-by: Dmitriy Ponomarev <diamond5170@gmail.com>

New parameters: clearNames and defaultRoomName. Remove unused sensorTamper channel. Remove polling for devices and scenes, all work is performed by the controller polling. Refresh name, location and states during polling, after config change, or after manually refresh. Signed-off-by: Dmitriy Ponomarev <diamond5170@gmail.com>

Signed-off-by: Dmitriy Ponomarev <diamond5170@gmail.com>

MakeSimpleOrg · 2017-06-10T19:46:48Z

Soo looong :)

While waiting, I made a new binding for the Fibaro.
How do I add it? If i push it, he will be added here, which is not good.

martinvw · 2017-06-10T19:59:43Z

Soo looong :)

You should try to read (or better review) all the code which is currently pending as PR here ;-) That should be over 500K lines of code :-D

While waiting, I made a new binding for the Fibaro.
How do I add it? If i push it, he will be added here, which is not good.

You should use feature branches for that, with the term you should be able to find more info online.

MakeSimpleOrg · 2018-01-12T09:39:15Z

Very fast approval ;)

kaikreuzer · 2018-08-07T15:37:51Z

@diamond5170 Very sorry, this somehow completely fell of my plate - I know, there's no excuse :-/
Before spending the time to do a final review, I just like to check that you are still around and still waiting for it to be merged. Just give me a "peep" and I'll do the review within hours, promised!

wborn

Thanks for your awesome work on the new Vera Binding @diamond5170! The code looks good. 👍

I've added my comments below. Quite a few are due to the passing of time and the binding still not being merged.

wborn · 2019-01-30T18:28:21Z

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

+Bundle-Name: Vera Binding
+Bundle-SymbolicName: org.openhab.binding.vera;singleton:=true
+Bundle-Vendor: openHAB
+Bundle-Version: 2.1.0.qualifier


Suggested change

Bundle-Version: 2.1.0.qualifier

Bundle-Version: 2.5.0.qualifier

wborn · 2019-01-30T18:30:46Z

addons/binding/org.openhab.binding.vera/OSGI-INF/VeraBridgeDiscovery.xml

+    http://www.eclipse.org/legal/epl-v10.html
+
+-->
+<scr:component xmlns:scr="http://www.osgi.org/xmlns/scr/v1.1.0" immediate="true"


Please use DS annotations instead of adding these XML files:

Remove this file

Add a .gitignore file to this directory ignoring all XML file i.e.: *.xml

Annotate VeraBridgeDiscoveryService with:

@Component(service = DiscoveryService.class, immediate = true, configurationPid = "discovery.vera")

wborn · 2019-01-30T18:31:38Z

addons/binding/org.openhab.binding.vera/OSGI-INF/VeraHandlerFactory.xml

+    http://www.eclipse.org/legal/epl-v10.html
+
+-->
+<scr:component xmlns:scr="http://www.osgi.org/xmlns/scr/v1.1.0" immediate="true"


Please use DS annotations instead of adding these XML files:

Remove this file

Add a .gitignore file to this directory ignoring all XML file i.e.: *.xml

Annotate VeraHandlerFactory with:

@Component(service = ThingHandlerFactory.class, configurationPid = "binding.vera")

wborn · 2019-01-30T18:32:34Z

addons/binding/org.openhab.binding.vera/about.html

+<h3>License</h3>
+
+<p>
+    The openHAB community makes available all content in this plug-in (&quot;Content&quot;). Unless otherwise


Please replace this with a NOTICE file. See also this community topic.

wborn · 2019-01-30T18:32:54Z

addons/binding/org.openhab.binding.vera/build.properties

+               .,\
+               OSGI-INF/,\
+               ESH-INF/,\
+               about.html


So this becomes:

Suggested change

about.html

NOTICE

wborn · 2019-01-30T19:18:21Z

...rg.openhab.binding.vera/src/main/java/org/openhab/binding/vera/handler/VeraSceneHandler.java

+                        updateState(channelUID, OnOffType.OFF);
+                    }
+                } else {
+                    logger.warn("Unknown command type: {}, {}, {}, {}", command, sceneId);


The number of parameters does not match the number of {} placeholders.

wborn · 2019-01-30T19:18:44Z

...rg.openhab.binding.vera/src/main/java/org/openhab/binding/vera/handler/VeraSceneHandler.java

+        if (scene != null) {
+            logger.debug("Add scene as channel: {}", scene.getName());
+
+            HashMap<String, String> properties = new HashMap<>();


Suggested change

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

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

wborn · 2019-01-30T19:18:57Z

...rg.openhab.binding.vera/src/main/java/org/openhab/binding/vera/handler/VeraSceneHandler.java

+    }
+
+    private synchronized void addChannel(String id, String acceptedItemType, String label,
+            HashMap<String, String> properties) {


Suggested change

HashMap<String, String> properties) {

Map<String, String> properties) {

wborn · 2019-01-30T19:20:11Z

...vera/src/main/java/org/openhab/binding/vera/internal/converter/VeraDeviceStateConverter.java

+                        return getBinaryState(device.getTripped());
+                }
+            case HVAC: // TODO
+                logger.warn("TODO: {}, {}", device, device.getCategoryType());


Can you change it to debug instead? I wouldn't like a log file full of warnings just because devices aren't yet supported by a binding. It would also help to make a note about this in the docs.

wborn · 2019-01-30T19:20:19Z

...vera/src/main/java/org/openhab/binding/vera/internal/converter/VeraDeviceStateConverter.java

+            case Weather:
+            case PhilipsController:
+            case Appliance:
+                logger.warn("TODO: {}, {}", device, device.getCategoryType());


Can you change it to debug instead? I wouldn't like a log file full of warnings just because devices aren't yet supported by a binding. It would also help to make a note about this in the docs.

martinvw · 2019-03-04T20:26:12Z

Hello @diamond5170,

Thanks for your contribution and sorry to keep you waiting. Because we are better staffed now we hope to wrap up a bunch of our old pending pull requests.

Our first important question is, are you still interested in finishing your binding and if so, can we get >your commitment to process the review comments in approximately 2-4 weeks.

What do we commit to, we will review your binding in the upcoming 1-2 week(s) and with a quick response from your side the binding should be merged in 2-4 weeks.

So the main question, are you able to commit to processing review comments of this PR in 2-4 weeks:

Yes, I’m going to process the comments in the next weeks.
Yes, but I’d like to get some help, (for example with my git branch and/or changes that have been made to openHAB that affect my contribution)
No, I have no time in the coming weeks. But please review it I will pick it up within 3 months.
No, I have other priorities or are not able to finish the work. I would welcome someone to takeover this PR (and I thus confirm the origin of my work )
No, this pr can be closed for reason: … (please specify. For example the service it connects to has been discontinued).

If you did not yet publish your binding on the marketplace and or announce it on the community forum that would be great thing to do as well:

See Distributing bindings through the IoT Marketplace

Thanks for your support,

Best regards,

The openhab2-maintainers

@wborn
@cweitkamp
@Hilbrand
@kaikreuzer
@martinvw

MakeSimpleOrg · 2019-03-13T09:36:12Z

I do not want to waste time.

davidgraeff · 2019-03-27T21:41:20Z

I'm unsure about the state of this PR. Are the review comments being incorporated so that we can merge?

kaikreuzer

Oh hell - I just noticed here that I had some pending review comments (since more than a year). Afair, I was waiting for a peep, which never came :-( .

Are the review comments being incorporated so that we can merge?

Afaics, only @martinvw's comments were addressed, so in order to merge this PR, there's still something to do.
As @diamond5170 seems to have lost interest in pursuing this further, is there anybody else using Vera and interested to get this job finished?

kaikreuzer · 2018-08-08T06:15:51Z

addons/binding/org.openhab.binding.vera/ESH-INF/config/bridge-config.xml

+			<context>network-address</context>
+			<label>IP address</label>
+			<description>The IP address or hostname of the Vera controller.</description>
+			<default>192.168.1.10</default>


remove this and rather add required=true instead.

kaikreuzer · 2018-08-08T06:18:12Z

addons/binding/org.openhab.binding.vera/ESH-INF/config/bridge-config.xml

+        http://eclipse.org/smarthome/schemas/config-description-1.0.0.xsd">
+	<config-description uri="binding:vera:veraController">
+
+		<parameter-group name="veraController">


I'd suggest to remove this group as you do not have that many parameters.

kaikreuzer · 2018-08-08T06:43:38Z

addons/binding/org.openhab.binding.vera/ESH-INF/thing/channels.xml

+		<state readOnly="false" />
+	</channel-type>
+
+	<channel-type id="battery">


You can remove this and rather use the existing system.battery-level type (see https://www.eclipse.org/smarthome/documentation/development/bindings/thing-definition.html#channels)

kaikreuzer · 2018-08-08T06:44:13Z

addons/binding/org.openhab.binding.vera/ESH-INF/thing/channels.xml

+	   <channel-type id="doorlock">
+        <item-type>Switch</item-type>
+        <label>Doorlock</label>
+        <category>Door</category>


Remove this line, it isn't a door, it is a lock :-)

kaikreuzer · 2018-08-08T06:44:29Z

addons/binding/org.openhab.binding.vera/ESH-INF/thing/channels.xml

+	</channel-type>
+
+	   <channel-type id="doorlock">
+        <item-type>Switch</item-type>


please re-apply the auto-formatter

kaikreuzer · 2018-08-08T06:52:02Z