From 8eaca940dce3fcbf94f6c42c53d82b31083f4957 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 15 Feb 2024 12:26:52 -0500 Subject: [PATCH] Decouple InteractionModelEngine from TimedHandler (#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 --- src/app/BUILD.gn | 11 ++ src/app/InteractionModelDelegatePointers.cpp | 36 +++++ src/app/InteractionModelDelegatePointers.h | 37 +++++ src/app/InteractionModelEngine.cpp | 2 +- src/app/InteractionModelEngine.h | 25 +--- src/app/TimedHandler.cpp | 10 +- src/app/TimedHandler.h | 68 +++++++--- src/lib/support/BUILD.gn | 4 + src/lib/support/static_support_smart_ptr.h | 104 ++++++++++++++ src/lib/support/tests/BUILD.gn | 2 + .../tests/TestStaticSupportSmartPtr.cpp | 127 ++++++++++++++++++ 11 files changed, 385 insertions(+), 41 deletions(-) create mode 100644 src/app/InteractionModelDelegatePointers.cpp create mode 100644 src/app/InteractionModelDelegatePointers.h create mode 100644 src/lib/support/static_support_smart_ptr.h create mode 100644 src/lib/support/tests/TestStaticSupportSmartPtr.cpp diff --git a/src/app/BUILD.gn b/src/app/BUILD.gn index 84540e97892e6a..73a0433f73b262 100644 --- a/src/app/BUILD.gn +++ b/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", diff --git a/src/app/InteractionModelDelegatePointers.cpp b/src/app/InteractionModelDelegatePointers.cpp new file mode 100644 index 00000000000000..af4c85ee1bb1c2 --- /dev/null +++ b/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::InstancePointer() +{ + return app::InteractionModelEngine::GetInstance(); +} + +} // namespace chip + +#endif diff --git a/src/app/InteractionModelDelegatePointers.h b/src/app/InteractionModelDelegatePointers.h new file mode 100644 index 00000000000000..6677022b5561cb --- /dev/null +++ b/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 +#include + +namespace chip { + +#if CHIP_CONFIG_STATIC_GLOBAL_INTERACTION_MODEL_ENGINE + +template +using InteractionModelDelegatePointer = chip::CheckedGlobalInstanceReference; + +#else + +template +using InteractionModelDelegatePointer = chip::SimpleInstanceReference; + +#endif // CHIP_CONFIG_STATIC_GLOBAL_INTERATION_MODEL_ENGINE + +} // namespace chip diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 98cb6ff156a653..565cddd2381b71 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/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"); diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index 8ed201a0d934e6..b222087867380f 100644 --- a/src/app/InteractionModelEngine.h +++ b/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 /** diff --git a/src/app/TimedHandler.cpp b/src/app/TimedHandler.cpp index d448784721ac59..8df0246ba26728 100644 --- a/src/app/TimedHandler.cpp +++ b/src/app/TimedHandler.cpp @@ -17,7 +17,7 @@ */ #include "TimedHandler.h" -#include +#include #include #include #include @@ -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, diff --git a/src/app/TimedHandler.h b/src/app/TimedHandler.h index 47faa9d5940946..e47bfb7b49e1e6 100644 --- a/src/app/TimedHandler.h +++ b/src/app/TimedHandler.h @@ -15,15 +15,9 @@ * 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 #include #include #include @@ -31,6 +25,11 @@ #include #include +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 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; }; diff --git a/src/lib/support/BUILD.gn b/src/lib/support/BUILD.gn index e1e87aeb2b2192..2342b26ae66d70 100644 --- a/src/lib/support/BUILD.gn +++ b/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" diff --git a/src/lib/support/static_support_smart_ptr.h b/src/lib/support/static_support_smart_ptr.h new file mode 100644 index 00000000000000..896073a9b15801 --- /dev/null +++ b/src/lib/support/static_support_smart_ptr.h @@ -0,0 +1,104 @@ +/* + * + * 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 + +namespace chip { + +/// A template that is able to provide the global instance +/// for some application-specific class +/// +/// It works specifically together with CheckedGlobalInstanceReference +template +struct GlobalInstanceProvider +{ + static T * InstancePointer(); +}; + +/// A class that looks like a smart pointer (overrides operator->) +/// +/// However internally it only checks that the provided value is a given +/// Global instance. +/// +/// The global instance should be provided via GlobalInstanceProvider, for +/// example +/// +/// namespace chip { +/// template<> +/// Foo *GlobalInstanceProvider::InstancePointer() { +/// return Foo::Instance(); +/// } +/// } // namespace chip +/// +/// The CheckedGlobalInstanceReference will have minimal size (1 byte because +/// comparing instance pointers has to work) and does not require alignment, +/// as opposed to sizeof(void*) usage for SimpleInstanceReferences +/// +template +class CheckedGlobalInstanceReference +{ +public: + CheckedGlobalInstanceReference(T * e) { VerifyOrDie(e == GlobalInstanceProvider::InstancePointer()); } + CheckedGlobalInstanceReference & operator=(T * value) + { + VerifyOrDie(value == GlobalInstanceProvider::InstancePointer()); + return *this; + } + + inline T * operator->() { return GlobalInstanceProvider::InstancePointer(); } + inline const T * operator->() const { return GlobalInstanceProvider::InstancePointer(); } +}; + +/// A class that acts as a wrapper to a pointer and provides +/// operator-> overrides. +/// +/// It provides the same interface as CheckedGlobalInstanceReference +/// however it does NOT use a global value. +/// +/// The intended usage of these pair of classes is to compile-time decide +/// if global variables are to be used or if fully dynamic pointers are +/// allowed. +/// +/// Example: +/// #if USE_GLOBALS +/// template +/// using PointerContainer = chip::CheckedGlobalInstanceReference; +/// #else +/// template +/// using PointerContainer = chip::SimpleInstanceReference; +/// #endif +template +class SimpleInstanceReference +{ +public: + SimpleInstanceReference(T * e) : mValue(e) {} + SimpleInstanceReference & operator=(T * value) + { + mValue = value; + return *this; + } + + T * operator->() { return mValue; } + const T * operator->() const { return mValue; } + +private: + T * mValue; +}; + +} // namespace chip diff --git a/src/lib/support/tests/BUILD.gn b/src/lib/support/tests/BUILD.gn index d0b91feeba7061..deac3e2cbb1342 100644 --- a/src/lib/support/tests/BUILD.gn +++ b/src/lib/support/tests/BUILD.gn @@ -49,6 +49,7 @@ chip_test_suite_using_nltest("tests") { "TestSerializableIntegerSet.cpp", "TestSpan.cpp", "TestStateMachine.cpp", + "TestStaticSupportSmartPtr.cpp", "TestStringBuilder.cpp", "TestStringSplitter.cpp", "TestTestPersistentStorageDelegate.cpp", @@ -79,6 +80,7 @@ chip_test_suite_using_nltest("tests") { public_deps = [ "${chip_root}/src/credentials", "${chip_root}/src/lib/core", + "${chip_root}/src/lib/support:static-support", "${chip_root}/src/lib/support:testing", "${chip_root}/src/lib/support:testing_nlunit", "${chip_root}/src/lib/support/jsontlv", diff --git a/src/lib/support/tests/TestStaticSupportSmartPtr.cpp b/src/lib/support/tests/TestStaticSupportSmartPtr.cpp new file mode 100644 index 00000000000000..809c2e4887969c --- /dev/null +++ b/src/lib/support/tests/TestStaticSupportSmartPtr.cpp @@ -0,0 +1,127 @@ +/* + * + * 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 + +#include +#include + +#include + +#include + +using namespace chip; +namespace { + +struct TestClass +{ + const char * str; + int num; + + TestClass(const char * s, int n) : str(s), num(n) {} +}; + +TestClass gTestClass("abc", 123); +} // namespace + +namespace chip { + +template <> +TestClass * GlobalInstanceProvider::InstancePointer() +{ + return &gTestClass; +} + +} // namespace chip + +namespace { + +void TestCheckedGlobalInstanceReference(nlTestSuite * inSuite, void * inContext) +{ + CheckedGlobalInstanceReference ref(&gTestClass); + + // We expect that sizes of global references is minimal + NL_TEST_ASSERT(inSuite, sizeof(ref) == 1); + + NL_TEST_ASSERT(inSuite, ref->num == 123); + NL_TEST_ASSERT(inSuite, strcmp(ref->str, "abc") == 0); + + { + ScopedChange change1(gTestClass.num, 100); + ScopedChange change2(ref->str, "xyz"); + + NL_TEST_ASSERT(inSuite, ref->num == 100); + NL_TEST_ASSERT(inSuite, strcmp(gTestClass.str, "xyz") == 0); + } + + CheckedGlobalInstanceReference ref2(&gTestClass); + + NL_TEST_ASSERT(inSuite, ref->num == ref2->num); + NL_TEST_ASSERT(inSuite, strcmp(ref->str, ref2->str) == 0); + + { + ScopedChange change1(gTestClass.num, 321); + ScopedChange change2(ref->str, "test"); + + NL_TEST_ASSERT(inSuite, ref->num == ref2->num); + NL_TEST_ASSERT(inSuite, strcmp(ref->str, ref2->str) == 0); + + NL_TEST_ASSERT(inSuite, ref2->num == 321); + NL_TEST_ASSERT(inSuite, strcmp(ref2->str, "test") == 0); + } +} + +void TestSimpleInstanceReference(nlTestSuite * inSuite, void * inContext) +{ + TestClass a("abc", 123); + TestClass b("xyz", 100); + + SimpleInstanceReference ref_a(&a); + SimpleInstanceReference ref_b(&b); + + // overhead of simple references should be a simple pointer + NL_TEST_ASSERT(inSuite, sizeof(ref_a) <= sizeof(void *)); + + NL_TEST_ASSERT(inSuite, ref_a->num == 123); + NL_TEST_ASSERT(inSuite, ref_b->num == 100); + + ref_a->num = 99; + b.num = 30; + + NL_TEST_ASSERT(inSuite, a.num == 99); + NL_TEST_ASSERT(inSuite, ref_b->num == 30); +} + +#define NL_TEST_DEF_FN(fn) NL_TEST_DEF("Test " #fn, fn) +const nlTest sTests[] = { + NL_TEST_DEF_FN(TestCheckedGlobalInstanceReference), + NL_TEST_DEF_FN(TestSimpleInstanceReference), + NL_TEST_SENTINEL(), +}; + +} // namespace + +int TestStaticSupportSmartPtr() +{ + nlTestSuite theSuite = { "CHIP Static support smart pointers", &sTests[0], nullptr, nullptr }; + + // Run test suite against one context. + nlTestRunner(&theSuite, nullptr); + return nlTestRunnerStats(&theSuite); +} + +CHIP_REGISTER_TEST_SUITE(TestStaticSupportSmartPtr)