diff --git a/src/app/WriteHandler.cpp b/src/app/WriteHandler.cpp index 8d4a82d11630c4..15776eb545fc21 100644 --- a/src/app/WriteHandler.cpp +++ b/src/app/WriteHandler.cpp @@ -16,7 +16,6 @@ * limitations under the License. */ -#include "messaging/ExchangeContext.h" #include <app/AppConfig.h> #include <app/AttributeAccessInterfaceRegistry.h> #include <app/InteractionModelEngine.h> @@ -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> @@ -62,7 +62,7 @@ void WriteHandler::Close() // successful. DeliverFinalListWriteEnd(false /* wasSuccessful */); mExchangeCtx.Release(); - mSuppressResponse = false; + mStateFlags.Clear(StateBits::kSuppressResponse); MoveToState(State::Uninitialized); } @@ -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(); } @@ -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(); } @@ -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); @@ -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); } @@ -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(); @@ -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, @@ -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: @@ -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; @@ -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) @@ -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()); @@ -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(); } @@ -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. @@ -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. @@ -641,7 +653,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