Conversation
invokes are now batched (when device supports it and non-root-endopoint and not groups) within the same macro-task
There was a problem hiding this comment.
Pull request overview
This PR adds automatic command batching to improve efficiency when multiple commands are invoked on a Matter device within the same timer tick. Commands sent to devices that support batching (maxPathsPerInvoke > 1) are now collected and sent as a single batched invoke request, reducing network overhead.
Changes:
- Introduced
CommandInvokerbase class andCommandBatcherextension for intelligent command batching - Refactored
ClientCommandMethodto use the new invoker infrastructure - Added batching support in
ClientNodeInteractionwhile maintaining non-batching behavior for groups viaClientGroupInteraction
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/node/src/node/client/commands/CommandInvoker.ts | New base class for command invocation with immediate execution logic |
| packages/node/src/node/client/commands/CommandBatcher.ts | Implements batching logic that collects commands within a timer tick and sends them as a single request |
| packages/node/src/node/client/commands/ClientCommandMethod.ts | Refactored to use the new invoker.invoke() API for batching support |
| packages/node/src/node/client/ClientNodeInteraction.ts | Integrated CommandBatcher and added cleanup in close() method |
| packages/node/src/node/client/ClientGroupInteraction.ts | Overrides to use plain CommandInvoker without batching for groups |
| packages/node/src/node/client/PeerBehavior.ts | Updated import path for moved ClientCommandMethod |
| packages/node/src/node/ClientNode.ts | Added prepareRuntimeShutdown() to close interaction and clean up batcher |
| packages/node/test/node/CommandBatcherTest.ts | Comprehensive test suite covering batching scenarios, cache clearing, and error handling |
| packages/node/test/node/ClientNodeTest.ts | Updated tests to use MockTime.resolve() for batched commands |
| packages/node/test/behaviors/scenes-management/ScenesManagementServerTest.ts | Updated tests to use MockTime.resolve() for batched commands |
| packages/node/test/behaviors/group-key-management/GroupKeyManagementServerTest.ts | Added comment explaining why root endpoint commands don't need MockTime.resolve() |
| CHANGELOG.md | Documented the enhancement |
| * | ||
| * Extends {@link CommandInvoker} to add batching capability on top of single command invocation. | ||
| */ | ||
| export class CommandBatcher extends CommandInvoker { |
There was a problem hiding this comment.
Would CommandBatch be an OK name?
Also, wondering if there's a reason to have a single instance for all batches vs. creating on-demand and integrating close/flush into a single process
| async [name](this: ClusterBehavior, fields?: {}) { | ||
| const node = this.env.get(Node) as ClientNode; | ||
|
|
||
| return (node.interaction as ClientNodeInteraction).invoker.invoke({ |
There was a problem hiding this comment.
What's the benefit of implementing this outside of ClientNodeInteraction vs. making it an internal implementation detail of invoke?
| * This class is used directly for groups (which don't support batching) | ||
| * and extended by {@link CommandBatcher} for nodes that support batched invokes. | ||
| */ | ||
| export class CommandInvoker { |
There was a problem hiding this comment.
Is there a more general way to implement this so that it only deals with an Interactable rather than a Node? Then it wouldn't need to be in @matter/node
The benefit of sticking stuff in @matter/protocol is that it avoids tight coupling between components... In order to keep packages well balanced I think we should consider @matter/protocol to include both low-level protocol implementation and associated helper logic. Otherwise I worry @matter/node is going to just become another big intertwined @project-chip/matter.js.
Same goes for CommandBatcher
| /** | ||
| * The node this interaction is associated with. | ||
| */ | ||
| protected get node(): ClientNode { |
There was a problem hiding this comment.
I'm a little leary of exposing internal implementation details like this as if you need to ask an Interactable what it's node is then you're limiting the flexibility of your logic.
And we already have a way to convey "this logic requires a ClientNodeInteraction + ClientNode", which is just to take the ClientNode as an argument instead of ClientNodeInteraction
2 participants
invokes are now batched (when device supports it and non-root-endopoint and not groups) within the same macro-task