Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

Changes to the Tradfri Binding to discover and use Groups #3799

Closed
wants to merge 4 commits into from
Closed

Changes to the Tradfri Binding to discover and use Groups #3799

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
594aea4
Changes to the Tradfri Binding to discover and use Groups (and additi…
Jul 4, 2017
de202b4
Couple of changes requested by sjka under id: #3799
S0urceror Jul 9, 2017
a0688bc
Merge https://github.com/S0urceror/smarthome
S0urceror Jul 22, 2017
0b6b329
Merge branch 'master' of https://github.com/eclipse/smarthome
S0urceror Aug 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 27 additions & 1 deletion extensions/binding/org.eclipse.smarthome.binding.tradfri/ESH-INF/thing/thing-types.xml
View file
Open in desktop
Expand Up @@ -69,6 +69,26 @@
</parameter>
</config-description>
</thing-type>
<thing-type id="group">
Copy link
Contributor

Choose a reason for hiding this comment

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

Do groups really differ significantly from lamps (apart from their URL & ID, of course)? Or could they rather be considered as a single lamp? What I want to say with it: Do we really need a dedicated thing-type for them? (And this really is meant to be an open question)

Copy link
Author

Choose a reason for hiding this comment

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

Hi sjka,

I noticed different behaviours (which I will retest) from groups vs lights. In a group with 1 light the remote switches on the group and light within (which changes the brightness). If the brightness of the light is changed I don't see the group brightness changing. Maybe group brightness is only a write only variable. I will check and come back to this.

Copy link
Author

Choose a reason for hiding this comment

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

Confirmed. Group brightness is a write-only variable. Setting it causes all lights to update it's brightness. After this it stays at this value even though we set all lights at another brightness value.

Don't know if we should include it if it has such strange behaviour. What do you think?

<supported-bridge-type-refs>
<bridge-type-ref id="gateway"/>
</supported-bridge-type-refs>

<label>Tradfri Group</label>
<description>A group of Tradfri lamps</description>

<channels>
<channel id="group_state" typeId="group_state"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

do groups really only have an on/off feature, or could we rather reuse the existing brightness channel here? I noticed the return value also includes the "5851" (aka DIMMER) field.

Copy link
Author

Choose a reason for hiding this comment

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

I'll test this.

Copy link
Author

Choose a reason for hiding this comment

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

It's write-only. Don't know if we should include the brightness channel if it behaves like this.

</channels>

<config-description>
<parameter name="id" type="integer" required="true">
<label>ID</label>
<description>The identifier of the group on the gateway</description>
</parameter>
</config-description>

</thing-type>

<!-- note that this isn't yet supported by the code as we do not receive any data from the gateway for it -->
<thing-type id="0820" listed="false">
Expand Down Expand Up @@ -109,5 +129,11 @@
<description>Allows to control the color temperature of light.</description>
<category>ColorLight</category>
</channel-type>


<channel-type id="group_state">
Copy link
Contributor

Choose a reason for hiding this comment

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

If there are constellations which don't expose a dimmer (5851) but only an ON/OFF (5850) so that this is really needed, I would suggest keeping it group-agnostic, i.e. name it switch and remove all the references to groups from label & description, so that it can be used also directly in case they ever decide to offer a non-dimmable lamp.

Copy link
Author

Choose a reason for hiding this comment

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

I would advocate against this.

Currently Groups are a collection of lamps, a remote and/or a IR switch. And currently not all info of the group is exposed thought the gateway.

We don't know what devices Ikea will launch in the future but I heard rumours about mains-switches and colour bulbs. And hopefully they will also correct the problem of directly listening to the remote device instead of checking the group status.

I am sure this will change the behaviour of Groups extending their current switch behaviour. Remotes will become Switches and Dimmers exposed through the GW.

<item-type>Switch</item-type>
<label>Group State</label>
<description>Allows to switch on or off all lamps in a group.</description>
</channel-type>

</thing:thing-descriptions>
3 changes: 3 additions & 0 deletions ....tradfri/src/main/java/org/eclipse/smarthome/binding/tradfri/TradfriBindingConstants.java
View file
Open in desktop
Expand Up @@ -29,6 +29,8 @@ public class TradfriBindingConstants {
public final static ThingTypeUID THING_TYPE_DIMMABLE_LIGHT = new ThingTypeUID(BINDING_ID, "0100");
public final static ThingTypeUID THING_TYPE_COLOR_TEMP_LIGHT = new ThingTypeUID(BINDING_ID, "0220");
public final static ThingTypeUID THING_TYPE_DIMMER = new ThingTypeUID(BINDING_ID, "0820");
// Grouped lights (and maybe in the future other devices)
public final static ThingTypeUID THING_TYPE_GROUP = new ThingTypeUID(BINDING_ID, "group");

public static final Set<ThingTypeUID> SUPPORTED_LIGHT_TYPES_UIDS = ImmutableSet.of(THING_TYPE_DIMMABLE_LIGHT,
THING_TYPE_COLOR_TEMP_LIGHT);
Expand All @@ -39,6 +41,7 @@ public class TradfriBindingConstants {
// List of all Channel IDs
public static final String CHANNEL_BRIGHTNESS = "brightness";
public static final String CHANNEL_COLOR_TEMPERATURE = "color_temperature";
public static final String CHANNEL_GROUP_STATE = "group_state";

// IPSO Objects
public static final String DEVICES = "15001";
Expand Down
182 changes: 106 additions & 76 deletions ...ri/src/main/java/org/eclipse/smarthome/binding/tradfri/handler/TradfriGatewayHandler.java
View file
Open in desktop
Expand Up @@ -7,7 +7,7 @@
*/
package org.eclipse.smarthome.binding.tradfri.handler;

import static org.eclipse.smarthome.binding.tradfri.TradfriBindingConstants.DEVICES;
import static org.eclipse.smarthome.binding.tradfri.TradfriBindingConstants.*;

import java.io.IOException;
import java.net.InetSocketAddress;
Expand Down Expand Up @@ -48,13 +48,13 @@
* sent to one of the channels.
*
* @author Kai Kreuzer - Initial contribution
* @author Mario Smit - Group Handler added
*/
public class TradfriGatewayHandler extends BaseBridgeHandler implements CoapCallback {
public class TradfriGatewayHandler extends BaseBridgeHandler {

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

private TradfriCoapClient deviceClient;
private String gatewayURI;
public MyCoapCallback devices, groups;
private DTLSConnector dtlsConnector;
private CoapEndpoint endPoint;

Expand Down Expand Up @@ -85,27 +85,44 @@ public void initialize() {
return;
}

this.gatewayURI = "coaps://" + configuration.host + ":" + configuration.port + "/" + DEVICES;
DtlsConnectorConfig.Builder builder = new DtlsConnectorConfig.Builder(new InetSocketAddress(0));
builder.setPskStore(new StaticPskStore("", configuration.code.getBytes()));
dtlsConnector = new DTLSConnector(builder.build());
endPoint = new CoapEndpoint(dtlsConnector, NetworkConfig.getStandard());

// setup DEVICES scanner
devices = new MyCoapCallback();
devices.gatewayURI = "coaps://" + configuration.host + ":" + configuration.port + "/" + DEVICES;
try {
URI uri = new URI(gatewayURI);
deviceClient = new TradfriCoapClient(uri);
URI uri = new URI(devices.gatewayURI);
devices.client = new TradfriCoapClient(uri);
} catch (URISyntaxException e) {
logger.debug("Illegal gateway URI `{}`: {}", gatewayURI, e.getMessage());
logger.debug("Illegal gateway URI `{}`: {}", devices.gatewayURI, e.getMessage());
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, e.getMessage());
return;
}
devices.client.setEndpoint(endPoint);

// setup GROUPS scanner
Copy link
Contributor

Choose a reason for hiding this comment

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

As this more or less duplicates the code from above, could you please factor out the common parts into a separate method?

Copy link
Author

Choose a reason for hiding this comment

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

done

groups = new MyCoapCallback();
groups.gatewayURI = "coaps://" + configuration.host + ":" + configuration.port + "/" + GROUPS;
try {
URI uri = new URI(groups.gatewayURI);
groups.client = new TradfriCoapClient(uri);
} catch (URISyntaxException e) {
logger.debug("Illegal gateway URI `{}`: {}", groups.gatewayURI, e.getMessage());
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, e.getMessage());
return;
}
groups.client.setEndpoint(endPoint);

DtlsConnectorConfig.Builder builder = new DtlsConnectorConfig.Builder(new InetSocketAddress(0));
builder.setPskStore(new StaticPskStore("", configuration.code.getBytes()));
dtlsConnector = new DTLSConnector(builder.build());
endPoint = new CoapEndpoint(dtlsConnector, NetworkConfig.getStandard());
deviceClient.setEndpoint(endPoint);
updateStatus(ThingStatus.UNKNOWN);

// schedule a new scan every minute
scanJob = scheduler.scheduleWithFixedDelay(() -> {
startScan();
}, 0, 1, TimeUnit.MINUTES);
devices.startScan();
groups.startScan();
}, 0, 60, TimeUnit.SECONDS);
}

