Skip to content

Commit

Permalink
[nrf fromtree] Improve size calculation for our packets. (project-chi…
Browse files Browse the repository at this point in the history 
…p#28563)

* Improve size calculation for our packets.

We were hardcoding kMaxAppMessageLen and kMaxSecureSduLengthBytes, but those
defines were not even consistent with each other, and both were overly
conservative.  Specific changes:

1. Fix computation of CHIP_SYSTEM_HEADER_RESERVE_SIZE: we don't have any "crypto
   headers".
2. Fix computation of kMaxAppMessageLen to use our actual packet buffer sizes
   and defined header sizes.
3. Fix kMaxSecureSduLengthBytes to be consistent with kMaxAppMessageLen.

* Simplify the size computation logic.
  • Loading branch information
@bzbarsky-apple @markaj-nordic
bzbarsky-apple authored and markaj-nordic committed Oct 10, 2023
1 parent 07bd870 commit 1b5019a
Show file tree
Hide file tree
Showing 7 changed files with 162 additions and 74 deletions.
3 changes: 2 additions & 1 deletion src/app/StatusResponse.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,11 @@
#include <messaging/ExchangeContext.h>
#include <protocols/interaction_model/Constants.h>
#include <system/TLVPacketBufferBackingStore.h>
#include <transport/raw/MessageHeader.h>

namespace chip {
namespace app {
static constexpr size_t kMaxSecureSduLengthBytes = 1024;
static constexpr size_t kMaxSecureSduLengthBytes = kMaxAppMessageLen + kMaxTagLen;

class StatusResponse
{
Expand Down
84 changes: 64 additions & 20 deletions src/app/tests/TestReadInteraction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,12 @@ chip::EndpointId kInvalidTestEndpointId = 3;
chip::DataVersion kTestDataVersion1 = 3;
chip::DataVersion kTestDataVersion2 = 5;

// Number of items in the list for MockAttributeId(4).
constexpr int kMockAttribute4ListLength = 6;

static chip::System::Clock::Internal::MockClock gMockClock;
static chip::System::Clock::ClockBase * gRealClock;

class TestContext : public chip::Test::AppContext
{
public:
Expand Down Expand Up @@ -1191,7 +1197,8 @@ void TestReadInteraction::TestReadChunking(nlTestSuite * apSuite, void * apConte
NL_TEST_ASSERT(apSuite, !delegate.mGotEventResponse);

chip::app::AttributePathParams attributePathParams[1];
// Mock Attribute 4 is a big attribute, with 6 large OCTET_STRING
// Mock Attribute 4 is a big attribute, with kMockAttribute4ListLength large
// OCTET_STRING elements.
attributePathParams[0].mEndpointId = Test::kMockEndpoint3;
attributePathParams[0].mClusterId = Test::MockClusterId(2);
attributePathParams[0].mAttributeId = Test::MockAttributeId(4);
Expand All @@ -1211,8 +1218,10 @@ void TestReadInteraction::TestReadChunking(nlTestSuite * apSuite, void * apConte

ctx.DrainAndServiceIO();

// We get one chunk with 3 array elements, and then one chunk per element.
NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 4);
// We get one chunk with 4 array elements, and then one chunk per
// element, and the total size of the array is
// kMockAttribute4ListLength.
NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 1 + (kMockAttribute4ListLength - 4));
NL_TEST_ASSERT(apSuite, delegate.mNumArrayItems == 6);
NL_TEST_ASSERT(apSuite, delegate.mGotReport);
NL_TEST_ASSERT(apSuite, !delegate.mReadError);
Expand Down Expand Up @@ -1357,13 +1366,16 @@ void TestReadInteraction::TestSetDirtyBetweenChunks(nlTestSuite * apSuite, void

ctx.DrainAndServiceIO();

// We should receive another (3 + 1) = 4 attribute reports represeting 6
// array items, since the underlying path iterator should be reset to
// the beginning of the cluster it is currently iterating.
// Our list has length kMockAttribute4ListLength. Since the underlying
// path iterator should be reset to the beginning of the cluster it is
// currently iterating, we expect to get another value for our
// attribute. The way the packet boundaries happen to fall, that value
// will encode 4 items in the first IB and then one IB per item.
const int expectedIBs = 1 + (kMockAttribute4ListLength - 4);
ChipLogError(DataManagement, "OLD: %d\n", currentAttributeResponsesWhenSetDirty);
ChipLogError(DataManagement, "NEW: %d\n", delegate.mNumAttributeResponse);
NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == currentAttributeResponsesWhenSetDirty + 4);
NL_TEST_ASSERT(apSuite, delegate.mNumArrayItems == currentArrayItemsWhenSetDirty + 6);
NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == currentAttributeResponsesWhenSetDirty + expectedIBs);
NL_TEST_ASSERT(apSuite, delegate.mNumArrayItems == currentArrayItemsWhenSetDirty + kMockAttribute4ListLength);
NL_TEST_ASSERT(apSuite, delegate.mGotReport);
NL_TEST_ASSERT(apSuite, !delegate.mReadError);
// By now we should have closed all exchanges and sent all pending acks, so
Expand Down Expand Up @@ -1897,16 +1909,47 @@ 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.
#if CHIP_CONFIG_ENABLE_EVENTLIST_ATTRIBUTE
// Mock attribute storage in src/app/util/mock/attribute-storage.cpp
// has the following items
// - Endpoint 0xFFFE
// - cluster 0xFFF1'FC01 (2 attributes)
// - cluster 0xFFF1'FC02 (3 attributes)
// - Endpoint 0xFFFD
// - cluster 0xFFF1'FC01 (2 attributes)
// - cluster 0xFFF1'FC02 (4 attributes)
// - cluster 0xFFF1'FC03 (5 attributes)
// - Endpoint 0xFFFC
// - cluster 0xFFF1'FC01 (3 attributes)
// - cluster 0xFFF1'FC02 (6 attributes)
// - cluster 0xFFF1'FC03 (2 attributes)
// - cluster 0xFFF1'FC04 (2 attributes)
//
// 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);
// For at total of 29 attributes. There are two wildcard subscription
// paths, for a total of 58 attributes.
//
// Attribute 0xFFFC::0xFFF1'FC02::0xFFF1'0004 (kMockEndpoint3::MockClusterId(2)::MockAttributeId(4))
// is a list of kMockAttribute4ListLength elements of size 256 bytes each, which cannot fit in a single
// packet, so gets list chunking applied to it.
//
// Because delegate.mNumAttributeResponse counts AttributeDataIB instances, not attributes,
// the count will depend on exactly how the list for attribute
// 0xFFFC::0xFFF1'FC02::0xFFF1'0004 is chunked. For each of the two instances of that attribute
// in the response, there will be one AttributeDataIB for the start of the list (which will include
// some number of 256-byte elements), then one AttributeDataIB for each of the remaining elements.
//
// When EventList is enabled, for the first report for the list attribute we receive three
// of its items in the initial list, then the remaining items. For the second report we
// receive 2 items in the initial list followed by the remaining items.
constexpr size_t kExpectedAttributeResponse = 29 * 2 + (kMockAttribute4ListLength - 3) + (kMockAttribute4ListLength - 2);
#else
// When EventList is not enabled, the packet boundaries shift and for the first
// report for the list attribute we receive four of its items in the initial list,
// then additional items. For the second report we receive 4 items in
// the initial list followed by additional items.
constexpr size_t kExpectedAttributeResponse = 29 * 2 + (kMockAttribute4ListLength - 4) + (kMockAttribute4ListLength - 4);
#endif
NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == kExpectedAttributeResponse);
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 Expand Up @@ -2994,9 +3037,10 @@ void TestReadInteraction::TestPostSubscribeRoundtripChunkReport(nlTestSuite * ap
}
ctx.DrainAndServiceIO();
}
// Two chunked reports carry 4 attributeDataIB: 1 with a list of 3 items,
// and then one per remaining item.
NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 4);
// We get one chunk with 4 array elements, and then one chunk per
// element, and the total size of the array is
// kMockAttribute4ListLength.
NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 1 + (kMockAttribute4ListLength - 4));
NL_TEST_ASSERT(apSuite, delegate.mNumArrayItems == 6);

NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadClients() == 0);
Expand Down
6 changes: 4 additions & 2 deletions src/app/tests/TestWriteInteraction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -580,8 +580,9 @@ void TestWriteInteraction::TestWriteHandlerReceiveInvalidMessage(nlTestSuite * a
err = engine->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable());
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

// Reserve all except the last 128 bytes, so that we make sure to chunk.
app::WriteClient writeClient(&ctx.GetExchangeManager(), &writeCallback, Optional<uint16_t>::Missing(),
static_cast<uint16_t>(900) /* reserved buffer size */);
static_cast<uint16_t>(kMaxSecureSduLengthBytes - 128) /* reserved buffer size */);

ByteSpan list[5];

Expand Down Expand Up @@ -648,8 +649,9 @@ void TestWriteInteraction::TestWriteHandlerInvalidateFabric(nlTestSuite * apSuit
err = engine->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable());
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

// Reserve all except the last 128 bytes, so that we make sure to chunk.
app::WriteClient writeClient(&ctx.GetExchangeManager(), &writeCallback, Optional<uint16_t>::Missing(),
static_cast<uint16_t>(900) /* reserved buffer size */);
static_cast<uint16_t>(kMaxSecureSduLengthBytes - 128) /* reserved buffer size */);

ByteSpan list[5];

