Fix computation of min/max interval for subscriptions to be spec comp…

…liant

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

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.
  */

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

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

src/app/ReadHandler.h

@@ -45,6 +45,8 @@
 #include <protocols/Protocols.h>
 #include <system/SystemPacketBuffer.h>
 
+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;
  }
 

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

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<TestContext *>(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<TestCluster::Attributes::ListStructOctetString::TypeInfo>(
+ &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<TestContext *>(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),