Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 3 commits into from
Jun 11, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4e7c8e4
Fix computation of min/max interval for subscriptions to be spec comp…
yunhanw-google Jun 7, 2022
8a9cf83
Fix logic in SetReportingIntervals
yunhanw-google Jun 7, 2022
5ecf97a
address comments
yunhanw-google Jun 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 12 additions & 43 deletions src/app/MessageDef/SubscribeResponseMessage.cpp
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
yunhanw-google marked this conversation as resolved.
Show resolved Hide resolved

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