From ef411c1c0ad4dde0c7215d9c9e8e4bc396505722 Mon Sep 17 00:00:00 2001 From: Thomas Hartwig Date: Wed, 16 Sep 2026 16:54:49 +0200 Subject: [PATCH] feat(cli): use the wildcard cluster read in tree -L 4 Replace the per-attribute read loop in treePopulateAttributes with one wildcard read per cluster, collapsing tree -L 4 from 1+N round-trips per cluster to 1. Level 3 keeps the cheap AttributeList-only read since it only needs names. The AttributeList discovered in a level-4 wildcard read's reports still write-throughs into the completion cache; a transport failure fails the whole cluster (TreeCluster.ListErr) while a per-attribute status stays scoped to that attribute, same as before. Closes #89. Co-Authored-By: Claude Sonnet 5 --- cli/attribute_cache_test.go | 168 +++++++++++++++++++++++++++++------- cli/output/tree.go | 2 +- cli/tree.go | 142 ++++++++++++++++++++++-------- 3 files changed, 244 insertions(+), 68 deletions(-) diff --git a/cli/attribute_cache_test.go b/cli/attribute_cache_test.go index 27acac0..49bfd2d 100644 --- a/cli/attribute_cache_test.go +++ b/cli/attribute_cache_test.go @@ -13,6 +13,7 @@ import ( "github.com/p0fi/matter-cli/cli/output" "github.com/p0fi/matter-cli/internal/store" + "github.com/p0fi/matter-cli/internal/tlv" "github.com/spf13/viper" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -44,6 +45,58 @@ func scriptedAttrLists(script map[attrListKey][]uint32, visited *[]attrListKey) var errBusy = errors.New("device busy") +// scriptedClusterReports builds a treeClusterReader that replays a fixed +// script and records the order in which clusters were visited, standing in +// for a level-4 wildcard read. Clusters absent from the script fail with +// errBusy, standing in for a device that is momentarily unreachable. +func scriptedClusterReports(script map[attrListKey][]attrReport, visited *[]attrListKey) treeClusterReader { + return func(_ context.Context, endpoint uint16, clusterID uint32) ([]attrReport, error) { + key := attrListKey{endpoint, clusterID} + if visited != nil { + *visited = append(*visited, key) + } + reports, ok := script[key] + if !ok { + return nil, errBusy + } + return reports, nil + } +} + +// failingListReader returns an attrListReader that fails the test if called, +// for asserting that level 4 never performs the level-3-only AttributeList +// read. +func failingListReader(t *testing.T) attrListReader { + return func(context.Context, uint16, uint32) ([]uint32, error) { + t.Helper() + t.Fatal("level 4 must not perform a separate AttributeList read") + return nil, nil + } +} + +// failingClusterReader returns a treeClusterReader that fails the test if +// called, for asserting that level 3 never performs a wildcard cluster read. +func failingClusterReader(t *testing.T) treeClusterReader { + return func(context.Context, uint16, uint32) ([]attrReport, error) { + t.Helper() + t.Fatal("level 3 must not perform a wildcard cluster read") + return nil, nil + } +} + +// tlvAttrList encodes an AttributeList (0xFFFB) payload the way a device +// would: a TLV array of attribute IDs. +func tlvAttrList(t *testing.T, ids ...uint32) []byte { + t.Helper() + w := tlv.NewWriter() + require.NoError(t, w.StartArray(tlv.AnonymousTag())) + for _, id := range ids { + require.NoError(t, w.PutUnsignedInt(tlv.AnonymousTag(), uint64(id))) + } + require.NoError(t, w.EndContainer()) + return w.Bytes() +} + // cacheTestNode is a node whose OnOff cluster already has a cached attribute // list from an earlier run, and whose LevelControl cluster has never been read. func cacheTestNode() *store.Node { @@ -209,10 +262,6 @@ func treeDataFor(node *store.Node, level int) *output.TreeData { // AttributeList read the tree already performs must write through to the same // cache `cluster discover` populates, with the same partial-failure semantics. func TestTreePopulateAttributes(t *testing.T) { - noValues := func(context.Context, uint16, uint32, uint32) (string, error) { - return "", errors.New("level 4 not requested") - } - t.Run("level 3 write-throughs every successful read", func(t *testing.T) { node := cacheTestNode() data := treeDataFor(node, 3) @@ -222,7 +271,7 @@ func TestTreePopulateAttributes(t *testing.T) { {1, 0x0008}: {0x0000, 0x0011}, }, nil) - updated := treePopulateAttributes(context.Background(), data, node, 3, read, noValues) + updated := treePopulateAttributes(context.Background(), data, node, 3, read, failingClusterReader(t)) assert.True(t, updated) assert.Equal(t, []uint32{0x0000, 0x4001}, clusterRef(t, node, 1, 0x0006).Attributes) @@ -240,7 +289,7 @@ func TestTreePopulateAttributes(t *testing.T) { {1, 0x0008}: {0x0000, 0x0011}, }, nil) - updated := treePopulateAttributes(context.Background(), data, node, 3, read, noValues) + updated := treePopulateAttributes(context.Background(), data, node, 3, read, failingClusterReader(t)) assert.True(t, updated, "other clusters were refreshed, so the node is worth persisting") assert.Equal(t, []uint32{0x0000, 0xFFFD}, clusterRef(t, node, 1, 0x0006).Attributes, @@ -255,51 +304,104 @@ func TestTreePopulateAttributes(t *testing.T) { data := treeDataFor(node, 3) read := scriptedAttrLists(nil, nil) - updated := treePopulateAttributes(context.Background(), data, node, 3, read, noValues) + updated := treePopulateAttributes(context.Background(), data, node, 3, read, failingClusterReader(t)) assert.False(t, updated) assert.Equal(t, []uint32{0x0000, 0xFFFD}, clusterRef(t, node, 1, 0x0006).Attributes) assert.Nil(t, clusterRef(t, node, 1, 0x0008).Attributes) }) - t.Run("level 4 also reads values without changing cache semantics", func(t *testing.T) { + t.Run("level 3 does not perform a wildcard cluster read", func(t *testing.T) { + node := cacheTestNode() + data := treeDataFor(node, 3) + read := scriptedAttrLists(map[attrListKey][]uint32{{1, 0x0006}: {0x0000}}, nil) + + treePopulateAttributes(context.Background(), data, node, 3, read, failingClusterReader(t)) + }) + + t.Run("level 4 reads every attribute in one wildcard read and caches the AttributeList it returns", func(t *testing.T) { node := cacheTestNode() data := treeDataFor(node, 4) - read := scriptedAttrLists(map[attrListKey][]uint32{ - {0, 0x001D}: {0x0000}, - {1, 0x0006}: {0x0000}, - {1, 0x0008}: {0x0000}, - }, nil) - readValue := func(_ context.Context, ep uint16, clID, attrID uint32) (string, error) { - if clID == 0x0008 { - return "", errBusy - } - return "42", nil + + reports := map[attrListKey][]attrReport{ + {0, 0x001D}: { + {attributeID: 0xFFFB, data: tlvAttrList(t, 0x0000)}, + {attributeID: 0x0000, data: tlvUint(t, 1)}, + }, + {1, 0x0006}: { + {attributeID: 0xFFFB, data: tlvAttrList(t, 0x0000)}, + {attributeID: 0x0000, data: tlvUint(t, 42)}, + }, + {1, 0x0008}: { + {attributeID: 0xFFFB, data: tlvAttrList(t, 0x0000, 0x0011)}, + {attributeID: 0x0000, err: errBusy}, + {attributeID: 0x0011, data: tlvUint(t, 5)}, + }, } + var visited []attrListKey + read := scriptedClusterReports(reports, &visited) - updated := treePopulateAttributes(context.Background(), data, node, 4, read, readValue) + updated := treePopulateAttributes(context.Background(), data, node, 4, failingListReader(t), read) assert.True(t, updated) - assert.Equal(t, "42", data.Endpoints[1].Clusters[0].Attrs[0].Value) - assert.NotEmpty(t, data.Endpoints[1].Clusters[1].Attrs[0].Err, - "a failed value read is surfaced per attribute") - assert.Equal(t, []uint32{0x0000}, clusterRef(t, node, 1, 0x0008).Attributes, - "a failed value read does not undo a successful AttributeList read") + assert.Equal(t, []attrListKey{{0, 0x001D}, {1, 0x0006}, {1, 0x0008}}, visited) + + onOff := data.Endpoints[1].Clusters[0] + require.Len(t, onOff.Attrs, 2) + assert.Equal(t, uint32(0x0000), onOff.Attrs[0].ID) + assert.Equal(t, "42", onOff.Attrs[0].Value) + + levelControl := data.Endpoints[1].Clusters[1] + require.Len(t, levelControl.Attrs, 3) + var statusAttrErr string + for _, a := range levelControl.Attrs { + if a.ID == 0x0000 { + statusAttrErr = a.Err + } + } + assert.NotEmpty(t, statusAttrErr, "a per-attribute status is surfaced without failing the whole cluster") + + assert.Equal(t, []uint32{0x0000, 0x0011}, clusterRef(t, node, 1, 0x0008).Attributes, + "the AttributeList in a successful wildcard read is cached even though one attribute in the same response errored") }) - t.Run("level 3 does not read values", func(t *testing.T) { + t.Run("a failed wildcard read keeps the stale cache and reports the error in the tree", func(t *testing.T) { node := cacheTestNode() - data := treeDataFor(node, 3) - read := scriptedAttrLists(map[attrListKey][]uint32{{1, 0x0006}: {0x0000}}, nil) + data := treeDataFor(node, 4) + // OnOff's wildcard read fails; LevelControl's succeeds. + reports := map[attrListKey][]attrReport{ + {0, 0x001D}: {{attributeID: 0xFFFB, data: tlvAttrList(t, 0x0000)}}, + {1, 0x0008}: {{attributeID: 0xFFFB, data: tlvAttrList(t, 0x0000)}}, + } + read := scriptedClusterReports(reports, nil) + + updated := treePopulateAttributes(context.Background(), data, node, 4, failingListReader(t), read) - valueReads := 0 - readValue := func(context.Context, uint16, uint32, uint32) (string, error) { - valueReads++ - return "", nil + assert.True(t, updated, "other clusters were refreshed, so the node is worth persisting") + assert.Equal(t, []uint32{0x0000, 0xFFFD}, clusterRef(t, node, 1, 0x0006).Attributes, + "the failing cluster must keep its previously cached list") + assert.Equal(t, []uint32{0x0000}, clusterRef(t, node, 1, 0x0008).Attributes) + assert.NotEmpty(t, data.Endpoints[1].Clusters[0].ListErr) + assert.Empty(t, data.Endpoints[1].Clusters[0].Attrs) + }) + + t.Run("a wildcard read missing AttributeList leaves the cache untouched", func(t *testing.T) { + node := cacheTestNode() + data := treeDataFor(node, 4) + reports := map[attrListKey][]attrReport{ + {0, 0x001D}: {{attributeID: 0xFFFB, data: tlvAttrList(t, 0x0000)}}, + {1, 0x0006}: {{attributeID: 0x0000, data: tlvUint(t, 1)}}, // no AttributeList in the response + {1, 0x0008}: {{attributeID: 0xFFFB, data: tlvAttrList(t, 0x0000)}}, } + read := scriptedClusterReports(reports, nil) + + treePopulateAttributes(context.Background(), data, node, 4, failingListReader(t), read) - treePopulateAttributes(context.Background(), data, node, 3, read, readValue) - assert.Zero(t, valueReads) + assert.Equal(t, []uint32{0x0000, 0xFFFD}, clusterRef(t, node, 1, 0x0006).Attributes, + "no AttributeList report means nothing to write through") + // The attribute the device did report is still shown, superset or not. + require.Len(t, data.Endpoints[1].Clusters[0].Attrs, 1) + assert.Equal(t, uint32(0x0000), data.Endpoints[1].Clusters[0].Attrs[0].ID) }) } diff --git a/cli/output/tree.go b/cli/output/tree.go index eed71ee..1910b4d 100644 --- a/cli/output/tree.go +++ b/cli/output/tree.go @@ -154,7 +154,7 @@ type TreeCluster struct { ID uint32 Name string Side string // "server" or "client" - ListErr string // non-empty if AttributeList read failed + ListErr string // non-empty if the attribute read (list-only at level 3, wildcard at level 4) failed Attrs []TreeAttribute // populated for level >= 3 } diff --git a/cli/tree.go b/cli/tree.go index 85b81c6..7fae3d4 100644 --- a/cli/tree.go +++ b/cli/tree.go @@ -32,8 +32,16 @@ const attrListAttrID uint32 = 0xFFFB // Per-read budgets for the live reads that levels 3 and 4 perform, so one // unresponsive cluster cannot stall the whole traversal. const ( - treeAttrListTimeout = 10 * time.Second - treeAttrValueTimeout = 5 * time.Second + treeAttrListTimeout = 10 * time.Second + + // treeClusterWildcardTimeout bounds a level-4 wildcard read of one + // cluster's attributes. Unlike the old per-attribute loop — where one + // slow attribute cost treeAttrValueTimeout and the rest still rendered — + // a wildcard read is bounded as a whole, so a stalled cluster now loses + // every attribute in it. It gets the same 30s budget `cluster read` + // uses (clusterReadTimeout), since a wildcard read returns more data + // than the AttributeList-only read treeAttrListTimeout was sized for. + treeClusterWildcardTimeout = clusterReadTimeout ) // globalAttrNames maps the standard Matter global attribute IDs (present on @@ -234,30 +242,36 @@ func buildTreeData(ctx context.Context, w io.Writer, node *store.Node, level int defer cancel() return treeReadAttrList(listCtx, dc, client, session, ep, clID) } - readValue := func(ctx context.Context, ep uint16, clID, attrID uint32) (string, error) { - valCtx, cancel := context.WithTimeout(ctx, treeAttrValueTimeout) + readCluster := func(ctx context.Context, ep uint16, clID uint32) ([]attrReport, error) { + clCtx, cancel := context.WithTimeout(ctx, treeClusterWildcardTimeout) defer cancel() - return treeReadAttrValue(valCtx, dc, client, session, ep, clID, attrID) + return treeReadClusterWildcard(clCtx, dc, client, session, ep, clID) } - cacheUpdated := treePopulateAttributes(ctx, data, node, level, readList, readValue) + cacheUpdated := treePopulateAttributes(ctx, data, node, level, readList, readCluster) // Complete step 2 with ✓ and leave the cursor on a clean line. stepper.Clear() return data, cacheUpdated, nil } -// treeAttrValueReader reads one attribute's already-formatted display value. -type treeAttrValueReader func(ctx context.Context, endpoint uint16, clusterID, attrID uint32) (string, error) +// treeClusterReader performs one wildcard read of every attribute a cluster +// instance reports, returning the reports in transport-neutral form. +type treeClusterReader func(ctx context.Context, endpoint uint16, clusterID uint32) ([]attrReport, error) // treePopulateAttributes fills each cluster in data with the attribute names it -// advertises and, at level 4, their values. Every AttributeList it reads -// successfully is also write-through into node's completion cache, so a tree run -// leaves attribute-name completion scoped exactly as `cluster discover` would. -// It reports whether any cache entry changed. +// advertises and, at level 4, their values. Level 3 uses the cheap AttributeList +// read since it only needs names; level 4 uses one wildcard read per cluster +// instead, since it needs every value anyway and a wildcard read returns them +// in the same round-trip AttributeList would have cost alone. Every +// AttributeList discovered — whether from the level-3 read or found among a +// level-4 wildcard read's reports — is also write-through into node's +// completion cache, so a tree run leaves attribute-name completion scoped +// exactly as `cluster discover` would. It reports whether any cache entry +// changed. // // The readers are injected so the traversal — including the partial-failure -// behaviour, where a cluster whose list read failed keeps its previously cached +// behaviour, where a cluster whose read failed keeps its previously cached // list — is testable without a device. func treePopulateAttributes( ctx context.Context, @@ -265,7 +279,7 @@ func treePopulateAttributes( node *store.Node, level int, readList attrListReader, - readValue treeAttrValueReader, + readCluster treeClusterReader, ) bool { cacheUpdated := false @@ -274,6 +288,13 @@ func treePopulateAttributes( for ci := range ep.Clusters { cl := &ep.Clusters[ci] + if level == 4 { + if treePopulateClusterWildcard(ctx, node, ep.ID, cl, readCluster) { + cacheUpdated = true + } + continue + } + attrIDs, listErr := readList(ctx, ep.ID, cl.ID) if recordAttrListResult(node, ep.ID, cl.ID, attrIDs, listErr) { cacheUpdated = true @@ -289,25 +310,65 @@ func treePopulateAttributes( Name: treeResolveAttrName(cl.ID, attrID), }) } + } + } - if level < 4 { - continue - } - for ai := range cl.Attrs { - attr := &cl.Attrs[ai] - value, valErr := readValue(ctx, ep.ID, cl.ID, attr.ID) - if valErr != nil { - attr.Err = treeFormatErr(valErr) - } else { - attr.Value = value - } - } + return cacheUpdated +} + +// treePopulateClusterWildcard fills cl with the attributes a single wildcard +// read reports and write-throughs the AttributeList found among them into +// node's completion cache. A transport failure fails the whole cluster — it +// becomes cl.ListErr, exactly as a failed AttributeList read does at level 3 — +// while a per-attribute status inside a successful read stays scoped to that +// one attribute via its TreeAttribute.Err, same as before. It reports whether +// the cache changed. +func treePopulateClusterWildcard(ctx context.Context, node *store.Node, endpoint uint16, cl *output.TreeCluster, readCluster treeClusterReader) bool { + reports, err := readCluster(ctx, endpoint, cl.ID) + if err != nil { + cl.ListErr = treeFormatErr(err) + return false + } + + cacheUpdated := false + if attrIDs, ok := treeExtractAttributeList(reports); ok { + cacheUpdated = recordAttrListResult(node, endpoint, cl.ID, attrIDs, nil) + } + + clInfo := &clusters.ClusterInfo{ID: cl.ID, DisplayName: cl.Name} + records := buildReadRecords(readTarget{nodeID: node.ID, endpoint: endpoint, cl: clInfo}, reports, time.Now()) + cl.Attrs = make([]output.TreeAttribute, 0, len(records)) + for _, rec := range records { + attr := output.TreeAttribute{ID: rec.AttributeID, Name: rec.Attribute} + if rec.Error != "" { + attr.Err = rec.Display + } else { + attr.Value = rec.Display } + cl.Attrs = append(cl.Attrs, attr) } return cacheUpdated } +// treeExtractAttributeList finds the AttributeList (0xFFFB) report among a +// wildcard read's reports and decodes it, reporting whether one was present +// and readable. A device that omits it, or answered it with a status instead +// of data, reports false — the caller must not write through in that case, so +// a partial or malformed response cannot wipe a previously cached list. +func treeExtractAttributeList(reports []attrReport) ([]uint32, bool) { + for _, r := range reports { + if r.attributeID != attrListAttrID { + continue + } + if r.err != nil { + return nil, false + } + return treeDecodeAttrList(r.data), true + } + return nil, false +} + // treeEstablishConnection returns either a daemon connection or a direct CASE // session. Exactly one of dc or (client, session) is non-nil on success. func treeEstablishConnection(ctx context.Context, nodeID uint64) ( @@ -405,16 +466,29 @@ func treeDecodeAttrList(raw []byte) []uint32 { return ids } -// treeReadAttrValue reads a single attribute and returns its formatted string value. -func treeReadAttrValue(ctx context.Context, dc *daemonNodeConn, client *interaction.Client, session *protocol.Session, ep uint16, clID, attrID uint32) (string, error) { - raw, err := treeReadAttrRaw(ctx, dc, client, session, ep, clID, attrID) - if err != nil { - return "", err +// treeReadClusterWildcard reads every attribute of one cluster instance in a +// single wildcard ReadRequest, returning the reports in transport-neutral +// form. It uses the daemon when dc is non-nil, otherwise the direct CASE +// session. +func treeReadClusterWildcard(ctx context.Context, dc *daemonNodeConn, client *interaction.Client, session *protocol.Session, ep uint16, clID uint32) ([]attrReport, error) { + if dc != nil { + dresp, err := dc.Read(daemon.AttrPathReq{ + Endpoint: ep, + ClusterID: clID, + WildcardAttribute: true, + }) + if err != nil { + return nil, err + } + return daemonAttrReports(dresp.Reports), nil } - if len(raw) == 0 { - return "", nil + + path := interaction.NewWildcardAttributePath(ep, clID) + reports, err := client.Read(ctx, session, path) + if err != nil { + return nil, err } - return decodeTLVValue(raw), nil + return directAttrReports(reports), nil } // treeResolveAttrName looks up the display name for an attribute ID within a