High level API fixes December 23 (#582)

* Only try to connect to controller node when commissioned fixes #579 pt 1 * Ignore node start errors when starting multiple nodes fixes #579 pt 2 On first error the initialization of the other nodes was not tried. * Call commissioningChanged callback on factoryReset * Clear really all contexts on factory reset * Changelog * Only bulk-ignore start errors on bulk starts * Introduce Decommissioned state info on PairedNode * Adjust tests * Add unique check for storage keys and log name on node-start-error
project-chip · Dec 16, 2023 · 383ffe4 · 383ffe4
1 parent 6c74a63
commit 383ffe4
Show file tree

Hide file tree

Showing 8 changed files with 92 additions and 40 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,19 @@ The main work (all changes without a GitHub username in brackets in the below li
 	## __WORK IN PROGRESS__
 -->
 
+## __WORK IN PROGRESS__
+* Matter-Core functionality:
+  * Feature: Added CASE Authenticated Tags support (initialization from NOC and validation only)
+  * Enhancement: Added validation handling to Invoke processing
+  * Fix: Fixed message size check to allow processing of two big non matter UDP messages
+* matter.js API:
+  * Feature: Added NodeStateInfo state "Decommissioned" to inform from about a successful decommissioning of a device
+  * Feature: Added check that provided unique storageKeys are really unique
+  * Fix: Makes sure to initialize all nodes in the MatterServer on startup also if errors occur on single ones
+  * Fix: Only try to connect to a commissioned device in controller if it has at least one
+  * Fix: Makes sure to call commissioningChanged callback when device is factory reset
+  * Fix: Really remove all data in factory reset of a device
+
 ## 0.7.2 (2023-12-04)
 * Matter-Core functionality:
   * Corrected default values for TemperatureMeasurement Cluster

diff --git a/packages/matter-node-shell.js/src/shell/cmd_nodes.ts b/packages/matter-node-shell.js/src/shell/cmd_nodes.ts
@@ -43,6 +43,9 @@ export function createDiagnosticCallbacks(): Partial<CommissioningControllerNode
                 case NodeStateInformation.StructureChanged:
                     console.log(`stateInformationCallback Node ${peerNodeId} structure changed`);
                     break;
+                case NodeStateInformation.Decommissioned:
+                    console.log(`stateInformationCallback Node ${peerNodeId} decommissioned`);
+                    break;
             }
         },
     };

diff --git a/packages/matter-node.js-examples/src/examples/ControllerNode.ts b/packages/matter-node.js-examples/src/examples/ControllerNode.ts
@@ -258,6 +258,9 @@ class ControllerNode {
                         case NodeStateInformation.StructureChanged:
                             console.log(`stateInformationCallback ${peerNodeId}: Node ${nodeId} structure changed`);
                             break;
+                        case NodeStateInformation.Decommissioned:
+                            console.log(`stateInformationCallback ${peerNodeId}: Node ${nodeId} decommissioned`);
+                            break;
                     }
                 },
             });

