From 1f2aff46470a9dcb9c37fab3be6ade62fe083051 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 21 Mar 2024 15:49:11 -0400 Subject: [PATCH] Start moving matter pre/post callbacks from `__weak__` handling to a injectable class instead. (#32648) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add MatterCallbacks.cpp to include callbacks and redirection * Fix compile dependencies * Replace a few more callbacks * Fix lint file - mattercallbacks is now tracked * Restyle * Update ApplicationCallbacks to DataModelCallbacks * Update src/app/util/MatterCallbacks.cpp Co-authored-by: Arkadiusz Bokowy * Update src/app/util/MatterCallbacks.h Co-authored-by: Damian Królik <66667989+Damian-Nordic@users.noreply.github.com> * Updated comments --------- Co-authored-by: Arkadiusz Bokowy Co-authored-by: Damian Królik <66667989+Damian-Nordic@users.noreply.github.com> --- .github/workflows/lint.yml | 1 - src/app/BUILD.gn | 2 + src/app/CommandHandler.cpp | 17 ++---- src/app/WriteHandler.cpp | 17 +++--- src/app/reporting/Engine.cpp | 13 +++-- src/app/util/BUILD.gn | 12 ++++ src/app/util/MatterCallbacks.cpp | 99 ++++++++++++++++++++++++++++++++ src/app/util/MatterCallbacks.h | 72 ++++++++++++++++------- 8 files changed, 185 insertions(+), 48 deletions(-) create mode 100644 src/app/util/MatterCallbacks.cpp diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 59c0d4e59a7c9b..38c1dac8188b02 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -117,7 +117,6 @@ jobs: --known-failure app/util/generic-callbacks.h \ --known-failure app/util/generic-callback-stubs.cpp \ --known-failure app/util/im-client-callbacks.h \ - --known-failure app/util/MatterCallbacks.h \ --known-failure app/util/util.cpp \ --known-failure app/util/util.h \ --known-failure app/WriteHandler.h \ diff --git a/src/app/BUILD.gn b/src/app/BUILD.gn index 2641356da45308..4cf96b3a1a84c8 100644 --- a/src/app/BUILD.gn +++ b/src/app/BUILD.gn @@ -210,6 +210,7 @@ static_library("interaction-model") { "${chip_root}/src/app/icd/server:icd-server-config", "${chip_root}/src/app/icd/server:observer", "${chip_root}/src/app/util:af-types", + "${chip_root}/src/app/util:callbacks", "${chip_root}/src/lib/address_resolve", "${chip_root}/src/lib/support", "${chip_root}/src/lib/support:static-support", @@ -318,6 +319,7 @@ static_library("app") { ":interaction-model", "${chip_root}/src/app/data-model", "${chip_root}/src/app/icd/server:icd-server-config", + "${chip_root}/src/app/util:callbacks", "${chip_root}/src/lib/address_resolve", "${chip_root}/src/lib/support", "${chip_root}/src/messaging", diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 8170b57abd7d23..f80f41e9a63c61 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -449,9 +449,9 @@ Status CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommandElem { ChipLogDetail(DataManagement, "Received command for Endpoint=%u Cluster=" ChipLogFormatMEI " Command=" ChipLogFormatMEI, concretePath.mEndpointId, ChipLogValueMEI(concretePath.mClusterId), ChipLogValueMEI(concretePath.mCommandId)); - SuccessOrExit(err = MatterPreCommandReceivedCallback(concretePath, GetSubjectDescriptor())); + SuccessOrExit(err = DataModelCallbacks::GetInstance()->PreCommandReceived(concretePath, GetSubjectDescriptor())); mpCallback->DispatchCommand(*this, concretePath, commandDataReader); - MatterPostCommandReceivedCallback(concretePath, GetSubjectDescriptor()); + DataModelCallbacks::GetInstance()->PostCommandReceived(concretePath, GetSubjectDescriptor()); } exit: @@ -555,11 +555,11 @@ Status CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aComman continue; } } - if ((err = MatterPreCommandReceivedCallback(concretePath, GetSubjectDescriptor())) == CHIP_NO_ERROR) + if ((err = DataModelCallbacks::GetInstance()->PreCommandReceived(concretePath, GetSubjectDescriptor())) == CHIP_NO_ERROR) { TLV::TLVReader dataReader(commandDataReader); mpCallback->DispatchCommand(*this, concretePath, dataReader); - MatterPostCommandReceivedCallback(concretePath, GetSubjectDescriptor()); + DataModelCallbacks::GetInstance()->PostCommandReceived(concretePath, GetSubjectDescriptor()); } else { @@ -1053,12 +1053,3 @@ void CommandHandler::TestOnlyInvokeCommandRequestWithFaultsInjected(Messaging::E } // namespace app } // namespace chip - -CHIP_ERROR __attribute__((weak)) MatterPreCommandReceivedCallback(const chip::app::ConcreteCommandPath & commandPath, - const chip::Access::SubjectDescriptor & subjectDescriptor) -{ - return CHIP_NO_ERROR; -} -void __attribute__((weak)) MatterPostCommandReceivedCallback(const chip::app::ConcreteCommandPath & commandPath, - const chip::Access::SubjectDescriptor & subjectDescriptor) -{} diff --git a/src/app/WriteHandler.cpp b/src/app/WriteHandler.cpp index f7fc1f3273d7c8..911c2f594095cd 100644 --- a/src/app/WriteHandler.cpp +++ b/src/app/WriteHandler.cpp @@ -328,7 +328,9 @@ CHIP_ERROR WriteHandler::ProcessAttributeDataIBs(TLV::TLVReader & aAttributeData mProcessingAttributeIsList = dataAttributePath.IsListOperation(); mProcessingAttributePath.SetValue(dataAttributePath); - MatterPreAttributeWriteCallback(dataAttributePath); + DataModelCallbacks::GetInstance()->AttributeOperation(DataModelCallbacks::OperationType::Write, + DataModelCallbacks::OperationOrder::Pre, dataAttributePath); + TLV::TLVWriter backup; DataVersion version = 0; mWriteResponseBuilder.GetWriteResponses().Checkpoint(backup); @@ -348,7 +350,9 @@ CHIP_ERROR WriteHandler::ProcessAttributeDataIBs(TLV::TLVReader & aAttributeData mWriteResponseBuilder.GetWriteResponses().Rollback(backup); err = AddStatus(dataAttributePath, StatusIB(err)); } - MatterPostAttributeWriteCallback(dataAttributePath); + + DataModelCallbacks::GetInstance()->AttributeOperation(DataModelCallbacks::OperationType::Write, + DataModelCallbacks::OperationOrder::Post, dataAttributePath); SuccessOrExit(err); } @@ -482,7 +486,8 @@ CHIP_ERROR WriteHandler::ProcessGroupAttributeDataIBs(TLV::TLVReader & aAttribut chip::TLV::TLVReader tmpDataReader(dataReader); - MatterPreAttributeWriteCallback(dataAttributePath); + DataModelCallbacks::GetInstance()->AttributeOperation(DataModelCallbacks::OperationType::Write, + DataModelCallbacks::OperationOrder::Pre, dataAttributePath); err = WriteSingleClusterData(subjectDescriptor, dataAttributePath, tmpDataReader, this); if (err != CHIP_NO_ERROR) @@ -493,7 +498,8 @@ CHIP_ERROR WriteHandler::ProcessGroupAttributeDataIBs(TLV::TLVReader & aAttribut mapping.endpoint_id, ChipLogValueMEI(dataAttributePath.mClusterId), ChipLogValueMEI(dataAttributePath.mAttributeId), err.Format()); } - MatterPostAttributeWriteCallback(dataAttributePath); + DataModelCallbacks::GetInstance()->AttributeOperation(DataModelCallbacks::OperationType::Write, + DataModelCallbacks::OperationOrder::Post, dataAttributePath); } dataAttributePath.mEndpointId = kInvalidEndpointId; @@ -677,6 +683,3 @@ void WriteHandler::MoveToState(const State aTargetState) } // namespace app } // namespace chip - -void __attribute__((weak)) MatterPreAttributeWriteCallback(const chip::app::ConcreteAttributePath & attributePath) {} -void __attribute__((weak)) MatterPostAttributeWriteCallback(const chip::app::ConcreteAttributePath & attributePath) {} diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp index e21080839ed248..91e1e1f52891ec 100644 --- a/src/app/reporting/Engine.cpp +++ b/src/app/reporting/Engine.cpp @@ -86,9 +86,15 @@ Engine::RetrieveClusterData(const SubjectDescriptor & aSubjectDescriptor, bool a { ChipLogDetail(DataManagement, " Cluster %" PRIx32 ", Attribute %" PRIx32 " is dirty", aPath.mClusterId, aPath.mAttributeId); - MatterPreAttributeReadCallback(aPath); + + DataModelCallbacks::GetInstance()->AttributeOperation(DataModelCallbacks::OperationType::Read, + DataModelCallbacks::OperationOrder::Pre, aPath); + ReturnErrorOnFailure(ReadSingleClusterData(aSubjectDescriptor, aIsFabricFiltered, aPath, aAttributeReportIBs, aEncoderState)); - MatterPostAttributeReadCallback(aPath); + + DataModelCallbacks::GetInstance()->AttributeOperation(DataModelCallbacks::OperationType::Read, + DataModelCallbacks::OperationOrder::Post, aPath); + return CHIP_NO_ERROR; } @@ -996,9 +1002,6 @@ void Engine::ScheduleUrgentEventDeliverySync(Optional fabricIndex) } // namespace app } // namespace chip -void __attribute__((weak)) MatterPreAttributeReadCallback(const chip::app::ConcreteAttributePath & attributePath) {} -void __attribute__((weak)) MatterPostAttributeReadCallback(const chip::app::ConcreteAttributePath & attributePath) {} - // TODO: MatterReportingAttributeChangeCallback should just live in libCHIP, // instead of being in ember-compatibility-functions. It does not depend on any // app-specific generated bits. diff --git a/src/app/util/BUILD.gn b/src/app/util/BUILD.gn index dfb2e4c248dc92..dccbc68c0d943e 100644 --- a/src/app/util/BUILD.gn +++ b/src/app/util/BUILD.gn @@ -50,3 +50,15 @@ source_set("af-types") { "${chip_root}/src/protocols/interaction_model", ] } + +source_set("callbacks") { + sources = [ + "MatterCallbacks.cpp", + "MatterCallbacks.h", + ] + + deps = [ + "${chip_root}/src/access", + "${chip_root}/src/app:paths", + ] +} diff --git a/src/app/util/MatterCallbacks.cpp b/src/app/util/MatterCallbacks.cpp new file mode 100644 index 00000000000000..71ab7fdc9df65d --- /dev/null +++ b/src/app/util/MatterCallbacks.cpp @@ -0,0 +1,99 @@ +/* + * Copyright (c) 2024 Project CHIP Authors + * + * 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 "MatterCallbacks.h" + +// The defines below are using link-time callback and should be removed +// +// TODO: applications should be converted to use DataModelCallbacks instead +// of relying on weak linkage +void __attribute__((weak)) MatterPreAttributeReadCallback(const chip::app::ConcreteAttributePath & attributePath) {} +void __attribute__((weak)) MatterPostAttributeReadCallback(const chip::app::ConcreteAttributePath & attributePath) {} +void __attribute__((weak)) MatterPreAttributeWriteCallback(const chip::app::ConcreteAttributePath & attributePath) {} +void __attribute__((weak)) MatterPostAttributeWriteCallback(const chip::app::ConcreteAttributePath & attributePath) {} +CHIP_ERROR __attribute__((weak)) MatterPreCommandReceivedCallback(const chip::app::ConcreteCommandPath & commandPath, + const chip::Access::SubjectDescriptor & subjectDescriptor) +{ + return CHIP_NO_ERROR; +} +void __attribute__((weak)) MatterPostCommandReceivedCallback(const chip::app::ConcreteCommandPath & commandPath, + const chip::Access::SubjectDescriptor & subjectDescriptor) +{} + +namespace chip { +namespace { + +class WeakRedirectCallbacks : public DataModelCallbacks +{ +public: + void AttributeOperation(OperationType operation, OperationOrder order, const chip::app::ConcreteAttributePath & path) override + { + switch (operation) + { + case OperationType::Read: + switch (order) + { + case OperationOrder::Pre: + MatterPreAttributeReadCallback(path); + break; + case OperationOrder::Post: + MatterPostAttributeReadCallback(path); + break; + } + break; + case OperationType::Write: + switch (order) + { + case OperationOrder::Pre: + MatterPreAttributeWriteCallback(path); + break; + case OperationOrder::Post: + MatterPostAttributeWriteCallback(path); + break; + } + break; + } + } + + CHIP_ERROR PreCommandReceived(const chip::app::ConcreteCommandPath & commandPath, + const chip::Access::SubjectDescriptor & subjectDescriptor) override + { + return MatterPreCommandReceivedCallback(commandPath, subjectDescriptor); + } + + void PostCommandReceived(const chip::app::ConcreteCommandPath & commandPath, + const chip::Access::SubjectDescriptor & subjectDescriptor) override + { + + MatterPostCommandReceivedCallback(commandPath, subjectDescriptor); + } +}; + +WeakRedirectCallbacks gWeakCallbacks; +DataModelCallbacks * gInstance = &gWeakCallbacks; + +} // namespace + +DataModelCallbacks * DataModelCallbacks::GetInstance() +{ + return gInstance; +} + +DataModelCallbacks * DataModelCallbacks::SetInstance(DataModelCallbacks * newInstance) +{ + return std::exchange(gInstance, newInstance); +} + +} // namespace chip diff --git a/src/app/util/MatterCallbacks.h b/src/app/util/MatterCallbacks.h index 6a0b69388d67a5..05d1653da84e1f 100644 --- a/src/app/util/MatterCallbacks.h +++ b/src/app/util/MatterCallbacks.h @@ -1,5 +1,4 @@ /* - * * Copyright (c) 2021 Project CHIP Authors * * Licensed under the Apache License, Version 2.0 (the "License"); @@ -14,32 +13,61 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - -// THIS FILE IS GENERATED BY ZAP - #pragma once #include #include #include -void MatterPreAttributeReadCallback(const chip::app::ConcreteAttributePath & attributePath); -void MatterPostAttributeReadCallback(const chip::app::ConcreteAttributePath & attributePath); -void MatterPreAttributeWriteCallback(const chip::app::ConcreteAttributePath & attributePath); -void MatterPostAttributeWriteCallback(const chip::app::ConcreteAttributePath & attributePath); +namespace chip { -/** @brief Matter Pre Command Received - * - * This callback is called once the message has been determined to be a command, and - * before the command is dispatched to the receiver. - */ -CHIP_ERROR MatterPreCommandReceivedCallback(const chip::app::ConcreteCommandPath & commandPath, - const chip::Access::SubjectDescriptor & subjectDescriptor); +/// Allows for application hooks for processing attribute and command operations +class DataModelCallbacks +{ +public: + enum class OperationType + { + Read, + Write + }; -/** @brief Matter Post Command Received - * - * This callback is called once the message has been determined to be a command, but - * after it being dispatched to the receiver. - */ -void MatterPostCommandReceivedCallback(const chip::app::ConcreteCommandPath & commandPath, - const chip::Access::SubjectDescriptor & subjectDescriptor); + enum class OperationOrder + { + Pre, + Post + }; + + virtual ~DataModelCallbacks() = default; + + /// This callback is called on attribute operations: + /// - for reads and writes + /// - both before and after attribute read/writes + /// + /// NOTE: PostRead is only called on read success. + virtual void AttributeOperation(OperationType operation, OperationOrder order, const chip::app::ConcreteAttributePath & path) {} + + /// This callback is called once for every command dispatch, before the dispatch is actually + /// done towards the receiver. + /// + /// This method is called once for every CommandDataIB (i.e. it may be called several times + /// in the case of batch invoke, where a single `InvokeRequestMessage` may contain several + /// CommandDataIB entries). + /// + /// Returning an error here will prevent the command to be dispatched further + virtual CHIP_ERROR PreCommandReceived(const chip::app::ConcreteCommandPath & commandPath, + const chip::Access::SubjectDescriptor & subjectDescriptor) + { + return CHIP_NO_ERROR; + } + + /// This callback is called once the message has been determined to be a command, but + /// after it being dispatched to the receiver. + virtual void PostCommandReceived(const chip::app::ConcreteCommandPath & commandPath, + const chip::Access::SubjectDescriptor & subjectDescriptor) + {} + + static DataModelCallbacks * GetInstance(); + static DataModelCallbacks * SetInstance(DataModelCallbacks * newInstance); +}; + +} // namespace chip