Refactor SafeAttributePersistenceProvider and AttributePersistenceProvider to move ember-specific bits to codegendatamodelprovider

examples/lighting-app/nrfconnect/main/AppTask.cpp

@@ -111,9 +111,8 @@ chip::DeviceLayer::DeviceInfoProviderImpl gExampleDeviceInfoProvider;
 // be written, so it must live so long as the DeferredAttributePersistenceProvider object.
 DeferredAttribute gCurrentLevelPersister(ConcreteAttributePath(kLightEndpointId, Clusters::LevelControl::Id,
                                                                Clusters::LevelControl::Attributes::CurrentLevel::Id));
-DeferredAttributePersistenceProvider gDeferredAttributePersister(Server::GetInstance().GetDefaultAttributePersister(),
-                                                                 Span<DeferredAttribute>(&gCurrentLevelPersister, 1),
-                                                                 System::Clock::Milliseconds32(5000));
+
+DeferredAttributePersistenceProvider gDeferredAttributePersister;
 
 #ifdef CONFIG_CHIP_CRYPTO_PSA
 chip::Crypto::PSAOperationalKeystore sPSAOperationalKeystore{};
@@ -278,7 +277,13 @@ CHIP_ERROR AppTask::Init()
 
     gExampleDeviceInfoProvider.SetStorageDelegate(&Server::GetInstance().GetPersistentStorage());
     chip::DeviceLayer::SetDeviceInfoProvider(&gExampleDeviceInfoProvider);
-    app::SetAttributePersistenceProvider(&gDeferredAttributePersister);
+
+    // Server will initialize a persistence provider
+    VerifyOrDie(GetAttributePersistenceProvider() != nullptr);
+    VerifyOrDie(gDeferredAttributePersister.Init(*GetAttributePersistenceProvider(),
+                                                 Span<DeferredAttribute>(&gCurrentLevelPersister, 1),
+                                                 System::Clock::Milliseconds32(5000)) == CHIP_NO_ERROR);
+    SetAttributePersistenceProvider(&gDeferredAttributePersister);
 
     ConfigurationMgr().LogDeviceConfig();
     PrintOnboardingCodes(chip::RendezvousInformationFlags(chip::RendezvousInformationFlag::kBLE));

examples/lighting-app/qpg/src/AppTask.cpp

@@ -41,6 +41,7 @@
 #include <app/server/Dnssd.h>
 #include <app/server/Server.h>
 #include <app/util/attribute-storage.h>
+#include <app/util/persistence/AttributePersistenceProvider.h>
 #include <lib/support/TypeTraits.h>
 
 #include <app/util/persistence/DeferredAttributePersistenceProvider.h>
@@ -113,9 +114,7 @@ DeferredAttribute gPersisters[] = {
 
 };
 
