-
Notifications
You must be signed in to change notification settings - Fork 19
Add TimeSync support for nodes with TimeSynchronization cluster #440
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ import { | |
| BridgedDeviceBasicInformation, | ||
| GeneralCommissioning, | ||
| OperationalCredentials, | ||
| TimeSynchronization, | ||
| } from "@matter/main/clusters"; | ||
| import { | ||
| DecodedAttributeReportValue, | ||
|
|
@@ -117,6 +118,7 @@ import { formatNodeId } from "../util/formatNodeId.js"; | |
| import { pingIp } from "../util/network.js"; | ||
| import { CustomClusterPoller } from "./CustomClusterPoller.js"; | ||
| import { Nodes } from "./Nodes.js"; | ||
| import { TimeSyncManager } from "./TimeSyncManager.js"; | ||
|
|
||
| const logger = Logger.get("ControllerCommandHandler"); | ||
|
|
||
|
|
@@ -174,6 +176,8 @@ export class ControllerCommandHandler { | |
| #availableUpdates = new Map<NodeId, SoftwareUpdateInfo>(); | ||
| /** Poller for custom cluster attributes (Eve energy, etc.) */ | ||
| #customClusterPoller: CustomClusterPoller; | ||
| /** Manages time synchronization for nodes with the TimeSynchronization cluster */ | ||
| #timeSyncManager: TimeSyncManager; | ||
| /** Track the last known availability for each node to detect changes */ | ||
| #lastAvailability = new Map<NodeId, boolean>(); | ||
| /** Track in-flight invoke-commands for deduplication across all WebSocket connections */ | ||
|
|
@@ -207,6 +211,12 @@ export class ControllerCommandHandler { | |
| handleReadAttributes: (nodeId, paths, fabricFiltered) => | ||
| this.handleReadAttributes(nodeId, paths, fabricFiltered), | ||
| }); | ||
|
|
||
| // Initialize time sync manager for nodes with TimeSynchronization cluster | ||
| this.#timeSyncManager = new TimeSyncManager({ | ||
| syncTime: nodeId => this.#syncNodeTime(nodeId), | ||
| nodeConnected: nodeId => !!(this.#nodes.has(nodeId) && this.#nodes.get(nodeId).isConnected), | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -286,9 +296,22 @@ export class ControllerCommandHandler { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Send a setUtcTime command to a node's TimeSynchronization cluster. | ||
| */ | ||
| async #syncNodeTime(nodeId: NodeId): Promise<void> { | ||
| const client = this.#nodes.clusterClientByIdFor(nodeId, EndpointNumber(0), TimeSynchronization.Cluster.id); | ||
| await client.commands.setUtcTime({ | ||
| utcTime: Date.now() * 1000, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we have matter.js Time.nowMs that can be used here |
||
| granularity: TimeSynchronization.Granularity.MillisecondsGranularity, | ||
| timeSource: TimeSynchronization.TimeSource.Admin, | ||
| }); | ||
| } | ||
|
|
||
| close() { | ||
| if (!this.#started) return; | ||
| this.#customClusterPoller.stop(); | ||
| this.#timeSyncManager.stop(); | ||
| return this.#controller.close(); | ||
| } | ||
|
|
||
|
|
@@ -317,7 +340,10 @@ export class ControllerCommandHandler { | |
| this.events.nodeStructureChanged.emit(nodeId); | ||
| } | ||
| }); | ||
| node.events.eventTriggered.on(data => this.events.eventChanged.emit(nodeId, data)); | ||
| node.events.eventTriggered.on(data => { | ||
| this.events.eventChanged.emit(nodeId, data); | ||
| this.#timeSyncManager.handleEvent(nodeId, data); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we only care about one event then it is better to subscribe for this one event using node.eventsOf(TimeSynchronizationClient).xxy.on(...) and add this to an ObserverGroup to clean up on close |
||
| }); | ||
| node.events.stateChanged.on(state => { | ||
| if (state === NodeStates.Disconnected) { | ||
| return; | ||
|
|
@@ -331,10 +357,11 @@ export class ControllerCommandHandler { | |
| // Only refresh cache on Connected state (not Reconnecting, WaitingForDiscovery, etc.) | ||
| if (state === NodeStates.Connected) { | ||
| attributeCache.update(node); | ||
| // Register for custom cluster polling (e.g., Eve energy) after cache is updated | ||
| // Register for custom cluster polling (e.g., Eve energy) and time sync after cache is updated | ||
| const attributes = attributeCache.get(nodeId); | ||
| if (attributes) { | ||
| this.#customClusterPoller.registerNode(nodeId, attributes); | ||
| this.#timeSyncManager.registerNode(nodeId, attributes); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -380,10 +407,11 @@ export class ControllerCommandHandler { | |
| // Initialize attribute cache if node is already initialized | ||
| if (node.initialized) { | ||
| attributeCache.add(node); | ||
| // Register for custom cluster polling (e.g., Eve energy) | ||
| // Register for custom cluster polling (e.g., Eve energy) and time sync | ||
| const attributes = attributeCache.get(nodeId); | ||
| if (attributes) { | ||
| this.#customClusterPoller.registerNode(nodeId, attributes); | ||
| this.#timeSyncManager.registerNode(nodeId, attributes); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -1193,8 +1221,9 @@ export class ControllerCommandHandler { | |
| await this.#controller.removeNode(nodeId, !!node?.isConnected); | ||
| // Remove node from storage (also clears attribute cache) | ||
| this.#nodes.delete(nodeId); | ||
| // Unregister from custom cluster polling | ||
| // Unregister from custom cluster polling and time sync | ||
| this.#customClusterPoller.unregisterNode(nodeId); | ||
| this.#timeSyncManager.unregisterNode(nodeId); | ||
| } | ||
|
|
||
| async openCommissioningWindow(data: OpenCommissioningWindowRequest): Promise<OpenCommissioningWindowResponse> { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,203 @@ | ||||||
| /** | ||||||
| * @license | ||||||
| * Copyright 2025-2026 Open Home Foundation | ||||||
| * SPDX-License-Identifier: Apache-2.0 | ||||||
| */ | ||||||
|
|
||||||
| /** | ||||||
| * Handles time synchronization for nodes with the TimeSynchronization cluster. | ||||||
| * Syncs UTC time on three triggers: | ||||||
| * 1. Node connects/reconnects (immediate) | ||||||
| * 2. timeFailure event from the node (reactive) | ||||||
| * 3. Periodic resync every 12 hours | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think 24h is sufficient because it is an intermediate solution anyway |
||||||
| */ | ||||||
|
|
||||||
| import { CancelablePromise, Duration, Logger, Millis, NodeId, Time, Timer } from "@matter/main"; | ||||||
| import { DecodedEventReportValue } from "@matter/main/protocol"; | ||||||
| import { AttributesData } from "../types/CommandHandler.js"; | ||||||
|
|
||||||
| const logger = Logger.get("TimeSyncManager"); | ||||||
|
|
||||||
| // TimeSynchronization cluster ID (0x0038 = 56 decimal) | ||||||
| const TIME_SYNC_CLUSTER_ID = 0x0038; | ||||||
|
|
||||||
| // timeFailure event ID within TimeSynchronization cluster | ||||||
| const TIME_FAILURE_EVENT_ID = 0x03; | ||||||
|
|
||||||
| // Periodic resync interval: 12 hours | ||||||
| const RESYNC_INTERVAL_MS = 12 * 60 * 60 * 1000; | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use "Hours(12)" (or better 24 as told above. and do not use a _MS naming |
||||||
|
|
||||||
| // Maximum initial delay in milliseconds (random 0-60s to stagger startup) | ||||||
| const MAX_INITIAL_DELAY_MS = 60_000; | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use "Seconds(60)" and also no _MS and better use 30-60mins because we need to ensure to not add additional traffic while the server tries to connect all nodes initially ... we have a bit of time when we do all this normally. We should also prevent initial "hey i connected" triggers from being executed while we startup ... |
||||||
|
|
||||||
| export interface TimeSyncConnector { | ||||||
| syncTime(nodeId: NodeId): Promise<void>; | ||||||
| nodeConnected(nodeId: NodeId): boolean; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Check if a node has the TimeSynchronization cluster based on its attribute cache. | ||||||
| * The cluster is always on endpoint 0 per the Matter spec. | ||||||
| */ | ||||||
| export function hasTimeSyncCluster(attributes: AttributesData): boolean { | ||||||
| const prefix = `0/${TIME_SYNC_CLUSTER_ID}/`; | ||||||
| for (const key of Object.keys(attributes)) { | ||||||
| if (key.startsWith(prefix)) { | ||||||
| return true; | ||||||
| } | ||||||
| } | ||||||
| return false; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Manages time synchronization for nodes with the TimeSynchronization cluster. | ||||||
| */ | ||||||
| export class TimeSyncManager { | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the whole class adds a lot of code duplication with the CustomClusterPoller.ts ... because this is done by AI anyway please refactor to use a common "NodeProcessor" class as basis and try to streamline the two use cases as sub classes |
||||||
| #registeredNodes = new Set<NodeId>(); | ||||||
| #resyncTimer: Timer; | ||||||
| #connector: TimeSyncConnector; | ||||||
| #isResyncing = false; | ||||||
| #currentDelayPromise?: CancelablePromise; | ||||||
| #closed = false; | ||||||
|
|
||||||
| constructor(connector: TimeSyncConnector) { | ||||||
| this.#connector = connector; | ||||||
| const delay = Millis(Math.random() * MAX_INITIAL_DELAY_MS); | ||||||
| this.#resyncTimer = Time.getTimer("time-sync-resync", delay, () => this.#resyncAllNodes()); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Register a node for time sync if it has the TimeSynchronization cluster. | ||||||
| * Call this after a node connects and its attributes are available. | ||||||
| * Immediately syncs time on the node (fire-and-forget). | ||||||
| */ | ||||||
| registerNode(nodeId: NodeId, attributes: AttributesData): void { | ||||||
| if (!hasTimeSyncCluster(attributes)) { | ||||||
| this.unregisterNode(nodeId); | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| const isNew = !this.#registeredNodes.has(nodeId); | ||||||
| this.#registeredNodes.add(nodeId); | ||||||
|
|
||||||
| if (isNew) { | ||||||
| logger.info(`Registered node ${nodeId} for time synchronization`); | ||||||
| } | ||||||
|
|
||||||
| // Sync time immediately on connect/reconnect | ||||||
| this.#syncNode(nodeId); | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see above: we need an initial startup protection delay! |
||||||
|
|
||||||
| // Start periodic resync if not already running | ||||||
| this.#scheduleResync(); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Unregister a node from time sync tracking. | ||||||
| */ | ||||||
| unregisterNode(nodeId: NodeId): void { | ||||||
| if (this.#registeredNodes.delete(nodeId)) { | ||||||
| logger.info(`Unregistered node ${nodeId} from time synchronization`); | ||||||
| } | ||||||
| if (this.#registeredNodes.size === 0) { | ||||||
| this.#resyncTimer.stop(); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Handle an event from a node. If it's a timeFailure event, sync time. | ||||||
| */ | ||||||
| handleEvent(nodeId: NodeId, data: DecodedEventReportValue<any>): void { | ||||||
| const { path } = data; | ||||||
| if (path.clusterId === TIME_SYNC_CLUSTER_ID && path.eventId === TIME_FAILURE_EVENT_ID) { | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The event can also be received because of the update on a reconnect so we need to ensure we do not execute the sync to often in parallel or such
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also see above directly subscribe to the event and not check any triggered event. thats too inefficient |
||||||
| logger.info(`Received timeFailure event from node ${nodeId}, syncing time`); | ||||||
| this.#syncNode(nodeId); | ||||||
| } | ||||||
| } | ||||||
|
Comment on lines
+109
to
+115
|
||||||
|
|
||||||
| /** | ||||||
| * Stop all time sync operations and cleanup. | ||||||
| */ | ||||||
| stop(): void { | ||||||
| this.#closed = true; | ||||||
| this.#currentDelayPromise?.cancel(new Error("Close")); | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should track the current "time sync promise" and await this here if any is in progress and so make this async to have a clean stop |
||||||
| this.#resyncTimer?.stop(); | ||||||
| this.#registeredNodes.clear(); | ||||||
| logger.info("Time sync manager stopped"); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Sync time on a single node (fire-and-forget with error handling). | ||||||
| */ | ||||||
| #syncNode(nodeId: NodeId): void { | ||||||
| if (this.#closed || !this.#registeredNodes.has(nodeId)) { | ||||||
| return; | ||||||
| } | ||||||
| if (!this.#connector.nodeConnected(nodeId)) { | ||||||
| logger.debug(`Node ${nodeId} not connected, skipping time sync`); | ||||||
| return; | ||||||
| } | ||||||
| this.#connector.syncTime(nodeId).then( | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as said above. track that promise and cleanup so that we can await it on close and it is not hidden |
||||||
| () => logger.info(`Synced time on node ${nodeId}`), | ||||||
| error => logger.warn(`Failed to sync time on node ${nodeId}:`, error), | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| #scheduleResync(): void { | ||||||
| if (this.#registeredNodes.size === 0 || this.#closed) { | ||||||
| return; | ||||||
| } | ||||||
| if (this.#resyncTimer?.isRunning || this.#isResyncing) { | ||||||
| return; | ||||||
| } | ||||||
| this.#resyncTimer.start(); | ||||||
| } | ||||||
|
|
||||||
| async #resyncAllNodes(): Promise<void> { | ||||||
| if (this.#isResyncing) { | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| const targetInterval = Millis(RESYNC_INTERVAL_MS); | ||||||
| if (this.#resyncTimer.interval !== targetInterval) { | ||||||
| this.#resyncTimer.interval = targetInterval; | ||||||
| } | ||||||
|
|
||||||
| this.#isResyncing = true; | ||||||
|
|
||||||
| let syncedNodes = 0; | ||||||
| try { | ||||||
| const nodes = Array.from(this.#registeredNodes); | ||||||
| for (let i = 0; i < nodes.length; i++) { | ||||||
| const nodeId = nodes[i]; | ||||||
| if (!this.#registeredNodes.has(nodeId)) { | ||||||
| continue; | ||||||
| } | ||||||
| if (!this.#connector.nodeConnected(nodeId)) { | ||||||
| continue; | ||||||
| } | ||||||
| syncedNodes++; | ||||||
| try { | ||||||
| await this.#connector.syncTime(nodeId); | ||||||
| logger.info(`Periodic resync: synced time on node ${nodeId}`); | ||||||
| } catch (error) { | ||||||
| logger.warn(`Periodic resync: failed to sync time on node ${nodeId}:`, error); | ||||||
| } | ||||||
|
Comment on lines
+167
to
+184
|
||||||
| // Small delay between nodes to avoid overwhelming the network | ||||||
| if (i < nodes.length - 1) { | ||||||
| this.#currentDelayPromise = Time.sleep("sleep", Millis(2_000)).finally(() => { | ||||||
|
||||||
| this.#currentDelayPromise = Time.sleep("sleep", Millis(2_000)).finally(() => { | |
| this.#currentDelayPromise = Time.sleep("time-sync-resync-delay", Millis(2_000)).finally(() => { |
Copilot
AI
Mar 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR adds significant periodic-resync behavior (interval adjustment, node iteration with inter-node delay, error handling, and rescheduling in finally), but the new unit tests don’t appear to cover this path. Add tests using fake timers to validate: (1) resync timer scheduling is started only when nodes are registered, (2) stop() cancels any in-flight delay and prevents further resync, (3) per-node delay is applied between nodes, and (4) failures don’t prevent remaining nodes from being processed/rescheduled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think thats more needed for it ... check home-assistant/core#166133 which basically implements anything correctly ... and we might also need a trigger on DST changes?