Refactor ZWave node actions to return status messages

Changed ZWaveThingActions and related handlers to return descriptive String status messages instead of booleans for node operations (check failed, remove, reinit, heal). Improved user feedback for battery nodes and error cases. Updated i18n labels and descriptions for clarity. Fixed RemoveFailedNodeMessageClass to remove fixed callback and adjusted related test.

Signed-off-by: Bob Eckhoff <katmandodo@yahoo.com>

Author: apella12

src/main/java/org/openhab/binding/zwave/actions/ZWaveThingActions.java

@@ -35,31 +35,31 @@
 public class ZWaveThingActions implements ThingActions {
     private @Nullable ZWaveThingHandler handler;
 
-    public static boolean setNodeAsFailed(ThingActions actions) {
+    public static String checkIsNodeFailed(ThingActions actions) {
         if (actions instanceof ZWaveThingActions nodeActions) {
-            return nodeActions.setNodeAsFailed();
+            return nodeActions.checkIsNodeFailed();
         } else {
             throw new IllegalArgumentException("The 'actions' argument is not an instance of ZWaveThingActions");
         }
     }
 
-    public static boolean removeFailedNode(ThingActions actions) {
+    public static String removeFailedNode(ThingActions actions) {
         if (actions instanceof ZWaveThingActions nodeActions) {
             return nodeActions.removeFailedNode();
         } else {
             throw new IllegalArgumentException("The 'actions' argument is not an instance of ZWaveThingActions");
         }
     }
 
-    public static boolean reinitNode(ThingActions actions) {
+    public static String reinitNode(ThingActions actions) {
         if (actions instanceof ZWaveThingActions nodeActions) {
             return nodeActions.reinitNode();
         } else {
             throw new IllegalArgumentException("The 'actions' argument is not an instance of ZWaveThingActions");
         }
     }
 
-    public static boolean healNode(ThingActions actions) {
+    public static String healNode(ThingActions actions) {
         if (actions instanceof ZWaveThingActions nodeActions) {
             return nodeActions.healNode();
         } else {
@@ -78,38 +78,38 @@ public void setThingHandler(ThingHandler thingHandler) {
     }
 
     @RuleAction(label = "@text/actions.node-failed.label", description = "@text/actions.node-failed.description", visibility = Visibility.EXPERT)
-    public @ActionOutput(type = "boolean", label = "Success") boolean setNodeAsFailed() {
+    public @ActionOutput(type = "String") String checkIsNodeFailed() {
         ZWaveThingHandler handler = this.handler;
         if (handler != null) {
-            return handler.setNodeAsFailed();
+            return handler.checkIsNodeFailed();
         }
-        return false;
+        return "Thing handler is null, check not possible";
     }
 
     @RuleAction(label = "@text/actions.node-remove.label", description = "@text/actions.node-remove.description", visibility = Visibility.EXPERT)
-    public @ActionOutput(type = "boolean", label = "Success") boolean removeFailedNode() {
+    public @ActionOutput(type = "String") String removeFailedNode() {
         ZWaveThingHandler handler = this.handler;
         if (handler != null) {
             return handler.removeFailedNode();
         }
-        return false;
+        return "Thing handler is null, removal not possible";
     }
 
     @RuleAction(label = "@text/actions.node-reinit.label", description = "@text/actions.node-reinit.description")
-    public @ActionOutput(type = "boolean", label = "Success") boolean reinitNode() {
+    public @ActionOutput(type = "String") String reinitNode() {
         ZWaveThingHandler handler = this.handler;
         if (handler != null) {
             return handler.reinitNode();
         }
-        return false;
+        return "Thing handler is null, re-interview not possible";
     }
 
     @RuleAction(label = "@text/actions.node-heal.label", description = "@text/actions.node-heal.description")
-    public @ActionOutput(type = "boolean", label = "Success") boolean healNode() {
+    public @ActionOutput(type = "String") String healNode() {
         ZWaveThingHandler handler = this.handler;
         if (handler != null) {
             return handler.healNode();
         }
-        return false;
+        return "Thing handler is null, Heal not possible.";
     }
 }

src/main/java/org/openhab/binding/zwave/handler/ZWaveControllerHandler.java

@@ -559,7 +559,7 @@ public void replaceFailedNode(int nodeId) {
         if (controller == null) {
             return;
         }
-        controller.requestSetFailedNode(nodeId);
+        controller.replaceFailedNode(nodeId);
     }
 
     public void reinitialiseNode(int nodeId) {
@@ -569,19 +569,19 @@ public void reinitialiseNode(int nodeId) {
         controller.reinitialiseNode(nodeId);
     }
 
-    public boolean healNode(int nodeId) {
+    public String healNode(int nodeId) {
         if (controller == null) {
-            return false;
+            return "Controller is null, Heal not possible.";
         }
         ZWaveNode node = controller.getNode(nodeId);
         if (node == null) {
             logger.debug("NODE {}: Can't be found!", nodeId);
-            return false;
+            return "Node not found, Heal not possible.";
         }
 
         node.healNode();
 
-        return true;
+        return "Rebuilding route tables started on node " + nodeId;
     }
 
     public boolean isControllerMaster() {

src/main/java/org/openhab/binding/zwave/handler/ZWaveThingHandler.java

@@ -1102,27 +1102,35 @@ public void handleConfigurationUpdate(Map<String, Object> configurationParameter
         }
     }
 
-    public boolean setNodeAsFailed() {
+    public String checkIsNodeFailed() {
+        ZWaveNode node = controllerHandler.getNode(nodeId);
+        if (!node.isListening() && !node.isFrequentlyListening()) {
+            return "Battery (sleeping) nodes cannot be checked for failure";
+        }
         if (controllerHandler.getNode(nodeId).getNodeState() != ZWaveNodeState.FAILED) {
             controllerHandler.checkNodeFailed(nodeId);
-            return true;
+            return "Check for node failure started, check logs to confirm";
         }
-        return false;
+        return "Node is already in FAILED state";
     }
 
-    public boolean removeFailedNode() {
-        if (controllerHandler.getNode(nodeId).getNodeState() == ZWaveNodeState.FAILED) {
+    public String removeFailedNode() {
+        ZWaveNode node = controllerHandler.getNode(nodeId);
+        if (!node.isListening() && !node.isFrequentlyListening()) {
+            return "Battery (sleeping) nodes cannot be removed";
+        }        
+        if (node.getNodeState() == ZWaveNodeState.FAILED) {
             controllerHandler.removeFailedNode(nodeId);
-            return true;
+            return "Failed node remove started, check logs for errors";
         }
-        return false;
+        return "Node is not in FAILED state, cannot be removed";
     }
 
-    public boolean reinitNode() {
+    public String reinitNode() {
         ZWaveNode node = controllerHandler.getNode(nodeId);
 
         if (!node.isInitializationComplete()) {
-            return false;
+            return "Initialization not complete, re-interview not possible";
         }
 
         logger.debug("NODE {}: Re-initialising node!", nodeId);
@@ -1132,10 +1140,16 @@ public boolean reinitNode() {
         nodeSerializer.deleteNode(node.getHomeId(), nodeId);
 
         controllerHandler.reinitialiseNode(nodeId);
-        return true;
+        return "Re-interview started for node " + nodeId;
     }
 
-    public boolean healNode() {
+    public String healNode() {
+        ZWaveNode node = controllerHandler.getNode(nodeId);
+
+        if (!node.isInitializationComplete()) {
+            return "Initialization not complete, Heal not possible.";
+        }
+
         logger.debug("NODE {}: Starting heal on node!", nodeId);
         return controllerHandler.healNode(nodeId);
     }

src/main/java/org/openhab/binding/zwave/internal/protocol/SerialMessage.java

@@ -552,9 +552,9 @@ public enum SerialMessageClass {
         RequestNodeNeighborUpdateOptions(0x5a), // Allow options for request node neighbor update
         ExploreRequestInclusion(0x5e), // Initiate a Network-Wide Inclusion process
         RequestNodeInfo(0x60, true, false, true), // Get info (supported command classes) for the specified node
-        RemoveFailedNodeID(0x61, true, true, false), // Mark a specified node id as failed
+        RemoveFailedNodeID(0x61, true, true, false), // Remove a failed node from the controller
         IsFailedNodeID(0x62, true, false, false), // Check to see if a specified node has failed
-        ReplaceFailedNode(0x63, true, true, false), // Remove a failed node from the controller's list (?)
+        ReplaceFailedNode(0x63, true, true, false), // Replace a failed node with a new node of same number
         GetRoutingInfo(0x80, true, false, false), // Get a specified node's neighbor information from
                                                   // the controller
         LockRoute(0x90),

src/main/java/org/openhab/binding/zwave/internal/protocol/ZWaveController.java

@@ -850,12 +850,13 @@ public void requestRemoveFailedNode(int nodeId) {
     }
 
     /**
-     * Marks a node as failed
+     * Substitutes a failed node in the network. Note that this won't substitute
+     * nodes that are not marked as failed.
      *
      * @param nodeId
-     *            The address of the node to set failed
+     *            The address of the node to be replaced
      */
-    public void requestSetFailedNode(int nodeId) {
+    public void replaceFailedNode(int nodeId) {
         enqueue(new ReplaceFailedNodeMessageClass().doRequest(nodeId));
     }
 

src/main/java/org/openhab/binding/zwave/internal/protocol/serialmessage/IsFailedNodeMessageClass.java

@@ -52,11 +52,14 @@ public boolean handleResponse(ZWaveController zController, ZWaveTransaction tran
             return false;
         }
 
+        // With Thing Actions, need to inform the user via non-debug log the status.
+        // The Thing UI page may only show OFFLINE for both a success and failure when
+        // testing an OFFLINE node for failed node status.
         if (incomingMessage.getMessagePayloadByte(0) != 0x00) {
-            logger.debug("NODE {}: Is currently marked as failed by the controller!", nodeId);
+            logger.info("NODE {}: Is currently marked as failed by the controller!", nodeId);
             node.setNodeState(ZWaveNodeState.FAILED);
         } else {
-            logger.debug("NODE {}: Is currently marked as healthy by the controller", nodeId);
+            logger.info("NODE {}: Is currently marked as healthy by the controller", nodeId);
             // ZWaveNodeState.ALIVE is not necessarily true - The check is only whether the node is in 
             // the controller's failed nodes list. No ZWave traffic to the node is sent here to verify.
             // Node state should be updated by other mechanisms such as polling or application commands.

src/main/java/org/openhab/binding/zwave/internal/protocol/serialmessage/RemoveFailedNodeMessageClass.java

@@ -62,7 +62,7 @@ public ZWaveSerialPayload doRequest(int nodeId) {
         logger.debug("NODE {}: Marking node as having failed.", nodeId);
 
         // Create the request
-        return new ZWaveTransactionMessageBuilder(SerialMessageClass.RemoveFailedNodeID).withPayload(nodeId, 1).build();
+        return new ZWaveTransactionMessageBuilder(SerialMessageClass.RemoveFailedNodeID).withPayload(nodeId).build();
     }
 
     @Override

src/main/resources/OH-INF/i18n/actions.properties

@@ -11,10 +11,10 @@ actions.controller-sync.label=Synchronize network
 actions.controller-sync.description=Download the network with the SUC.
 
 actions.node-failed.label=Check if node is failed
-actions.node-failed.description=Check if the Controller considers this node has failed, not just offline.
+actions.node-failed.description=Check if the Controller considers this node is failed. For mains powered nodes only.
 
 actions.node-remove.label=Remove failed node from the controller
-actions.node-remove.description=ONLY works if the node is marked as failed. Normally use controller Exclude Devices action.
+actions.node-remove.description= Node must marked as failed first. For mains powered nodes only. Controller Exclude Devices is preferred.
 
 actions.node-reinit.label=Re-Interview the node
 actions.node-reinit.description=Clear all info about this node and make a new full interview.

src/test/java/org/openhab/binding/zwave/internal/protocol/serialmessage/RemoveFailedNodeMessageClassTest.java

@@ -24,13 +24,13 @@
  * Test cases for RemoveFailedNodeMessageClass message.
  * This takes some example packets, processes them, and checks that the processing is correct.
  *
- * @author Chris Jackson
+ * @author Chris Jackson - Initial contribution
  *
  */
 public class RemoveFailedNodeMessageClassTest {
     @Test
     public void doRequest() {
-        byte[] expectedResponse = { 12, 1 };
+        byte[] expectedResponse = { 12 };
 
         RemoveFailedNodeMessageClass handler = new RemoveFailedNodeMessageClass();
         ZWaveSerialPayload msg = handler.doRequest(12);