Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions addons/binding/org.openhab.binding.knx/META-INF/MANIFEST.MF
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

org.eclipse.smarthome.core.library.items,
org.eclipse.smarthome.core.library.types,
org.eclipse.smarthome.core.net,
org.eclipse.smarthome.core.service,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
import org.eclipse.smarthome.core.items.GenericItem;
import org.eclipse.smarthome.core.types.Type;

import tuwien.auto.calimero.datapoint.Datapoint;
Expand Down Expand Up @@ -50,4 +51,7 @@ public interface KNXTypeMapper {
@Nullable
public Class<? extends Type> toTypeClass(@Nullable String dpt);

@Nullable
public Class<? extends GenericItem> toItemTypeClass(@Nullable Class<? extends @Nullable Type> typeClass);

}
Original file line number Diff line number Diff line change
Expand Up @@ -176,13 +176,17 @@ protected void attachToClient() {
address = new IndividualAddress(config.getAddress());

long pingInterval = config.getPingInterval().longValue();
long initialPingDelay = Math.round(INITIAL_PING_DELAY * random.nextFloat());

ScheduledFuture<?> pollingJob = this.pollingJob;
if ((pollingJob == null || pollingJob.isCancelled())) {
logger.debug("'{}' will be polled every {}s", getThing().getUID(), pingInterval);
this.pollingJob = getBackgroundScheduler().scheduleWithFixedDelay(() -> pollDeviceStatus(),
initialPingDelay, pingInterval, TimeUnit.SECONDS);
if (pingInterval != 0) {
long initialPingDelay = Math.round(INITIAL_PING_DELAY * random.nextFloat());

ScheduledFuture<?> pollingJob = this.pollingJob;
if ((pollingJob == null || pollingJob.isCancelled())) {
logger.debug("'{}' will be polled every {}s", getThing().getUID(), pingInterval);
this.pollingJob = getBackgroundScheduler().scheduleWithFixedDelay(() -> pollDeviceStatus(),
initialPingDelay, pingInterval, TimeUnit.SECONDS);
}
} else {
updateStatus(ThingStatus.ONLINE);
}
} else {
updateStatus(ThingStatus.ONLINE);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,194 @@
/**
* 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.channel;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


import static java.util.stream.Collectors.*;

import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Objects;
import java.util.Set;

import org.eclipse.jdt.annotation.NonNull;
import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
import org.eclipse.smarthome.config.core.Configuration;
import org.eclipse.smarthome.core.types.Type;
import org.openhab.binding.knx.KNXTypeMapper;
import org.openhab.binding.knx.client.InboundSpec;
import org.openhab.binding.knx.client.OutboundSpec;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import tuwien.auto.calimero.GroupAddress;
import tuwien.auto.calimero.KNXFormatException;

/**
* Abstract base class for the meta-data abstraction for the KNX channel configurations.
*
* @author Karel Goderis - Initial contribution
*/
@NonNullByDefault
public abstract class AbstractKNXChannelType {

protected final Set<String> channelTypeIDs;

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

protected AbstractKNXChannelType(String... channelTypeIDs) {
this.channelTypeIDs = new HashSet<>(Arrays.asList(channelTypeIDs));
}

@Nullable
protected abstract ChannelConfiguration parse(@Nullable String fancy);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


public final Set<String> getChannelIDs() {
return channelTypeIDs;
}

protected final Set<String> asSet(String... values) {
return Collections.unmodifiableSet(new HashSet<>(Arrays.asList(values)));
}

protected abstract Set<@NonNull String> getAllGAKeys();

public final Set<GroupAddress> getListenAddresses(Configuration channelConfiguration) {
Set<GroupAddress> ret = new HashSet<>();
for (String key : getAllGAKeys()) {
ChannelConfiguration conf = parse((String) channelConfiguration.get(key));
if (conf != null) {
ret.addAll(conf.getListenGAs().stream().map(this::toGroupAddress).filter(Objects::nonNull)
.collect(toSet()));
}
}
return ret;
}

public final Set<GroupAddress> getReadAddresses(Configuration channelConfiguration) {
Set<GroupAddress> ret = new HashSet<>();
for (String key : getAllGAKeys()) {
ChannelConfiguration conf = parse((String) channelConfiguration.get(key));
if (conf != null) {
ret.addAll(
conf.getReadGAs().stream().map(this::toGroupAddress).filter(Objects::nonNull).collect(toSet()));
}
}
return ret;
}

public final Set<GroupAddress> getWriteAddresses(Configuration channelConfiguration) {
Set<GroupAddress> ret = new HashSet<>();
for (String key : getAllGAKeys()) {
ChannelConfiguration conf = parse((String) channelConfiguration.get(key));
if (conf != null && conf.getMainGA() != null) {
GroupAddress ga = toGroupAddress(conf.getMainGA());
if (ga != null) {
ret.add(ga);
}
}
}
return ret;
}

private @Nullable GroupAddress toGroupAddress(@Nullable GroupAddressConfiguration ga) {
if (ga != null) {
try {
return new GroupAddress(ga.getGA());
} catch (KNXFormatException e) {
logger.warn("Could not parse group address '{}'", ga.getGA());
}
}
return null;
}

protected final Set<GroupAddress> getAddresses(@Nullable Configuration configuration, Iterable<String> addresses)
throws KNXFormatException {
Set<GroupAddress> ret = new HashSet<>();
for (String address : addresses) {
if (configuration != null && configuration.get(address) != null) {
ret.add(new GroupAddress((String) configuration.get(address)));
}
}
return ret;
}

protected final boolean isEquals(@Nullable Configuration configuration, String address, GroupAddress groupAddress)
throws KNXFormatException {
if (configuration != null && configuration.get(address) != null) {
return Objects.equals(new GroupAddress((String) configuration.get(address)), groupAddress);
}
return false;
}

public final @Nullable OutboundSpec getCommandSpec(Configuration configuration, KNXTypeMapper typeHelper,
@Nullable Type command) throws KNXFormatException {
for (String key : getAllGAKeys()) {
ChannelConfiguration config = parse((String) configuration.get(key));
if (config != null) {
String dpt = config.getDPT();
if (dpt == null) {
dpt = getDefaultDPT(key);
}

Class<? extends Type> expectedTypeClass = typeHelper.toTypeClass(dpt);
if (expectedTypeClass != null) {
logger.trace("The expected Type class for dpt '{}' is '{}'", dpt,
expectedTypeClass.getSimpleName());
Type convertedType = convertType(command, configuration);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (convertedType != null) {
logger.trace("The converted type for command '{}' based on config '{}' is '{}' ('{}')", command,
configuration.get(key), convertedType, convertedType.getClass().getSimpleName());
if (expectedTypeClass.isInstance(convertedType)) {
logger.trace("The expected Type '{}' is an instance of the converted Type '{}'",
expectedTypeClass.getSimpleName(), convertedType.getClass().getSimpleName());
return new WriteSpecImpl(config, dpt, convertedType);
} else {
logger.trace("The expected Type '{}' is NOT an instance of the converted Type '{}'",
expectedTypeClass.getSimpleName(), convertedType.getClass().getSimpleName());
}
}
}
}
}
return null;
}

public final List<InboundSpec> getReadSpec(Configuration configuration) throws KNXFormatException {
return getAllGAKeys().stream()
.map(key -> new ReadRequestSpecImpl(parse((String) configuration.get(key)), getDefaultDPT(key)))
.filter(spec -> !spec.getGroupAddresses().isEmpty()).collect(toList());
}

public final @Nullable InboundSpec getListenSpec(Configuration configuration, @Nullable GroupAddress groupAddress) {
return getAllGAKeys().stream()
.map(key -> new ListenSpecImpl(parse((String) configuration.get(key)), getDefaultDPT(key)))
.filter(spec -> !spec.getGroupAddresses().isEmpty())
.filter(spec -> spec.getGroupAddresses().contains(groupAddress)).findFirst().orElse(null);
}

protected abstract String getDefaultDPT(String gaConfigKey);

public final @Nullable OutboundSpec getResponseSpec(Configuration configuration, GroupAddress groupAddress,
Type type) throws KNXFormatException {
return getAllGAKeys().stream()
.map(key -> new ReadResponseSpecImpl(parse((String) configuration.get(key)), getDefaultDPT(key), type))
.filter(spec -> groupAddress.equals(spec.getGroupAddress())).findFirst().orElse(null);
}

protected @Nullable Type convertType(@Nullable Type type, Configuration channelConfiguration) {
return type;
}

@Override
public String toString() {
return channelTypeIDs.toString();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,13 @@ protected AbstractSpec(@Nullable ChannelConfiguration channelConfiguration, Stri
* @param ga the group address configuration
* @return a group address object
*/
protected final GroupAddress toGroupAddress(GroupAddressConfiguration ga) {
protected final GroupAddress toGroupAddress(@Nullable GroupAddressConfiguration ga) {
try {
return new GroupAddress(ga.getGA());
if (ga != null) {
return new GroupAddress(ga.getGA());
} else {
throw new IllegalArgumentException("GroupAddressConfiguration should contain a group address");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

}
} catch (KNXFormatException e) {
throw new IllegalArgumentException(e);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import static java.util.stream.Collectors.toList;

import java.util.List;
import java.util.Objects;
import java.util.stream.Stream;

import org.eclipse.jdt.annotation.NonNullByDefault;
Expand All @@ -26,10 +27,10 @@
public class ChannelConfiguration {

private final @Nullable String dpt;
private final GroupAddressConfiguration mainGA;
private final @Nullable GroupAddressConfiguration mainGA;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

private final List<GroupAddressConfiguration> listenGAs;

public ChannelConfiguration(@Nullable String dpt, GroupAddressConfiguration mainGA,
public ChannelConfiguration(@Nullable String dpt, @Nullable GroupAddressConfiguration mainGA,
List<GroupAddressConfiguration> listenGAs) {
this.dpt = dpt;
this.mainGA = mainGA;
Expand All @@ -40,12 +41,12 @@ public ChannelConfiguration(@Nullable String dpt, GroupAddressConfiguration main
return dpt;
}

public GroupAddressConfiguration getMainGA() {
public @Nullable GroupAddressConfiguration getMainGA() {
return mainGA;
}

public List<GroupAddressConfiguration> getListenGAs() {
return Stream.concat(Stream.of(mainGA), listenGAs.stream()).collect(toList());
return Stream.concat(Stream.of(mainGA), listenGAs.stream()).filter(Objects::nonNull).collect(toList());
}

public List<GroupAddressConfiguration> getReadGAs() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,9 @@ public boolean isRead() {
return read;
}

@Override
public String toString() {
return "[ga= " + ga + ", read=" + read + "]";
}

}
Loading