[avmfritz] Added ThingActions to handle Boost and Window-Open modes for heating devices / groups #8222

cweitkamp · 2020-07-29T12:38:44Z

Added ThingActions to handle Boost and Window-Open modes for heating devices / groups

Signed-off-by: Christoph Weitkamp [email protected]

kaikreuzer · 2020-08-04T21:39:37Z

...ritz/src/main/java/org/openhab/binding/avmfritz/internal/actions/AVMFritzHeatingActions.java

+ *
+ * SPDX-License-Identifier: EPL-2.0
+ */
+package org.openhab.binding.avmfritz.internal.actions;


I am actually not certain myself anymore, but shouldn't actions be in exported packages, so that they can be seen by the rule engine? Scanning our repo here, I noticed that we have a few bindings putting actions in exported packages, while others have it in internal packages...

Me neither. But does the rule engine really need to know them? I don't think so because we use a wrapper action to access all available ThingActions by filtering on scope and ThingUID.

It is using the wrapper at runtime, but for the DSL and specifically the content completion in the editor, it is imho required:

The mail action is so far the only one I am using, but that one is in an exported package, so I'd suggest to do the same with the others.

Good point. I moved them.

I see that you moved not only the Actions class, but also the AVMFritzHeatingActionsHandler interface - is this necessary to be exposed? If not, I'd say it should stay internal.

I moved it back to an internal package.

...b.binding.avmfritz/src/main/java/org/openhab/binding/avmfritz/internal/dto/HeatingModel.java

cpmeister · 2020-08-26T18:37:31Z

...g.avmfritz/src/main/java/org/openhab/binding/avmfritz/internal/AVMFritzBindingConstants.java

+            .of(SUPPORTED_BUTTON_THING_TYPES_UIDS.stream(), SUPPORTED_HEATING_THING_TYPES.stream(),
+                    SUPPORTED_DEVICE_THING_TYPES_UIDS.stream(), SUPPORTED_GROUP_THING_TYPES_UIDS.stream(),
+                    SUPPORTED_BRIDGE_THING_TYPES_UIDS.stream())
            .reduce(Stream::concat).orElseGet(Stream::empty).collect(Collectors.toSet()));


Suggested change

.of(SUPPORTED_BUTTON_THING_TYPES_UIDS.stream(), SUPPORTED_HEATING_THING_TYPES.stream(),

SUPPORTED_DEVICE_THING_TYPES_UIDS.stream(), SUPPORTED_GROUP_THING_TYPES_UIDS.stream(),

SUPPORTED_BRIDGE_THING_TYPES_UIDS.stream())

.reduce(Stream::concat).orElseGet(Stream::empty).collect(Collectors.toSet()));

.of(SUPPORTED_BUTTON_THING_TYPES_UIDS, SUPPORTED_HEATING_THING_TYPES,

SUPPORTED_DEVICE_THING_TYPES_UIDS, SUPPORTED_GROUP_THING_TYPES_UIDS,

SUPPORTED_BRIDGE_THING_TYPES_UIDS)

.flatMap(Set::stream).orElseGet(Stream::empty).collect(Collectors.toSet()));

Thanks for the hint. I changed it. But we do not need orElseGet(...) combined with flatMap(...).

cpmeister · 2020-08-26T18:43:59Z

...ding.avmfritz/src/main/java/org/openhab/binding/avmfritz/actions/AVMFritzHeatingActions.java

+    private static AVMFritzHeatingActions invokeMethodOf(@Nullable ThingActions actions) {
+        if (actions == null) {
+            throw new IllegalArgumentException("Actions cannot be null");
+        }
+        if (actions.getClass().getName().equals(AVMFritzHeatingActions.class.getName())) {
+            if (actions instanceof AVMFritzHeatingActions) {
+                return (AVMFritzHeatingActions) actions;
+            } else {
+                return (AVMFritzHeatingActions) Proxy.newProxyInstance(AVMFritzHeatingActions.class.getClassLoader(),
+                        new Class[] { AVMFritzHeatingActions.class }, (Object proxy, Method method, Object[] args) -> {
+                            Method m = actions.getClass().getDeclaredMethod(method.getName(),
+                                    method.getParameterTypes());
+                            return m.invoke(actions, args);
+                        });
+            }
+        }
+        throw new IllegalArgumentException("Actions is not an instance of AVMFritzHeatingActions");
+    }


Proxy instances can only be interfaces, not concrete classes. So you need to make an interface that has all of your methods your actions are going to use.

This invokeMethodOf method should likewise return an interface instead.

Isn't that the pattern that was just done on ALL our thing actions in this repo?

Isn't that the pattern that was just done on ALL our thing actions in this repo?

It is, but the pattern wasn't done correctly in this particular instance. All of the other ones (or at least the ones that I approved) require an additional interface class to be created.

bundles/org.openhab.binding.avmfritz/src/main/resources/ESH-INF/i18n/avmfritz.properties

cpmeister · 2020-08-31T19:42:27Z

...ding.avmfritz/src/main/java/org/openhab/binding/avmfritz/actions/AVMFritzHeatingActions.java