-DeferredAttributePersistenceProvider gDeferredAttributePersister(Server::GetInstance().GetDefaultAttributePersister(),
-                                                                 Span<DeferredAttribute>(gPersisters, 3),
-                                                                 System::Clock::Milliseconds32(5000));
+DeferredAttributePersistenceProvider gDeferredAttributePersister;
 
 /**********************************************************
  * Identify Callbacks
@@ -277,7 +276,11 @@ void AppTask::InitServer(intptr_t arg)
 
     chip::Server::GetInstance().Init(initParams);
 
-    app::SetAttributePersistenceProvider(&gDeferredAttributePersister);
+    // Server will initialize a persistence provider
+    VerifyOrDie(GetAttributePersistenceProvider() != nullptr);
+    VerifyOrDie(gDeferredAttributePersister.Init(*GetAttributePersistenceProvider(), Span<DeferredAttribute>(gPersisters, 3),
+                                                 System::Clock::Milliseconds32(5000)) == CHIP_NO_ERROR);
+    SetAttributePersistenceProvider(&gDeferredAttributePersister);
 
 #if CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY
     chip::app::DnssdServer::Instance().SetExtendedDiscoveryTimeoutSecs(extDiscTimeoutSecs);

src/BUILD.gn

@@ -100,10 +100,7 @@ if (chip_build_tests) {
     }
 
     if (chip_device_platform != "efr32") {
-      tests += [
-        "${chip_root}/src/app/tests",
-        "${chip_root}/src/app/util/persistence/tests",
-      ]
+      tests += [ "${chip_root}/src/app/tests" ]
 
       # Disabled for EFR32 because _open is not implemented.
       # https://github.com/project-chip/connectedhomeip/issues/35624

src/app/BUILD.gn

@@ -393,7 +393,10 @@ source_set("command-handler-impl") {
 }
 
 source_set("attribute-persistence") {
-  sources = [ "SafeAttributePersistenceProvider.h" ]
+  sources = [
+    "SafeAttributePersistenceProvider.cpp",
+    "SafeAttributePersistenceProvider.h",
+  ]
 
   public_deps = [
     ":paths",
@@ -441,6 +444,7 @@ static_library("app") {
   public_deps = [
     ":app_config",
     ":attribute-access",
+    ":attribute-persistence",
     ":constants",
     ":global-attributes",
     ":interaction-model",

src/app/InteractionModelEngine.cpp

@@ -246,6 +246,7 @@ void InteractionModelEngine::Shutdown()
 
     mpCASESessionMgr = nullptr;
 
+    SetDataModelProvider(nullptr);
     //
     // We _should_ be clearing these out, but doing so invites a world
     // of trouble. #21233 tracks fixing the underlying assumptions to make

src/app/SafeAttributePersistenceProvider.cpp

@@ -0,0 +1,81 @@
+/*
+ *    Copyright (c) 2024 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/SafeAttributePersistenceProvider.h>
+
+#include <lib/core/CHIPError.h>
+#include <lib/support/CodeUtils.h>
+#include <lib/support/SafeInt.h>
+
+namespace chip {
+namespace app {
+
+CHIP_ERROR SafeAttributePersistenceProvider::KeyWriteValue(const StorageKeyName & aKey, const ByteSpan & aValue)
+{
+    VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE);
+
+    // 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.
+    if (!CanCastTo<uint16_t>(aValue.size()))
+    {
+        return CHIP_ERROR_BUFFER_TOO_SMALL;
+    }
+    return mStorage->SyncSetKeyValue(aKey.KeyName(), aValue.data(), static_cast<uint16_t>(aValue.size()));
+}
+
+CHIP_ERROR SafeAttributePersistenceProvider::KeyReadValue(const StorageKeyName & aKey, MutableByteSpan & aValue)
+{
+    VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE);
+
+    uint16_t size = static_cast<uint16_t>(std::min(aValue.size(), static_cast<size_t>(UINT16_MAX)));
+    ReturnErrorOnFailure(mStorage->SyncGetKeyValue(aKey.KeyName(), aValue.data(), size));
+    aValue.reduce_size(size);
+    return CHIP_NO_ERROR;
+}
+
+CHIP_ERROR SafeAttributePersistenceProvider::SafeWriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue)
+{
+    return KeyWriteValue(DefaultStorageKeyAllocator::SafeAttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId),
+                         aValue);
+}
+
+CHIP_ERROR SafeAttributePersistenceProvider::SafeReadValue(const ConcreteAttributePath & aPath, MutableByteSpan & aValue)
+{
+    return KeyReadValue(DefaultStorageKeyAllocator::SafeAttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId),
+                        aValue);
+}
+
+namespace {
+
+SafeAttributePersistenceProvider * gSafeAttributeSaver = nullptr;
+
+} // anonymous namespace
+
+SafeAttributePersistenceProvider * GetSafeAttributePersistenceProvider()
+{
+    return gSafeAttributeSaver;
+}
+
+void SetSafeAttributePersistenceProvider(SafeAttributePersistenceProvider * aProvider)
+{
+    if (aProvider != nullptr)
+    {
+        gSafeAttributeSaver = aProvider;
+    }
+}
+
+} // namespace app
+} // namespace chip

src/app/SafeAttributePersistenceProvider.h

@@ -20,10 +20,10 @@
 #include <app/util/attribute-metadata.h>
 #include <lib/support/BufferReader.h>
 #include <lib/support/BufferWriter.h>
+#include <lib/support/DefaultStorageKeyAllocator.h>
 #include <lib/support/Span.h>
 
 #include <cstring>
-#include <inttypes.h>
 
 namespace chip {
 namespace app {
@@ -39,6 +39,19 @@ class SafeAttributePersistenceProvider
     virtual ~SafeAttributePersistenceProvider() = default;
     SafeAttributePersistenceProvider()          = default;
 
+    // Passed-in storage must outlive this object.
+    CHIP_ERROR Init(PersistentStorageDelegate * storage)
+    {
+        if (storage == nullptr)
+        {
+            return CHIP_ERROR_INVALID_ARGUMENT;
+        }
+        mStorage = storage;
+        return CHIP_NO_ERROR;
+    }
+
+    void Shutdown() {}
+
     // The following API provides helper functions to simplify the access of commonly used types.
     // The API may not be complete.
     // Currently implemented write and read types are: bool, uint8_t, uint16_t, uint32_t, unit64_t and
@@ -141,7 +154,7 @@ class SafeAttributePersistenceProvider
      * @param [in] aPath the attribute path for the data being written.
      * @param [in] aValue the data to write. The value will be stored as-is.
      */
