Skip to content

Commit

Permalink
address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
yunhanw-google committed Jun 10, 2022
1 parent 8a9cf83 commit f1dfa08
Show file tree
Hide file tree
Showing 7 changed files with 493 additions and 54 deletions.
11 changes: 4 additions & 7 deletions src/app/MessageDef/SubscribeResponseMessage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,7 @@ CHIP_ERROR SubscribeResponseMessage::Parser::CheckSchemaValidity() const
#endif // CHIP_DETAIL_LOGGING
break;
case to_underlying(Tag::kMaxInterval):
VerifyOrReturnError(!(tagPresenceMask & (1 << to_underlying(Tag::kMaxInterval))),
CHIP_ERROR_INVALID_TLV_TAG);
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
Expand All @@ -78,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::kMaxInterval));
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 @@ -107,8 +105,7 @@ SubscribeResponseMessage::Builder & SubscribeResponseMessage::Builder::Subscript
return *this;
}

SubscribeResponseMessage::Builder &
SubscribeResponseMessage::Builder::MaxInterval(const uint16_t aMaxInterval)
SubscribeResponseMessage::Builder & SubscribeResponseMessage::Builder::MaxInterval(const uint16_t aMaxInterval)
{
if (mError == CHIP_NO_ERROR)
{
Expand Down
4 changes: 2 additions & 2 deletions src/app/MessageDef/SubscribeResponseMessage.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ namespace app {
namespace SubscribeResponseMessage {
enum class Tag : uint8_t
{
kSubscriptionId = 0,
kMaxInterval = 2,
kSubscriptionId = 0,
kMaxInterval = 2,
};

class Parser : public MessageParser
Expand Down
18 changes: 8 additions & 10 deletions src/app/ReadClient.cpp
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;
mMaxInterval = 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(mMaxInterval) + 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 @@ -824,8 +823,7 @@ CHIP_ERROR ReadClient::ProcessSubscribeResponse(System::PacketBufferHandle && aP
ChipLogProgress(DataManagement,
"Subscription established with SubscriptionID = 0x%08" PRIx32 " MinInterval = %u"
"s MaxInterval = %us Peer = %02x:" ChipLogFormatX64,
mSubscriptionId, mMinIntervalFloorSeconds, mMaxInterval, mFabricIndex,
ChipLogValueX64(mPeerNodeId));
mSubscriptionId, mMinIntervalFloorSeconds, mMaxInterval, mFabricIndex, ChipLogValueX64(mPeerNodeId));

ReturnErrorOnFailure(subscribeResponse.ExitContainer());

Expand Down
20 changes: 10 additions & 10 deletions src/app/ReadClient.h
Original file line number Diff line number Diff line change
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 mMaxInterval = 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
11 changes: 5 additions & 6 deletions src/app/ReadHandler.h
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
#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
// 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 {
Expand Down Expand Up @@ -183,8 +183,7 @@ class ReadHandler : public Messaging::ExchangeDelegate
{
VerifyOrReturnError(IsIdle(), CHIP_ERROR_INCORRECT_STATE);
VerifyOrReturnError(mMinIntervalFloorSeconds <= aMaxInterval, CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(aMaxInterval <=
std::max(kSubscriptionMaxIntervalPublisherLimit, mMaxInterval),
VerifyOrReturnError(aMaxInterval <= std::max(kSubscriptionMaxIntervalPublisherLimit, mMaxInterval),
CHIP_ERROR_INVALID_ARGUMENT);
mMaxInterval = aMaxInterval;
return CHIP_NO_ERROR;
Expand Down Expand Up @@ -434,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 mMaxInterval = 0;
SubscriptionId mSubscriptionId = 0;
uint16_t mMinIntervalFloorSeconds = 0;
uint16_t mMaxInterval = 0;

EventNumber mEventMin = 0;

Expand Down
2 changes: 1 addition & 1 deletion src/app/tests/TestMessageDef.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1343,7 +1343,7 @@ void ParseSubscribeResponseMessage(nlTestSuite * apSuite, chip::TLV::TLVReader &

SubscribeResponseMessage::Parser subscribeResponseParser;
chip::SubscriptionId subscriptionId = 0;
uint16_t maxInterval = 0;
uint16_t maxInterval = 0;
err = subscribeResponseParser.Init(aReader);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
Expand Down
Loading

0 comments on commit f1dfa08

Please sign in to comment.