Implement persistent storage for attributes with "NVM" storage select…

…ed. (project-chip#13172)

General changes:

1. Set the CurrentLevel attribute in all-clusters app to be NVM and
   default to 254.  This requires some YAML changes to expect the new
   value.
2. Remove the hardcoded setting of that attribute to 255 during boot
   in the ESP32 all-clusters app.
3. Define and implement AttributePersistenceProvider to read/write persistent
   attributes.
4. Fix bug in ServerStorageDelegate::SyncGetKeyValue: it was not setting the
   size outparam to the actual size read.
5. Move EmberAfAttributeMetadata and a few functions for working with
   strings into a new header that can be included in places where
   generated headers (which af.h and af-types.h include) can't be.
6. Add those functions to out mock attribute-storage to fix linking of
   some tests.  Fix mistyped "darwin" in gn files so the tests get run
   on Darwin too, not just on Linux.
7. Rearrange the logic in emAfLoadAttributeDefaults a bit: instead of
   loading the default value into the RAM store and then checking for
   a persisted value, check for a persisted value first (if the
   attribute might have one), and only look for a default value if
   there is no persisted value. This removes the need for a separate
   emAfLoadAttributesFromStorage pass (which made sense back when that
   code was all generated, but it's not anymore).
8. Fix emAfSaveAttributeToStorageIfNeeded to actually store the value.

examples/all-clusters-app/all-clusters-common/all-clusters-app.zap

@@ -8035,10 +8035,10 @@
               "mfgCode": null,
               "side": "server",
               "included": 1,
-              "storageOption": "RAM",
+              "storageOption": "NVM",
               "singleton": 0,
               "bounded": 0,
-              "defaultValue": "0x00",
+              "defaultValue": "0xFE",
               "reportable": 1,
               "minInterval": 0,
               "maxInterval": 65344,

examples/all-clusters-app/esp32/main/main.cpp

@@ -428,14 +428,6 @@ class CustomScreen : public Screen
 
 #endif // CONFIG_DEVICE_TYPE_M5STACK
 
-void SetupInitialLevelControlValues(chip::EndpointId endpointId)
-{
-    uint8_t level = UINT8_MAX;
-
-    emberAfWriteAttribute(endpointId, ZCL_LEVEL_CONTROL_CLUSTER_ID, ZCL_CURRENT_LEVEL_ATTRIBUTE_ID, CLUSTER_MASK_SERVER, &level,
-                          ZCL_INT8U_ATTRIBUTE_TYPE);
-}
-
 void SetupPretendDevices()
 {
     AddDevice("Watch");
@@ -523,8 +515,6 @@ static void InitServer(intptr_t context)
     SetDeviceAttestationCredentialsProvider(Examples::GetExampleDACProvider());
 
     SetupPretendDevices();
-    SetupInitialLevelControlValues(/* endpointId = */ 1);
-    SetupInitialLevelControlValues(/* endpointId = */ 2);
 }
 
 extern "C" void app_main()

src/app/AttributePersistenceProvider.h

@@ -0,0 +1,95 @@
+/*
+ *    Copyright (c) 2021 Project CHIP Authors
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+#pragma once
+
+#include <app/ConcreteAttributePath.h>
+#include <app/util/attribute-metadata.h>
+#include <lib/support/Span.h>
+
+namespace chip {
+namespace app {
+
+/**
+ * Interface for persisting attribute values.
+ */
+
+class AttributePersistenceProvider
+{
+public:
+    virtual ~AttributePersistenceProvider() = default;
+    AttributePersistenceProvider()          = default;
+
+    /**
+     * Write an attribute value from the attribute store (i.e. not a struct or
+     * list) to non-volatile memory.
+     *
+     * @param [in] aPath the attribute path for the data being written.
+     * @param [in] aMetadata the attribute metadata, as a convenience.
+     * @param [in] aValue the data to write.  Integers and floats are
+     *             represented in native endianness.  Strings are represented
+     *             as Pascal-style strings, as in ZCL, with a length prefix
+     *             whose size depends on the actual string type.  The length is
+     *             stored as little-endian.
+     *
+     *             Integer and float values have a size that matches the `size`
+     *             member of aMetadata.
+     *
+     *             String values have a size that corresponds to the actual size
+     *             of the data in the string (including the length prefix),
+     *             which is no larger than the `size` member of aMetadata.
+     */
+    virtual CHIP_ERROR WriteValue(const ConcreteAttributePath & aPath, const EmberAfAttributeMetadata * aMetadata,
+                                  const ByteSpan & aValue) = 0;
+
+    /**
+     * Read an attribute value from non-volatile memory.
+     *
+     * @param [in]     aPath the attribute path for the data being persisted.
+     * @param [in]     aMetadata the attribute metadata, as a convenience.
+     * @param [in,out] aValue where to place the data.  The size of the buffer
+     *                 will be equal to `size` member of aMetadata.
+     *
+     *                 The data is expected to be in native endianness for
+     *                 integers and floats.  For strings, see the string
+     *                 representation description in the WriteValue
+     *                 documentation.
+     */
+    virtual CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, const EmberAfAttributeMetadata * aMetadata,
+                                 MutableByteSpan & aValue) = 0;
+};
+
+/**
+ * Instance getter for the global AttributePersistenceProvider.
+ *
+ * Callers have to externally synchronize usage of this function.
+ *
+ * @return The global AttributePersistenceProvider.  This must never be null.
+ */
+AttributePersistenceProvider * GetAttributePersistenceProvider();
+
+/**
+ * Instance setter for the global AttributePersistenceProvider.
+ *
+ * Callers have to externally synchronize usage of this function.
+ *
+ * If the `provider` is nullptr, the value is not changed.
+ *
+ * @param[in] provider the AttributePersistenceProvider implementation to use.
+ */
+void SetAttributePersistenceProvider(AttributePersistenceProvider * aProvider);
+
+} // namespace app
+} // namespace chip

