From c278b9b3a41e80a8254e70cb2b0b77e67cbb254d Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 17 Apr 2024 11:35:54 -0400 Subject: [PATCH 01/16] Uniform API: provide subject descriptor only for both value encode and decode --- src/app/AttributeValueDecoder.h | 9 ++------- src/app/AttributeValueEncoder.h | 17 ++++++++--------- .../access-control-server.cpp | 4 ++-- src/app/clusters/bindings/bindings.cpp | 2 +- .../group-key-mgmt-server.cpp | 2 +- .../operational-credentials-server.cpp | 4 ++-- .../ota-requestor/ota-requestor-server.cpp | 2 +- .../test-cluster-server/test-cluster-server.cpp | 2 +- src/app/util/ember-compatibility-functions.cpp | 6 +++--- 9 files changed, 21 insertions(+), 27 deletions(-) diff --git a/src/app/AttributeValueDecoder.h b/src/app/AttributeValueDecoder.h index 5f5e2255b2fa3e..93f48fd29a4ece 100644 --- a/src/app/AttributeValueDecoder.h +++ b/src/app/AttributeValueDecoder.h @@ -46,19 +46,14 @@ class AttributeValueDecoder mTriedDecode = true; // The WriteRequest comes with no fabric index, this will happen when receiving a write request on a PASE session before // AddNOC. - VerifyOrReturnError(AccessingFabricIndex() != kUndefinedFabricIndex, CHIP_IM_GLOBAL_STATUS(UnsupportedAccess)); + VerifyOrReturnError(GetSubjectDescriptor().fabricIndex != kUndefinedFabricIndex, CHIP_IM_GLOBAL_STATUS(UnsupportedAccess)); ReturnErrorOnFailure(DataModel::Decode(mReader, aArg)); - aArg.SetFabricIndex(AccessingFabricIndex()); + aArg.SetFabricIndex(GetSubjectDescriptor().fabricIndex); return CHIP_NO_ERROR; } bool TriedDecode() const { return mTriedDecode; } - /** - * The accessing fabric index for this write interaction. - */ - FabricIndex AccessingFabricIndex() const { return mSubjectDescriptor.fabricIndex; } - /** * The accessing subject descriptor for this write interaction. */ diff --git a/src/app/AttributeValueEncoder.h b/src/app/AttributeValueEncoder.h index d5efe1768c9d06..ec5b313ed66bc7 100644 --- a/src/app/AttributeValueEncoder.h +++ b/src/app/AttributeValueEncoder.h @@ -16,6 +16,7 @@ */ #pragma once +#include #include #include #include @@ -51,9 +52,10 @@ class AttributeValueEncoder // If we are encoding for a fabric filtered attribute read and the fabric index does not match that present in the // request, skip encoding this list item. VerifyOrReturnError(!mAttributeValueEncoder.mIsFabricFiltered || - aArg.GetFabricIndex() == mAttributeValueEncoder.mAccessingFabricIndex, + aArg.GetFabricIndex() == mAttributeValueEncoder.GetSubjectDescriptor().fabricIndex, CHIP_NO_ERROR); - return mAttributeValueEncoder.EncodeListItem(mAttributeValueEncoder.mAccessingFabricIndex, std::forward(aArg)); + return mAttributeValueEncoder.EncodeListItem(mAttributeValueEncoder.GetSubjectDescriptor().fabricIndex, + std::forward(aArg)); } template ::value, bool> = true> @@ -97,11 +99,11 @@ class AttributeValueEncoder ListIndex mCurrentEncodingListIndex = kInvalidListIndex; }; - AttributeValueEncoder(AttributeReportIBs::Builder & aAttributeReportIBsBuilder, FabricIndex aAccessingFabricIndex, + AttributeValueEncoder(AttributeReportIBs::Builder & aAttributeReportIBsBuilder, Access::SubjectDescriptor subjectDescriptor, const ConcreteAttributePath & aPath, DataVersion aDataVersion, bool aIsFabricFiltered = false, const AttributeEncodeState & aState = AttributeEncodeState()) : mAttributeReportIBsBuilder(aAttributeReportIBsBuilder), - mAccessingFabricIndex(aAccessingFabricIndex), mPath(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId), + mSubjectDescriptor(subjectDescriptor), mPath(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId), mDataVersion(aDataVersion), mIsFabricFiltered(aIsFabricFiltered), mEncodeState(aState) {} @@ -176,10 +178,7 @@ class AttributeValueEncoder bool TriedEncode() const { return mTriedEncode; } - /** - * The accessing fabric index for this read or subscribe interaction. - */ - FabricIndex AccessingFabricIndex() const { return mAccessingFabricIndex; } + const Access::SubjectDescriptor & GetSubjectDescriptor() const { return mSubjectDescriptor; } /** * AttributeValueEncoder is a short lived object, and the state is persisted by mEncodeState and restored by constructor. @@ -268,7 +267,7 @@ class AttributeValueEncoder bool mTriedEncode = false; AttributeReportIBs::Builder & mAttributeReportIBsBuilder; - const FabricIndex mAccessingFabricIndex; + const Access::SubjectDescriptor mSubjectDescriptor; ConcreteDataAttributePath mPath; DataVersion mDataVersion; bool mIsFabricFiltered = false; diff --git a/src/app/clusters/access-control-server/access-control-server.cpp b/src/app/clusters/access-control-server/access-control-server.cpp index 316825bc91848c..b84f8dc084f0ee 100644 --- a/src/app/clusters/access-control-server/access-control-server.cpp +++ b/src/app/clusters/access-control-server/access-control-server.cpp @@ -230,7 +230,7 @@ CHIP_ERROR AccessControlAttribute::WriteImpl(const ConcreteDataAttributePath & a CHIP_ERROR AccessControlAttribute::WriteAcl(const ConcreteDataAttributePath & aPath, AttributeValueDecoder & aDecoder) { - FabricIndex accessingFabricIndex = aDecoder.AccessingFabricIndex(); + FabricIndex accessingFabricIndex = aDecoder.GetSubjectDescriptor().fabricIndex; size_t oldCount; ReturnErrorOnFailure(GetAccessControl().GetEntryCount(accessingFabricIndex, oldCount)); @@ -293,7 +293,7 @@ CHIP_ERROR AccessControlAttribute::WriteExtension(const ConcreteDataAttributePat { auto & storage = Server::GetInstance().GetPersistentStorage(); - FabricIndex accessingFabricIndex = aDecoder.AccessingFabricIndex(); + FabricIndex accessingFabricIndex = aDecoder.GetSubjectDescriptor().fabricIndex; uint8_t buffer[kExtensionDataMaxLength] = { 0 }; uint16_t size = static_cast(sizeof(buffer)); diff --git a/src/app/clusters/bindings/bindings.cpp b/src/app/clusters/bindings/bindings.cpp index 63b085d7ee6de4..0fe00ef50fef9c 100644 --- a/src/app/clusters/bindings/bindings.cpp +++ b/src/app/clusters/bindings/bindings.cpp @@ -200,7 +200,7 @@ void BindingTableAccess::OnListWriteEnd(const app::ConcreteAttributePath & aPath CHIP_ERROR BindingTableAccess::WriteBindingTable(const ConcreteDataAttributePath & path, AttributeValueDecoder & decoder) { - mAccessingFabricIndex = decoder.AccessingFabricIndex(); + mAccessingFabricIndex = decoder.GetSubjectDescriptor().fabricIndex; if (!path.IsListOperation() || path.mListOp == ConcreteDataAttributePath::ListOperation::ReplaceAll) { DecodableBindingListType newBindingList; diff --git a/src/app/clusters/group-key-mgmt-server/group-key-mgmt-server.cpp b/src/app/clusters/group-key-mgmt-server/group-key-mgmt-server.cpp index c3cf09eb283f63..368d6ff13cc5f2 100644 --- a/src/app/clusters/group-key-mgmt-server/group-key-mgmt-server.cpp +++ b/src/app/clusters/group-key-mgmt-server/group-key-mgmt-server.cpp @@ -192,7 +192,7 @@ class GroupKeyManagementAttributeAccess : public AttributeAccessInterface CHIP_ERROR WriteGroupKeyMap(const ConcreteDataAttributePath & aPath, AttributeValueDecoder & aDecoder) { - auto fabric_index = aDecoder.AccessingFabricIndex(); + auto fabric_index = aDecoder.GetSubjectDescriptor().fabricIndex; auto provider = GetGroupDataProvider(); if (!aPath.IsListItemOperation()) diff --git a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp index 12fe1c43de0baf..6338d04d8f7989 100644 --- a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp +++ b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp @@ -117,7 +117,7 @@ class OperationalCredentialsAttrAccess : public AttributeAccessInterface CHIP_ERROR OperationalCredentialsAttrAccess::ReadNOCs(EndpointId endpoint, AttributeValueEncoder & aEncoder) { - auto accessingFabricIndex = aEncoder.AccessingFabricIndex(); + auto accessingFabricIndex = aEncoder.GetSubjectDescriptor().fabricIndex; return aEncoder.EncodeList([accessingFabricIndex](const auto & encoder) -> CHIP_ERROR { const auto & fabricTable = Server::GetInstance().GetFabricTable(); @@ -252,7 +252,7 @@ CHIP_ERROR OperationalCredentialsAttrAccess::Read(const ConcreteReadAttributePat return ReadRootCertificates(aPath.mEndpointId, aEncoder); } case Attributes::CurrentFabricIndex::Id: { - return aEncoder.Encode(aEncoder.AccessingFabricIndex()); + return aEncoder.Encode(aEncoder.GetSubjectDescriptor().fabricIndex); } default: break; diff --git a/src/app/clusters/ota-requestor/ota-requestor-server.cpp b/src/app/clusters/ota-requestor/ota-requestor-server.cpp index 7bfbde1fa54ac5..47595242a4e32d 100644 --- a/src/app/clusters/ota-requestor/ota-requestor-server.cpp +++ b/src/app/clusters/ota-requestor/ota-requestor-server.cpp @@ -120,7 +120,7 @@ CHIP_ERROR OtaSoftwareUpdateRequestorAttrAccess::WriteDefaultOtaProviders(const DataModel::DecodableList list; ReturnErrorOnFailure(aDecoder.Decode(list)); - ReturnErrorOnFailure(requestor->ClearDefaultOtaProviderList(aDecoder.AccessingFabricIndex())); + ReturnErrorOnFailure(requestor->ClearDefaultOtaProviderList(aDecoder.GetSubjectDescriptor().fabricIndex)); auto iter = list.begin(); while (iter.Next()) diff --git a/src/app/clusters/test-cluster-server/test-cluster-server.cpp b/src/app/clusters/test-cluster-server/test-cluster-server.cpp index 0d805347f792f9..dcd0a6722a8003 100644 --- a/src/app/clusters/test-cluster-server/test-cluster-server.cpp +++ b/src/app/clusters/test-cluster-server/test-cluster-server.cpp @@ -661,7 +661,7 @@ CHIP_ERROR TestAttrAccess::WriteListFabricScopedAttribute(const ConcreteDataAttr size_t srcIndex = 0, dstIndex = 0; while (srcIndex < gListFabricScopedAttributeLen) { - if (gListFabricScopedAttributeValue[srcIndex].fabricIndex != aDecoder.AccessingFabricIndex()) + if (gListFabricScopedAttributeValue[srcIndex].fabricIndex != aDecoder.GetSubjectDescriptor().fabricIndex) { auto & dstEntry = gListFabricScopedAttributeValue[dstIndex]; auto & srcEntry = gListFabricScopedAttributeValue[srcIndex]; diff --git a/src/app/util/ember-compatibility-functions.cpp b/src/app/util/ember-compatibility-functions.cpp index 3c98863aa50fb1..dfbec067427e2a 100644 --- a/src/app/util/ember-compatibility-functions.cpp +++ b/src/app/util/ember-compatibility-functions.cpp @@ -425,7 +425,7 @@ CHIP_ERROR GlobalAttributeReader::EncodeCommandList(const ConcreteClusterPath & // Helper function for trying to read an attribute value via an // AttributeAccessInterface. On failure, the read has failed. On success, the // aTriedEncode outparam is set to whether the AttributeAccessInterface tried to encode a value. -CHIP_ERROR ReadViaAccessInterface(FabricIndex aAccessingFabricIndex, bool aIsFabricFiltered, +CHIP_ERROR ReadViaAccessInterface(SubjectDescriptor subjectDescriptor, bool aIsFabricFiltered, const ConcreteReadAttributePath & aPath, AttributeReportIBs::Builder & aAttributeReports, AttributeValueEncoder::AttributeEncodeState * aEncoderState, AttributeAccessInterface * aAccessInterface, bool * aTriedEncode) @@ -434,7 +434,7 @@ CHIP_ERROR ReadViaAccessInterface(FabricIndex aAccessingFabricIndex, bool aIsFab (aEncoderState == nullptr ? AttributeValueEncoder::AttributeEncodeState() : *aEncoderState); DataVersion version = 0; ReturnErrorOnFailure(ReadClusterDataVersion(aPath, version)); - AttributeValueEncoder valueEncoder(aAttributeReports, aAccessingFabricIndex, aPath, version, aIsFabricFiltered, state); + AttributeValueEncoder valueEncoder(aAttributeReports, subjectDescriptor, aPath, version, aIsFabricFiltered, state); CHIP_ERROR err = aAccessInterface->Read(aPath, valueEncoder); if (err == CHIP_IM_GLOBAL_STATUS(UnsupportedRead) && aPath.mExpanded) @@ -569,7 +569,7 @@ CHIP_ERROR ReadSingleClusterData(const SubjectDescriptor & aSubjectDescriptor, b if (attributeOverride) { bool triedEncode = false; - ReturnErrorOnFailure(ReadViaAccessInterface(aSubjectDescriptor.fabricIndex, aIsFabricFiltered, aPath, aAttributeReports, + ReturnErrorOnFailure(ReadViaAccessInterface(aSubjectDescriptor, aIsFabricFiltered, aPath, aAttributeReports, apEncoderState, attributeOverride, &triedEncode)); ReturnErrorCodeIf(triedEncode, CHIP_NO_ERROR); } From 3debb6d2e9a667b3aa481c855673a1a54e6b3218 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 17 Apr 2024 11:52:23 -0400 Subject: [PATCH 02/16] Split out AttributeEncodeState and decouple things to use less friend classes --- src/app/AttributeValueEncoder.cpp | 10 +-- src/app/AttributeValueEncoder.h | 89 ++++++++++++------- src/app/ReadHandler.cpp | 4 +- src/app/ReadHandler.h | 6 +- src/app/dynamic_server/DynamicDispatcher.cpp | 2 +- src/app/reporting/Engine.cpp | 8 +- src/app/reporting/Engine.h | 3 +- src/app/tests/TestAttributeValueEncoder.cpp | 6 +- src/app/tests/TestReadInteraction.cpp | 2 +- .../tests/integration/chip_im_initiator.cpp | 2 +- .../tests/integration/chip_im_responder.cpp | 2 +- .../util/ember-compatibility-functions.cpp | 11 ++- src/app/util/ember-compatibility-functions.h | 2 +- src/app/util/mock/Functions.h | 2 +- src/app/util/mock/attribute-storage.cpp | 6 +- src/controller/tests/data_model/TestRead.cpp | 32 ++++--- 16 files changed, 109 insertions(+), 78 deletions(-) diff --git a/src/app/AttributeValueEncoder.cpp b/src/app/AttributeValueEncoder.cpp index da4edd1fc483e7..4b6bb10e66abdb 100644 --- a/src/app/AttributeValueEncoder.cpp +++ b/src/app/AttributeValueEncoder.cpp @@ -32,13 +32,13 @@ CHIP_ERROR AttributeValueEncoder::EnsureListStarted() { VerifyOrDie(mCurrentEncodingListIndex == kInvalidListIndex); - mEncodingInitialList = (mEncodeState.mCurrentEncodingListIndex == kInvalidListIndex); + mEncodingInitialList = (mEncodeState.CurrentEncodingListIndex() == kInvalidListIndex); if (mEncodingInitialList) { // Clear mAllowPartialData flag here since this encode procedure is not atomic. // The most common error in this function is CHIP_ERROR_NO_MEMORY / CHIP_ERROR_BUFFER_TOO_SMALL, just revert and try // next time is ok. - mEncodeState.mAllowPartialData = false; + mEncodeState.SetAllowPartialData(false); AttributeReportBuilder builder; @@ -59,7 +59,7 @@ CHIP_ERROR AttributeValueEncoder::EnsureListStarted() ReturnErrorOnFailure( mAttributeReportIBsBuilder.GetWriter()->ReserveBuffer(kEndOfAttributeReportIBByteCount + kEndOfListByteCount)); - mEncodeState.mCurrentEncodingListIndex = 0; + mEncodeState.SetCurrentEncodingListIndex(0); } else { @@ -72,7 +72,7 @@ CHIP_ERROR AttributeValueEncoder::EnsureListStarted() // After encoding the initial list start, the remaining items are atomically encoded into the buffer. Tell report engine to not // revert partial data. - mEncodeState.mAllowPartialData = true; + mEncodeState.SetAllowPartialData(true); return CHIP_NO_ERROR; } @@ -105,7 +105,7 @@ void AttributeValueEncoder::EnsureListEnded() // If we succeeded at encoding the whole list (i.e. the list is in fact // empty and we fit in the packet), mAllowPartialData will be ignored, // so it's safe to set it to false even if encoding succeeded. - mEncodeState.mAllowPartialData = false; + mEncodeState.SetAllowPartialData(false); } } diff --git a/src/app/AttributeValueEncoder.h b/src/app/AttributeValueEncoder.h index ec5b313ed66bc7..8da2e2babdec05 100644 --- a/src/app/AttributeValueEncoder.h +++ b/src/app/AttributeValueEncoder.h @@ -28,6 +28,60 @@ namespace chip { namespace app { +/** + * Maintains the internal state of list encoding + */ +class AttributeEncodeState +{ +public: + AttributeEncodeState() = default; + + bool AllowPartialData() const { return mAllowPartialData; } + ListIndex CurrentEncodingListIndex() const { return mCurrentEncodingListIndex; } + + AttributeEncodeState & SetAllowPartialData(bool allow) + { + mAllowPartialData = allow; + return *this; + } + + AttributeEncodeState & SetCurrentEncodingListIndex(ListIndex idx) + { + mCurrentEncodingListIndex = idx; + return *this; + } + + void Reset() + { + mCurrentEncodingListIndex = kInvalidListIndex; + mAllowPartialData = false; + } + +private: + /** + * If set to kInvalidListIndex, indicates that we have not encoded any data for the list yet and + * need to start by encoding an empty list before we start encoding any list items. + * + * When set to a valid ListIndex value, indicates the index of the next list item that needs to be + * encoded (i.e. the count of items encoded so far). + */ + ListIndex mCurrentEncodingListIndex = kInvalidListIndex; + + /** + * When an attempt to encode an attribute returns an error, the buffer may contain tailing dirty data + * (since the put was aborted). The report engine normally rolls back the buffer to right before encoding + * of the attribute started on errors. + * + * When chunking a list, EncodeListItem will atomically encode list items, ensuring that the + * state of the buffer is valid to send (i.e. contains no trailing garbage), and return an error + * if the list doesn't entirely fit. In this situation, mAllowPartialData is set to communicate to the + * report engine that it should not roll back the list items. + * + * TODO: There might be a better name for this variable. + */ + bool mAllowPartialData = false; +}; + /** * The AttributeValueEncoder is a helper class for filling report payloads into AttributeReportIBs. * The attribute value encoder can be initialized with a AttributeEncodeState for saving and recovering its state between encode @@ -68,37 +122,6 @@ class AttributeValueEncoder AttributeValueEncoder & mAttributeValueEncoder; }; - class AttributeEncodeState - { - public: - AttributeEncodeState() : mAllowPartialData(false), mCurrentEncodingListIndex(kInvalidListIndex) {} - bool AllowPartialData() const { return mAllowPartialData; } - - private: - friend class AttributeValueEncoder; - /** - * When an attempt to encode an attribute returns an error, the buffer may contain tailing dirty data - * (since the put was aborted). The report engine normally rolls back the buffer to right before encoding - * of the attribute started on errors. - * - * When chunking a list, EncodeListItem will atomically encode list items, ensuring that the - * state of the buffer is valid to send (i.e. contains no trailing garbage), and return an error - * if the list doesn't entirely fit. In this situation, mAllowPartialData is set to communicate to the - * report engine that it should not roll back the list items. - * - * TODO: There might be a better name for this variable. - */ - bool mAllowPartialData = false; - /** - * If set to kInvalidListIndex, indicates that we have not encoded any data for the list yet and - * need to start by encoding an empty list before we start encoding any list items. - * - * When set to a valid ListIndex value, indicates the index of the next list item that needs to be - * encoded (i.e. the count of items encoded so far). - */ - ListIndex mCurrentEncodingListIndex = kInvalidListIndex; - }; - AttributeValueEncoder(AttributeReportIBs::Builder & aAttributeReportIBsBuilder, Access::SubjectDescriptor subjectDescriptor, const ConcreteAttributePath & aPath, DataVersion aDataVersion, bool aIsFabricFiltered = false, const AttributeEncodeState & aState = AttributeEncodeState()) : @@ -194,7 +217,7 @@ class AttributeValueEncoder { // EncodeListItem must be called after EnsureListStarted(), thus mCurrentEncodingListIndex and // mEncodeState.mCurrentEncodingListIndex are not invalid values. - if (mCurrentEncodingListIndex < mEncodeState.mCurrentEncodingListIndex) + if (mCurrentEncodingListIndex < mEncodeState.CurrentEncodingListIndex()) { // We have encoded this element in previous chunks, skip it. mCurrentEncodingListIndex++; @@ -225,7 +248,7 @@ class AttributeValueEncoder } mCurrentEncodingListIndex++; - mEncodeState.mCurrentEncodingListIndex++; + mEncodeState.SetCurrentEncodingListIndex(mCurrentEncodingListIndex); mEncodedAtLeastOneListItem = true; return CHIP_NO_ERROR; } diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp index 142df8015601ca..461add0f2dc150 100644 --- a/src/app/ReadHandler.cpp +++ b/src/app/ReadHandler.cpp @@ -841,7 +841,7 @@ void ReadHandler::PersistSubscription() void ReadHandler::ResetPathIterator() { mAttributePathExpandIterator = AttributePathExpandIterator(mpAttributePathList); - mAttributeEncoderState = AttributeValueEncoder::AttributeEncodeState(); + mAttributeEncoderState.Reset(); } void ReadHandler::AttributePathIsDirty(const AttributePathParams & aAttributeChanged) @@ -870,7 +870,7 @@ void ReadHandler::AttributePathIsDirty(const AttributePathParams & aAttributeCha // our iterator to point back to the beginning of that cluster. This ensures that the receiver will get a coherent view of // the state of the cluster as present on the server mAttributePathExpandIterator.ResetCurrentCluster(); - mAttributeEncoderState = AttributeValueEncoder::AttributeEncodeState(); + mAttributeEncoderState.Reset(); } // ReportScheduler will take care of verifying the reportability of the handler and schedule the run diff --git a/src/app/ReadHandler.h b/src/app/ReadHandler.h index f5355372ac5301..a30f1f8974ad25 100644 --- a/src/app/ReadHandler.h +++ b/src/app/ReadHandler.h @@ -412,8 +412,8 @@ class ReadHandler : public Messaging::ExchangeDelegate /// or after the min interval is reached if it has not yet been reached. void ForceDirtyState(); - const AttributeValueEncoder::AttributeEncodeState & GetAttributeEncodeState() const { return mAttributeEncoderState; } - void SetAttributeEncodeState(const AttributeValueEncoder::AttributeEncodeState & aState) { mAttributeEncoderState = aState; } + const AttributeEncodeState & GetAttributeEncodeState() const { return mAttributeEncoderState; } + void SetAttributeEncodeState(const AttributeEncodeState & aState) { mAttributeEncoderState = aState; } uint32_t GetLastWrittenEventsBytes() const { return mLastWrittenEventsBytes; } // Returns the number of interested paths, including wildcard and concrete paths. @@ -562,7 +562,7 @@ class ReadHandler : public Messaging::ExchangeDelegate // The detailed encoding state for a single attribute, used by list chunking feature. // The size of AttributeEncoderState is 2 bytes for now. - AttributeValueEncoder::AttributeEncodeState mAttributeEncoderState; + AttributeEncodeState mAttributeEncoderState; // Current Handler state HandlerState mState = HandlerState::Idle; diff --git a/src/app/dynamic_server/DynamicDispatcher.cpp b/src/app/dynamic_server/DynamicDispatcher.cpp index 5f9476aa6fd40a..e1f458f0bc3df1 100644 --- a/src/app/dynamic_server/DynamicDispatcher.cpp +++ b/src/app/dynamic_server/DynamicDispatcher.cpp @@ -123,7 +123,7 @@ Status DetermineAttributeStatus(const ConcreteAttributePath & aPath, bool aIsWri CHIP_ERROR ReadSingleClusterData(const SubjectDescriptor & aSubjectDescriptor, bool aIsFabricFiltered, const ConcreteReadAttributePath & aPath, AttributeReportIBs::Builder & aAttributeReports, - AttributeValueEncoder::AttributeEncodeState * aEncoderState) + AttributeEncodeState * aEncoderState) { Status status = DetermineAttributeStatus(aPath, /* aIsWrite = */ false); return aAttributeReports.EncodeAttributeStatus(aPath, StatusIB(status)); diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp index 37c7e31dd645fb..39c582f3e45f0b 100644 --- a/src/app/reporting/Engine.cpp +++ b/src/app/reporting/Engine.cpp @@ -82,7 +82,7 @@ bool Engine::IsClusterDataVersionMatch(const SingleLinkedListNode Cluster %" PRIx32 ", Attribute %" PRIx32 " is dirty", aPath.mClusterId, aPath.mAttributeId); @@ -199,7 +199,7 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu attributeReportIBs.Checkpoint(attributeBackup); ConcreteReadAttributePath pathForRetrieval(readPath); // Load the saved state from previous encoding session for chunking of one single attribute (list chunking). - AttributeValueEncoder::AttributeEncodeState encodeState = apReadHandler->GetAttributeEncodeState(); + AttributeEncodeState encodeState = apReadHandler->GetAttributeEncodeState(); err = RetrieveClusterData(apReadHandler->GetSubjectDescriptor(), apReadHandler->IsFabricFiltered(), attributeReportIBs, pathForRetrieval, &encodeState); if (err != CHIP_NO_ERROR) @@ -226,7 +226,7 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu // We met a error during writing reports, one common case is we are running out of buffer, rollback the // attributeReportIB to avoid any partial data. attributeReportIBs.Rollback(attributeBackup); - apReadHandler->SetAttributeEncodeState(AttributeValueEncoder::AttributeEncodeState()); + apReadHandler->SetAttributeEncodeState(AttributeEncodeState()); if (!IsOutOfWriterSpaceError(err)) { @@ -256,7 +256,7 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu } SuccessOrExit(err); // Successfully encoded the attribute, clear the internal state. - apReadHandler->SetAttributeEncodeState(AttributeValueEncoder::AttributeEncodeState()); + apReadHandler->SetAttributeEncodeState(AttributeEncodeState()); } // We just visited all paths interested by this read handler and did not abort in the middle of iteration, there are no more // chunks for this report. diff --git a/src/app/reporting/Engine.h b/src/app/reporting/Engine.h index fccf9e08ab020f..0bf7a9f83de1ae 100644 --- a/src/app/reporting/Engine.h +++ b/src/app/reporting/Engine.h @@ -170,8 +170,7 @@ class Engine bool aBufferIsUsed, bool * apHasMoreChunks, bool * apHasEncodedData); CHIP_ERROR RetrieveClusterData(const Access::SubjectDescriptor & aSubjectDescriptor, bool aIsFabricFiltered, AttributeReportIBs::Builder & aAttributeReportIBs, - const ConcreteReadAttributePath & aClusterInfo, - AttributeValueEncoder::AttributeEncodeState * apEncoderState); + const ConcreteReadAttributePath & aClusterInfo, AttributeEncodeState * apEncoderState); CHIP_ERROR CheckAccessDeniedEventPaths(TLV::TLVWriter & aWriter, bool & aHasEncodedData, ReadHandler * apReadHandler); // If version match, it means don't send, if version mismatch, it means send. diff --git a/src/app/tests/TestAttributeValueEncoder.cpp b/src/app/tests/TestAttributeValueEncoder.cpp index 25ea87736c84e8..b0c12bcb85dbfd 100644 --- a/src/app/tests/TestAttributeValueEncoder.cpp +++ b/src/app/tests/TestAttributeValueEncoder.cpp @@ -55,7 +55,7 @@ template struct LimitedTestSetup { LimitedTestSetup(nlTestSuite * aSuite, const FabricIndex aFabricIndex = kUndefinedFabricIndex, - const AttributeValueEncoder::AttributeEncodeState & aState = AttributeValueEncoder::AttributeEncodeState()) : + const AttributeEncodeState & aState = AttributeEncodeState()) : encoder(builder, aFabricIndex, ConcreteAttributePath(kRandomEndpointId, kRandomClusterId, kRandomAttributeId), kRandomDataVersion, aFabricIndex != kUndefinedFabricIndex, aState) { @@ -275,7 +275,7 @@ void TestEncodeFabricScoped(nlTestSuite * aSuite, void * aContext) void TestEncodeListChunking(nlTestSuite * aSuite, void * aContext) { - AttributeValueEncoder::AttributeEncodeState state; + AttributeEncodeState state; bool list[] = { true, false, false, true, true, false }; auto listEncoder = [&list](const auto & encoder) -> CHIP_ERROR { @@ -400,7 +400,7 @@ void TestEncodeListChunking(nlTestSuite * aSuite, void * aContext) void TestEncodeListChunking2(nlTestSuite * aSuite, void * aContext) { - AttributeValueEncoder::AttributeEncodeState state; + AttributeEncodeState state; bool list[] = { true, false, false, true, true, false }; auto listEncoder = [&list](const auto & encoder) -> CHIP_ERROR { diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp index eacd9a1d6f7762..ed00fee99caa47 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -303,7 +303,7 @@ namespace app { CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescriptor, bool aIsFabricFiltered, const ConcreteReadAttributePath & aPath, AttributeReportIBs::Builder & aAttributeReports, - AttributeValueEncoder::AttributeEncodeState * apEncoderState) + AttributeEncodeState * apEncoderState) { if (aPath.mClusterId >= Test::kMockEndpointMin) { diff --git a/src/app/tests/integration/chip_im_initiator.cpp b/src/app/tests/integration/chip_im_initiator.cpp index 56cd7f37005ce5..48a66cb7fc2bba 100644 --- a/src/app/tests/integration/chip_im_initiator.cpp +++ b/src/app/tests/integration/chip_im_initiator.cpp @@ -631,7 +631,7 @@ void DispatchSingleClusterCommand(const ConcreteCommandPath & aCommandPath, chip CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescriptor, bool aIsFabricFiltered, const ConcreteReadAttributePath & aPath, AttributeReportIBs::Builder & aAttributeReports, - AttributeValueEncoder::AttributeEncodeState * apEncoderState) + AttributeEncodeState * apEncoderState) { AttributeReportIB::Builder & attributeReport = aAttributeReports.CreateAttributeReport(); ReturnErrorOnFailure(aAttributeReports.GetError()); diff --git a/src/app/tests/integration/chip_im_responder.cpp b/src/app/tests/integration/chip_im_responder.cpp index 0250cd0fbc7e69..880e9e75f50ef4 100644 --- a/src/app/tests/integration/chip_im_responder.cpp +++ b/src/app/tests/integration/chip_im_responder.cpp @@ -121,7 +121,7 @@ void DispatchSingleClusterCommand(const ConcreteCommandPath & aRequestCommandPat CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescriptor, bool aIsFabricFiltered, const ConcreteReadAttributePath & aPath, AttributeReportIBs::Builder & aAttributeReports, - AttributeValueEncoder::AttributeEncodeState * apEncoderState) + AttributeEncodeState * apEncoderState) { ReturnErrorOnFailure(AttributeValueEncoder(aAttributeReports, 0, aPath, 0).Encode(kTestFieldValue1)); return CHIP_NO_ERROR; diff --git a/src/app/util/ember-compatibility-functions.cpp b/src/app/util/ember-compatibility-functions.cpp index dfbec067427e2a..30cbb7e96c7ba0 100644 --- a/src/app/util/ember-compatibility-functions.cpp +++ b/src/app/util/ember-compatibility-functions.cpp @@ -427,12 +427,11 @@ CHIP_ERROR GlobalAttributeReader::EncodeCommandList(const ConcreteClusterPath & // aTriedEncode outparam is set to whether the AttributeAccessInterface tried to encode a value. CHIP_ERROR ReadViaAccessInterface(SubjectDescriptor subjectDescriptor, bool aIsFabricFiltered, const ConcreteReadAttributePath & aPath, AttributeReportIBs::Builder & aAttributeReports, - AttributeValueEncoder::AttributeEncodeState * aEncoderState, - AttributeAccessInterface * aAccessInterface, bool * aTriedEncode) + AttributeEncodeState * aEncoderState, AttributeAccessInterface * aAccessInterface, + bool * aTriedEncode) { - AttributeValueEncoder::AttributeEncodeState state = - (aEncoderState == nullptr ? AttributeValueEncoder::AttributeEncodeState() : *aEncoderState); - DataVersion version = 0; + AttributeEncodeState state = (aEncoderState == nullptr ? AttributeEncodeState() : *aEncoderState); + DataVersion version = 0; ReturnErrorOnFailure(ReadClusterDataVersion(aPath, version)); AttributeValueEncoder valueEncoder(aAttributeReports, subjectDescriptor, aPath, version, aIsFabricFiltered, state); CHIP_ERROR err = aAccessInterface->Read(aPath, valueEncoder); @@ -523,7 +522,7 @@ bool ConcreteAttributePathExists(const ConcreteAttributePath & aPath) CHIP_ERROR ReadSingleClusterData(const SubjectDescriptor & aSubjectDescriptor, bool aIsFabricFiltered, const ConcreteReadAttributePath & aPath, AttributeReportIBs::Builder & aAttributeReports, - AttributeValueEncoder::AttributeEncodeState * apEncoderState) + AttributeEncodeState * apEncoderState) { ChipLogDetail(DataManagement, "Reading attribute: Cluster=" ChipLogFormatMEI " Endpoint=%x AttributeId=" ChipLogFormatMEI " (expanded=%d)", diff --git a/src/app/util/ember-compatibility-functions.h b/src/app/util/ember-compatibility-functions.h index 72d4379bce69f0..ca7f16950bad39 100644 --- a/src/app/util/ember-compatibility-functions.h +++ b/src/app/util/ember-compatibility-functions.h @@ -60,7 +60,7 @@ bool ConcreteAttributePathExists(const ConcreteAttributePath & aPath); */ CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescriptor, bool aIsFabricFiltered, const ConcreteReadAttributePath & aPath, AttributeReportIBs::Builder & aAttributeReports, - AttributeValueEncoder::AttributeEncodeState * apEncoderState); + AttributeEncodeState * apEncoderState); /** * Returns the metadata of the attribute for the given path. diff --git a/src/app/util/mock/Functions.h b/src/app/util/mock/Functions.h index 2304d54c589787..6d58e6370cf39d 100644 --- a/src/app/util/mock/Functions.h +++ b/src/app/util/mock/Functions.h @@ -35,7 +35,7 @@ namespace Test { CHIP_ERROR ReadSingleMockClusterData(FabricIndex aAccessingFabricIndex, const app::ConcreteAttributePath & aPath, app::AttributeReportIBs::Builder & aAttributeReports, - app::AttributeValueEncoder::AttributeEncodeState * apEncoderState); + app::AttributeEncodeState * apEncoderState); /// Increase the current value for `GetVersion` void BumpVersion(); diff --git a/src/app/util/mock/attribute-storage.cpp b/src/app/util/mock/attribute-storage.cpp index 7326886851ff37..41b14a6181ecbc 100644 --- a/src/app/util/mock/attribute-storage.cpp +++ b/src/app/util/mock/attribute-storage.cpp @@ -321,8 +321,7 @@ DataVersion GetVersion() } CHIP_ERROR ReadSingleMockClusterData(FabricIndex aAccessingFabricIndex, const ConcreteAttributePath & aPath, - AttributeReportIBs::Builder & aAttributeReports, - AttributeValueEncoder::AttributeEncodeState * apEncoderState) + AttributeReportIBs::Builder & aAttributeReports, AttributeEncodeState * apEncoderState) { bool dataExists = (emberAfGetServerAttributeIndexByAttributeId(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId) != UINT16_MAX); @@ -351,8 +350,7 @@ CHIP_ERROR ReadSingleMockClusterData(FabricIndex aAccessingFabricIndex, const Co // Attribute 4 acts as a large attribute to trigger chunking. if (aPath.mAttributeId == MockAttributeId(4)) { - AttributeValueEncoder::AttributeEncodeState state = - (apEncoderState == nullptr ? AttributeValueEncoder::AttributeEncodeState() : *apEncoderState); + AttributeEncodeState state = (apEncoderState == nullptr ? AttributeEncodeState() : *apEncoderState); AttributeValueEncoder valueEncoder(aAttributeReports, aAccessingFabricIndex, aPath, dataVersion, false, state); CHIP_ERROR err = valueEncoder.EncodeList([](const auto & encoder) -> CHIP_ERROR { diff --git a/src/controller/tests/data_model/TestRead.cpp b/src/controller/tests/data_model/TestRead.cpp index fc2cfaf516524b..d6a0c4cb794e38 100644 --- a/src/controller/tests/data_model/TestRead.cpp +++ b/src/controller/tests/data_model/TestRead.cpp @@ -89,7 +89,7 @@ namespace chip { namespace app { CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescriptor, bool aIsFabricFiltered, const ConcreteReadAttributePath & aPath, AttributeReportIBs::Builder & aAttributeReports, - AttributeValueEncoder::AttributeEncodeState * apEncoderState) + AttributeEncodeState * apEncoderState) { if (aPath.mEndpointId >= chip::Test::kMockEndpointMin) { @@ -109,8 +109,11 @@ CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescr // Use an incorrect attribute id for some of the responses. path.mAttributeId = static_cast(path.mAttributeId + (i / 2) + (responseDirective == kSendManyDataResponsesWrongPath)); - AttributeValueEncoder::AttributeEncodeState state = - (apEncoderState == nullptr ? AttributeValueEncoder::AttributeEncodeState() : *apEncoderState); + AttributeEncodeState state; + if (apEncoderState == nullptr) + { + state = *apEncoderState; + } AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor.fabricIndex, path, kDataVersion /* data version */, aIsFabricFiltered, state); ReturnErrorOnFailure(valueEncoder.Encode(true)); @@ -124,8 +127,11 @@ CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescr if (aPath.mClusterId == app::Clusters::UnitTesting::Id && aPath.mAttributeId == app::Clusters::UnitTesting::Attributes::ListFabricScoped::Id) { - AttributeValueEncoder::AttributeEncodeState state = - (apEncoderState == nullptr ? AttributeValueEncoder::AttributeEncodeState() : *apEncoderState); + AttributeEncodeState state; + if (apEncoderState == nullptr) + { + state = *apEncoderState; + } AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor.fabricIndex, aPath, kDataVersion /* data version */, aIsFabricFiltered, state); @@ -141,8 +147,11 @@ CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescr if (aPath.mClusterId == app::Clusters::UnitTesting::Id && aPath.mAttributeId == app::Clusters::UnitTesting::Attributes::Int16u::Id) { - AttributeValueEncoder::AttributeEncodeState state = - (apEncoderState == nullptr ? AttributeValueEncoder::AttributeEncodeState() : *apEncoderState); + AttributeEncodeState state; + if (apEncoderState == nullptr) + { + state = *apEncoderState; + } AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor.fabricIndex, aPath, kDataVersion /* data version */, aIsFabricFiltered, state); @@ -151,7 +160,7 @@ CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescr if (aPath.mClusterId == kPerpetualClusterId || (aPath.mClusterId == app::Clusters::UnitTesting::Id && aPath.mAttributeId == kPerpetualAttributeid)) { - AttributeValueEncoder::AttributeEncodeState state = AttributeValueEncoder::AttributeEncodeState(); + AttributeEncodeState state; AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor.fabricIndex, aPath, kDataVersion /* data version */, aIsFabricFiltered, state); @@ -174,8 +183,11 @@ CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescr if (aPath.mClusterId == app::Clusters::IcdManagement::Id && aPath.mAttributeId == app::Clusters::IcdManagement::Attributes::OperatingMode::Id) { - AttributeValueEncoder::AttributeEncodeState state = - (apEncoderState == nullptr ? AttributeValueEncoder::AttributeEncodeState() : *apEncoderState); + AttributeEncodeState state; + if (apEncoderState == nullptr) + { + state = *apEncoderState; + } AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor.fabricIndex, aPath, kDataVersion /* data version */, aIsFabricFiltered, state); From 4301461897cf0b278b1e9596bf22e1c71b1e62df Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 17 Apr 2024 12:03:09 -0400 Subject: [PATCH 03/16] Fix tests --- src/app/tests/TestAttributeValueEncoder.cpp | 24 ++++++++++++++++++-- src/app/tests/TestPowerSourceCluster.cpp | 3 ++- src/app/tests/TestReadInteraction.cpp | 2 +- src/app/util/mock/attribute-storage.cpp | 5 +++- src/controller/tests/data_model/TestRead.cpp | 20 ++++++++-------- 5 files changed, 39 insertions(+), 15 deletions(-) diff --git a/src/app/tests/TestAttributeValueEncoder.cpp b/src/app/tests/TestAttributeValueEncoder.cpp index b0c12bcb85dbfd..5b171d20d0f1cb 100644 --- a/src/app/tests/TestAttributeValueEncoder.cpp +++ b/src/app/tests/TestAttributeValueEncoder.cpp @@ -49,15 +49,35 @@ constexpr ClusterId kRandomClusterId = 0xaa; constexpr AttributeId kRandomAttributeId = 0xcc; constexpr DataVersion kRandomDataVersion = 0x99; constexpr FabricIndex kTestFabricIndex = 1; +constexpr NodeId kFakeNodeId = 1; constexpr TLV::Tag kFabricIndexTag = TLV::ContextTag(254); +Access::SubjectDescriptor DescriptorWithFabric(FabricIndex fabricIndex) +{ + Access::SubjectDescriptor result; + + result.fabricIndex = fabricIndex; + result.subject = kFakeNodeId; + + if (fabricIndex == kUndefinedFabricIndex) + { + result.authMode = Access::AuthMode::kCase; + } + else + { + result.authMode = Access::AuthMode::kPase; + } + return result; +} + template struct LimitedTestSetup { LimitedTestSetup(nlTestSuite * aSuite, const FabricIndex aFabricIndex = kUndefinedFabricIndex, const AttributeEncodeState & aState = AttributeEncodeState()) : - encoder(builder, aFabricIndex, ConcreteAttributePath(kRandomEndpointId, kRandomClusterId, kRandomAttributeId), - kRandomDataVersion, aFabricIndex != kUndefinedFabricIndex, aState) + encoder(builder, DescriptorWithFabric(aFabricIndex), + ConcreteAttributePath(kRandomEndpointId, kRandomClusterId, kRandomAttributeId), kRandomDataVersion, + aFabricIndex != kUndefinedFabricIndex, aState) { writer.Init(buf); { diff --git a/src/app/tests/TestPowerSourceCluster.cpp b/src/app/tests/TestPowerSourceCluster.cpp index 46371b332a1804..374b9619d38684 100644 --- a/src/app/tests/TestPowerSourceCluster.cpp +++ b/src/app/tests/TestPowerSourceCluster.cpp @@ -80,7 +80,8 @@ std::vector ReadEndpointsThroughAttributeReader(nlTestSuite * apSuit ConcreteAttributePath path(endpoint, Clusters::PowerSource::Id, Clusters::PowerSource::Attributes::EndpointList::Id); ConcreteReadAttributePath readPath(path); chip::DataVersion dataVersion(0); - AttributeValueEncoder aEncoder(builder, 0, path, dataVersion); + Access::SubjectDescriptor subjectDescriptor; + AttributeValueEncoder aEncoder(builder, subjectDescriptor, path, dataVersion); err = attrAccess.Read(readPath, aEncoder); diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp index ed00fee99caa47..8173761b6ca83a 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -331,7 +331,7 @@ CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescr return attributeReport.EndOfAttributeReportIB(); } - return AttributeValueEncoder(aAttributeReports, 0, aPath, 0).Encode(kTestFieldValue1); + return AttributeValueEncoder(aAttributeReports, aSubjectDescriptor, aPath, 0 /* dataVersion */).Encode(kTestFieldValue1); } bool IsClusterDataVersionEqual(const ConcreteClusterPath & aConcreteClusterPath, DataVersion aRequiredVersion) diff --git a/src/app/util/mock/attribute-storage.cpp b/src/app/util/mock/attribute-storage.cpp index 41b14a6181ecbc..87cf013952eb57 100644 --- a/src/app/util/mock/attribute-storage.cpp +++ b/src/app/util/mock/attribute-storage.cpp @@ -351,7 +351,10 @@ CHIP_ERROR ReadSingleMockClusterData(FabricIndex aAccessingFabricIndex, const Co if (aPath.mAttributeId == MockAttributeId(4)) { AttributeEncodeState state = (apEncoderState == nullptr ? AttributeEncodeState() : *apEncoderState); - AttributeValueEncoder valueEncoder(aAttributeReports, aAccessingFabricIndex, aPath, dataVersion, false, state); + Access::SubjectDescriptor subject; + subject.fabricIndex = aAccessingFabricIndex; + + AttributeValueEncoder valueEncoder(aAttributeReports, subject, aPath, dataVersion, false /* isFabricFiltered */, state); CHIP_ERROR err = valueEncoder.EncodeList([](const auto & encoder) -> CHIP_ERROR { for (int i = 0; i < 6; i++) diff --git a/src/controller/tests/data_model/TestRead.cpp b/src/controller/tests/data_model/TestRead.cpp index d6a0c4cb794e38..3a1426b64a675b 100644 --- a/src/controller/tests/data_model/TestRead.cpp +++ b/src/controller/tests/data_model/TestRead.cpp @@ -114,8 +114,8 @@ CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescr { state = *apEncoderState; } - AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor.fabricIndex, path, - kDataVersion /* data version */, aIsFabricFiltered, state); + AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor, path, kDataVersion /* data version */, + aIsFabricFiltered, state); ReturnErrorOnFailure(valueEncoder.Encode(true)); } @@ -132,8 +132,8 @@ CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescr { state = *apEncoderState; } - AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor.fabricIndex, aPath, - kDataVersion /* data version */, aIsFabricFiltered, state); + AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor, aPath, kDataVersion /* data version */, + aIsFabricFiltered, state); return valueEncoder.EncodeList([aSubjectDescriptor](const auto & encoder) -> CHIP_ERROR { app::Clusters::UnitTesting::Structs::TestFabricScoped::Type val; @@ -152,8 +152,8 @@ CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescr { state = *apEncoderState; } - AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor.fabricIndex, aPath, - kDataVersion /* data version */, aIsFabricFiltered, state); + AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor, aPath, kDataVersion /* data version */, + aIsFabricFiltered, state); return valueEncoder.Encode(++totalReadCount); } @@ -161,8 +161,8 @@ CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescr (aPath.mClusterId == app::Clusters::UnitTesting::Id && aPath.mAttributeId == kPerpetualAttributeid)) { AttributeEncodeState state; - AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor.fabricIndex, aPath, - kDataVersion /* data version */, aIsFabricFiltered, state); + AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor, aPath, kDataVersion /* data version */, + aIsFabricFiltered, state); CHIP_ERROR err = valueEncoder.EncodeList([](const auto & encoder) -> CHIP_ERROR { encoder.Encode(static_cast(1)); @@ -188,8 +188,8 @@ CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescr { state = *apEncoderState; } - AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor.fabricIndex, aPath, - kDataVersion /* data version */, aIsFabricFiltered, state); + AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor, aPath, kDataVersion /* data version */, + aIsFabricFiltered, state); return valueEncoder.Encode(isLitIcd ? Clusters::IcdManagement::OperatingModeEnum::kLit : Clusters::IcdManagement::OperatingModeEnum::kSit); From 361e1e5089a815448357f6543e0291a4c24c8adc Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 17 Apr 2024 12:14:16 -0400 Subject: [PATCH 04/16] Slight move around based on pahole results, to try to make size differences a bit smaller --- src/app/AttributeValueEncoder.h | 4 ++-- src/app/ConcreteAttributePath.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/AttributeValueEncoder.h b/src/app/AttributeValueEncoder.h index 8da2e2babdec05..581763d1b2de70 100644 --- a/src/app/AttributeValueEncoder.h +++ b/src/app/AttributeValueEncoder.h @@ -288,11 +288,11 @@ class AttributeValueEncoder */ void EnsureListEnded(); - bool mTriedEncode = false; AttributeReportIBs::Builder & mAttributeReportIBsBuilder; const Access::SubjectDescriptor mSubjectDescriptor; ConcreteDataAttributePath mPath; DataVersion mDataVersion; + bool mTriedEncode = false; bool mIsFabricFiltered = false; // mEncodingInitialList is true if we're encoding a list and we have not // started chunking it yet, so we're encoding a single attribute report IB @@ -300,8 +300,8 @@ class AttributeValueEncoder bool mEncodingInitialList = false; // mEncodedAtLeastOneListItem becomes true once we successfully encode a list item. bool mEncodedAtLeastOneListItem = false; - AttributeEncodeState mEncodeState; ListIndex mCurrentEncodingListIndex = kInvalidListIndex; + AttributeEncodeState mEncodeState; }; } // namespace app diff --git a/src/app/ConcreteAttributePath.h b/src/app/ConcreteAttributePath.h index cfde88ab9bd374..2a6f51cffd1326 100644 --- a/src/app/ConcreteAttributePath.h +++ b/src/app/ConcreteAttributePath.h @@ -101,7 +101,7 @@ struct ConcreteReadAttributePath : public ConcreteAttributePath */ struct ConcreteDataAttributePath : public ConcreteAttributePath { - enum class ListOperation + enum class ListOperation: uint8_t { NotList, // Path points to an attribute that isn't a list. ReplaceAll, // Path points to an attribute that is a list, indicating that the contents of the list should be replaced in From 0fa6b4d5eb375cface7f98bf09047cf55625e915 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 17 Apr 2024 12:19:10 -0400 Subject: [PATCH 05/16] Fix typo ... amazingly enough tests still pass, so test read is maybe broken --- src/controller/tests/data_model/TestRead.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/controller/tests/data_model/TestRead.cpp b/src/controller/tests/data_model/TestRead.cpp index 3a1426b64a675b..bed16bf9d69695 100644 --- a/src/controller/tests/data_model/TestRead.cpp +++ b/src/controller/tests/data_model/TestRead.cpp @@ -110,7 +110,7 @@ CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescr path.mAttributeId = static_cast(path.mAttributeId + (i / 2) + (responseDirective == kSendManyDataResponsesWrongPath)); AttributeEncodeState state; - if (apEncoderState == nullptr) + if (apEncoderState != nullptr) { state = *apEncoderState; } @@ -128,7 +128,7 @@ CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescr aPath.mAttributeId == app::Clusters::UnitTesting::Attributes::ListFabricScoped::Id) { AttributeEncodeState state; - if (apEncoderState == nullptr) + if (apEncoderState != nullptr) { state = *apEncoderState; } @@ -148,7 +148,7 @@ CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescr aPath.mAttributeId == app::Clusters::UnitTesting::Attributes::Int16u::Id) { AttributeEncodeState state; - if (apEncoderState == nullptr) + if (apEncoderState != nullptr) { state = *apEncoderState; } @@ -184,7 +184,7 @@ CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescr aPath.mAttributeId == app::Clusters::IcdManagement::Attributes::OperatingMode::Id) { AttributeEncodeState state; - if (apEncoderState == nullptr) + if (apEncoderState != nullptr) { state = *apEncoderState; } From ecf844b25babe52f0322252a664d464860404427 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 17 Apr 2024 12:21:45 -0400 Subject: [PATCH 06/16] Two more replacements for state compare logic --- src/app/util/ember-compatibility-functions.cpp | 9 +++++++-- src/app/util/mock/attribute-storage.cpp | 6 +++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/app/util/ember-compatibility-functions.cpp b/src/app/util/ember-compatibility-functions.cpp index 30cbb7e96c7ba0..b2b3d2dc929412 100644 --- a/src/app/util/ember-compatibility-functions.cpp +++ b/src/app/util/ember-compatibility-functions.cpp @@ -430,8 +430,13 @@ CHIP_ERROR ReadViaAccessInterface(SubjectDescriptor subjectDescriptor, bool aIsF AttributeEncodeState * aEncoderState, AttributeAccessInterface * aAccessInterface, bool * aTriedEncode) { - AttributeEncodeState state = (aEncoderState == nullptr ? AttributeEncodeState() : *aEncoderState); - DataVersion version = 0; + AttributeEncodeState state; + if (aEncoderState != nullptr) + { + state = *aEncoderState; + } + + DataVersion version = 0; ReturnErrorOnFailure(ReadClusterDataVersion(aPath, version)); AttributeValueEncoder valueEncoder(aAttributeReports, subjectDescriptor, aPath, version, aIsFabricFiltered, state); CHIP_ERROR err = aAccessInterface->Read(aPath, valueEncoder); diff --git a/src/app/util/mock/attribute-storage.cpp b/src/app/util/mock/attribute-storage.cpp index 87cf013952eb57..ce6b09ff4b83ea 100644 --- a/src/app/util/mock/attribute-storage.cpp +++ b/src/app/util/mock/attribute-storage.cpp @@ -350,7 +350,11 @@ CHIP_ERROR ReadSingleMockClusterData(FabricIndex aAccessingFabricIndex, const Co // Attribute 4 acts as a large attribute to trigger chunking. if (aPath.mAttributeId == MockAttributeId(4)) { - AttributeEncodeState state = (apEncoderState == nullptr ? AttributeEncodeState() : *apEncoderState); + AttributeEncodeState state; + if (apEncoderState != nullptr) + { + state = *apEncoderState; + } Access::SubjectDescriptor subject; subject.fabricIndex = aAccessingFabricIndex; From eb410b4201a4aa991a491b7b5390e03c3da9a1c1 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 17 Apr 2024 12:23:34 -0400 Subject: [PATCH 07/16] Restyle --- src/app/AttributeValueEncoder.h | 4 ++-- src/app/ConcreteAttributePath.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/AttributeValueEncoder.h b/src/app/AttributeValueEncoder.h index 581763d1b2de70..2b931ab1b568f5 100644 --- a/src/app/AttributeValueEncoder.h +++ b/src/app/AttributeValueEncoder.h @@ -292,14 +292,14 @@ class AttributeValueEncoder const Access::SubjectDescriptor mSubjectDescriptor; ConcreteDataAttributePath mPath; DataVersion mDataVersion; - bool mTriedEncode = false; + bool mTriedEncode = false; bool mIsFabricFiltered = false; // mEncodingInitialList is true if we're encoding a list and we have not // started chunking it yet, so we're encoding a single attribute report IB // for the whole list, not one per item. bool mEncodingInitialList = false; // mEncodedAtLeastOneListItem becomes true once we successfully encode a list item. - bool mEncodedAtLeastOneListItem = false; + bool mEncodedAtLeastOneListItem = false; ListIndex mCurrentEncodingListIndex = kInvalidListIndex; AttributeEncodeState mEncodeState; }; diff --git a/src/app/ConcreteAttributePath.h b/src/app/ConcreteAttributePath.h index 2a6f51cffd1326..c99c936c2867ce 100644 --- a/src/app/ConcreteAttributePath.h +++ b/src/app/ConcreteAttributePath.h @@ -101,7 +101,7 @@ struct ConcreteReadAttributePath : public ConcreteAttributePath */ struct ConcreteDataAttributePath : public ConcreteAttributePath { - enum class ListOperation: uint8_t + enum class ListOperation : uint8_t { NotList, // Path points to an attribute that isn't a list. ReplaceAll, // Path points to an attribute that is a list, indicating that the contents of the list should be replaced in From 2ca06cf8b986b9b97b7ce345a4078bab7925d12c Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 17 Apr 2024 14:53:01 -0400 Subject: [PATCH 08/16] Fix logic error in Descripto creation for test --- src/app/tests/TestAttributeValueEncoder.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/app/tests/TestAttributeValueEncoder.cpp b/src/app/tests/TestAttributeValueEncoder.cpp index 5b171d20d0f1cb..085c4f870cdf64 100644 --- a/src/app/tests/TestAttributeValueEncoder.cpp +++ b/src/app/tests/TestAttributeValueEncoder.cpp @@ -61,11 +61,13 @@ Access::SubjectDescriptor DescriptorWithFabric(FabricIndex fabricIndex) if (fabricIndex == kUndefinedFabricIndex) { - result.authMode = Access::AuthMode::kCase; + // Make it seem somewhat valid: a fabric index is not available in PASE + // before AddNOC + result.authMode = Access::AuthMode::kPase; } else { - result.authMode = Access::AuthMode::kPase; + result.authMode = Access::AuthMode::kCase; } return result; } From 7c671eedd833bbe1355cf1cc0adb13e8f290686c Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 17 Apr 2024 14:55:05 -0400 Subject: [PATCH 09/16] Update argument name --- src/app/util/mock/attribute-storage.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/util/mock/attribute-storage.cpp b/src/app/util/mock/attribute-storage.cpp index ce6b09ff4b83ea..b7f93ed0da301a 100644 --- a/src/app/util/mock/attribute-storage.cpp +++ b/src/app/util/mock/attribute-storage.cpp @@ -358,7 +358,7 @@ CHIP_ERROR ReadSingleMockClusterData(FabricIndex aAccessingFabricIndex, const Co Access::SubjectDescriptor subject; subject.fabricIndex = aAccessingFabricIndex; - AttributeValueEncoder valueEncoder(aAttributeReports, subject, aPath, dataVersion, false /* isFabricFiltered */, state); + AttributeValueEncoder valueEncoder(aAttributeReports, subject, aPath, dataVersion, /* aIsFabricFiltered = */ false, state); CHIP_ERROR err = valueEncoder.EncodeList([](const auto & encoder) -> CHIP_ERROR { for (int i = 0; i < 6; i++) From 8ae866fc6962dd0370c66e2d25eb2db3fa451f0f Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 17 Apr 2024 14:57:05 -0400 Subject: [PATCH 10/16] Remove useless comments --- src/controller/tests/data_model/TestRead.cpp | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/controller/tests/data_model/TestRead.cpp b/src/controller/tests/data_model/TestRead.cpp index bed16bf9d69695..4413e619fe6395 100644 --- a/src/controller/tests/data_model/TestRead.cpp +++ b/src/controller/tests/data_model/TestRead.cpp @@ -114,8 +114,7 @@ CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescr { state = *apEncoderState; } - AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor, path, kDataVersion /* data version */, - aIsFabricFiltered, state); + AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor, path, kDataVersion, aIsFabricFiltered, state); ReturnErrorOnFailure(valueEncoder.Encode(true)); } @@ -132,8 +131,8 @@ CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescr { state = *apEncoderState; } - AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor, aPath, kDataVersion /* data version */, - aIsFabricFiltered, state); + AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor, aPath, kDataVersion, aIsFabricFiltered, + state); return valueEncoder.EncodeList([aSubjectDescriptor](const auto & encoder) -> CHIP_ERROR { app::Clusters::UnitTesting::Structs::TestFabricScoped::Type val; @@ -152,8 +151,8 @@ CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescr { state = *apEncoderState; } - AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor, aPath, kDataVersion /* data version */, - aIsFabricFiltered, state); + AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor, aPath, kDataVersion, aIsFabricFiltered, + state); return valueEncoder.Encode(++totalReadCount); } @@ -161,8 +160,8 @@ CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescr (aPath.mClusterId == app::Clusters::UnitTesting::Id && aPath.mAttributeId == kPerpetualAttributeid)) { AttributeEncodeState state; - AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor, aPath, kDataVersion /* data version */, - aIsFabricFiltered, state); + AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor, aPath, kDataVersion, aIsFabricFiltered, + state); CHIP_ERROR err = valueEncoder.EncodeList([](const auto & encoder) -> CHIP_ERROR { encoder.Encode(static_cast(1)); @@ -188,8 +187,8 @@ CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescr { state = *apEncoderState; } - AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor, aPath, kDataVersion /* data version */, - aIsFabricFiltered, state); + AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor, aPath, kDataVersion, aIsFabricFiltered, + state); return valueEncoder.Encode(isLitIcd ? Clusters::IcdManagement::OperatingModeEnum::kLit : Clusters::IcdManagement::OperatingModeEnum::kSit); From 4a710230a4aa6c9a732f27bad3d24b33cb483552 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 17 Apr 2024 14:59:51 -0400 Subject: [PATCH 11/16] move AttributeEncodeState to a separate file --- src/app/AttributeEncodeState.h | 79 +++++++++++++++++++++++++++++++++ src/app/AttributeValueEncoder.h | 55 +---------------------- src/app/BUILD.gn | 3 +- 3 files changed, 82 insertions(+), 55 deletions(-) create mode 100644 src/app/AttributeEncodeState.h diff --git a/src/app/AttributeEncodeState.h b/src/app/AttributeEncodeState.h new file mode 100644 index 00000000000000..762faf8416d75d --- /dev/null +++ b/src/app/AttributeEncodeState.h @@ -0,0 +1,79 @@ +/* + * Copyright (c) 2021-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 + +namespace chip { +namespace app { + +/** + * Maintains the internal state of list encoding + */ +class AttributeEncodeState +{ +public: + AttributeEncodeState() = default; + + bool AllowPartialData() const { return mAllowPartialData; } + ListIndex CurrentEncodingListIndex() const { return mCurrentEncodingListIndex; } + + AttributeEncodeState & SetAllowPartialData(bool allow) + { + mAllowPartialData = allow; + return *this; + } + + AttributeEncodeState & SetCurrentEncodingListIndex(ListIndex idx) + { + mCurrentEncodingListIndex = idx; + return *this; + } + + void Reset() + { + mCurrentEncodingListIndex = kInvalidListIndex; + mAllowPartialData = false; + } + +private: + /** + * If set to kInvalidListIndex, indicates that we have not encoded any data for the list yet and + * need to start by encoding an empty list before we start encoding any list items. + * + * When set to a valid ListIndex value, indicates the index of the next list item that needs to be + * encoded (i.e. the count of items encoded so far). + */ + ListIndex mCurrentEncodingListIndex = kInvalidListIndex; + + /** + * When an attempt to encode an attribute returns an error, the buffer may contain tailing dirty data + * (since the put was aborted). The report engine normally rolls back the buffer to right before encoding + * of the attribute started on errors. + * + * When chunking a list, EncodeListItem will atomically encode list items, ensuring that the + * state of the buffer is valid to send (i.e. contains no trailing garbage), and return an error + * if the list doesn't entirely fit. In this situation, mAllowPartialData is set to communicate to the + * report engine that it should not roll back the list items. + * + * TODO: There might be a better name for this variable. + */ + bool mAllowPartialData = false; +}; + +} // namespace app +} // namespace chip diff --git a/src/app/AttributeValueEncoder.h b/src/app/AttributeValueEncoder.h index 2b931ab1b568f5..c2593c9ccecd20 100644 --- a/src/app/AttributeValueEncoder.h +++ b/src/app/AttributeValueEncoder.h @@ -18,6 +18,7 @@ #include #include +#include #include #include #include @@ -28,60 +29,6 @@ namespace chip { namespace app { -/** - * Maintains the internal state of list encoding - */ -class AttributeEncodeState -{ -public: - AttributeEncodeState() = default; - - bool AllowPartialData() const { return mAllowPartialData; } - ListIndex CurrentEncodingListIndex() const { return mCurrentEncodingListIndex; } - - AttributeEncodeState & SetAllowPartialData(bool allow) - { - mAllowPartialData = allow; - return *this; - } - - AttributeEncodeState & SetCurrentEncodingListIndex(ListIndex idx) - { - mCurrentEncodingListIndex = idx; - return *this; - } - - void Reset() - { - mCurrentEncodingListIndex = kInvalidListIndex; - mAllowPartialData = false; - } - -private: - /** - * If set to kInvalidListIndex, indicates that we have not encoded any data for the list yet and - * need to start by encoding an empty list before we start encoding any list items. - * - * When set to a valid ListIndex value, indicates the index of the next list item that needs to be - * encoded (i.e. the count of items encoded so far). - */ - ListIndex mCurrentEncodingListIndex = kInvalidListIndex; - - /** - * When an attempt to encode an attribute returns an error, the buffer may contain tailing dirty data - * (since the put was aborted). The report engine normally rolls back the buffer to right before encoding - * of the attribute started on errors. - * - * When chunking a list, EncodeListItem will atomically encode list items, ensuring that the - * state of the buffer is valid to send (i.e. contains no trailing garbage), and return an error - * if the list doesn't entirely fit. In this situation, mAllowPartialData is set to communicate to the - * report engine that it should not roll back the list items. - * - * TODO: There might be a better name for this variable. - */ - bool mAllowPartialData = false; -}; - /** * The AttributeValueEncoder is a helper class for filling report payloads into AttributeReportIBs. * The attribute value encoder can be initialized with a AttributeEncodeState for saving and recovering its state between encode diff --git a/src/app/BUILD.gn b/src/app/BUILD.gn index 042d3ba92281d4..5534fb13ce05bb 100644 --- a/src/app/BUILD.gn +++ b/src/app/BUILD.gn @@ -277,10 +277,11 @@ source_set("events") { static_library("attribute-access") { sources = [ - "AttributeAccessInterface.h", "AttributeAccessInterfaceCache.h", + "AttributeAccessInterface.h", "AttributeAccessInterfaceRegistry.cpp", "AttributeAccessInterfaceRegistry.h", + "AttributeEncodeState.h", "AttributeReportBuilder.cpp", "AttributeReportBuilder.h", "AttributeValueDecoder.h", From c9024ed895aef34ec689e56fb69ffde894fa084a Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 17 Apr 2024 15:07:58 -0400 Subject: [PATCH 12/16] Make the copy from a pointer logic cleaner --- src/app/AttributeEncodeState.h | 30 +++++++++++++++++-- src/app/AttributeValueEncoder.h | 4 +-- src/app/BUILD.gn | 2 +- .../util/ember-compatibility-functions.cpp | 7 +---- src/app/util/mock/attribute-storage.cpp | 6 +--- src/controller/tests/data_model/TestRead.cpp | 24 +++------------ 6 files changed, 36 insertions(+), 37 deletions(-) diff --git a/src/app/AttributeEncodeState.h b/src/app/AttributeEncodeState.h index 762faf8416d75d..f91c0081a7a92c 100644 --- a/src/app/AttributeEncodeState.h +++ b/src/app/AttributeEncodeState.h @@ -21,14 +21,38 @@ namespace chip { namespace app { -/** - * Maintains the internal state of list encoding - */ +/// Maintains the internal state of list encoding +/// +/// List encoding is generally assumed incremental and chunkable (i.e. +/// partial encoding is ok.). For this purpose the class maintains two +/// pieces of data: +/// - AllowPartialData tracks if partial encoding is acceptable in the +/// current encoding state (to be used for atomic/non-atomic list item writes) +/// - CurrentEncodingListIndex representing the list index that is next +/// to be encoded in the output. kInvalidListIndex means that a new list +/// encoding has been started. class AttributeEncodeState { public: AttributeEncodeState() = default; + /// Allows the encode state to be initialized from an OPTIONAL + /// other encoding state + /// + /// if other is nullptr, this is the same as the default initializer. + AttributeEncodeState(AttributeEncodeState * other) + { + if (other != nullptr) + { + *this = *other; + } + else + { + mCurrentEncodingListIndex = kInvalidListIndex; + mAllowPartialData = false; + } + } + bool AllowPartialData() const { return mAllowPartialData; } ListIndex CurrentEncodingListIndex() const { return mCurrentEncodingListIndex; } diff --git a/src/app/AttributeValueEncoder.h b/src/app/AttributeValueEncoder.h index c2593c9ccecd20..52999e912dbd24 100644 --- a/src/app/AttributeValueEncoder.h +++ b/src/app/AttributeValueEncoder.h @@ -17,8 +17,8 @@ #pragma once #include -#include #include +#include #include #include #include @@ -141,7 +141,7 @@ class AttributeValueEncoder if (err == CHIP_NO_ERROR) { // The Encode procedure finished without any error, clear the state. - mEncodeState = AttributeEncodeState(); + mEncodeState.Reset(); } return err; } diff --git a/src/app/BUILD.gn b/src/app/BUILD.gn index 5534fb13ce05bb..e40847403cd01f 100644 --- a/src/app/BUILD.gn +++ b/src/app/BUILD.gn @@ -277,8 +277,8 @@ source_set("events") { static_library("attribute-access") { sources = [ - "AttributeAccessInterfaceCache.h", "AttributeAccessInterface.h", + "AttributeAccessInterfaceCache.h", "AttributeAccessInterfaceRegistry.cpp", "AttributeAccessInterfaceRegistry.h", "AttributeEncodeState.h", diff --git a/src/app/util/ember-compatibility-functions.cpp b/src/app/util/ember-compatibility-functions.cpp index b2b3d2dc929412..392404d084d8a6 100644 --- a/src/app/util/ember-compatibility-functions.cpp +++ b/src/app/util/ember-compatibility-functions.cpp @@ -430,12 +430,7 @@ CHIP_ERROR ReadViaAccessInterface(SubjectDescriptor subjectDescriptor, bool aIsF AttributeEncodeState * aEncoderState, AttributeAccessInterface * aAccessInterface, bool * aTriedEncode) { - AttributeEncodeState state; - if (aEncoderState != nullptr) - { - state = *aEncoderState; - } - + AttributeEncodeState state(aEncoderState); DataVersion version = 0; ReturnErrorOnFailure(ReadClusterDataVersion(aPath, version)); AttributeValueEncoder valueEncoder(aAttributeReports, subjectDescriptor, aPath, version, aIsFabricFiltered, state); diff --git a/src/app/util/mock/attribute-storage.cpp b/src/app/util/mock/attribute-storage.cpp index b7f93ed0da301a..2293a48a8403e4 100644 --- a/src/app/util/mock/attribute-storage.cpp +++ b/src/app/util/mock/attribute-storage.cpp @@ -350,11 +350,7 @@ CHIP_ERROR ReadSingleMockClusterData(FabricIndex aAccessingFabricIndex, const Co // Attribute 4 acts as a large attribute to trigger chunking. if (aPath.mAttributeId == MockAttributeId(4)) { - AttributeEncodeState state; - if (apEncoderState != nullptr) - { - state = *apEncoderState; - } + AttributeEncodeState state(apEncoderState); Access::SubjectDescriptor subject; subject.fabricIndex = aAccessingFabricIndex; diff --git a/src/controller/tests/data_model/TestRead.cpp b/src/controller/tests/data_model/TestRead.cpp index 4413e619fe6395..bcf0233662d2bc 100644 --- a/src/controller/tests/data_model/TestRead.cpp +++ b/src/controller/tests/data_model/TestRead.cpp @@ -109,11 +109,7 @@ CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescr // Use an incorrect attribute id for some of the responses. path.mAttributeId = static_cast(path.mAttributeId + (i / 2) + (responseDirective == kSendManyDataResponsesWrongPath)); - AttributeEncodeState state; - if (apEncoderState != nullptr) - { - state = *apEncoderState; - } + AttributeEncodeState state(apEncoderState); AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor, path, kDataVersion, aIsFabricFiltered, state); ReturnErrorOnFailure(valueEncoder.Encode(true)); } @@ -126,11 +122,7 @@ CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescr if (aPath.mClusterId == app::Clusters::UnitTesting::Id && aPath.mAttributeId == app::Clusters::UnitTesting::Attributes::ListFabricScoped::Id) { - AttributeEncodeState state; - if (apEncoderState != nullptr) - { - state = *apEncoderState; - } + AttributeEncodeState state(apEncoderState); AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor, aPath, kDataVersion, aIsFabricFiltered, state); @@ -146,11 +138,7 @@ CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescr if (aPath.mClusterId == app::Clusters::UnitTesting::Id && aPath.mAttributeId == app::Clusters::UnitTesting::Attributes::Int16u::Id) { - AttributeEncodeState state; - if (apEncoderState != nullptr) - { - state = *apEncoderState; - } + AttributeEncodeState state(apEncoderState); AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor, aPath, kDataVersion, aIsFabricFiltered, state); @@ -182,11 +170,7 @@ CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescr if (aPath.mClusterId == app::Clusters::IcdManagement::Id && aPath.mAttributeId == app::Clusters::IcdManagement::Attributes::OperatingMode::Id) { - AttributeEncodeState state; - if (apEncoderState != nullptr) - { - state = *apEncoderState; - } + AttributeEncodeState state(apEncoderState); AttributeValueEncoder valueEncoder(aAttributeReports, aSubjectDescriptor, aPath, kDataVersion, aIsFabricFiltered, state); From 9b305dbcf672d3dd3501549e9124286fdedda111 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 17 Apr 2024 15:08:54 -0400 Subject: [PATCH 13/16] Fix value passing --- src/app/util/ember-compatibility-functions.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/util/ember-compatibility-functions.cpp b/src/app/util/ember-compatibility-functions.cpp index 392404d084d8a6..a9bd1c5cb203fa 100644 --- a/src/app/util/ember-compatibility-functions.cpp +++ b/src/app/util/ember-compatibility-functions.cpp @@ -425,7 +425,7 @@ CHIP_ERROR GlobalAttributeReader::EncodeCommandList(const ConcreteClusterPath & // Helper function for trying to read an attribute value via an // AttributeAccessInterface. On failure, the read has failed. On success, the // aTriedEncode outparam is set to whether the AttributeAccessInterface tried to encode a value. -CHIP_ERROR ReadViaAccessInterface(SubjectDescriptor subjectDescriptor, bool aIsFabricFiltered, +CHIP_ERROR ReadViaAccessInterface(const SubjectDescriptor &subjectDescriptor, bool aIsFabricFiltered, const ConcreteReadAttributePath & aPath, AttributeReportIBs::Builder & aAttributeReports, AttributeEncodeState * aEncoderState, AttributeAccessInterface * aAccessInterface, bool * aTriedEncode) From 7a9b56cf6993e45548462654a9d819caf1e17b07 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 17 Apr 2024 15:10:24 -0400 Subject: [PATCH 14/16] Fix const correctness --- src/app/AttributeEncodeState.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/AttributeEncodeState.h b/src/app/AttributeEncodeState.h index f91c0081a7a92c..271e16235f0947 100644 --- a/src/app/AttributeEncodeState.h +++ b/src/app/AttributeEncodeState.h @@ -40,7 +40,7 @@ class AttributeEncodeState /// other encoding state /// /// if other is nullptr, this is the same as the default initializer. - AttributeEncodeState(AttributeEncodeState * other) + AttributeEncodeState(const AttributeEncodeState * other) { if (other != nullptr) { From ff78944430c3e490a53a73f9c2c8727c1298f0e3 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 17 Apr 2024 15:39:09 -0400 Subject: [PATCH 15/16] Fix one more compile --- src/app/tests/integration/chip_im_responder.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/app/tests/integration/chip_im_responder.cpp b/src/app/tests/integration/chip_im_responder.cpp index 880e9e75f50ef4..72f056ca6ab8b7 100644 --- a/src/app/tests/integration/chip_im_responder.cpp +++ b/src/app/tests/integration/chip_im_responder.cpp @@ -123,8 +123,7 @@ CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescr const ConcreteReadAttributePath & aPath, AttributeReportIBs::Builder & aAttributeReports, AttributeEncodeState * apEncoderState) { - ReturnErrorOnFailure(AttributeValueEncoder(aAttributeReports, 0, aPath, 0).Encode(kTestFieldValue1)); - return CHIP_NO_ERROR; + return AttributeValueEncoder(aAttributeReports, aSubjectDescriptor, aPath, 0).Encode(kTestFieldValue1); } bool ConcreteAttributePathExists(const ConcreteAttributePath & aPath) From e1d22c67ecfa75ce15b3a3aa91899401da71f5a0 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 18 Apr 2024 09:18:25 -0400 Subject: [PATCH 16/16] Put back AccessingFabricIndex --- src/app/AttributeValueDecoder.h | 9 ++++----- src/app/AttributeValueEncoder.h | 7 ++++--- .../access-control-server/access-control-server.cpp | 4 ++-- src/app/clusters/bindings/bindings.cpp | 2 +- .../group-key-mgmt-server/group-key-mgmt-server.cpp | 2 +- .../operational-credentials-server.cpp | 4 ++-- src/app/clusters/ota-requestor/ota-requestor-server.cpp | 2 +- .../clusters/test-cluster-server/test-cluster-server.cpp | 2 +- src/app/util/ember-compatibility-functions.cpp | 2 +- 9 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/app/AttributeValueDecoder.h b/src/app/AttributeValueDecoder.h index 93f48fd29a4ece..9776c911671fda 100644 --- a/src/app/AttributeValueDecoder.h +++ b/src/app/AttributeValueDecoder.h @@ -46,19 +46,18 @@ class AttributeValueDecoder mTriedDecode = true; // The WriteRequest comes with no fabric index, this will happen when receiving a write request on a PASE session before // AddNOC. - VerifyOrReturnError(GetSubjectDescriptor().fabricIndex != kUndefinedFabricIndex, CHIP_IM_GLOBAL_STATUS(UnsupportedAccess)); + VerifyOrReturnError(AccessingFabricIndex() != kUndefinedFabricIndex, CHIP_IM_GLOBAL_STATUS(UnsupportedAccess)); ReturnErrorOnFailure(DataModel::Decode(mReader, aArg)); - aArg.SetFabricIndex(GetSubjectDescriptor().fabricIndex); + aArg.SetFabricIndex(AccessingFabricIndex()); return CHIP_NO_ERROR; } bool TriedDecode() const { return mTriedDecode; } - /** - * The accessing subject descriptor for this write interaction. - */ const Access::SubjectDescriptor & GetSubjectDescriptor() const { return mSubjectDescriptor; } + FabricIndex AccessingFabricIndex() const { return GetSubjectDescriptor().fabricIndex; } + private: TLV::TLVReader & mReader; bool mTriedDecode = false; diff --git a/src/app/AttributeValueEncoder.h b/src/app/AttributeValueEncoder.h index 52999e912dbd24..79ee5953a2ca6b 100644 --- a/src/app/AttributeValueEncoder.h +++ b/src/app/AttributeValueEncoder.h @@ -53,10 +53,9 @@ class AttributeValueEncoder // If we are encoding for a fabric filtered attribute read and the fabric index does not match that present in the // request, skip encoding this list item. VerifyOrReturnError(!mAttributeValueEncoder.mIsFabricFiltered || - aArg.GetFabricIndex() == mAttributeValueEncoder.GetSubjectDescriptor().fabricIndex, + aArg.GetFabricIndex() == mAttributeValueEncoder.AccessingFabricIndex(), CHIP_NO_ERROR); - return mAttributeValueEncoder.EncodeListItem(mAttributeValueEncoder.GetSubjectDescriptor().fabricIndex, - std::forward(aArg)); + return mAttributeValueEncoder.EncodeListItem(mAttributeValueEncoder.AccessingFabricIndex(), std::forward(aArg)); } template ::value, bool> = true> @@ -150,6 +149,8 @@ class AttributeValueEncoder const Access::SubjectDescriptor & GetSubjectDescriptor() const { return mSubjectDescriptor; } + FabricIndex AccessingFabricIndex() const { return GetSubjectDescriptor().fabricIndex; } + /** * AttributeValueEncoder is a short lived object, and the state is persisted by mEncodeState and restored by constructor. */ diff --git a/src/app/clusters/access-control-server/access-control-server.cpp b/src/app/clusters/access-control-server/access-control-server.cpp index b84f8dc084f0ee..316825bc91848c 100644 --- a/src/app/clusters/access-control-server/access-control-server.cpp +++ b/src/app/clusters/access-control-server/access-control-server.cpp @@ -230,7 +230,7 @@ CHIP_ERROR AccessControlAttribute::WriteImpl(const ConcreteDataAttributePath & a CHIP_ERROR AccessControlAttribute::WriteAcl(const ConcreteDataAttributePath & aPath, AttributeValueDecoder & aDecoder) { - FabricIndex accessingFabricIndex = aDecoder.GetSubjectDescriptor().fabricIndex; + FabricIndex accessingFabricIndex = aDecoder.AccessingFabricIndex(); size_t oldCount; ReturnErrorOnFailure(GetAccessControl().GetEntryCount(accessingFabricIndex, oldCount)); @@ -293,7 +293,7 @@ CHIP_ERROR AccessControlAttribute::WriteExtension(const ConcreteDataAttributePat { auto & storage = Server::GetInstance().GetPersistentStorage(); - FabricIndex accessingFabricIndex = aDecoder.GetSubjectDescriptor().fabricIndex; + FabricIndex accessingFabricIndex = aDecoder.AccessingFabricIndex(); uint8_t buffer[kExtensionDataMaxLength] = { 0 }; uint16_t size = static_cast(sizeof(buffer)); diff --git a/src/app/clusters/bindings/bindings.cpp b/src/app/clusters/bindings/bindings.cpp index 0fe00ef50fef9c..63b085d7ee6de4 100644 --- a/src/app/clusters/bindings/bindings.cpp +++ b/src/app/clusters/bindings/bindings.cpp @@ -200,7 +200,7 @@ void BindingTableAccess::OnListWriteEnd(const app::ConcreteAttributePath & aPath CHIP_ERROR BindingTableAccess::WriteBindingTable(const ConcreteDataAttributePath & path, AttributeValueDecoder & decoder) { - mAccessingFabricIndex = decoder.GetSubjectDescriptor().fabricIndex; + mAccessingFabricIndex = decoder.AccessingFabricIndex(); if (!path.IsListOperation() || path.mListOp == ConcreteDataAttributePath::ListOperation::ReplaceAll) { DecodableBindingListType newBindingList; diff --git a/src/app/clusters/group-key-mgmt-server/group-key-mgmt-server.cpp b/src/app/clusters/group-key-mgmt-server/group-key-mgmt-server.cpp index 368d6ff13cc5f2..c3cf09eb283f63 100644 --- a/src/app/clusters/group-key-mgmt-server/group-key-mgmt-server.cpp +++ b/src/app/clusters/group-key-mgmt-server/group-key-mgmt-server.cpp @@ -192,7 +192,7 @@ class GroupKeyManagementAttributeAccess : public AttributeAccessInterface CHIP_ERROR WriteGroupKeyMap(const ConcreteDataAttributePath & aPath, AttributeValueDecoder & aDecoder) { - auto fabric_index = aDecoder.GetSubjectDescriptor().fabricIndex; + auto fabric_index = aDecoder.AccessingFabricIndex(); auto provider = GetGroupDataProvider(); if (!aPath.IsListItemOperation()) diff --git a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp index 6338d04d8f7989..12fe1c43de0baf 100644 --- a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp +++ b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp @@ -117,7 +117,7 @@ class OperationalCredentialsAttrAccess : public AttributeAccessInterface CHIP_ERROR OperationalCredentialsAttrAccess::ReadNOCs(EndpointId endpoint, AttributeValueEncoder & aEncoder) { - auto accessingFabricIndex = aEncoder.GetSubjectDescriptor().fabricIndex; + auto accessingFabricIndex = aEncoder.AccessingFabricIndex(); return aEncoder.EncodeList([accessingFabricIndex](const auto & encoder) -> CHIP_ERROR { const auto & fabricTable = Server::GetInstance().GetFabricTable(); @@ -252,7 +252,7 @@ CHIP_ERROR OperationalCredentialsAttrAccess::Read(const ConcreteReadAttributePat return ReadRootCertificates(aPath.mEndpointId, aEncoder); } case Attributes::CurrentFabricIndex::Id: { - return aEncoder.Encode(aEncoder.GetSubjectDescriptor().fabricIndex); + return aEncoder.Encode(aEncoder.AccessingFabricIndex()); } default: break; diff --git a/src/app/clusters/ota-requestor/ota-requestor-server.cpp b/src/app/clusters/ota-requestor/ota-requestor-server.cpp index 47595242a4e32d..7bfbde1fa54ac5 100644 --- a/src/app/clusters/ota-requestor/ota-requestor-server.cpp +++ b/src/app/clusters/ota-requestor/ota-requestor-server.cpp @@ -120,7 +120,7 @@ CHIP_ERROR OtaSoftwareUpdateRequestorAttrAccess::WriteDefaultOtaProviders(const DataModel::DecodableList list; ReturnErrorOnFailure(aDecoder.Decode(list)); - ReturnErrorOnFailure(requestor->ClearDefaultOtaProviderList(aDecoder.GetSubjectDescriptor().fabricIndex)); + ReturnErrorOnFailure(requestor->ClearDefaultOtaProviderList(aDecoder.AccessingFabricIndex())); auto iter = list.begin(); while (iter.Next()) diff --git a/src/app/clusters/test-cluster-server/test-cluster-server.cpp b/src/app/clusters/test-cluster-server/test-cluster-server.cpp index dcd0a6722a8003..0d805347f792f9 100644 --- a/src/app/clusters/test-cluster-server/test-cluster-server.cpp +++ b/src/app/clusters/test-cluster-server/test-cluster-server.cpp @@ -661,7 +661,7 @@ CHIP_ERROR TestAttrAccess::WriteListFabricScopedAttribute(const ConcreteDataAttr size_t srcIndex = 0, dstIndex = 0; while (srcIndex < gListFabricScopedAttributeLen) { - if (gListFabricScopedAttributeValue[srcIndex].fabricIndex != aDecoder.GetSubjectDescriptor().fabricIndex) + if (gListFabricScopedAttributeValue[srcIndex].fabricIndex != aDecoder.AccessingFabricIndex()) { auto & dstEntry = gListFabricScopedAttributeValue[dstIndex]; auto & srcEntry = gListFabricScopedAttributeValue[srcIndex]; diff --git a/src/app/util/ember-compatibility-functions.cpp b/src/app/util/ember-compatibility-functions.cpp index a9bd1c5cb203fa..e87e0f564a5ff7 100644 --- a/src/app/util/ember-compatibility-functions.cpp +++ b/src/app/util/ember-compatibility-functions.cpp @@ -425,7 +425,7 @@ CHIP_ERROR GlobalAttributeReader::EncodeCommandList(const ConcreteClusterPath & // Helper function for trying to read an attribute value via an // AttributeAccessInterface. On failure, the read has failed. On success, the // aTriedEncode outparam is set to whether the AttributeAccessInterface tried to encode a value. -CHIP_ERROR ReadViaAccessInterface(const SubjectDescriptor &subjectDescriptor, bool aIsFabricFiltered, +CHIP_ERROR ReadViaAccessInterface(const SubjectDescriptor & subjectDescriptor, bool aIsFabricFiltered, const ConcreteReadAttributePath & aPath, AttributeReportIBs::Builder & aAttributeReports, AttributeEncodeState * aEncoderState, AttributeAccessInterface * aAccessInterface, bool * aTriedEncode)