[lcn] Add configuration option invertOpenClosed to binary sensor channel #8102

toweosp · 2020-07-10T20:40:53Z

Added configuration option invertOpenClosed to binarysensor channel. Per default lcn binary sensor state 0 corresponds to "closed" while 1 corresponds to "open". Use this parameter to invert that logic.

closes #8069

TravisBuddy · 2020-07-10T20:43:51Z

Travis tests have failed

Hey @toweosp,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

fwolter

Thanks for your contribution! Here is my feedback.

bundles/org.openhab.binding.lcn/src/main/resources/ESH-INF/thing/thing-types.xml

fwolter · 2020-07-10T21:43:40Z

...org.openhab.binding.lcn/src/main/java/org/openhab/binding/lcn/internal/LcnModuleHandler.java

+        if (channelGroup.equals(LcnChannelGroup.BINARYSENSOR)) {
+            Channel channel = thing.getChannel(channelUid);
+            if (channel != null) {
+                Object invertConfig = channel.getConfiguration().get("invertOpenClosed");
+                boolean invertOpenClosed = invertConfig instanceof Boolean && (boolean) invertConfig;
+                if (invertOpenClosed) {
+                    if (state.equals(OpenClosedType.OPEN)) {
+                        convertedState = OpenClosedType.CLOSED;
+                    } else {
+                        convertedState = OpenClosedType.OPEN;


Can you move this into LcnModuleBinarySensorSubHandler similar to the inversion of the roller shutter up/down in LcnModuleRollershutterRelaySubHandler?

BaseThingHandler.updateState is not visible from AbstractLcnModuleSubHandler. So one can not delegate channel update handling to a sub handler similar to the rollershutter command handling (without revealing BaseThingHandler.updateState in LcnModuleHandler). We could delegate state conversion within LcnModuleHandler.updateChannel to a converter (similar to Converter.onStateUpdateFromHandler(State)). This would require Converter to be an (abstract) superclass or better interface for arbitrary converters which are initialized in LcnModuleHandler.initialize. At the moment Converter is specialized to deal with variables only.

What do you think?

If I see correctly, BaseThingHandler.updateState() is invoked via LcnModuleHandler.updateChannel() -> AbstractLcnModuleSubHandler.fireUpdate() -> LcnModuleBinarySensorSubHandler.handleStatusMessage(). I think the conversion could happen in LcnModuleBinarySensorSubHandler:55.

Thanks for your feedback! If I didn't miss something when following your suggestion channel update (and status message handling resp.) would differ between binary sensors and variables. So from a design perspective I think we should choose between using the converter approach or uniquely handle value conversion within the sub-handler's handleStatusMessage.

Would you like to decide how to uniquely handle value conversion and adapt the code correspondingly? Perhaps it would make sense to handle this on a separate enhancement/pull request due to the necessary refactorings.

Although I'm a fan a of abstracting things, I wouldn't combine conversion of raw data into human readable values and the inversion of binary sensor states. First is a conversion, the other is a manipulation. From the functional perspective, I think they have to less in common to merge these functions.

From my point of view there is no conceptual difference between converting raw variable data (i.e. lcn value) into temperature, speed etc. and converting raw lcn binary sensor data (0 or 1) into open/closed states. In my opinion in both cases conversion applies semantic to raw sensor data leading to human readable values.

Ok, you convinced me. I refactored the conversion code and made Converter an interface. Do you want to implement an inversion Converter?

Can you give me access rights to merge the changes into your branch? Alternatively, you could merge my lcn-converter branch into yours. Before doing so you have to rebase this PR's branch to bb6722c.

I added an InversionConverter. For this I moved Converter.onCommandFromItem(double) and Converter.onCommandFromItem(QuantityType<?>) down the hierarchy to ValueConverter because I think these methods make only sense when converting numerical values. The InversionConverter might later additionally be used e.g. for visualizing channels with inversed OnOff or UpDown states.

TravisBuddy · 2020-07-11T07:26:07Z

Travis tests have failed

Hey @toweosp,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

TravisBuddy · 2020-07-21T14:35:19Z

Travis tests have failed

Hey @toweosp,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

fwolter

Thanks! There seem to be commits from the 2.5.x branch in this PR, which don't belong to it.

There are some formatting issues. You can view them with mvn spotless:check -Dspotless.check.skip=false and fix them with mvn spotless:apply.

fwolter · 2020-07-21T14:46:37Z

...org.openhab.binding.lcn/src/main/java/org/openhab/binding/lcn/internal/LcnModuleHandler.java

+                if (valueConverter instanceof ValueConverter) {
+                    DecimalType nativeValue = ((ValueConverter) valueConverter)
+                            .onCommandFromItem(decimalType.doubleValue());
+                    subHandler.handleCommandDecimal(nativeValue, channelGroup, number.get());
+                }else{


This casting bothers me a bit. Is there an other way?

Sorry, had some issues with my eclipse/EGit installation. I hopefully managed to "clean" the branch from the commits not belonging to this PR, but I still have the following issues:

I can't satisfy DCO (your commit apparently was missing signature). Tried to apply the steps mentioned in the DCO log but with no success.

J-N-K is still listed as a reviewer for this PR, although the commits not belonging to this PR are now (hopefully) removed.

To your question regarding the cast: One could move ValueConverter.onCommandFromItem(double) and ValueConverter.onCommandFromItem(QuantityType<?>) up to Converter again making them Nullable so InversionConverter could return null. But in my opinion this would violate separation of concerns, see my last comment.

From a design perspective it might be better to separate conversion for representation (lcn bus -> UI) from conversion of commands (UI -> lcn bus) leading to two different converter hierarchies/interfaces, e.g. StateRepresentationConverter.onStateUpdateFromHandler and CommandConverter.onCommandFromItem - or to be okay with the casting ;-)

I'd go for a modified version of your first proposal. Move the methods back to Converter. Make Converter a class and add a default implementation for all four methods, that return the identity. Then, Nullable is not needed. Casting, instanceof and semantic based null values violate OOP paradigms most often.

I don't think splitting the converters by the message direction doesn't solve the problem as the differentiation between values and binary states is still necessary.

Converters refactored as you requested.

TravisBuddy · 2020-07-21T14:58:42Z

Travis tests were successful

Hey @toweosp,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

TravisBuddy · 2020-07-21T20:42:23Z

Travis tests have failed

Hey @toweosp,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

TravisBuddy · 2020-07-21T21:07:15Z

Travis tests have failed

Hey @toweosp,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

TravisBuddy · 2020-07-21T21:20:51Z

Travis tests were successful

Hey @toweosp,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

TravisBuddy · 2020-07-22T20:25:20Z

Travis tests have failed

Hey @toweosp,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

TravisBuddy · 2020-07-23T20:40:00Z

Travis tests have failed

Hey @toweosp,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

fwolter

Thanks! Just one small change.

bundles/org.openhab.binding.lcn/src/main/resources/ESH-INF/thing/thing-types.xml

TravisBuddy · 2020-07-24T11:57:45Z

Travis tests have failed

Hey @toweosp,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

fwolter

LGTM

fwolter · 2020-07-26T11:26:56Z

Can you adopt the sign-off of my commit to the merge commit 507251a to satisfy the DCO check?

Signed-off-by: Fabian Wolter <github@fabian-wolter.de>

References PR #8102 Signed-off-by: Thomas Weiler <toweosp@gmail.com>

Signed-off-by: Thomas Weiler <toweosp@gmail.com>

TravisBuddy · 2020-07-31T15:41:17Z

Travis tests have failed

Hey @toweosp,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

toweosp · 2020-07-31T18:54:13Z

Done (beside failing jdk11 build #8094)

fwolter · 2020-08-01T10:50:03Z

Can you fix the conflicts raised by the other PR?

TravisBuddy · 2020-08-01T12:42:44Z

Travis tests have failed

Hey @toweosp,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

fwolter

LGTM

…nel (openhab#8102) Closes openhab#8069 Signed-off-by: Thomas Weiler <toweosp@gmail.com> Signed-off-by: MPH80 <michael@hazelden.me>

…nel (openhab#8102) Closes openhab#8069 Signed-off-by: Thomas Weiler <toweosp@gmail.com> Signed-off-by: Bernhard <bern77@gmail.com>

…nel (openhab#8102) Closes openhab#8069 Signed-off-by: Thomas Weiler <toweosp@gmail.com>

…nel (openhab#8102) Closes openhab#8069 Signed-off-by: Thomas Weiler <toweosp@gmail.com> Signed-off-by: Daan Meijer <daan@studioseptember.nl>

…nel (openhab#8102) Closes openhab#8069 Signed-off-by: Thomas Weiler <toweosp@gmail.com>

toweosp requested a review from fwolter as a code owner July 10, 2020 20:40

fwolter changed the title ~~[lcn] Added configuration option invertOpenClosed to binarysensor channel~~ [lcn] Add configuration option invertOpenClosed to binary sensor channel Jul 10, 2020

fwolter added the enhancement An enhancement or new feature for an existing add-on label Jul 10, 2020

fwolter requested changes Jul 10, 2020

View reviewed changes

toweosp requested a review from J-N-K as a code owner July 21, 2020 14:35

fwolter requested changes Jul 21, 2020

View reviewed changes

fwolter requested changes Jul 23, 2020

View reviewed changes

bundles/org.openhab.binding.lcn/src/main/resources/ESH-INF/thing/thing-types.xml Outdated Show resolved Hide resolved

fwolter approved these changes Jul 26, 2020

View reviewed changes

fwolter and others added 4 commits July 31, 2020 16:23

Refactor Converters

4f47203

Signed-off-by: Fabian Wolter <github@fabian-wolter.de>

[lcn] Add configuration option invertState to binary sensor channel

a01e9bd

References PR #8102 Signed-off-by: Thomas Weiler <toweosp@gmail.com>

[lcn] Refactor converters

0bcc5de

Signed-off-by: Thomas Weiler <toweosp@gmail.com>

[lcn] Correct label formatting

eb367e1

Signed-off-by: Thomas Weiler <toweosp@gmail.com>

Merge branch '2.5.x' into invertOpenClosed

5b924ee

fwolter approved these changes Aug 1, 2020

View reviewed changes

fwolter merged commit b9d13c7 into openhab:2.5.x Aug 1, 2020

fwolter added this to the 2.5.8 milestone Aug 1, 2020

toweosp deleted the invertOpenClosed branch August 1, 2020 13:21

andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020

[lcn] Add configuration option invertOpenClosed to binary sensor chan…

2acd865

…nel (openhab#8102) Closes openhab#8069 Signed-off-by: Thomas Weiler <toweosp@gmail.com>

andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020

[lcn] Add configuration option invertOpenClosed to binary sensor chan…

a1a999e

…nel (openhab#8102) Closes openhab#8069 Signed-off-by: Thomas Weiler <toweosp@gmail.com>

andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020

[lcn] Add configuration option invertOpenClosed to binary sensor chan…

b31e548

…nel (openhab#8102) Closes openhab#8069 Signed-off-by: Thomas Weiler <toweosp@gmail.com>

andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020

[lcn] Add configuration option invertOpenClosed to binary sensor chan…

9794fa4

…nel (openhab#8102) Closes openhab#8069 Signed-off-by: Thomas Weiler <toweosp@gmail.com>

CSchlipp pushed a commit to CSchlipp/openhab-addons that referenced this pull request Sep 12, 2020

[lcn] Add configuration option invertOpenClosed to binary sensor chan…

ceec5a4

…nel (openhab#8102) Closes openhab#8069 Signed-off-by: Thomas Weiler <toweosp@gmail.com>

markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Sep 19, 2020

[lcn] Add configuration option invertOpenClosed to binary sensor chan…

a014388

…nel (openhab#8102) Closes openhab#8069 Signed-off-by: Thomas Weiler <toweosp@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lcn] Add configuration option invertOpenClosed to binary sensor channel #8102

[lcn] Add configuration option invertOpenClosed to binary sensor channel #8102

toweosp commented Jul 10, 2020 •

edited by fwolter

Loading

TravisBuddy commented Jul 10, 2020

fwolter left a comment

fwolter Jul 10, 2020

toweosp Jul 11, 2020

fwolter Jul 11, 2020

toweosp Jul 12, 2020

fwolter Jul 13, 2020

fwolter Jul 19, 2020

toweosp Jul 19, 2020

fwolter Jul 19, 2020

toweosp Jul 21, 2020

TravisBuddy commented Jul 11, 2020

TravisBuddy commented Jul 21, 2020

fwolter left a comment

fwolter Jul 21, 2020

toweosp Jul 21, 2020 •

edited

Loading

fwolter Jul 22, 2020

toweosp Jul 23, 2020

TravisBuddy commented Jul 21, 2020

TravisBuddy commented Jul 21, 2020

TravisBuddy commented Jul 21, 2020

TravisBuddy commented Jul 21, 2020

TravisBuddy commented Jul 22, 2020

TravisBuddy commented Jul 23, 2020

fwolter left a comment

TravisBuddy commented Jul 24, 2020

fwolter left a comment

fwolter commented Jul 26, 2020

TravisBuddy commented Jul 31, 2020

toweosp commented Jul 31, 2020

fwolter commented Aug 1, 2020

TravisBuddy commented Aug 1, 2020

fwolter left a comment

[lcn] Add configuration option invertOpenClosed to binary sensor channel #8102

[lcn] Add configuration option invertOpenClosed to binary sensor channel #8102

Conversation

toweosp commented Jul 10, 2020 • edited by fwolter Loading

TravisBuddy commented Jul 10, 2020

Travis tests have failed

fwolter 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

TravisBuddy commented Jul 11, 2020

Travis tests have failed

TravisBuddy commented Jul 21, 2020

Travis tests have failed

fwolter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toweosp Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TravisBuddy commented Jul 21, 2020

Travis tests were successful

TravisBuddy commented Jul 21, 2020

Travis tests have failed

TravisBuddy commented Jul 21, 2020

Travis tests have failed

TravisBuddy commented Jul 21, 2020

Travis tests were successful

TravisBuddy commented Jul 22, 2020

Travis tests have failed

TravisBuddy commented Jul 23, 2020

Travis tests have failed

fwolter left a comment

Choose a reason for hiding this comment

TravisBuddy commented Jul 24, 2020

Travis tests have failed

fwolter left a comment

Choose a reason for hiding this comment

fwolter commented Jul 26, 2020

TravisBuddy commented Jul 31, 2020

Travis tests have failed

toweosp commented Jul 31, 2020

fwolter commented Aug 1, 2020

TravisBuddy commented Aug 1, 2020

Travis tests have failed

fwolter left a comment

Choose a reason for hiding this comment

toweosp commented Jul 10, 2020 •

edited by fwolter

Loading

toweosp Jul 21, 2020 •

edited

Loading