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 <commits@restyled.io>
diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h
index 6c2af0e..4771a45 100644
--- a/src/app/CommandHandler.h
+++ b/src/app/CommandHandler.h
@@ -25,6 +25,7 @@
#include <lib/support/CodeUtils.h>
#include <lib/support/IntrusiveList.h>
#include <lib/support/logging/CHIPLogging.h>
+#include <protocols/interaction_model/StatusCode.h>
namespace chip {
namespace app {
@@ -116,21 +117,51 @@
/**
* 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 926e5b3..d57fab2 100644
--- a/src/app/CommandHandlerImpl.cpp
+++ b/src/app/CommandHandlerImpl.cpp
@@ -19,6 +19,7 @@
#include <access/AccessControl.h>
#include <app-common/zap-generated/cluster-objects.h>
+#include <app/MessageDef/StatusIB.h>
#include <app/RequiredPrivilege.h>
#include <app/StatusResponse.h>
#include <app/util/MatterCallbacks.h>
@@ -30,6 +31,7 @@
#include <lib/support/TypeTraits.h>
#include <messaging/ExchangeContext.h>
#include <platform/LockTracker.h>
+#include <protocols/interaction_model/StatusCode.h>
#include <protocols/secure_channel/Constants.h>
namespace chip {
@@ -592,11 +594,11 @@
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 @@
}
}
-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<unsigned>(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 db7c851..55e356b 100644
--- a/src/app/CommandHandlerImpl.h
+++ b/src/app/CommandHandlerImpl.h
@@ -29,6 +29,7 @@
#include <messaging/Flags.h>
#include <protocols/Protocols.h>
#include <protocols/interaction_model/Constants.h>
+#include <protocols/interaction_model/StatusCode.h>
#include <system/SystemPacketBuffer.h>
#include <system/TLVPacketBufferBackingStore.h>
@@ -114,15 +115,16 @@
/**************** 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 a0209ac..9d8112f 100644
--- a/src/app/CommandResponseHelper.h
+++ b/src/app/CommandResponseHelper.h
@@ -21,6 +21,7 @@
#include <app/CommandHandler.h>
#include <app/ConcreteCommandPath.h>
#include <protocols/interaction_model/Constants.h>
+#include <protocols/interaction_model/StatusCode.h>
namespace chip {
namespace app {
@@ -38,35 +39,27 @@
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 f5ade6c..0813704 100644
--- a/src/app/MessageDef/StatusIB.h
+++ b/src/app/MessageDef/StatusIB.h
@@ -34,6 +34,7 @@
#include <lib/support/CodeUtils.h>
#include <lib/support/logging/CHIPLogging.h>
#include <protocols/interaction_model/Constants.h>
+#include <protocols/interaction_model/StatusCode.h>
#include <protocols/secure_channel/Constants.h>
namespace chip {
@@ -41,10 +42,24 @@
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> 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 90f0f3a..b67bf4e 100644
--- a/src/app/WriteClient.h
+++ b/src/app/WriteClient.h
@@ -228,12 +228,6 @@
*/
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 068f1f6..aadb59a 100644
--- a/src/app/WriteHandler.cpp
+++ b/src/app/WriteHandler.cpp
@@ -21,6 +21,7 @@
#include <app/AttributeAccessInterfaceRegistry.h>
#include <app/InteractionModelEngine.h>
#include <app/MessageDef/EventPathIB.h>
+#include <app/MessageDef/StatusIB.h>
#include <app/StatusResponse.h>
#include <app/WriteHandler.h>
#include <app/reporting/Engine.h>
@@ -28,6 +29,7 @@
#include <app/util/ember-compatibility-functions.h>
#include <credentials/GroupDataProvider.h>
#include <lib/support/TypeTraits.h>
+#include <protocols/interaction_model/StatusCode.h>
namespace chip {
namespace app {
@@ -315,7 +317,7 @@
// 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 @@
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 @@
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 907aa04..1884654 100644
--- a/src/app/WriteHandler.h
+++ b/src/app/WriteHandler.h
@@ -31,6 +31,7 @@
#include <messaging/Flags.h>
#include <protocols/Protocols.h>
#include <protocols/interaction_model/Constants.h>
+#include <protocols/interaction_model/StatusCode.h>
#include <system/SystemPacketBuffer.h>
#include <system/TLVPacketBufferBackingStore.h>
@@ -100,11 +101,14 @@
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 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 AddClusterSpecificSuccess(const ConcreteDataAttributePath & aAttributePathParams, uint8_t aClusterStatus);
-
- 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 @@
// 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 54ae9ba..9890c3b 100644
--- a/src/app/data-model-interface/InvokeResponder.h
+++ b/src/app/data-model-interface/InvokeResponder.h
@@ -138,7 +138,7 @@
{
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 8da643d..2280616 100644
--- a/src/app/tests/TestStatusIB.cpp
+++ b/src/app/tests/TestStatusIB.cpp
@@ -161,4 +161,24 @@
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 adc6bd0..b6d39ae 100644
--- a/src/controller/tests/data_model/TestCommands.cpp
+++ b/src/controller/tests/data_model/TestCommands.cpp
@@ -37,6 +37,7 @@
#include <lib/support/logging/CHIPLogging.h>
#include <messaging/tests/MessagingContext.h>
#include <protocols/interaction_model/Constants.h>
+#include <protocols/interaction_model/StatusCode.h>
using TestContext = chip::Test::AppContext;
@@ -144,11 +145,13 @@
}
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 28f5797..e0f2c19 100644
--- a/src/controller/tests/data_model/TestWrite.cpp
+++ b/src/controller/tests/data_model/TestWrite.cpp
@@ -22,6 +22,7 @@
#include <app-common/zap-generated/cluster-objects.h>
#include <app/AttributeValueDecoder.h>
#include <app/InteractionModelEngine.h>
+#include <app/WriteClient.h>
#include <app/tests/AppTestContext.h>
#include <controller/WriteInteraction.h>
#include <lib/core/ErrorStr.h>
@@ -43,15 +44,20 @@
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 @@
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 @@
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 @@
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 @@
}
}
+ void ResetCallback() { mSingleWriteCallback.reset(); }
+
+ void PrepareWriteCallback(ConcreteAttributePath path) { mSingleWriteCallback = std::make_unique<SingleWriteCallback>(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 @@
void TearDown() { mpContext->TearDown(); }
static TestContext * mpContext;
+
+ std::unique_ptr<SingleWriteCallback> mSingleWriteCallback;
};
+
TestContext * TestWrite::mpContext = nullptr;
TEST_F(TestWrite, TestDataResponse)
@@ -236,7 +314,7 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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<uint16_t>::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<uint16_t>::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 b004b4e..9326c86 100644
--- a/src/protocols/interaction_model/StatusCode.h
+++ b/src/protocols/interaction_model/StatusCode.h
@@ -18,6 +18,8 @@
#pragma once
#include <limits>
+#include <type_traits>
+
#include <stdint.h>
#include <lib/core/CHIPConfig.h>
@@ -58,10 +60,6 @@
* 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 @@
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 @@
* (e.g. chip::app::Clusters::AdministratorCommissioning::CommissioningWindowStatusEnum::kWindowNotOpen)
* @return a ClusterStatusCode instance properly configured.
*/
- template <typename T>
+ template <typename T, typename std::enable_if_t<std::is_enum<T>::value, bool> = true>
static ClusterStatusCode ClusterSpecificFailure(T cluster_specific_code)
{
static_assert(std::numeric_limits<std::underlying_type_t<T>>::max() <= std::numeric_limits<ClusterStatus>::max(),
@@ -107,6 +105,11 @@
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 @@
* (e.g. chip::app::Clusters::AdministratorCommissioning::CommissioningWindowStatusEnum::kBasicWindowOpen)
* @return a ClusterStatusCode instance properly configured.
*/
- template <typename T>
+ template <typename T, typename std::enable_if_t<std::is_enum<T>::value, bool> = true>
static ClusterStatusCode ClusterSpecificSuccess(T cluster_specific_code)
{
static_assert(std::numeric_limits<std::underlying_type_t<T>>::max() <= std::numeric_limits<ClusterStatus>::max(),
@@ -124,6 +127,11 @@
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 @@
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 2be78c4..3183d68 100644
--- a/src/protocols/interaction_model/tests/TestStatusCode.cpp
+++ b/src/protocols/interaction_model/tests/TestStatusCode.cpp
@@ -40,26 +40,22 @@
// 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 @@
// 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<uint8_t>(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<uint8_t>(RobotoClusterStatus::kSandwichError));
EXPECT_FALSE(status_code_failure.IsSuccess());