Expand Down
38 changes: 21 additions & 17 deletions src/controller/tests/TestWriteChunking.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -203,21 +203,22 @@ void TestWriteChunking::TestListChunking(nlTestSuite * apSuite, void * apContext

app::AttributePathParams attributePath(kTestEndpointId, app::Clusters::UnitTesting::Id, kTestListAttribute);
//
// We've empirically determined that by reserving 950 bytes in the packet buffer, we can fit 2
// We've empirically determined that by reserving all but 75 bytes in the packet buffer, we can fit 2
// AttributeDataIBs into the packet. ~30-40 bytes covers a single write chunk, but let's 2-3x that
// to ensure we'll sweep from fitting 2 chunks to 3-4 chunks.
//
for (int i = 100; i > 0; i--)
constexpr size_t minReservationSize = kMaxSecureSduLengthBytes - 75 - 100;
for (uint32_t i = 100; i > 0; i--)
{
CHIP_ERROR err = CHIP_NO_ERROR;
TestWriteCallback writeCallback;

ChipLogDetail(DataManagement, "Running iteration %d\n", i);

gIterationCount = (uint32_t) i;
gIterationCount = i;

app::WriteClient writeClient(&ctx.GetExchangeManager(), &writeCallback, Optional<uint16_t>::Missing(),
static_cast<uint16_t>(850 + i) /* reserved buffer size */);
static_cast<uint16_t>(minReservationSize + i) /* reserved buffer size */);

ByteSpan list[kTestListLength];

Expand Down Expand Up @@ -358,15 +359,16 @@ void TestWriteChunking::TestConflictWrite(nlTestSuite * apSuite, void * apContex

app::AttributePathParams attributePath(kTestEndpointId, app::Clusters::UnitTesting::Id, kTestListAttribute);

/* use a smaller chunk (128 bytes) so we only need a few attributes in the write request. */
constexpr size_t kReserveSize = kMaxSecureSduLengthBytes - 128;

TestWriteCallback writeCallback1;
app::WriteClient writeClient1(
&ctx.GetExchangeManager(), &writeCallback1, Optional<uint16_t>::Missing(),
static_cast<uint16_t>(900) /* use a smaller chunk so we only need a few attributes in the write request. */);
app::WriteClient writeClient1(&ctx.GetExchangeManager(), &writeCallback1, Optional<uint16_t>::Missing(),
static_cast<uint16_t>(kReserveSize));

TestWriteCallback writeCallback2;
app::WriteClient writeClient2(
&ctx.GetExchangeManager(), &writeCallback2, Optional<uint16_t>::Missing(),
static_cast<uint16_t>(900) /* use a smaller chunk so we only need a few attributes in the write request. */);
app::WriteClient writeClient2(&ctx.GetExchangeManager(), &writeCallback2, Optional<uint16_t>::Missing(),
static_cast<uint16_t>(kReserveSize));

ByteSpan list[kTestListLength];

Expand Down Expand Up @@ -435,15 +437,16 @@ void TestWriteChunking::TestNonConflictWrite(nlTestSuite * apSuite, void * apCon
app::AttributePathParams attributePath1(kTestEndpointId, app::Clusters::UnitTesting::Id, kTestListAttribute);
app::AttributePathParams attributePath2(kTestEndpointId, app::Clusters::UnitTesting::Id, kTestListAttribute2);

/* use a smaller chunk (128 bytes) so we only need a few attributes in the write request. */
constexpr size_t kReserveSize = kMaxSecureSduLengthBytes - 128;

TestWriteCallback writeCallback1;
app::WriteClient writeClient1(
&ctx.GetExchangeManager(), &writeCallback1, Optional<uint16_t>::Missing(),
static_cast<uint16_t>(900) /* use a smaller chunk so we only need a few attributes in the write request. */);
app::WriteClient writeClient1(&ctx.GetExchangeManager(), &writeCallback1, Optional<uint16_t>::Missing(),
static_cast<uint16_t>(kReserveSize));

TestWriteCallback writeCallback2;
app::WriteClient writeClient2(
&ctx.GetExchangeManager(), &writeCallback2, Optional<uint16_t>::Missing(),
static_cast<uint16_t>(900) /* use a smaller chunk so we only need a few attributes in the write request. */);
app::WriteClient writeClient2(&ctx.GetExchangeManager(), &writeCallback2, Optional<uint16_t>::Missing(),
static_cast<uint16_t>(kReserveSize));

ByteSpan list[kTestListLength];

Expand Down Expand Up @@ -514,7 +517,8 @@ void RunTest(nlTestSuite * apSuite, TestContext & ctx, Instructions instructions
TestWriteCallback writeCallback;
std::unique_ptr<WriteClient> writeClient = std::make_unique<WriteClient>(
&ctx.GetExchangeManager(), &writeCallback, Optional<uint16_t>::Missing(),
static_cast<uint16_t>(900) /* use a smaller chunk so we only need a few attributes in the write request. */);
static_cast<uint16_t>(kMaxSecureSduLengthBytes -
128) /* use a smaller chunk so we only need a few attributes in the write request. */);

ConcreteAttributePath onGoingPath = ConcreteAttributePath();
std::vector<PathStatus> status;
Expand Down
22 changes: 15 additions & 7 deletions src/controller/tests/data_model/TestRead.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1056,7 +1056,7 @@ void TestReadInteraction::TestReadSubscribeAttributeResponseWithCache(nlTestSuit
}

// Read of E2C3A* and E3C2A2, and inject a large amount of event path list, then it would try to apply previous cache
// latest data version and construct data version list but no enough memory, finally fully rollback data version filter. Expect
// latest data version and construct data version list but run out of memory, finally fully rollback data version filter. Expect
// E2C3A* attributes in report, and E3C2A2 attribute in report
{
testId++;
Expand All @@ -1074,8 +1074,12 @@ void TestReadInteraction::TestReadSubscribeAttributeResponseWithCache(nlTestSuit
readPrepareParams.mpAttributePathParamsList = attributePathParams2;
readPrepareParams.mAttributePathParamsListSize = 2;

readPrepareParams.mpEventPathParamsList = eventPathParams;
readPrepareParams.mEventPathParamsListSize = 64;
readPrepareParams.mpEventPathParamsList = eventPathParams;
// This size needs to be big enough that we can't fit our
// DataVersionFilterIBs in the same packet. Max size is
// ArraySize(eventPathParams);
static_assert(75 <= ArraySize(eventPathParams), "Invalid eventPathParams size");
readPrepareParams.mEventPathParamsListSize = 75;

err = readClient.SendRequest(readPrepareParams);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
Expand Down Expand Up @@ -1245,8 +1249,8 @@ void TestReadInteraction::TestReadSubscribeAttributeResponseWithCache(nlTestSuit

// Read of E1C2A*(3 attributes) and E2C3A*(5 attributes) and E2C2A*(4 attributes), and inject a large amount of event path
// list, then it would try to apply previous cache latest data version and construct data version list with the ordering from
// largest cluster size to smallest cluster size(C2, C3, C1) but no enough memory, finally partially rollback data version
// filter with only C2. Expect E1C2A*, E2C2A* attributes(7 attributes) in report,
// largest cluster size to smallest cluster size(C3, C2, C1) but run out of memory, finally partially rollback data version
// filter with only C3. Expect E1C2A*, E2C2A* attributes(7 attributes) in report,
{
testId++;
ChipLogProgress(DataManagement, "\t -- Running Read with ClusterStateCache Test ID %d", testId);
Expand All @@ -1268,8 +1272,12 @@ void TestReadInteraction::TestReadSubscribeAttributeResponseWithCache(nlTestSuit
readPrepareParams.mpAttributePathParamsList = attributePathParams3;
readPrepareParams.mAttributePathParamsListSize = 3;
readPrepareParams.mpEventPathParamsList = eventPathParams;
readPrepareParams.mEventPathParamsListSize = 62;
err = readClient.SendRequest(readPrepareParams);

// This size needs to be big enough that we can only fit our first
// DataVersionFilterIB. Max size is ArraySize(eventPathParams);
static_assert(73 <= ArraySize(eventPathParams), "Invalid size of eventPathParams");
readPrepareParams.mEventPathParamsListSize = 73;
err = readClient.SendRequest(readPrepareParams);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

ctx.DrainAndServiceIO();
Expand Down
Loading

0 comments on commit 1b5019a

Please sign in to comment.