[knx] Mods to enable the KNX ‘ETS’ Binding #3500

kgoderis · 2018-04-19T17:16:36Z

This PR contains all the changes to the KNX Basic binding that are required by https://github.com/openhab/openhab2-addons/pull/3451.

Signed-Off-By: Karel Goderis [email protected]

wborn

Here are a few comments while I casually scrolled through the code. :-)

wborn · 2018-04-20T14:25:46Z

...nding.knx/src/main/java/org/openhab/binding/knx/internal/channel/AbstractKNXChannelType.java

+        return Collections.unmodifiableSet(new HashSet<>(Arrays.asList(values)));
+    }
+
+    protected abstract @NonNull Set<@NonNull String> getAllGAKeys();


You should annotate the class with @NonNullByDefault and use @Nullable where needed.

See also the ESH documentation.

wborn · 2018-04-20T14:28:29Z

...penhab.binding.knx/src/main/java/org/openhab/binding/knx/internal/dpt/KNXCoreTypeMapper.java

+    /** stores the default Item Type to use for each openHAB type */
+    private final Map<Class<? extends Type>, Class<? extends GenericItem>> defaulItemTypeMap;
+
+    private HashMap<Class<? extends GenericItem>, String> itemTypeStringMap;


Can you change the type from HashMap to Map here?

wborn · 2018-04-20T14:29:21Z

...penhab.binding.knx/src/main/java/org/openhab/binding/knx/internal/dpt/KNXCoreTypeMapper.java