src/app/BUILD.gn

@@ -41,6 +41,7 @@ static_library("app") {
     "AttributePathExpandIterator.cpp",
     "AttributePathExpandIterator.h",
     "AttributePathParams.h",
+    "AttributePersistenceProvider.h",
     "BufferedReadCallback.cpp",
     "CASEClient.cpp",
     "CASEClient.h",
@@ -49,6 +50,7 @@ static_library("app") {
     "CASESessionManager.h",
     "CommandHandler.cpp",
     "CommandSender.cpp",
+    "DefaultAttributePersistenceProvider.cpp",
     "DeviceProxy.cpp",
     "DeviceProxy.h",
     "EventManagement.cpp",

src/app/DefaultAttributePersistenceProvider.cpp

@@ -0,0 +1,89 @@
+/*
+ *    Copyright (c) 2021 Project CHIP Authors
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+#include <app/DefaultAttributePersistenceProvider.h>
+#include <lib/support/CodeUtils.h>
+#include <lib/support/DefaultStorageKeyAllocator.h>
+#include <lib/support/SafeInt.h>
+
+namespace chip {
+namespace app {
+
+CHIP_ERROR DefaultAttributePersistenceProvider::WriteValue(const ConcreteAttributePath & aPath,
+                                                           const EmberAfAttributeMetadata * aMetadata, const ByteSpan & aValue)
+{
+    // TODO: we may want to have a small cache for values that change a lot, so
+    // we only write them once a bunch of changes happen or on timer or
+    // shutdown.
+    DefaultStorageKeyAllocator key;
+    if (!CanCastTo<uint16_t>(aValue.size()))
+    {
+        return CHIP_ERROR_BUFFER_TOO_SMALL;
+    }
+    return mStorage.SyncSetKeyValue(key.AttributeValue(aPath), aValue.data(), static_cast<uint16_t>(aValue.size()));
+}
+
+CHIP_ERROR DefaultAttributePersistenceProvider::ReadValue(const ConcreteAttributePath & aPath,
+                                                          const EmberAfAttributeMetadata * aMetadata, MutableByteSpan & aValue)
+{
+    DefaultStorageKeyAllocator key;
+    uint16_t size = static_cast<uint16_t>(min(aValue.size(), static_cast<size_t>(UINT16_MAX)));
+    ReturnErrorOnFailure(mStorage.SyncGetKeyValue(key.AttributeValue(aPath), aValue.data(), size));
+    EmberAfAttributeType type = aMetadata->attributeType;
+    if (emberAfIsStringAttributeType(type))
+    {
+        // Ensure that we've read enough bytes that we are not ending up with
+        // un-initialized memory.  Should have read length + 1 (for the length
+        // byte).
+        VerifyOrReturnError(size >= emberAfStringLength(aValue.data()) + 1, CHIP_ERROR_INCORRECT_STATE);
+    }
+    else if (emberAfIsLongStringAttributeType(type))
+    {
+        // Ensure that we've read enough bytes that we are not ending up with
+        // un-initialized memory.  Should have read length + 2 (for the length
+        // bytes).
+        VerifyOrReturnError(size >= emberAfLongStringLength(aValue.data()) + 2, CHIP_ERROR_INCORRECT_STATE);
+    }
+    else
+    {
+        // Ensure we got the expected number of bytes for all other types.
+        VerifyOrReturnError(size == aMetadata->size, CHIP_ERROR_INCORRECT_STATE);
+    }
+    aValue.reduce_size(size);
+    return CHIP_NO_ERROR;
+}
+
+namespace {
+
+AttributePersistenceProvider * gAttributeSaver = nullptr;
+
+} // anonymous namespace
+
+AttributePersistenceProvider * GetAttributePersistenceProvider()
+{
+    return gAttributeSaver;
+}
+
+void SetAttributePersistenceProvider(AttributePersistenceProvider * aProvider)
+{
+    if (aProvider != nullptr)
+    {
+        gAttributeSaver = aProvider;
+    }
+}
+
+} // namespace app
+} // namespace chip

src/app/DefaultAttributePersistenceProvider.h

@@ -0,0 +1,50 @@
+/*
+ *    Copyright (c) 2021 Project CHIP Authors
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+#pragma once
+
+#include <app/AttributePersistenceProvider.h>
+#include <lib/core/CHIPPersistentStorageDelegate.h>
+#include <lib/support/DefaultStorageKeyAllocator.h>
+
+namespace chip {
+namespace app {
+
+/**
+ * Default implementation of AttributePersistenceProvider.  This uses
+ * PersistentStorageDelegate to store the attribute values.
+ *
+ * NOTE: SetAttributePersistenceProvider must still be called with an instance
+ * of this class, since it can't be constructed automatically without knowing
+ * what PersistentStorageDelegate is to be used.
+ */
+class DefaultAttributePersistenceProvider : public AttributePersistenceProvider
+{
+public:
+    // aStorage must outlive this object.
+    DefaultAttributePersistenceProvider(PersistentStorageDelegate & aStorage) : mStorage(aStorage) {}
+
+    // AttributePersistenceProvider implementation.
+    CHIP_ERROR WriteValue(const ConcreteAttributePath & aPath, const EmberAfAttributeMetadata * aMetadata,
+                          const ByteSpan & aValue) override;
+    CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, const EmberAfAttributeMetadata * aMetadata,
+                         MutableByteSpan & aValue) override;
+
+private:
+    PersistentStorageDelegate & mStorage;
+};
+
+} // namespace app
+} // namespace chip

