From 01ebed836870f9f7b47803741d10776d9dd4c9f9 Mon Sep 17 00:00:00 2001 From: chrisdecenzo Date: Thu, 15 Feb 2024 11:03:25 -0800 Subject: [PATCH] address comments --- .../clusters/messages/MessagesManager.cpp | 3 +- .../clusters/messages/MessagesManager.h | 31 ++++++++++--------- .../messages-server/messages-delegate.h | 14 ++++----- .../messages-server/messages-server.cpp | 13 ++++++-- 4 files changed, 34 insertions(+), 27 deletions(-) diff --git a/examples/tv-app/tv-common/clusters/messages/MessagesManager.cpp b/examples/tv-app/tv-common/clusters/messages/MessagesManager.cpp index a07da9c80e6bc6..33a5de94499275 100644 --- a/examples/tv-app/tv-common/clusters/messages/MessagesManager.cpp +++ b/examples/tv-app/tv-common/clusters/messages/MessagesManager.cpp @@ -25,6 +25,7 @@ using namespace chip; using namespace chip::app; using namespace chip::app::Clusters::Messages; using Message = chip::app::Clusters::Messages::Structs::MessageStruct::Type; +using MessageResponseOption = chip::app::Clusters::Messages::Structs::MessageResponseOptionStruct::Type; // Commands CHIP_ERROR MessagesManager::HandlePresentMessagesRequest( @@ -64,7 +65,7 @@ CHIP_ERROR MessagesManager::HandleCancelMessagesRequest(const DataModel::Decodab { auto & id = iter.GetValue(); - mCachedMessages.remove_if([id](CachedMessage & entry) { return entry.mMessageId.data_equal(id); }); + mCachedMessages.remove_if([id](CachedMessage & entry) { return entry.MessageIdMatches(id); }); // per spec, the command succeeds even when the message id does not match an existing message } return CHIP_NO_ERROR; diff --git a/examples/tv-app/tv-common/clusters/messages/MessagesManager.h b/examples/tv-app/tv-common/clusters/messages/MessagesManager.h index 8cda54b018dfaf..325cd56fc788af 100644 --- a/examples/tv-app/tv-common/clusters/messages/MessagesManager.h +++ b/examples/tv-app/tv-common/clusters/messages/MessagesManager.h @@ -34,27 +34,24 @@ struct CachedMessageOption mOption{ option.mOption.messageResponseID, chip::MakeOptional(chip::CharSpan::fromCharString(mLabel.c_str())) } {} - CachedMessageOption & operator=(const CachedMessageOption & option) { return *this; }; + CachedMessageOption & operator=(const CachedMessageOption & option) = delete; - chip::app::Clusters::Messages::MessageResponseOption GetMessageOption() { return mOption; } + chip::app::Clusters::Messages::Structs::MessageResponseOptionStruct::Type GetMessageOption() { return mOption; } ~CachedMessageOption() {} protected: std::string mLabel; - chip::app::Clusters::Messages::MessageResponseOption mOption; + chip::app::Clusters::Messages::Structs::MessageResponseOptionStruct::Type mOption; }; struct CachedMessage { - chip::ByteSpan mMessageId; - CachedMessage(const CachedMessage & message) : mPriority(message.mPriority), mMessageControl(message.mMessageControl), mStartTime(message.mStartTime), mDuration(message.mDuration), mMessageText(message.mMessageText), mOptions(message.mOptions) { memcpy(mMessageIdBuffer, message.mMessageIdBuffer, sizeof(mMessageIdBuffer)); - mMessageId = chip::ByteSpan(mMessageIdBuffer); for (CachedMessageOption & entry : mOptions) { @@ -62,7 +59,7 @@ struct CachedMessage } } - CachedMessage & operator=(const CachedMessage & message) { return *this; }; + CachedMessage & operator=(const CachedMessage & message) = delete; CachedMessage(const chip::ByteSpan & messageId, const chip::app::Clusters::Messages::MessagePriorityEnum & priority, const chip::BitMask & messageControl, @@ -72,9 +69,10 @@ struct CachedMessage mMessageControl(messageControl), mStartTime(startTime), mDuration(duration), mMessageText(messageText) { memcpy(mMessageIdBuffer, messageId.data(), sizeof(mMessageIdBuffer)); - mMessageId = chip::ByteSpan(mMessageIdBuffer); } + bool MessageIdMatches(const chip::ByteSpan & id) { return chip::ByteSpan(mMessageIdBuffer).data_equal(id); } + void AddOption(CachedMessageOption option) { mOptions.push_back(option); @@ -85,9 +83,9 @@ struct CachedMessage { if (mResponseOptions.size() > 0) { - chip::app::DataModel::List options(mResponseOptions.data(), - mResponseOptions.size()); - chip::app::Clusters::Messages::Structs::MessageStruct::Type message{ mMessageId, + chip::app::DataModel::List options( + mResponseOptions.data(), mResponseOptions.size()); + chip::app::Clusters::Messages::Structs::MessageStruct::Type message{ chip::ByteSpan(mMessageIdBuffer), mPriority, mMessageControl, mStartTime, @@ -97,9 +95,12 @@ struct CachedMessage chip::MakeOptional(options) }; return message; } - chip::app::Clusters::Messages::Structs::MessageStruct::Type message{ - mMessageId, mPriority, mMessageControl, mStartTime, mDuration, chip::CharSpan::fromCharString(mMessageText.c_str()) - }; + chip::app::Clusters::Messages::Structs::MessageStruct::Type message{ chip::ByteSpan(mMessageIdBuffer), + mPriority, + mMessageControl, + mStartTime, + mDuration, + chip::CharSpan::fromCharString(mMessageText.c_str()) }; return message; } @@ -114,7 +115,7 @@ struct CachedMessage std::string mMessageText; uint8_t mMessageIdBuffer[chip::app::Clusters::Messages::kMessageIdLength]; - std::vector mResponseOptions; + std::vector mResponseOptions; std::list mOptions; }; diff --git a/src/app/clusters/messages-server/messages-delegate.h b/src/app/clusters/messages-server/messages-delegate.h index 9c0b8094b6daa1..7949ceb71e4068 100644 --- a/src/app/clusters/messages-server/messages-delegate.h +++ b/src/app/clusters/messages-server/messages-delegate.h @@ -29,8 +29,6 @@ namespace app { namespace Clusters { namespace Messages { -using MessageResponseOption = chip::app::Clusters::Messages::Structs::MessageResponseOptionStruct::Type; - constexpr static size_t kMessageIdLength = 16; constexpr static size_t kMessageTextLengthMax = 256; constexpr static size_t kMessageMaxOptionCount = 4; @@ -41,12 +39,12 @@ class Delegate { public: // Commands - virtual CHIP_ERROR - HandlePresentMessagesRequest(const ByteSpan & messageId, const MessagePriorityEnum & priority, - const chip::BitMask & messageControl, - const DataModel::Nullable & startTime, const DataModel::Nullable & duration, - const CharSpan & messageText, - const chip::Optional> & responses) = 0; + virtual CHIP_ERROR HandlePresentMessagesRequest( + const ByteSpan & messageId, const MessagePriorityEnum & priority, + const chip::BitMask & messageControl, const DataModel::Nullable & startTime, + const DataModel::Nullable & duration, const CharSpan & messageText, + const chip::Optional> & + responses) = 0; virtual CHIP_ERROR HandleCancelMessagesRequest(const DataModel::DecodableList & messageIds) = 0; // Attributes diff --git a/src/app/clusters/messages-server/messages-server.cpp b/src/app/clusters/messages-server/messages-server.cpp index 04fd97444c466b..be5c8ddf0a60be 100644 --- a/src/app/clusters/messages-server/messages-server.cpp +++ b/src/app/clusters/messages-server/messages-server.cpp @@ -200,6 +200,12 @@ bool emberAfMessagesClusterPresentMessagesRequestCallback( ChipLogProgress(Zcl, "emberAfMessagesClusterPresentMessagesRequestCallback size check failed"); status = Status::ConstraintError); + VerifyOrExit( + delegate->HasFeature(endpoint, Feature::kConfirmationResponse), + ChipLogProgress( + Zcl, "emberAfMessagesClusterPresentMessagesRequestCallback responses sent but response feature not supported"); + status = Status::ConstraintError); + VerifyOrExit(size <= kMessageMaxOptionCount, ChipLogProgress(Zcl, "emberAfMessagesClusterPresentMessagesRequestCallback too many options"); status = Status::ConstraintError); @@ -209,9 +215,10 @@ bool emberAfMessagesClusterPresentMessagesRequestCallback( { auto & response = iter.GetValue(); + // response feature is checked above VerifyOrExit(response.messageResponseID.HasValue() && response.label.HasValue(), ChipLogProgress(Zcl, "emberAfMessagesClusterPresentMessagesRequestCallback missing response id or label"); - status = Status::InvalidDataType); + status = Status::InvalidCommand); VerifyOrExit(response.messageResponseID.Value() >= kMessageResponseIdMin, ChipLogProgress(Zcl, "emberAfMessagesClusterPresentMessagesRequestCallback responseID value check failed"); @@ -223,7 +230,7 @@ bool emberAfMessagesClusterPresentMessagesRequestCallback( } VerifyOrExit(iter.GetStatus() == CHIP_NO_ERROR, ChipLogProgress(Zcl, "emberAfMessagesClusterPresentMessagesRequestCallback TLV parsing error"); - status = Status::InvalidDataType); + status = Status::InvalidAction); } err = delegate->HandlePresentMessagesRequest(messageId, priority, messageControl, startTime, duration, messageText, responses); @@ -267,7 +274,7 @@ bool emberAfMessagesClusterCancelMessagesRequestCallback( } VerifyOrExit(iter.GetStatus() == CHIP_NO_ERROR, ChipLogProgress(Zcl, "emberAfMessagesClusterCancelMessagesRequestCallback TLV parsing error"); - status = Status::InvalidDataType); + status = Status::InvalidAction); } err = delegate->HandleCancelMessagesRequest(messageIds);