-    virtual CHIP_ERROR SafeWriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue) = 0;
+    virtual CHIP_ERROR SafeWriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue);
 
     /**
      * Read an attribute value as a raw sequence of bytes from non-volatile memory.
@@ -154,7 +167,20 @@ class SafeAttributePersistenceProvider
      * @retval CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND if no stored value exists for the attribute
      * @retval CHIP_ERROR_BUFFER_TOO_SMALL aValue.size() is too small to hold the value.
      */
-    virtual CHIP_ERROR SafeReadValue(const ConcreteAttributePath & aPath, MutableByteSpan & aValue) = 0;
+    virtual CHIP_ERROR SafeReadValue(const ConcreteAttributePath & aPath, MutableByteSpan & aValue);
+
+    /**
+     * Key-based I/O. Prefer using the `Safe` calls above
+     */
+    CHIP_ERROR KeyWriteValue(const StorageKeyName & aKey, const ByteSpan & aValue);
+
+    /**
+     * Key-based I/O. Prefer using the `Safe` calls above
+     */
+    CHIP_ERROR KeyReadValue(const StorageKeyName & aKey, MutableByteSpan & aValue);
+
+protected:
+    PersistentStorageDelegate * mStorage;
 };
 
 /**

src/app/codegen-data-model-provider/CodegenDataModelProvider.cpp

@@ -24,12 +24,15 @@
 #include <app/ConcreteCommandPath.h>
 #include <app/EventPathParams.h>
 #include <app/RequiredPrivilege.h>
+#include <app/SafeAttributePersistenceProvider.h>
 #include <app/data-model-provider/MetadataTypes.h>
 #include <app/data-model-provider/Provider.h>
 #include <app/util/IMClusterCommandHandler.h>
 #include <app/util/af-types.h>
 #include <app/util/attribute-storage.h>
 #include <app/util/endpoint-config-api.h>
+#include <app/util/persistence/AttributePersistenceProvider.h>
+#include <app/util/persistence/DefaultAttributePersistenceProvider.h>
 #include <lib/core/CHIPError.h>
 #include <lib/core/DataModelTypes.h>
 #include <lib/support/CodeUtils.h>
@@ -300,8 +303,37 @@ unsigned FindNextDeviceTypeIndex(Span<const EmberAfDeviceType> types, const Data
 
 const ConcreteCommandPath kInvalidCommandPath(kInvalidEndpointId, kInvalidClusterId, kInvalidCommandId);
 
+// Ember requires a persistence provider
+DefaultAttributePersistenceProvider gDefaultAttributePersistenceProvider;
+
 } // namespace
 
+CHIP_ERROR CodegenDataModelProvider::Startup(DataModel::InteractionModelContext context)
+{
+    ReturnErrorOnFailure(DataModel::Provider::Startup(context));
+
+    // Ember/codegen requires a persistence provider. Ensure one is set.
+    if (GetAttributePersistenceProvider() == nullptr)
+    {
+        ReturnErrorOnFailure(gDefaultAttributePersistenceProvider.Init(GetSafeAttributePersistenceProvider()));
+        SetAttributePersistenceProvider(&gDefaultAttributePersistenceProvider);
+    }
+
+    return CHIP_NO_ERROR;
+}
+
+CHIP_ERROR CodegenDataModelProvider::Shutdown()
+{
+    Reset();
+
+    if (GetAttributePersistenceProvider() == &gDefaultAttributePersistenceProvider)
+    {
+        gDefaultAttributePersistenceProvider.Shutdown();
+        // We cannot actually reset it as `SetAttributePersistenceProvider` ignores nullptr
+    }
+    return CHIP_NO_ERROR;
+}
+
 std::optional<CommandId> CodegenDataModelProvider::EmberCommandListIterator::First(const CommandId * list)
 {
     VerifyOrReturnValue(list != nullptr, std::nullopt);

src/app/codegen-data-model-provider/CodegenDataModelProvider.h

@@ -135,11 +135,8 @@ class CodegenDataModelProvider : public chip::app::DataModel::Provider
     }
 
     /// Generic model implementations
-    CHIP_ERROR Shutdown() override
-    {
-        Reset();
-        return CHIP_NO_ERROR;
-    }
+    CHIP_ERROR Startup(DataModel::InteractionModelContext context) override;
+    CHIP_ERROR Shutdown() override;
 
     DataModel::ActionReturnStatus ReadAttribute(const DataModel::ReadAttributeRequest & request,
                                                 AttributeValueEncoder & encoder) override;

src/app/codegen-data-model-provider/model.gni

@@ -40,4 +40,5 @@ codegen_data_model_PUBLIC_DEPS = [
   "${chip_root}/src/app/common:attribute-type",
   "${chip_root}/src/app/data-model-provider",
   "${chip_root}/src/app/codegen-data-model-provider:instance-header",
+  "${chip_root}/src/app/util/persistence",
 ]

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

@@ -16,15 +16,14 @@
  */
 #pragma once
 
