Feature: DMX binding #2704

J-N-K · 2016-12-26T19:00:49Z

This binding ist still between alpha- and beta state. It runs fine at my home but needs further real-life testing, This especially true for all Bridges besides the sACN/E1.31 bridge.

Since these 5k loc will probably take a lot of work to review, I created this PR so the review process can start. After reviewing is finished and all requests incorporated I'll squash and add proper sign-off.

J-N-K · 2017-02-12T15:38:59Z

I think nothing has changed for quite some time and several people report successfull operation, so this should be ready for review.

maggu2810 · 2017-02-13T14:54:19Z

Just a side not: extensions/binding/org.eclipse.smarthome.binding.hue/build.properties shouldn't be part of that PR.
Will try to find some time to review...

Two question about the JavaDoc style (IIRC there is currently no format specified so I only like to ask for the reason) 😉

Is there any reason for the empty line between @param and @return
Is there any reason for the line break after the parameter name and the parameter description

J-N-K · 2017-02-13T19:55:32Z

I removed the file but unfortunately forgot to sign-off. I'll fix that after review. Regarding the JavaDoc: I only adapted a style found in other bindings. There is no special reason for that.

J-N-K · 2017-06-28T18:42:17Z

Any news on this one?

triller-telekom · 2017-07-06T14:28:23Z

@J-N-K Reading your post we are sorry to hear that. Though I will have a look on this PR tomorrow so you get some feedback.

Since this PR consists of >5000loc this isn't the first choice if you start to review PRs, so that might be the reason why it was a bit neglected in the past. This doesn't mean we are not interested in this binding or that we do not value your work! We really appreciate contributions to ESH!

triller-telekom

Thank you for the new binding and sorry again that we did not review it earlier.

Before I started my review I was wondering what DMX is and who is the target audience to such a binding. In your README file you also note that you do not implement multicast because it is irrelevant for home users. So for home users this binding is surely a great addon.

But I hardly believe that this binding is of great use for commercial solutions builders that take ESH as a base and build their own product on top of it. Therefore I think the best place for this binding would be the openhab2-addons repository. Since I have started my review now here, we can move it later once its in a state ready for merging though.

The major things that I noticed while reviewing are:

Please avoid copy&paste and make use of functions or if you already have base classes, implement common things in them not not again in each sub class.
Also make sure that bridges are connected before setting them to the ONLINE state. In general since you have the hardware it would be nice if you try some error states like turning things of in hardware or disconnect a bridge and see if you handle those cases correctly.

triller-telekom · 2017-07-07T08:29:30Z

extensions/binding/org.eclipse.smarthome.binding.dmx.test/META-INF/MANIFEST.MF

+Bundle-Vendor: Eclipse.org/SmartHome
+Bundle-Version: 0.9.0.qualifier
+Bundle-RequiredExecutionEnvironment: JavaSE-1.8
+Fragment-Host: org.eclipse.smarthome.binding.dmx


Missing dependency:

Bundle-ClassPath: ., lib/ola-java-client-0.0.3.jar, lib/protobuf-java-2.4.1.jar

Though I am not sure about the later one, but ola-client is definitely missing

I see it in the files-list as binary?

triller-telekom · 2017-07-07T08:32:00Z

extensions/binding/org.eclipse.smarthome.binding.dmx/ESH-INF/i18n/dmx_xx_XX.properties

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


This is a sample file, please remove it.

triller-telekom · 2017-07-07T08:32:36Z

extensions/binding/org.eclipse.smarthome.binding.dmx.test/.classpath

@@ -0,0 +1,9 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<classpath>
+	<classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/JavaSE-1.7"/>


Please update to use Java 1.8

triller-telekom · 2017-07-07T08:36:23Z

...g.dmx.test/src/test/groovy/org/eclipse/smarthome/binding/dmx/test/AbstractDmxOSGiTest.groovy

