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

#19262)

* Fix computation of min/max interval for subscriptions to be spec compliant

* Fix logic in SetReportingIntervals

* address comments

src/app/MessageDef/SubscribeResponseMessage.cpp

@@ -52,29 +52,15 @@ CHIP_ERROR SubscribeResponseMessage::Parser::CheckSchemaValidity() const
  }
 #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));
+ case to_underlying(Tag::kMaxInterval):
+ VerifyOrReturnError(!(tagPresenceMask & (1 << to_underlying(Tag::kMaxInterval))), CHIP_ERROR_INVALID_TLV_TAG);
+ tagPresenceMask |= (1 << to_underlying(Tag::kMaxInterval));
  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):
- VerifyOrReturnError(!(tagPresenceMask & (1 << to_underlying(Tag::kMaxIntervalCeilingSeconds))),
- CHIP_ERROR_INVALID_TLV_TAG);
- tagPresenceMask |= (1 << to_underlying(Tag::kMaxIntervalCeilingSeconds));
- VerifyOrReturnError(TLV::kTLVType_UnsignedInteger == reader.GetType(), CHIP_ERROR_WRONG_TLV_TYPE);
-#if CHIP_DETAIL_LOGGING
- {
- uint16_t maxIntervalCeilingSeconds;
- ReturnErrorOnFailure(reader.Get(maxIntervalCeilingSeconds));
- PRETTY_PRINT("\tMaxIntervalCeilingSeconds = 0x%x,", maxIntervalCeilingSeconds);
+ uint16_t maxInterval;
+ ReturnErrorOnFailure(reader.Get(maxInterval));
+ PRETTY_PRINT("\tMaxInterval = 0x%x,", maxInterval);
  }
 #endif // CHIP_DETAIL_LOGGING
  break;
@@ -91,9 +77,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));
- err = (tagPresenceMask & requiredFields) == requiredFields ? CHIP_NO_ERROR
+ const uint16_t requiredFields = (1 << to_underlying(Tag::kSubscriptionId)) | (1 << to_underlying(Tag::kMaxInterval));
+ err = (tagPresenceMask & requiredFields) == requiredFields ? CHIP_NO_ERROR
  : CHIP_ERROR_IM_MALFORMED_SUBSCRIBE_RESPONSE_MESSAGE;
  }
  ReturnErrorOnFailure(err);
@@ -106,14 +91,9 @@ 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
+CHIP_ERROR SubscribeResponseMessage::Parser::GetMaxInterval(uint16_t * const apMaxInterval) const
 {
- return GetUnsignedInteger(to_underlying(Tag::kMaxIntervalCeilingSeconds), apMaxIntervalCeilingSeconds);
+ return GetUnsignedInteger(to_underlying(Tag::kMaxInterval), apMaxInterval);
 }
 
 SubscribeResponseMessage::Builder & SubscribeResponseMessage::Builder::SubscriptionId(const chip::SubscriptionId aSubscribeId)
@@ -125,22 +105,11 @@ 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)
+SubscribeResponseMessage::Builder & SubscribeResponseMessage::Builder::MaxInterval(const uint16_t aMaxInterval)
 {
  if (mError == CHIP_NO_ERROR)
  {
- mError = mpWriter->Put(TLV::ContextTag(to_underlying(Tag::kMaxIntervalCeilingSeconds)), aMaxIntervalCeilingSeconds);
+ mError = mpWriter->Put(TLV::ContextTag(to_underlying(Tag::kMaxInterval)), aMaxInterval);
  }
  return *this;
 }

src/app/MessageDef/SubscribeResponseMessage.h

