[homematic] Use system channel for PRESS datapoint #6587
Conversation
Fixes #6585. Signed-off-by: Michael Reitler <[email protected]>
| @@ -131,6 +132,7 @@ | |||
| virtualDatapointHandlers.add(new DisplayTextVirtualDatapoint()); | |||
| virtualDatapointHandlers.add(new HmwIoModuleVirtualDatapointHandler()); | |||
| virtualDatapointHandlers.add(new PressVirtualDatapointHandler()); | |||
There was a problem hiding this comment.
What about this PressVirtualDatapointHandler? The implementation looks very similar to the new Handler, except that it uses the options "SHORT", "LONG", "LONG_RELEASE", "CONT" instead of SHORT_PRESSED, LONG_PRESSED, DOUBLE_PRESSED. It looks like the old implementation, but shouldn't this be obsolete now?
There was a problem hiding this comment.
Well, I think there are two drawbacks of PressVirtualDatapointHandler
- It does not use the system channel, therefore it is specific to this binding.
- The devices that I tested with, do not send PRESSED_LONG before you release the key. Therefore the events LONG and LONG_RELEASE appear almost at the same time on PressVirtualDatapoint
Therefore I aggree that ButtonVirtualDatapointHandler may deprecate PressVirtualDatapointHandler. However, this would be API breaking. That means people who use the current trigger (for instance in a rule), will loose compability if we remove it.
How about the following?
- I mark the class as deprecated (annotation)
- I update the binding's readme accordingly (maybe striketrough of PRESS channel and putting system.button channel instead)
There was a problem hiding this comment.
That seems like a good solution.
| if (dp.getValue() == null || !dp.getValue().equals(dp.getPreviousValue())) { | ||
| vdp.setValue(CommonTriggerEvents.SHORT_PRESSED); | ||
| } else { | ||
| // two (or more) PRESS_SHORT events were received |
There was a problem hiding this comment.
Would this not also be true for two successive LONG_PRESSED events?
There was a problem hiding this comment.
The variable dp contains the original (non virtual) datapoint. This branch of the switch-case is only used for the PRESS_SHORT datapoint. Therefore it will not trigger on two successive LONG_PRESSED events
There was a problem hiding this comment.
Of course you're right; I missed that the original datapoint is used here.
| } | ||
|
|
||
| @Override | ||
| public void handleEvent(VirtualGateway gateway, HmDatapoint dp) { |
There was a problem hiding this comment.
A unit test for the handleEvent() method would be nice, especially for the mapping to the DOUBLE_PRESSED event.
Fixes #6585. Documentation & unittest Signed-off-by: Michael Reitler <[email protected]>
| @@ -28,8 +28,22 @@ | |||
| * A virtual String datapoint which adds a PRESS datapoint for each key with a PRESS_SHORT and PRESS_LONG datapoint to | |||
| * simulate a key trigger. | |||
| * | |||
| * <p> | |||
| * <b>Depreaction notice:</b> | |||
| hmChannel.setDevice(device); | ||
| device.addChannel(hmChannel); | ||
| bvdpHandler.initialize(device); | ||
| // shortPressChannel.addDatapoint(buttonVirtualDatapoint); |
There was a problem hiding this comment.
Here are three lines commented out, should they have been removed?
There was a problem hiding this comment.
That is correct. I decided to use bvdpHandler.initialize(device); instead of initializing it manually. But I forgot to remove it. Thx
* minor fixes Signed-off-by: Michael Reitler <[email protected]>
|
LGTM |
* fix for unittest Signed-off-by: Michael Reitler <[email protected]>
|
Oooops, I missed sth when I simplified the test in the previous commit. Just fixed it and updated... |
|
I checked the fix, LGTM |
There was a problem hiding this comment.
I'd like to get @gerrieg's feedback here as well as he had introduced the PRESS channel type.
If he is ok with the change and does not feel strong about removing it, I'd rather go for a breaking change instead of only deprecating it.
extensions/binding/org.eclipse.smarthome.binding.homematic/README.md
Outdated
Show resolved
Hide resolved
* remove reference from README.md to sourcecode Signed-off-by: Michael Reitler <[email protected]>
|
Thx @kaikreuzer and @gerrieg |
* remove deprecated PressDatapoint Signed-off-by: Michael Reitler <[email protected]>
Fixes #6585.
Signed-off-by: Michael Reitler [email protected]