+
+    private Set<ThingHandler> getThingHandlers(ThingHandlerFactory factory) {
+        def thingManager = getService(ThingTypeMigrationService.class, { "org.eclipse.smarthome.core.thing.internal.ThingManager" } )
+        assertThat thingManager, not(null)


to be consistent with your other code you can use is(notNullValue()) here

triller-telekom · 2017-07-07T08:44:50Z

...test/src/test/groovy/org/eclipse/smarthome/binding/dmx/test/SacnBridgeHandlerOSGiTest.groovy

+        BaseChannel returnedChannel = bridgeHandler.getDmxChannel(channel, thing)
+
+        Integer channelId = returnedChannel.getChannelId()
+        assertThat channelId, equalTo(100)


triller-telekom · 2017-07-07T12:51:25Z

...e.binding.dmx/src/main/java/org/eclipse/smarthome/binding/dmx/handler/SacnBridgeHandler.java

+        logger.debug("refresh mode set to always: {}", refreshAlways);
+
+        super.updateConfiguration();
+        updateStatus(ThingStatus.ONLINE);


triller-telekom · 2017-07-07T12:55:45Z

...e.binding.dmx/src/main/java/org/eclipse/smarthome/binding/dmx/internal/DmxBridgeHandler.java

+    }
+
+    @Override
+    public void initialize() {


If you do not implement it, why do you override it here?

If I don't override it here, it's not available in the child-classes.

true, but it is empty. So what's the point of calling an empty method in the child class? :)

Interesting. Removing it works now. Eclipse complained before if I didn't include it. Maybe that was before I changed it to "abstract".

triller-telekom · 2017-07-07T13:03:58Z

...smarthome.binding.dmx/src/main/java/org/eclipse/smarthome/binding/dmx/internal/ValueSet.java

+    public ValueSet(int fadeTime, int holdTime, int value) {
+        this.fadeTime = fadeTime;
+        this.holdTime = holdTime;
+        addValue(value);


Below you use Util.toDmxValue(value) to ensure that the value is within range. I think you should do that here as well.

That would check the range twice, in addValue and here.

Sorry my fault.

triller-telekom · 2017-07-07T13:07:09Z

....binding.dmx/src/main/java/org/eclipse/smarthome/binding/dmx/internal/action/FadeAction.java

+ * state in the given amount of time. After the fade, the new state is held for
+ * a given or indefinite time.
+ *
+ * @author Davy Vanherbergen


If we want to integrate the code into ESH, not only you (Jan N. Klug) who submitted the PR has to sign the Eclipse contributions agreement, but also Davy Vanherbergen. Do you know how to contact him?

According to @kaikreuzer (https://community.openhab.org/t/oh2-esh-dmx-binding/16592/13) this should be available already.

Ok, but you have to also include him in the commit message as "Also-By" with his full name and email.

But I think the signing of the Eclipse contributions agreement is not the problem for this binding because the LGPL licence of the external OpenLightingProject lib is... See my other comment on the about.html.

My original commit included the Also-By. see the first commit.

triller-telekom · 2017-07-07T13:26:13Z

...binding.dmx/src/main/java/org/eclipse/smarthome/binding/dmx/internal/multiverse/Channel.java

+
+    private HashMap<ChannelUID, DmxThingHandler> onOffListeners = new HashMap<ChannelUID, DmxThingHandler>();
+    private HashMap<ChannelUID, DmxThingHandler> valueListeners = new HashMap<ChannelUID, DmxThingHandler>();
+    private Entry<ChannelUID, DmxThingHandler> actionListener = null;


You have one Channel object per ChannelUID is that correct? if so, why do you store this actionListener in a Map.Entry?

This could be stored in two variables. I chose to use the Entry to clearly show that these values belong together, they are set from the thing that adds the listener.

J-N-K · 2017-07-07T19:06:52Z

I think I addressed all comments. Please have a look (esp. on my longer comments). Thank you for your help.

triller-telekom · 2017-07-10T09:19:01Z

extensions/binding/org.eclipse.smarthome.binding.dmx/ESH-INF/thing/lib485-bridge.xml

+                <context>network_address</context>
+                <label>Network Address</label>
+                <description>Network address of bridge, format: address[:port]. Default port is 9020.</description>
+                <required>false</required>


I think in this case it would be better to have the parameter "required" and specify a <default>localhost</default>, because this is what "really happens" behind the scenes.

triller-telekom · 2017-07-10T09:19:50Z

extensions/binding/org.eclipse.smarthome.binding.dmx/ESH-INF/thing/ola-bridge.xml

+                <context>network_address</context>
+                <label>Network Address</label>
+                <description>Network address of bridge, format: address[:port]. Default port is 9010.</description>
+                <required>false</required>


See above, I think this should be required and you should show the default value to the user.

triller-telekom · 2017-07-10T09:48:01Z

extensions/binding/org.eclipse.smarthome.binding.dmx/about.html

+terms and conditions of use.</p>
+
+<h4>Java OLA Client 0.0.3</h4>
+<p>This Client Library is part of the OpenLighting project, licensed under LGPL.


Unfortunately the LGPL is not compatible with the EPL used in ESH :( However, I think we can move this binding to openhab2-addons which does not have to be compatible with the EPL.

My fault. I did check compatibilty earlier and forgot that the JavaClientLibrary of OLA is under another license than the whole project. The Java-Parts are Apache 2.0, which is ok. We don't need any of the LGPL parts.

J-N-K · 2017-07-10T15:58:29Z

extensions/binding/org.eclipse.smarthome.binding.dmx/ESH-INF/thing/ola-bridge.xml

+                <context>network_address</context>
+                <label>Network Address</label>
+                <description>Network address of bridge, format: address[:port]. Default port is 9010.</description>
+                <required>false</required>


triller-telekom

Thank you for the changes. Unfortunately you broke your test cases, can you have a look please?

triller-telekom · 2017-07-11T07:24:52Z

...e.binding.dmx/src/main/java/org/eclipse/smarthome/binding/dmx/handler/SacnBridgeHandler.java

@@ -94,7 +80,7 @@ protected void updateConfiguration() {
        logger.debug("refresh mode set to always: {}", refreshAlways);

        super.updateConfiguration();
-        updateStatus(ThingStatus.ONLINE);
+        updateStatus(ThingStatus.OFFLINE);


I think this change broke your test cases, can you have a look please? Also while you are at the tests have a look if the others are still working.

J-N-K · 2017-07-12T20:59:50Z

Strange. I will investigate why it succedes on my machine but fails on Travis.

J-N-K · 2017-07-20T04:37:29Z

As far as I can see, the only open issue here is the 0-100/0-255 discussion. All comments in the Forum agree that 0-255 is needed for the configuration. From rules, users can already use 0-100 by sending PercentType, which Seems perfectly compatible with ESH. How do we proceed from here?

triller-telekom

Since the 0-255 is a configuration on the thing only and sending a PercentType with values from 0-100 is fine, I guess we can leave it with 0-255 for the thing configuration.

triller-telekom · 2017-07-20T09:02:48Z

However, I still think that we should move the binding to openhab2-addons...

J-N-K · 2017-07-20T09:54:28Z

What makes this binding less usefull for ESH users compared to the Tradfri or Hue binding? Essentially they all cover gateways to lighting standards.

maggu2810 · 2017-07-24T06:24:28Z

However, I still think that we should move the binding to openhab2-addons...

As long as the binding is not "exotic" (only interesting for a very small / special user group) and will be maintained by some guys (e.g. @J-N-K) I would prefer to use the ESH repo. The license and IP related stuff is then part of the Eclipse infrastructure.

kaikreuzer · 2017-08-02T13:41:16Z

As ArtNet and OLA are in widespread use for DMX, I agree that this is a nice contribution to ESH in general and should not be considered "exotic".
As the code (or rather the technology, so no offense!) is pretty complex, I would also prefer if @J-N-K plans to further do its maintenance (and not just "dumping" it here). @J-N-K, can you confirm?

triller-telekom

Some minor remarks: One enum that should be in a separate file and changes in the build.properties file

triller-telekom · 2017-08-02T14:26:13Z

extensions/binding/org.eclipse.smarthome.binding.dmx/build.properties

@@ -0,0 +1,11 @@
+source.. = src/main/java/
+output.. = target/classes
+bin.includes = META-INF/,\


Could you please add the about.html to the bin.includes here?

triller-telekom · 2017-08-02T14:28:46Z

....binding.dmx/src/main/java/org/eclipse/smarthome/binding/dmx/internal/action/FadeAction.java

+    }
+}
+
+enum FadeDirection {


Please extract this enum in a separate file.

triller-telekom · 2017-08-02T15:09:52Z

extensions/binding/org.eclipse.smarthome.binding.dmx/build.properties

+               OSGI-INF/,\
+               ESH-INF/,\
+               lib/
+               lib/ola-java-client-0.0.3.jar,\


Since you have the lib directory in the bin.includes you do not have to specify the jar files here, so please remove them and keep the lib/ entry.

triller-telekom

2 new lines missing

triller-telekom · 2017-08-02T15:30:37Z

extensions/binding/org.eclipse.smarthome.binding.dmx/about.html

+
+
+</body>
+</html>


Please add a newline here

triller-telekom · 2017-08-02T15:31:43Z

extensions/binding/org.eclipse.smarthome.binding.dmx/README.md

+        Switch item=        MyChaserItem
+    }
+}
+```


Please add a new line ad the end of the file

J-N-K · 2017-08-02T18:42:55Z

@kaikreuzer If nothing unforseen happens, I can maintain this binding.

However, I switched to a new computer and can't make "mvn clean install" to run successfull on any ESH branch (not even master). So this will be stalled unless I manage to fix that.

kaikreuzer · 2017-08-02T19:32:07Z

@J-N-K great, thanks! If your current build problems are due to timeouts on eclipse.org, this is normal - we are seeing this on the official builds as well right now :-(

I'll do another last review the next days and once all is ready to be merged, I will have to create CQs (i.e. requests for approval by the Eclipse IP team) for the PR itself as well as for the included 3rd party libraries. Will keep you posted!

J-N-K · 2017-08-03T16:32:01Z

retriggering build. it's not clear to me why opening ports fails when it was working before.

triller-telekom · 2017-08-04T09:06:34Z

@J-N-K The build fails because one of your tests is failing...

Results :

Failed tests: 
  ArtnetBridgeHandlerOSGiTest.assert that bridge can be initialized:58->OSGiTest.waitForAssert:-1->OSGiTest.waitForAssert:191->OSGiTest.waitForAssert:220 
Expected: is <ONLINE>
     but: was <OFFLINE>

Tests run: 20, Failures: 1, Errors: 0, Skipped: 0

Since there is a bug where you say you can not really explain why that happens, this might actually be the right moment to tell you that we want to switch over to Java tests only and get rid of the groovy ones.

I did not mention this before, because your PR was created looooong before we took that decision, but maybe you want to give it a try and convert your tests to Java and see if this also solves your problem. There is an JavaOSGiTest you can derive from and then you should be pretty much straight forward to convert them. There are already quite some converted tests you can have a look at.

J-N-K · 2017-08-05T14:54:26Z

Seems to work now. As far as I can see all comments have been included now. And I haven't seen failing tests on my development machine. Unfortunately I had to rebase to get maven to build the system.

kaikreuzer

Thanks @J-N-K for your awesome work and sorry for taking so long for the final review.
Most of my comments are minor things, it should not involve any bigger refactoring.
I will meanwhile create CQs for the included libraries - I'll report back here, once they are approved (might take a while, though).

kaikreuzer · 2017-08-15T14:30:16Z

extensions/binding/org.eclipse.smarthome.binding.dmx/ESH-INF/thing/artnet-bridge.xml

+        </channels>
+
+        <config-description>
+            <parameter name="address" type="text">


We usually separate host and port into two parameters.
The host parameter should have <context>network-address</context>. The port parameter can be marked required=false as there is a default.

@triller-telekom suggested that before. My reason to keep address and port together is the following (copied from above):
I thought about that and decided against. The reason is: Most "address"-fields allow lists, where you can use different receivers. This is in the form "address[:port],address[:port]". If address and port are splitted into different fields, it would be mandatory (and possibly easy to get wrong) to add ports for every address. Example:

192.168.0.0,192.168.1.3:4000,192.168.4.2:5000, 10.0.1.3

The first and last would use the default port, while the second and third the port specified. If you split it, you would need to specify the default port, too, so

192.168.0.0,192.168.1.3,192.168.4.2,10.0.1.3
6454,4000,5000,6454

This is hard to understand. To keep address specification constistent, I used the same format for the local address.

Hm, I am not sure if I can follow you here. Where and how is it possible to give multiple addresses? The description says "Network address of bridge", so we are clearly speaking about a single one here. And as you have a default port, we would not require the user to always specify it.
Note that with the context "network-address" we would plan UI support in future (for syntax checking, possibly making suggestions, etc.), that's we it is strongly preferred to do it the same way in all bindings.

I'll try to explain. What usually is meant if we speak of DMX is DMX512A, a RS485-like serial protocol for transmitting multiple channels. You can have only one sender, but up to 32 receivers (with one or more channels) on one network. ArtNet (or any other DMX over Ethernet like sACN) are similar, they are Point-to-Multipoint protocols, one sender (ESH in this case) and one or more receivers. While the most common application probably will have one receiver in the form of a ArtNet-DMX512A-gateway. ArtNet is only used as a intermediate protocol for transporting the information. But - depending on the use-case - it is possible to have dimmers which directly speak ArtNet or multiple gateways.
This means that we need to define multiple receivers for each bridge. For sACN it may be possible to use multicast, but for ArtNet we are either limited to one receiver per bridge (which is in my opinion an unnecessary limitation) or allow some sort of address:port combination for each receiver.
I would propose to change the list I have implemented now to a "multiple" but leave address and port combined.

Ok, thanks for the deep dive. So actually, this "bridge" is not a single physical device (which is the normal intention when modelling an ESH bridge), but rather already a logical aggregation of different devices on the network that are supposed to interact like if they were a single device.
So then it is up to you whether you want to change it to multiple=true or keep it as a comma-separated list (to stay with a similar approach as with the channel definitions). But you would definitely have to update the parameter description.

kaikreuzer · 2017-08-15T14:31:30Z

extensions/binding/org.eclipse.smarthome.binding.dmx/ESH-INF/thing/artnet-bridge.xml

+                <label>DMX Universe</label>
+                <description>ID of DMX universe (0-32767)</description>
+                <default>0</default>
+                <required>false</required>


false is the default, so you could remove those lines

kaikreuzer · 2017-08-15T14:33:08Z

extensions/binding/org.eclipse.smarthome.binding.dmx/ESH-INF/thing/artnet-bridge.xml

+                <required>false</required>
+                <advanced>true</advanced>
+            </parameter>
+            <parameter name="refreshrate"  type="decimal" min="0" max="100">


Does "0" mean it is deactivated? If so, maybe add this info to the description.

kaikreuzer · 2017-08-15T14:35:56Z

extensions/binding/org.eclipse.smarthome.binding.dmx/ESH-INF/thing/channels.xml

+        <description>Change Fade Configuration</description>
+        <category>Light</category>
+        <tags>
+            <tag>Lighting</tag>


Lighting tag is not defined for String items, see here.

kaikreuzer · 2017-08-15T14:36:46Z

extensions/binding/org.eclipse.smarthome.binding.dmx/ESH-INF/thing/channels.xml

+        <description>Mutes the DMX output of the Bridge</description>
+        <category>Light</category>
+        <tags>
+            <tag>Lighting</tag>


This tag means that sending an ON would turn on some lights - this is not the case here, so better remove the tag.

kaikreuzer · 2017-08-16T12:05:07Z

...binding.dmx/src/main/java/org/eclipse/smarthome/binding/dmx/internal/multiverse/Channel.java

+     * Get the new value for this channel as determined by active actions or the
+     * current value.
+     *
+     * @param calculationTime


add a description (e.g. about the expected unit)

kaikreuzer · 2017-08-16T12:05:48Z

...inding.dmx/src/main/java/org/eclipse/smarthome/binding/dmx/internal/multiverse/Universe.java

+    public synchronized Channel registerChannel(BaseChannel baseChannel, Thing thing) {
+        for (Channel channel : channels) {
+            if (baseChannel.compareTo(channel) == 0) {
+                logger.trace("returning existing channel {}", channel.toString());


remove .toString()

kaikreuzer · 2017-08-16T12:05:59Z

...inding.dmx/src/main/java/org/eclipse/smarthome/binding/dmx/internal/multiverse/Universe.java

+        Channel channel = new Channel(baseChannel);
+        addChannel(channel);
+        channel.registerThing(thing);
+        logger.debug("creating and returning channel {}", channel.toString());


remove .toString()

kaikreuzer · 2017-08-16T12:06:25Z

...inding.dmx/src/main/java/org/eclipse/smarthome/binding/dmx/internal/multiverse/Universe.java

+                universeLock.unlock();
+            }
+        } else {
+            logger.warn("Adding channel {} to universe {} is not possible.", channel, universeId);


-> exception

kaikreuzer · 2017-08-16T12:07:33Z

extensions/binding/pom.xml

  </modules>

-</project>
+</project>


Could you also add an entry to https://github.com/eclipse/smarthome/blob/master/features/karaf/esh-ext/src/main/feature/feature.xml?

And also to https://github.com/eclipse/smarthome/blob/master/docs/_includes/documentation-menu.html#L23-L33?

kaikreuzer · 2017-08-16T15:03:25Z

extensions/binding/org.eclipse.smarthome.binding.dmx/about.html

+
+<h4>Java OLA Client 0.0.3</h4>
+<p>This Client Library is part of the OpenLighting project, licensed under Apache License, Version 2.0.
+The source code can be found at <a href="https://github.com/OpenLightingProject/ola">https://github.com/OpenLightingProject/ola</a>.


Question: Where did you find the binary version 0.0.3? I failed finding that.
Same for the sources - even the current master claims to be only 0.0.2, not 0.0.3. Could you please clarify?

openhab1-addons repo. I did not properly check that this is indeed the same version as in the OLA repository (assumed it is because the authors are identical and the class names are the same). We should ask Davy if possible or remove the OLA part from the binding. I have no idea where to find the correct source for that version.

or remove the OLA part from the binding

I cannot assess how relevant it is - if it really isn't that important, it might be the simplest solution as we won't require the approval of any external libs in that case. Wdyt?

I don't know either. I would suggest to remove it now and maybe add it later if that problem is resolved. OLA is nice because it offers support for a wide range of (USB-)DMX adapters but it never worked perfectly for me.
I can have a look into supporting at least some directly in the future. OLA adds a lot of overhead for most installations.

Ok, then let us go without it for the start - that makes our live much easier!

I removed it for now, but saved the files. We can easily add it back later after the license issue is resolved.

kaikreuzer · 2017-08-22T09:33:31Z

docs/_includes/documentation-menu.html

@@ -21,6 +21,7 @@
 				<ul>
 					<li><a href="{{docu}}/features/bindings/astro/readme.html">Astro</a></li>
 					<li><a href="{{docu}}/features/bindings/digitalstrom/readme.html">digitalSTROM</a></li>
+				    <li><a href="{{docu}}/features/bindings/dmx/readme.html">DMX</a></li>


Could you please replace the spaces by tabs, so that it is aligned with the other rows here?

kaikreuzer

Many thanks - we require muss less work regarding the CQs now :-)
Code looks all good to me, so if you are yourself happy with that version, please squash the branch to a single commit and I will pass this PR on as a CQ to the IP team.

triller-telekom · 2017-08-22T15:48:26Z

extensions/binding/org.eclipse.smarthome.binding.dmx.test/OSGI-INF/DmxTestHandlerFactory.xml

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


Could you please replace this file with the according DS annotations? For an example you can have a look at the SonosHandlerFactory But since this is a "test service" you can leave out the "name=" parameter so it uses the default name

J-N-K · 2017-08-22T20:40:38Z

Can someone shed some light on why this fails? It seems the DmxTestHandlerFactory is not found. This works on my local machine, in the IDE and with 'mvn clean install' on the command line. I have no idea how to fix this.

J-N-K · 2017-08-23T12:15:43Z

I tracked it down to the point that no XML is generated for the TestHandlerFactory (it works for the "normal" HandlerFactory) . Either someone has an idea what is missing here or we have to revert to the old style. I can't see the difference between both packages.

kaikreuzer · 2017-08-23T13:26:31Z

I cannot see any obvious errors in the test fragment either. Ok for me to revert to hand-written XML until a solution is found.

J-N-K · 2017-08-23T13:57:48Z

Will do so. The strange thing is, that it generates a correct XML if I select "Build project" in the IDE.

J-N-K · 2017-08-23T16:25:56Z

Retriggering build (unrelated test failure)

J-N-K · 2017-08-24T15:51:42Z

I think we can proceed and create the CQ.

kaikreuzer · 2017-08-24T19:23:27Z

Cool, thanks!
I have created CQ 14031, so we now have to wait for approval. Please do not do any changes to this PR from now on.

J-N-K · 2017-08-28T17:37:16Z

According to the discussion in #4074 we agreed that each binding should handle bridgeStatusChanged in the correct way for that binding. Therefore I need to update the PR (either before #4086 or similar is merged or before this is merged). Can this be done after CQ approval is here?

kaikreuzer · 2017-08-28T18:03:03Z

Yes, we can do that change once the approval is there - best as a separate commit then. I'll keep you updated.

openhab-bot · 2017-09-07T06:54:48Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/knx-binding-tunneling-request-with-invalid-rcv-seq-sporadic-dysfunction-help-needed/33946/3

htreu · 2017-09-07T12:35:53Z

hey @openhab-bot what are you doing here? fix some bugs!

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

Good news, the CQ has been approved!

@J-N-K As we have a merge conflict on the PR anyhow, I would suggest that you resolve that issue and you can implement an updated version of bridgeStatusChanged in a NEW commit (please do not change the existing one on that branch) - we can then merge by a rebase, so that both commits are kept.

J-N-K · 2017-09-07T17:30:11Z

I have no idea how to solve that. Sorry.

kaikreuzer · 2017-09-07T18:11:06Z

Rebase your branch on the latest master and you should see a single file being in conflict, which you can then solve manually.

If you do not know how to do such operations in git, I can try to do it myself on your branch.

Also-by: Davy Vanherbergen <davy.vanherbergen@gmail.com> Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>

Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>

J-N-K · 2017-09-08T10:03:59Z

If rebasing is ok, then I can do it myself. I have also fixed some other things that mad ethe build fail after the static-code-analysis was introduced. Hopefully Travis builds successfully now.

kaikreuzer · 2017-09-08T13:24:13Z

Well done, everything as it should be - so let's merge!
Thanks again for the fantastic contribution!

maggu2810 added Binding New Contribution labels Feb 7, 2017

triller-telekom suggested changes Jul 7, 2017

View reviewed changes

triller-telekom reviewed Jul 10, 2017

View reviewed changes

J-N-K commented Jul 10, 2017

View reviewed changes

triller-telekom reviewed Jul 11, 2017

View reviewed changes

triller-telekom approved these changes Jul 20, 2017

View reviewed changes

triller-telekom reviewed Aug 2, 2017

View reviewed changes

J-N-K closed this Aug 3, 2017

J-N-K reopened this Aug 3, 2017

kaikreuzer suggested changes Aug 16, 2017

View reviewed changes

kaikreuzer reviewed Aug 16, 2017

View reviewed changes

kaikreuzer reviewed Aug 22, 2017

View reviewed changes

kaikreuzer approved these changes Aug 22, 2017

View reviewed changes

triller-telekom reviewed Aug 22, 2017

View reviewed changes

J-N-K closed this Aug 23, 2017

J-N-K reopened this Aug 23, 2017

kaikreuzer added the CQ label Aug 24, 2017

J-N-K mentioned this pull request Sep 5, 2017

Hue lamp is not removed when removing hue bridge #1763

Open

kaikreuzer removed the CQ label Sep 7, 2017

J-N-K added 2 commits September 8, 2017 08:47

initial contribution

2a2ea26

Also-by: Davy Vanherbergen <davy.vanherbergen@gmail.com> Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>

improve tests and fixes from static-code-analysis tool

3efba13

Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>

kaikreuzer merged commit 6083f86 into eclipse-archived:master Sep 8, 2017

J-N-K mentioned this pull request Sep 8, 2017

DMX binding in ESH #2703

Closed

J-N-K deleted the feature-dmx-binding branch September 10, 2017 07:53

kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017



		</body>
		</html>

Feature: DMX binding #2704

Feature: DMX binding #2704

Conversation

J-N-K commented Dec 26, 2016

J-N-K commented Feb 12, 2017

maggu2810 commented Feb 13, 2017

J-N-K commented Feb 13, 2017

J-N-K commented Jun 28, 2017

triller-telekom commented Jul 6, 2017

triller-telekom 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

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

J-N-K commented Jul 7, 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

triller-telekom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

J-N-K commented Jul 12, 2017

J-N-K commented Jul 20, 2017

triller-telekom left a comment

Choose a reason for hiding this comment

triller-telekom commented Jul 20, 2017

J-N-K commented Jul 20, 2017

maggu2810 commented Jul 24, 2017

kaikreuzer commented Aug 2, 2017

triller-telekom 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

triller-telekom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

J-N-K commented Aug 2, 2017

kaikreuzer commented Aug 2, 2017

J-N-K commented Aug 3, 2017

triller-telekom commented Aug 4, 2017

J-N-K commented Aug 5, 2017

kaikreuzer 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