From 1f9cff49d78681c779d63cb01011bb2cb95d7aa3 Mon Sep 17 00:00:00 2001 From: Kamil Kasperczyk Date: Thu, 9 Nov 2023 08:34:41 +0100 Subject: [PATCH] Revert "[nrf fromtree] Improve encoding of lists where first item can't fit in packet. (#28346)" This reverts commit 07bd870a0e4a96b1c3e06c8412a6cd2f6fe64e58. --- src/app/AttributeAccessInterface.cpp | 13 -- src/app/AttributeAccessInterface.h | 3 - src/app/tests/TestReadInteraction.cpp | 20 +-- src/controller/tests/TestReadChunking.cpp | 190 ++-------------------- 4 files changed, 27 insertions(+), 199 deletions(-) diff --git a/src/app/AttributeAccessInterface.cpp b/src/app/AttributeAccessInterface.cpp index f72c8b8779df53..0c5a85c2fe92f8 100644 --- a/src/app/AttributeAccessInterface.cpp +++ b/src/app/AttributeAccessInterface.cpp @@ -130,19 +130,6 @@ void AttributeValueEncoder::EnsureListEnded() AttributeReportBuilder builder; VerifyOrDie(builder.FinishAttribute(mAttributeReportIBsBuilder) == CHIP_NO_ERROR); - - if (!mEncodedAtLeastOneListItem) - { - // If we have not managed to encode any list items, we don't actually - // want to output the single "empty list" IB that will then be followed - // by one-IB-per-item in the next packet. Just have the reporting - // engine roll back our entire attribute and put us in the next packet. - // - // 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; - } } } // namespace app diff --git a/src/app/AttributeAccessInterface.h b/src/app/AttributeAccessInterface.h index 63b5c143e0c60e..2b16392e107fb1 100644 --- a/src/app/AttributeAccessInterface.h +++ b/src/app/AttributeAccessInterface.h @@ -292,7 +292,6 @@ class AttributeValueEncoder mCurrentEncodingListIndex++; mEncodeState.mCurrentEncodingListIndex++; - mEncodedAtLeastOneListItem = true; return CHIP_NO_ERROR; } @@ -341,8 +340,6 @@ class AttributeValueEncoder // 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; AttributeEncodeState mEncodeState; ListIndex mCurrentEncodingListIndex = kInvalidListIndex; }; diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp index f8d99245bb898a..d49a4fa61a5be7 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -1286,8 +1286,8 @@ void TestReadInteraction::TestSetDirtyBetweenChunks(nlTestSuite * apSuite, void !aPath.IsListItemOperation()) { mGotStartOfSecondReport = true; - // We always have data chunks, so go ahead to mark things - // dirty as needed. + // Wait for an actual data chunk. + return; } if (!mGotStartOfSecondReport) @@ -1897,16 +1897,14 @@ void TestReadInteraction::TestSubscribeWildcard(nlTestSuite * apSuite, void * ap NL_TEST_ASSERT(apSuite, delegate.mGotReport); - // Thus we should receive 29*2 + 4 + 6 = 68 attribute data in total. - - // When EventList is not enabled, the packet boundaries shift and for the first - // report for the list attribute we receive two of its items in the initial list, - // then 4 additional items. For the second report we receive 3 items in - // the initial list followed by 3 additional items. + // We have 29 attributes in our mock attribute storage. And we subscribed twice. + // And attribute 3/2/4 is a list with 6 elements and list chunking is + // applied to it, but the way the packet boundaries fall we get two of + // its items as a single list, followed by 4 more single items for one + // of our subscriptions, but every item as a separate IB for the other. // - // Thus we should receive 29*2 + 4 + 3 = 65 attribute data when the eventlist - // attribute is not available. - NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 65); + // Thus we should receive 29*2 + 4 + 6 = 68 attribute data in total. + NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 68); NL_TEST_ASSERT(apSuite, delegate.mNumArrayItems == 12); NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadHandlers(ReadHandler::InteractionType::Subscribe) == 1); NL_TEST_ASSERT(apSuite, engine->ActiveHandlerAt(0) != nullptr); diff --git a/src/controller/tests/TestReadChunking.cpp b/src/controller/tests/TestReadChunking.cpp index d98d0586180940..03a6c85a6f7c92 100644 --- a/src/controller/tests/TestReadChunking.cpp +++ b/src/controller/tests/TestReadChunking.cpp @@ -70,8 +70,6 @@ constexpr AttributeId kTestListAttribute = 6; constexpr AttributeId kTestBadAttribute = 7; // Reading this attribute will return CHIP_ERROR_NO_MEMORY but nothing is actually encoded. -constexpr int kListAttributeItems = 5; - class TestReadChunking { public: @@ -127,116 +125,6 @@ DECLARE_DYNAMIC_ENDPOINT(testEndpoint5, testEndpoint5Clusters); uint8_t sAnStringThatCanNeverFitIntoTheMTU[4096] = { 0 }; -// Buffered callback class that lets us count the number of attribute data IBs -// we receive. BufferedReadCallback has all its ReadClient::Callback bits -// private, so we can't just inherit from it and call our super-class functions. -class TestBufferedReadCallback : public ReadClient::Callback -{ -public: - TestBufferedReadCallback(ReadClient::Callback & aNextCallback) : mBufferedCallback(aNextCallback) {} - - // Workaround for all the methods on BufferedReadCallback being private. - ReadClient::Callback & NextCallback() { return *static_cast(&mBufferedCallback); } - - void OnReportBegin() override { NextCallback().OnReportBegin(); } - void OnReportEnd() override { NextCallback().OnReportEnd(); } - - void OnAttributeData(const ConcreteDataAttributePath & aPath, TLV::TLVReader * apData, const StatusIB & aStatus) override - { - if (apData) - { - ++mAttributeDataIBCount; - - TLV::TLVReader reader(*apData); - do - { - if (reader.GetType() != TLV::TLVType::kTLVType_Array) - { - // Not a list. - break; - } - - TLV::TLVType containerType; - CHIP_ERROR err = reader.EnterContainer(containerType); - if (err != CHIP_NO_ERROR) - { - mDecodingFailed = true; - break; - } - - err = reader.Next(); - if (err == CHIP_END_OF_TLV) - { - mSawEmptyList = true; - } - else if (err != CHIP_NO_ERROR) - { - mDecodingFailed = true; - break; - } - } while (false); - } - else - { - ++mAttributeStatusIBCount; - } - - NextCallback().OnAttributeData(aPath, apData, aStatus); - } - - void OnError(CHIP_ERROR aError) override { NextCallback().OnError(aError); } - - void OnEventData(const EventHeader & aEventHeader, TLV::TLVReader * apData, const StatusIB * apStatus) override - { - NextCallback().OnEventData(aEventHeader, apData, apStatus); - } - - void OnDone(ReadClient * apReadClient) override { NextCallback().OnDone(apReadClient); } - - void OnSubscriptionEstablished(SubscriptionId aSubscriptionId) override - { - NextCallback().OnSubscriptionEstablished(aSubscriptionId); - } - - CHIP_ERROR OnResubscriptionNeeded(ReadClient * apReadClient, CHIP_ERROR aTerminationCause) override - { - return NextCallback().OnResubscriptionNeeded(apReadClient, aTerminationCause); - } - - void OnDeallocatePaths(ReadPrepareParams && aReadPrepareParams) override - { - NextCallback().OnDeallocatePaths(std::move(aReadPrepareParams)); - } - - CHIP_ERROR OnUpdateDataVersionFilterList(DataVersionFilterIBs::Builder & aDataVersionFilterIBsBuilder, - const Span & aAttributePaths, - bool & aEncodedDataVersionList) override - { - return NextCallback().OnUpdateDataVersionFilterList(aDataVersionFilterIBsBuilder, aAttributePaths, aEncodedDataVersionList); - } - - CHIP_ERROR GetHighestReceivedEventNumber(Optional & aEventNumber) override - { - return NextCallback().GetHighestReceivedEventNumber(aEventNumber); - } - - void OnUnsolicitedMessageFromPublisher(ReadClient * apReadClient) override - { - NextCallback().OnUnsolicitedMessageFromPublisher(apReadClient); - } - - void OnCASESessionEstablished(const SessionHandle & aSession, ReadPrepareParams & aSubscriptionParams) override - { - NextCallback().OnCASESessionEstablished(aSession, aSubscriptionParams); - } - - BufferedReadCallback mBufferedCallback; - bool mSawEmptyList = false; - bool mDecodingFailed = false; - uint32_t mAttributeDataIBCount = 0; - uint32_t mAttributeStatusIBCount = 0; -}; - class TestReadCallback : public app::ReadClient::Callback { public: @@ -250,13 +138,10 @@ class TestReadCallback : public app::ReadClient::Callback void OnSubscriptionEstablished(SubscriptionId aSubscriptionId) override { mOnSubscriptionEstablished = true; } - void OnError(CHIP_ERROR aError) override { mReadError = aError; } - uint32_t mAttributeCount = 0; bool mOnReportEnd = false; bool mOnSubscriptionEstablished = false; - CHIP_ERROR mReadError = CHIP_NO_ERROR; - TestBufferedReadCallback mBufferedCallback; + app::BufferedReadCallback mBufferedCallback; }; void TestReadCallback::OnAttributeData(const app::ConcreteDataAttributePath & aPath, TLV::TLVReader * apData, @@ -391,7 +276,7 @@ CHIP_ERROR TestAttrAccess::Read(const app::ConcreteReadAttributePath & aPath, ap { case kTestListAttribute: return aEncoder.EncodeList([](const auto & encoder) { - for (int i = 0; i < kListAttributeItems; i++) + for (int i = 0; i < 5; i++) { ReturnErrorOnFailure(encoder.Encode((uint8_t) gIterationCount)); } @@ -561,36 +446,23 @@ void TestReadChunking::TestListChunking(nlTestSuite * apSuite, void * apContext) app::AttributePathParams attributePath(kTestEndpointId3, app::Clusters::UnitTesting::Id, kTestListAttribute); app::ReadPrepareParams readParams(sessionHandle); - // Read the path twice, so we get two lists. This make it easier to check - // for what happens when one of the lists starts near the end of a packet - // boundary. - - AttributePathParams pathList[] = { attributePath, attributePath }; - - readParams.mpAttributePathParamsList = pathList; - readParams.mAttributePathParamsListSize = ArraySize(pathList); - - constexpr size_t maxPacketSize = kMaxSecureSduLengthBytes; - bool gotSuccessfulEncode = false; - bool gotFailureResponse = false; + readParams.mpAttributePathParamsList = &attributePath; + readParams.mAttributePathParamsListSize = 1; // - // Make sure we start off the packet size large enough that we can fit a - // single status response in it. Verify that we get at least one status - // response. Then sweep up over packet sizes until we're big enough to hold - // something like 7 IBs (at 30-40 bytes each, so call it 200 bytes) and check - // the behavior for all those cases. + // We've empirically determined that by reserving 950 bytes in the packet buffer, we can fit 2 + // AttributeDataIBs into the packet. ~30-40 bytes covers a single AttributeDataIB, but let's 2-3x that + // to ensure we'll sweep from fitting 2 IBs to 3-4 IBs. // - for (uint32_t packetSize = 30; packetSize < 200; packetSize++) + for (int i = 100; i > 0; i--) { TestReadCallback readCallback; - ChipLogDetail(DataManagement, "Running iteration %d\n", packetSize); + ChipLogDetail(DataManagement, "Running iteration %d\n", i); - gIterationCount = packetSize; + gIterationCount = (uint32_t) i; - app::InteractionModelEngine::GetInstance()->GetReportingEngine().SetWriterReserved( - static_cast(maxPacketSize - packetSize)); + app::InteractionModelEngine::GetInstance()->GetReportingEngine().SetWriterReserved(static_cast(850 + i)); app::ReadClient readClient(engine, &ctx.GetExchangeManager(), readCallback.mBufferedCallback, app::ReadClient::InteractionType::Read); @@ -598,37 +470,14 @@ void TestReadChunking::TestListChunking(nlTestSuite * apSuite, void * apContext) NL_TEST_ASSERT(apSuite, readClient.SendRequest(readParams) == CHIP_NO_ERROR); ctx.DrainAndServiceIO(); + NL_TEST_ASSERT(apSuite, readCallback.mOnReportEnd); - // Up until our packets are big enough, we might just keep getting - // errors due to the inability to encode even a single IB in a packet. - // But once we manage a successful encode, we should not have any more failures. - if (!gotSuccessfulEncode && readCallback.mReadError != CHIP_NO_ERROR) - { - gotFailureResponse = true; - // Check for the right error type. - NL_TEST_ASSERT(apSuite, - StatusIB(readCallback.mReadError).mStatus == Protocols::InteractionModel::Status::ResourceExhausted); - } - else - { - gotSuccessfulEncode = true; - - NL_TEST_ASSERT(apSuite, readCallback.mOnReportEnd); - - // - // Always returns the same number of attributes read (merged by buffered read callback). The content is checked in - // TestReadCallback::OnAttributeData. The attribute count is 1 - // because the buffered callback treats the second read's path as being - // just a replace of the first read's path and buffers it all up as a - // single value. - // - NL_TEST_ASSERT(apSuite, readCallback.mAttributeCount == 1); - readCallback.mAttributeCount = 0; - - // Check that we never saw an empty-list data IB. - NL_TEST_ASSERT(apSuite, !readCallback.mBufferedCallback.mDecodingFailed); - NL_TEST_ASSERT(apSuite, !readCallback.mBufferedCallback.mSawEmptyList); - } + // + // Always returns the same number of attributes read (merged by buffered read callback). The content is checked in + // TestReadCallback::OnAttributeData + // + NL_TEST_ASSERT(apSuite, readCallback.mAttributeCount == 1); + readCallback.mAttributeCount = 0; NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); @@ -641,9 +490,6 @@ void TestReadChunking::TestListChunking(nlTestSuite * apSuite, void * apContext) } } - // If this fails, our smallest packet size was not small enough. - NL_TEST_ASSERT(apSuite, gotFailureResponse); - emberAfClearDynamicEndpoint(0); }