[nikobus] fixed refresh handling (#9114)

* * fixed refresh handing due `channelLinked` not called anymore on startup for example (similar to https://github.com/openhab/openhab-core/issues/1707),
* "Impacted Modules" can be empty if button is configured as "standalone" and does not impact modules,
* minor doc fix

* * check if configured impacted module for `impactedModules` is referencing an existing thing,
* fixed warnings
* Fixed review comment.

Signed-off-by: Boris Krivonog <boris.krivonog@inova.si>

bundles/org.openhab.binding.nikobus/src/main/java/org/openhab/binding/nikobus/internal/handler/NikobusModuleHandler.java

@@ -14,11 +14,9 @@ package org.openhab.binding.nikobus.internal.handler;
 
 import static org.openhab.binding.nikobus.internal.NikobusBindingConstants.CHANNEL_OUTPUT_PREFIX;
 
-import java.util.ArrayList;
 import java.util.EnumSet;
 import java.util.HashMap;
 import java.util.HashSet;
-import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
@@ -26,6 +24,7 @@ import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.openhab.binding.nikobus.internal.protocol.NikobusCommand;
 import org.openhab.binding.nikobus.internal.protocol.SwitchModuleCommandFactory;
 import org.openhab.binding.nikobus.internal.protocol.SwitchModuleGroup;
+import org.openhab.core.thing.Channel;
 import org.openhab.core.thing.ChannelUID;
 import org.openhab.core.thing.Thing;
 import org.openhab.core.thing.ThingStatus;
@@ -46,12 +45,21 @@ abstract class NikobusModuleHandler extends NikobusBaseThingHandler {
     private final EnumSet<SwitchModuleGroup> pendingRefresh = EnumSet.noneOf(SwitchModuleGroup.class);
     private final Logger logger = LoggerFactory.getLogger(NikobusModuleHandler.class);
     private final Map<String, Integer> cachedStates = new HashMap<>();
-    private final List<ChannelUID> linkedChannels = new ArrayList<>();
 
     protected NikobusModuleHandler(Thing thing) {
         super(thing);
     }
 
+    @Override
+    public void initialize() {
+        super.initialize();
+
+        if (thing.getStatus() != ThingStatus.OFFLINE) {
+            // Fetch all linked channels to get initial values.
+            thing.getChannels().forEach(channel -> refreshChannel(channel.getUID()));
+        }
+    }
+
     @Override
     public void dispose() {
         super.dispose();
@@ -67,6 +75,8 @@ abstract class NikobusModuleHandler extends NikobusBaseThingHandler {
 
     @Override
     public void handleCommand(ChannelUID channelUID, Command command) {
+        logger.debug("handleCommand '{}' for channel '{}'", command, channelUID.getId());
+
         if (command instanceof RefreshType) {
             refreshChannel(channelUID);
         } else {
@@ -85,26 +95,11 @@ abstract class NikobusModuleHandler extends NikobusBaseThingHandler {
         updateGroup(SwitchModuleGroup.mapFromChannel(channelUID));
     }
 
-    @Override
-    public void channelLinked(ChannelUID channelUID) {
-        synchronized (linkedChannels) {
-            linkedChannels.add(channelUID);
-        }
-        super.channelLinked(channelUID);
-    }
-
-    @Override
-    public void channelUnlinked(ChannelUID channelUID) {
-        synchronized (linkedChannels) {
-            linkedChannels.remove(channelUID);
-        }
-        super.channelUnlinked(channelUID);
-    }
-
     public void refreshModule() {
         Set<SwitchModuleGroup> groups = new HashSet<>();
-        synchronized (linkedChannels) {
+        for (Channel channel : thing.getChannels()) {
-            for (ChannelUID channelUID : linkedChannels) {
+            ChannelUID channelUID = channel.getUID();
+            if (isLinked(channelUID)) {
                 groups.add(SwitchModuleGroup.mapFromChannel(channelUID));
             }
         }

bundles/org.openhab.binding.nikobus/src/main/java/org/openhab/binding/nikobus/internal/handler/NikobusPushButtonHandler.java

@@ -118,27 +118,41 @@ public class NikobusPushButtonHandler extends NikobusBaseThingHandler {
 
         impactedModules.clear();
 
-        try {
+        Object impactedModulesObject = getConfig().get(CONFIG_IMPACTED_MODULES);
-            ThingUID bridgeUID = thing.getBridgeUID();
+        if (impactedModulesObject != null) {
-            if (bridgeUID == null) {
+            try {
-                throw new IllegalArgumentException("Bridge does not exist!");
+                Bridge bridge = getBridge();
+                if (bridge == null) {
+                    throw new IllegalArgumentException("Bridge does not exist!");
+                }
+
+                ThingUID bridgeUID = thing.getBridgeUID();
+                if (bridgeUID == null) {
+                    throw new IllegalArgumentException("Unable to read BridgeUID!");
+                }
+
+                String[] impactedModulesString = impactedModulesObject.toString().split(",");
+                for (String impactedModuleString : impactedModulesString) {
+                    ImpactedModuleUID impactedModuleUID = new ImpactedModuleUID(impactedModuleString.trim());
+                    ThingTypeUID thingTypeUID = new ThingTypeUID(bridgeUID.getBindingId(),
+                            impactedModuleUID.getThingTypeId());
+                    ThingUID thingUID = new ThingUID(thingTypeUID, bridgeUID, impactedModuleUID.getThingId());
+
+                    if (!bridge.getThings().stream().anyMatch(thing -> thing.getUID().equals(thingUID))) {
+                        throw new IllegalArgumentException(
+                                "Impacted module " + thingUID + " not found for '" + impactedModuleString + "'");
+                    }
+
+                    impactedModules.add(new ImpactedModule(thingUID, impactedModuleUID.getGroup()));
+                }
+            } catch (RuntimeException e) {
+                updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, e.getMessage());
+                return;
             }
 
-            String[] impactedModulesString = getConfig().get(CONFIG_IMPACTED_MODULES).toString().split(",");
+            logger.debug("Impacted modules for {} = {}", thing.getUID(), impactedModules);
-            for (String impactedModuleString : impactedModulesString) {
-                ImpactedModuleUID impactedModuleUID = new ImpactedModuleUID(impactedModuleString.trim());
-                ThingTypeUID thingTypeUID = new ThingTypeUID(bridgeUID.getBindingId(),
-                        impactedModuleUID.getThingTypeId());
-                ThingUID thingUID = new ThingUID(thingTypeUID, bridgeUID, impactedModuleUID.getThingId());
-                impactedModules.add(new ImpactedModule(thingUID, impactedModuleUID.getGroup()));
-            }
-        } catch (RuntimeException e) {
-            updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, e.getMessage());
-            return;
         }
 
-        logger.debug("Impacted modules for {} = {}", thing.getUID(), impactedModules);
-
         NikobusPcLinkHandler pcLink = getPcLink();
         if (pcLink != null) {
             pcLink.addListener(getAddress(), this::commandReceived);
@@ -183,8 +197,10 @@ public class NikobusPushButtonHandler extends NikobusBaseThingHandler {
 
         updateState(CHANNEL_BUTTON, OnOffType.ON);
 
-        Utils.cancel(requestUpdateFuture);
+        if (!impactedModules.isEmpty()) {
-        requestUpdateFuture = scheduler.schedule(this::update, 400, TimeUnit.MILLISECONDS);
+            Utils.cancel(requestUpdateFuture);
+            requestUpdateFuture = scheduler.schedule(this::update, 400, TimeUnit.MILLISECONDS);
+        }
     }
 
     private void update() {

bundles/org.openhab.binding.nikobus/src/main/java/org/openhab/binding/nikobus/internal/utils/CRCUtil.java

@@ -12,7 +12,8 @@
  */
 package org.openhab.binding.nikobus.internal.utils;
 
-import org.apache.commons.lang.StringUtils;
+import org.eclipse.jdt.annotation.NonNullByDefault;
+import org.eclipse.jdt.annotation.Nullable;
 import org.openhab.core.util.HexUtils;
 
 /**
@@ -21,6 +22,7 @@ import org.openhab.core.util.HexUtils;
  * @author Davy Vanherbergen - Initial contribution
  * @author Boris Krivonog - Removed dependency to javax.xml.bind.DatatypeConverter
  */
+@NonNullByDefault
 public class CRCUtil {
 
     private static final int CRC_INIT = 0xFFFF;
@@ -35,7 +37,7 @@ public class CRCUtil {
      *            String representing hex numbers.
      * @return input string + CRC.
      */
-    public static String appendCRC(String input) {
+    public static @Nullable String appendCRC(@Nullable String input) {
         if (input == null) {
             return null;
         }
@@ -54,7 +56,7 @@ public class CRCUtil {
         }
 
         check = check & CRC_INIT;
-        String checksum = StringUtils.leftPad(Integer.toHexString(check), 4, "0");
+        String checksum = leftPadWithZeros(Integer.toHexString(check), 4);
         return (input + checksum).toUpperCase();
     }
 
@@ -85,6 +87,14 @@ public class CRCUtil {
             }
         }
 
-        return input + StringUtils.leftPad(Integer.toHexString(check), 2, "0").toUpperCase();
+        return input + leftPadWithZeros(Integer.toHexString(check), 2).toUpperCase();
+    }
+
+    private static String leftPadWithZeros(String text, int size) {
+        StringBuilder builder = new StringBuilder(text);
+        while (builder.length() < size) {
+            builder.insert(0, '0');
+        }
+        return builder.toString();
     }
 }

bundles/org.openhab.binding.nikobus/src/main/java/org/openhab/binding/nikobus/internal/utils/Utils.java

@@ -14,13 +14,17 @@ package org.openhab.binding.nikobus.internal.utils;
 
 import java.util.concurrent.Future;
 
+import org.eclipse.jdt.annotation.NonNullByDefault;
+import org.eclipse.jdt.annotation.Nullable;
+
 /**
  * The {@link Utils} class defines commonly used utility functions.
  *
  * @author Boris Krivonog - Initial contribution
  */
+@NonNullByDefault
 public class Utils {
-    public static void cancel(Future<?> future) {
+    public static void cancel(@Nullable Future<?> future) {
         if (future != null) {
             future.cancel(true);
         }

bundles/org.openhab.binding.nikobus/src/main/resources/OH-INF/thing/thing-types.xml

@@ -42,7 +42,7 @@
 			</parameter>
 			<parameter name="impactedModules" type="text">
 				<label>Impacted Modules</label>
-				<description>Comma separated list of impacted modules, i.e. 4C6C-1,4C6C-2</description>
+				<description>Comma separated list of impacted modules, i.e. switch-module:s1:1</description>
 			</parameter>
 		</config-description>
 	</thing-type>