diff --git a/src/app/MessageDef/SubscribeResponseMessage.cpp b/src/app/MessageDef/SubscribeResponseMessage.cpp index f88ecd58a9c806..57cdf385b0ab06 100644 --- a/src/app/MessageDef/SubscribeResponseMessage.cpp +++ b/src/app/MessageDef/SubscribeResponseMessage.cpp @@ -50,19 +50,6 @@ CHIP_ERROR SubscribeResponseMessage::Parser::CheckSchemaValidity() const ReturnErrorOnFailure(reader.Get(subscriptionId)); PRETTY_PRINT("\tSubscriptionId = 0x%" PRIx32 ",", subscriptionId); } -#endif // CHIP_DETAIL_LOGGING - break; - case to_underlying(Tag::kMinIntervalFloorSeconds): - VerifyOrReturnError(!(tagPresenceMask & (1 << to_underlying(Tag::kMinIntervalFloorSeconds))), - CHIP_ERROR_INVALID_TLV_TAG); - tagPresenceMask |= (1 << to_underlying(Tag::kMinIntervalFloorSeconds)); - VerifyOrReturnError(TLV::kTLVType_UnsignedInteger == reader.GetType(), CHIP_ERROR_WRONG_TLV_TYPE); -#if CHIP_DETAIL_LOGGING - { - uint16_t minIntervalFloorSeconds; - ReturnErrorOnFailure(reader.Get(minIntervalFloorSeconds)); - PRETTY_PRINT("\tMinIntervalFloorSeconds = 0x%x,", minIntervalFloorSeconds); - } #endif // CHIP_DETAIL_LOGGING break; case to_underlying(Tag::kMaxIntervalCeilingSeconds): @@ -91,8 +78,8 @@ CHIP_ERROR SubscribeResponseMessage::Parser::CheckSchemaValidity() const if (CHIP_END_OF_TLV == err) { - const uint16_t requiredFields = (1 << to_underlying(Tag::kSubscriptionId)) | - (1 << to_underlying(Tag::kMinIntervalFloorSeconds)) | (1 << to_underlying(Tag::kMaxIntervalCeilingSeconds)); + const uint16_t requiredFields = + (1 << to_underlying(Tag::kSubscriptionId)) | (1 << to_underlying(Tag::kMaxIntervalCeilingSeconds)); err = (tagPresenceMask & requiredFields) == requiredFields ? CHIP_NO_ERROR : CHIP_ERROR_IM_MALFORMED_SUBSCRIBE_RESPONSE_MESSAGE; } @@ -106,11 +93,6 @@ CHIP_ERROR SubscribeResponseMessage::Parser::GetSubscriptionId(SubscriptionId * return GetUnsignedInteger(to_underlying(Tag::kSubscriptionId), apSubscribeId); } -CHIP_ERROR SubscribeResponseMessage::Parser::GetMinIntervalFloorSeconds(uint16_t * const apMinIntervalFloorSeconds) const -{ - return GetUnsignedInteger(to_underlying(Tag::kMinIntervalFloorSeconds), apMinIntervalFloorSeconds); -} - CHIP_ERROR SubscribeResponseMessage::Parser::GetMaxIntervalCeilingSeconds(uint16_t * const apMaxIntervalCeilingSeconds) const { return GetUnsignedInteger(to_underlying(Tag::kMaxIntervalCeilingSeconds), apMaxIntervalCeilingSeconds); @@ -125,16 +107,6 @@ SubscribeResponseMessage::Builder & SubscribeResponseMessage::Builder::Subscript return *this; } -SubscribeResponseMessage::Builder & -SubscribeResponseMessage::Builder::MinIntervalFloorSeconds(const uint16_t aMinIntervalFloorSeconds) -{ - if (mError == CHIP_NO_ERROR) - { - mError = mpWriter->Put(TLV::ContextTag(to_underlying(Tag::kMinIntervalFloorSeconds)), aMinIntervalFloorSeconds); - } - return *this; -} - SubscribeResponseMessage::Builder & SubscribeResponseMessage::Builder::MaxIntervalCeilingSeconds(const uint16_t aMaxIntervalCeilingSeconds) { diff --git a/src/app/MessageDef/SubscribeResponseMessage.h b/src/app/MessageDef/SubscribeResponseMessage.h index 7c4b3a0e5dd5b1..b712e6bf25081c 100644 --- a/src/app/MessageDef/SubscribeResponseMessage.h +++ b/src/app/MessageDef/SubscribeResponseMessage.h @@ -32,7 +32,6 @@ namespace SubscribeResponseMessage { enum class Tag : uint8_t { kSubscriptionId = 0, - kMinIntervalFloorSeconds = 1, kMaxIntervalCeilingSeconds = 2, }; @@ -62,14 +61,6 @@ class Parser : public MessageParser */ CHIP_ERROR GetSubscriptionId(SubscriptionId * const apSubscriptionId) const; - /** - * @brief Get Final MinIntervalFloorSeconds. Next() must be called before accessing them. - * - * @return #CHIP_NO_ERROR on success - * #CHIP_END_OF_TLV if there is no such element - */ - CHIP_ERROR GetMinIntervalFloorSeconds(uint16_t * const apMinIntervalFloorSeconds) const; - /** * @brief Get Final MaxIntervalCeilingSeconds. Next() must be called before accessing them. * @@ -87,11 +78,6 @@ class Builder : public MessageBuilder */ SubscribeResponseMessage::Builder & SubscriptionId(const chip::SubscriptionId SubscriptionId); - /** - * @brief Final Min Interval for the subscription back to the clients. - */ - SubscribeResponseMessage::Builder & MinIntervalFloorSeconds(const uint16_t aMinIntervalFloorSeconds); - /** * @brief Final Max Interval for the subscription back to the clients. */ diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index 0d52d5649fbdcc..55983102b57fd7 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -806,7 +806,6 @@ CHIP_ERROR ReadClient::ProcessSubscribeResponse(System::PacketBufferHandle && aP SubscriptionId subscriptionId = 0; ReturnErrorOnFailure(subscribeResponse.GetSubscriptionId(&subscriptionId)); VerifyOrReturnError(IsMatchingClient(subscriptionId), CHIP_ERROR_INVALID_ARGUMENT); - ReturnErrorOnFailure(subscribeResponse.GetMinIntervalFloorSeconds(&mMinIntervalFloorSeconds)); ReturnErrorOnFailure(subscribeResponse.GetMaxIntervalCeilingSeconds(&mMaxIntervalCeilingSeconds)); ChipLogProgress(DataManagement, @@ -851,6 +850,7 @@ CHIP_ERROR ReadClient::SendSubscribeRequest(const ReadPrepareParams & aReadPrepa { VerifyOrReturnError(aReadPrepareParams.mMinIntervalFloorSeconds <= aReadPrepareParams.mMaxIntervalCeilingSeconds, CHIP_ERROR_INVALID_ARGUMENT); + mMinIntervalFloorSeconds = aReadPrepareParams.mMinIntervalFloorSeconds; return SendSubscribeRequestImpl(aReadPrepareParams); } diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp index 47877edff568c5..aff3f18563d94e 100644 --- a/src/app/ReadHandler.cpp +++ b/src/app/ReadHandler.cpp @@ -676,10 +676,7 @@ CHIP_ERROR ReadHandler::SendSubscribeResponse() SubscribeResponseMessage::Builder response; ReturnErrorOnFailure(response.Init(&writer)); - response.SubscriptionId(mSubscriptionId) - .MinIntervalFloorSeconds(mMinIntervalFloorSeconds) - .MaxIntervalCeilingSeconds(mMaxIntervalCeilingSeconds) - .EndOfSubscribeResponseMessage(); + response.SubscriptionId(mSubscriptionId).MaxIntervalCeilingSeconds(mMaxIntervalCeilingSeconds).EndOfSubscribeResponseMessage(); ReturnErrorOnFailure(response.GetError()); ReturnErrorOnFailure(writer.Finalize(&packet)); diff --git a/src/app/ReadHandler.h b/src/app/ReadHandler.h index f339aba1c3b552..30f16635cd24bf 100644 --- a/src/app/ReadHandler.h +++ b/src/app/ReadHandler.h @@ -45,6 +45,8 @@ #include #include +constexpr uint16_t kSubscriptionMaxIntervalPublisherLimit = 3600; // 3600 seconds + namespace chip { namespace app { @@ -172,15 +174,18 @@ class ReadHandler : public Messaging::ExchangeDelegate /* * Set the reporting intervals for the subscription. This SHALL only be called - * from the OnSubscriptionRequested callback above. + * from the OnSubscriptionRequested callback above. The restriction is as below + * MinIntervalFloor ≤ MaxInterval ≤ MAX(SUBSCRIPTION_MAX_INTERVAL_PUBLISHER_LIMIT, MaxIntervalCeiling) + * Where SUBSCRIPTION_MAX_INTERVAL_PUBLISHER_LIMIT is set to 60m in the spec. */ - CHIP_ERROR SetReportingIntervals(uint16_t aMinInterval, uint16_t aMaxInterval) + CHIP_ERROR SetReportingIntervals(uint16_t aMaxInterval) { VerifyOrReturnError(IsIdle(), CHIP_ERROR_INCORRECT_STATE); - VerifyOrReturnError(aMinInterval <= aMaxInterval, CHIP_ERROR_INVALID_ARGUMENT); - - mMinIntervalFloorSeconds = aMinInterval; mMaxIntervalCeilingSeconds = aMaxInterval; + VerifyOrReturnError(mMinIntervalFloorSeconds <= mMaxIntervalCeilingSeconds, CHIP_ERROR_INVALID_ARGUMENT); + VerifyOrReturnError(mMaxIntervalCeilingSeconds <= + std::max(kSubscriptionMaxIntervalPublisherLimit, mMaxIntervalCeilingSeconds), + CHIP_ERROR_INVALID_ARGUMENT); return CHIP_NO_ERROR; } diff --git a/src/app/tests/TestMessageDef.cpp b/src/app/tests/TestMessageDef.cpp index 4235e5356b4ae0..f709f7d5a16e38 100644 --- a/src/app/tests/TestMessageDef.cpp +++ b/src/app/tests/TestMessageDef.cpp @@ -1330,9 +1330,6 @@ void BuildSubscribeResponseMessage(nlTestSuite * apSuite, chip::TLV::TLVWriter & subscribeResponseBuilder.SubscriptionId(1); NL_TEST_ASSERT(apSuite, subscribeResponseBuilder.GetError() == CHIP_NO_ERROR); - subscribeResponseBuilder.MinIntervalFloorSeconds(1); - NL_TEST_ASSERT(apSuite, subscribeResponseBuilder.GetError() == CHIP_NO_ERROR); - subscribeResponseBuilder.MaxIntervalCeilingSeconds(2); NL_TEST_ASSERT(apSuite, subscribeResponseBuilder.GetError() == CHIP_NO_ERROR); @@ -1346,7 +1343,6 @@ void ParseSubscribeResponseMessage(nlTestSuite * apSuite, chip::TLV::TLVReader & SubscribeResponseMessage::Parser subscribeResponseParser; chip::SubscriptionId subscriptionId = 0; - uint16_t minIntervalFloorSeconds = 0; uint16_t maxIntervalCeilingSeconds = 0; err = subscribeResponseParser.Init(aReader); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); @@ -1357,9 +1353,6 @@ void ParseSubscribeResponseMessage(nlTestSuite * apSuite, chip::TLV::TLVReader & err = subscribeResponseParser.GetSubscriptionId(&subscriptionId); NL_TEST_ASSERT(apSuite, subscriptionId == 1 && err == CHIP_NO_ERROR); - err = subscribeResponseParser.GetMinIntervalFloorSeconds(&minIntervalFloorSeconds); - NL_TEST_ASSERT(apSuite, minIntervalFloorSeconds == 1 && err == CHIP_NO_ERROR); - err = subscribeResponseParser.GetMaxIntervalCeilingSeconds(&maxIntervalCeilingSeconds); NL_TEST_ASSERT(apSuite, maxIntervalCeilingSeconds == 2 && err == CHIP_NO_ERROR); diff --git a/src/controller/tests/data_model/TestRead.cpp b/src/controller/tests/data_model/TestRead.cpp index dcda1d48ea0238..9fdd28649da7d6 100644 --- a/src/controller/tests/data_model/TestRead.cpp +++ b/src/controller/tests/data_model/TestRead.cpp @@ -218,6 +218,7 @@ class TestReadInteraction : public app::ReadHandler::ApplicationCallback static void TestReadHandler_TwoSubscribesMultipleReads(nlTestSuite * apSuite, void * apContext); static void TestReadHandler_MultipleSubscriptionsWithDataVersionFilter(nlTestSuite * apSuite, void * apContext); static void TestReadHandler_SubscriptionAlteredReportingIntervals(nlTestSuite * apSuite, void * apContext); + static void TestReadHandler_SubscriptionLargeAlteredReportingIntervals(nlTestSuite * apSuite, void * apContext); static void TestReadHandlerResourceExhaustion_MultipleReads(nlTestSuite * apSuite, void * apContext); static void TestReadSubscribeAttributeResponseWithCache(nlTestSuite * apSuite, void * apContext); static void TestReadHandler_KillOverQuotaSubscriptions(nlTestSuite * apSuite, void * apContext); @@ -227,16 +228,17 @@ class TestReadInteraction : public app::ReadHandler::ApplicationCallback static void TestReadHandler_TwoParallelReadsSecondTooManyPaths(nlTestSuite * apSuite, void * apContext); private: - static constexpr uint16_t kTestMinInterval = 33; static constexpr uint16_t kTestMaxInterval = 66; + static uint16_t mMaxInterval; + CHIP_ERROR OnSubscriptionRequested(app::ReadHandler & aReadHandler, Transport::SecureSession & aSecureSession) { VerifyOrReturnError(!mEmitSubscriptionError, CHIP_ERROR_INVALID_ARGUMENT); if (mAlterSubscriptionIntervals) { - ReturnErrorOnFailure(aReadHandler.SetReportingIntervals(kTestMinInterval, kTestMaxInterval)); + ReturnErrorOnFailure(aReadHandler.SetReportingIntervals(mMaxInterval)); } return CHIP_NO_ERROR; } @@ -258,6 +260,8 @@ class TestReadInteraction : public app::ReadHandler::ApplicationCallback bool mAlterSubscriptionIntervals = false; }; +uint16_t TestReadInteraction::mMaxInterval = 66; + TestReadInteraction gTestReadInteraction; class MockInteractionModelApp : public chip::app::ClusterStateCache::Callback @@ -1583,7 +1587,7 @@ void TestReadInteraction::TestReadHandler_SubscriptionAlteredReportingIntervals( NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - NL_TEST_ASSERT(apSuite, minInterval == kTestMinInterval); + NL_TEST_ASSERT(apSuite, minInterval == 0); NL_TEST_ASSERT(apSuite, maxInterval == kTestMaxInterval); numSubscriptionEstablishedCalls++; @@ -1626,6 +1630,76 @@ void TestReadInteraction::TestReadHandler_SubscriptionAlteredReportingIntervals( gTestReadInteraction.mAlterSubscriptionIntervals = false; } +void TestReadInteraction::TestReadHandler_SubscriptionLargeAlteredReportingIntervals(nlTestSuite * apSuite, void * apContext) +{ + TestContext & ctx = *static_cast(apContext); + auto sessionHandle = ctx.GetSessionBobToAlice(); + uint32_t numSuccessCalls = 0; + uint32_t numFailureCalls = 0; + uint32_t numSubscriptionEstablishedCalls = 0; + uint16_t largeMaxInterval = 6000; + responseDirective = kSendDataResponse; + + // Passing of stack variables by reference is only safe because of synchronous completion of the interaction. Otherwise, it's + // not safe to do so. + auto onSuccessCb = [&numSuccessCalls](const app::ConcreteDataAttributePath & attributePath, const auto & dataResponse) { + numSuccessCalls++; + }; + + // Passing of stack variables by reference is only safe because of synchronous completion of the interaction. Otherwise, it's + // not safe to do so. + auto onFailureCb = [&numFailureCalls](const app::ConcreteDataAttributePath * attributePath, CHIP_ERROR aError) { + numFailureCalls++; + }; + + auto onSubscriptionEstablishedCb = [&numSubscriptionEstablishedCalls, &apSuite, + largeMaxInterval](const app::ReadClient & readClient) { + uint16_t minInterval = 0, maxInterval = 0; + CHIP_ERROR err = readClient.GetReportingIntervals(minInterval, maxInterval); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(apSuite, minInterval == 0); + NL_TEST_ASSERT(apSuite, maxInterval == largeMaxInterval); + numSubscriptionEstablishedCalls++; + }; + + // + // Test the application callback as well to ensure we get the right number of SubscriptionEstablishment/Termination + // callbacks. + // + app::InteractionModelEngine::GetInstance()->RegisterReadHandlerAppCallback(&gTestReadInteraction); + + // + // Test the server-side application altering the subscription intervals. + // + gTestReadInteraction.mAlterSubscriptionIntervals = true; + gTestReadInteraction.mMaxInterval = largeMaxInterval; + + NL_TEST_ASSERT(apSuite, + Controller::SubscribeAttribute( + &ctx.GetExchangeManager(), sessionHandle, kTestEndpointId, onSuccessCb, onFailureCb, 0, 10, + onSubscriptionEstablishedCb, false, true) == CHIP_NO_ERROR); + + ctx.DrainAndServiceIO(); + + // + // Failures won't get routed to us here since re-subscriptions are enabled by default in the Controller::SubscribeAttribute + // implementation. + // + NL_TEST_ASSERT(apSuite, numSuccessCalls == 1); + NL_TEST_ASSERT(apSuite, numFailureCalls == 0); + NL_TEST_ASSERT(apSuite, numSubscriptionEstablishedCalls == 1); + NL_TEST_ASSERT(apSuite, gTestReadInteraction.mNumActiveSubscriptions == 1); + + app::InteractionModelEngine::GetInstance()->ShutdownActiveReads(); + + NL_TEST_ASSERT(apSuite, gTestReadInteraction.mNumActiveSubscriptions == 0); + + NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); + + app::InteractionModelEngine::GetInstance()->UnregisterReadHandlerAppCallback(); + gTestReadInteraction.mAlterSubscriptionIntervals = false; +} + void TestReadInteraction::TestReadHandler_MultipleReads(nlTestSuite * apSuite, void * apContext) { TestContext & ctx = *static_cast(apContext); @@ -2556,6 +2630,7 @@ const nlTest sTests[] = NL_TEST_DEF("TestReadHandlerResourceExhaustion_MultipleReads", TestReadInteraction::TestReadHandlerResourceExhaustion_MultipleReads), NL_TEST_DEF("TestReadAttributeTimeout", TestReadInteraction::TestReadAttributeTimeout), NL_TEST_DEF("TestReadHandler_SubscriptionAlteredReportingIntervals", TestReadInteraction::TestReadHandler_SubscriptionAlteredReportingIntervals), + NL_TEST_DEF("TestReadHandler_SubscriptionLargeAlteredReportingIntervals", TestReadInteraction::TestReadHandler_SubscriptionLargeAlteredReportingIntervals), NL_TEST_DEF("TestReadSubscribeAttributeResponseWithCache", TestReadInteraction::TestReadSubscribeAttributeResponseWithCache), NL_TEST_DEF("TestReadHandler_KillOverQuotaSubscriptions", TestReadInteraction::TestReadHandler_KillOverQuotaSubscriptions), NL_TEST_DEF("TestReadHandler_KillOldestSubscriptions", TestReadInteraction::TestReadHandler_KillOldestSubscriptions),