From a9bd0ce94d0bb34a20b1fe59374b02d80cb78fb9 Mon Sep 17 00:00:00 2001 From: Yufeng Wang Date: Tue, 17 Dec 2024 11:26:26 -0800 Subject: [PATCH] Decouple InitDataModelHandler from libCHIP (#36725) * Decouple InitDataModelHandler from libCHIP * Use StartUp to init * Seperate namespace cleanup out * Mock function for linking * Restyled by clang-format * Add API comment * Fix mutiple defination conflicts * Address review comments * Restyled by whitespace * Seperate InitDataModel out * Revert "Seperate InitDataModel out" This reverts commit 5a8af59841ed4eb0bc573ba877d5ade7f31aa212. * Do not directly manipulate the base class's Startup method * Address review comment * Restyled by whitespace * Adjust the init order * Restyled by whitespace * Update src/app/server/Server.cpp Co-authored-by: Boris Zbarsky * Update src/controller/CHIPDeviceControllerFactory.cpp Co-authored-by: Boris Zbarsky * Add TODO comment * documented/named InitDataModel to make it clear that this is a temporary hack for a test --------- Co-authored-by: Restyled.io Co-authored-by: Boris Zbarsky --- src/app/server/Server.cpp | 30 ++++++++++--------- .../tests/TestCommissioningWindowManager.cpp | 3 -- src/app/tests/TestInteractionModelEngine.cpp | 2 +- src/app/tests/test-ember-api.cpp | 3 ++ .../CHIPDeviceControllerFactory.cpp | 10 ++----- .../tests/TestServerCommandDispatch.cpp | 18 +++++++++++ .../tests/data_model/DataModelFixtures.cpp | 3 ++ .../codegen/CodegenDataModelProvider.cpp | 9 ++++++ .../codegen/CodegenDataModelProvider.h | 6 ++++ .../tests/TestCodegenModelViaMocks.cpp | 3 ++ 10 files changed, 61 insertions(+), 26 deletions(-) diff --git a/src/app/server/Server.cpp b/src/app/server/Server.cpp index 4e0df666e36e47..2e4a10a452fa9e 100644 --- a/src/app/server/Server.cpp +++ b/src/app/server/Server.cpp @@ -27,7 +27,6 @@ #include #include #include -#include #if CONFIG_NETWORK_LAYER_BLE #include @@ -170,17 +169,6 @@ CHIP_ERROR Server::Init(const ServerInitParams & initParams) SuccessOrExit(err = mAttributePersister.Init(mDeviceStorage)); SetSafeAttributePersistenceProvider(&mAttributePersister); - // SetDataModelProvider() actually initializes/starts the provider. We need - // to preserve the following ordering guarantees: - // - // 1) Provider initialization (under SetDataModelProvider) happens after - // SetSafeAttributePersistenceProvider, since the provider can then use - // the safe persistence provider to implement and initialize its own attribute persistence logic. - // 2) For now, provider initialization happens before InitDataModelHandler(), which depends - // on atttribute persistence being already set up before it runs. Longer-term, the logic from - // InitDataModelHandler should just move into the codegen provider. - app::InteractionModelEngine::GetInstance()->SetDataModelProvider(initParams.dataModelProvider); - { FabricTable::InitParams fabricTableInitParams; fabricTableInitParams.storage = mDeviceStorage; @@ -302,8 +290,22 @@ CHIP_ERROR Server::Init(const ServerInitParams & initParams) } #endif // CHIP_CONFIG_ENABLE_SERVER_IM_EVENT - // This initializes clusters, so should come after lower level initialization. - InitDataModelHandler(); + // SetDataModelProvider() initializes and starts the provider, which in turn + // triggers the initialization of cluster implementations. This callsite is + // critical because it ensures that cluster-level initialization occurs only + // after all necessary low-level dependencies have been set up. + // + // Ordering guarantees: + // 1) Provider initialization (under SetDataModelProvider) must happen after + // SetSafeAttributePersistenceProvider to ensure the provider can leverage + // the safe persistence provider for attribute persistence logic. + // 2) It must occur after all low-level components that cluster implementations + // might depend on have been initialized, as they rely on these components + // during their own initialization. + // + // This remains the single point of entry to ensure that all cluster-level + // initialization is performed in the correct order. + app::InteractionModelEngine::GetInstance()->SetDataModelProvider(initParams.dataModelProvider); #if defined(CHIP_APP_USE_ECHO) err = InitEchoHandler(&mExchangeMgr); diff --git a/src/app/tests/TestCommissioningWindowManager.cpp b/src/app/tests/TestCommissioningWindowManager.cpp index 7d1e22626b29dd..1fe151107707ce 100644 --- a/src/app/tests/TestCommissioningWindowManager.cpp +++ b/src/app/tests/TestCommissioningWindowManager.cpp @@ -41,9 +41,6 @@ using chip::CommissioningWindowAdvertisement; using chip::CommissioningWindowManager; using chip::Server; -// Mock function for linking -void InitDataModelHandler() {} - namespace { bool sAdminFabricIndexDirty = false; bool sAdminVendorIdDirty = false; diff --git a/src/app/tests/TestInteractionModelEngine.cpp b/src/app/tests/TestInteractionModelEngine.cpp index 30ca7d0001f01b..87aa3e3c64f58b 100644 --- a/src/app/tests/TestInteractionModelEngine.cpp +++ b/src/app/tests/TestInteractionModelEngine.cpp @@ -39,8 +39,8 @@ #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS #include #include - #endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS + namespace { class NullReadHandlerCallback : public chip::app::ReadHandler::ManagementCallback diff --git a/src/app/tests/test-ember-api.cpp b/src/app/tests/test-ember-api.cpp index 2b630327a21897..48ab29cb53942d 100644 --- a/src/app/tests/test-ember-api.cpp +++ b/src/app/tests/test-ember-api.cpp @@ -34,3 +34,6 @@ uint16_t emberAfGetClusterServerEndpointIndex(chip::EndpointId endpoint, chip::C } return endpoint; } + +// Mock function for linking +void InitDataModelHandler() {} diff --git a/src/controller/CHIPDeviceControllerFactory.cpp b/src/controller/CHIPDeviceControllerFactory.cpp index 649c3a37408a26..3d84d0b6b35442 100644 --- a/src/controller/CHIPDeviceControllerFactory.cpp +++ b/src/controller/CHIPDeviceControllerFactory.cpp @@ -252,16 +252,10 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params) chip::app::InteractionModelEngine * interactionModelEngine = chip::app::InteractionModelEngine::GetInstance(); - // Note placement of this BEFORE `InitDataModelHandler` since InitDataModelHandler may - // rely on ember (does emberAfInit() and configure which may load data from NVM). - // - // Expected forward path is that we will move move and more things inside datamodel - // provider (e.g. storage settings) so we want datamodelprovider available before - // `InitDataModelHandler`. + // Initialize the data model now that everything cluster implementations might + // depend on is initalized. interactionModelEngine->SetDataModelProvider(params.dataModelProvider); - InitDataModelHandler(); - ReturnErrorOnFailure(Dnssd::Resolver::Instance().Init(stateParams.udpEndPointManager)); if (params.enableServerInteractions) diff --git a/src/controller/tests/TestServerCommandDispatch.cpp b/src/controller/tests/TestServerCommandDispatch.cpp index 2389ce08e6baf2..f4457c0013ef34 100644 --- a/src/controller/tests/TestServerCommandDispatch.cpp +++ b/src/controller/tests/TestServerCommandDispatch.cpp @@ -126,6 +126,9 @@ CHIP_ERROR TestClusterCommandHandler::EnumerateAcceptedCommands(const ConcreteCl namespace { +// TODO:(#36837) implementing its own provider instead of using "CodegenDataModelProvider" +// TestServerCommandDispatch should provide its own dedicated data model provider rather than using CodegenDataModelProvider +// provider. This class exists solely for one specific test scenario, on a temporary basis. class DispatchTestDataModel : public CodegenDataModelProvider { public: @@ -134,6 +137,20 @@ class DispatchTestDataModel : public CodegenDataModelProvider static DispatchTestDataModel instance; return instance; } + + // The Startup method initializes the data model provider with a given context. + // This approach ensures that the test relies on a more controlled and explicit data model provider + // rather than depending on the code-generated one with undefined modifications. + CHIP_ERROR Startup(DataModel::InteractionModelContext context) override + { + ReturnErrorOnFailure(CodegenDataModelProvider::Startup(context)); + return CHIP_NO_ERROR; + } + +protected: + // Since the current unit tests do not involve any cluster implementations, we override InitDataModelForTesting + // to do nothing, thereby preventing calls to the Ember-specific InitDataModelHandler. + void InitDataModelForTesting() override {} }; class TestServerCommandDispatch : public chip::Test::AppContext @@ -144,6 +161,7 @@ class TestServerCommandDispatch : public chip::Test::AppContext AppContext::SetUp(); mOldProvider = InteractionModelEngine::GetInstance()->SetDataModelProvider(&DispatchTestDataModel::Instance()); } + void TearDown() { InteractionModelEngine::GetInstance()->SetDataModelProvider(mOldProvider); diff --git a/src/controller/tests/data_model/DataModelFixtures.cpp b/src/controller/tests/data_model/DataModelFixtures.cpp index 6cb4f114c3d1ef..d5c02b5f0d16d3 100644 --- a/src/controller/tests/data_model/DataModelFixtures.cpp +++ b/src/controller/tests/data_model/DataModelFixtures.cpp @@ -40,6 +40,9 @@ using namespace chip::app::Clusters; using namespace chip::app::Clusters::UnitTesting; using namespace chip::Protocols; +// Mock function for linking +void InitDataModelHandler() {} + namespace chip { namespace app { diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp index b599ba1dfdc0dd..1cf34fcb2c8fbb 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -417,6 +418,8 @@ CHIP_ERROR CodegenDataModelProvider::Startup(DataModel::InteractionModelContext } } + InitDataModelForTesting(); + return CHIP_NO_ERROR; } @@ -859,6 +862,12 @@ ConcreteCommandPath CodegenDataModelProvider::NextGeneratedCommand(const Concret return ConcreteCommandPath(before.mEndpointId, before.mClusterId, commandId); } +void CodegenDataModelProvider::InitDataModelForTesting() +{ + // Call the Ember-specific InitDataModelHandler + InitDataModelHandler(); +} + std::optional CodegenDataModelProvider::FirstDeviceType(EndpointId endpoint) { // Use the `Index` version even though `emberAfDeviceTypeListFromEndpoint` would work because diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.h b/src/data-model-providers/codegen/CodegenDataModelProvider.h index 0493272dbbff47..e3455d5ce03a1b 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.h +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.h @@ -187,6 +187,12 @@ class CodegenDataModelProvider : public DataModel::Provider void Temporary_ReportAttributeChanged(const AttributePathParams & path) override; +protected: + // Temporary hack for a test: Initializes the data model for testing purposes only. + // This method serves as a placeholder and should NOT be used outside of specific tests. + // It is expected to be removed or replaced with a proper implementation in the future.TODO:(#36837). + virtual void InitDataModelForTesting(); + private: // Iteration is often done in a tight loop going through all values. // To avoid N^2 iterations, cache a hint of where something is positioned diff --git a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp index 302d26a8941cd0..fd2165983891b5 100644 --- a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp +++ b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp @@ -75,6 +75,9 @@ using namespace chip::app::Clusters::Globals::Attributes; using chip::Protocols::InteractionModel::Status; +// Mock function for linking +void InitDataModelHandler() {} + namespace { constexpr AttributeId kAttributeIdReadOnly = 0x3001;