Add TimeSync support for nodes with TimeSynchronization cluster#440
Add TimeSync support for nodes with TimeSynchronization cluster#440markvp wants to merge 5 commits intomatter-js:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds proactive UTC time synchronization for Matter nodes that implement the TimeSynchronization cluster, ensuring devices that lose time after power loss are resynced on reconnect, on timeFailure, and periodically.
Changes:
- Introduce
TimeSyncManagerwith cluster detection and periodic resync scheduling - Integrate
TimeSyncManagerinto controller lifecycle (connect, events, removal, close) - Add ws-controller test compilation config and unit tests for sync triggers
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ws-controller/tsconfig.json | Adds test project reference so ws-controller tests are typechecked/compiled. |
| packages/ws-controller/test/tsconfig.json | New tsconfig for ws-controller tests, wiring in shared test settings and references. |
| packages/ws-controller/test/controller/TimeSyncManagerTest.ts | New unit tests for cluster detection + immediate/reactive sync behavior. |
| packages/ws-controller/src/controller/TimeSyncManager.ts | Implements the TimeSyncManager (register/unregister, event-driven sync, periodic resync). |
| packages/ws-controller/src/controller/ControllerCommandHandler.ts | Wires TimeSyncManager into node lifecycle/events and adds setUtcTime command sender. |
|
How will it work when multiple matter controller wants to sync time. For example, if I connected my IKEA ALPSTUGA to 2 matters server with different time. I know it's an edge case but we have concurrency issue because each control may have different logic to 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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Also see above directly subscribe to the event and not check any triggered event. thats too inefficient
There was a problem hiding this comment.
Fixed: syncNode() now tracks in-flight syncs via Map<NodeId, Promise<void>>. Calling syncNode() when a sync is already in-progress for that node is a no-op (with a debug log). The promise is cleaned up in a .finally() block.
There was a problem hiding this comment.
The event handler is now in a per-node ObserverGroup with direct filtering (clusterId + eventId) before calling syncNode(). The generic handleEvent method is removed.
| /** | ||
| * Send a setUtcTime command to a node's TimeSynchronization cluster. | ||
| */ | ||
| async #syncNodeTime(nodeId: NodeId): Promise<void> { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Thanks for the reference. The HA PR uses ObserverGroup for event cleanup (which we've now adopted). Regarding DST changes: the 24-hour periodic resync is sufficient for DST correction. Pushing UTC epoch time to Matter nodes means DST is irrelevant on the device side — the device converts UTC to local time itself. No additional DST trigger is needed.
| 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, |
There was a problem hiding this comment.
we have matter.js Time.nowMs that can be used here
There was a problem hiding this comment.
Fixed: now uses Time.nowMs instead of Date.now().
| 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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Adopted ObserverGroup per node. Note: node.eventsOf(TimeSynchronizationClient) is not available because TimeSynchronizationClient is in @matter/node which is not a dependency of ws-controller. Instead, node.events.eventTriggered is filtered directly by clusterId === TIME_SYNC_CLUSTER_ID && eventId === TIME_FAILURE_EVENT_ID. The ObserverGroup per node ensures cleanup on decommission (close() in decommissionNode) and on manager stop.
| * Syncs UTC time on three triggers: | ||
| * 1. Node connects/reconnects (immediate) | ||
| * 2. timeFailure event from the node (reactive) | ||
| * 3. Periodic resync every 12 hours |
There was a problem hiding this comment.
i think 24h is sufficient because it is an intermediate solution anyway
There was a problem hiding this comment.
Changed to Hours(24).
| const RESYNC_INTERVAL_MS = 12 * 60 * 60 * 1000; | ||
|
|
||
| // Maximum initial delay in milliseconds (random 0-60s to stagger startup) | ||
| const MAX_INITIAL_DELAY_MS = 60_000; |
There was a problem hiding this comment.
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 ...
There was a problem hiding this comment.
Fixed: startup delay is now Minutes(30) + Math.random() * Minutes(30) (random 30–60 min). Startup protection is implemented via a #startupComplete flag that is set to true only after the first periodic resync cycle fires. During the startup window, registerNode does not trigger immediate syncs. The first periodic cycle handles all registered nodes and sets the flag.
| } | ||
|
|
||
| // Sync time immediately on connect/reconnect | ||
| this.#syncNode(nodeId); |
There was a problem hiding this comment.
see above: we need an initial startup protection delay!
There was a problem hiding this comment.
Added: #startupComplete flag blocks immediate syncs until the first periodic cycle completes (30–60 min after startup).
| */ | ||
| stop(): void { | ||
| this.#closed = true; | ||
| this.#currentDelayPromise?.cancel(new Error("Close")); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Fixed: stop() is now async. It cancels the delay promise, stops the timer, then await Promise.allSettled(#inFlightSyncs.values()) before clearing.
| logger.debug(`Node ${nodeId} not connected, skipping time sync`); | ||
| return; | ||
| } | ||
| this.#connector.syncTime(nodeId).then( |
There was a problem hiding this comment.
as said above. track that promise and cleanup so that we can await it on close and it is not hidden
There was a problem hiding this comment.
Done: #inFlightSyncs: Map<NodeId, Promise<void>> tracks all in-flight immediate syncs. stop() awaits all of them via Promise.allSettled() before clearing.
| /** | ||
| * Manages time synchronization for nodes with the TimeSynchronization cluster. | ||
| */ | ||
| export class TimeSyncManager { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Done: extracted NodeProcessor abstract base class (packages/ws-controller/src/controller/NodeProcessor.ts). Both TimeSyncManager and CustomClusterPoller now extend it. The base handles timer lifecycle, node registration/unregistration, the per-node processing loop (with 2s inter-node delay and re-entrancy guard), and async stop(). Subclasses implement shouldProcess(nodeId), processNode(nodeId), and the optional onCycleComplete() hook.
|
@piitaya I completely agree :-) Thats why also the real official solution is a bit more effort :-) ... and I need to real all spec on it |
In Home Assistant the Supervisor exposes if the time go synchronized through NTP (see Currently we don't have an event when synchronization state changes 😢 . So we'd probably have to poll the flag for a while if it is false on startup. |
|
Ok, so from what I get ... the interestung question is how to determine IF a host time is synced with any relevant source and so a valid source for it ... and when that happened ... that's maybe a tough one |
|
Sooo, we had a discussion yesterday because we should know if the host is synced time wise:
|
|
@Apollon77 do you want me to work on this or do you want to close it. IF you want me to work on it, please let me know what you would like me to do / not do. |
|
Give all my comments to your AI and I guess he can work with it. If you like feel free to continue. If now I will take it over somewhen ;-) |
|
Would be great to get this one finally over the line. FWIW, having an ENV var to enable the time sync would work for me. |
Review addressedAll feedback from @Apollon77 and @copilot-pull-request-reviewer has been incorporated in the latest commit. Summary of changes: New:
New:
Tests rewritten
|
…n cluster Introduces opt-in time synchronization via --enable-time-sync / ENABLE_TIME_SYNC. Adds NodeProcessor abstract base class for timer-driven periodic node processing, and TimeSyncManager which extends it to sync UTC time using PeerAddress (matching the CustomClusterPoller upstream migration). Syncs on reconnect after startup window and periodically every 24 hours. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rClientByIdFor Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Eliminates duplicated timer/loop boilerplate (~50 lines) by extending the shared NodeProcessor abstraction. Per-node attribute paths stay in the subclass; shouldProcess, processNode, and onCycleComplete implement the polling logic. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Tests cover: registration (startup window guard, connectivity check, cluster detection), syncNode (immediate fire, deduplication, in-flight tracking), unregisterNode, periodic resync via NodeProcessor timer (startup delay, 24h cycle, skip disconnected/in-flight peers), and stop() awaiting in-flight syncs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Math.random() produces a float; @matter/testing's MockTimer requires integer milliseconds and throws on fractional values. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@Apollon77 please can you confirm if my updates address all your concerns or if there are any other changes required? thanks for your patience while I try to get this right :) |
|
@markvp Yes I have seen the updates and did not had time so far to review. |
Summary
TimeSyncManagerthat proactively syncs UTC time on nodes with the TimeSynchronization cluster (0x38)timeFailureevent (reactive), and periodic resync every 12 hoursCustomClusterPollerpattern for consistencyContext
Devices like IKEA ALPSTUGA lack battery backup and lose their time after power loss. The controller previously only set time during commissioning, causing
timeSynchronization.timeFailureevents after reconnection.The
setUtcTimecommand is sent withMillisecondsGranularityandTimeSource.Admin.TlvEpochUshandles automatic conversion from Unix epoch to Matter epoch (2000-01-01).Changes
packages/ws-controller/src/controller/TimeSyncManager.ts—TimeSyncManagerclass andhasTimeSyncCluster()utilitypackages/ws-controller/test/controller/TimeSyncManagerTest.ts— 12 unit testspackages/ws-controller/test/tsconfig.json— enables test compilation for ws-controllerpackages/ws-controller/src/controller/ControllerCommandHandler.ts— integrates TimeSyncManager at all lifecycle pointspackages/ws-controller/tsconfig.json— adds test referenceTest plan
npm run formatpassesnpm run lintpasses (0 warnings, 0 errors)npm run buildpasses (including test type validation)npm test— all 12 ws-controller unit tests passCloses #245
🤖 Generated with Claude Code