Skip to content

Commit

Permalink
Decouple InteractionModelEngine from TimedHandler (#32076)
Browse files Browse the repository at this point in the history
* 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 <[email protected]>
  • Loading branch information
andy31415 and andreilitvin authored Feb 15, 2024
1 parent 274719d commit 8eaca94
Show file tree
Hide file tree
Showing 11 changed files with 385 additions and 41 deletions.
11 changes: 11 additions & 0 deletions src/app/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
Expand All @@ -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" ]
Expand Down Expand Up @@ -129,6 +137,8 @@ static_library("interaction-model") {
"CASESessionManager.h",
"DeviceProxy.cpp",
"DeviceProxy.h",
"InteractionModelDelegatePointers.cpp",
"InteractionModelDelegatePointers.h",
"InteractionModelEngine.cpp",
"InteractionModelEngine.h",
"InteractionModelTimeout.h",
Expand Down Expand Up @@ -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",
Expand Down
36 changes: 36 additions & 0 deletions src/app/InteractionModelDelegatePointers.cpp
Original file line number Diff line number Diff line change
@@ -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
37 changes: 37 additions & 0 deletions src/app/InteractionModelDelegatePointers.h
Original file line number Diff line number Diff line change
@@ -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
2 changes: 1 addition & 1 deletion src/app/InteractionModelEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
25 changes: 6 additions & 19 deletions src/app/InteractionModelEngine.h
Original file line number Diff line number Diff line change
Expand Up @@ -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:
/**
Expand Down Expand Up @@ -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
/**
Expand Down
10 changes: 4 additions & 6 deletions src/app/TimedHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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;
}
}
Expand All @@ -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,
Expand Down
68 changes: 53 additions & 15 deletions src/app/TimedHandler.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
Expand Down Expand Up @@ -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;
};

Expand Down
4 changes: 4 additions & 0 deletions src/lib/support/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down
Loading

0 comments on commit 8eaca94

Please sign in to comment.