@Override
Expand All @@ -114,9 +131,13 @@ public void dispose() {
scanJob.cancel(true);
scanJob = null;
}
if (deviceClient != null) {
deviceClient.shutdown();
deviceClient = null;
if (devices.client != null) {
devices.client.shutdown();
devices.client = null;
}
if (groups.client != null) {
groups.client.shutdown();
groups.client = null;
}
if (endPoint != null) {
endPoint.destroy();
Expand All @@ -125,74 +146,83 @@ public void dispose() {
super.dispose();
}

/**
* Does a request to the gateway to list all available devices/services.
* The response is received and processed by the method {@link onUpdate(JsonElement data)}.
*/
public void startScan() {
if (endPoint != null) {
deviceClient.get(new TradfriCoapHandler(this));
public class MyCoapCallback implements CoapCallback {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about naming it TradfriCoapCallback?

Copy link
Author

Choose a reason for hiding this comment

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

done

private TradfriCoapClient client;
private String gatewayURI;

MyCoapCallback() {
Copy link
Contributor

Choose a reason for hiding this comment

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

As this is empty, there is no need to implement it.

Copy link
Author

Choose a reason for hiding this comment

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

done


}
}

/**
* Returns the root URI of the gateway.
*
* @return root URI of the gateway with coaps scheme
*/
public String getGatewayURI() {
return gatewayURI;
}
/**
* Does a request to the gateway to list all available devices/services.
* The response is received and processed by the method {@link onUpdate(JsonElement data)}.
*/
public void startScan() {
if (endPoint != null) {
client.get(new TradfriCoapHandler(this));
}
}

/**
* Returns the coap endpoint that can be used within coap clients.
*
* @return the coap endpoint
*/
public CoapEndpoint getEndpoint() {
return endPoint;
}
/**
* Returns the root URI of the gateway.
*
* @return root URI of the gateway with coaps scheme
*/
public String getGatewayURI() {
return gatewayURI;
}

@Override
public void onUpdate(JsonElement data) {
logger.trace("Response: {}", data);
/**
* Returns the coap endpoint that can be used within coap clients.
*
* @return the coap endpoint
*/
public CoapEndpoint getEndpoint() {
return endPoint;
}

if (endPoint != null) {
try {
JsonArray array = data.getAsJsonArray();
for (int i = 0; i < array.size(); i++) {
requestDeviceDetails(array.get(i).getAsString());
@Override
public void onUpdate(JsonElement data) {
logger.debug("Gateway response: {}", data);

if (endPoint != null) {
try {
JsonArray array = data.getAsJsonArray();
for (int i = 0; i < array.size(); i++) {
requestDeviceDetails(array.get(i).getAsString());
}
} catch (JsonSyntaxException e) {
logger.debug("JSON error: {}", e.getMessage());
setStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR);
}
} catch (JsonSyntaxException e) {
logger.debug("JSON error: {}", e.getMessage());
setStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR);
}
}
}

private void requestDeviceDetails(String instanceId) {
// we are reusing our coap client and merely temporarily set a sub-URI to call
deviceClient.setURI(gatewayURI + "/" + instanceId);
deviceClient.asyncGet().thenAccept(data -> {
logger.debug("Response: {}", data);
JsonObject json = new JsonParser().parse(data).getAsJsonObject();
deviceUpdateListeners.forEach(listener -> listener.onUpdate(instanceId, json));
});
// restore root URI
deviceClient.setURI(gatewayURI);
}
private void requestDeviceDetails(String instanceId) {
// we are reusing our coap client and merely temporarily set a sub-URI to call
client.setURI(gatewayURI + "/" + instanceId);
client.asyncGet().thenAccept(data -> {
logger.debug("Response: {}", data);
JsonObject json = new JsonParser().parse(data).getAsJsonObject();
deviceUpdateListeners.forEach(listener -> listener.onUpdate(instanceId, json));
});
// restore root URI
client.setURI(gatewayURI);
}

@Override
public void setStatus(ThingStatus status, ThingStatusDetail statusDetail) {
// are we still connected at all?
if (endPoint != null) {
updateStatus(status, statusDetail);
if (dtlsConnector != null && status == ThingStatus.OFFLINE) {
try {
dtlsConnector.stop();
dtlsConnector.start();
} catch (IOException e) {
logger.debug("Error restarting the DTLS connector: {}", e.getMessage());
@Override
public void setStatus(ThingStatus status, ThingStatusDetail statusDetail) {
// are we still connected at all?
if (endPoint != null) {
updateStatus(status, statusDetail);
if (dtlsConnector != null && status == ThingStatus.OFFLINE) {
try {
dtlsConnector.stop();
dtlsConnector.start();
} catch (IOException e) {
logger.debug("Error restarting the DTLS connector: {}", e.getMessage());
}
}
}
}
Expand Down