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.

Sign up for GitHub

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

Optimize RAM utilization for WriteHandler #34992

Merged
merged 3 commits into from
Aug 16, 2024
Merged

Optimize RAM utilization for WriteHandler #34992

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
59eb04e
Pull RAM usage reduction from write impl
andy31415 Aug 14, 2024
9b1b9a9
remove some extra added includes
andy31415 Aug 14, 2024
6a289f9
Apply review comment from previous PR
andy31415 Aug 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 39 additions & 27 deletions src/app/WriteHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
* limitations under the License.
*/

#include "messaging/ExchangeContext.h"
#include <app/AppConfig.h>
#include <app/AttributeAccessInterfaceRegistry.h>
#include <app/InteractionModelEngine.h>
Expand All @@ -28,6 +27,7 @@
#include <app/util/MatterCallbacks.h>
#include <app/util/ember-compatibility-functions.h>
#include <credentials/GroupDataProvider.h>
#include <lib/support/CodeUtils.h>
#include <lib/support/TypeTraits.h>
#include <protocols/interaction_model/StatusCode.h>

Expand Down Expand Up @@ -62,7 +62,7 @@ void WriteHandler::Close()
// successful.
DeliverFinalListWriteEnd(false /* wasSuccessful */);
mExchangeCtx.Release();
mSuppressResponse = false;
mStateFlags.Clear(StateBits::kSuppressResponse);
MoveToState(State::Uninitialized);
}

Expand Down Expand Up @@ -106,7 +106,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();
}
Expand Down Expand Up @@ -142,7 +142,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();
}
Expand Down Expand Up @@ -184,8 +184,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);
Expand All @@ -212,7 +212,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);
}
Expand All @@ -221,7 +221,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();
Expand Down Expand Up @@ -321,18 +322,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,
Expand Down Expand Up @@ -370,9 +373,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:
Expand Down Expand Up @@ -426,8 +429,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;
Expand Down Expand Up @@ -457,7 +460,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)
Expand Down Expand Up @@ -498,11 +502,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());
Expand All @@ -512,7 +515,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();
}
Expand Down Expand Up @@ -557,24 +560,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);
if (err == CHIP_END_OF_TLV)
{
err = CHIP_NO_ERROR;
}
SuccessOrExit(err);
mStateFlags.Set(StateBits::kSuppressResponse, boolValue);

err = writeRequestParser.GetTimedRequest(&mIsTimedRequest);
boolValue = mStateFlags.Has(StateBits::kIsTimedRequest);
err = writeRequestParser.GetTimedRequest(&boolValue);
SuccessOrExit(err);
mStateFlags.Set(StateBits::kIsTimedRequest, boolValue);

err = writeRequestParser.GetMoreChunkedMessages(&mHasMoreChunks);
boolValue = mStateFlags.Has(StateBits::kHasMoreChunks);
err = writeRequestParser.GetMoreChunkedMessages(&boolValue);
if (err == CHIP_ERROR_END_OF_TLV)
{
err = CHIP_NO_ERROR;
}
SuccessOrExit(err);
mStateFlags.Set(StateBits::kHasMoreChunks, boolValue);

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.
Expand All @@ -584,7 +596,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.
Expand Down Expand Up @@ -641,7 +653,7 @@ CHIP_ERROR WriteHandler::AddStatusInternal(const ConcreteDataAttributePath & aPa

if (!aStatus.IsSuccess())
{
mAttributeWriteSuccessful = false;
mStateFlags.Clear(StateBits::kAttributeWriteSuccessful);
}

ReturnErrorOnFailure(writeResponses.GetError());
Expand Down
50 changes: 32 additions & 18 deletions src/app/WriteHandler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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
{
Expand All @@ -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
Expand Down Expand Up @@ -172,30 +177,14 @@ 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;
void OnResponseTimeout(Messaging::ExchangeContext * apExchangeContext) override;

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
Expand All @@ -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
Loading