diff --git a/config/standalone/CHIPProjectConfig.h b/config/standalone/CHIPProjectConfig.h index 6e3f47e2fbf1f5..5cb03319c166ae 100644 --- a/config/standalone/CHIPProjectConfig.h +++ b/config/standalone/CHIPProjectConfig.h @@ -75,4 +75,6 @@ #define DYNAMIC_ENDPOINT_COUNT 4 #endif +#define CONFIG_IM_BUILD_FOR_UNIT_TEST 1 + #endif /* CHIPPROJECTCONFIG_H */ diff --git a/src/app/MessageDef/Builder.cpp b/src/app/MessageDef/Builder.cpp index 035c88b3a32509..e93582412f1922 100644 --- a/src/app/MessageDef/Builder.cpp +++ b/src/app/MessageDef/Builder.cpp @@ -47,8 +47,7 @@ void Builder::ResetError() void Builder::ResetError(CHIP_ERROR aErr) { - mError = aErr; - mOuterContainerType = chip::TLV::kTLVType_NotSpecified; + mError = aErr; } void Builder::EndOfContainer() diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp index 9acb2cf77f7569..0d865c138b81c5 100644 --- a/src/app/reporting/Engine.cpp +++ b/src/app/reporting/Engine.cpp @@ -67,6 +67,8 @@ Engine::RetrieveClusterData(FabricIndex aAccessingFabricIndex, AttributeReportIB CHIP_ERROR err = CHIP_NO_ERROR; AttributeReportIB::Builder attributeReport = aAttributeReportIBs.CreateAttributeReport(); + err = attributeReport.GetError(); + SuccessOrExit(attributeReport.GetError()); ChipLogDetail(DataManagement, " Cluster %" PRIx32 ", Attribute %" PRIx32 " is dirty", aPath.mClusterId, aPath.mAttributeId); @@ -95,10 +97,17 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu bool attributeDataWritten = false; bool hasMoreChunks = true; TLV::TLVWriter backup; + const uint32_t kReservedSizeEndOfReportIBs = 1; + aReportDataBuilder.Checkpoint(backup); auto attributeReportIBs = aReportDataBuilder.CreateAttributeReportIBs(); SuccessOrExit(err = aReportDataBuilder.GetError()); + // + // Reserve enough space for closing out the Report IB list + // + attributeReportIBs.GetWriter()->ReserveBuffer(kReservedSizeEndOfReportIBs); + { // TODO: Figure out how AttributePathExpandIterator should handle read // vs write paths. @@ -127,8 +136,8 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu continue; } } - // If we are processing a read request, or the initial report of a subscription, just regard all paths as dirty paths. + // If we are processing a read request, or the initial report of a subscription, just regard all paths as dirty paths. TLV::TLVWriter attributeBackup; attributeReportIBs.Checkpoint(attributeBackup); ConcreteReadAttributePath pathForRetrieval(readPath); @@ -147,19 +156,48 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu hasMoreChunks = false; } exit: + // + // Running out of space is an error that we're expected to handle - the incompletely written DataIB has already been rolled back + // earlier to ensure only whole and complete DataIBs are present in the stream. + // + // We can safely clear out the error so that the rest of the machinery to close out the reports, etc. will function correctly. + // These are are guaranteed to not fail since we've already reserved memory for the remaining 'close out' TLV operations in this + // function and its callers. + // if ((err == CHIP_ERROR_BUFFER_TOO_SMALL) || (err == CHIP_ERROR_NO_MEMORY)) { ChipLogDetail(DataManagement, " We cannot put more chunks into this report. Enable chunking."); + + // + // Reset the error tracked within the builder. Otherwise, any further attempts to write + // data through the builder will be blocked by that error. + // + attributeReportIBs.ResetError(); err = CHIP_NO_ERROR; } + // + // Only close out the report if we haven't hit an error yet so far. + // if (err == CHIP_NO_ERROR) { + attributeReportIBs.GetWriter()->UnreserveBuffer(kReservedSizeEndOfReportIBs); + attributeReportIBs.EndOfAttributeReportIBs(); err = attributeReportIBs.GetError(); + + // + // We reserved space for this earlier - consequently, the call to end the ReportIBs should + // never fail, so assert if we do since that's a logic bug. + // + VerifyOrDie(err == CHIP_NO_ERROR); } - if (!attributeDataWritten || err != CHIP_NO_ERROR) + // + // Rollback the the entire ReportIB array if we never wrote any attributes + // AND never hit an error. + // + if (!attributeDataWritten && err == CHIP_NO_ERROR) { aReportDataBuilder.Rollback(backup); } @@ -269,7 +307,7 @@ CHIP_ERROR Engine::BuildSingleReportDataEventReports(ReportDataMessage::Builder ChipLogDetail(DataManagement, "Fetched %zu events", eventCount); exit: - if (err != CHIP_NO_ERROR || eventCount == 0 || eventClean) + if (err == CHIP_NO_ERROR && (eventCount == 0 || eventClean)) { aReportDataBuilder.Rollback(backup); } @@ -289,6 +327,12 @@ CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler) uint16_t reservedSize = 0; bool hasMoreChunks = false; + // Reserved size for the MoreChunks boolean flag, which takes up 1 byte for the control tag and 1 byte for the context tag. + const uint32_t kReservedSizeForMoreChunksFlag = 1 + 1; + + // Reserved size for the end of report message, which is an end-of-container (i.e 1 byte for the control tag). + const uint32_t kReservedSizeForEndOfReportMessage = 1; + VerifyOrExit(!bufHandle.IsNull(), err = CHIP_ERROR_NO_MEMORY); if (bufHandle->AvailableDataLength() > kMaxSecureSduLengthBytes) @@ -298,6 +342,10 @@ CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler) reportDataWriter.Init(std::move(bufHandle)); +#if CONFIG_IM_BUILD_FOR_UNIT_TEST + reportDataWriter.ReserveBuffer(mReservedSize); +#endif + // Always limit the size of the generated packet to fit within kMaxSecureSduLengthBytes regardless of the available buffer // capacity. // Also, we need to reserve some extra space for the MIC field. @@ -314,7 +362,7 @@ CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler) reportDataBuilder.SubscriptionId(subscriptionId); } - SuccessOrExit(err = reportDataWriter.ReserveBuffer(Engine::kReservedSizeForMoreChunksFlag)); + SuccessOrExit(err = reportDataWriter.ReserveBuffer(kReservedSizeForMoreChunksFlag + kReservedSizeForEndOfReportMessage)); err = BuildSingleReportDataAttributeReportIBs(reportDataBuilder, apReadHandler, &hasMoreChunks); SuccessOrExit(err); @@ -322,7 +370,9 @@ CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler) err = BuildSingleReportDataEventReports(reportDataBuilder, apReadHandler, &hasMoreChunks); SuccessOrExit(err); - SuccessOrExit(err = reportDataWriter.UnreserveBuffer(Engine::kReservedSizeForMoreChunksFlag)); + SuccessOrExit(reportDataBuilder.GetError()); + + SuccessOrExit(err = reportDataWriter.UnreserveBuffer(kReservedSizeForMoreChunksFlag + kReservedSizeForEndOfReportMessage)); if (hasMoreChunks) { reportDataBuilder.MoreChunkedMessages(hasMoreChunks); @@ -333,7 +383,12 @@ CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler) } reportDataBuilder.EndOfReportDataMessage(); - SuccessOrExit(err = reportDataBuilder.GetError()); + + // + // Since we've already reserved space for both the MoreChunked/SuppressResponse flags, as well as + // the end-of-container flag for the end of the report, we should never hit an error closing out the message. + // + VerifyOrDie(reportDataBuilder.GetError() == CHIP_NO_ERROR); err = reportDataWriter.Finalize(&bufHandle); SuccessOrExit(err); diff --git a/src/app/reporting/Engine.h b/src/app/reporting/Engine.h index 9a74acc5cf5923..71890bd5900a26 100644 --- a/src/app/reporting/Engine.h +++ b/src/app/reporting/Engine.h @@ -62,6 +62,10 @@ class Engine void Shutdown(); +#if CONFIG_IM_BUILD_FOR_UNIT_TEST + void SetWriterReserved(uint32_t aReservedSize) { mReservedSize = aReservedSize; } +#endif + /** * Main work-horse function that executes the run-loop. */ @@ -146,8 +150,9 @@ class Engine */ ClusterInfo * mpGlobalDirtySet = nullptr; - constexpr static uint32_t kReservedSizeForMoreChunksFlag = - 2; // According to TLV encoding, the TAG length is 1 byte and its type is 1 byte. +#if CONFIG_IM_BUILD_FOR_UNIT_TEST + uint32_t mReservedSize = 0; +#endif }; }; // namespace reporting diff --git a/src/app/util/ember-compatibility-functions.cpp b/src/app/util/ember-compatibility-functions.cpp index 01f6dd4c7ff735..90298769c21cbc 100644 --- a/src/app/util/ember-compatibility-functions.cpp +++ b/src/app/util/ember-compatibility-functions.cpp @@ -250,7 +250,6 @@ CHIP_ERROR ReadSingleClusterData(FabricIndex aAccessingFabricIndex, const Concre ReturnErrorOnFailure(attributeDataIBBuilder.GetError()); attributePathIBBuilder = attributeDataIBBuilder.CreatePath(); - ReturnErrorOnFailure(attributePathIBBuilder.GetError()); attributePathIBBuilder.Endpoint(aPath.mEndpointId) .Cluster(aPath.mClusterId) diff --git a/src/controller/python/chip/ChipDeviceCtrl.py b/src/controller/python/chip/ChipDeviceCtrl.py index aae7de7271ea87..edaf1438fdc7b6 100644 --- a/src/controller/python/chip/ChipDeviceCtrl.py +++ b/src/controller/python/chip/ChipDeviceCtrl.py @@ -427,7 +427,7 @@ async def ReadAttribute(self, nodeid: int, attributes: typing.List[typing.Union[ (Clusters.ClusterA.AttributeA): Endpoint = *, Cluster = specific, Attribute = specific endpoint: Endpoint = specific, Cluster = *, Attribute = * Clusters.ClusterA: Endpoint = *, Cluster = specific, Attribute = * - None: Endpoint = *, Cluster = *, Attribute = * + '*' or (): Endpoint = *, Cluster = *, Attribute = * The cluster and attributes specified above are to be selected from the generated cluster objects. @@ -446,7 +446,7 @@ async def ReadAttribute(self, nodeid: int, attributes: typing.List[typing.Union[ endpoint = None cluster = None attribute = None - if v == None or v == ('*'): + if v == ('*') or v == (): # Wildcard pass elif type(v) is not tuple: diff --git a/src/controller/python/test/test_scripts/cluster_objects.py b/src/controller/python/test/test_scripts/cluster_objects.py index 2036cb6eee1858..2bcf85f81d780b 100644 --- a/src/controller/python/test/test_scripts/cluster_objects.py +++ b/src/controller/python/test/test_scripts/cluster_objects.py @@ -111,11 +111,10 @@ async def TestReadRequests(cls, devCtrl): (0, Clusters.Basic.Attributes.ProductID), (0, Clusters.Basic.Attributes.HardwareVersion), ] - await devCtrl.ReadAttribute(nodeid=NODE_ID, attributes=req) - - ''' - Note: The following tests is disabled temporarily due to reports of random failure. However, the CI is still green. - Disable these tests to avoid making CI flaky before finding the root cause. + res = await devCtrl.ReadAttribute(nodeid=NODE_ID, attributes=req) + if (len(res) != 3): + raise AssertionError( + f"Got back {len(res)} data items instead of 3") logger.info("2: Reading Ex Cx A*") req = [ @@ -146,7 +145,6 @@ async def TestReadRequests(cls, devCtrl): None ] await devCtrl.ReadAttribute(nodeid=NODE_ID, attributes=req) - ''' @classmethod async def RunTest(cls, devCtrl): diff --git a/src/controller/tests/BUILD.gn b/src/controller/tests/BUILD.gn index f274c53eb96862..a819b0ba0f1730 100644 --- a/src/controller/tests/BUILD.gn +++ b/src/controller/tests/BUILD.gn @@ -25,6 +25,7 @@ chip_test_suite("tests") { if (chip_device_platform != "mbed" && chip_device_platform != "efr32") { test_sources += [ "TestServerCommandDispatch.cpp" ] + test_sources += [ "TestReadChunking.cpp" ] } cflags = [ "-Wconversion" ] diff --git a/src/controller/tests/TestReadChunking.cpp b/src/controller/tests/TestReadChunking.cpp new file mode 100644 index 00000000000000..d4af5407088e17 --- /dev/null +++ b/src/controller/tests/TestReadChunking.cpp @@ -0,0 +1,239 @@ +/* + * + * Copyright (c) 2021 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. + */ + +#include "app-common/zap-generated/ids/Attributes.h" +#include "app-common/zap-generated/ids/Clusters.h" +#include "app/ConcreteAttributePath.h" +#include "protocols/interaction_model/Constants.h" +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +using TestContext = chip::Test::AppContext; +using namespace chip; +using namespace chip::app::Clusters; + +namespace { + +uint32_t gIterationCount = 0; +nlTestSuite * gSuite = nullptr; + +// +// The generated endpoint_config for the controller app has Endpoint 1 +// already used in the fixed endpoint set of size 1. Consequently, let's use the next +// number higher than that for our dynamic test endpoint. +// +constexpr EndpointId kTestEndpointId = 2; + +class TestCommandInteraction +{ +public: + TestCommandInteraction() {} + static void TestChunking(nlTestSuite * apSuite, void * apContext); + +private: +}; + +//clang-format off +DECLARE_DYNAMIC_ATTRIBUTE_LIST_BEGIN(testClusterAttrs) +DECLARE_DYNAMIC_ATTRIBUTE(0x00000001, INT8U, 1, 0), DECLARE_DYNAMIC_ATTRIBUTE(0x00000002, INT8U, 1, 0), + DECLARE_DYNAMIC_ATTRIBUTE(0x00000003, INT8U, 1, 0), DECLARE_DYNAMIC_ATTRIBUTE(0x00000004, INT8U, 1, 0), + DECLARE_DYNAMIC_ATTRIBUTE(0x00000005, INT8U, 1, 0), DECLARE_DYNAMIC_ATTRIBUTE_LIST_END(); + +DECLARE_DYNAMIC_CLUSTER_LIST_BEGIN(testEndpointClusters) +DECLARE_DYNAMIC_CLUSTER(TestCluster::Id, testClusterAttrs), DECLARE_DYNAMIC_CLUSTER_LIST_END; + +DECLARE_DYNAMIC_ENDPOINT(testEndpoint, testEndpointClusters); +//clang-format on + +class TestReadCallback : public app::ReadClient::Callback +{ +public: + void OnAttributeData(const app::ReadClient * apReadClient, const app::ConcreteDataAttributePath & aPath, + TLV::TLVReader * apData, const app::StatusIB & aStatus) override; + + void OnDone(app::ReadClient * apReadClient) override; + + void OnReportEnd(const app::ReadClient * apReadClient) override { mOnReportEnd = true; } + + uint32_t mAttributeCount = 0; + bool mOnReportEnd = false; +}; + +void TestReadCallback::OnAttributeData(const app::ReadClient * apReadClient, const app::ConcreteDataAttributePath & aPath, + TLV::TLVReader * apData, const app::StatusIB & aStatus) +{ + uint8_t v; + NL_TEST_ASSERT(gSuite, app::DataModel::Decode(*apData, v) == CHIP_NO_ERROR); + NL_TEST_ASSERT(gSuite, v == (uint8_t) gIterationCount); + mAttributeCount++; +} + +void TestReadCallback::OnDone(app::ReadClient * apReadClient) {} + +class TestAttrAccess : public app::AttributeAccessInterface +{ +public: + // Register for the Test Cluster cluster on all endpoints. + TestAttrAccess() : AttributeAccessInterface(Optional::Missing(), TestCluster::Id) {} + + CHIP_ERROR Read(const app::ConcreteReadAttributePath & aPath, app::AttributeValueEncoder & aEncoder) override; + CHIP_ERROR Write(const app::ConcreteDataAttributePath & aPath, app::AttributeValueDecoder & aDecoder) override; +}; + +CHIP_ERROR TestAttrAccess::Read(const app::ConcreteReadAttributePath & aPath, app::AttributeValueEncoder & aEncoder) +{ + return aEncoder.Encode((uint8_t) gIterationCount); +} + +CHIP_ERROR TestAttrAccess::Write(const app::ConcreteDataAttributePath & aPath, app::AttributeValueDecoder & aDecoder) +{ + return CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE; +} + +/* + * This validates all the various corner cases encountered during chunking by + * artifically reducing the size of a packet buffer used to encode attribute data + * to force chunking to happen over multiple packets even with a small number of attributes + * and then slowly increasing the available size by 1 byte in each test iteration and re-running + * the report generation logic. This 1-byte incremental approach sweeps through from a base scenario of + * N attributes fitting in a report, to eventually resulting in N+1 attributes fitting in a report. + + * This will cause all the various corner cases encountered of closing out the various containers within + * the report and thoroughly and definitely validate those edge cases. + * + * Importantly, this test tries to re-use *as much as possible* the actual IM constructs used by real + * server-side applications. Consequently, this is why it registers a dynamic endpoint + fake attribute access + * interface to simulate faithfully a real application. This ensures validation of as much production logic pathways + * as we can possibly cover. + * + */ +void TestCommandInteraction::TestChunking(nlTestSuite * apSuite, void * apContext) +{ + TestContext & ctx = *static_cast(apContext); + auto sessionHandle = ctx.GetSessionBobToAlice(); + app::InteractionModelEngine * engine = app::InteractionModelEngine::GetInstance(); + app::ReadClient * readClient; + TestAttrAccess testServer; + + // Initialize the ember side server logic + InitDataModelHandler(&ctx.GetExchangeManager()); + + // Register our fake dynamic endpoint. + emberAfSetDynamicEndpoint(0, kTestEndpointId, &testEndpoint, 0, 0); + + // Register our fake attribute access interface. + registerAttributeAccessOverride(&testServer); + + app::AttributePathParams attributePath(kTestEndpointId, app::Clusters::TestCluster::Id); + app::ReadPrepareParams readParams(sessionHandle); + + readParams.mpAttributePathParamsList = &attributePath; + readParams.mAttributePathParamsListSize = 1; + + // + // 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 (int i = 100; i > 0; i--) + { + TestReadCallback readCallback; + + ChipLogDetail(DataManagement, "Running iteration %d\n", i); + + gIterationCount = (uint32_t) i; + + app::InteractionModelEngine::GetInstance()->GetReportingEngine().SetWriterReserved((uint32_t)(850 + i)); + + NL_TEST_ASSERT(apSuite, + engine->NewReadClient(&readClient, app::ReadClient::InteractionType::Read, &readCallback) == CHIP_NO_ERROR); + NL_TEST_ASSERT(apSuite, readClient->SendReadRequest(readParams) == CHIP_NO_ERROR); + + // + // Service the IO + Engine till we get a ReportEnd callback on the client. + // Since bugs can happen, we don't want this test to never stop, so create a ceiling for how many + // times this can run without seeing expected results. + // + for (int j = 0; j < 10 && !readCallback.mOnReportEnd; j++) + { + ctx.DrainAndServiceIO(); + chip::app::InteractionModelEngine::GetInstance()->GetReportingEngine().Run(); + ctx.DrainAndServiceIO(); + } + + // + // Always returns the same number of attributes read (5 + revision = 6). + // + NL_TEST_ASSERT(apSuite, readCallback.mAttributeCount == 6); + readCallback.mAttributeCount = 0; + + NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); + + // + // Stop the test if we detected an error. Otherwise, it'll be difficult to read the logs. + // + if (apSuite->flagError) + { + break; + } + } +} + +// clang-format off +const nlTest sTests[] = +{ + NL_TEST_DEF("TestChunking", TestCommandInteraction::TestChunking), + NL_TEST_SENTINEL() +}; + +// clang-format on + +// clang-format off +nlTestSuite sSuite = +{ + "TestReadChunking", + &sTests[0], + TestContext::InitializeAsync, + TestContext::Finalize +}; +// clang-format on + +} // namespace + +int TestReadChunkingTests() +{ + TestContext gContext; + gSuite = &sSuite; + nlTestRunner(&sSuite, &gContext); + return (nlTestRunnerStats(&sSuite)); +} + +CHIP_REGISTER_TEST_SUITE(TestReadChunkingTests) diff --git a/src/lib/core/CHIPConfig.h b/src/lib/core/CHIPConfig.h index 83c4461bed4b3e..0f78b4c2eef9ae 100644 --- a/src/lib/core/CHIPConfig.h +++ b/src/lib/core/CHIPConfig.h @@ -2494,6 +2494,16 @@ extern const char CHIP_NON_PRODUCTION_MARKER[]; #define CHIP_IM_MAX_NUM_WRITE_CLIENT 4 #endif +/** + * @def CONFIG_IM_BUILD_FOR_UNIT_TEST + * + * @brief Defines whether we're currently building the IM for unit testing, which enables a set of features + * that are only utilized in those tests. + */ +#ifndef CONFIG_IM_BUILD_FOR_UNIT_TEST +#define CONFIG_IM_BUILD_FOR_UNIT_TEST 0 +#endif + /** * @def CHIP_DEVICE_CONTROLLER_SUBSCRIPTION_ATTRIBUTE_PATH_POOL_SIZE *