[dmx] fixes #4774 #4775

J-N-K · 2016-11-13T11:31:14Z

Adds configurable DMX transmit refresh. The "refresh" configuration option defaults to -1 (never refresh), which is compatible with the current implementation. A value of 0 would refresh every 35ms, another value would transmit data if it has been changed or the last transmission is more than (value) ms in the past.

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

watou · 2016-11-13T11:52:22Z

...g.openhab.binding.dmx/src/main/java/org/openhab/binding/dmx/internal/core/DmxController.java

    private static Logger logger = LoggerFactory.getLogger(DmxController.class);

    private static int TRANSMIT_FREQUENCY_MS = 35;
+    private int transmitRefreshMs = 0; // default is send all updates


This disagrees with the PR comment:

The "refresh" configuration option defaults to -1 (never refresh)

watou · 2016-11-13T11:53:37Z

...g.openhab.binding.dmx/src/main/java/org/openhab/binding/dmx/internal/core/DmxController.java

-
        logger.trace("Starting Dmx transmitter ...");
        transmitter = new DmxTransmitter(this);
+        transmitter.setTransmitRefresh(transmitRefreshMs);


Could the variables/members/methods all use the phrase refreshInterval so the changed code is easier to follow?

watou · 2016-11-13T11:54:26Z

....openhab.binding.dmx/src/main/java/org/openhab/binding/dmx/internal/core/DmxTransmitter.java

+     *
+     * @param refreshInterval
+     *            interval in ms (if output did not change)
+     */


setRefreshInterval as mentioned above?

watou · 2016-11-13T11:55:39Z

