Skip to content

Commit

Permalink
Fix computation of min/max interval for subscriptions to be spec comp… (
Browse files Browse the repository at this point in the history 
#19262)

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

* Fix logic in SetReportingIntervals

* address comments
  • Loading branch information
@yunhanw-google @pull
yunhanw-google authored and pull[bot] committed Jul 18, 2023
1 parent ae96f2f commit 3b319e9
Show file tree
Hide file tree
Showing 8 changed files with 662 additions and 130 deletions.
55 changes: 12 additions & 43 deletions src/app/MessageDef/SubscribeResponseMessage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
Expand All @@ -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)
Expand All @@ -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;
}
Expand Down
22 changes: 4 additions & 18 deletions src/app/MessageDef/SubscribeResponseMessage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
22 changes: 10 additions & 12 deletions src/app/ReadClient.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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());

Expand Down Expand Up @@ -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);
}

Expand Down
22 changes: 11 additions & 11 deletions src/app/ReadClient.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;

Expand Down
17 changes: 7 additions & 10 deletions src/app/ReadHandler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down Expand Up @@ -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.
Expand All @@ -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));
Expand All @@ -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);
}

Expand All @@ -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(
Expand Down
25 changes: 15 additions & 10 deletions src/app/ReadHandler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;

Expand Down
Loading

0 comments on commit 3b319e9

Please sign in to comment.