[plugwise] Use ThingHandlerService for discovery (#9041)

This simplifies the PlugwiseHandlerFactory code so it no longer needs to register and keep track of a discovery service for each bridge.

Also contains a few other improvements:

* fix lastRequest timestamp not updated when sending messages to discover node properties
* use java.time Duration and Instant
* use List.of, Set.of
* remove redundant null suppression because of EEAs

Signed-off-by: Wouter Born <github@maindrain.net>

bundles/org.openhab.binding.plugwise/src/main/java/org/openhab/binding/plugwise/internal/PlugwiseBindingConstants.java

@@ -12,11 +12,7 @@
  */
 package org.openhab.binding.plugwise.internal;
 
-import static java.util.stream.Collectors.*;
-
-import java.util.Collections;
 import java.util.Set;
-import java.util.stream.Stream;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.openhab.core.thing.ThingTypeUID;
@@ -65,8 +61,6 @@ public class PlugwiseBindingConstants {
     public static final ThingTypeUID THING_TYPE_STICK = new ThingTypeUID(BINDING_ID, "stick");
     public static final ThingTypeUID THING_TYPE_SWITCH = new ThingTypeUID(BINDING_ID, "switch");
 
-    public static final Set<ThingTypeUID> SUPPORTED_THING_TYPES_UIDS = Stream
+    public static final Set<ThingTypeUID> SUPPORTED_THING_TYPES_UIDS = Set.of(THING_TYPE_CIRCLE, THING_TYPE_CIRCLE_PLUS,
-            .of(THING_TYPE_CIRCLE, THING_TYPE_CIRCLE_PLUS, THING_TYPE_SCAN, THING_TYPE_SENSE, THING_TYPE_STEALTH,
+            THING_TYPE_SCAN, THING_TYPE_SENSE, THING_TYPE_STEALTH, THING_TYPE_STICK, THING_TYPE_SWITCH);
-                    THING_TYPE_STICK, THING_TYPE_SWITCH)
-            .collect(collectingAndThen(toSet(), Collections::unmodifiableSet));
 }

bundles/org.openhab.binding.plugwise/src/main/java/org/openhab/binding/plugwise/internal/PlugwiseHandlerFactory.java

@@ -14,10 +14,6 @@ package org.openhab.binding.plugwise.internal;
 
 import static org.openhab.binding.plugwise.internal.PlugwiseBindingConstants.*;
 
-import java.util.HashMap;
-import java.util.Hashtable;
-import java.util.Map;
-
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
 import org.openhab.binding.plugwise.internal.handler.PlugwiseRelayDeviceHandler;
@@ -25,16 +21,13 @@ import org.openhab.binding.plugwise.internal.handler.PlugwiseScanHandler;
 import org.openhab.binding.plugwise.internal.handler.PlugwiseSenseHandler;
 import org.openhab.binding.plugwise.internal.handler.PlugwiseStickHandler;
 import org.openhab.binding.plugwise.internal.handler.PlugwiseSwitchHandler;
-import org.openhab.core.config.discovery.DiscoveryService;
 import org.openhab.core.io.transport.serial.SerialPortManager;
 import org.openhab.core.thing.Bridge;
 import org.openhab.core.thing.Thing;
 import org.openhab.core.thing.ThingTypeUID;
-import org.openhab.core.thing.ThingUID;
 import org.openhab.core.thing.binding.BaseThingHandlerFactory;
 import org.openhab.core.thing.binding.ThingHandler;
 import org.openhab.core.thing.binding.ThingHandlerFactory;
-import org.osgi.framework.ServiceRegistration;
 import org.osgi.service.component.annotations.Activate;
 import org.osgi.service.component.annotations.Component;
 import org.osgi.service.component.annotations.Reference;
@@ -48,8 +41,6 @@ import org.osgi.service.component.annotations.Reference;
 @Component(service = ThingHandlerFactory.class, configurationPid = "binding.plugwise")
 public class PlugwiseHandlerFactory extends BaseThingHandlerFactory {
 
-    private final Map<ThingUID, ServiceRegistration<?>> discoveryServiceRegistrations = new HashMap<>();
-
     private final SerialPortManager serialPortManager;
 
     @Activate
@@ -67,9 +58,7 @@ public class PlugwiseHandlerFactory extends BaseThingHandlerFactory {
         ThingTypeUID thingTypeUID = thing.getThingTypeUID();
 
         if (thingTypeUID.equals(THING_TYPE_STICK)) {
-            PlugwiseStickHandler handler = new PlugwiseStickHandler((Bridge) thing, serialPortManager);
+            return new PlugwiseStickHandler((Bridge) thing, serialPortManager);
-            registerDiscoveryService(handler);
-            return handler;
         } else if (thingTypeUID.equals(THING_TYPE_CIRCLE) || thingTypeUID.equals(THING_TYPE_CIRCLE_PLUS)
                 || thingTypeUID.equals(THING_TYPE_STEALTH)) {
             return new PlugwiseRelayDeviceHandler(thing);
@@ -83,23 +72,4 @@ public class PlugwiseHandlerFactory extends BaseThingHandlerFactory {
 
         return null;
     }
-
-    private void registerDiscoveryService(PlugwiseStickHandler handler) {
-        PlugwiseThingDiscoveryService discoveryService = new PlugwiseThingDiscoveryService(handler);
-        discoveryService.activate();
-        this.discoveryServiceRegistrations.put(handler.getThing().getUID(),
-                bundleContext.registerService(DiscoveryService.class.getName(), discoveryService, new Hashtable<>()));
-    }
-
-    @Override
-    protected void removeHandler(ThingHandler thingHandler) {
-        ServiceRegistration<?> registration = this.discoveryServiceRegistrations.get(thingHandler.getThing().getUID());
-        if (registration != null) {
-            PlugwiseThingDiscoveryService discoveryService = (PlugwiseThingDiscoveryService) bundleContext
-                    .getService(registration.getReference());
-            discoveryService.deactivate();
-            registration.unregister();
-            discoveryServiceRegistrations.remove(thingHandler.getThing().getUID());
-        }
-    }
 }

bundles/org.openhab.binding.plugwise/src/main/java/org/openhab/binding/plugwise/internal/PlugwiseThingDiscoveryService.java

@@ -15,6 +15,8 @@ package org.openhab.binding.plugwise.internal;
 import static java.util.stream.Collectors.*;
 import static org.openhab.binding.plugwise.internal.PlugwiseBindingConstants.*;
 
+import java.time.Duration;
+import java.time.Instant;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Iterator;
@@ -44,6 +46,8 @@ 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;
 
@@ -56,13 +60,13 @@ import org.slf4j.LoggerFactory;
  */
 @NonNullByDefault
 public class PlugwiseThingDiscoveryService extends AbstractDiscoveryService
-        implements PlugwiseMessageListener, PlugwiseStickStatusListener {
+        implements PlugwiseMessageListener, PlugwiseStickStatusListener, ThingHandlerService {
 
     private static class CurrentRoleCall {
         private boolean isRoleCalling;
         private int currentNodeID;
         private int attempts;
-        private long lastRequestMillis;
+        private Instant lastRequest = Instant.MIN;
     }
 
     private static class DiscoveredNode {
@@ -70,7 +74,7 @@ public class PlugwiseThingDiscoveryService extends AbstractDiscoveryService
         private final Map<String, String> properties = new HashMap<>();
         private DeviceType deviceType = DeviceType.UNKNOWN;
         private int attempts;
-        private long lastRequestMillis;
+        private Instant lastRequest = Instant.MIN;
 
         public DiscoveredNode(MACAddress macAddress) {
             this.macAddress = macAddress;
@@ -87,16 +91,16 @@ public class PlugwiseThingDiscoveryService extends AbstractDiscoveryService
 
     private static final int MIN_NODE_ID = 0;
     private static final int MAX_NODE_ID = 63;
-    private static final int DISCOVERY_INTERVAL = 180;
 
-    private static final int WATCH_INTERVAL = 1;
-
-    private static final int MESSAGE_TIMEOUT = 15;
     private static final int MESSAGE_RETRY_ATTEMPTS = 5;
 
+    private static final Duration DISCOVERY_INTERVAL = Duration.ofMinutes(3);
+    private static final Duration MESSAGE_TIMEOUT = Duration.ofSeconds(15);
+    private static final Duration WATCH_INTERVAL = Duration.ofSeconds(1);
+
     private final Logger logger = LoggerFactory.getLogger(PlugwiseThingDiscoveryService.class);
 
-    private final PlugwiseStickHandler stickHandler;
+    private @NonNullByDefault({}) PlugwiseStickHandler stickHandler;
 
     private @Nullable ScheduledFuture<?> discoveryJob;
     private @Nullable ScheduledFuture<?> watchJob;
@@ -104,10 +108,8 @@ public class PlugwiseThingDiscoveryService extends AbstractDiscoveryService
 
     private final Map<MACAddress, DiscoveredNode> discoveredNodes = new ConcurrentHashMap<>();
 
-    public PlugwiseThingDiscoveryService(PlugwiseStickHandler stickHandler) throws IllegalArgumentException {
+    public PlugwiseThingDiscoveryService() throws IllegalArgumentException {
         super(DISCOVERED_THING_TYPES_UIDS, 1, true);
-        this.stickHandler = stickHandler;
-        this.stickHandler.addStickStatusListener(this);
     }
 
     @Override
@@ -118,6 +120,7 @@ public class PlugwiseThingDiscoveryService extends AbstractDiscoveryService
         stopDiscoveryWatchJob();
     }
 
+    @Override
     public void activate() {
         super.activate(new HashMap<>());
     }
@@ -138,7 +141,7 @@ public class PlugwiseThingDiscoveryService extends AbstractDiscoveryService
     }
 
     @Override
-    protected void deactivate() {
+    public void deactivate() {
         super.deactivate();
         stickHandler.removeMessageListener(this);
         stickHandler.removeStickStatusListener(this);
@@ -147,8 +150,10 @@ public class PlugwiseThingDiscoveryService extends AbstractDiscoveryService
     private void discoverNewNodeDetails(MACAddress macAddress) {
         if (!isAlreadyDiscovered(macAddress)) {
             logger.debug("Discovered new node ({})", macAddress);
-            discoveredNodes.put(macAddress, new DiscoveredNode(macAddress));
+            DiscoveredNode node = new DiscoveredNode(macAddress);
+            discoveredNodes.put(macAddress, node);
             updateInformation(macAddress);
+            node.lastRequest = Instant.now();
         } else {
             logger.debug("Already discovered node ({})", macAddress);
         }
@@ -184,6 +189,11 @@ public class PlugwiseThingDiscoveryService extends AbstractDiscoveryService
         return stickHandler.getThing().getStatus();
     }
 
+    @Override
+    public @Nullable ThingHandler getThingHandler() {
+        return stickHandler;
+    }
+
     private void handleAnnounceAwakeRequest(AnnounceAwakeRequestMessage message) {
         discoverNewNodeDetails(message.getMACAddress());
     }
@@ -267,7 +277,7 @@ public class PlugwiseThingDiscoveryService extends AbstractDiscoveryService
                 currentRoleCall.attempts++;
             }
             currentRoleCall.currentNodeID = nodeID;
-            currentRoleCall.lastRequestMillis = System.currentTimeMillis();
+            currentRoleCall.lastRequest = Instant.now();
         } else {
             logger.warn("Invalid node ID for role call: {}", nodeID);
         }
@@ -277,6 +287,14 @@ public class PlugwiseThingDiscoveryService extends AbstractDiscoveryService
         stickHandler.sendMessage(message, PlugwiseMessagePriority.UPDATE_AND_DISCOVERY);
     }
 
+    @Override
+    public void setThingHandler(ThingHandler handler) {
+        if (handler instanceof PlugwiseStickHandler) {
+            stickHandler = (PlugwiseStickHandler) handler;
+            stickHandler.addStickStatusListener(this);
+        }
+    }
+
     @Override
     protected void startBackgroundDiscovery() {
         logger.debug("Starting Plugwise device background discovery");
@@ -288,7 +306,8 @@ public class PlugwiseThingDiscoveryService extends AbstractDiscoveryService
 
         ScheduledFuture<?> localDiscoveryJob = discoveryJob;
         if (localDiscoveryJob == null || localDiscoveryJob.isCancelled()) {
-            discoveryJob = scheduler.scheduleWithFixedDelay(discoveryRunnable, 0, DISCOVERY_INTERVAL, TimeUnit.SECONDS);
+            discoveryJob = scheduler.scheduleWithFixedDelay(discoveryRunnable, 0, DISCOVERY_INTERVAL.toMillis(),
+                    TimeUnit.MILLISECONDS);
         }
     }
 
@@ -297,7 +316,8 @@ public class PlugwiseThingDiscoveryService extends AbstractDiscoveryService
 
         Runnable watchRunnable = () -> {
             if (currentRoleCall.isRoleCalling) {
-                if ((System.currentTimeMillis() - currentRoleCall.lastRequestMillis) > (MESSAGE_TIMEOUT * 1000)
+                Duration durationSinceLastRequest = Duration.between(currentRoleCall.lastRequest, Instant.now());
+                if (durationSinceLastRequest.compareTo(MESSAGE_TIMEOUT) > 0
                         && currentRoleCall.attempts < MESSAGE_RETRY_ATTEMPTS) {
                     logger.debug("Resending timed out role call message for node with ID {} on Circle+ ({})",
                             currentRoleCall.currentNodeID, getCirclePlusMAC());
@@ -313,10 +333,11 @@ public class PlugwiseThingDiscoveryService extends AbstractDiscoveryService
             while (it.hasNext()) {
                 Entry<MACAddress, DiscoveredNode> entry = it.next();
                 DiscoveredNode node = entry.getValue();
-                if (System.currentTimeMillis() - node.lastRequestMillis > (MESSAGE_TIMEOUT * 1000)
+                Duration durationSinceLastRequest = Duration.between(node.lastRequest, Instant.now());
-                        && node.attempts < MESSAGE_RETRY_ATTEMPTS) {
+                if (durationSinceLastRequest.compareTo(MESSAGE_TIMEOUT) > 0 && node.attempts < MESSAGE_RETRY_ATTEMPTS) {
                     logger.debug("Resending timed out information request message to node ({})", node.macAddress);
                     updateInformation(node.macAddress);
+                    node.lastRequest = Instant.now();
                     node.attempts++;
                 } else if (node.attempts >= MESSAGE_RETRY_ATTEMPTS) {
                     logger.debug("Giving up on information request for node ({})", node.macAddress);
@@ -332,8 +353,8 @@ public class PlugwiseThingDiscoveryService extends AbstractDiscoveryService
 
         ScheduledFuture<?> localWatchJob = watchJob;
         if (localWatchJob == null || localWatchJob.isCancelled()) {
-            watchJob = scheduler.scheduleWithFixedDelay(watchRunnable, WATCH_INTERVAL, WATCH_INTERVAL,
+            watchJob = scheduler.scheduleWithFixedDelay(watchRunnable, WATCH_INTERVAL.toMillis(),
-                    TimeUnit.SECONDS);
+                    WATCH_INTERVAL.toMillis(), TimeUnit.MILLISECONDS);
         }
     }
 

bundles/org.openhab.binding.plugwise/src/main/java/org/openhab/binding/plugwise/internal/PlugwiseUtils.java

@@ -43,17 +43,17 @@ public final class PlugwiseUtils {
     }
 
     public static DeviceType getDeviceType(ThingTypeUID uid) {
-        if (uid.equals(THING_TYPE_CIRCLE)) {
+        if (THING_TYPE_CIRCLE.equals(uid)) {
             return CIRCLE;
-        } else if (uid.equals(THING_TYPE_CIRCLE_PLUS)) {
+        } else if (THING_TYPE_CIRCLE_PLUS.equals(uid)) {
             return CIRCLE_PLUS;
-        } else if (uid.equals(THING_TYPE_SCAN)) {
+        } else if (THING_TYPE_SCAN.equals(uid)) {
             return SCAN;
-        } else if (uid.equals(THING_TYPE_SENSE)) {
+        } else if (THING_TYPE_SENSE.equals(uid)) {
             return SENSE;
-        } else if (uid.equals(THING_TYPE_STEALTH)) {
+        } else if (THING_TYPE_STEALTH.equals(uid)) {
             return STEALTH;
-        } else if (uid.equals(THING_TYPE_SWITCH)) {
+        } else if (THING_TYPE_SWITCH.equals(uid)) {
             return SWITCH;
         } else {
             return UNKNOWN;
@@ -106,7 +106,6 @@ public final class PlugwiseUtils {
         return upperCamel.substring(0, 1).toLowerCase() + upperCamel.substring(1);
     }
 
-    @SuppressWarnings("null")
     public static boolean updateProperties(Map<String, String> properties, InformationResponseMessage message) {
         boolean update = false;
 

bundles/org.openhab.binding.plugwise/src/main/java/org/openhab/binding/plugwise/internal/handler/PlugwiseRelayDeviceHandler.java

@@ -12,16 +12,13 @@
  */
 package org.openhab.binding.plugwise.internal.handler;
 
-import static java.util.stream.Collectors.*;
 import static org.openhab.binding.plugwise.internal.PlugwiseBindingConstants.*;
 import static org.openhab.core.thing.ThingStatus.*;
 
 import java.time.Duration;
 import java.time.LocalDateTime;
-import java.util.Collections;
 import java.util.List;
 import java.util.concurrent.TimeUnit;
-import java.util.stream.Stream;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
@@ -222,10 +219,8 @@ public class PlugwiseRelayDeviceHandler extends AbstractPlugwiseThingHandler {
         }
     };
 
-    private final List<PlugwiseDeviceTask> recurringTasks = Stream
+    private final List<PlugwiseDeviceTask> recurringTasks = List.of(clockUpdateTask, currentPowerUpdateTask,
-            .of(clockUpdateTask, currentPowerUpdateTask, energyUpdateTask, informationUpdateTask,
+            energyUpdateTask, informationUpdateTask, realTimeClockUpdateTask, setClockTask);
-                    realTimeClockUpdateTask, setClockTask)
-            .collect(collectingAndThen(toList(), Collections::unmodifiableList));
 
     private final Logger logger = LoggerFactory.getLogger(PlugwiseRelayDeviceHandler.class);
     private final DeviceType deviceType;

bundles/org.openhab.binding.plugwise/src/main/java/org/openhab/binding/plugwise/internal/handler/PlugwiseStickHandler.java

@@ -18,6 +18,7 @@ import static org.openhab.core.thing.ThingStatus.*;
 
 import java.io.IOException;
 import java.time.Duration;
+import java.util.Collection;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.CopyOnWriteArrayList;
@@ -28,6 +29,7 @@ import org.openhab.binding.plugwise.internal.PlugwiseCommunicationHandler;
 import org.openhab.binding.plugwise.internal.PlugwiseDeviceTask;
 import org.openhab.binding.plugwise.internal.PlugwiseInitializationException;
 import org.openhab.binding.plugwise.internal.PlugwiseMessagePriority;
+import org.openhab.binding.plugwise.internal.PlugwiseThingDiscoveryService;
 import org.openhab.binding.plugwise.internal.PlugwiseUtils;
 import org.openhab.binding.plugwise.internal.config.PlugwiseStickConfig;
 import org.openhab.binding.plugwise.internal.listener.PlugwiseMessageListener;
@@ -48,6 +50,7 @@ import org.openhab.core.thing.Thing;
 import org.openhab.core.thing.ThingStatus;
 import org.openhab.core.thing.ThingStatusDetail;
 import org.openhab.core.thing.binding.BaseBridgeHandler;
+import org.openhab.core.thing.binding.ThingHandlerService;
 import org.openhab.core.types.Command;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -122,6 +125,11 @@ public class PlugwiseStickHandler extends BaseBridgeHandler implements PlugwiseM
         return circlePlusMAC;
     }
 
+    @Override
+    public Collection<Class<? extends ThingHandlerService>> getServices() {
+        return List.of(PlugwiseThingDiscoveryService.class);
+    }
+
     public @Nullable MACAddress getStickMAC() {
         return stickMAC;
     }