-        if (actions.getClass().getName().equals(AVMFritzHeatingActions.class.getName())) {
-            if (actions instanceof AVMFritzHeatingActions) {
-                return (AVMFritzHeatingActions) actions;
+        if (actions.getClass().getName().equals(AVMFritzHeatingActionsHandler.class.getName())) {


Suggested change

if (actions.getClass().getName().equals(AVMFritzHeatingActionsHandler.class.getName())) {

if (actions.getClass().getName().equals(AVMFritzHeatingActions.class.getName())) {

The class name should be compared against the name of the concrete action class since you would never receive an instance of a non-concrete class as a parameter.

cpmeister · 2020-08-31T19:55:45Z

...ding.avmfritz/src/main/java/org/openhab/binding/avmfritz/actions/AVMFritzHeatingActions.java

+        invokeMethodOf(actions).setWindowOpenMode(duration);
+    }
+
+    private static AVMFritzHeatingActionsHandler invokeMethodOf(@Nullable ThingActions actions) {


Although yes AVMFritzHeatingActionsHandler is an interface, I think it is the wrong interface here since it extends ThingHandler. The interface returned by the invokeMethodOf should be an interface implemented by AVMFritzHeatingActions so it would reason that it would be a type of ThingHandlerService not a ThingHandler.
You should create a new interface that has only two methods: setBoostMode(Long) and setWindowOpenMode(Long) and have AVMFritzHeatingActions implement that new interface. Likewise it is that new interface which would be returned from invokeMethodOf. So you would need to change your usages of AVMFritzHeatingActionsHandler here use the new interface instead.

Sorry if this has all been confusing but you are almost there. If you need a concrete reference to follow then you can look at the way the Ecobee binding has done it.

https://github.com/openhab/openhab-addons/blob/2.5.x/bundles/org.openhab.binding.ecobee/src/main/java/org/openhab/binding/ecobee/action/EcobeeActions.java#L89

Thanks for your details explanation. I think I now got the point.

Signed-off-by: Christoph Weitkamp <[email protected]>

cpmeister

LGTM

…or heating devices / groups (openhab#8222) * Added ThingActions to handle Boost and Window-Open modes Signed-off-by: Christoph Weitkamp <[email protected]>

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

cweitkamp mentioned this pull request Jul 29, 2020

[avmfritz] Add window-open and boost mode for radiator thermostats #8144

Closed

kaikreuzer reviewed Aug 4, 2020

View reviewed changes

...b.binding.avmfritz/src/main/java/org/openhab/binding/avmfritz/internal/dto/HeatingModel.java Show resolved Hide resolved

cpmeister requested changes Aug 26, 2020

View reviewed changes

kaikreuzer approved these changes Aug 30, 2020

View reviewed changes

kaikreuzer requested a review from cpmeister August 30, 2020 13:15

cpmeister requested changes Aug 31, 2020

View reviewed changes

cweitkamp added 4 commits September 1, 2020 20:55

Added ThingActions to handle Boost and Window-Open modes

f4cd766

Signed-off-by: Christoph Weitkamp <[email protected]>

Expose ThingAction classes / interfaces

bc0ec94

Signed-off-by: Christoph Weitkamp <[email protected]>

Incorporated comments from review

c14fb93

Signed-off-by: Christoph Weitkamp <[email protected]>

Incorporated comments from review

6c820a0

Signed-off-by: Christoph Weitkamp <[email protected]>

cweitkamp force-pushed the feature-avmfritz-heating-modes branch from 2e9ca32 to 6c820a0 Compare September 1, 2020 18:55

cweitkamp added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Sep 2, 2020

cpmeister approved these changes Sep 2, 2020

View reviewed changes

cpmeister merged commit cfd2ae5 into openhab:2.5.x Sep 2, 2020

cpmeister added this to the 2.5.9 milestone Sep 2, 2020

cweitkamp deleted the feature-avmfritz-heating-modes branch September 3, 2020 05:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[avmfritz] Added ThingActions to handle Boost and Window-Open modes for heating devices / groups #8222

[avmfritz] Added ThingActions to handle Boost and Window-Open modes for heating devices / groups #8222

cweitkamp commented Jul 29, 2020

kaikreuzer Aug 4, 2020

cweitkamp Aug 5, 2020 •

edited

Loading

kaikreuzer Aug 5, 2020

cweitkamp Aug 6, 2020

kaikreuzer Aug 25, 2020

cweitkamp Aug 30, 2020

cpmeister Aug 26, 2020

cweitkamp Aug 30, 2020

cpmeister Aug 26, 2020

kaikreuzer Aug 26, 2020

cpmeister Aug 27, 2020

cweitkamp Aug 30, 2020

cpmeister Aug 31, 2020

cpmeister Aug 31, 2020

cweitkamp Sep 1, 2020

cpmeister left a comment

	if (actions.getClass().getName().equals(AVMFritzHeatingActionsHandler.class.getName())) {
	if (actions.getClass().getName().equals(AVMFritzHeatingActions.class.getName())) {

[avmfritz] Added ThingActions to handle Boost and Window-Open modes for heating devices / groups #8222

[avmfritz] Added ThingActions to handle Boost and Window-Open modes for heating devices / groups #8222

Conversation

cweitkamp commented Jul 29, 2020

Choose a reason for hiding this comment

cweitkamp Aug 5, 2020 • edited Loading

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

cpmeister left a comment

Choose a reason for hiding this comment

cweitkamp Aug 5, 2020 •

edited

Loading