diff --git a/packages/matter-node.js/test/IntegrationTest.ts b/packages/matter-node.js/test/IntegrationTest.ts
@@ -1614,8 +1614,9 @@ describe("Integration Test", () => {
             assert.equal(commissioningChangedCallsServer[2].fabricIndex, FabricIndex(1));
             assert.equal(commissioningChangedCallsServer2.length, 1);
 
-            assert.equal(nodeStateChangesController1Node1.length, 2);
+            assert.equal(nodeStateChangesController1Node1.length, 3);
             assert.equal(nodeStateChangesController1Node1[1].nodeState, NodeStateInformation.Disconnected);
+            assert.equal(nodeStateChangesController1Node1[2].nodeState, NodeStateInformation.Decommissioned);
         });
 
         it("read and remove second node by removing fabric from device unplanned and doing factory reset", async () => {

diff --git a/packages/matter.js/src/CommissioningController.ts b/packages/matter.js/src/CommissioningController.ts
@@ -393,7 +393,7 @@ export class CommissioningController extends MatterNode {
         if (this.controllerInstance === undefined) {
             this.controllerInstance = await this.initializeController();
         }
-        if (this.options.autoConnect !== false) {
+        if (this.options.autoConnect !== false && this.controllerInstance.isCommissioned()) {
             await this.connect();
         }
     }

diff --git a/packages/matter.js/src/CommissioningServer.ts b/packages/matter.js/src/CommissioningServer.ts
@@ -50,6 +50,7 @@ import { VendorId } from "./datatype/VendorId.js";
 import { Aggregator } from "./device/Aggregator.js";
 import { Device, RootEndpoint } from "./device/Device.js";
 import { Endpoint } from "./device/Endpoint.js";
+import { Fabric } from "./fabric/Fabric.js";
 import { Logger } from "./log/Logger.js";
 import { MdnsBroadcaster } from "./mdns/MdnsBroadcaster.js";
 import { MdnsInstanceBroadcaster } from "./mdns/MdnsInstanceBroadcaster.js";
@@ -872,14 +873,17 @@ export class CommissioningServer extends MatterNode {
             );
         }
         const wasStarted = this.interactionServer !== undefined || this.deviceInstance !== undefined;
+        let fabrics = new Array<Fabric>();
         if (wasStarted) {
+            fabrics = this.isCommissioned() ? this.deviceInstance?.getFabrics() ?? [] : [];
             await this.close();
         }
 
-        this.storage.clear();
+        this.storage.clearAll();
 
         if (wasStarted) {
             await this.advertise();
+            fabrics.forEach(fabric => this.options.commissioningChangedCallback?.(fabric.fabricIndex));
         }
         logger.info(`The device was factory reset${wasStarted ? " and restarted" : ""}.`);
     }

diff --git a/packages/matter.js/src/MatterServer.ts b/packages/matter.js/src/MatterServer.ts
@@ -52,7 +52,7 @@ export type MatterServerOptions = {
  */
 export class MatterServer {
     private started = false;
-    private readonly nodes: MatterNode[] = [];
+    private readonly nodes = new Map<string, MatterNode>();
 
     private mdnsScanner?: MdnsScanner;
     private mdnsBroadcaster?: MdnsBroadcaster;
@@ -77,7 +77,7 @@ export class MatterServer {
     private getNextMatterPort(desiredPort?: number) {
         // Build a temporary map with all ports in use
         const portCheckMap = new Map<number, boolean>();
-        for (const node of this.nodes) {
+        for (const node of this.nodes.values()) {
             const nodePort = node.getPort();
             if (nodePort === undefined) continue;
             if (portCheckMap.has(nodePort)) {
@@ -124,12 +124,15 @@ export class MatterServer {
      * @param nodeOptions Optional options for the node (e.g. unique node id)
      */
     async addCommissioningServer(commissioningServer: CommissioningServer, nodeOptions?: NodeOptions) {
+        const storageKey = nodeOptions?.uniqueStorageKey ?? nodeOptions?.uniqueNodeId ?? this.nodes.size.toString();
+        if (this.nodes.has(storageKey)) {
+            throw new Error(`Node with storage key "${storageKey}" already exists.`);
+        }
         commissioningServer.setPort(this.getNextMatterPort(commissioningServer.getPort()));
-        const storageKey = nodeOptions?.uniqueStorageKey ?? nodeOptions?.uniqueNodeId ?? this.nodes.length.toString();
         commissioningServer.setStorage(this.storageManager.createContext(storageKey));
         logger.debug(`Adding CommissioningServer using storage key "${storageKey}".`);
         await this.prepareNode(commissioningServer);
-        this.nodes.push(commissioningServer);
+        this.nodes.set(storageKey, commissioningServer);
     }
 
     /**
@@ -140,26 +143,29 @@ export class MatterServer {
      * @param destroyStorage If true the storage context will be destroyed
      */
     async removeCommissioningServer(commissioningServer: CommissioningServer, destroyStorage = false) {
-        // Remove instance from list
-        const index = this.nodes.indexOf(commissioningServer);
-        if (index < 0) {
-            throw new Error("CommissioningServer not found");
-        }
-        this.nodes.splice(index, 1);
+        // Find instance from list
+        for (const [key, value] of this.nodes.entries()) {
+            if (value === commissioningServer) {
+                this.nodes.delete(key);
 
-        const port = commissioningServer.getPort();
-        if (port !== undefined) {
-            // Remember port to not reuse for this run if not needed to prevent issues with controllers
-            this.formerlyUsedPorts.push(port);
-        }
+                const port = commissioningServer.getPort();
+                if (port !== undefined) {
+                    // Remember port to not reuse for this run if not needed to prevent issues with controllers
+                    this.formerlyUsedPorts.push(port);
+                }
 
-        // Close instance
-        await commissioningServer.close();
+                // Close instance
+                await commissioningServer.close();
 
-        if (destroyStorage) {
-            // Destroy storage
-            await commissioningServer.factoryReset();
+                if (destroyStorage) {
+                    // Destroy storage
+                    await commissioningServer.factoryReset();
+                }
+                return;
+            }
         }
+
+        throw new Error("CommissioningServer to remove not found.");
     }
 
     /**
@@ -169,17 +175,21 @@ export class MatterServer {
      * @param nodeOptions Optional options for the node (e.g. unique node id)
      */
     async addCommissioningController(commissioningController: CommissioningController, nodeOptions?: NodeOptions) {
+        const storageKey = nodeOptions?.uniqueStorageKey ?? nodeOptions?.uniqueNodeId ?? this.nodes.size.toString();
+        if (this.nodes.has(storageKey)) {
+            throw new Error(`Node with storage key "${storageKey}" already exists.`);
+        }
+
         const localPort = commissioningController.getPort();
         if (localPort !== undefined) {
             // If a local port for controller is defined verify that the port is not overlapping with other nodes
             // Method throws if port is already used
             this.getNextMatterPort(localPort);
         }
-        const storageKey = nodeOptions?.uniqueStorageKey ?? nodeOptions?.uniqueNodeId ?? this.nodes.length.toString();
         commissioningController.setStorage(this.storageManager.createContext(storageKey));
         logger.debug(`Adding CommissioningController using storage key "${storageKey}".`);
         await this.prepareNode(commissioningController);
-        this.nodes.push(commissioningController);
+        this.nodes.set(storageKey, commissioningController);
     }
 
     /**
@@ -189,20 +199,23 @@ export class MatterServer {
      * @param destroyStorage If true the storage context will be destroyed
      */
     async removeCommissioningController(commissioningController: CommissioningController, destroyStorage = false) {
-        // Remove instance from list
-        const index = this.nodes.indexOf(commissioningController);
-        if (index < 0) {
-            throw new Error("CommissioningController not found");
-        }
-        this.nodes.splice(index, 1);
+        // Find instance from list
+        for (const [key, value] of this.nodes.entries()) {
+            if (value === commissioningController) {
+                this.nodes.delete(key);
 
-        // Close instance
-        await commissioningController.close();
+                // Close instance
+                await commissioningController.close();
 
-        if (destroyStorage) {
-            // Destroy storage
-            commissioningController.resetStorage();
+                if (destroyStorage) {
+                    // Destroy storage
+                    commissioningController.resetStorage();
+                }
+                return;
+            }
         }
+
+        throw new Error("CommissioningController to remove not found.");
     }
 
     /**
@@ -223,8 +236,13 @@ export class MatterServer {
             });
         }
         this.started = true;
-        for (const node of this.nodes) {
-            await this.prepareNode(node);
+        for (const [key, node] of this.nodes.entries()) {
+            try {
+                await this.prepareNode(node);
+            } catch (error) {
+                // TODO: Find a better way how to report back such issues and which nodes errored
+                logger.error(`Failed to start node with storageKey ${key}: ${error}`);
+            }
         }
     }
 
@@ -245,8 +263,12 @@ export class MatterServer {
      * Close the server and all nodes
      */
     async close() {
-        for (const node of this.nodes) {
-            await node.close();
+        for (const [key, node] of this.nodes.entries()) {
+            try {
+                await node.close();
+            } catch (error) {
+                logger.error(`Failed to close node with storageKey ${key}: ${error}`);
+            }
         }
         await this.mdnsBroadcaster?.close();
         this.mdnsBroadcaster = undefined;

diff --git a/packages/matter.js/src/device/PairedNode.ts b/packages/matter.js/src/device/PairedNode.ts
@@ -81,6 +81,11 @@ export enum NodeStateInformation {
      * This State information will only be fired when the subscribeAllAttributesAndEvents option is set to true.
      */
     StructureChanged,
+
+    /**
+     * The node was just Decommissioned.
+     */
+    Decommissioned,
 }
 
 export type CommissioningControllerNodeOptions = {
@@ -643,6 +648,7 @@ export class PairedNode {
             );
         }
         this.setConnectionState(NodeStateInformation.Disconnected);
+        this.options.stateInformationCallback?.(this.nodeId, NodeStateInformation.Decommissioned);
         await this.commissioningController.removeNode(this.nodeId, false);
     }