From 86f9028c43d25103f4bd95bf1750367330eb3c9f Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 11 Feb 2025 13:41:42 -0500 Subject: [PATCH 1/9] Start adding complete metadata, before updating tests --- src/app/GlobalAttributes.cpp | 8 ------ .../ProviderMetadataTree.h | 10 ++++---- .../codegen/CodegenDataModelProvider.cpp | 25 +++++++++++++++++-- 3 files changed, 28 insertions(+), 15 deletions(-) diff --git a/src/app/GlobalAttributes.cpp b/src/app/GlobalAttributes.cpp index a65f79b22bd870..ac6c00ab447d58 100644 --- a/src/app/GlobalAttributes.cpp +++ b/src/app/GlobalAttributes.cpp @@ -97,14 +97,6 @@ DataModel::ActionReturnStatus ReadGlobalAttributeFromMetadata(DataModel::Provide // and this reduces template variants for Encode, saving flash. ReturnErrorOnFailure(listEncodeHelper.Encode(static_cast(entry.attributeId))); } - - for (auto id : GlobalAttributesNotInMetadata) - { - // NOTE: cast to u64 because TLV encodes all numbers the same (no TLV sideffects) - // and this reduces template variants for Encode, saving flash. - ReturnErrorOnFailure(listEncodeHelper.Encode(static_cast(id))); - } - return CHIP_NO_ERROR; }); } diff --git a/src/app/data-model-provider/ProviderMetadataTree.h b/src/app/data-model-provider/ProviderMetadataTree.h index 3b0820e6efb73e..7d1ef04824bb95 100644 --- a/src/app/data-model-provider/ProviderMetadataTree.h +++ b/src/app/data-model-provider/ProviderMetadataTree.h @@ -60,14 +60,14 @@ class ProviderMetadataTree virtual CHIP_ERROR ClientClusters(EndpointId endpointId, ListBuilder & builder) = 0; virtual CHIP_ERROR ServerClusters(EndpointId endpointId, ListBuilder & builder) = 0; - /// Attribute lists contain all attributes EXCEPT the list attributes that - /// are part of metadata. The output from this method MUST NOT contain: - /// - AttributeList::Id + /// Attribute lists contain all attributes. This MUST include all global + /// attributes (See SPEC 7.13 Global Elements / Global Attributes Table). + /// In particular this MUST contain: /// - AcceptedCommandList::Id - /// - GeneratedCommandList::Id - /// However it MUST ALWAYS contain: + /// - AttributeList::Id /// - ClusterRevision::Id /// - FeatureMap::Id + /// - GeneratedCommandList::Id virtual CHIP_ERROR Attributes(const ConcreteClusterPath & path, ListBuilder & builder) = 0; virtual CHIP_ERROR GeneratedCommands(const ConcreteClusterPath & path, ListBuilder & builder) = 0; virtual CHIP_ERROR AcceptedCommands(const ConcreteClusterPath & path, ListBuilder & builder) = 0; diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp index b891346d170432..99281b01f8fb00 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp @@ -14,6 +14,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +#include "access/Privilege.h" #include #include @@ -24,6 +25,7 @@ #include #include #include +#include #include #include #include @@ -267,8 +269,15 @@ CHIP_ERROR CodegenDataModelProvider::Attributes(const ConcreteClusterPath & path VerifyOrReturnValue(cluster->attributeCount > 0, CHIP_NO_ERROR); VerifyOrReturnValue(cluster->attributes != nullptr, CHIP_NO_ERROR); - // TODO: if ember would encode data in AttributeEntry form, we could reference things directly - ReturnErrorOnFailure(builder.EnsureAppendCapacity(cluster->attributeCount)); + // TODO: if ember would encode data in AttributeEntry form, we could reference things directly (shorter code, + // although still allocation overhead due to global attributes not in metadata) + // + // We have Attributes from ember + global attributes that are NOT in ember metadata. + // We have to report them all + constexpr size_t kGlobalAttributeNotInMetadataCount = + sizeof(GlobalAttributesNotInMetadata) / sizeof(GlobalAttributesNotInMetadata[0]); + + ReturnErrorOnFailure(builder.EnsureAppendCapacity(cluster->attributeCount + kGlobalAttributeNotInMetadataCount)); Span attributeSpan(cluster->attributes, cluster->attributeCount); @@ -277,6 +286,18 @@ CHIP_ERROR CodegenDataModelProvider::Attributes(const ConcreteClusterPath & path ReturnErrorOnFailure(builder.Append(AttributeEntryFrom(path, attribute))); } + DataModel::AttributeEntry globalListEntry; + + globalListEntry.readPrivilege = Access::Privilege::kView; + globalListEntry.flags.Set(DataModel::AttributeQualityFlags::kListAttribute); + // these entries should also be `Fixed` however no such flag is propagated (we do not track) + + for (auto & attribute : GlobalAttributesNotInMetadata) + { + globalListEntry.attributeId = attribute; + ReturnErrorOnFailure(builder.Append(globalListEntry)); + } + return CHIP_NO_ERROR; } From a3de73ed7b8b5174974668b1f8717607acb8f91f Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 11 Feb 2025 13:43:53 -0500 Subject: [PATCH 2/9] Restyle --- src/app/data-model-provider/ProviderMetadataTree.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/data-model-provider/ProviderMetadataTree.h b/src/app/data-model-provider/ProviderMetadataTree.h index 7d1ef04824bb95..8e7763dc489020 100644 --- a/src/app/data-model-provider/ProviderMetadataTree.h +++ b/src/app/data-model-provider/ProviderMetadataTree.h @@ -61,7 +61,7 @@ class ProviderMetadataTree virtual CHIP_ERROR ServerClusters(EndpointId endpointId, ListBuilder & builder) = 0; /// Attribute lists contain all attributes. This MUST include all global - /// attributes (See SPEC 7.13 Global Elements / Global Attributes Table). + /// attributes (See SPEC 7.13 Global Elements / Global Attributes Table). /// In particular this MUST contain: /// - AcceptedCommandList::Id /// - AttributeList::Id From b97b040c515b0e69dc68d89a0cf5e99b5ae5c289 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 11 Feb 2025 13:47:55 -0500 Subject: [PATCH 3/9] Fix codegen data model provider unit tests --- .../tests/TestCodegenModelViaMocks.cpp | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp index 2505fdd87d0edf..1e33459fc5bd4a 100644 --- a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp +++ b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp @@ -1073,7 +1073,7 @@ TEST_F(TestCodegenModelViaMocks, IterateOverAttributes) EXPECT_EQ(model.Attributes(ConcreteClusterPath(kMockEndpoint2, MockClusterId(2)), builder), CHIP_NO_ERROR); auto attributes = builder.TakeBuffer(); - ASSERT_EQ(attributes.size(), 4u); + ASSERT_EQ(attributes.size(), 7u); ASSERT_EQ(attributes[0].attributeId, ClusterRevision::Id); ASSERT_FALSE(attributes[0].flags.Has(AttributeQualityFlags::kListAttribute)); @@ -1086,6 +1086,16 @@ TEST_F(TestCodegenModelViaMocks, IterateOverAttributes) ASSERT_EQ(attributes[3].attributeId, MockAttributeId(2)); ASSERT_TRUE(attributes[3].flags.Has(AttributeQualityFlags::kListAttribute)); + + // Ends with global list attributes + ASSERT_EQ(attributes[4].attributeId, GeneratedCommandList::Id); + ASSERT_TRUE(attributes[4].flags.Has(AttributeQualityFlags::kListAttribute)); + + ASSERT_EQ(attributes[5].attributeId, AcceptedCommandList::Id); + ASSERT_TRUE(attributes[5].flags.Has(AttributeQualityFlags::kListAttribute)); + + ASSERT_EQ(attributes[6].attributeId, AttributeList::Id); + ASSERT_TRUE(attributes[6].flags.Has(AttributeQualityFlags::kListAttribute)); } TEST_F(TestCodegenModelViaMocks, FindAttribute) @@ -1127,7 +1137,7 @@ TEST_F(TestCodegenModelViaMocks, FindAttribute) EXPECT_FALSE(info->writePrivilege.has_value()); // NOLINT(bugprone-unchecked-optional-access) } -// global attributes are EXPLICITLY not supported +// global attributes are EXPLICITLY supported TEST_F(TestCodegenModelViaMocks, GlobalAttributeInfo) { UseMockNodeConfig config(gTestNodeConfig); @@ -1138,10 +1148,13 @@ TEST_F(TestCodegenModelViaMocks, GlobalAttributeInfo) std::optional info = finder.Find( ConcreteAttributePath(kMockEndpoint1, MockClusterId(1), Clusters::Globals::Attributes::GeneratedCommandList::Id)); - ASSERT_FALSE(info.has_value()); + ASSERT_TRUE(info.has_value()); info = finder.Find(ConcreteAttributePath(kMockEndpoint1, MockClusterId(1), Clusters::Globals::Attributes::AttributeList::Id)); - ASSERT_FALSE(info.has_value()); + ASSERT_TRUE(info.has_value()); + + info = finder.Find(ConcreteAttributePath(kMockEndpoint1, MockClusterId(1), Clusters::Globals::Attributes::AcceptedCommandList::Id)); + ASSERT_TRUE(info.has_value()); } TEST_F(TestCodegenModelViaMocks, IterateOverAcceptedCommands) From 9a8af9b958a7d0f6f02aa95ac66dd2bc8846bb0a Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 11 Feb 2025 13:50:41 -0500 Subject: [PATCH 4/9] Fix includes --- src/data-model-providers/codegen/CodegenDataModelProvider.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp index 99281b01f8fb00..4a3aaa09d9f164 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp @@ -14,10 +14,10 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -#include "access/Privilege.h" #include #include +#include #include #include #include From 450c980b7710d255901911afc07fa7f33c4eba72 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Tue, 11 Feb 2025 18:51:45 +0000 Subject: [PATCH 5/9] Restyled by clang-format --- .../codegen/tests/TestCodegenModelViaMocks.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp index 1e33459fc5bd4a..a0b0dcd7ba1a15 100644 --- a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp +++ b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp @@ -1153,7 +1153,8 @@ TEST_F(TestCodegenModelViaMocks, GlobalAttributeInfo) info = finder.Find(ConcreteAttributePath(kMockEndpoint1, MockClusterId(1), Clusters::Globals::Attributes::AttributeList::Id)); ASSERT_TRUE(info.has_value()); - info = finder.Find(ConcreteAttributePath(kMockEndpoint1, MockClusterId(1), Clusters::Globals::Attributes::AcceptedCommandList::Id)); + info = finder.Find( + ConcreteAttributePath(kMockEndpoint1, MockClusterId(1), Clusters::Globals::Attributes::AcceptedCommandList::Id)); ASSERT_TRUE(info.has_value()); } From 48b83d25cedcea86d57a7b079d3cc066da9cd887 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 11 Feb 2025 23:59:33 -0500 Subject: [PATCH 6/9] Update src/data-model-providers/codegen/CodegenDataModelProvider.cpp Co-authored-by: Boris Zbarsky --- src/data-model-providers/codegen/CodegenDataModelProvider.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp index 4a3aaa09d9f164..f8dc0c22b750b4 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp @@ -275,7 +275,7 @@ CHIP_ERROR CodegenDataModelProvider::Attributes(const ConcreteClusterPath & path // We have Attributes from ember + global attributes that are NOT in ember metadata. // We have to report them all constexpr size_t kGlobalAttributeNotInMetadataCount = - sizeof(GlobalAttributesNotInMetadata) / sizeof(GlobalAttributesNotInMetadata[0]); + ArraySize(GlobalAttributesNotInMetadata); ReturnErrorOnFailure(builder.EnsureAppendCapacity(cluster->attributeCount + kGlobalAttributeNotInMetadataCount)); From f60a021d3453f071f2e267732ebd582f209fa15c Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 12 Feb 2025 08:45:10 -0500 Subject: [PATCH 7/9] Restyle --- src/data-model-providers/codegen/CodegenDataModelProvider.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp index f8dc0c22b750b4..028851ac154fd3 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp @@ -274,8 +274,7 @@ CHIP_ERROR CodegenDataModelProvider::Attributes(const ConcreteClusterPath & path // // We have Attributes from ember + global attributes that are NOT in ember metadata. // We have to report them all - constexpr size_t kGlobalAttributeNotInMetadataCount = - ArraySize(GlobalAttributesNotInMetadata); + constexpr size_t kGlobalAttributeNotInMetadataCount = ArraySize(GlobalAttributesNotInMetadata); ReturnErrorOnFailure(builder.EnsureAppendCapacity(cluster->attributeCount + kGlobalAttributeNotInMetadataCount)); From 033863b5e6df7d4e51b34f215de4041efabc0be4 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 12 Feb 2025 08:48:32 -0500 Subject: [PATCH 8/9] Add/update comment about global items we add --- .../codegen/CodegenDataModelProvider.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp index 028851ac154fd3..e12d0c5a83fe00 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp @@ -285,11 +285,18 @@ CHIP_ERROR CodegenDataModelProvider::Attributes(const ConcreteClusterPath & path ReturnErrorOnFailure(builder.Append(AttributeEntryFrom(path, attribute))); } + + // This "GlobalListEntry" is specific for metadata that ember does not include + // in its attribute list metadata. + // + // By spec these Attribute/AcceptedCommands/GeneratedCommants lists are: + // - lists of elements + // - read-only, with read privilege view + // - fixed value (no such flag exists, so this is not a quality flag we set/track) DataModel::AttributeEntry globalListEntry; globalListEntry.readPrivilege = Access::Privilege::kView; globalListEntry.flags.Set(DataModel::AttributeQualityFlags::kListAttribute); - // these entries should also be `Fixed` however no such flag is propagated (we do not track) for (auto & attribute : GlobalAttributesNotInMetadata) { From 9059a344a9d7e562241d697f0142c7464bebc1ce Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 12 Feb 2025 08:50:40 -0500 Subject: [PATCH 9/9] Restyle --- src/data-model-providers/codegen/CodegenDataModelProvider.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp index e12d0c5a83fe00..7fd6d241704b0e 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp @@ -285,7 +285,6 @@ CHIP_ERROR CodegenDataModelProvider::Attributes(const ConcreteClusterPath & path ReturnErrorOnFailure(builder.Append(AttributeEntryFrom(path, attribute))); } - // This "GlobalListEntry" is specific for metadata that ember does not include // in its attribute list metadata. //