Skip to content

Commit

Permalink
address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@chrisdecenzo
chrisdecenzo committed Feb 15, 2024
1 parent 0c2fa27 commit 01ebed8
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 27 deletions.
3 changes: 2 additions & 1 deletion examples/tv-app/tv-common/clusters/messages/MessagesManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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;
Expand Down
31 changes: 16 additions & 15 deletions examples/tv-app/tv-common/clusters/messages/MessagesManager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,35 +34,32 @@ 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)
{
mResponseOptions.push_back(entry.GetMessageOption());
}
}

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<chip::app::Clusters::Messages::MessageControlBitmap> & messageControl,
Expand All @@ -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);
Expand All @@ -85,9 +83,9 @@ struct CachedMessage
{
if (mResponseOptions.size() > 0)
{
chip::app::DataModel::List<chip::app::Clusters::Messages::MessageResponseOption> options(mResponseOptions.data(),
mResponseOptions.size());
chip::app::Clusters::Messages::Structs::MessageStruct::Type message{ mMessageId,
chip::app::DataModel::List<chip::app::Clusters::Messages::Structs::MessageResponseOptionStruct::Type> options(
mResponseOptions.data(), mResponseOptions.size());
chip::app::Clusters::Messages::Structs::MessageStruct::Type message{ chip::ByteSpan(mMessageIdBuffer),
mPriority,
mMessageControl,
mStartTime,
Expand All @@ -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;
}

Expand All @@ -114,7 +115,7 @@ struct CachedMessage
std::string mMessageText;
uint8_t mMessageIdBuffer[chip::app::Clusters::Messages::kMessageIdLength];

std::vector<chip::app::Clusters::Messages::MessageResponseOption> mResponseOptions;
std::vector<chip::app::Clusters::Messages::Structs::MessageResponseOptionStruct::Type> mResponseOptions;
std::list<CachedMessageOption> mOptions;
};

Expand Down
14 changes: 6 additions & 8 deletions src/app/clusters/messages-server/messages-delegate.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -41,12 +39,12 @@ class Delegate
{
public:
// Commands
virtual CHIP_ERROR
HandlePresentMessagesRequest(const ByteSpan & messageId, const MessagePriorityEnum & priority,
const chip::BitMask<MessageControlBitmap> & messageControl,
const DataModel::Nullable<uint32_t> & startTime, const DataModel::Nullable<uint16_t> & duration,
const CharSpan & messageText,
const chip::Optional<DataModel::DecodableList<MessageResponseOption>> & responses) = 0;
virtual CHIP_ERROR HandlePresentMessagesRequest(
const ByteSpan & messageId, const MessagePriorityEnum & priority,
const chip::BitMask<MessageControlBitmap> & messageControl, const DataModel::Nullable<uint32_t> & startTime,
const DataModel::Nullable<uint16_t> & duration, const CharSpan & messageText,
const chip::Optional<DataModel::DecodableList<chip::app::Clusters::Messages::Structs::MessageResponseOptionStruct::Type>> &
responses) = 0;
virtual CHIP_ERROR HandleCancelMessagesRequest(const DataModel::DecodableList<chip::ByteSpan> & messageIds) = 0;

// Attributes
Expand Down
13 changes: 10 additions & 3 deletions src/app/clusters/messages-server/messages-server.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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");
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit 01ebed8

Please sign in to comment.