src/app/server/Server.cpp

@@ -92,7 +92,8 @@ Server::Server() :
         .dnsCache          = nullptr,
         .devicePool        = &mDevicePool,
         .dnsResolver       = nullptr,
-    }), mCommissioningWindowManager(this), mGroupsProvider(mGroupsStorage)
+    }), mCommissioningWindowManager(this), mGroupsProvider(mGroupsStorage),
+    mAttributePersister(mServerStorage)
 {}
 
 CHIP_ERROR Server::Init(AppDelegate * delegate, uint16_t secureServicePort, uint16_t unsecureServicePort)
@@ -106,6 +107,11 @@ CHIP_ERROR Server::Init(AppDelegate * delegate, uint16_t secureServicePort, uint
 
     mCommissioningWindowManager.SetAppDelegate(delegate);
     mCommissioningWindowManager.SetSessionIDAllocator(&mSessionIDAllocator);
+
+    // Set up attribute persistence before we try to bring up the data model
+    // handler.
+    SetAttributePersistenceProvider(&mAttributePersister);
+
     InitDataModelHandler(&mExchangeMgr);
 
 #if CHIP_DEVICE_LAYER_TARGET_DARWIN

src/app/server/Server.h

@@ -19,13 +19,15 @@
 
 #include <app/CASEClientPool.h>
 #include <app/CASESessionManager.h>
+#include <app/DefaultAttributePersistenceProvider.h>
 #include <app/OperationalDeviceProxyPool.h>
 #include <app/server/AppDelegate.h>
 #include <app/server/CommissioningWindowManager.h>
 #include <credentials/FabricTable.h>
 #include <credentials/GroupDataProviderImpl.h>
 #include <inet/InetConfig.h>
 #include <lib/core/CHIPConfig.h>
+#include <lib/support/SafeInt.h>
 #include <lib/support/TestPersistentStorageDelegate.h>
 #include <messaging/ExchangeMgr.h>
 #include <platform/KeyValueStoreManager.h>
@@ -98,8 +100,15 @@ class Server
     {
         CHIP_ERROR SyncGetKeyValue(const char * key, void * buffer, uint16_t & size) override
         {
-            ReturnErrorOnFailure(DeviceLayer::PersistedStorage::KeyValueStoreMgr().Get(key, buffer, size));
+            size_t bytesRead;
+            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);
+                return CHIP_ERROR_BUFFER_TOO_SMALL;
+            }
             ChipLogProgress(AppServer, "Retrieved from server storage: %s", key);
+            size = static_cast<uint16_t>(bytesRead);
             return CHIP_NO_ERROR;
         }
 
