From 59eb04e41d6193617073bb5b86d068ecc289be04 Mon Sep 17 00:00:00 2001
From: Andrei Litvin <andy314@gmail.com>
Date: Wed, 14 Aug 2024 14:30:11 -0400
Subject: [PATCH 1/3] Pull RAM usage reduction from write impl
---
src/app/WriteHandler.cpp | 70 ++++++++++++++++++++++++----------------
src/app/WriteHandler.h | 50 +++++++++++++++++-----------
2 files changed, 75 insertions(+), 45 deletions(-)
diff --git a/src/app/WriteHandler.cpp b/src/app/WriteHandler.cpp
index 8d4a82d11630c4..2571385908df9a 100644
--- a/src/app/WriteHandler.cpp
+++ b/src/app/WriteHandler.cpp
@@ -16,19 +16,23 @@
* limitations under the License.
*/
-#include "messaging/ExchangeContext.h"
#include <app/AppConfig.h>
#include <app/AttributeAccessInterfaceRegistry.h>
+#include <app/AttributeValueDecoder.h>
#include <app/InteractionModelEngine.h>
#include <app/MessageDef/EventPathIB.h>
#include <app/MessageDef/StatusIB.h>
#include <app/StatusResponse.h>
#include <app/WriteHandler.h>
+#include <app/data-model-provider/OperationTypes.h>
#include <app/reporting/Engine.h>
#include <app/util/MatterCallbacks.h>
#include <app/util/ember-compatibility-functions.h>
#include <credentials/GroupDataProvider.h>
+#include <lib/core/CHIPError.h>
+#include <lib/support/CodeUtils.h>
#include <lib/support/TypeTraits.h>
+#include <messaging/ExchangeContext.h>
#include <protocols/interaction_model/StatusCode.h>
namespace chip {
@@ -62,7 +66,7 @@ void WriteHandler::Close()
// successful.
DeliverFinalListWriteEnd(false /* wasSuccessful */);
mExchangeCtx.Release();
- mSuppressResponse = false;
+ mStateFlags.Clear(StateBits::kSuppressResponse);
MoveToState(State::Uninitialized);
}
@@ -106,7 +110,7 @@ Status WriteHandler::OnWriteRequest(Messaging::ExchangeContext * apExchangeConte
Status status = HandleWriteRequestMessage(apExchangeContext, std::move(aPayload), aIsTimedWrite);
// The write transaction will be alive only when the message was handled successfully and there are more chunks.
- if (!(status == Status::Success && mHasMoreChunks))
+ if (!(status == Status::Success && mStateFlags.Has(StateBits::kHasMoreChunks)))
{
Close();
}
@@ -142,7 +146,7 @@ CHIP_ERROR WriteHandler::OnMessageReceived(Messaging::ExchangeContext * apExchan
if (status == Status::Success)
{
// We have no more chunks, the write response has been sent in HandleWriteRequestMessage, so close directly.
- if (!mHasMoreChunks)
+ if (!mStateFlags.Has(StateBits::kHasMoreChunks))
{
Close();
}
@@ -184,8 +188,8 @@ CHIP_ERROR WriteHandler::SendWriteResponse(System::PacketBufferTLVWriter && aMes
VerifyOrExit(mExchangeCtx, err = CHIP_ERROR_INCORRECT_STATE);
mExchangeCtx->UseSuggestedResponseTimeout(app::kExpectedIMProcessingTime);
err = mExchangeCtx->SendMessage(Protocols::InteractionModel::MsgType::WriteResponse, std::move(packet),
- mHasMoreChunks ? Messaging::SendMessageFlags::kExpectResponse
- : Messaging::SendMessageFlags::kNone);
+ mStateFlags.Has(StateBits::kHasMoreChunks) ? Messaging::SendMessageFlags::kExpectResponse
+ : Messaging::SendMessageFlags::kNone);
SuccessOrExit(err);
MoveToState(State::Sending);
@@ -212,7 +216,7 @@ void WriteHandler::DeliverListWriteEnd(const ConcreteAttributePath & aPath, bool
void WriteHandler::DeliverFinalListWriteEnd(bool writeWasSuccessful)
{
- if (mProcessingAttributePath.HasValue() && mProcessingAttributeIsList)
+ if (mProcessingAttributePath.HasValue() && mStateFlags.Has(StateBits::kProcessingAttributeIsList))
{
DeliverListWriteEnd(mProcessingAttributePath.Value(), writeWasSuccessful);
}
@@ -221,7 +225,8 @@ void WriteHandler::DeliverFinalListWriteEnd(bool writeWasSuccessful)
CHIP_ERROR WriteHandler::DeliverFinalListWriteEndForGroupWrite(bool writeWasSuccessful)
{
- VerifyOrReturnError(mProcessingAttributePath.HasValue() && mProcessingAttributeIsList, CHIP_NO_ERROR);
+ VerifyOrReturnError(mProcessingAttributePath.HasValue() && mStateFlags.Has(StateBits::kProcessingAttributeIsList),
+ CHIP_NO_ERROR);
Credentials::GroupDataProvider::GroupEndpoint mapping;
Credentials::GroupDataProvider * groupDataProvider = Credentials::GetGroupDataProvider();
@@ -321,18 +326,20 @@ CHIP_ERROR WriteHandler::ProcessAttributeDataIBs(TLV::TLVReader & aAttributeData
continue;
}
- if (ShouldReportListWriteEnd(mProcessingAttributePath, mProcessingAttributeIsList, dataAttributePath))
+ if (ShouldReportListWriteEnd(mProcessingAttributePath, mStateFlags.Has(StateBits::kProcessingAttributeIsList),
+ dataAttributePath))
{
- DeliverListWriteEnd(mProcessingAttributePath.Value(), mAttributeWriteSuccessful);
+ DeliverListWriteEnd(mProcessingAttributePath.Value(), mStateFlags.Has(StateBits::kAttributeWriteSuccessful));
}
- if (ShouldReportListWriteBegin(mProcessingAttributePath, mProcessingAttributeIsList, dataAttributePath))
+ if (ShouldReportListWriteBegin(mProcessingAttributePath, mStateFlags.Has(StateBits::kProcessingAttributeIsList),
+ dataAttributePath))
{
DeliverListWriteBegin(dataAttributePath);
- mAttributeWriteSuccessful = true;
+ mStateFlags.Set(StateBits::kAttributeWriteSuccessful);
}
- mProcessingAttributeIsList = dataAttributePath.IsListOperation();
+ mStateFlags.Set(StateBits::kProcessingAttributeIsList, dataAttributePath.IsListOperation());
mProcessingAttributePath.SetValue(dataAttributePath);
DataModelCallbacks::GetInstance()->AttributeOperation(DataModelCallbacks::OperationType::Write,
@@ -370,9 +377,9 @@ CHIP_ERROR WriteHandler::ProcessAttributeDataIBs(TLV::TLVReader & aAttributeData
SuccessOrExit(err);
- if (!mHasMoreChunks)
+ if (!mStateFlags.Has(StateBits::kHasMoreChunks))
{
- DeliverFinalListWriteEnd(mAttributeWriteSuccessful);
+ DeliverFinalListWriteEnd(mStateFlags.Has(StateBits::kAttributeWriteSuccessful));
}
exit:
@@ -426,8 +433,8 @@ CHIP_ERROR WriteHandler::ProcessGroupAttributeDataIBs(TLV::TLVReader & aAttribut
iterator = groupDataProvider->IterateEndpoints(fabric);
VerifyOrExit(iterator != nullptr, err = CHIP_ERROR_NO_MEMORY);
- bool shouldReportListWriteEnd =
- ShouldReportListWriteEnd(mProcessingAttributePath, mProcessingAttributeIsList, dataAttributePath);
+ bool shouldReportListWriteEnd = ShouldReportListWriteEnd(
+ mProcessingAttributePath, mStateFlags.Has(StateBits::kProcessingAttributeIsList), dataAttributePath);
bool shouldReportListWriteBegin = false; // This will be set below.
const EmberAfAttributeMetadata * attributeMetadata = nullptr;
@@ -457,7 +464,8 @@ CHIP_ERROR WriteHandler::ProcessGroupAttributeDataIBs(TLV::TLVReader & aAttribut
dataAttributePath.mEndpointId, dataAttributePath.mListOp,
dataAttributePath.mListIndex);
shouldReportListWriteBegin =
- ShouldReportListWriteBegin(mProcessingAttributePath, mProcessingAttributeIsList, pathForCheckingListWriteBegin);
+ ShouldReportListWriteBegin(mProcessingAttributePath, mStateFlags.Has(StateBits::kProcessingAttributeIsList),
+ pathForCheckingListWriteBegin);
}
if (shouldReportListWriteEnd)
@@ -498,11 +506,10 @@ CHIP_ERROR WriteHandler::ProcessGroupAttributeDataIBs(TLV::TLVReader & aAttribut
DataModelCallbacks::GetInstance()->AttributeOperation(DataModelCallbacks::OperationType::Write,
DataModelCallbacks::OperationOrder::Pre, dataAttributePath);
err = WriteSingleClusterData(subjectDescriptor, dataAttributePath, tmpDataReader, this);
-
if (err != CHIP_NO_ERROR)
{
ChipLogError(DataManagement,
- "WriteSingleClusterData Endpoint=%u Cluster=" ChipLogFormatMEI " Attribute =" ChipLogFormatMEI
+ "WriteClusterData Endpoint=%u Cluster=" ChipLogFormatMEI " Attribute =" ChipLogFormatMEI
" failed: %" CHIP_ERROR_FORMAT,
mapping.endpoint_id, ChipLogValueMEI(dataAttributePath.mClusterId),
ChipLogValueMEI(dataAttributePath.mAttributeId), err.Format());
@@ -512,7 +519,7 @@ CHIP_ERROR WriteHandler::ProcessGroupAttributeDataIBs(TLV::TLVReader & aAttribut
}
dataAttributePath.mEndpointId = kInvalidEndpointId;
- mProcessingAttributeIsList = dataAttributePath.IsListOperation();
+ mStateFlags.Set(StateBits::kProcessingAttributeIsList, dataAttributePath.IsListOperation());
mProcessingAttributePath.SetValue(dataAttributePath);
iterator->Release();
}
@@ -557,24 +564,33 @@ Status WriteHandler::ProcessWriteRequest(System::PacketBufferHandle && aPayload,
#if CHIP_CONFIG_IM_PRETTY_PRINT
writeRequestParser.PrettyPrint();
#endif
- err = writeRequestParser.GetSuppressResponse(&mSuppressResponse);
+ bool boolValue;
+
+ boolValue = mStateFlags.Has(StateBits::kSuppressResponse);
+ err = writeRequestParser.GetSuppressResponse(&boolValue);
+ mStateFlags.Set(StateBits::kSuppressResponse, boolValue);
if (err == CHIP_END_OF_TLV)
{
err = CHIP_NO_ERROR;
}
SuccessOrExit(err);
- err = writeRequestParser.GetTimedRequest(&mIsTimedRequest);
+ boolValue = mStateFlags.Has(StateBits::kIsTimedRequest);
+ err = writeRequestParser.GetTimedRequest(&boolValue);
+ mStateFlags.Set(StateBits::kIsTimedRequest, boolValue);
SuccessOrExit(err);
- err = writeRequestParser.GetMoreChunkedMessages(&mHasMoreChunks);
+ boolValue = mStateFlags.Has(StateBits::kHasMoreChunks);
+ err = writeRequestParser.GetMoreChunkedMessages(&boolValue);
+ mStateFlags.Set(StateBits::kHasMoreChunks, boolValue);
if (err == CHIP_ERROR_END_OF_TLV)
{
err = CHIP_NO_ERROR;
}
SuccessOrExit(err);
- if (mHasMoreChunks && (mExchangeCtx->IsGroupExchangeContext() || mIsTimedRequest))
+ if (mStateFlags.Has(StateBits::kHasMoreChunks) &&
+ (mExchangeCtx->IsGroupExchangeContext() || mStateFlags.Has(StateBits::kIsTimedRequest)))
{
// Sanity check: group exchange context should only have one chunk.
// Also, timed requests should not have more than one chunk.
@@ -584,7 +600,7 @@ Status WriteHandler::ProcessWriteRequest(System::PacketBufferHandle && aPayload,
err = writeRequestParser.GetWriteRequests(&AttributeDataIBsParser);
SuccessOrExit(err);
- if (mIsTimedRequest != aIsTimedWrite)
+ if (mStateFlags.Has(StateBits::kIsTimedRequest) != aIsTimedWrite)
{
// The message thinks it should be part of a timed interaction but it's
// not, or vice versa.
@@ -641,7 +657,7 @@ CHIP_ERROR WriteHandler::AddStatusInternal(const ConcreteDataAttributePath & aPa
if (!aStatus.IsSuccess())
{
- mAttributeWriteSuccessful = false;
+ mStateFlags.Clear(StateBits::kAttributeWriteSuccessful);
}
ReturnErrorOnFailure(writeResponses.GetError());
diff --git a/src/app/WriteHandler.h b/src/app/WriteHandler.h
index 1884654f070710..0723a316738498 100644
--- a/src/app/WriteHandler.h
+++ b/src/app/WriteHandler.h
@@ -17,12 +17,15 @@
*/
#pragma once
+
+#include <app/AppConfig.h>
#include <app/AttributeAccessToken.h>
#include <app/AttributePathParams.h>
#include <app/InteractionModelDelegatePointers.h>
#include <app/MessageDef/WriteResponseMessage.h>
#include <lib/core/CHIPCore.h>
#include <lib/core/TLVDebug.h>
+#include <lib/support/BitFlags.h>
#include <lib/support/CodeUtils.h>
#include <lib/support/DLLUtil.h>
#include <lib/support/logging/CHIPLogging.h>
@@ -68,9 +71,11 @@ class WriteHandler : public Messaging::ExchangeDelegate
*
* @param[in] apWriteHandlerDelegate A Valid pointer to the WriteHandlerDelegate.
*
+ * @retval #CHIP_ERROR_INVALID_ARGUMENT on invalid pointers
* @retval #CHIP_ERROR_INCORRECT_STATE If the state is not equal to
* kState_NotInitialized.
* @retval #CHIP_NO_ERROR On success.
+ *
*/
CHIP_ERROR Init(WriteHandlerDelegate * apWriteHandlerDelegate);
@@ -115,7 +120,7 @@ class WriteHandler : public Messaging::ExchangeDelegate
/**
* Check whether the WriteRequest we are handling is a timed write.
*/
- bool IsTimedWrite() const { return mIsTimedRequest; }
+ bool IsTimedWrite() const { return mStateFlags.Has(StateBits::kIsTimedRequest); }
bool MatchesExchangeContext(Messaging::ExchangeContext * apExchangeContext) const
{
@@ -136,7 +141,7 @@ class WriteHandler : public Messaging::ExchangeDelegate
private:
friend class TestWriteInteraction;
- enum class State
+ enum class State : uint8_t
{
Uninitialized = 0, // The handler has not been initialized
Initialized, // The handler has been initialized and is ready
@@ -172,7 +177,6 @@ class WriteHandler : public Messaging::ExchangeDelegate
CHIP_ERROR AddStatusInternal(const ConcreteDataAttributePath & aPath, const StatusIB & aStatus);
-private:
// ExchangeDelegate
CHIP_ERROR OnMessageReceived(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader,
System::PacketBufferHandle && aPayload) override;
@@ -180,22 +184,7 @@ class WriteHandler : public Messaging::ExchangeDelegate
Messaging::ExchangeHolder mExchangeCtx;
WriteResponseMessage::Builder mWriteResponseBuilder;
- State mState = State::Uninitialized;
- bool mIsTimedRequest = false;
- bool mSuppressResponse = false;
- bool mHasMoreChunks = false;
Optional<ConcreteAttributePath> mProcessingAttributePath;
- bool mProcessingAttributeIsList = false;
- // We record the Status when AddStatus is called to determine whether all data of a list write is accepted.
- // This value will be used by DeliverListWriteEnd and DeliverFinalListWriteEnd but it won't be used by group writes based on the
- // fact that the errors that won't be delivered to AttributeAccessInterface are:
- // (1) Attribute not found
- // (2) Access control failed
- // (3) Write request to a read-only attribute
- // (4) Data version mismatch
- // (5) Not using timed write.
- // Where (1)-(3) will be consistent among the whole list write request, while (4) and (5) are not appliable to group writes.
- bool mAttributeWriteSuccessful = false;
Optional<AttributeAccessToken> mACLCheckCache = NullOptional;
// This may be a "fake" pointer or a real delegate pointer, depending
@@ -205,6 +194,31 @@ class WriteHandler : public Messaging::ExchangeDelegate
// set to the global InteractionModelEngine and the size of this
// member is 1 byte.
InteractionModelDelegatePointer<WriteHandlerDelegate> mDelegate;
+
+ // bit level enums to save storage for this object. InteractionModelEngine maintains
+ // several of these objects, so every bit of storage multiplies storage usage.
+ enum class StateBits : uint8_t
+ {
+ kIsTimedRequest = 0x01,
+ kSuppressResponse = 0x02,
+ kHasMoreChunks = 0x04,
+ kProcessingAttributeIsList = 0x08,
+ // We record the Status when AddStatus is called to determine whether all data of a list write is accepted.
+ // This value will be used by DeliverListWriteEnd and DeliverFinalListWriteEnd but it won't be used by group writes based on
+ // the fact that the errors that won't be delivered to AttributeAccessInterface are:
+ // (1) Attribute not found
+ // (2) Access control failed
+ // (3) Write request to a read-only attribute
+ // (4) Data version mismatch
+ // (5) Not using timed write.
+ // Where (1)-(3) will be consistent among the whole list write request, while (4) and (5) are not appliable to group
+ // writes.
+ kAttributeWriteSuccessful = 0x10,
+ };
+
+ BitFlags<StateBits> mStateFlags;
+ State mState = State::Uninitialized;
};
+
} // namespace app
} // namespace chip
From 9b1b9a974b3158a1cd33203c786b89e52b06eadc Mon Sep 17 00:00:00 2001
From: Andrei Litvin <andy314@gmail.com>
Date: Wed, 14 Aug 2024 14:31:12 -0400
Subject: [PATCH 2/3] remove some extra added includes
---
src/app/WriteHandler.cpp | 4 ----
1 file changed, 4 deletions(-)
diff --git a/src/app/WriteHandler.cpp b/src/app/WriteHandler.cpp
index 2571385908df9a..906baac6c19592 100644
--- a/src/app/WriteHandler.cpp
+++ b/src/app/WriteHandler.cpp
@@ -18,21 +18,17 @@
#include <app/AppConfig.h>
#include <app/AttributeAccessInterfaceRegistry.h>
-#include <app/AttributeValueDecoder.h>
#include <app/InteractionModelEngine.h>
#include <app/MessageDef/EventPathIB.h>
#include <app/MessageDef/StatusIB.h>
#include <app/StatusResponse.h>
#include <app/WriteHandler.h>
-#include <app/data-model-provider/OperationTypes.h>
#include <app/reporting/Engine.h>
#include <app/util/MatterCallbacks.h>
#include <app/util/ember-compatibility-functions.h>
#include <credentials/GroupDataProvider.h>
-#include <lib/core/CHIPError.h>
#include <lib/support/CodeUtils.h>
#include <lib/support/TypeTraits.h>
-#include <messaging/ExchangeContext.h>
#include <protocols/interaction_model/StatusCode.h>
namespace chip {
From 6a289f988935df05328449293d49b354e8d34925 Mon Sep 17 00:00:00 2001
From: Andrei Litvin <andy314@gmail.com>
Date: Wed, 14 Aug 2024 14:33:22 -0400
Subject: [PATCH 3/3] Apply review comment from previous PR
---
src/app/WriteHandler.cpp | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/app/WriteHandler.cpp b/src/app/WriteHandler.cpp
index 906baac6c19592..15776eb545fc21 100644
--- a/src/app/WriteHandler.cpp
+++ b/src/app/WriteHandler.cpp
@@ -564,26 +564,26 @@ Status WriteHandler::ProcessWriteRequest(System::PacketBufferHandle && aPayload,
boolValue = mStateFlags.Has(StateBits::kSuppressResponse);
err = writeRequestParser.GetSuppressResponse(&boolValue);
- mStateFlags.Set(StateBits::kSuppressResponse, boolValue);
if (err == CHIP_END_OF_TLV)
{
err = CHIP_NO_ERROR;
}
SuccessOrExit(err);
+ mStateFlags.Set(StateBits::kSuppressResponse, boolValue);
boolValue = mStateFlags.Has(StateBits::kIsTimedRequest);
err = writeRequestParser.GetTimedRequest(&boolValue);
- mStateFlags.Set(StateBits::kIsTimedRequest, boolValue);
SuccessOrExit(err);
+ mStateFlags.Set(StateBits::kIsTimedRequest, boolValue);
boolValue = mStateFlags.Has(StateBits::kHasMoreChunks);
err = writeRequestParser.GetMoreChunkedMessages(&boolValue);
- mStateFlags.Set(StateBits::kHasMoreChunks, boolValue);
if (err == CHIP_ERROR_END_OF_TLV)
{
err = CHIP_NO_ERROR;
}
SuccessOrExit(err);
+ mStateFlags.Set(StateBits::kHasMoreChunks, boolValue);
if (mStateFlags.Has(StateBits::kHasMoreChunks) &&
(mExchangeCtx->IsGroupExchangeContext() || mStateFlags.Has(StateBits::kIsTimedRequest)))