@@ -116,6 +131,9 @@ public KNXCoreTypeMapper() {

        dptTypeMap = new HashMap<String, Class<? extends Type>>();
        dptMainTypeMap = new HashMap<Integer, Class<? extends Type>>();
+        defaultDptMap = new HashMap<Class<? extends Type>, String>();
+        defaulItemTypeMap = new HashMap<Class<? extends Type>, Class<? extends GenericItem>>();
+        itemTypeStringMap = new HashMap<Class<? extends GenericItem>, String>();


Can you use the diamond operator here?

wborn · 2018-04-21T07:03:06Z

...rg.openhab.binding.knx/src/main/java/org/openhab/binding/knx/internal/handler/LoadState.java

+ *
+ * @author Karel Goderis - Initial contribution
+ */
+public enum LoadState {


Is this Enum used? I couldn't find any references to it.

kgoderis · 2018-04-21T07:23:44Z

I re-added this as it got lost during the refactoring into the basic binding. There are a few other classes that are part of the knx spec that are already in the basic binding and that will serve for any follow up binding

…

Sent from my iPhone

On 21 Apr 2018, at 09:03, Wouter Born ***@***.***> wrote: @wborn commented on this pull request. In addons/binding/org.openhab.binding.knx/src/main/java/org/openhab/binding/knx/internal/handler/LoadState.java: > +/** + * Copyright (c) 2010-2018 by the respective copyright holders. + * + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License v1.0 + * which accompanies this distribution, and is available at + * http://www.eclipse.org/legal/epl-v10.html + */ +package org.openhab.binding.knx.internal.handler; + +/** + * Enumeration containing the load states + * + * @author Karel Goderis - Initial contribution + */ +public enum LoadState { Is this Enum used? I couldn't find any references to it. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

wborn · 2018-04-21T07:32:15Z

...rg.openhab.binding.knx/src/main/java/org/openhab/binding/knx/internal/handler/LoadState.java

+    L5(5, "Load Completing");
+
+    private int code;
+    private String name;


@kgoderis I was searching for uses of this Enum to find out how the name property was used. The name property and getName(..) methods of this Enum are confusing because an Enum already has a .name() method. Maybe the property can be renamed to something else so there will be no confusion?

Signed-off-by: Karel Goderis <[email protected]>

kgoderis · 2018-05-06T10:35:23Z

@SJKA Simon, any thoughts on this PR?

sjsf · 2018-05-17T14:30:37Z

...penhab.binding.knx/src/main/java/org/openhab/binding/knx/internal/dpt/KNXCoreTypeMapper.java

+        return defaulItemTypeMap.get(toTypeClass(dpt));
+    }
+
+    public String toItemType(String dpt) {


This is neither part of the public API (i.e. interface) nor is it used internally. Therefore this method is dead code and can be deleted.

It is used in the advanced binding, and in that context its natural place is in the KNXCoreTypeMapper (it can be potentially be used by other knx binding derivaties), but if it is really an issue I will put it in an inheriting class.

sjsf · 2018-05-17T14:31:08Z

...penhab.binding.knx/src/main/java/org/openhab/binding/knx/internal/dpt/KNXCoreTypeMapper.java

+        return defaulItemTypeMap.get(typeClass);
+    }
+
+    public Class<? extends GenericItem> toItemTypeClass(String dpt) {


dead code - see below

sjsf · 2018-05-17T14:33:38Z

...penhab.binding.knx/src/main/java/org/openhab/binding/knx/internal/dpt/KNXCoreTypeMapper.java

@@ -534,6 +551,25 @@ public KNXCoreTypeMapper() {
        defaultDptMap.put(DateTimeType.class, DPTXlatorTime.DPT_TIMEOFDAY.getID());
        defaultDptMap.put(StringType.class, DPTXlatorString.DPT_STRING_8859_1.getID());
        defaultDptMap.put(HSBType.class, DPTXlatorRGB.DPT_RGB.getID());
+
+        defaulItemTypeMap.put(DateTimeType.class, DateTimeItem.class);


You are effectively duplicating (partially) the information from acceptedItemTypes/acceptedCommandTypes here. Not sure if that's a good idea...

In the advanced binding I do implement I use a ChannelBuilder to create Channels based on the information stored in the ETS5 files. ChannelBuilder::create() takes an acceptedItemType as input, which is provided by the defaulItemTypeMap you mention, and for which in turn the Key is provided by KNXCoreTypeMapper::toTypeClass() based on the DPT that is derived from the ETS5 data.

sjsf · 2018-05-17T14:36:20Z

addons/binding/org.openhab.binding.knx/META-INF/MANIFEST.MF

@@ -11,6 +11,8 @@ Import-Package: gnu.io;resolution:=optional,
 org.eclipse.smarthome.config.discovery,
 org.eclipse.smarthome.core.common,
 org.eclipse.smarthome.core.i18n,
+ org.eclipse.smarthome.core.items,


I think we had this discussion a couple of times already: Bindings must be agnostic of items. Otherwise the whole functional abstraction won't work. So I hope you'll understand this won't pass without a single word of explanation?

See https://github.com/openhab/openhab2-addons/pull/3500#discussion_r189028969

I do not use Items directly, but I need their class definitions for the TypeMapper.

sjsf · 2018-05-17T14:40:02Z

...nding.knx/src/main/java/org/openhab/binding/knx/internal/channel/AbstractKNXChannelType.java

+    }
+
+    @Nullable
+    protected abstract ChannelConfiguration parse(@Nullable String fancy);


The ChannelConfiguration class is something internal to the "basic" KNX binding and therefore must not "leak" out of the API.

Most of the methods below have the same "issue": They are specific to the way the basic binding configures channels and therefore cannot reside in a commonly shared abstract class.

My main objective was to re-use as much as possible existing classes without re-inventing the wheel. The only part that is specific for each binding is the parsing of the configuration String. In fact, in the advanced binding I do construct a configuration String based on ETS5 information that is similar (but not the same) as the basic binding's configuration Strings

sjsf · 2018-05-17T14:40:32Z

...nding.knx/src/main/java/org/openhab/binding/knx/internal/channel/AbstractKNXChannelType.java

+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ */
+package org.openhab.binding.knx.internal.channel;


If you plan to extend it, then this must not be in an "internal" package. I don't think though this is a good idea at all (see below).

sjsf · 2018-05-17T14:42:40Z

...openhab.binding.knx/src/main/java/org/openhab/binding/knx/internal/channel/AbstractSpec.java

+            if (ga != null) {
+                return new GroupAddress(ga.getGA());
+            } else {
+                throw new IllegalArgumentException("GroupAddressConfiguration should contain a group address");


Why did you change the signature to be @Nullable and then throw a IAE in case null is passed?

I should revert this and do the null check before calling this. Underlying to this is that in the advanced binding there can be ChannelConfigurations that do not have a MainGA (toGroupAddress is called in ReadResponseSpecImpl)

sjsf · 2018-05-17T14:44:50Z

...binding.knx/src/main/java/org/openhab/binding/knx/internal/channel/ChannelConfiguration.java

@@ -26,10 +27,10 @@
 public class ChannelConfiguration {

    private final @Nullable String dpt;
-    private final GroupAddressConfiguration mainGA;
+    private final @Nullable GroupAddressConfiguration mainGA;


No, that's a pretty fundamental assumption: the first one is the "main" GA - and IF there is a configuration, it MUST contain an address.

Are you maybe changing the meaning of "main GA"? Then please don't do so for the basic binding.

Actually, I don't think it is a good idea to reuse this class. Advanced binding(s) may make their own choices how a channels is configured. That's why this is in an "internal" package - it is internal to the basic binding.

mmh.. It is related to https://github.com/openhab/openhab2-addons/pull/3500#discussion_r189029968 . Maybe some of the internal classes of the basic binding should be exposed in order to facilitate re-usage elsewhere.

sjsf · 2018-05-17T14:56:55Z

...nding.knx/src/main/java/org/openhab/binding/knx/internal/channel/AbstractKNXChannelType.java

+                if (expectedTypeClass != null) {
+                    logger.trace("The expected Type class for dpt '{}' is '{}'", dpt,
+                            expectedTypeClass.getSimpleName());
+                    Type convertedType = convertType(command, configuration);


How is this related to the "ETS" binding? Aren't you changing the basic binding's behavior here?

Don't get me wrong, potentially this makes sense. I'm just not quite sure yet about the side-effects this might have. In any case, I'm just trying to understand the reason for this change.

For the basic binding this function returns the Type is was fed without any modification. In the advanced binding I overload it to do something meaningful. See for example TypeRollerShutter.java on how I do use this functionality.

sjsf · 2018-05-17T15:01:58Z

...penhab.binding.knx/src/main/java/org/openhab/binding/knx/internal/channel/WriteSpecImpl.java

@@ -26,12 +28,13 @@
    private final Type type;
    private final @Nullable GroupAddress groupAddress;

+    private final Logger logger = LoggerFactory.getLogger(WriteSpecImpl.class);


what do you need this one for?

Debugging artefact. It should be removed indeed

kaikreuzer · 2018-08-17T05:20:46Z

@kgoderis & @SJKA Where are we here? Is it right that we are waiting for some changes to be pushed by @kgoderis?

lewie · 2018-12-03T13:35:44Z

@kgoderis, the review of sjka is complete. I personally think that without extensions in the basic binding, we will not get along. Duplication of source code is much more complicated and error-prone.

I would suggest to mark the parts required for extensions. A short Comment with the reason in the places as a dead code appear and in the classes which should not be internal.

That would be the easiest way to get ahead?
@SJKA, would that be a solution for your comments?

In order to be able to abstract the 'basic' knx binding better from extended bindings, it must be clarified which classes should be internally which should be available as extensions. This is a copy of kgoderis PR 'knx-api' # 3500. It is the first step, further delimitations come in another PR. Signed-off-by: lewie <[email protected]>

vguna · 2019-08-11T07:49:48Z

Any news on this? This would be a really nice feature for the KNX integration into OH!

Hilbrand · 2020-06-28T20:29:35Z

Given the discussion here it seems a valuable addition. But due too long time no activity closing this pr. if someone wants to pick this up please open a new pr.

kaikreuzer requested a review from sjsf April 19, 2018 18:18

kgoderis force-pushed the knx-api branch from e3471e9 to c60e65a Compare April 20, 2018 13:46

wborn reviewed Apr 20, 2018

View reviewed changes

wborn reviewed Apr 21, 2018

View reviewed changes

[KNX] Mods to enable the KNX ‘ETS’ Binding

cb1ee29

Signed-off-by: Karel Goderis <[email protected]>

kgoderis force-pushed the knx-api branch from c60e65a to cb1ee29 Compare April 23, 2018 18:58

sjsf reviewed May 17, 2018

View reviewed changes

sjsf mentioned this pull request May 17, 2018

[knx] Extension of the KNX Binding with an ETS knxproj parser #3451

Closed

kgoderis mentioned this pull request Jun 15, 2018

[knx] Performance problems in high-load environment #3664

Closed

wborn added the knx label Aug 15, 2018

wborn mentioned this pull request Sep 26, 2018

[knx] Made constants and handler packages internal #4048

Merged

wborn changed the title ~~[KNX] Mods to enable the KNX ‘ETS’ Binding~~ [knx] Mods to enable the KNX ‘ETS’ Binding Dec 19, 2018

davidgraeff added the discussion label Mar 24, 2019

kaikreuzer removed the binding: knx label Nov 27, 2019

Hilbrand added the stale As soon as a PR is marked stale, it can be removed 6 months later. label Jan 22, 2020

Hilbrand closed this Jun 28, 2020

kgoderis deleted the knx-api branch October 29, 2020 16:37

kgoderis mentioned this pull request Jun 16, 2023

[knx] java.lang.ClassFormatError: Unexpected error from weaving hook. #15099

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[knx] Mods to enable the KNX ‘ETS’ Binding #3500

[knx] Mods to enable the KNX ‘ETS’ Binding #3500

kgoderis commented Apr 19, 2018

wborn left a comment

wborn Apr 20, 2018

wborn Apr 20, 2018

wborn Apr 20, 2018

wborn Apr 21, 2018

kgoderis commented Apr 21, 2018 via email

wborn Apr 21, 2018

kgoderis commented May 6, 2018

sjsf May 17, 2018

kgoderis May 17, 2018

sjsf May 17, 2018

sjsf May 17, 2018

kgoderis May 17, 2018

sjsf May 17, 2018

kgoderis May 17, 2018

sjsf May 17, 2018

kgoderis May 17, 2018

sjsf May 17, 2018

sjsf May 17, 2018

kgoderis May 17, 2018

sjsf May 17, 2018

kgoderis May 17, 2018

sjsf May 17, 2018

kgoderis May 17, 2018

sjsf May 17, 2018

kgoderis May 17, 2018

kaikreuzer commented Aug 17, 2018

lewie commented Dec 3, 2018

vguna commented Aug 11, 2019

Hilbrand commented Jun 28, 2020

[knx] Mods to enable the KNX ‘ETS’ Binding #3500

[knx] Mods to enable the KNX ‘ETS’ Binding #3500

Conversation

kgoderis commented Apr 19, 2018

wborn 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

kgoderis commented Apr 21, 2018 via email

Choose a reason for hiding this comment

kgoderis commented May 6, 2018

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

kaikreuzer commented Aug 17, 2018

lewie commented Dec 3, 2018

vguna commented Aug 11, 2019

Hilbrand commented Jun 28, 2020