From ad344f51882dcb343872d388d68aad939e36daa1 Mon Sep 17 00:00:00 2001 From: Tennessee Carmel-Veilleux Date: Fri, 14 Jun 2024 12:15:50 -0400 Subject: [PATCH] Add new AddStatus overloads using ClusterStatusCode (#33904) * Add new AddStatus overloads using ClusterStatusCode - CommandHandler and WriteHandler did not have a way to cleanly just `AddStatus` with a cluster-specific status code (to eventually harmonize handling methods to always return just a ClusterStatusCode). This PR: - Introduces an `AddStatus` overload for `ClusterStatusCode` to CommandHandler and WriteHandler. - Removes the unimplemented `WriteClient::Shutdown` method. - Adds notes to the `AddClusterSpecificSuccess` and `AddClusterSpecificFailure`. - Removes implicit conversion from `ClusterStatusCode` to `Status` - Was never used and was error-prone/lossy Fixes #31120 Testing done: - Updated necessary unit tests. - Added unit tests for cluster specific statuses on writes. - Integration tests still pass. * Restyled by clang-format * Improve type safety of test * Address review comments - Make StatusIB initializable from ClusterStatusCode - Clean-ups requested - CommandResponseHelper loses the error-prone cluster-specific-code on success (can be added back if ever needed). * Restyled by clang-format --------- Co-authored-by: Restyled.io --- src/app/CommandHandler.h | 47 ++++- src/app/CommandHandlerImpl.cpp | 46 +++-- src/app/CommandHandlerImpl.h | 10 +- src/app/CommandResponseHelper.h | 25 +-- src/app/MessageDef/StatusIB.h | 19 +- src/app/WriteClient.h | 6 - src/app/WriteHandler.cpp | 17 +- src/app/WriteHandler.h | 14 +- .../data-model-interface/InvokeResponder.h | 2 +- src/app/tests/TestStatusIB.cpp | 20 ++ .../tests/data_model/TestCommands.cpp | 7 +- src/controller/tests/data_model/TestWrite.cpp | 172 ++++++++++++++++-- src/protocols/interaction_model/StatusCode.h | 27 +-- .../tests/TestStatusCode.cpp | 8 +- 14 files changed, 320 insertions(+), 100 deletions(-) diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index 6c2af0e8cf1e1e..4771a45d38f493 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -25,6 +25,7 @@ #include #include #include +#include namespace chip { namespace app { @@ -116,21 +117,51 @@ class CommandHandler /** * Adds the given command status and returns any failures in adding statuses (e.g. out - * of buffer space) to the caller + * of buffer space) to the caller. `context` is an optional (if not nullptr) + * debug string to include in logging. */ virtual CHIP_ERROR FallibleAddStatus(const ConcreteCommandPath & aRequestCommandPath, - const Protocols::InteractionModel::Status aStatus, const char * context = nullptr) = 0; + const Protocols::InteractionModel::ClusterStatusCode & aStatus, + const char * context = nullptr) = 0; + CHIP_ERROR FallibleAddStatus(const ConcreteCommandPath & aRequestCommandPath, const Protocols::InteractionModel::Status aStatus, + const char * context = nullptr) + { + return FallibleAddStatus(aRequestCommandPath, Protocols::InteractionModel::ClusterStatusCode{ aStatus }, context); + } /** - * Adds a status when the caller is unable to handle any failures. Logging is performed - * and failure to register the status is checked with VerifyOrDie. + * Adds an IM global or Cluster status when the caller is unable to handle any failures. Logging is performed + * and failure to register the status is checked with VerifyOrDie. `context` is an optional (if not nullptr) + * debug string to include in logging. */ - virtual void AddStatus(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::Status aStatus, - const char * context = nullptr) = 0; + virtual void AddStatus(const ConcreteCommandPath & aRequestCommandPath, + const Protocols::InteractionModel::ClusterStatusCode & aStatus, const char * context = nullptr) = 0; + void AddStatus(const ConcreteCommandPath & aRequestCommandPath, const Protocols::InteractionModel::Status aStatus, + const char * context = nullptr) + { + AddStatus(aRequestCommandPath, Protocols::InteractionModel::ClusterStatusCode{ aStatus }, context); + } - virtual CHIP_ERROR AddClusterSpecificSuccess(const ConcreteCommandPath & aRequestCommandPath, ClusterStatus aClusterStatus) = 0; + /** + * Sets the response to indicate Success with a cluster-specific status code `aClusterStatus` included. + * + * NOTE: For regular success, what you want is AddStatus/FailibleAddStatus(aRequestCommandPath, + * InteractionModel::Status::Success). + */ + virtual CHIP_ERROR AddClusterSpecificSuccess(const ConcreteCommandPath & aRequestCommandPath, ClusterStatus aClusterStatus) + { + return FallibleAddStatus(aRequestCommandPath, + Protocols::InteractionModel::ClusterStatusCode::ClusterSpecificSuccess(aClusterStatus)); + } - virtual CHIP_ERROR AddClusterSpecificFailure(const ConcreteCommandPath & aRequestCommandPath, ClusterStatus aClusterStatus) = 0; + /** + * Sets the response to indicate Failure with a cluster-specific status code `aClusterStatus` included. + */ + virtual CHIP_ERROR AddClusterSpecificFailure(const ConcreteCommandPath & aRequestCommandPath, ClusterStatus aClusterStatus) + { + return FallibleAddStatus(aRequestCommandPath, + Protocols::InteractionModel::ClusterStatusCode::ClusterSpecificFailure(aClusterStatus)); + } /** * GetAccessingFabricIndex() may only be called during synchronous command diff --git a/src/app/CommandHandlerImpl.cpp b/src/app/CommandHandlerImpl.cpp index 926e5b3877c6bd..d57fab2b00f78a 100644 --- a/src/app/CommandHandlerImpl.cpp +++ b/src/app/CommandHandlerImpl.cpp @@ -19,6 +19,7 @@ #include #include +#include #include #include #include @@ -30,6 +31,7 @@ #include #include #include +#include #include namespace chip { @@ -592,11 +594,11 @@ CHIP_ERROR CommandHandlerImpl::AddStatusInternal(const ConcreteCommandPath & aCo return TryAddingResponse([&]() -> CHIP_ERROR { return TryAddStatusInternal(aCommandPath, aStatus); }); } -void CommandHandlerImpl::AddStatus(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::Status aStatus, - const char * context) +void CommandHandlerImpl::AddStatus(const ConcreteCommandPath & aCommandPath, + const Protocols::InteractionModel::ClusterStatusCode & status, const char * context) { - CHIP_ERROR error = FallibleAddStatus(aCommandPath, aStatus, context); + CHIP_ERROR error = FallibleAddStatus(aCommandPath, status, context); if (error != CHIP_NO_ERROR) { @@ -610,33 +612,37 @@ void CommandHandlerImpl::AddStatus(const ConcreteCommandPath & aCommandPath, con } } -CHIP_ERROR CommandHandlerImpl::FallibleAddStatus(const ConcreteCommandPath & path, const Protocols::InteractionModel::Status status, +CHIP_ERROR CommandHandlerImpl::FallibleAddStatus(const ConcreteCommandPath & path, + const Protocols::InteractionModel::ClusterStatusCode & status, const char * context) { - if (status != Status::Success) + if (!status.IsSuccess()) { if (context == nullptr) { context = "no additional context"; } - ChipLogError(DataManagement, - "Endpoint=%u Cluster=" ChipLogFormatMEI " Command=" ChipLogFormatMEI " status " ChipLogFormatIMStatus " (%s)", - path.mEndpointId, ChipLogValueMEI(path.mClusterId), ChipLogValueMEI(path.mCommandId), - ChipLogValueIMStatus(status), context); + if (status.HasClusterSpecificCode()) + { + ChipLogError(DataManagement, + "Endpoint=%u Cluster=" ChipLogFormatMEI " Command=" ChipLogFormatMEI " status " ChipLogFormatIMStatus + " ClusterSpecificCode=%u (%s)", + path.mEndpointId, ChipLogValueMEI(path.mClusterId), ChipLogValueMEI(path.mCommandId), + ChipLogValueIMStatus(status.GetStatus()), static_cast(status.GetClusterSpecificCode().Value()), + context); + } + else + { + ChipLogError(DataManagement, + "Endpoint=%u Cluster=" ChipLogFormatMEI " Command=" ChipLogFormatMEI " status " ChipLogFormatIMStatus + " (%s)", + path.mEndpointId, ChipLogValueMEI(path.mClusterId), ChipLogValueMEI(path.mCommandId), + ChipLogValueIMStatus(status.GetStatus()), context); + } } - return AddStatusInternal(path, StatusIB(status)); -} - -CHIP_ERROR CommandHandlerImpl::AddClusterSpecificSuccess(const ConcreteCommandPath & aCommandPath, ClusterStatus aClusterStatus) -{ - return AddStatusInternal(aCommandPath, StatusIB(Status::Success, aClusterStatus)); -} - -CHIP_ERROR CommandHandlerImpl::AddClusterSpecificFailure(const ConcreteCommandPath & aCommandPath, ClusterStatus aClusterStatus) -{ - return AddStatusInternal(aCommandPath, StatusIB(Status::Failure, aClusterStatus)); + return AddStatusInternal(path, StatusIB{ status }); } CHIP_ERROR CommandHandlerImpl::PrepareInvokeResponseCommand(const ConcreteCommandPath & aResponseCommandPath, diff --git a/src/app/CommandHandlerImpl.h b/src/app/CommandHandlerImpl.h index db7c8516bc5510..55e356bcceb35a 100644 --- a/src/app/CommandHandlerImpl.h +++ b/src/app/CommandHandlerImpl.h @@ -29,6 +29,7 @@ #include #include #include +#include #include #include @@ -114,15 +115,16 @@ class CommandHandlerImpl : public CommandHandler /**************** CommandHandler interface implementation ***********************/ using CommandHandler::AddResponseData; + using CommandHandler::AddStatus; + using CommandHandler::FallibleAddStatus; void FlushAcksRightAwayOnSlowCommand() override; - CHIP_ERROR FallibleAddStatus(const ConcreteCommandPath & aRequestCommandPath, const Protocols::InteractionModel::Status aStatus, + CHIP_ERROR FallibleAddStatus(const ConcreteCommandPath & aRequestCommandPath, + const Protocols::InteractionModel::ClusterStatusCode & aStatus, const char * context = nullptr) override; - void AddStatus(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::Status aStatus, + void AddStatus(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::ClusterStatusCode & aStatus, const char * context = nullptr) override; - CHIP_ERROR AddClusterSpecificSuccess(const ConcreteCommandPath & aRequestCommandPath, ClusterStatus aClusterStatus) override; - CHIP_ERROR AddClusterSpecificFailure(const ConcreteCommandPath & aRequestCommandPath, ClusterStatus aClusterStatus) override; CHIP_ERROR AddResponseData(const ConcreteCommandPath & aRequestCommandPath, CommandId aResponseCommandId, const DataModel::EncodableToTLV & aEncodable) override; diff --git a/src/app/CommandResponseHelper.h b/src/app/CommandResponseHelper.h index a0209acbe32194..9d8112fe8514e8 100644 --- a/src/app/CommandResponseHelper.h +++ b/src/app/CommandResponseHelper.h @@ -21,6 +21,7 @@ #include #include #include +#include namespace chip { namespace app { @@ -38,35 +39,27 @@ class CommandResponseHelper mCommandHandler->AddResponse(mCommandPath, aResponse); mSentResponse = true; return CHIP_NO_ERROR; - }; + } - CHIP_ERROR Success(ClusterStatus aClusterStatus) + CHIP_ERROR Success() { - CHIP_ERROR err = mCommandHandler->AddClusterSpecificSuccess(mCommandPath, aClusterStatus); - if (err == CHIP_NO_ERROR) - { - mSentResponse = true; - } + CHIP_ERROR err = mCommandHandler->FallibleAddStatus(mCommandPath, Protocols::InteractionModel::Status::Success); + mSentResponse = (err == CHIP_NO_ERROR); return err; } CHIP_ERROR Failure(Protocols::InteractionModel::Status aStatus) { CHIP_ERROR err = mCommandHandler->FallibleAddStatus(mCommandPath, aStatus); - if (err == CHIP_NO_ERROR) - { - mSentResponse = true; - } + mSentResponse = (err == CHIP_NO_ERROR); return err; } CHIP_ERROR Failure(ClusterStatus aClusterStatus) { - CHIP_ERROR err = mCommandHandler->AddClusterSpecificFailure(mCommandPath, aClusterStatus); - if (err == CHIP_NO_ERROR) - { - mSentResponse = true; - } + CHIP_ERROR err = mCommandHandler->FallibleAddStatus( + mCommandPath, Protocols::InteractionModel::ClusterStatusCode::ClusterSpecificFailure(aClusterStatus)); + mSentResponse = (err == CHIP_NO_ERROR); return err; } diff --git a/src/app/MessageDef/StatusIB.h b/src/app/MessageDef/StatusIB.h index f5ade6cc71c628..08137045ffcd7b 100644 --- a/src/app/MessageDef/StatusIB.h +++ b/src/app/MessageDef/StatusIB.h @@ -34,6 +34,7 @@ #include #include #include +#include #include namespace chip { @@ -41,10 +42,24 @@ namespace app { struct StatusIB { StatusIB() = default; - StatusIB(Protocols::InteractionModel::Status imStatus) : mStatus(imStatus) {} - StatusIB(Protocols::InteractionModel::Status imStatus, ClusterStatus clusterStatus) : + explicit StatusIB(Protocols::InteractionModel::Status imStatus) : mStatus(imStatus) {} + + explicit StatusIB(Protocols::InteractionModel::Status imStatus, ClusterStatus clusterStatus) : mStatus(imStatus), mClusterStatus(clusterStatus) {} + + explicit StatusIB(const Protocols::InteractionModel::ClusterStatusCode & statusCode) : mStatus(statusCode.GetStatus()) + { + // NOTE: Cluster-specific codes are only valid on SUCCESS/FAILURE IM status (7.10.7. Status Codes) + chip::Optional clusterStatus = statusCode.GetClusterSpecificCode(); + if (clusterStatus.HasValue()) + { + mStatus = statusCode.IsSuccess() ? Protocols::InteractionModel::Status::Success + : Protocols::InteractionModel::Status::Failure; + mClusterStatus = clusterStatus; + } + } + explicit StatusIB(CHIP_ERROR error) { InitFromChipError(error); } enum class Tag : uint8_t diff --git a/src/app/WriteClient.h b/src/app/WriteClient.h index 90f0f3a24b6c1a..b67bf4ea0b8c84 100644 --- a/src/app/WriteClient.h +++ b/src/app/WriteClient.h @@ -228,12 +228,6 @@ class WriteClient : public Messaging::ExchangeDelegate */ CHIP_ERROR SendWriteRequest(const SessionHandle & session, System::Clock::Timeout timeout = System::Clock::kZero); - /** - * Shutdown the WriteClient. This terminates this instance - * of the object and releases all held resources. - */ - void Shutdown(); - private: friend class TestWriteInteraction; friend class InteractionModelEngine; diff --git a/src/app/WriteHandler.cpp b/src/app/WriteHandler.cpp index 068f1f6b5c3004..aadb59ae864176 100644 --- a/src/app/WriteHandler.cpp +++ b/src/app/WriteHandler.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -28,6 +29,7 @@ #include #include #include +#include namespace chip { namespace app { @@ -315,7 +317,7 @@ CHIP_ERROR WriteHandler::ProcessAttributeDataIBs(TLV::TLVReader & aAttributeData // it with Busy status code. (dataAttributePath.IsListItemOperation() && !IsSameAttribute(mProcessingAttributePath, dataAttributePath))) { - err = AddStatus(dataAttributePath, StatusIB(Status::Busy)); + err = AddStatusInternal(dataAttributePath, StatusIB(Status::Busy)); continue; } @@ -353,7 +355,7 @@ CHIP_ERROR WriteHandler::ProcessAttributeDataIBs(TLV::TLVReader & aAttributeData if (err != CHIP_NO_ERROR) { mWriteResponseBuilder.GetWriteResponses().Rollback(backup); - err = AddStatus(dataAttributePath, StatusIB(err)); + err = AddStatusInternal(dataAttributePath, StatusIB(err)); } DataModelCallbacks::GetInstance()->AttributeOperation(DataModelCallbacks::OperationType::Write, @@ -616,22 +618,23 @@ Status WriteHandler::ProcessWriteRequest(System::PacketBufferHandle && aPayload, return status; } -CHIP_ERROR WriteHandler::AddStatus(const ConcreteDataAttributePath & aPath, const Protocols::InteractionModel::Status aStatus) +CHIP_ERROR WriteHandler::AddStatus(const ConcreteDataAttributePath & aPath, + const Protocols::InteractionModel::ClusterStatusCode & aStatus) { - return AddStatus(aPath, StatusIB(aStatus)); + return AddStatusInternal(aPath, StatusIB{ aStatus }); } CHIP_ERROR WriteHandler::AddClusterSpecificSuccess(const ConcreteDataAttributePath & aPath, ClusterStatus aClusterStatus) { - return AddStatus(aPath, StatusIB(Status::Success, aClusterStatus)); + return AddStatus(aPath, Protocols::InteractionModel::ClusterStatusCode::ClusterSpecificSuccess(aClusterStatus)); } CHIP_ERROR WriteHandler::AddClusterSpecificFailure(const ConcreteDataAttributePath & aPath, ClusterStatus aClusterStatus) { - return AddStatus(aPath, StatusIB(Status::Failure, aClusterStatus)); + return AddStatus(aPath, Protocols::InteractionModel::ClusterStatusCode::ClusterSpecificFailure(aClusterStatus)); } -CHIP_ERROR WriteHandler::AddStatus(const ConcreteDataAttributePath & aPath, const StatusIB & aStatus) +CHIP_ERROR WriteHandler::AddStatusInternal(const ConcreteDataAttributePath & aPath, const StatusIB & aStatus) { AttributeStatusIBs::Builder & writeResponses = mWriteResponseBuilder.GetWriteResponses(); AttributeStatusIB::Builder & attributeStatusIB = writeResponses.CreateAttributeStatus(); diff --git a/src/app/WriteHandler.h b/src/app/WriteHandler.h index 907aa043561eed..1884654f070710 100644 --- a/src/app/WriteHandler.h +++ b/src/app/WriteHandler.h @@ -31,6 +31,7 @@ #include #include #include +#include #include #include @@ -100,11 +101,14 @@ class WriteHandler : public Messaging::ExchangeDelegate CHIP_ERROR ProcessAttributeDataIBs(TLV::TLVReader & aAttributeDataIBsReader); CHIP_ERROR ProcessGroupAttributeDataIBs(TLV::TLVReader & aAttributeDataIBsReader); - CHIP_ERROR AddStatus(const ConcreteDataAttributePath & aPath, const Protocols::InteractionModel::Status aStatus); - - CHIP_ERROR AddClusterSpecificSuccess(const ConcreteDataAttributePath & aAttributePathParams, uint8_t aClusterStatus); + CHIP_ERROR AddStatus(const ConcreteDataAttributePath & aPath, const Protocols::InteractionModel::ClusterStatusCode & aStatus); + CHIP_ERROR AddStatus(const ConcreteDataAttributePath & aPath, const Protocols::InteractionModel::Status aStatus) + { + return AddStatus(aPath, Protocols::InteractionModel::ClusterStatusCode{ aStatus }); + } - CHIP_ERROR AddClusterSpecificFailure(const ConcreteDataAttributePath & aAttributePathParams, uint8_t aClusterStatus); + CHIP_ERROR AddClusterSpecificSuccess(const ConcreteDataAttributePath & aAttributePathParams, ClusterStatus aClusterStatus); + CHIP_ERROR AddClusterSpecificFailure(const ConcreteDataAttributePath & aAttributePathParams, ClusterStatus aClusterStatus); FabricIndex GetAccessingFabricIndex() const; @@ -166,7 +170,7 @@ class WriteHandler : public Messaging::ExchangeDelegate // ProcessGroupAttributeDataIBs. CHIP_ERROR DeliverFinalListWriteEndForGroupWrite(bool writeWasSuccessful); - CHIP_ERROR AddStatus(const ConcreteDataAttributePath & aPath, const StatusIB & aStatus); + CHIP_ERROR AddStatusInternal(const ConcreteDataAttributePath & aPath, const StatusIB & aStatus); private: // ExchangeDelegate diff --git a/src/app/data-model-interface/InvokeResponder.h b/src/app/data-model-interface/InvokeResponder.h index 54ae9ba0f51a08..9890c3bb6f6d7e 100644 --- a/src/app/data-model-interface/InvokeResponder.h +++ b/src/app/data-model-interface/InvokeResponder.h @@ -138,7 +138,7 @@ class AutoCompleteInvokeResponder { if (mCompleteState != CompleteState::kComplete) { - mWriter->Complete(Protocols::InteractionModel::Status::Failure); + mWriter->Complete(StatusIB{ Protocols::InteractionModel::Status::Failure }); } } diff --git a/src/app/tests/TestStatusIB.cpp b/src/app/tests/TestStatusIB.cpp index 8da643d2b71a31..22806168a9e6da 100644 --- a/src/app/tests/TestStatusIB.cpp +++ b/src/app/tests/TestStatusIB.cpp @@ -161,4 +161,24 @@ TEST_F(TestStatusIB, TestStatusIBEqualityOperator) EXPECT_NE(invalid_argument, StatusIB(CHIP_NO_ERROR)); } +TEST_F(TestStatusIB, ConversionsFromClusterStatusCodeWork) +{ + StatusIB successWithCode{ ClusterStatusCode::ClusterSpecificSuccess(123u) }; + EXPECT_EQ(successWithCode.mStatus, Status::Success); + EXPECT_TRUE(successWithCode.IsSuccess()); + ASSERT_TRUE(successWithCode.mClusterStatus.HasValue()); + EXPECT_EQ(successWithCode.mClusterStatus.Value(), 123u); + + StatusIB failureWithCode{ ClusterStatusCode::ClusterSpecificFailure(42u) }; + EXPECT_EQ(failureWithCode.mStatus, Status::Failure); + EXPECT_FALSE(failureWithCode.IsSuccess()); + ASSERT_TRUE(failureWithCode.mClusterStatus.HasValue()); + EXPECT_EQ(failureWithCode.mClusterStatus.Value(), 42u); + + StatusIB imStatusInClusterStatusCode{ ClusterStatusCode{ Status::ConstraintError } }; + EXPECT_EQ(imStatusInClusterStatusCode.mStatus, Status::ConstraintError); + EXPECT_FALSE(imStatusInClusterStatusCode.IsSuccess()); + EXPECT_FALSE(imStatusInClusterStatusCode.mClusterStatus.HasValue()); +} + } // namespace diff --git a/src/controller/tests/data_model/TestCommands.cpp b/src/controller/tests/data_model/TestCommands.cpp index adc6bd0cf7c9d0..b6d39ae41978bc 100644 --- a/src/controller/tests/data_model/TestCommands.cpp +++ b/src/controller/tests/data_model/TestCommands.cpp @@ -37,6 +37,7 @@ #include #include #include +#include using TestContext = chip::Test::AppContext; @@ -144,11 +145,13 @@ void DispatchSingleClusterCommand(const ConcreteCommandPath & aCommandPath, chip } else if (responseDirective == kSendSuccessStatusCodeWithClusterStatus) { - apCommandObj->AddClusterSpecificSuccess(aCommandPath, kTestSuccessClusterStatus); + apCommandObj->AddStatus( + aCommandPath, Protocols::InteractionModel::ClusterStatusCode::ClusterSpecificSuccess(kTestSuccessClusterStatus)); } else if (responseDirective == kSendErrorWithClusterStatus) { - apCommandObj->AddClusterSpecificFailure(aCommandPath, kTestFailureClusterStatus); + apCommandObj->AddStatus( + aCommandPath, Protocols::InteractionModel::ClusterStatusCode::ClusterSpecificFailure(kTestFailureClusterStatus)); } else if (responseDirective == kAsync) { diff --git a/src/controller/tests/data_model/TestWrite.cpp b/src/controller/tests/data_model/TestWrite.cpp index 28f57970dd6b6c..e0f2c19480a1b0 100644 --- a/src/controller/tests/data_model/TestWrite.cpp +++ b/src/controller/tests/data_model/TestWrite.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -43,15 +44,20 @@ constexpr EndpointId kTestEndpointId = 1; constexpr DataVersion kRejectedDataVersion = 1; constexpr DataVersion kAcceptedDataVersion = 5; +constexpr uint8_t kExampleClusterSpecificSuccess = 11u; +constexpr uint8_t kExampleClusterSpecificFailure = 12u; + enum ResponseDirective { kSendAttributeSuccess, kSendAttributeError, kSendMultipleSuccess, kSendMultipleErrors, + kSendClusterSpecificSuccess, + kSendClusterSpecificFailure, }; -ResponseDirective responseDirective; +ResponseDirective gResponseDirective = kSendAttributeSuccess; } // namespace @@ -78,7 +84,7 @@ CHIP_ERROR WriteSingleClusterData(const Access::SubjectDescriptor & aSubjectDesc if (aPath.mClusterId == Clusters::UnitTesting::Id && aPath.mAttributeId == Attributes::ListStructOctetString::TypeInfo::GetAttributeId()) { - if (responseDirective == kSendAttributeSuccess) + if (gResponseDirective == kSendAttributeSuccess) { if (!aPath.IsListOperation() || aPath.mListOp == ConcreteDataAttributePath::ListOperation::ReplaceAll) { @@ -153,20 +159,22 @@ CHIP_ERROR WriteSingleClusterData(const Access::SubjectDescriptor & aSubjectDesc return CHIP_NO_ERROR; } + // Boolean attribute of unit testing cluster triggers "multiple errors" case. if (aPath.mClusterId == Clusters::UnitTesting::Id && aPath.mAttributeId == Attributes::Boolean::TypeInfo::GetAttributeId()) { - InteractionModel::Status status; - if (responseDirective == kSendMultipleSuccess) + InteractionModel::ClusterStatusCode status{ Protocols::InteractionModel::Status::InvalidValue }; + + if (gResponseDirective == kSendMultipleSuccess) { status = InteractionModel::Status::Success; } - else if (responseDirective == kSendMultipleErrors) + else if (gResponseDirective == kSendMultipleErrors) { status = InteractionModel::Status::Failure; } else { - return CHIP_ERROR_INCORRECT_STATE; + VerifyOrDie(false); } for (size_t i = 0; i < 4; ++i) @@ -177,8 +185,69 @@ CHIP_ERROR WriteSingleClusterData(const Access::SubjectDescriptor & aSubjectDesc return CHIP_NO_ERROR; } + if (aPath.mClusterId == Clusters::UnitTesting::Id && aPath.mAttributeId == Attributes::Int8u::TypeInfo::GetAttributeId()) + { + InteractionModel::ClusterStatusCode status{ Protocols::InteractionModel::Status::InvalidValue }; + if (gResponseDirective == kSendClusterSpecificSuccess) + { + status = InteractionModel::ClusterStatusCode::ClusterSpecificSuccess(kExampleClusterSpecificSuccess); + } + else if (gResponseDirective == kSendClusterSpecificFailure) + { + status = InteractionModel::ClusterStatusCode::ClusterSpecificFailure(kExampleClusterSpecificFailure); + } + else + { + VerifyOrDie(false); + } + + aWriteHandler->AddStatus(aPath, status); + return CHIP_NO_ERROR; + } + return CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE; } + +class SingleWriteCallback : public WriteClient::Callback +{ +public: + explicit SingleWriteCallback(ConcreteAttributePath path) : mPath(path) {} + + void OnResponse(const WriteClient * apWriteClient, const ConcreteDataAttributePath & aPath, StatusIB attributeStatus) override + { + + if (aPath.MatchesConcreteAttributePath(mPath)) + { + mPathWasReponded = true; + mPathStatus = attributeStatus; + } + } + + void OnError(const WriteClient * apWriteClient, CHIP_ERROR aError) override + { + (void) apWriteClient; + mLastChipError = aError; + } + + void OnDone(WriteClient * apWriteClient) override + { + (void) apWriteClient; + mOnDoneCalled = true; + } + + bool WasDone() const { return mOnDoneCalled; } + bool PathWasResponded() const { return mOnDoneCalled; } + CHIP_ERROR GetLastChipError() const { return mLastChipError; } + StatusIB GetPathStatus() const { return mPathStatus; } + +private: + ConcreteAttributePath mPath; + bool mOnDoneCalled = false; + CHIP_ERROR mLastChipError = CHIP_NO_ERROR; + bool mPathWasReponded = false; + StatusIB mPathStatus; +}; + } // namespace app } // namespace chip @@ -209,6 +278,12 @@ class TestWrite : public ::testing::Test } } + void ResetCallback() { mSingleWriteCallback.reset(); } + + void PrepareWriteCallback(ConcreteAttributePath path) { mSingleWriteCallback = std::make_unique(path); } + + SingleWriteCallback * GetWriteCallback() { return mSingleWriteCallback.get(); } + protected: // Performs setup for each individual test in the test suite void SetUp() { mpContext->SetUp(); } @@ -217,7 +292,10 @@ class TestWrite : public ::testing::Test void TearDown() { mpContext->TearDown(); } static TestContext * mpContext; + + std::unique_ptr mSingleWriteCallback; }; + TestContext * TestWrite::mpContext = nullptr; TEST_F(TestWrite, TestDataResponse) @@ -236,7 +314,7 @@ TEST_F(TestWrite, TestDataResponse) i++; } - responseDirective = kSendAttributeSuccess; + gResponseDirective = kSendAttributeSuccess; // Passing of stack variables by reference is only safe because of synchronous completion of the interaction. Otherwise, it's // not safe to do so. @@ -274,7 +352,7 @@ TEST_F(TestWrite, TestDataResponseWithAcceptedDataVersion) i++; } - responseDirective = kSendAttributeSuccess; + gResponseDirective = kSendAttributeSuccess; // Passing of stack variables by reference is only safe because of synchronous completion of the interaction. Otherwise, it's // not safe to do so. @@ -314,7 +392,7 @@ TEST_F(TestWrite, TestDataResponseWithRejectedDataVersion) i++; } - responseDirective = kSendAttributeSuccess; + gResponseDirective = kSendAttributeSuccess; // Passing of stack variables by reference is only safe because of synchronous completion of the interaction. Otherwise, it's // not safe to do so. @@ -353,7 +431,7 @@ TEST_F(TestWrite, TestAttributeError) i++; } - responseDirective = kSendAttributeError; + gResponseDirective = kSendAttributeError; // Passing of stack variables by reference is only safe because of synchronous completion of the interaction. Otherwise, it's // not safe to do so. @@ -419,7 +497,7 @@ TEST_F(TestWrite, TestMultipleSuccessResponses) size_t successCalls = 0; size_t failureCalls = 0; - responseDirective = kSendMultipleSuccess; + gResponseDirective = kSendMultipleSuccess; // Passing of stack variables by reference is only safe because of synchronous completion of the interaction. Otherwise, it's // not safe to do so. @@ -446,7 +524,7 @@ TEST_F(TestWrite, TestMultipleFailureResponses) size_t successCalls = 0; size_t failureCalls = 0; - responseDirective = kSendMultipleErrors; + gResponseDirective = kSendMultipleErrors; // Passing of stack variables by reference is only safe because of synchronous completion of the interaction. Otherwise, it's // not safe to do so. @@ -467,4 +545,74 @@ TEST_F(TestWrite, TestMultipleFailureResponses) EXPECT_EQ(mpContext->GetExchangeManager().GetNumActiveExchanges(), 0u); } +TEST_F(TestWrite, TestWriteClusterSpecificStatuses) +{ + auto sessionHandle = mpContext->GetSessionBobToAlice(); + + // Cluster-specific success code case + { + gResponseDirective = kSendClusterSpecificSuccess; + + this->ResetCallback(); + this->PrepareWriteCallback( + ConcreteAttributePath{ kTestEndpointId, Clusters::UnitTesting::Id, Clusters::UnitTesting::Attributes::Int8u::Id }); + + SingleWriteCallback * writeCb = this->GetWriteCallback(); + + WriteClient writeClient(&mpContext->GetExchangeManager(), this->GetWriteCallback(), Optional::Missing()); + AttributePathParams attributePath{ kTestEndpointId, Clusters::UnitTesting::Id, + Clusters::UnitTesting::Attributes::Int8u::Id }; + constexpr uint8_t attributeValue = 1u; + ASSERT_EQ(writeClient.EncodeAttribute(attributePath, attributeValue), CHIP_NO_ERROR); + ASSERT_EQ(writeClient.SendWriteRequest(sessionHandle), CHIP_NO_ERROR); + + mpContext->DrainAndServiceIO(); + + EXPECT_TRUE(writeCb->WasDone()); + EXPECT_TRUE(writeCb->PathWasResponded()); + EXPECT_EQ(writeCb->GetLastChipError(), CHIP_NO_ERROR); + + StatusIB pathStatus = writeCb->GetPathStatus(); + EXPECT_EQ(pathStatus.mStatus, Protocols::InteractionModel::Status::Success); + ASSERT_TRUE(pathStatus.mClusterStatus.HasValue()); + EXPECT_EQ(pathStatus.mClusterStatus.Value(), kExampleClusterSpecificSuccess); + + EXPECT_EQ(chip::app::InteractionModelEngine::GetInstance()->GetNumActiveWriteHandlers(), 0u); + EXPECT_EQ(mpContext->GetExchangeManager().GetNumActiveExchanges(), 0u); + } + + // Cluster-specific failure code case + { + gResponseDirective = kSendClusterSpecificFailure; + + this->ResetCallback(); + this->PrepareWriteCallback( + ConcreteAttributePath{ kTestEndpointId, Clusters::UnitTesting::Id, Clusters::UnitTesting::Attributes::Int8u::Id }); + + SingleWriteCallback * writeCb = this->GetWriteCallback(); + + WriteClient writeClient(&mpContext->GetExchangeManager(), this->GetWriteCallback(), Optional::Missing()); + AttributePathParams attributePath{ kTestEndpointId, Clusters::UnitTesting::Id, + Clusters::UnitTesting::Attributes::Int8u::Id }; + + constexpr uint8_t attributeValue = 2u; + ASSERT_EQ(writeClient.EncodeAttribute(attributePath, attributeValue), CHIP_NO_ERROR); + ASSERT_EQ(writeClient.SendWriteRequest(sessionHandle), CHIP_NO_ERROR); + + mpContext->DrainAndServiceIO(); + + EXPECT_TRUE(writeCb->WasDone()); + EXPECT_TRUE(writeCb->PathWasResponded()); + EXPECT_EQ(writeCb->GetLastChipError(), CHIP_NO_ERROR); + + StatusIB pathStatus = writeCb->GetPathStatus(); + EXPECT_EQ(pathStatus.mStatus, Protocols::InteractionModel::Status::Failure); + ASSERT_TRUE(pathStatus.mClusterStatus.HasValue()); + EXPECT_EQ(pathStatus.mClusterStatus.Value(), kExampleClusterSpecificFailure); + + EXPECT_EQ(chip::app::InteractionModelEngine::GetInstance()->GetNumActiveWriteHandlers(), 0u); + EXPECT_EQ(mpContext->GetExchangeManager().GetNumActiveExchanges(), 0u); + } +} + } // namespace diff --git a/src/protocols/interaction_model/StatusCode.h b/src/protocols/interaction_model/StatusCode.h index b004b4e04a9057..9326c86653759c 100644 --- a/src/protocols/interaction_model/StatusCode.h +++ b/src/protocols/interaction_model/StatusCode.h @@ -18,6 +18,8 @@ #pragma once #include +#include + #include #include @@ -58,10 +60,6 @@ const char * StatusName(Status status); * are the components of a StatusIB, used in many IM actions, in a * way which allows both of them to carry together. * - * This can be used everywhere a `Status` is used, but it is lossy - * to the cluster-specific code if used in place of `Status` when - * the cluster-specific code is set. - * * This class can only be directly constructed from a `Status`. To * attach a cluster-specific-code, please use the `ClusterSpecificFailure()` * and `ClusterSpecificSuccess()` factory methods. @@ -75,13 +73,13 @@ class ClusterStatusCode ClusterStatusCode(const ClusterStatusCode & other) = default; ClusterStatusCode & operator=(const ClusterStatusCode & other) = default; - bool operator==(const ClusterStatusCode & other) + bool operator==(const ClusterStatusCode & other) const { return (this->mStatus == other.mStatus) && (this->HasClusterSpecificCode() == other.HasClusterSpecificCode()) && (this->GetClusterSpecificCode() == other.GetClusterSpecificCode()); } - bool operator!=(const ClusterStatusCode & other) { return !(*this == other); } + bool operator!=(const ClusterStatusCode & other) const { return !(*this == other); } ClusterStatusCode & operator=(const Status & status) { @@ -99,7 +97,7 @@ class ClusterStatusCode * (e.g. chip::app::Clusters::AdministratorCommissioning::CommissioningWindowStatusEnum::kWindowNotOpen) * @return a ClusterStatusCode instance properly configured. */ - template + template ::value, bool> = true> static ClusterStatusCode ClusterSpecificFailure(T cluster_specific_code) { static_assert(std::numeric_limits>::max() <= std::numeric_limits::max(), @@ -107,6 +105,11 @@ class ClusterStatusCode return ClusterStatusCode(Status::Failure, chip::to_underlying(cluster_specific_code)); } + static ClusterStatusCode ClusterSpecificFailure(ClusterStatus cluster_specific_code) + { + return ClusterStatusCode(Status::Failure, cluster_specific_code); + } + /** * @brief Builder for a cluster-specific success status code. * @@ -116,7 +119,7 @@ class ClusterStatusCode * (e.g. chip::app::Clusters::AdministratorCommissioning::CommissioningWindowStatusEnum::kBasicWindowOpen) * @return a ClusterStatusCode instance properly configured. */ - template + template ::value, bool> = true> static ClusterStatusCode ClusterSpecificSuccess(T cluster_specific_code) { static_assert(std::numeric_limits>::max() <= std::numeric_limits::max(), @@ -124,6 +127,11 @@ class ClusterStatusCode return ClusterStatusCode(Status::Success, chip::to_underlying(cluster_specific_code)); } + static ClusterStatusCode ClusterSpecificSuccess(ClusterStatus cluster_specific_code) + { + return ClusterStatusCode(Status::Success, cluster_specific_code); + } + /// @return true if the core Status associated with this ClusterStatusCode is the one for success. bool IsSuccess() const { return mStatus == Status::Success; } @@ -143,9 +151,6 @@ class ClusterStatusCode return mClusterSpecificCode; } - // Automatic conversions to common types, using the status code alone. - operator Status() const { return mStatus; } - private: ClusterStatusCode() = delete; ClusterStatusCode(Status status, ClusterStatus cluster_specific_code) : diff --git a/src/protocols/interaction_model/tests/TestStatusCode.cpp b/src/protocols/interaction_model/tests/TestStatusCode.cpp index 2be78c4f3ac418..3183d68e2e5f22 100644 --- a/src/protocols/interaction_model/tests/TestStatusCode.cpp +++ b/src/protocols/interaction_model/tests/TestStatusCode.cpp @@ -40,26 +40,22 @@ TEST(TestStatusCode, TestClusterStatusCode) // Basic usage as a Status. { ClusterStatusCode status_code_success{ Status::Success }; - EXPECT_EQ(status_code_success, Status::Success); EXPECT_EQ(status_code_success.GetStatus(), Status::Success); EXPECT_FALSE(status_code_success.HasClusterSpecificCode()); EXPECT_EQ(status_code_success.GetClusterSpecificCode(), chip::NullOptional); EXPECT_TRUE(status_code_success.IsSuccess()); ClusterStatusCode status_code_failure{ Status::Failure }; - EXPECT_EQ(status_code_failure, Status::Failure); EXPECT_EQ(status_code_failure.GetStatus(), Status::Failure); EXPECT_FALSE(status_code_failure.HasClusterSpecificCode()); EXPECT_FALSE(status_code_failure.IsSuccess()); ClusterStatusCode status_code_unsupported_ep{ Status::UnsupportedEndpoint }; - EXPECT_EQ(status_code_unsupported_ep, Status::UnsupportedEndpoint); EXPECT_EQ(status_code_unsupported_ep.GetStatus(), Status::UnsupportedEndpoint); EXPECT_FALSE(status_code_unsupported_ep.HasClusterSpecificCode()); EXPECT_FALSE(status_code_unsupported_ep.IsSuccess()); ClusterStatusCode status_code_invalid_in_state{ Status::InvalidInState }; - EXPECT_EQ(status_code_invalid_in_state, Status::InvalidInState); EXPECT_EQ(status_code_invalid_in_state.GetStatus(), Status::InvalidInState); EXPECT_FALSE(status_code_invalid_in_state.HasClusterSpecificCode()); EXPECT_FALSE(status_code_invalid_in_state.IsSuccess()); @@ -74,13 +70,13 @@ TEST(TestStatusCode, TestClusterStatusCode) // Cluster-specific usage. { ClusterStatusCode status_code_success = ClusterStatusCode::ClusterSpecificSuccess(RobotoClusterStatus::kSauceSuccess); - EXPECT_EQ(status_code_success, Status::Success); + EXPECT_EQ(status_code_success.GetStatus(), Status::Success); EXPECT_TRUE(status_code_success.HasClusterSpecificCode()); EXPECT_EQ(status_code_success.GetClusterSpecificCode(), static_cast(RobotoClusterStatus::kSauceSuccess)); EXPECT_TRUE(status_code_success.IsSuccess()); ClusterStatusCode status_code_failure = ClusterStatusCode::ClusterSpecificFailure(RobotoClusterStatus::kSandwichError); - EXPECT_EQ(status_code_failure, Status::Failure); + EXPECT_EQ(status_code_failure.GetStatus(), Status::Failure); EXPECT_TRUE(status_code_failure.HasClusterSpecificCode()); EXPECT_EQ(status_code_failure.GetClusterSpecificCode(), static_cast(RobotoClusterStatus::kSandwichError)); EXPECT_FALSE(status_code_failure.IsSuccess());