Decouple InteractionModelEngine from TimedHandler (project-chip#32076)

* Decouple TimedHandler.h/cpp from InteractionModelEngine

* Restyle

* Use override instead of virtual

* Update comment - re-add a variant of the previous comment explaining why mTimeLimit is last

* Pull in the IM pointers support, to make less RAM/BSS usage for embedded

* Fix typo and kick restyler

* Restyle

* Previous align was better

* fix name for handler

---------

Co-authored-by: Andrei Litvin <andreilitvin@google.com>

Author: andy31415

src/app/BUILD.gn

@@ -42,6 +42,13 @@ declare_args() {
   chip_im_force_fabric_quota_check = false
 
   enable_eventlist_attribute = false
+
+  # Systems that can spare a bit of RAM for InteractionModelEngine/delegate
+  # pointers should do so (allows InteractionModelEngine decoupling and less usage
+  # of global pointers)
+  chip_im_static_global_interaction_model_engine =
+      current_os != "linux" && current_os != "mac" && current_os != "ios" &&
+      current_os != "android"
 }
 
 buildconfig_header("app_buildconfig") {
@@ -57,6 +64,7 @@ buildconfig_header("app_buildconfig") {
     "CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION=${chip_subscription_timeout_resumption}",
     "CHIP_CONFIG_ENABLE_EVENTLIST_ATTRIBUTE=${enable_eventlist_attribute}",
     "CHIP_CONFIG_ENABLE_READ_CLIENT=${chip_enable_read_client}",
+    "CHIP_CONFIG_STATIC_GLOBAL_INTERACTION_MODEL_ENGINE=${chip_im_static_global_interaction_model_engine}",
   ]
 
   visibility = [ ":app_config" ]
@@ -129,6 +137,8 @@ static_library("interaction-model") {
     "CASESessionManager.h",
     "DeviceProxy.cpp",
     "DeviceProxy.h",
+    "InteractionModelDelegatePointers.cpp",
+    "InteractionModelDelegatePointers.h",
     "InteractionModelEngine.cpp",
     "InteractionModelEngine.h",
     "InteractionModelTimeout.h",
@@ -161,6 +171,7 @@ static_library("interaction-model") {
     "${chip_root}/src/app/icd/server:observer",
     "${chip_root}/src/lib/address_resolve",
     "${chip_root}/src/lib/support",
+    "${chip_root}/src/lib/support:static-support",
     "${chip_root}/src/protocols/interaction_model",
     "${chip_root}/src/protocols/secure_channel",
     "${chip_root}/src/system",

src/app/InteractionModelDelegatePointers.cpp

@@ -0,0 +1,36 @@
+/*
+ *
+ *    Copyright (c) 2024 Project CHIP Authors
+ *    All rights reserved.
+ *
+ *    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 "InteractionModelDelegatePointers.h"
+
+#if CHIP_CONFIG_STATIC_GLOBAL_INTERACTION_MODEL_ENGINE
+
+// TODO: It would be nice to not need to couple the pointers class
+//       to the global interaction model engine
+#include "InteractionModelEngine.h"
+
+namespace chip {
+
+template <>
+app::TimedHandlerDelegate * GlobalInstanceProvider<app::TimedHandlerDelegate>::InstancePointer()
+{
+    return app::InteractionModelEngine::GetInstance();
+}
+
+} // namespace chip
+
+#endif

src/app/InteractionModelDelegatePointers.h

@@ -0,0 +1,37 @@
+/*
+ *
+ *    Copyright (c) 2024 Project CHIP Authors
+ *    All rights reserved.
+ *
+ *    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/AppConfig.h>
+#include <lib/support/static_support_smart_ptr.h>
+
+namespace chip {
+
+#if CHIP_CONFIG_STATIC_GLOBAL_INTERACTION_MODEL_ENGINE
+
+template <class T>
+using InteractionModelDelegatePointer = chip::CheckedGlobalInstanceReference<T>;
+
+#else
+
+template <class T>
+using InteractionModelDelegatePointer = chip::SimpleInstanceReference<T>;
+
+#endif // CHIP_CONFIG_STATIC_GLOBAL_INTERATION_MODEL_ENGINE
+
+} // namespace chip

src/app/InteractionModelEngine.cpp

@@ -840,7 +840,7 @@ CHIP_ERROR InteractionModelEngine::OnTimedRequest(Messaging::ExchangeContext * a
                                                   const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload,
                                                   Protocols::InteractionModel::Status & aStatus)
 {
-    TimedHandler * handler = mTimedHandlers.CreateObject();
+    TimedHandler * handler = mTimedHandlers.CreateObject(this);
     if (handler == nullptr)
     {
         ChipLogProgress(InteractionModel, "no resource for Timed interaction");

src/app/InteractionModelEngine.h

@@ -80,7 +80,8 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
                                public CommandHandler::Callback,
                                public ReadHandler::ManagementCallback,
                                public FabricTable::Delegate,
-                               public SubscriptionsInfoProvider
+                               public SubscriptionsInfoProvider,
+                               public TimedHandlerDelegate
 {
 public:
     /**
@@ -218,26 +219,12 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
     }
     void UnregisterReadHandlerAppCallback() { mpReadHandlerApplicationCallback = nullptr; }
 
-    /**
-     * Called when a timed interaction has failed (i.e. the exchange it was
-     * happening on has closed while the exchange delegate was the timed
-     * handler).
-     */
-    void OnTimedInteractionFailed(TimedHandler * apTimedHandler);
-
-    /**
-     * Called when a timed invoke is received.  This function takes over all
-     * handling of the exchange, status reporting, and so forth.
-     */
+    // TimedHandlerDelegate implementation
+    void OnTimedInteractionFailed(TimedHandler * apTimedHandler) override;
     void OnTimedInvoke(TimedHandler * apTimedHandler, Messaging::ExchangeContext * apExchangeContext,
-                       const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload);
-
-    /**
-     * Called when a timed write is received.  This function takes over all
-     * handling of the exchange, status reporting, and so forth.
-     */
+                       const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload) override;
     void OnTimedWrite(TimedHandler * apTimedHandler, Messaging::ExchangeContext * apExchangeContext,
-                      const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload);
+                      const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload) override;
 
 #if CHIP_CONFIG_ENABLE_READ_CLIENT
     /**

src/app/TimedHandler.cpp

@@ -17,7 +17,7 @@
  */
 
 #include "TimedHandler.h"
-#include <app/InteractionModelEngine.h>
+#include <app/InteractionModelTimeout.h>
 #include <app/MessageDef/TimedRequestMessage.h>
 #include <app/StatusResponse.h>
 #include <lib/core/TLV.h>
@@ -74,19 +74,17 @@ CHIP_ERROR TimedHandler::OnMessageReceived(Messaging::ExchangeContext * aExchang
 
         if (aPayloadHeader.HasMessageType(MsgType::InvokeCommandRequest))
         {
-            auto * imEngine = InteractionModelEngine::GetInstance();
             ChipLogDetail(DataManagement, "Handing timed invoke to IM engine: handler %p exchange " ChipLogFormatExchange, this,
                           ChipLogValueExchange(aExchangeContext));
-            imEngine->OnTimedInvoke(this, aExchangeContext, aPayloadHeader, std::move(aPayload));
+            mDelegate->OnTimedInvoke(this, aExchangeContext, aPayloadHeader, std::move(aPayload));
             return CHIP_NO_ERROR;
         }
 
         if (aPayloadHeader.HasMessageType(MsgType::WriteRequest))
         {
-            auto * imEngine = InteractionModelEngine::GetInstance();
             ChipLogDetail(DataManagement, "Handing timed write to IM engine: handler %p exchange " ChipLogFormatExchange, this,
                           ChipLogValueExchange(aExchangeContext));
-            imEngine->OnTimedWrite(this, aExchangeContext, aPayloadHeader, std::move(aPayload));
+            mDelegate->OnTimedWrite(this, aExchangeContext, aPayloadHeader, std::move(aPayload));
             return CHIP_NO_ERROR;
         }
     }
@@ -101,7 +99,7 @@ CHIP_ERROR TimedHandler::OnMessageReceived(Messaging::ExchangeContext * aExchang
 
 void TimedHandler::OnExchangeClosing(Messaging::ExchangeContext *)
 {
-    InteractionModelEngine::GetInstance()->OnTimedInteractionFailed(this);
+    mDelegate->OnTimedInteractionFailed(this);
 }
 
 CHIP_ERROR TimedHandler::HandleTimedRequestAction(Messaging::ExchangeContext * aExchangeContext,

src/app/TimedHandler.h

@@ -15,22 +15,21 @@
  *    See the License for the specific language governing permissions and
  *    limitations under the License.
  */
-
-/**
- *    @file
- *      Definition of a handler for timed interactions.
- *
- */
-
 #pragma once
 
+#include <app/InteractionModelDelegatePointers.h>
 #include <messaging/ExchangeContext.h>
 #include <messaging/ExchangeDelegate.h>
 #include <system/SystemClock.h>
 #include <system/SystemLayer.h>
 #include <system/SystemPacketBuffer.h>
 #include <transport/raw/MessageHeader.h>
 
+namespace chip {
+namespace app {
+
+class TimedHandler;
+
 /**
  * A TimedHandler handles a Timed Request action and then waits for a
  * subsequent Invoke or Write action and hands those on to
@@ -43,14 +42,37 @@
  * either the exchange is closed or the interaction is handed on to the
  * InteractionModelEngine.
  */
+class TimedHandlerDelegate
+{
+public:
+    virtual ~TimedHandlerDelegate() = default;
 
-namespace chip {
-namespace app {
+    /**
+     * Called when a timed invoke is received.  This function takes over all
+     * handling of the exchange, status reporting, and so forth.
+     */
+    virtual void OnTimedInvoke(TimedHandler * apTimedHandler, Messaging::ExchangeContext * apExchangeContext,
+                               const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload) = 0;
+
+    /**
+     * Called when a timed write is received.  This function takes over all
+     * handling of the exchange, status reporting, and so forth.
+     */
+    virtual void OnTimedWrite(TimedHandler * apTimedHandler, Messaging::ExchangeContext * apExchangeContext,
+                              const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload) = 0;
+
+    /**
+     * Called when a timed interaction has failed (i.e. the exchange it was
+     * happening on has closed while the exchange delegate was the timed
+     * handler).
+     */
+    virtual void OnTimedInteractionFailed(TimedHandler * apTimedHandler) = 0;
+};
 
 class TimedHandler : public Messaging::ExchangeDelegate
 {
 public:
-    TimedHandler() {}
+    TimedHandler(TimedHandlerDelegate * delegate) : mDelegate(delegate) {}
     ~TimedHandler() override {}
 
     // ExchangeDelegate implementation.
@@ -83,15 +105,31 @@ class TimedHandler : public Messaging::ExchangeDelegate
         kExpectingFollowingAction, // Expecting write or invoke.
     };
 
-    // Because we have a vtable pointer and mTimeLimit needs to be 8-byte
-    // aligned on ARM, putting mState first here means we fit in 16 bytes on
-    // 32-bit ARM, whereas if we put it second we'd be 24 bytes.
-    // On platforms where either vtable pointers are 8 bytes or 64-bit ints can
-    // be 4-byte-aligned the ordering here does not matter.
     State mState = State::kExpectingTimedAction;
+
+    /// This may be "fake" pointer or a real delegate pointer, depending
+    /// on CHIP_CONFIG_STATIC_GLOBAL_INTERACTION_MODEL_ENGINE setting.
+    ///
+    /// When this is not a real pointer, it checks that the value is always
+    /// set to the global InteractionModelEngine and the size of this
+    /// member is 1 byte.
+    InteractionModelDelegatePointer<TimedHandlerDelegate> mDelegate;
+
     // We keep track of the time limit for message reception, in case our
     // exchange's "response expected" timer gets delayed and does not fire when
     // the time runs out.
+    //
+    // NOTE: mTimeLimit needs to be 8-byte aligned on ARM so we place this last,
+    //       to allow previous values to potentially use remaining packing space.
+    //       Rationale:
+    //         -   vtable is 4-byte aligned on 32-bit arm
+    //         -   mTimeLimit requires 8-byte aligment
+    //         =>  As a result we may gain 4 bytes if we place mTimeLimit last.
+    // Expectation of memory layout:
+    //   - vtable pointer (4 bytes & 4 byte alignment)
+    //   - other members  (2 bytes on embedded "global pointer" arm)
+    //                    (2 bytes padding for 8-byte alignment)
+    //   - mTimeLimit     (8 bytes & 8 byte alignment)
     System::Clock::Timestamp mTimeLimit;
 };
 

src/lib/support/BUILD.gn

@@ -171,6 +171,10 @@ source_set("type-traits") {
   sources = [ "TypeTraits.h" ]
 }
 
+source_set("static-support") {
+  sources = [ "static_support_smart_ptr.h" ]
+}
+
 static_library("support") {
   output_name = "libSupportLayer"