diff --git a/src/app/AttributeAccessInterface.cpp b/src/app/AttributeAccessInterface.cpp index f4fbec03b34ac4..3152bbf604ffd9 100644 --- a/src/app/AttributeAccessInterface.cpp +++ b/src/app/AttributeAccessInterface.cpp @@ -130,6 +130,19 @@ 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 8b4ec3a0c91d47..c195dd4f8b66ef 100644 --- a/src/app/AttributeAccessInterface.h +++ b/src/app/AttributeAccessInterface.h @@ -292,6 +292,7 @@ class AttributeValueEncoder mCurrentEncodingListIndex++; mEncodeState.mCurrentEncodingListIndex++; + mEncodedAtLeastOneListItem = true; return CHIP_NO_ERROR; } @@ -340,6 +341,8 @@ 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 0c490f86002f73..0026b1567abb66 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -1309,8 +1309,8 @@ void TestReadInteraction::TestSetDirtyBetweenChunks(nlTestSuite * apSuite, void !aPath.IsListItemOperation()) { mGotStartOfSecondReport = true; - // Wait for an actual data chunk. - return; + // We always have data chunks, so go ahead to mark things + // dirty as needed. } if (!mGotStartOfSecondReport) @@ -2359,12 +2359,12 @@ void TestReadInteraction::TestSubscribeWildcard(nlTestSuite * apSuite, void * ap #else // 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 0 items in - // the initial list followed by 6 additional items. + // then 4 additional items. For the second report we receive 3 items in + // the initial list followed by 3 additional items. // - // Thus we should receive 29*2 + 4 + 6 = 68 attribute data when the eventlist + // Thus we should receive 29*2 + 4 + 3 = 65 attribute data when the eventlist // attribute is not available. - constexpr size_t kExpectedAttributeResponse = 68; + constexpr size_t kExpectedAttributeResponse = 65; #endif NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == kExpectedAttributeResponse); NL_TEST_ASSERT(apSuite, delegate.mNumArrayItems == 12); diff --git a/src/controller/tests/TestReadChunking.cpp b/src/controller/tests/TestReadChunking.cpp index 03a6c85a6f7c92..d98d0586180940 100644 --- a/src/controller/tests/TestReadChunking.cpp +++ b/src/controller/tests/TestReadChunking.cpp @@ -70,6 +70,8 @@ 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: @@ -125,6 +127,116 @@ 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: @@ -138,10 +250,13 @@ 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; - app::BufferedReadCallback mBufferedCallback; + CHIP_ERROR mReadError = CHIP_NO_ERROR; + TestBufferedReadCallback mBufferedCallback; }; void TestReadCallback::OnAttributeData(const app::ConcreteDataAttributePath & aPath, TLV::TLVReader * apData, @@ -276,7 +391,7 @@ CHIP_ERROR TestAttrAccess::Read(const app::ConcreteReadAttributePath & aPath, ap { case kTestListAttribute: return aEncoder.EncodeList([](const auto & encoder) { - for (int i = 0; i < 5; i++) + for (int i = 0; i < kListAttributeItems; i++) { ReturnErrorOnFailure(encoder.Encode((uint8_t) gIterationCount)); } @@ -446,23 +561,36 @@ void TestReadChunking::TestListChunking(nlTestSuite * apSuite, void * apContext) app::AttributePathParams attributePath(kTestEndpointId3, app::Clusters::UnitTesting::Id, kTestListAttribute); app::ReadPrepareParams readParams(sessionHandle); - readParams.mpAttributePathParamsList = &attributePath; - readParams.mAttributePathParamsListSize = 1; + // 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; // - // 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. + // 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. // - for (int i = 100; i > 0; i--) + for (uint32_t packetSize = 30; packetSize < 200; packetSize++) { TestReadCallback readCallback; - ChipLogDetail(DataManagement, "Running iteration %d\n", i); + ChipLogDetail(DataManagement, "Running iteration %d\n", packetSize); - gIterationCount = (uint32_t) i; + gIterationCount = packetSize; - app::InteractionModelEngine::GetInstance()->GetReportingEngine().SetWriterReserved(static_cast(850 + i)); + app::InteractionModelEngine::GetInstance()->GetReportingEngine().SetWriterReserved( + static_cast(maxPacketSize - packetSize)); app::ReadClient readClient(engine, &ctx.GetExchangeManager(), readCallback.mBufferedCallback, app::ReadClient::InteractionType::Read); @@ -470,14 +598,37 @@ 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); - // - // 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; + // 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); + } NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); @@ -490,6 +641,9 @@ 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); }