Skip to content

Commit

Permalink
[wemo] Fix stability issues (openhab#14163)
Browse files Browse the repository at this point in the history 
* Stabilize UPnP subscription handling
* Remove unused UpnpService
* Fix integration test

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
  • Loading branch information
@jlaur @FordPrfkt
jlaur authored and FordPrfkt committed Apr 19, 2023
1 parent f993622 commit 7da73ff
Show file tree
Hide file tree
Showing 15 changed files with 59 additions and 130 deletions.
2 changes: 1 addition & 1 deletion ...ab.binding.wemo/src/main/java/org/openhab/binding/wemo/internal/WemoBindingConstants.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ public class WemoBindingConstants {
public static final String DEVICE_ID = "deviceID";
public static final String POLLING_INTERVAL = "pollingInterval";
public static final int DEFAULT_REFRESH_INTERVAL_SECONDS = 60;
public static final int SUBSCRIPTION_DURATION_SECONDS = 600;
public static final int SUBSCRIPTION_DURATION_SECONDS = 1800;
public static final int LINK_DISCOVERY_SERVICE_INITIAL_DELAY = 5;
public static final String HTTP_CALL_CONTENT_HEADER = "text/xml; charset=utf-8";

Expand Down
27 changes: 12 additions & 15 deletions ...nhab.binding.wemo/src/main/java/org/openhab/binding/wemo/internal/WemoHandlerFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
import org.jupnp.UpnpService;
import org.openhab.binding.wemo.internal.discovery.WemoLinkDiscoveryService;
import org.openhab.binding.wemo.internal.handler.WemoBridgeHandler;
import org.openhab.binding.wemo.internal.handler.WemoCoffeeHandler;
Expand Down Expand Up @@ -68,7 +67,6 @@ public class WemoHandlerFactory extends BaseThingHandlerFactory {
public static final Set<ThingTypeUID> SUPPORTED_THING_TYPES = WemoBindingConstants.SUPPORTED_THING_TYPES;

private final UpnpIOService upnpIOService;
private final UpnpService upnpService;
private @Nullable WemoHttpCallFactory wemoHttpCallFactory;

@Override
Expand All @@ -79,9 +77,8 @@ public boolean supportsThingType(ThingTypeUID thingTypeUID) {
private final Map<ThingUID, ServiceRegistration<?>> discoveryServiceRegs = new HashMap<>();

@Activate
public WemoHandlerFactory(final @Reference UpnpIOService upnpIOService, @Reference UpnpService upnpService) {
public WemoHandlerFactory(final @Reference UpnpIOService upnpIOService) {
this.upnpIOService = upnpIOService;
this.upnpService = upnpService;
}

@Reference(cardinality = ReferenceCardinality.OPTIONAL, policy = ReferencePolicy.DYNAMIC)
Expand Down Expand Up @@ -111,46 +108,46 @@ public void unsetWemoHttpCallFactory(WemoHttpCallFactory wemoHttpCallFactory) {
} else if (WemoBindingConstants.THING_TYPE_INSIGHT.equals(thing.getThingTypeUID())) {
logger.debug("Creating a WemoInsightHandler for thing '{}' with UDN '{}'", thing.getUID(),
thing.getConfiguration().get(UDN));
return new WemoInsightHandler(thing, upnpIOService, upnpService, wemoHttpcaller);
return new WemoInsightHandler(thing, upnpIOService, wemoHttpcaller);
} else if (WemoBindingConstants.THING_TYPE_SOCKET.equals(thing.getThingTypeUID())
|| WemoBindingConstants.THING_TYPE_LIGHTSWITCH.equals(thing.getThingTypeUID())) {
logger.debug("Creating a WemoSwitchHandler for thing '{}' with UDN '{}'", thing.getUID(),
thing.getConfiguration().get(UDN));
return new WemoSwitchHandler(thing, upnpIOService, upnpService, wemoHttpcaller);
return new WemoSwitchHandler(thing, upnpIOService, wemoHttpcaller);
} else if (WemoBindingConstants.THING_TYPE_MOTION.equals(thing.getThingTypeUID())) {
logger.debug("Creating a WemoMotionHandler for thing '{}' with UDN '{}'", thing.getUID(),
thing.getConfiguration().get(UDN));
return new WemoMotionHandler(thing, upnpIOService, upnpService, wemoHttpcaller);
return new WemoMotionHandler(thing, upnpIOService, wemoHttpcaller);
} else if (thingTypeUID.equals(WemoBindingConstants.THING_TYPE_MAKER)) {
logger.debug("Creating a WemoMakerHandler for thing '{}' with UDN '{}'", thing.getUID(),
thing.getConfiguration().get(UDN));
return new WemoMakerHandler(thing, upnpIOService, upnpService, wemoHttpcaller);
return new WemoMakerHandler(thing, upnpIOService, wemoHttpcaller);
} else if (thingTypeUID.equals(WemoBindingConstants.THING_TYPE_COFFEE)) {
logger.debug("Creating a WemoCoffeeHandler for thing '{}' with UDN '{}'", thing.getUID(),
thing.getConfiguration().get(UDN));
return new WemoCoffeeHandler(thing, upnpIOService, upnpService, wemoHttpcaller);
return new WemoCoffeeHandler(thing, upnpIOService, wemoHttpcaller);
} else if (thingTypeUID.equals(WemoBindingConstants.THING_TYPE_DIMMER)) {
logger.debug("Creating a WemoDimmerHandler for thing '{}' with UDN '{}'", thing.getUID(),
thing.getConfiguration().get("udn"));
return new WemoDimmerHandler(thing, upnpIOService, upnpService, wemoHttpcaller);
return new WemoDimmerHandler(thing, upnpIOService, wemoHttpcaller);
} else if (thingTypeUID.equals(WemoBindingConstants.THING_TYPE_CROCKPOT)) {
logger.debug("Creating a WemoCockpotHandler for thing '{}' with UDN '{}'", thing.getUID(),
thing.getConfiguration().get("udn"));
return new WemoCrockpotHandler(thing, upnpIOService, upnpService, wemoHttpcaller);
return new WemoCrockpotHandler(thing, upnpIOService, wemoHttpcaller);
} else if (thingTypeUID.equals(WemoBindingConstants.THING_TYPE_PURIFIER)) {
logger.debug("Creating a WemoHolmesHandler for thing '{}' with UDN '{}'", thing.getUID(),
thing.getConfiguration().get("udn"));
return new WemoHolmesHandler(thing, upnpIOService, upnpService, wemoHttpcaller);
return new WemoHolmesHandler(thing, upnpIOService, wemoHttpcaller);
} else if (thingTypeUID.equals(WemoBindingConstants.THING_TYPE_HUMIDIFIER)) {
logger.debug("Creating a WemoHolmesHandler for thing '{}' with UDN '{}'", thing.getUID(),
thing.getConfiguration().get("udn"));
return new WemoHolmesHandler(thing, upnpIOService, upnpService, wemoHttpcaller);
return new WemoHolmesHandler(thing, upnpIOService, wemoHttpcaller);
} else if (thingTypeUID.equals(WemoBindingConstants.THING_TYPE_HEATER)) {
logger.debug("Creating a WemoHolmesHandler for thing '{}' with UDN '{}'", thing.getUID(),
thing.getConfiguration().get("udn"));
return new WemoHolmesHandler(thing, upnpIOService, upnpService, wemoHttpcaller);
return new WemoHolmesHandler(thing, upnpIOService, wemoHttpcaller);
} else if (thingTypeUID.equals(WemoBindingConstants.THING_TYPE_MZ100)) {
return new WemoLightHandler(thing, upnpIOService, upnpService, wemoHttpcaller);
return new WemoLightHandler(thing, upnpIOService, wemoHttpcaller);
} else {
logger.warn("ThingHandler not found for {}", thingTypeUID);
return null;
Expand Down
97 changes: 24 additions & 73 deletions ...ng.wemo/src/main/java/org/openhab/binding/wemo/internal/handler/WemoBaseThingHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,12 @@
import java.util.Map;
import java.util.Map.Entry;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import java.util.stream.Stream;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
import org.jupnp.UpnpService;
import org.jupnp.model.message.header.RootDeviceHeader;
import org.openhab.binding.wemo.internal.WemoBindingConstants;
import org.openhab.binding.wemo.internal.http.WemoHttpCall;
import org.openhab.core.io.transport.upnp.UpnpIOParticipant;
Expand All @@ -51,26 +47,21 @@
@NonNullByDefault
public abstract class WemoBaseThingHandler extends BaseThingHandler implements UpnpIOParticipant {

private static final int SUBSCRIPTION_RENEWAL_INITIAL_DELAY_SECONDS = 15;
private static final int SUBSCRIPTION_RENEWAL_INTERVAL_SECONDS = 60;
private static final int PORT_RANGE_START = 49151;
private static final int PORT_RANGE_END = 49157;

private final Logger logger = LoggerFactory.getLogger(WemoBaseThingHandler.class);
private final UpnpIOService service;
private final UpnpService upnpService;
private final Object upnpLock = new Object();

protected WemoHttpCall wemoHttpCaller;

private @Nullable String host;
private Map<String, Instant> subscriptions = new ConcurrentHashMap<String, Instant>();
private @Nullable ScheduledFuture<?> subscriptionRenewalJob;

public WemoBaseThingHandler(Thing thing, UpnpIOService upnpIOService, UpnpService upnpService,
WemoHttpCall wemoHttpCaller) {
public WemoBaseThingHandler(Thing thing, UpnpIOService upnpIOService, WemoHttpCall wemoHttpCaller) {
super(thing);
this.service = upnpIOService;
this.upnpService = upnpService;
this.wemoHttpCaller = wemoHttpCaller;
}

Expand All @@ -83,7 +74,6 @@ public void initialize() {

@Override
public void dispose() {
cancelSubscriptionRenewalJob();
removeSubscriptions();
logger.debug("Unregistering UPnP participant for {}", getThing().getUID());
service.unregisterParticipant(this);
Expand All @@ -98,16 +88,14 @@ public void handleCommand(ChannelUID channelUID, Command command) {
public void onStatusChanged(boolean status) {
if (status) {
logger.debug("UPnP device {} for {} is present", getUDN(), getThing().getUID());
if (service.isRegistered(this)) {
// After successful discovery, try to subscribe again.
renewSubscriptions();
}
} else {
logger.info("UPnP device {} for {} is absent", getUDN(), getThing().getUID());
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.OFFLINE.COMMUNICATION_ERROR);
// Expire subscriptions.
for (Entry<String, Instant> subscription : subscriptions.entrySet()) {
subscription.setValue(Instant.MIN);
synchronized (upnpLock) {
for (Entry<String, Instant> subscription : subscriptions.entrySet()) {
subscription.setValue(Instant.MIN);
}
}
}
}
Expand All @@ -124,7 +112,9 @@ public void onServiceSubscribed(@Nullable String service, boolean succeeded) {
}
logger.debug("Subscription to service {} for {} {}", service, getUDN(), succeeded ? "succeeded" : "failed");
if (succeeded) {
subscriptions.put(service, Instant.now());
synchronized (upnpLock) {
subscriptions.put(service, Instant.now());
}
}
}

Expand All @@ -138,67 +128,28 @@ protected boolean isUpnpDeviceRegistered() {
}

protected void addSubscription(String serviceId) {
if (subscriptions.containsKey(serviceId)) {
logger.debug("{} already subscribed to {}", getUDN(), serviceId);
return;
}
if (subscriptions.isEmpty()) {
logger.debug("Adding first GENA subscription for {}, scheduling renewal job", getUDN());
scheduleSubscriptionRenewalJob();
synchronized (upnpLock) {
if (subscriptions.containsKey(serviceId)) {
logger.debug("{} already subscribed to {}", getUDN(), serviceId);
return;
}
subscriptions.put(serviceId, Instant.MIN);
logger.debug("Adding GENA subscription {} for {}, participant is {}", serviceId, getUDN(),
service.isRegistered(this) ? "registered" : "not registered");
}
subscriptions.put(serviceId, Instant.MIN);
logger.debug("Adding GENA subscription {} for {}, participant is {}", serviceId, getUDN(),
service.isRegistered(this) ? "registered" : "not registered");
service.addSubscription(this, serviceId, WemoBindingConstants.SUBSCRIPTION_DURATION_SECONDS);
}

private void scheduleSubscriptionRenewalJob() {
cancelSubscriptionRenewalJob();
this.subscriptionRenewalJob = scheduler.scheduleWithFixedDelay(this::renewSubscriptions,
SUBSCRIPTION_RENEWAL_INITIAL_DELAY_SECONDS, SUBSCRIPTION_RENEWAL_INTERVAL_SECONDS, TimeUnit.SECONDS);
}

private void cancelSubscriptionRenewalJob() {
ScheduledFuture<?> subscriptionRenewalJob = this.subscriptionRenewalJob;
if (subscriptionRenewalJob != null) {
subscriptionRenewalJob.cancel(true);
}
this.subscriptionRenewalJob = null;
}

private synchronized void renewSubscriptions() {
if (subscriptions.isEmpty()) {
return;
}
if (!service.isRegistered(this)) {
logger.debug("Participant not registered when renewing GENA subscriptions for {}, starting UPnP discovery",
getUDN());
upnpService.getControlPoint().search(new RootDeviceHeader());
return;
}
logger.debug("Renewing GENA subscriptions for {}", getUDN());
subscriptions.forEach((serviceId, lastRenewed) -> {
if (lastRenewed.isBefore(Instant.now().minusSeconds(
WemoBindingConstants.SUBSCRIPTION_DURATION_SECONDS - SUBSCRIPTION_RENEWAL_INTERVAL_SECONDS))) {
logger.debug("Subscription for service {} with timestamp {} has expired, renewing", serviceId,
lastRenewed);
service.removeSubscription(this, serviceId);
service.addSubscription(this, serviceId, WemoBindingConstants.SUBSCRIPTION_DURATION_SECONDS);
}
});
}

private void removeSubscriptions() {
if (subscriptions.isEmpty()) {
return;
}
logger.debug("Removing GENA subscriptions for {}, participant is {}", getUDN(),
service.isRegistered(this) ? "registered" : "not registered");
subscriptions.forEach((serviceId, lastRenewed) -> {
logger.debug("Removing subscription for service {}", serviceId);
service.removeSubscription(this, serviceId);
});
subscriptions.clear();
synchronized (upnpLock) {
subscriptions.forEach((serviceId, lastRenewed) -> {
logger.debug("Removing subscription for service {}", serviceId);
service.removeSubscription(this, serviceId);
});
subscriptions.clear();
}
}

public @Nullable String getWemoURL(String actionService) {
Expand Down
6 changes: 2 additions & 4 deletions ...nding.wemo/src/main/java/org/openhab/binding/wemo/internal/handler/WemoCoffeeHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
import org.jupnp.UpnpService;
import org.openhab.binding.wemo.internal.http.WemoHttpCall;
import org.openhab.core.config.core.Configuration;
import org.openhab.core.io.transport.upnp.UpnpIOService;
Expand Down Expand Up @@ -70,9 +69,8 @@ public class WemoCoffeeHandler extends WemoBaseThingHandler {

private @Nullable ScheduledFuture<?> pollingJob;

public WemoCoffeeHandler(Thing thing, UpnpIOService upnpIOService, UpnpService upnpService,
WemoHttpCall wemoHttpCaller) {
super(thing, upnpIOService, upnpService, wemoHttpCaller);
public WemoCoffeeHandler(Thing thing, UpnpIOService upnpIOService, WemoHttpCall wemoHttpCaller) {
super(thing, upnpIOService, wemoHttpCaller);

logger.debug("Creating a WemoCoffeeHandler for thing '{}'", getThing().getUID());
}
Expand Down
6 changes: 2 additions & 4 deletions ...ing.wemo/src/main/java/org/openhab/binding/wemo/internal/handler/WemoCrockpotHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
import org.jupnp.UpnpService;
import org.openhab.binding.wemo.internal.http.WemoHttpCall;
import org.openhab.core.config.core.Configuration;
import org.openhab.core.io.transport.upnp.UpnpIOService;
Expand Down Expand Up @@ -61,9 +60,8 @@ public class WemoCrockpotHandler extends WemoBaseThingHandler {

private @Nullable ScheduledFuture<?> pollingJob;

public WemoCrockpotHandler(Thing thing, UpnpIOService upnpIOService, UpnpService upnpService,
WemoHttpCall wemoHttpCaller) {
super(thing, upnpIOService, upnpService, wemoHttpCaller);
public WemoCrockpotHandler(Thing thing, UpnpIOService upnpIOService, WemoHttpCall wemoHttpCaller) {
super(thing, upnpIOService, wemoHttpCaller);

logger.debug("Creating a WemoCrockpotHandler for thing '{}'", getThing().getUID());
}
Expand Down
6 changes: 2 additions & 4 deletions ...nding.wemo/src/main/java/org/openhab/binding/wemo/internal/handler/WemoDimmerHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
import org.jupnp.UpnpService;
import org.openhab.binding.wemo.internal.http.WemoHttpCall;
import org.openhab.core.config.core.Configuration;
import org.openhab.core.io.transport.upnp.UpnpIOService;
Expand Down Expand Up @@ -74,9 +73,8 @@ public class WemoDimmerHandler extends WemoBaseThingHandler {
*/
private static final int DIM_STEPSIZE = 5;

public WemoDimmerHandler(Thing thing, UpnpIOService upnpIOService, UpnpService upnpService,
WemoHttpCall wemoHttpCaller) {
super(thing, upnpIOService, upnpService, wemoHttpCaller);
public WemoDimmerHandler(Thing thing, UpnpIOService upnpIOService, WemoHttpCall wemoHttpCaller) {
super(thing, upnpIOService, wemoHttpCaller);

logger.debug("Creating a WemoDimmerHandler for thing '{}'", getThing().getUID());
}
Expand Down
5 changes: 2 additions & 3 deletions ...hab.binding.wemo/src/main/java/org/openhab/binding/wemo/internal/handler/WemoHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
import org.jupnp.UpnpService;
import org.openhab.binding.wemo.internal.http.WemoHttpCall;
import org.openhab.core.io.transport.upnp.UpnpIOService;
import org.openhab.core.library.types.OnOffType;
Expand Down Expand Up @@ -53,8 +52,8 @@ public abstract class WemoHandler extends WemoBaseThingHandler {

private @Nullable ScheduledFuture<?> pollingJob;

public WemoHandler(Thing thing, UpnpIOService upnpIOService, UpnpService upnpService, WemoHttpCall wemoHttpCaller) {
super(thing, upnpIOService, upnpService, wemoHttpCaller);
public WemoHandler(Thing thing, UpnpIOService upnpIOService, WemoHttpCall wemoHttpCaller) {
super(thing, upnpIOService, wemoHttpCaller);

logger.debug("Creating a WemoHandler for thing '{}'", getThing().getUID());
}
Expand Down
6 changes: 2 additions & 4 deletions ...nding.wemo/src/main/java/org/openhab/binding/wemo/internal/handler/WemoHolmesHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
import org.jupnp.UpnpService;
import org.openhab.binding.wemo.internal.http.WemoHttpCall;
import org.openhab.core.config.core.Configuration;
import org.openhab.core.io.transport.upnp.UpnpIOService;
Expand Down Expand Up @@ -75,9 +74,8 @@ public class WemoHolmesHandler extends WemoBaseThingHandler {

private @Nullable ScheduledFuture<?> pollingJob;

public WemoHolmesHandler(Thing thing, UpnpIOService upnpIOService, UpnpService upnpService,
WemoHttpCall wemoHttpCaller) {
super(thing, upnpIOService, upnpService, wemoHttpCaller);
public WemoHolmesHandler(Thing thing, UpnpIOService upnpIOService, WemoHttpCall wemoHttpCaller) {
super(thing, upnpIOService, wemoHttpCaller);

logger.debug("Creating a WemoHolmesHandler for thing '{}'", getThing().getUID());
}
Expand Down
6 changes: 2 additions & 4 deletions ...ding.wemo/src/main/java/org/openhab/binding/wemo/internal/handler/WemoInsightHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
import org.jupnp.UpnpService;
import org.openhab.binding.wemo.internal.InsightParser;
import org.openhab.binding.wemo.internal.WemoBindingConstants;
import org.openhab.binding.wemo.internal.WemoPowerBank;
Expand Down Expand Up @@ -52,9 +51,8 @@ public class WemoInsightHandler extends WemoHandler {
private int currentPowerSlidingSeconds;
private int currentPowerDeltaTrigger;

public WemoInsightHandler(Thing thing, UpnpIOService upnpIOService, UpnpService upnpService,
WemoHttpCall wemoHttpCaller) {
super(thing, upnpIOService, upnpService, wemoHttpCaller);
public WemoInsightHandler(Thing thing, UpnpIOService upnpIOService, WemoHttpCall wemoHttpCaller) {
super(thing, upnpIOService, wemoHttpCaller);
}

@Override
Expand Down
Loading

0 comments on commit 7da73ff

Please sign in to comment.