-#include "access/SubjectDescriptor.h"
-#include "app/EventPathParams.h"
-#include "lib/core/CHIPError.h"
-#include <lib/core/TLVReader.h>
-#include <lib/core/TLVWriter.h>
-
+#include <access/SubjectDescriptor.h>
 #include <app/AttributeValueDecoder.h>
 #include <app/AttributeValueEncoder.h>
 #include <app/CommandHandler.h>
+#include <app/EventPathParams.h>
+#include <lib/core/CHIPError.h>
+#include <lib/core/TLVReader.h>
+#include <lib/core/TLVWriter.h>
 
 #include <app/data-model-provider/ActionReturnStatus.h>
 #include <app/data-model-provider/Context.h>

src/app/server/Server.cpp

@@ -166,9 +166,16 @@ CHIP_ERROR Server::Init(const ServerInitParams & initParams)
 
     // Set up attribute persistence before we try to bring up the data model
     // handler.
-    SuccessOrExit(err = mAttributePersister.Init(mDeviceStorage));
-    SetAttributePersistenceProvider(&mAttributePersister);
-    SetSafeAttributePersistenceProvider(&mAttributePersister);
+    SuccessOrExit(err = mSafeAttributePersister.Init(mDeviceStorage));
+    SetSafeAttributePersistenceProvider(&mSafeAttributePersister);
+
+    // Temporary setup until a full ember decoupling (including the InitDataModelHandler) is done outside
+    // of app code:
+    //   - InitDataModelHandler uses ember init (so needs a data model provider)
+    //   - Ember requires a persistence provider which is available currently from CodegenDataModelProvider, so
+    //     ensure the codegen data model provider is initialized.
+    // TODO: this should eventually become a `SetDataModelProvider` that is based on `initParams`
+    (void) chip::app::InteractionModelEngine::GetInstance()->GetDataModelProvider();
 
     // SetDataModelProvider() actually initializes/starts the provider.  We need
     // to preserve the following ordering guarantees:
@@ -665,7 +672,7 @@ void Server::Shutdown()
     }
     mICDManager.Shutdown();
 #endif // CHIP_CONFIG_ENABLE_ICD_SERVER
-    mAttributePersister.Shutdown();
+
     // TODO(16969): Remove chip::Platform::MemoryInit() call from Server class, it belongs to outer code
     chip::Platform::MemoryShutdown();
 }