Set dataModelProvider as a required argument for Controller::FactoryInitParams (project-chip#36613)

* Add dataModelProvider to Factor init parameters for the codegen data model provider

* make setting the same data model on interaction model provider a noop

* Fix compile

* Fix links

* Restyle

* Restyled by prettier-markdown

* Fix text

* Fix some deps

* Restyled by gn

* Remove dependencies/auto-init of codegen data model in the interaction model. We now fully expect all applications to provide a data model provider

* Fix include

* Update src/app/InteractionModelEngine.cpp

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

* Address some code review comments

* Fix unit tests

* Restyle

* Add more comments about why we have very intentional ordering

---------

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

docs/upgrading.md

@@ -93,11 +93,12 @@ Replacements for methods are:
 -   `chip::app::GetAttributeAccessOverride` replaced by
     `chip::app::AttributeAccessInterfaceRegistry::Instance().Get`
 
-### `ServerInitParams::dataModelProvider` in `Server::Init`
+### `ServerInitParams::dataModelProvider` in `Server::Init` and `FactoryInitParams`
 
-Server initialization requires a set data model provider to work rather than
-auto-initializing ember-compatible code-generated data models.
+Server and controller initialization require a set data model provider to work
+rather than auto-initializing ember-compatible code-generated data models.
 
 To preserve `codegen/zap` generated logic, use
 `CodegenDataModelProviderInstance` (see changes in
-<https://github.com/project-chip/connectedhomeip/pull/36558>).
+[36558](https://github.com/project-chip/connectedhomeip/pull/36558) and
+[36613](https://github.com/project-chip/connectedhomeip/pull/36613) ).

examples/chip-tool/commands/common/CHIPCommand.cpp

@@ -18,6 +18,7 @@
 
 #include "CHIPCommand.h"
 
+#include <app/codegen-data-model-provider/Instance.h>
 #include <commands/icd/ICDCommand.h>
 #include <controller/CHIPDeviceControllerFactory.h>
 #include <credentials/attestation_verifier/FileAttestationTrustStore.h>
@@ -137,6 +138,7 @@ CHIP_ERROR CHIPCommand::MaybeSetUpStack()
     factoryInitParams.opCertStore              = &mOpCertStore;
     factoryInitParams.enableServerInteractions = NeedsOperationalAdvertising();
     factoryInitParams.sessionKeystore          = &sSessionKeystore;
+    factoryInitParams.dataModelProvider        = chip::app::CodegenDataModelProviderInstance();
 
     // Init group data provider that will be used for all group keys and IPKs for the
     // chip-tool-configured fabrics. This is OK to do once since the fabric tables

examples/fabric-admin/commands/common/CHIPCommand.cpp

@@ -19,6 +19,8 @@
 #include "CHIPCommand.h"
 
 #include "IcdManager.h"
+
+#include <app/codegen-data-model-provider/Instance.h>
 #include <controller/CHIPDeviceControllerFactory.h>
 #include <credentials/attestation_verifier/FileAttestationTrustStore.h>
 #include <device_manager/PairingManager.h>
@@ -121,6 +123,7 @@ CHIP_ERROR CHIPCommand::MaybeSetUpStack()
     factoryInitParams.opCertStore              = &mOpCertStore;
     factoryInitParams.enableServerInteractions = NeedsOperationalAdvertising();
     factoryInitParams.sessionKeystore          = &sSessionKeystore;
+    factoryInitParams.dataModelProvider        = chip::app::CodegenDataModelProviderInstance();
 
     // Init group data provider that will be used for all group keys and IPKs for the
     // fabric-admin-configured fabrics. This is OK to do once since the fabric tables

examples/platform/linux/BUILD.gn

@@ -164,7 +164,10 @@ source_set("commissioner-main") {
     "${chip_root}/src/controller:controller",
     "${chip_root}/src/lib",
   ]
-  deps = [ "${chip_root}/src/app/server" ]
+  deps = [
+    "${chip_root}/src/app/codegen-data-model-provider:instance-header",
+    "${chip_root}/src/app/server",
+  ]
 
   if (chip_enable_transport_trace) {
     deps += [ "${chip_root}/examples/common/tracing:trace_handlers" ]

examples/platform/linux/CommissionerMain.cpp

@@ -24,6 +24,7 @@
 #if CHIP_DEVICE_CONFIG_ENABLE_BOTH_COMMISSIONER_AND_COMMISSIONEE
 
 #include <app/clusters/network-commissioning/network-commissioning.h>
+#include <app/codegen-data-model-provider/Instance.h>
 #include <app/server/Dnssd.h>
 #include <app/server/OnboardingCodesUtil.h>
 #include <app/server/Server.h>
@@ -134,6 +135,7 @@ CHIP_ERROR InitCommissioner(uint16_t commissionerPort, uint16_t udcListenPort, F
     factoryParams.fabricIndependentStorage = &gServerStorage;
     factoryParams.fabricTable              = &Server::GetInstance().GetFabricTable();
     factoryParams.sessionKeystore          = &gSessionKeystore;
+    factoryParams.dataModelProvider        = chip::app::CodegenDataModelProviderInstance();
 
     gGroupDataProvider.SetStorageDelegate(&gServerStorage);
     gGroupDataProvider.SetSessionKeystore(factoryParams.sessionKeystore);

src/app/BUILD.gn

@@ -206,10 +206,6 @@ static_library("interaction-model") {
     "${chip_root}/src/app:global-attributes",
   ]
 
-  # Temporary dependency: codegen data provider instance should be provided
-  # by the application
-  deps += [ "${chip_root}/src/app/codegen-data-model-provider:instance-header" ]
-
   public_deps = [
     ":app_config",
     ":command-handler-impl",

src/app/InteractionModelEngine.cpp

@@ -50,10 +50,6 @@
 #include <lib/support/FibonacciUtils.h>
 #include <protocols/interaction_model/StatusCode.h>
 
-// TODO: defaulting to codegen should eventually be an application choice and not
-//       hard-coded in the interaction model
-#include <app/codegen-data-model-provider/Instance.h>
-
 namespace chip {
 namespace app {
 namespace {
@@ -1798,9 +1794,15 @@ Protocols::InteractionModel::Status InteractionModelEngine::CheckCommandExistenc
 
 DataModel::Provider * InteractionModelEngine::SetDataModelProvider(DataModel::Provider * model)
 {
-    // Alternting data model should not be done while IM is actively handling requests.
+    // Altering data model should not be done while IM is actively handling requests.
     VerifyOrDie(mReadHandlers.begin() == mReadHandlers.end());
 
+    if (model == mDataModelProvider)
+    {
+        // no-op, just return
+        return model;
+    }
+
     DataModel::Provider * oldModel = mDataModelProvider;
     if (oldModel != nullptr)
     {
@@ -1830,14 +1832,11 @@ DataModel::Provider * InteractionModelEngine::SetDataModelProvider(DataModel::Pr
     return oldModel;
 }
 
-DataModel::Provider * InteractionModelEngine::GetDataModelProvider()
+DataModel::Provider * InteractionModelEngine::GetDataModelProvider() const
 {
-    if (mDataModelProvider == nullptr)
-    {
-        // These should be called within the CHIP processing loop.
-        assertChipStackLockedByCurrentThread();
-        SetDataModelProvider(CodegenDataModelProviderInstance());
-    }
+    // These should be called within the CHIP processing loop.
+    assertChipStackLockedByCurrentThread();
+
     return mDataModelProvider;
 }
 

src/app/InteractionModelEngine.h

@@ -404,10 +404,7 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
     }
 #endif
 
-    // Temporarily NOT const because the data model provider will be auto-set
-    // to codegen on first usage. This behaviour will be changed once each
-    // application must explicitly set the data model provider.
-    DataModel::Provider * GetDataModelProvider();
+    DataModel::Provider * GetDataModelProvider() const;
 
     // MUST NOT be used while the interaction model engine is running as interaction
     // model functionality (e.g. active reads/writes/subscriptions) rely on data model

src/app/SubscriptionResumptionSessionEstablisher.cpp

@@ -17,7 +17,6 @@
 
 #include <app/InteractionModelEngine.h>
 #include <app/SubscriptionResumptionSessionEstablisher.h>
-#include <app/codegen-data-model-provider/Instance.h>
 
 namespace chip {
 namespace app {

src/app/tests/TestAclEvent.cpp

@@ -24,6 +24,7 @@
 #include <app/InteractionModelEngine.h>
 #include <app/MessageDef/AttributeReportIBs.h>
 #include <app/MessageDef/EventDataIB.h>
+#include <app/codegen-data-model-provider/Instance.h>
 #include <app/reporting/tests/MockReportScheduler.h>
 #include <app/tests/AppTestContext.h>
 #include <app/tests/test-interaction-model-api.h>
@@ -217,6 +218,7 @@ TEST_F(TestAclEvent, TestReadRoundtripWithEventStatusIBInEventReport)
 
     auto * engine = chip::app::InteractionModelEngine::GetInstance();
 
+    engine->SetDataModelProvider(CodegenDataModelProviderInstance());
     EXPECT_EQ(engine->Init(&GetExchangeManager(), &GetFabricTable(), app::reporting::GetDefaultReportScheduler()), CHIP_NO_ERROR);
 
     // A custom AccessControl::Delegate has been installed that grants privilege to any cluster except the test cluster.

src/app/tests/TestInteractionModelEngine.cpp

@@ -88,6 +88,7 @@ TEST_F(TestInteractionModelEngine, TestAttributePathParamsPushRelease)
 
     InteractionModelEngine * engine = InteractionModelEngine::GetInstance();
 
+    engine->SetDataModelProvider(CodegenDataModelProviderInstance());
     EXPECT_EQ(engine->Init(&GetExchangeManager(), &GetFabricTable(), app::reporting::GetDefaultReportScheduler()), CHIP_NO_ERROR);
 
     SingleLinkedListNode<AttributePathParams> * attributePathParamsList = nullptr;
@@ -123,6 +124,7 @@ TEST_F(TestInteractionModelEngine, TestRemoveDuplicateConcreteAttribute)
 
     InteractionModelEngine * engine = InteractionModelEngine::GetInstance();
 
+    engine->SetDataModelProvider(CodegenDataModelProviderInstance());
     EXPECT_EQ(CHIP_NO_ERROR, engine->Init(&GetExchangeManager(), &GetFabricTable(), app::reporting::GetDefaultReportScheduler()));
 
     SingleLinkedListNode<AttributePathParams> * attributePathParamsList = nullptr;
@@ -259,6 +261,7 @@ TEST_F_FROM_FIXTURE(TestInteractionModelEngine, TestSubjectHasActiveSubscription
     ASSERT_TRUE(exchangeCtx1);
 
     // InteractionModelEngine init
+    engine->SetDataModelProvider(CodegenDataModelProviderInstance());
     EXPECT_EQ(CHIP_NO_ERROR, engine->Init(&GetExchangeManager(), &GetFabricTable(), reporting::GetDefaultReportScheduler()));
 
     // Verify that there are no active subscriptions
@@ -306,6 +309,7 @@ TEST_F_FROM_FIXTURE(TestInteractionModelEngine, TestSubjectHasActiveSubscription
     ASSERT_TRUE(exchangeCtx1);
 
     // InteractionModelEngine init
+    engine->SetDataModelProvider(CodegenDataModelProviderInstance());
     EXPECT_EQ(CHIP_NO_ERROR, engine->Init(&GetExchangeManager(), &GetFabricTable(), reporting::GetDefaultReportScheduler()));
 
     // Verify that both Alice and Bob have no active subscriptions
@@ -364,6 +368,7 @@ TEST_F_FROM_FIXTURE(TestInteractionModelEngine, TestSubjectHasActiveSubscription
     ASSERT_TRUE(exchangeCtx2);
 
     // InteractionModelEngine init
+    engine->SetDataModelProvider(CodegenDataModelProviderInstance());
     EXPECT_EQ(CHIP_NO_ERROR, engine->Init(&GetExchangeManager(), &GetFabricTable(), reporting::GetDefaultReportScheduler()));
 
     // Verify that both Alice and Bob have no active subscriptions
@@ -446,6 +451,7 @@ TEST_F_FROM_FIXTURE(TestInteractionModelEngine, TestSubjectHasActiveSubscription
     ASSERT_TRUE(exchangeCtx22);
 
     // InteractionModelEngine init
+    engine->SetDataModelProvider(CodegenDataModelProviderInstance());
     EXPECT_EQ(CHIP_NO_ERROR, engine->Init(&GetExchangeManager(), &GetFabricTable(), reporting::GetDefaultReportScheduler()));
 
     // Verify that both Alice and Bob have no active subscriptions
@@ -525,6 +531,7 @@ TEST_F_FROM_FIXTURE(TestInteractionModelEngine, TestSubjectHasActiveSubscription
     FabricIndex bobFabricIndex = 1;
 
     // InteractionModelEngine init
+    engine->SetDataModelProvider(CodegenDataModelProviderInstance());
     EXPECT_EQ(CHIP_NO_ERROR, engine->Init(&GetExchangeManager(), &GetFabricTable(), reporting::GetDefaultReportScheduler()));
 
     // Make sure we are using CASE sessions, because there is no defunct-marking for PASE.
@@ -575,6 +582,7 @@ TEST_F(TestInteractionModelEngine, TestSubjectHasPersistedSubscription)
 
     EXPECT_EQ(subscriptionStorage.Init(&storage), CHIP_NO_ERROR);
 
+    engine->SetDataModelProvider(CodegenDataModelProviderInstance());
     EXPECT_EQ(CHIP_NO_ERROR,
               engine->Init(&GetExchangeManager(), &GetFabricTable(), app::reporting::GetDefaultReportScheduler(), nullptr,
                            &subscriptionStorage));
@@ -630,6 +638,7 @@ TEST_F_FROM_FIXTURE(TestInteractionModelEngine, TestSubscriptionResumptionTimer)
 
     InteractionModelEngine * engine = InteractionModelEngine::GetInstance();
 
+    engine->SetDataModelProvider(CodegenDataModelProviderInstance());
     EXPECT_EQ(engine->Init(&GetExchangeManager(), &GetFabricTable(), app::reporting::GetDefaultReportScheduler()), CHIP_NO_ERROR);
 
     uint32_t timeTillNextResubscriptionMs;
@@ -661,6 +670,7 @@ TEST_F_FROM_FIXTURE(TestInteractionModelEngine, TestDecrementNumSubscriptionsToR
     constexpr uint8_t kNumberOfSubsToResume = 5;
     uint8_t numberOfSubsRemaining           = kNumberOfSubsToResume;
 
+    engine->SetDataModelProvider(CodegenDataModelProviderInstance());
     EXPECT_EQ(engine->Init(&GetExchangeManager(), &GetFabricTable(), app::reporting::GetDefaultReportScheduler()), CHIP_NO_ERROR);
 
 #if CHIP_CONFIG_ENABLE_ICD_CIP && !CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION

src/controller/CHIPDeviceControllerFactory.cpp

@@ -24,6 +24,7 @@
 
 #include <controller/CHIPDeviceControllerFactory.h>
 
+#include <app/InteractionModelEngine.h>
 #include <app/OperationalSessionSetup.h>
 #include <app/TimerDelegates.h>
 #include <app/reporting/ReportSchedulerImpl.h>
@@ -100,6 +101,10 @@ CHIP_ERROR DeviceControllerFactory::ReinitSystemStateIfNecessary()
     params.certificateValidityPolicy = mCertificateValidityPolicy;
     params.sessionResumptionStorage  = mSessionResumptionStorage;
 
+    // re-initialization keeps any previously initialized values. The only place where
+    // a provider exists is in the InteractionModelEngine, so just say "keep it as is".
+    params.dataModelProvider = app::InteractionModelEngine::GetInstance()->GetDataModelProvider();
+
     return InitSystemState(params);
 }
 
@@ -127,6 +132,13 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params)
     ChipLogError(Controller, "Warning: Device Controller Factory should be with a CHIP Device Layer...");
 #endif // CONFIG_DEVICE_LAYER
 
+    if (params.dataModelProvider == nullptr)
+    {
+        ChipLogError(AppServer, "Device Controller Factory requires a `dataModelProvider` value.");
+        ChipLogError(AppServer, "For backwards compatibility, you likely can use `CodegenDataModelProviderInstance()`");
+    }
+
+    VerifyOrReturnError(params.dataModelProvider != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
     VerifyOrReturnError(stateParams.systemLayer != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
     VerifyOrReturnError(stateParams.udpEndPointManager != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
 
@@ -238,6 +250,16 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params)
     ReturnErrorOnFailure(stateParams.unsolicitedStatusHandler->Init(stateParams.exchangeMgr));
     ReturnErrorOnFailure(stateParams.bdxTransferServer->Init(stateParams.systemLayer, stateParams.exchangeMgr));
 
+    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`.
+    interactionModelEngine->SetDataModelProvider(params.dataModelProvider);
+
     InitDataModelHandler();
 
     ReturnErrorOnFailure(Dnssd::Resolver::Instance().Init(stateParams.udpEndPointManager));
@@ -295,8 +317,8 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params)
     stateParams.caseSessionManager = Platform::New<CASESessionManager>();
     ReturnErrorOnFailure(stateParams.caseSessionManager->Init(stateParams.systemLayer, sessionManagerConfig));
 
-    ReturnErrorOnFailure(chip::app::InteractionModelEngine::GetInstance()->Init(
-        stateParams.exchangeMgr, stateParams.fabricTable, stateParams.reportScheduler, stateParams.caseSessionManager));
+    ReturnErrorOnFailure(interactionModelEngine->Init(stateParams.exchangeMgr, stateParams.fabricTable, stateParams.reportScheduler,
+                                                      stateParams.caseSessionManager));
 
     // store the system state
     mSystemState = chip::Platform::New<DeviceControllerSystemState>(std::move(stateParams));

src/controller/CHIPDeviceControllerFactory.h

@@ -160,6 +160,11 @@ struct FactoryInitParams
     /* The port used for operational communication to listen for and send messages over UDP/TCP.
      * The default value of `0` will pick any available port. */
     uint16_t listenPort = 0;
+
+    // MUST NOT be null during initialization: every application must define the
+    // data model it wants to use. Backwards-compatibility can use `CodegenDataModelProviderInstance`
+    // for ember/zap-generated models.
+    chip::app::DataModel::Provider * dataModelProvider = nullptr;
 };
 
 class DeviceControllerFactory

src/controller/java/AndroidDeviceControllerWrapper.cpp

@@ -25,17 +25,16 @@
 
 #include <string.h>
 
+#include <app/codegen-data-model-provider/Instance.h>
 #include <app/server/Dnssd.h>
-
-#include <lib/support/CodeUtils.h>
-#include <lib/support/JniReferences.h>
-#include <lib/support/JniTypeWrappers.h>
-
 #include <controller/CHIPDeviceControllerFactory.h>
 #include <controller/java/AndroidICDClient.h>
 #include <credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h>
 #include <credentials/attestation_verifier/DeviceAttestationVerifier.h>
 #include <lib/core/TLV.h>
+#include <lib/support/CodeUtils.h>
+#include <lib/support/JniReferences.h>
+#include <lib/support/JniTypeWrappers.h>
 #include <lib/support/PersistentStorageMacros.h>
 #include <lib/support/SafeInt.h>
 #include <lib/support/ScopedBuffer.h>
@@ -210,6 +209,7 @@ AndroidDeviceControllerWrapper * AndroidDeviceControllerWrapper::AllocateNew(
     setupParams.defaultCommissioner            = &wrapper->mAutoCommissioner;
     initParams.fabricIndependentStorage        = wrapperStorage;
     initParams.sessionKeystore                 = &wrapper->mSessionKeystore;
+    initParams.dataModelProvider               = app::CodegenDataModelProviderInstance();
 
     wrapper->mGroupDataProvider.SetStorageDelegate(wrapperStorage);
     wrapper->mGroupDataProvider.SetSessionKeystore(initParams.sessionKeystore);

src/controller/python/ChipDeviceController-ScriptBinding.cpp

@@ -42,6 +42,7 @@
 
 #include <app/DeviceProxy.h>
 #include <app/InteractionModelEngine.h>
+#include <app/codegen-data-model-provider/Instance.h>
 #include <app/icd/client/CheckInHandler.h>
 #include <app/icd/client/DefaultCheckInDelegate.h>
 #include <app/icd/client/DefaultICDClientStorage.h>
@@ -271,6 +272,7 @@ PyChipError pychip_DeviceController_StackInit(Controller::Python::StorageAdapter
 
     factoryParams.fabricIndependentStorage = storageAdapter;
     factoryParams.sessionKeystore          = &sSessionKeystore;
+    factoryParams.dataModelProvider        = app::CodegenDataModelProviderInstance();
 
     sICDClientStorage.Init(storageAdapter, &sSessionKeystore);
 

src/controller/python/chip/internal/CommissionerImpl.cpp

@@ -16,6 +16,7 @@
  */
 #include <memory>
 
+#include <app/codegen-data-model-provider/Instance.h>
 #include <controller/CHIPDeviceController.h>
 #include <controller/CHIPDeviceControllerFactory.h>
 #include <controller/ExampleOperationalCredentialsIssuer.h>
@@ -135,6 +136,7 @@ extern "C" chip::Controller::DeviceCommissioner * pychip_internal_Commissioner_N
 
         factoryParams.fabricIndependentStorage = &gServerStorage;
         factoryParams.sessionKeystore          = &gSessionKeystore;
+        factoryParams.dataModelProvider        = chip::app::CodegenDataModelProviderInstance();
 
         // Initialize group data provider for local group key state and IPKs
         gGroupDataProvider.SetStorageDelegate(&gServerStorage);