@@ -159,6 +168,7 @@ class Server
     // (https://github.com/project-chip/connectedhomeip/issues/12174)
     TestPersistentStorageDelegate mGroupsStorage;
     Credentials::GroupDataProviderImpl mGroupsProvider;
+    app::DefaultAttributePersistenceProvider mAttributePersister;
 
     // TODO @ceille: Maybe use OperationalServicePort and CommissionableServicePort
     uint16_t mSecuredServicePort;

src/app/tests/BUILD.gn

@@ -83,7 +83,7 @@ chip_test_suite("tests") {
   ]
 
   if (chip_config_network_layer_ble &&
-      (chip_device_platform == "linux" || chip_device_platform == "Darwin")) {
+      (chip_device_platform == "linux" || chip_device_platform == "darwin")) {
     test_sources += [ "TestCommissionManager.cpp" ]
     public_deps += [ "${chip_root}/src/app/server" ]
   }

src/app/tests/suites/certification/Test_TC_LVL_2_1.yaml

@@ -27,7 +27,7 @@ tests:
       command: "readAttribute"
       attribute: "current Level"
       response:
-          value: 0
+          value: 254
 
     - label: "sends a Move to level command"
       command: "MoveToLevel"
@@ -116,12 +116,12 @@ tests:
       response:
           value: 254
 
-    - label: "Reset level to 0"
+    - label: "Reset level to 254"
       command: "MoveToLevel"
       arguments:
           values:
               - name: "level"
-                value: 0
+                value: 254
               - name: "transitionTime"
                 value: 0
               - name: "optionMask"