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.

Sign up for GitHub

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

[ecobee] Rework discovery #9522

Merged
merged 4 commits into from
Dec 30, 2020
Merged

[ecobee] Rework discovery #9522

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
98f421c
Rework discovery
mhilbush Dec 25, 2020
cdca202
Consolidate discovery into one service
mhilbush Dec 25, 2020
5546563
Address review feedback
mhilbush Dec 27, 2020
89f444a
Address review feedback
mhilbush Dec 28, 2020
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
1 change: 0 additions & 1 deletion bundles/org.openhab.binding.ecobee/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,6 @@ The following configuration parameters are available on the Ecobee Account:
| refreshIntervalQuick | Integer | Required | Specifies the interval in seconds with which the Ecobee data will be updated after sending an update or executing a function. |
| apiTimeout | Integer | Required | Time in seconds to allow an API request against the Ecobee servers to complete. |
| discoveryEnabled | Switch | Required | Specifies whether the binding should auto-discover thermostats and remote sensors. |
| discoveryInterval | Integer | Optional | Specifies time interval in seconds in which the binding will attempt to discover thermostats. |

### Ecobee Thermostat

Expand Down
9 changes: 9 additions & 0 deletions ...ding.ecobee/src/main/java/org/openhab/binding/ecobee/internal/EcobeeBindingConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,19 @@ public class EcobeeBindingConstants {
public static final Set<ThingTypeUID> SUPPORTED_SENSOR_THING_TYPES_UIDS = Collections
.unmodifiableSet(Stream.of(UID_SENSOR_THING).collect(Collectors.toSet()));

// Collection of thermostat and sensor thing types
public static final Set<ThingTypeUID> SUPPORTED_THERMOSTAT_AND_SENSOR_THING_TYPES_UIDS = Stream
.concat(SUPPORTED_THERMOSTAT_BRIDGE_THING_TYPES_UIDS.stream(), SUPPORTED_SENSOR_THING_TYPES_UIDS.stream())
.collect(Collectors.toSet());

// Collection of all supported thing types
public static final Set<ThingTypeUID> SUPPORTED_THING_TYPES_UIDS = Collections.unmodifiableSet(
Stream.of(UID_ACCOUNT_BRIDGE, UID_THERMOSTAT_BRIDGE, UID_SENSOR_THING).collect(Collectors.toSet()));

// Background discovery frequency
public static final int DISCOVERY_INTERVAL_SECONDS = 300;
public static final int DISCOVERY_INITIAL_DELAY_SECONDS = 10;

// Thermostat bridge and remote sensor thing config parameters
public static final String CONFIG_THERMOSTAT_ID = "thermostatId";
public static final String CONFIG_SENSOR_ID = "sensorId";
Expand Down
5 changes: 0 additions & 5 deletions .../src/main/java/org/openhab/binding/ecobee/internal/config/EcobeeAccountConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,4 @@ public class EcobeeAccountConfiguration {
* Enable/disable automatic discovery
*/
public @Nullable Boolean discoveryEnabled;

/**
* Interval with which to run the thermostat discovery process
*/
public @Nullable Integer discoveryInterval;
}
182 changes: 182 additions & 0 deletions ...e/src/main/java/org/openhab/binding/ecobee/internal/discovery/EcobeeDiscoveryService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,182 @@
/**
* Copyright (c) 2010-2020 Contributors to the openHAB project
*
* See the NOTICE file(s) distributed with this work for additional
* information.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License 2.0 which is available at
* http://www.eclipse.org/legal/epl-2.0
*
* SPDX-License-Identifier: EPL-2.0
*/
package org.openhab.binding.ecobee.internal.discovery;

import static org.openhab.binding.ecobee.internal.EcobeeBindingConstants.*;

import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
import org.openhab.binding.ecobee.internal.dto.thermostat.RemoteSensorDTO;
import org.openhab.binding.ecobee.internal.dto.thermostat.ThermostatDTO;
import org.openhab.binding.ecobee.internal.handler.EcobeeAccountBridgeHandler;
import org.openhab.binding.ecobee.internal.handler.EcobeeThermostatBridgeHandler;
import org.openhab.core.config.discovery.AbstractDiscoveryService;
import org.openhab.core.config.discovery.DiscoveryResult;
import org.openhab.core.config.discovery.DiscoveryResultBuilder;
import org.openhab.core.thing.Thing;
import org.openhab.core.thing.ThingStatus;
import org.openhab.core.thing.ThingTypeUID;
import org.openhab.core.thing.ThingUID;
import org.openhab.core.thing.binding.ThingHandler;
import org.openhab.core.thing.binding.ThingHandlerService;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
* The {@link EcobeeDiscoveryService} is responsible for discovering the Ecobee
* thermostats that are associated with the Ecobee Account, as well as the sensors
* are associated with the Ecobee thermostats.
*
* @author Mark Hilbush - Initial contribution
*/
@NonNullByDefault
public class EcobeeDiscoveryService extends AbstractDiscoveryService implements ThingHandlerService {

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

private @NonNullByDefault({}) EcobeeAccountBridgeHandler bridgeHandler;

private @Nullable Future<?> discoveryJob;

public EcobeeDiscoveryService() {
super(SUPPORTED_THERMOSTAT_AND_SENSOR_THING_TYPES_UIDS, 8, true);
}

@Override
public void setThingHandler(@Nullable ThingHandler handler) {
if (handler instanceof EcobeeAccountBridgeHandler) {
this.bridgeHandler = (EcobeeAccountBridgeHandler) handler;
}
}

@Override
public @Nullable ThingHandler getThingHandler() {
return bridgeHandler;
}

@Override
public void activate() {
super.activate(null);
ThingHandlerService.super.activate();
mhilbush marked this conversation as resolved.
Show resolved Hide resolved
}

@Override
public void deactivate() {
super.deactivate();
}

@Override
public Set<ThingTypeUID> getSupportedThingTypes() {
return SUPPORTED_THERMOSTAT_AND_SENSOR_THING_TYPES_UIDS;
}

@Override
protected void startBackgroundDiscovery() {
logger.debug("EcobeeDiscovery: Starting background discovery job");
Future<?> localDiscoveryJob = discoveryJob;
if (localDiscoveryJob == null || localDiscoveryJob.isCancelled()) {
discoveryJob = scheduler.scheduleWithFixedDelay(this::discover, DISCOVERY_INITIAL_DELAY_SECONDS,
Copy link
Member

Choose a reason for hiding this comment

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

You should check whether isBackgroundDiscoveryEnabled returns true before scheduling this job.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I can see, startBackgroundDiscovery is being called before the bridge handler has initialized, therefore the configuration is not available at the time the job is scheduled.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I see. Then keep the scheduled job active in any case and do the check within the job as you intended to do it.

Copy link
Contributor Author

@mhilbush mhilbush Dec 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I thought it was odd that startBackgroundDiscovery can be called before the bridge thing has initialized. Especially in the case where the discovery service implements ThingHandlerService in order to get access to the bridge handler.

I also ran into something a little weird for which I'd like your opinion. Let me know if you think I should log an issue for this.

Here's the scenario:

I have a things file containing two bridge things:

Bridge ecobee:account:account "Ecobee Account" [ apiKey="XXXXXXX", apiTimeout=20, discoveryEnabled=false, refreshIntervalNormal=30, refreshIntervalQuick=5 ] {
}

Bridge zoneminder:server:server "Zoneminder Server" [ host="zoneminder.host", useSSL=false, portNumber="80", urlPath="/zm", discoveryEnabled=false, refreshInterval=30, defaultAlarmDuration=180 ] {
}

Notice that both Bridge definitions have a parameter called discoveryEnabled and that both are false.

If I change just the Zoneminder discoveryEnabled to true, I see this behavior.

2020-12-27 13:15:07.103 [INFO ] [el.core.internal.ModelRepositoryImpl] - Loading model 'test.things'
2020-12-27 13:15:07.120 [DEBUG] [.thing.internal.GenericThingProvider] - Read things from model 'test.things'
2020-12-27 13:15:07.122 [DEBUG] [.thing.internal.GenericThingProvider] - Updating thing 'ecobee:account:account' from model 'test.things'.
2020-12-27 13:15:07.136 [DEBUG] [.thing.internal.GenericThingProvider] - Updating thing 'zoneminder:server:server' from model 'test.things'.

If I change just the Zoneminder discoveryEnabled back to false, I see this behavior.

2020-12-27 13:17:36.865 [INFO ] [el.core.internal.ModelRepositoryImpl] - Loading model 'test.things'
2020-12-27 13:17:36.883 [DEBUG] [.thing.internal.GenericThingProvider] - Read things from model 'test.things'
2020-12-27 13:17:36.886 [DEBUG] [.thing.internal.GenericThingProvider] - Updating thing 'ecobee:account:account' from model 'test.things'.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds not what one would expect. Clearly, the thing where something has changed should be updated...
If you are able to minimise this to an easily reproducible case (without requiring any hardware, so the astro binding might be a good fit), an issue in openhab-core will make sense.

DISCOVERY_INTERVAL_SECONDS, TimeUnit.SECONDS);
}
}

@Override
protected void stopBackgroundDiscovery() {
logger.debug("EcobeeDiscovery: Stopping background discovery job");
Future<?> localDiscoveryJob = discoveryJob;
if (localDiscoveryJob != null) {
localDiscoveryJob.cancel(true);
discoveryJob = null;
}
}

@Override
public void startScan() {
logger.debug("EcobeeDiscovery: Starting discovery scan");
discover();
}

private void discover() {
if (!bridgeHandler.isBackgroundDiscoveryEnabled()) {
Copy link
Member

Choose a reason for hiding this comment

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

This check should be removed. This method is called upon manually triggered scans, so it must be performed even if background discovery is not activated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I'll fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

return;
}
if (bridgeHandler.getThing().getStatus() != ThingStatus.ONLINE) {
logger.debug("EcobeeDiscovery: Skipping discovery because Account Bridge thing is not ONLINE");
return;
}
logger.debug("EcobeeDiscovery: Discovering Ecobee devices");
discoverThermostats();
discoverSensors();
}

private synchronized void discoverThermostats() {
logger.debug("EcobeeDiscovery: Discovering thermostats");
for (ThermostatDTO thermostat : bridgeHandler.getRegisteredThermostats()) {
String name = thermostat.name;
String identifier = thermostat.identifier;
if (identifier != null && name != null) {
ThingUID thingUID = new ThingUID(UID_THERMOSTAT_BRIDGE, bridgeHandler.getThing().getUID(), identifier);
thingDiscovered(createThermostatDiscoveryResult(thingUID, identifier, name));
logger.debug("EcobeeDiscovery: Thermostat '{}' and name '{}' added with UID '{}'", identifier, name,
thingUID);
}
}
}

private DiscoveryResult createThermostatDiscoveryResult(ThingUID thermostatUID, String identifier, String name) {
Map<String, Object> properties = new HashMap<>();
properties.put(CONFIG_THERMOSTAT_ID, identifier);
return DiscoveryResultBuilder.create(thermostatUID).withProperties(properties)
.withRepresentationProperty(CONFIG_THERMOSTAT_ID).withBridge(bridgeHandler.getThing().getUID())
.withLabel(String.format("Ecobee Thermostat %s", name)).build();
}

private synchronized void discoverSensors() {
List<Thing> thermostatThings = bridgeHandler.getThing().getThings();
if (thermostatThings.size() == 0) {
logger.debug("EcobeeDiscovery: Skipping sensor discovery because there are no thermostat things");
return;
}
logger.debug("EcobeeDiscovery: Discovering sensors");
for (Thing thermostat : thermostatThings) {
EcobeeThermostatBridgeHandler thermostatHandler = (EcobeeThermostatBridgeHandler) thermostat.getHandler();
if (thermostatHandler != null) {
String thermostatId = thermostatHandler.getThermostatId();
logger.debug("EcobeeDiscovery: Discovering sensors for thermostat '{}'", thermostatId);
for (RemoteSensorDTO sensor : thermostatHandler.getSensors()) {
ThingUID bridgeUID = thermostatHandler.getThing().getUID();
ThingUID sensorUID = new ThingUID(UID_SENSOR_THING, bridgeUID, sensor.id.replace(":", "-"));
thingDiscovered(createSensorDiscoveryResult(sensorUID, bridgeUID, sensor));
logger.debug("EcobeeDiscovery: Sensor for '{}' with id '{}' and name '{}' added with UID '{}'",
thermostatId, sensor.id, sensor.name, sensorUID);
}
}
}
}

private DiscoveryResult createSensorDiscoveryResult(ThingUID sensorUID, ThingUID bridgeUID,
RemoteSensorDTO sensor) {
Map<String, Object> properties = new HashMap<>();
properties.put(CONFIG_SENSOR_ID, sensor.id);
return DiscoveryResultBuilder.create(sensorUID).withProperties(properties)
.withRepresentationProperty(CONFIG_SENSOR_ID).withBridge(bridgeUID)
.withLabel(String.format("Ecobee Sensor %s", sensor.name)).build();
}
}
124 changes: 0 additions & 124 deletions ...e/src/main/java/org/openhab/binding/ecobee/internal/discovery/SensorDiscoveryService.java
View file
Open in desktop

This file was deleted.

Loading