Make DataModel::Provider return a Complete attribute metadata as its public interface (#37519)

* Start adding complete metadata, before updating tests

* Restyle

* Fix codegen data model provider unit tests

* Fix includes

* Restyled by clang-format

* Update src/data-model-providers/codegen/CodegenDataModelProvider.cpp

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

* Restyle

* Add/update comment about global items we add

* Restyle

---------

Co-authored-by: Andrei Litvin <andreilitvin@google.com>
Co-authored-by: Restyled.io <commits@restyled.io>
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

Author: 4 people

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<uint64_t>(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<uint64_t>(id)));
-            }
-
             return CHIP_NO_ERROR;
         });
     }

src/app/data-model-provider/ProviderMetadataTree.h

@@ -60,14 +60,14 @@ class ProviderMetadataTree
     virtual CHIP_ERROR ClientClusters(EndpointId endpointId, ListBuilder<ClusterId> & builder)          = 0;
     virtual CHIP_ERROR ServerClusters(EndpointId endpointId, ListBuilder<ServerClusterEntry> & 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<AttributeEntry> & builder)             = 0;
     virtual CHIP_ERROR GeneratedCommands(const ConcreteClusterPath & path, ListBuilder<CommandId> & builder)           = 0;
     virtual CHIP_ERROR AcceptedCommands(const ConcreteClusterPath & path, ListBuilder<AcceptedCommandEntry> & builder) = 0;

src/data-model-providers/codegen/CodegenDataModelProvider.cpp

@@ -17,13 +17,15 @@
 #include <data-model-providers/codegen/CodegenDataModelProvider.h>
 
 #include <access/AccessControl.h>
+#include <access/Privilege.h>
 #include <app-common/zap-generated/attribute-type.h>
 #include <app/CommandHandlerInterface.h>
 #include <app/CommandHandlerInterfaceRegistry.h>
 #include <app/ConcreteAttributePath.h>
 #include <app/ConcreteClusterPath.h>
 #include <app/ConcreteCommandPath.h>
 #include <app/EventPathParams.h>
+#include <app/GlobalAttributes.h>
 #include <app/RequiredPrivilege.h>
 #include <app/data-model-provider/MetadataList.h>
 #include <app/data-model-provider/MetadataTypes.h>
@@ -267,8 +269,14 @@ 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 = ArraySize(GlobalAttributesNotInMetadata);
+
+    ReturnErrorOnFailure(builder.EnsureAppendCapacity(cluster->attributeCount + kGlobalAttributeNotInMetadataCount));
 
     Span<const EmberAfAttributeMetadata> attributeSpan(cluster->attributes, cluster->attributeCount);
 
@@ -277,6 +285,24 @@ 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);
+
+    for (auto & attribute : GlobalAttributesNotInMetadata)
+    {
+        globalListEntry.attributeId = attribute;
+        ReturnErrorOnFailure(builder.Append(globalListEntry));
+    }
+
     return CHIP_NO_ERROR;
 }
 

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,14 @@ TEST_F(TestCodegenModelViaMocks, GlobalAttributeInfo)
     std::optional<AttributeEntry> 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)