Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decouple InteractionModelEngine from TimedHandler #32076

Merged
merged 10 commits into from
Feb 15, 2024
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.
andy31415 marked this conversation as resolved.
Show resolved Hide resolved
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
Loading