Skip to content

Commit

Permalink
Revert "[nrf fromtree] Improve encoding of lists where first item can…
Browse files Browse the repository at this point in the history 
…'t fit in packet. (project-chip#28346)"

This reverts commit 07bd870.
  • Loading branch information
@kkasperczyk-no
kkasperczyk-no committed Nov 9, 2023
1 parent d6b79d2 commit 1f9cff4
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 199 deletions.
13 changes: 0 additions & 13 deletions src/app/AttributeAccessInterface.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 0 additions & 3 deletions src/app/AttributeAccessInterface.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,6 @@ class AttributeValueEncoder

mCurrentEncodingListIndex++;
mEncodeState.mCurrentEncodingListIndex++;
mEncodedAtLeastOneListItem = true;
return CHIP_NO_ERROR;
}

Expand Down Expand Up @@ -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;
};
Expand Down
20 changes: 9 additions & 11 deletions src/app/tests/TestReadInteraction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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);
Expand Down
190 changes: 18 additions & 172 deletions src/controller/tests/TestReadChunking.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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<ReadClient::Callback *>(&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<AttributePathParams> & aAttributePaths,
bool & aEncodedDataVersionList) override
{
return NextCallback().OnUpdateDataVersionFilterList(aDataVersionFilterIBsBuilder, aAttributePaths, aEncodedDataVersionList);
}

CHIP_ERROR GetHighestReceivedEventNumber(Optional<EventNumber> & 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:
Expand All @@ -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,
Expand Down Expand Up @@ -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));
}
Expand Down Expand Up @@ -561,74 +446,38 @@ 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<uint32_t>(maxPacketSize - packetSize));
app::InteractionModelEngine::GetInstance()->GetReportingEngine().SetWriterReserved(static_cast<uint32_t>(850 + i));

app::ReadClient readClient(engine, &ctx.GetExchangeManager(), readCallback.mBufferedCallback,
app::ReadClient::InteractionType::Read);

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);

Expand All @@ -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);
}

Expand Down

0 comments on commit 1f9cff4

Please sign in to comment.