From d3423c8b050c547d151d0ab89cd7a518107425f7 Mon Sep 17 00:00:00 2001 From: Ingo Fischer Date: Mon, 6 Nov 2023 17:22:21 +0100 Subject: [PATCH] Address review feedback --- .../matter-node-ble.js/src/ble/BleScanner.ts | 15 ++-- .../src/shell/cmd_commission.ts | 50 +---------- .../src/shell/cmd_nodes.ts | 82 +++++++++--------- .../matter.js/src/CommissioningController.ts | 32 +++---- packages/matter.js/src/MatterController.ts | 83 ++++++++++--------- packages/matter.js/src/device/PairedNode.ts | 25 ++++-- packages/matter.js/src/mdns/MdnsScanner.ts | 58 ++++--------- 7 files changed, 150 insertions(+), 195 deletions(-) diff --git a/packages/matter-node-ble.js/src/ble/BleScanner.ts b/packages/matter-node-ble.js/src/ble/BleScanner.ts index 7f29c94ebe..9bf2c8c76b 100644 --- a/packages/matter-node-ble.js/src/ble/BleScanner.ts +++ b/packages/matter-node-ble.js/src/ble/BleScanner.ts @@ -34,7 +34,11 @@ type CommissionableDeviceData = CommissionableDevice & { export class BleScanner implements Scanner { private readonly recordWaiters = new Map< string, - { resolver: () => void; timer: Timer; resolveOnUpdatedRecords: boolean } + { + resolver: () => void; + timer: Timer; + resolveOnUpdatedRecords: boolean; + } >(); private readonly discoveredMatterDevices = new Map(); @@ -65,7 +69,7 @@ export class BleScanner implements Scanner { resolveOnUpdatedRecords ? "" : " (not resolving on updated records)" }`, ); - return { promise }; + await promise; } /** @@ -221,11 +225,9 @@ export class BleScanner implements Scanner { let storedRecords = this.getCommissionableDevices(identifier); if (storedRecords.length === 0) { const queryKey = this.buildCommissionableQueryIdentifier(identifier); - const { promise } = await this.registerWaiterPromise(queryKey, timeoutSeconds); await this.nobleClient.startScanning(); - - await promise; + await this.registerWaiterPromise(queryKey, timeoutSeconds); storedRecords = this.getCommissionableDevices(identifier); await this.nobleClient.stopScanning(); @@ -257,8 +259,7 @@ export class BleScanner implements Scanner { if (remainingTime <= 0) { break; } - const { promise } = await this.registerWaiterPromise(queryKey, remainingTime, false); - await promise; + await this.registerWaiterPromise(queryKey, remainingTime, false); } await this.nobleClient.stopScanning(); return this.getCommissionableDevices(identifier).map(({ deviceData }) => deviceData); diff --git a/packages/matter-node-shell.js/src/shell/cmd_commission.ts b/packages/matter-node-shell.js/src/shell/cmd_commission.ts index 271c3a3b4c..626adee177 100644 --- a/packages/matter-node-shell.js/src/shell/cmd_commission.ts +++ b/packages/matter-node-shell.js/src/shell/cmd_commission.ts @@ -7,11 +7,11 @@ import { NodeCommissioningOptions } from "@project-chip/matter-node.js"; import { BasicInformationCluster, DescriptorCluster, GeneralCommissioning } from "@project-chip/matter-node.js/cluster"; import { NodeId } from "@project-chip/matter-node.js/datatype"; -import { NodeStateInformation } from "@project-chip/matter-node.js/device"; import { Logger } from "@project-chip/matter-node.js/log"; import { ManualPairingCodeCodec, QrCode } from "@project-chip/matter-node.js/schema"; import type { Argv } from "yargs"; import { MatterNode } from "../MatterNode"; +import { createDiagnosticCallbacks } from "./cmd_nodes"; export default function commands(theNode: MatterNode) { return { @@ -92,53 +92,7 @@ export default function commands(theNode: MatterNode) { }, }, passcode: setupPinCode, - attributeChangedCallback: ( - peerNodeId, - { path: { nodeId, clusterId, endpointId, attributeName }, value }, - ) => - console.log( - `attributeChangedCallback ${peerNodeId} Attribute ${nodeId}/${endpointId}/${clusterId}/${attributeName} changed to ${Logger.toJSON( - value, - )}`, - ), - eventTriggeredCallback: ( - peerNodeId, - { path: { nodeId, clusterId, endpointId, eventName }, events }, - ) => - console.log( - `eventTriggeredCallback ${peerNodeId} Event ${nodeId}/${endpointId}/${clusterId}/${eventName} triggered with ${Logger.toJSON( - events, - )}`, - ), - stateInformationCallback: (peerNodeId, info) => { - switch (info) { - case NodeStateInformation.Connected: - console.log( - `stateInformationCallback: Node ${peerNodeId} connected`, - ); - break; - case NodeStateInformation.Disconnected: - console.log( - `stateInformationCallback: Node ${peerNodeId} disconnected`, - ); - break; - case NodeStateInformation.Reconnecting: - console.log( - `stateInformationCallback: Node ${peerNodeId} reconnecting`, - ); - break; - case NodeStateInformation.WaitingForDeviceDiscovery: - console.log( - `stateInformationCallback: Node ${peerNodeId} waiting that device gets discovered again`, - ); - break; - case NodeStateInformation.StructureChanged: - console.log( - `stateInformationCallback: Node ${peerNodeId} structure changed`, - ); - break; - } - }, + ...createDiagnosticCallbacks(), } as NodeCommissioningOptions; options.commissioning = { diff --git a/packages/matter-node-shell.js/src/shell/cmd_nodes.ts b/packages/matter-node-shell.js/src/shell/cmd_nodes.ts index 1649fb9677..28a803a298 100644 --- a/packages/matter-node-shell.js/src/shell/cmd_nodes.ts +++ b/packages/matter-node-shell.js/src/shell/cmd_nodes.ts @@ -5,11 +5,49 @@ */ import { NodeId } from "@project-chip/matter-node.js/datatype"; -import { NodeStateInformation } from "@project-chip/matter-node.js/device"; +import { CommissioningControllerNodeOptions, NodeStateInformation } from "@project-chip/matter-node.js/device"; import { Logger } from "@project-chip/matter-node.js/log"; import type { Argv } from "yargs"; import { MatterNode } from "../MatterNode"; +export function createDiagnosticCallbacks(): Partial { + return { + attributeChangedCallback: (peerNodeId, { path: { nodeId, clusterId, endpointId, attributeName }, value }) => + console.log( + `attributeChangedCallback ${peerNodeId}: Attribute ${nodeId}/${endpointId}/${clusterId}/${attributeName} changed to ${Logger.toJSON( + value, + )}`, + ), + eventTriggeredCallback: (peerNodeId, { path: { nodeId, clusterId, endpointId, eventName }, events }) => + console.log( + `eventTriggeredCallback ${peerNodeId}: Event ${nodeId}/${endpointId}/${clusterId}/${eventName} triggered with ${Logger.toJSON( + events, + )}`, + ), + stateInformationCallback: (peerNodeId, info) => { + switch (info) { + case NodeStateInformation.Connected: + console.log(`stateInformationCallback Node ${peerNodeId} connected`); + break; + case NodeStateInformation.Disconnected: + console.log(`stateInformationCallback Node ${peerNodeId} disconnected`); + break; + case NodeStateInformation.Reconnecting: + console.log(`stateInformationCallback Node ${peerNodeId} reconnecting`); + break; + case NodeStateInformation.WaitingForDeviceDiscovery: + console.log( + `stateInformationCallback Node ${peerNodeId} waiting that device gets discovered again`, + ); + break; + case NodeStateInformation.StructureChanged: + console.log(`stateInformationCallback Node ${peerNodeId} structure changed`); + break; + } + }, + }; +} + export default function commands(theNode: MatterNode) { return { command: ["nodes", "node"], @@ -114,47 +152,7 @@ export default function commands(theNode: MatterNode) { subscribeMaxIntervalCeilingSeconds: autoSubscribe ? maxSubscriptionInterval ?? 30 : undefined, - attributeChangedCallback: ( - peerNodeId, - { path: { nodeId, clusterId, endpointId, attributeName }, value }, - ) => - console.log( - `attributeChangedCallback ${peerNodeId}: Attribute ${nodeId}/${endpointId}/${clusterId}/${attributeName} changed to ${Logger.toJSON( - value, - )}`, - ), - eventTriggeredCallback: ( - peerNodeId, - { path: { nodeId, clusterId, endpointId, eventName }, events }, - ) => - console.log( - `eventTriggeredCallback ${peerNodeId}: Event ${nodeId}/${endpointId}/${clusterId}/${eventName} triggered with ${Logger.toJSON( - events, - )}`, - ), - stateInformationCallback: (peerNodeId, info) => { - switch (info) { - case NodeStateInformation.Connected: - console.log(`stateInformationCallback Node ${peerNodeId} connected`); - break; - case NodeStateInformation.Disconnected: - console.log(`stateInformationCallback Node ${peerNodeId} disconnected`); - break; - case NodeStateInformation.Reconnecting: - console.log(`stateInformationCallback Node ${peerNodeId} reconnecting`); - break; - case NodeStateInformation.WaitingForDeviceDiscovery: - console.log( - `stateInformationCallback Node ${peerNodeId} waiting that device gets discovered again`, - ); - break; - case NodeStateInformation.StructureChanged: - console.log( - `stateInformationCallback Node ${peerNodeId} structure changed`, - ); - break; - } - }, + ...createDiagnosticCallbacks(), }); } }, diff --git a/packages/matter.js/src/CommissioningController.ts b/packages/matter.js/src/CommissioningController.ts index 3efa8eadd1..0912f3a691 100644 --- a/packages/matter.js/src/CommissioningController.ts +++ b/packages/matter.js/src/CommissioningController.ts @@ -75,19 +75,23 @@ export type NodeCommissioningOptions = CommissioningControllerNodeOptions & { commissioning?: CommissioningOptions; /** Discovery related options. */ - discovery: { - /** - * Device identifiers (Short or Long Discriminator, Product/Vendor-Ids, Device-type or a pre-discovered - * instance Id, or "nothing" to discover all commissionable matter devices) to use for discovery. - */ - identifierData: CommissionableDeviceIdentifiers; - - /** - * Commissionable device object returned by a discovery run. - * If this property is provided then identifierData and knownAddress are ignored. - */ - commissionableDevice?: CommissionableDevice; - + discovery: ( + | { + /** + * Device identifiers (Short or Long Discriminator, Product/Vendor-Ids, Device-type or a pre-discovered + * instance Id, or "nothing" to discover all commissionable matter devices) to use for discovery. + * If the property commissionableDevice is provided this property is ignored. + */ + identifierData: CommissionableDeviceIdentifiers; + } + | { + /** + * Commissionable device object returned by a discovery run. + * If this property is provided then identifierData and knownAddress are ignored. + */ + commissionableDevice: CommissionableDevice; + } + ) & { /** * Discovery capabilities to use for discovery. These are included in the QR code normally and defined if BLE * is supported for initial commissioning. @@ -248,7 +252,7 @@ export class CommissioningController extends MatterNode { const existingNode = this.connectedNodes.get(nodeId); if (existingNode !== undefined) { - if (!existingNode.isConnencted) { + if (!existingNode.isConnected) { await existingNode.reconnect(); } return existingNode; diff --git a/packages/matter.js/src/MatterController.ts b/packages/matter.js/src/MatterController.ts index 53c8487afd..920cbe2232 100644 --- a/packages/matter.js/src/MatterController.ts +++ b/packages/matter.js/src/MatterController.ts @@ -238,12 +238,15 @@ export class MatterController { regulatoryLocation: GeneralCommissioning.RegulatoryLocationType.Outdoor, // Set to the most restrictive if relevant regulatoryCountryCode: "XX", }, - discovery: { commissionableDevice, timeoutSeconds = 30 }, + discovery: { timeoutSeconds = 30 }, passcode, } = options; + const commissionableDevice = + "commissionableDevice" in options.discovery ? options.discovery.commissionableDevice : undefined; let { - discovery: { identifierData, discoveryCapabilities, knownAddress }, + discovery: { discoveryCapabilities, knownAddress }, } = options; + let identifierData = "identifierData" in options.discovery ? options.discovery.identifierData : {}; if (commissionableDevice !== undefined) { let { addresses } = commissionableDevice; @@ -446,47 +449,41 @@ export class MatterController { return peerNodeId; } - /** - * Resume a device connection and establish a CASE session that was previously paired with the controller. This - * method will try to connect to the device using the previously used server address (if set). If that fails, the - * device is discovered again using its operational instance details. - * It returns the operational MessageChannel on success. - */ - private async resume(peerNodeId: NodeId, timeoutSeconds?: number) { - const operationalAddress = this.getLastOperationalAddress(peerNodeId); - - const reconnectLastKnownAddress = async ( - operationalAddress: ServerAddressIp, - ): Promise | undefined> => { - const { ip, port } = operationalAddress; - try { - logger.debug(`Resume device connection to configured server at ${ip}:${port}`); - const channel = await this.pair(peerNodeId, operationalAddress); - this.setOperationalServerAddress(peerNodeId, operationalAddress); - return channel; - } catch (error) { - if ( - error instanceof RetransmissionLimitReachedError || - (error instanceof Error && error.message.includes("EHOSTUNREACH")) - ) { - logger.debug( - `Failed to resume device connection with ${ip}:${port}, discover the device ...`, - error, - ); - return undefined; - } else { - throw error; - } + private async reconnectLastKnownAddress( + peerNodeId: NodeId, + operationalAddress: ServerAddressIp, + ): Promise | undefined> { + const { ip, port } = operationalAddress; + try { + logger.debug(`Resume device connection to configured server at ${ip}:${port}`); + const channel = await this.pair(peerNodeId, operationalAddress); + this.setOperationalServerAddress(peerNodeId, operationalAddress); + return channel; + } catch (error) { + if ( + error instanceof RetransmissionLimitReachedError || + (error instanceof Error && error.message.includes("EHOSTUNREACH")) + ) { + logger.debug(`Failed to resume device connection with ${ip}:${port}, discover the device ...`, error); + return undefined; + } else { + throw error; } - }; + } + } + private async connectOrDiscoverNode( + peerNodeId: NodeId, + operationalAddress?: ServerAddressIp, + timeoutSeconds?: number, + ) { const discoveryPromises = new Array<() => Promise>>(); // Additionally to general discovery we also try to poll the formerly known operational address let reconnectionPollingTimer: Timer | undefined; if (operationalAddress !== undefined) { - const directReconnection = await reconnectLastKnownAddress(operationalAddress); + const directReconnection = await this.reconnectLastKnownAddress(peerNodeId, operationalAddress); if (directReconnection !== undefined) { return directReconnection; } @@ -497,7 +494,7 @@ export class MatterController { reconnectionPollingTimer = Time.getPeriodicTimer(RECONNECTION_POLLING_INTERVAL, async () => { try { logger.debug(`Polling for device at ${serverAddressToString(operationalAddress)} ...`); - const result = await reconnectLastKnownAddress(operationalAddress); + const result = await this.reconnectLastKnownAddress(peerNodeId, operationalAddress); if (result !== undefined && reconnectionPollingTimer?.isRunning) { reconnectionPollingTimer?.stop(); resolver(result); @@ -537,8 +534,20 @@ export class MatterController { return result; }); + return await anyPromise(discoveryPromises); + } + + /** + * Resume a device connection and establish a CASE session that was previously paired with the controller. This + * method will try to connect to the device using the previously used server address (if set). If that fails, the + * device is discovered again using its operational instance details. + * It returns the operational MessageChannel on success. + */ + private async resume(peerNodeId: NodeId, timeoutSeconds?: number) { + const operationalAddress = this.getLastOperationalAddress(peerNodeId); + try { - return await anyPromise(discoveryPromises); + return await this.connectOrDiscoverNode(peerNodeId, operationalAddress, timeoutSeconds); } catch (error) { if ( (error instanceof DiscoveryError || error instanceof PairRetransmissionLimitReachedError) && diff --git a/packages/matter.js/src/device/PairedNode.ts b/packages/matter.js/src/device/PairedNode.ts index f31fa5b37e..9045bad1fe 100644 --- a/packages/matter.js/src/device/PairedNode.ts +++ b/packages/matter.js/src/device/PairedNode.ts @@ -55,6 +55,9 @@ import { EndpointLoggingOptions, logEndpoint } from "./EndpointStructureLogger.j const logger = Logger.get("PairedNode"); +/** Delay after receiving a changed partList from a device to update the device structure */ +const STRUCTURE_UPDATE_TIMEOUT_MS = 5_000; // 5 seconds, TODO: Verify if this value makes sense in practice + export enum NodeStateInformation { /** Node is connected and all data is up-to-date. */ Connected, @@ -125,7 +128,10 @@ export type CommissioningControllerNodeOptions = { export class PairedNode { private readonly endpoints = new Map(); private interactionClient?: InteractionClient; - private readonly reconnectDelayTimer = Time.getTimer(5_000, async () => await this.reconnect()); + private readonly reconnectDelayTimer = Time.getTimer( + STRUCTURE_UPDATE_TIMEOUT_MS, + async () => await this.reconnect(), + ); private readonly updateEndpointStructureTimer = Time.getTimer( 5_000, async () => await this.updateEndpointStructure(), @@ -169,7 +175,7 @@ export class PairedNode { }); } - get isConnencted() { + get isConnected() { return this.connectionState === NodeStateInformation.Connected; } @@ -199,10 +205,17 @@ export class PairedNode { try { await this.initialize(); } catch (error) { - if (this.connectionState === NodeStateInformation.Disconnected) return; - logger.warn(`Node ${this.nodeId}: Error waiting for device rediscovery`, error); - this.setConnectionState(NodeStateInformation.WaitingForDeviceDiscovery); - await this.reconnect(); + if (error instanceof MatterError) { + // When we already know that the node is disconnected ignore all MatterErrors and rethrow all others + if (this.connectionState === NodeStateInformation.Disconnected) { + return; + } + logger.warn(`Node ${this.nodeId}: Error waiting for device rediscovery`, error); + this.setConnectionState(NodeStateInformation.WaitingForDeviceDiscovery); + await this.reconnect(); + } else { + throw error; + } } } diff --git a/packages/matter.js/src/mdns/MdnsScanner.ts b/packages/matter.js/src/mdns/MdnsScanner.ts index 126e09770f..f6a11ddfef 100644 --- a/packages/matter.js/src/mdns/MdnsScanner.ts +++ b/packages/matter.js/src/mdns/MdnsScanner.ts @@ -84,7 +84,11 @@ export class MdnsScanner implements Scanner { private readonly commissionableDeviceRecords = new Map(); private readonly recordWaiters = new Map< string, - { resolver: () => void; timer?: Timer; resolveOnUpdatedRecords: boolean } + { + resolver: () => void; + timer?: Timer; + resolveOnUpdatedRecords: boolean; + } >(); private readonly periodicTimer: Timer; private closing = false; @@ -470,60 +474,32 @@ export class MdnsScanner implements Scanner { return undefined; } - private getCommissionableQueryRecords(identifier: CommissionableDeviceIdentifiers) { - const queries = new Array(); + private getCommissionableQueryRecords(identifier: CommissionableDeviceIdentifiers): DnsQuery[] { + const names = new Array(); - queries.push({ - name: MATTER_COMMISSION_SERVICE_QNAME, - recordClass: DnsRecordClass.IN, - recordType: DnsRecordType.PTR, - }); + names.push(MATTER_COMMISSION_SERVICE_QNAME); if ("instanceId" in identifier) { - queries.push({ - name: getDeviceInstanceQname(identifier.instanceId), - recordClass: DnsRecordClass.IN, - recordType: DnsRecordType.PTR, - }); + names.push(getDeviceInstanceQname(identifier.instanceId)); } else if ("longDiscriminator" in identifier) { - queries.push({ - name: getLongDiscriminatorQname(identifier.longDiscriminator), - recordClass: DnsRecordClass.IN, - recordType: DnsRecordType.PTR, - }); + names.push(getLongDiscriminatorQname(identifier.longDiscriminator)); } else if ("shortDiscriminator" in identifier) { - queries.push({ - name: getShortDiscriminatorQname(identifier.shortDiscriminator), - recordClass: DnsRecordClass.IN, - recordType: DnsRecordType.PTR, - }); + names.push(getShortDiscriminatorQname(identifier.shortDiscriminator)); } else if ("vendorId" in identifier) { - queries.push({ - name: getVendorQname(identifier.vendorId), - recordClass: DnsRecordClass.IN, - recordType: DnsRecordType.PTR, - }); + names.push(getVendorQname(identifier.vendorId)); } else if ("deviceType" in identifier) { - queries.push({ - name: getDeviceTypeQname(identifier.deviceType), - recordClass: DnsRecordClass.IN, - recordType: DnsRecordType.PTR, - }); + names.push(getDeviceTypeQname(identifier.deviceType)); } else { // Other queries just scan for commissionable devices - queries.push({ - name: getCommissioningModeQname(), - recordClass: DnsRecordClass.IN, - recordType: DnsRecordType.PTR, - }); + names.push(getCommissioningModeQname()); } - return queries; + return names.map(name => ({ name, recordClass: DnsRecordClass.IN, recordType: DnsRecordType.PTR })); } /** - * Discovers commissionable devices based on a defined identifier for ma maximal given timeout, but returns the - * first found entries. If already a discovered device matches in the cache the query it is returned directly and + * Discovers commissionable devices based on a defined identifier for maximal given timeout, but returns the + * first found entries. If already a discovered device matches in the cache the response is returned directly and * no query is triggered. If no record exists a query is sent out and the promise gets fulfilled as soon as at least * one device is found. If no device is discovered in the defined timeframe an empty array is returned. When the * promise got fulfilled no more queries are send out, but more device entries might be added when discovered later.