@@ -31,9 +31,8 @@ namespace app {
 namespace SubscribeResponseMessage {
 enum class Tag : uint8_t
 {
- kSubscriptionId = 0,
- kMinIntervalFloorSeconds = 1,
- kMaxIntervalCeilingSeconds = 2,
+ kSubscriptionId = 0,
+ kMaxInterval = 2,
 };
 
 class Parser : public MessageParser
@@ -62,21 +61,13 @@ 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.
  *
  * @return #CHIP_NO_ERROR on success
  * #CHIP_END_OF_TLV if there is no such element
  */
- CHIP_ERROR GetMaxIntervalCeilingSeconds(uint16_t * const apMaxIntervalCeilingSeconds) const;
+ CHIP_ERROR GetMaxInterval(uint16_t * const apMaxInterval) const;
 };
 
 class Builder : public MessageBuilder
@@ -87,15 +78,10 @@ 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.
  */
- SubscribeResponseMessage::Builder & MaxIntervalCeilingSeconds(const uint16_t aMaxIntervalCeilingSeconds);
+ SubscribeResponseMessage::Builder & MaxInterval(const uint16_t aMaxInterval);
 
  /**
  * @brief Mark the end of this SubscribeResponseMessage

src/app/ReadClient.cpp

@@ -91,12 +91,12 @@ ReadClient::ReadClient(InteractionModelEngine * apImEngine, Messaging::ExchangeM
 
 void ReadClient::ClearActiveSubscriptionState()
 {
- mIsInitialReport  = true;
- mIsPrimingReports  = true;
- mPendingMoreChunks  = false;
- mMinIntervalFloorSeconds  = 0;
- mMaxIntervalCeilingSeconds = 0;
- mSubscriptionId  = 0;
+ mIsInitialReport = true;
+ mIsPrimingReports = true;
+ mPendingMoreChunks = false;
+ mMinIntervalFloorSeconds = 0;
+ mMaxInterval  = 0;
+ mSubscriptionId = 0;
  MoveToState(ClientState::Idle);
 }
 
@@ -754,8 +754,7 @@ CHIP_ERROR ReadClient::RefreshLivenessCheckTimer()
  VerifyOrReturnError(mpExchangeCtx != nullptr, err = CHIP_ERROR_INCORRECT_STATE);
  VerifyOrReturnError(mpExchangeCtx->HasSessionHandle(), err = CHIP_ERROR_INCORRECT_STATE);
 
- System::Clock::Timeout timeout =
- System::Clock::Seconds16(mMaxIntervalCeilingSeconds) + mpExchangeCtx->GetSessionHandle()->GetAckTimeout();
+ System::Clock::Timeout timeout = System::Clock::Seconds16(mMaxInterval) + mpExchangeCtx->GetSessionHandle()->GetAckTimeout();
  // EFR32/MBED/INFINION/K32W's chrono count return long unsinged, but other platform returns unsigned
  ChipLogProgress(DataManagement,
  "Refresh LivenessCheckTime for %lu milliseconds with SubscriptionId = 0x%08" PRIx32
@@ -819,14 +818,12 @@ 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));
+ ReturnErrorOnFailure(subscribeResponse.GetMaxInterval(&mMaxInterval));
 
  ChipLogProgress(DataManagement,
  "Subscription established with SubscriptionID = 0x%08" PRIx32 " MinInterval = %u"
  "s MaxInterval = %us Peer = %02x:" ChipLogFormatX64,
- mSubscriptionId, mMinIntervalFloorSeconds, mMaxIntervalCeilingSeconds, mFabricIndex,
- ChipLogValueX64(mPeerNodeId));
+ mSubscriptionId, mMinIntervalFloorSeconds, mMaxInterval, mFabricIndex, ChipLogValueX64(mPeerNodeId));
 
  ReturnErrorOnFailure(subscribeResponse.ExitContainer());
 
@@ -864,6 +861,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/ReadClient.h

@@ -290,7 +290,7 @@ class ReadClient : public Messaging::ExchangeDelegate
  VerifyOrReturnError(IsSubscriptionActive(), CHIP_ERROR_INCORRECT_STATE);
 
  aMinIntervalFloorSeconds = mMinIntervalFloorSeconds;
- aMaxIntervalCeilingSeconds = mMaxIntervalCeilingSeconds;
+ aMaxIntervalCeilingSeconds = mMaxInterval;
 
  return CHIP_NO_ERROR;
  }
@@ -413,16 +413,16 @@ class ReadClient : public Messaging::ExchangeDelegate
  Messaging::ExchangeManager * mpExchangeMgr = nullptr;
  Messaging::ExchangeContext * mpExchangeCtx = nullptr;
  Callback & mpCallback;
- ClientState mState  = ClientState::Idle;
- bool mIsInitialReport  = true;
- bool mIsPrimingReports  = true;
- bool mPendingMoreChunks  = false;
- uint16_t mMinIntervalFloorSeconds  = 0;
- uint16_t mMaxIntervalCeilingSeconds = 0;
- SubscriptionId mSubscriptionId  = 0;
- NodeId mPeerNodeId  = kUndefinedNodeId;
- FabricIndex mFabricIndex  = kUndefinedFabricIndex;
- InteractionType mInteractionType  = InteractionType::Read;
+ ClientState mState = ClientState::Idle;
+ bool mIsInitialReport = true;
+ bool mIsPrimingReports = true;
+ bool mPendingMoreChunks = false;
+ uint16_t mMinIntervalFloorSeconds = 0;
+ uint16_t mMaxInterval  = 0;
+ SubscriptionId mSubscriptionId = 0;
+ NodeId mPeerNodeId = kUndefinedNodeId;
+ FabricIndex mFabricIndex = kUndefinedFabricIndex;
+ InteractionType mInteractionType = InteractionType::Read;
  Timestamp mEventTimestamp;
  bool mSawAttributeReportsInCurrentReport = false;
 

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).MaxInterval(mMaxInterval).EndOfSubscribeResponseMessage();
  ReturnErrorOnFailure(response.GetError());
 
  ReturnErrorOnFailure(writer.Finalize(&packet));
@@ -747,8 +744,8 @@ CHIP_ERROR ReadHandler::ProcessSubscribeRequest(System::PacketBufferHandle && aP
  ReturnErrorOnFailure(err);
 
  ReturnErrorOnFailure(subscribeRequestParser.GetMinIntervalFloorSeconds(&mMinIntervalFloorSeconds));
- ReturnErrorOnFailure(subscribeRequestParser.GetMaxIntervalCeilingSeconds(&mMaxIntervalCeilingSeconds));
- VerifyOrReturnError(mMinIntervalFloorSeconds <= mMaxIntervalCeilingSeconds, CHIP_ERROR_INVALID_ARGUMENT);
+ ReturnErrorOnFailure(subscribeRequestParser.GetMaxIntervalCeilingSeconds(&mMaxInterval));
+ VerifyOrReturnError(mMinIntervalFloorSeconds <= mMaxInterval, CHIP_ERROR_INVALID_ARGUMENT);
 
  //
  // Notify the application (if requested) of the impending subscription and check whether we should still proceed to set it up.
@@ -764,7 +761,7 @@ CHIP_ERROR ReadHandler::ProcessSubscribeRequest(System::PacketBufferHandle && aP
  }
 
  ChipLogProgress(DataManagement, "Final negotiated min/max parameters: Min = %ds, Max = %ds", mMinIntervalFloorSeconds,
- mMaxIntervalCeilingSeconds);
+ mMaxInterval);
 
  bool isFabricFiltered;
  ReturnErrorOnFailure(subscribeRequestParser.GetIsFabricFiltered(&isFabricFiltered));
@@ -789,7 +786,7 @@ void ReadHandler::OnUnblockHoldReportCallback(System::Layer * apSystemLayer, voi
  InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun();
  }
  InteractionModelEngine::GetInstance()->GetExchangeManager()->GetSessionManager()->SystemLayer()->StartTimer(
- System::Clock::Seconds16(readHandler->mMaxIntervalCeilingSeconds - readHandler->mMinIntervalFloorSeconds),
+ System::Clock::Seconds16(readHandler->mMaxInterval - readHandler->mMinIntervalFloorSeconds),
  OnRefreshSubscribeTimerSyncCallback, readHandler);
 }
 
@@ -799,13 +796,13 @@ void ReadHandler::OnRefreshSubscribeTimerSyncCallback(System::Layer * apSystemLa
  ReadHandler * readHandler = static_cast<ReadHandler *>(apAppState);
  readHandler->mFlags.Set(ReadHandlerFlags::HoldSync, false);
  ChipLogDetail(DataManagement, "Refresh subscribe timer sync after %d seconds",
- readHandler->mMaxIntervalCeilingSeconds - readHandler->mMinIntervalFloorSeconds);
+ readHandler->mMaxInterval - readHandler->mMinIntervalFloorSeconds);
  InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun();
 }
 
 CHIP_ERROR ReadHandler::RefreshSubscribeSyncTimer()
 {
- ChipLogDetail(DataManagement, "Refresh Subscribe Sync Timer with max %d seconds", mMaxIntervalCeilingSeconds);
+ ChipLogDetail(DataManagement, "Refresh Subscribe Sync Timer with max %d seconds", mMaxInterval);
  InteractionModelEngine::GetInstance()->GetExchangeManager()->GetSessionManager()->SystemLayer()->CancelTimer(
  OnUnblockHoldReportCallback, this);
  InteractionModelEngine::GetInstance()->GetExchangeManager()->GetSessionManager()->SystemLayer()->CancelTimer(

src/app/ReadHandler.h

@@ -45,6 +45,9 @@
 #include <protocols/Protocols.h>
 #include <system/SystemPacketBuffer.h>
 
+// https://github.com/CHIP-Specifications/connectedhomeip-spec/blob/61a9d19e6af12fdfb0872bcff26d19de6c680a1a/src/Ch02_Architecture.adoc#1122-subscribe-interaction-limits
+constexpr uint16_t kSubscriptionMaxIntervalPublisherLimit = 3600; // 3600 seconds
+
 namespace chip {
 namespace app {
 
@@ -167,20 +170,22 @@ class ReadHandler : public Messaging::ExchangeDelegate
  void GetReportingIntervals(uint16_t & aMinInterval, uint16_t & aMaxInterval) const
  {
  aMinInterval = mMinIntervalFloorSeconds;
- aMaxInterval = mMaxIntervalCeilingSeconds;
+ aMaxInterval = mMaxInterval;
  }
 
  /*
  * 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 <= aMaxInterval, CHIP_ERROR_INVALID_ARGUMENT);
+ VerifyOrReturnError(aMaxInterval <= std::max(kSubscriptionMaxIntervalPublisherLimit, mMaxInterval),
+  CHIP_ERROR_INVALID_ARGUMENT);
+ mMaxInterval = aMaxInterval;
  return CHIP_NO_ERROR;
  }
 
@@ -428,9 +433,9 @@ class ReadHandler : public Messaging::ExchangeDelegate
  // engine, the "oldest" subscription is the subscription with the smallest generation.
  uint64_t mSubscriptionStartGeneration = 0;
 
- SubscriptionId mSubscriptionId  = 0;
- uint16_t mMinIntervalFloorSeconds  = 0;
- uint16_t mMaxIntervalCeilingSeconds = 0;
+ SubscriptionId mSubscriptionId = 0;
+ uint16_t mMinIntervalFloorSeconds = 0;
+ uint16_t mMaxInterval  = 0;
 
  EventNumber mEventMin = 0;