Address review comments on PR 13172. (#13600)

* Address review comments on PR 13172.

Changes:

* Remove length anotations on some lists that are always handled via
  AttributeAccessInterface so should not count toward
  ATTRIBUTE_LARGEST.  Reduces ATTRIBUTE_LARGEST from 401 to 255/259
  for most apps.

* Fix some .zap files that did not have those lists marked as External
  storage.

* Add a way for applications to static_assert via a config variable
  that ATTRIBUTE_LARGEST is not larger than expected.

* Stop using %zu for logging in Server.h

* Add checks + logging for a missing attribute persistence provider in
  attribute-storage.

* Address review comments.

examples/log-source-app/log-source-common/log-source-app.zap

@@ -1580,7 +1580,7 @@
               "mfgCode": null,
               "side": "server",
               "included": 1,
-              "storageOption": "RAM",
+              "storageOption": "External",
               "singleton": 0,
               "bounded": 0,
               "defaultValue": "",
@@ -1625,7 +1625,7 @@
               "mfgCode": null,
               "side": "server",
               "included": 1,
-              "storageOption": "RAM",
+              "storageOption": "External",
               "singleton": 0,
               "bounded": 0,
               "defaultValue": "",

examples/placeholder/linux/apps/app1/config.zap

@@ -1040,7 +1040,7 @@
               "mfgCode": null,
               "side": "server",
               "included": 1,
-              "storageOption": "RAM",
+              "storageOption": "External",
               "singleton": 0,
               "bounded": 0,
               "defaultValue": "",

examples/placeholder/linux/apps/app2/config.zap

@@ -1040,7 +1040,7 @@
               "mfgCode": null,
               "side": "server",
               "included": 1,
-              "storageOption": "RAM",
+              "storageOption": "External",
               "singleton": 0,
               "bounded": 0,
               "defaultValue": "",

src/app/server/Server.h

@@ -104,7 +104,7 @@ class Server
             ReturnErrorOnFailure(DeviceLayer::PersistedStorage::KeyValueStoreMgr().Get(key, buffer, size, &bytesRead));
             if (!CanCastTo<uint16_t>(bytesRead))
             {
-                ChipLogDetail(AppServer, "%zu is too big to fit in uint16_t", bytesRead);
+                ChipLogDetail(AppServer, "0x%" PRIx32 " is too big to fit in uint16_t", static_cast<uint32_t>(bytesRead));
                 return CHIP_ERROR_BUFFER_TOO_SMALL;
             }
             ChipLogProgress(AppServer, "Retrieved from server storage: %s", key);

src/app/util/attribute-storage.cpp

@@ -1170,6 +1170,8 @@ void emAfLoadAttributeDefaults(EndpointId endpoint, bool ignoreStorage, Optional
     uint16_t epCount = emberAfEndpointCount();
     uint8_t attrData[ATTRIBUTE_LARGEST];
     auto * attrStorage = ignoreStorage ? nullptr : app::GetAttributePersistenceProvider();
+    // Don't check whether we actually have an attrStorage here, because it's OK
+    // to have one if none of our attributes have NVM storage.
 
     for (ep = 0; ep < epCount; ep++)
     {
@@ -1211,6 +1213,7 @@ void emAfLoadAttributeDefaults(EndpointId endpoint, bool ignoreStorage, Optional
                 // First check for a persisted value.
                 if (!ignoreStorage && am->IsNonVolatile())
                 {
+                    VerifyOrDie(attrStorage && "Attribute persistence needs a persistence provider");
                     MutableByteSpan bytes(attrData);
                     CHIP_ERROR err = attrStorage->ReadValue(
                         app::ConcreteAttributePath(de->endpoint, cluster->clusterId, am->attributeId), am, bytes);
@@ -1331,8 +1334,15 @@ void emAfSaveAttributeToStorageIfNeeded(uint8_t * data, EndpointId endpoint, Clu
     }
 
     auto * attrStorage = app::GetAttributePersistenceProvider();
-    attrStorage->WriteValue(app::ConcreteAttributePath(endpoint, clusterId, metadata->attributeId), metadata,
-                            ByteSpan(data, dataSize));
+    if (attrStorage)
+    {
+        attrStorage->WriteValue(app::ConcreteAttributePath(endpoint, clusterId, metadata->attributeId), metadata,
+                                ByteSpan(data, dataSize));
+    }
+    else
+    {
+        ChipLogProgress(DataManagement, "Can't store attribute value: no persistence provider");
+    }
 }
 
 // This function returns the actual function point from the array,

src/app/zap-templates/templates/app/endpoint_config.zapt

@@ -3,8 +3,9 @@
 // Prevent multiple inclusion
 #pragma once
 
-{{#endpoint_config}}
+#include <lib/core/CHIPConfig.h>
 
+{{#endpoint_config}}
 
 // Default values for the attributes longer than a pointer,
 // in a form of a binary blob
@@ -73,6 +74,9 @@
 // Largest attribute size is needed for various buffers
 #define ATTRIBUTE_LARGEST ({{endpoint_largest_attribute_size}})
 
+static_assert(ATTRIBUTE_LARGEST <= CHIP_CONFIG_MAX_ATTRIBUTE_STORE_ELEMENT_SIZE,
+              "ATTRIBUTE_LARGEST larger than expected");
+
 // Total size of singleton attributes
 #define ATTRIBUTE_SINGLETONS_SIZE ({{endpoint_singletons_size}})
 

src/app/zap-templates/zcl/data-model/chip/descriptor-cluster.xml

@@ -29,9 +29,9 @@ limitations under the License.
     <code>0x001d</code>
     <define>DESCRIPTOR_CLUSTER</define>
     <description>The Descriptor Cluster is meant to replace the support from the Zigbee Device Object (ZDO) for describing a node, its endpoints and clusters.</description>
-    <attribute side="server" code="0x0000" define="DEVICE_LIST" type="ARRAY" entryType="DeviceType" length="254" writable="false" optional="false">device list</attribute>
-    <attribute side="server" code="0x0001" define="SERVER_LIST" type="ARRAY" entryType="CLUSTER_ID" length="254" writable="false" optional="false">server list</attribute>
-    <attribute side="server" code="0x0002" define="CLIENT_LIST" type="ARRAY" entryType="CLUSTER_ID" length="254" writable="false" optional="false">client list</attribute>
-    <attribute side="server" code="0x0003" define="PARTS_LIST" type="ARRAY" entryType="ENDPOINT_NO" length="254" writable="false" optional="false">parts list</attribute>
+    <attribute side="server" code="0x0000" define="DEVICE_LIST" type="ARRAY" entryType="DeviceType" writable="false" optional="false">device list</attribute>
+    <attribute side="server" code="0x0001" define="SERVER_LIST" type="ARRAY" entryType="CLUSTER_ID" writable="false" optional="false">server list</attribute>
+    <attribute side="server" code="0x0002" define="CLIENT_LIST" type="ARRAY" entryType="CLUSTER_ID" writable="false" optional="false">client list</attribute>
+    <attribute side="server" code="0x0003" define="PARTS_LIST" type="ARRAY" entryType="ENDPOINT_NO" writable="false" optional="false">parts list</attribute>
   </cluster>
 </configurator>

src/app/zap-templates/zcl/data-model/chip/operational-credentials-cluster.xml

@@ -56,11 +56,10 @@ limitations under the License.
     <description>This cluster is used to add or remove Operational Credentials on a Commissionee or Node, as well as manage the associated Fabrics.</description>
 
     <attribute side="server" code="0x0000" define="NOCS" type="ARRAY" entryType="NOCStruct" writable="false" optional="false">NOCs</attribute>
-    <attribute side="server" code="0x0001" define="FABRICS" type="ARRAY" entryType="FabricDescriptor" length="320" writable="false" optional="false">fabrics list</attribute>
+    <attribute side="server" code="0x0001" define="FABRICS" type="ARRAY" entryType="FabricDescriptor" writable="false" optional="false">fabrics list</attribute>
     <attribute side="server" code="0x0002" define="SUPPORTED_FABRICS" type="INT8U" writable="false" optional="false">SupportedFabrics</attribute>
     <attribute side="server" code="0x0003" define="COMMISSIONED_FABRICS" type="INT8U" writable="false" optional="false">CommissionedFabrics</attribute>
-    <!-- 400 = 400 bytes for root cert -->
-    <attribute side="server" code="0x0004" define="TRUSTED_ROOTS" type="ARRAY" entryType="OCTET_STRING" length="400" writable="false" optional="false">TrustedRootCertificates</attribute>
+    <attribute side="server" code="0x0004" define="TRUSTED_ROOTS" type="ARRAY" entryType="OCTET_STRING" writable="false" optional="false">TrustedRootCertificates</attribute>
     <attribute side="server" code="0x0005" define="CURRENT_FABRIC_INDEX" type="fabric_idx" writable="false" optional="false">CurrentFabricIndex</attribute>
 
     <command source="client" code="0x00" name="AttestationRequest" response="AttestationResponse" optional="false">

src/lib/core/CHIPConfig.h

@@ -1752,6 +1752,23 @@ extern const char CHIP_NON_PRODUCTION_MARKER[];
 #ifndef CHIP_RESUBSCRIBE_WAIT_TIME_MULTIPLIER_MS
 #define CHIP_RESUBSCRIBE_WAIT_TIME_MULTIPLIER_MS 10000
 #endif
+
+/*
+ * @def CHIP_CONFIG_MAX_ATTRIBUTE_STORE_ELEMENT_SIZE
+ *
+ * @brief Safety limit to ensure that we don't end up with a
+ * larger-than-expected buffer for temporary attribute storage (on the stack or
+ * in .bss).  The SDK will fail to compile if this value is set below the value
+ * it thinks it needs for a buffer size that can store any simple (not list or
+ * struct) attribute value.
+ */
+#ifndef CHIP_CONFIG_MAX_ATTRIBUTE_STORE_ELEMENT_SIZE
+// I can't figure out how to get all-clusters-app to sanely use a different
+// value here, and that app includes TestCluster, which has very large string
+// attributes (1000 octets, leading to a 1003 octet buffer).
+#define CHIP_CONFIG_MAX_ATTRIBUTE_STORE_ELEMENT_SIZE 1003
+#endif // CHIP_CONFIG_MAX_ATTRIBUTE_STORE_ELEMENT_SIZE
+
 /**
  * @}
  */