Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 33 additions & 4 deletions packages/ws-controller/src/controller/ControllerCommandHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
BridgedDeviceBasicInformation,
GeneralCommissioning,
OperationalCredentials,
TimeSynchronization,
} from "@matter/main/clusters";
import {
DecodedAttributeReportValue,
Expand Down Expand Up @@ -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");

Expand Down Expand Up @@ -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 */
Expand Down Expand Up @@ -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),
});
}

/**
Expand Down Expand Up @@ -286,9 +296,22 @@ export class ControllerCommandHandler {
}
}

/**
* Send a setUtcTime command to a node's TimeSynchronization cluster.
*/
async #syncNodeTime(nodeId: NodeId): Promise<void> {
Copy link
Copy Markdown
Collaborator

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?

const client = this.#nodes.clusterClientByIdFor(nodeId, EndpointNumber(0), TimeSynchronization.Cluster.id);
await client.commands.setUtcTime({
utcTime: Date.now() * 1000,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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();
}

Expand Down Expand Up @@ -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);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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;
Expand All @@ -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);
}
}

Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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> {
Expand Down
203 changes: 203 additions & 0 deletions packages/ws-controller/src/controller/TimeSyncManager.ts
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
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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 {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logs syncing time even when the manager is stopped or the node isn’t registered/connected (since #syncNode will early-return). Consider guarding here (e.g., check this.#closed, this.#registeredNodes.has(nodeId), and optionally this.#connector.nodeConnected(nodeId)) before logging, or have #syncNode return a boolean indicating whether a sync was actually attempted so the log reflects reality.

Copilot uses AI. Check for mistakes.

/**
* Stop all time sync operations and cleanup.
*/
stop(): void {
this.#closed = true;
this.#currentDelayPromise?.cancel(new Error("Close"));
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

syncedNodes is incremented before syncTime succeeds, but later the summary log says synced ${syncedNodes} nodes, which will over-report when there are failures. Track separate counters (e.g., attempted vs succeeded) or increment only after a successful await this.#connector.syncTime(nodeId) to keep the summary accurate.

Copilot uses AI. Check for mistakes.
// Small delay between nodes to avoid overwhelming the network
if (i < nodes.length - 1) {
this.#currentDelayPromise = Time.sleep("sleep", Millis(2_000)).finally(() => {
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The timer/sleep label \"sleep\" is very generic and makes diagnostics harder (and can be ambiguous if the underlying timer system uses names for tracing). Use a more specific name (e.g., \"time-sync-resync-delay\") so logs/traces clearly attribute these delays to TimeSyncManager.

Suggested change
this.#currentDelayPromise = Time.sleep("sleep", Millis(2_000)).finally(() => {
this.#currentDelayPromise = Time.sleep("time-sync-resync-delay", Millis(2_000)).finally(() => {

Copilot uses AI. Check for mistakes.
this.#currentDelayPromise = undefined;
});
await this.#currentDelayPromise;
}
}
} finally {
this.#isResyncing = false;
this.#scheduleResync();
}
if (syncedNodes > 0) {
logger.info(
`Periodic resync complete: synced ${syncedNodes} nodes. Next resync in ${Duration.format(this.#resyncTimer.interval)}`,
);
}
}
Comment on lines +155 to +202
Copy link

Copilot AI Mar 24, 2026

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.

Copilot uses AI. Check for mistakes.
}
Loading