....openhab.binding.dmx/src/main/java/org/openhab/binding/dmx/internal/core/DmxTransmitter.java

     */
    public DmxTransmitter(DmxService service) {
        this.service = service;
+        lastTransmit = 0;


Could lastTransmit be initialized to 0 above, instead of one private member being initialized that way, and the other initialized in the constructor?

J-N-K · 2016-11-14T15:59:29Z

I have incorporated all comments. However, after re-reading E1.11 (DMX 512) and E1.31 (sACN) as well as ArtNet, I come to the conclusion that the "never-refresh-policy" is plain wrong when it comes to standards compliance.

E1.11 states for DMX512 that there is a continous data stream (with a maximum MARK before BREAK of 1s), and defines that longer periods are considered as Data Lost (8.9). The standard does not define how a receiver should behave if that occurs, besides that it should remain operational for at least 60s. So after 60s without signal a standard compliant receiver could even turn off (9.2).

E1.31 defines that if the output is not changing, the last packet should be re-transmitted three times. After that the sender is allowed to pause sending for 800-1000ms (6.6.2).

ArtNet defines that non-changing DMX data should be retransmitted at approximately 4s intervals, but it is recommended for compatibility with sACN to re-transmit after 800-1000ms (chapter ArtDmx, p. 46).

So my proposal would be to

either change the default to "send always", and transmit three unchanged packets before suppressing further updates for the configured refreshTimeout or
change the configuration option to "suppressRetransmission" with a default of "false" and suppressing re-transmission of unchanged data for 800ms after sending three packets.

Sorry, I didn't think about that before.

(Sorry #2, closed PR by accident).

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

watou · 2016-11-18T11:34:09Z

Hi @J-N-K, could you circulate your thoughts about proper refresh handling to other DMX users on the forum (unless you've already done that), and make sure your proposed changes don't meet thoughtful disagreement? Your focus on this is probably a rare opportunity to get this right vis-a-vis standards and real-world operation. So I will mark this PR as awaiting feedback until the related issues are all well considered. Thanks for your contributions!

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

J-N-K · 2016-11-25T05:41:13Z

Since nobody seems to object, how do we proceed on this one?

watou

Hi @J-N-K, here are a few more review comments.

watou · 2016-11-25T07:53:03Z

...g.openhab.binding.dmx/src/main/java/org/openhab/binding/dmx/internal/core/DmxController.java

                logger.debug("Setting connection from config: {}", connectionString);
            }
+            // refresh timeout (i.e. interval between transmits if nothing changed)
+            String configuredRepeatMode = ((String) config.get("repeatMode")).toLowerCase();


This will cause an NPE if repeatMode is not configured in the configuration, but in order to be compatible for existing users, the code must allow that no repeatMode is configured. See my advice earlier about using an enum type.

watou · 2016-11-25T07:54:43Z

...g.openhab.binding.dmx/src/main/java/org/openhab/binding/dmx/internal/core/DmxController.java

+            // refresh timeout (i.e. interval between transmits if nothing changed)
+            String configuredRepeatMode = ((String) config.get("repeatMode")).toLowerCase();
+            if (StringUtils.isNotBlank(configuredRepeatMode)) {
+                if (configuredRepeatMode.matches("always")) {


Use the enum type as suggested, or if not, equalsIgnoreCase instead of matches here and below You can then remove the NPE possibility by removing the toLowerCase above.

watou · 2016-11-25T08:02:18Z

...g.openhab.binding.dmx/src/main/java/org/openhab/binding/dmx/internal/core/DmxController.java

    private static Logger logger = LoggerFactory.getLogger(DmxController.class);

    private static int TRANSMIT_FREQUENCY_MS = 35;
+    private int repeatMode = DmxTransmitter.REPEAT_MODE_ALWAYS; // default is send every update


Instead of int, it would be safer to use an enum type. Here is the construct that would seem to suit this optionally configured value.

Good idea. A question regarding the coding style: should this enum be below DmxTransmitter (as are the constants now) or seperate?

watou · 2016-11-25T08:04:37Z

....openhab.binding.dmx/src/main/java/org/openhab/binding/dmx/internal/core/DmxTransmitter.java

+
+    public static final int REPEAT_MODE_ALWAYS = 0;
+    public static final int REPEAT_MODE_NEVER = 1;
+    public static final int REPEAT_MODE_REDUCED = 2;


These are more suitably an enum type.

watou · 2016-11-25T08:05:31Z

....openhab.binding.dmx/src/main/java/org/openhab/binding/dmx/internal/core/DmxTransmitter.java

+     *
+     * @param refreshInterval
+     *            interval in ms (if output did not change)
+     */


enum type

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

watou · 2016-11-25T17:19:58Z

The code looks good to me. If you report back that this latest version of the PR is working reliably, and will continue to work the same (or better) for other existing users, I think it's ready to merge. Also please update the wiki to reflect the repeatMode parameter and its detailed meaning. Thanks!

J-N-K · 2016-11-25T17:57:40Z

I'll test that for some days and report back. Do you think that positioning the enum in DmxTransmitter is a good place? If so, I'll squash all commits into one.

Thanks for your guidance.

watou · 2016-11-25T20:49:58Z

Do you think that positioning the enum in DmxTransmitter is a good place?

There is no issue with nesting it where you did. Java enums are implicitly static, so that's good too.

If so, I'll squash all commits into one.

No need to unless you want to, since maintainers squash and merge in one step nowadays.

Thanks for your good work, @J-N-K!

J-N-K · 2016-11-30T11:24:51Z

This is working like a charm. I think we can merge it.

J-N-K · 2016-12-28T21:28:54Z

It's four weeks now since this is ready for merge. Please let me know if I should close this PR

watou · 2016-12-28T21:51:08Z

lgtm, thanks for the reminder @J-N-K.

watou reviewed Nov 13, 2016

View reviewed changes

J-N-K closed this Nov 14, 2016

J-N-K reopened this Nov 14, 2016

J-N-K mentioned this pull request Nov 14, 2016

[dmx] binding never refreshes if values do not change #4774

Closed

fix wrong DMX retransmitting scheme

b48609d

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

watou added the awaiting-feedback label Nov 18, 2016

reduce status update traffic to "update on change" instead on "update…

980fd86

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

watou changed the title ~~fixes #4774~~ [dmx] fixes #4774 Nov 24, 2016

watou suggested changes Nov 25, 2016

View reviewed changes

J-N-K added 2 commits November 25, 2016 10:43

change to enmum - untested - do not merge

399ed6d

fixes

6d92177

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

watou added the enhancement label Nov 25, 2016

watou added this to the 1.9.0 milestone Nov 25, 2016

J-N-K closed this Dec 28, 2016

J-N-K reopened this Dec 28, 2016

watou removed the awaiting-feedback label Dec 28, 2016

watou merged commit b9a08b4 into openhab:master Dec 28, 2016

J-N-K deleted the fix-dmx-refresh branch December 31, 2016 15:13

Uh oh!

[dmx] fixes #4774 #4775

[dmx] fixes #4774 #4775

Uh oh!

Conversation

J-N-K commented Nov 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

J-N-K commented Nov 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

watou commented Nov 18, 2016

Uh oh!

J-N-K commented Nov 25, 2016

Uh oh!

watou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

watou commented Nov 25, 2016

Uh oh!

J-N-K commented Nov 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

watou commented Nov 25, 2016

Uh oh!

J-N-K commented Nov 30, 2016

Uh oh!

J-N-K commented Dec 28, 2016

Uh oh!

watou commented Dec 28, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

J-N-K commented Nov 14, 2016 •

edited

Loading

J-N-K commented Nov 25, 2016 •

edited

Loading