From af81a818df27ec506ab225c6551abe03c533b189 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 31 Jul 2024 13:55:06 -0400 Subject: [PATCH 01/31] In progress --- .../ActionReturnStatus.cpp | 86 +++++++++++++++++++ .../data-model-provider/ActionReturnStatus.h | 76 ++++++++++++++++ src/app/data-model-provider/BUILD.gn | 2 + src/app/data-model-provider/Provider.h | 1 + 4 files changed, 165 insertions(+) create mode 100644 src/app/data-model-provider/ActionReturnStatus.cpp create mode 100644 src/app/data-model-provider/ActionReturnStatus.h diff --git a/src/app/data-model-provider/ActionReturnStatus.cpp b/src/app/data-model-provider/ActionReturnStatus.cpp new file mode 100644 index 00000000000000..7978a3acba70e0 --- /dev/null +++ b/src/app/data-model-provider/ActionReturnStatus.cpp @@ -0,0 +1,86 @@ +/* + * Copyright (c) 2024 Project CHIP Authors + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#include + +#include + +namespace chip { +namespace app { +namespace DataModel { + +Protocols::InteractionModel::ClusterStatusCode ActionReturnStatus::GetStatusCode() const +{ + // FIXME +} + +bool ActionReturnStatus::IsError() const +{ + + // FIXME +} + +bool ActionReturnStatus::IsOutOfSpaceError() const +{ + + // FIXME +} + +void ActionReturnStatus::LogError(const char * prefix) const +{ + + if (mError.has_value()) + { + ChipLogError(InteractionModel, "%s: %" CHIP_ERROR_FORMAT, prefix, mError->Format()); + } + + if (mStatusCode.has_value()) + { + StringBuilder<48> txt; + +#if CHIP_CONFIG_IM_STATUS_CODE_VERBOSE_FORMAT + txt.AddFormat("%s(%d)", Protocols::InteractionModel::StatusName(mStatusCode->GetStatus()), + static_cast(mStatusCode->GetStatus())); +#else + if (mStatusCode->IsSuccess()) + { + txt.Add("Success"); + } + else + { + txt.AddFormat("Status<%d>", static_cast(mStatusCode->GetStatus())); + } +#endif + + chip::Optional clusterCode = mStatusCode->GetClusterSpecificCode(); + if (mStatusCode.has_value()) + { + txt.AddFormat(", Code %d", static_cast(clusterCode.Value())); + } + + ChipLogError(InteractionModel, "%s: %s", prefix, txt.c_str()); + } + + if (!mError.has_value() && !mStatusCode.has_value()) + { + // This should be impossible - we enforce this in constructors ... + ChipLogError(InteractionModel, "%s: Unknown/Invalid action return status", prefix); + } +} + +} // namespace DataModel +} // namespace app +} // namespace chip diff --git a/src/app/data-model-provider/ActionReturnStatus.h b/src/app/data-model-provider/ActionReturnStatus.h new file mode 100644 index 00000000000000..2eb83a584d12cd --- /dev/null +++ b/src/app/data-model-provider/ActionReturnStatus.h @@ -0,0 +1,76 @@ +/* + * Copyright (c) 2024 Project CHIP Authors + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#pragma once + +#include +#include + +#include + +namespace chip { +namespace app { +namespace DataModel { + +/// An ActionReturnStatus encodes the result of a read/write/invoke. +/// +/// Generally such actions result in a StatusIB in the interaction model, +/// which is a code (InteractionModel::Status) and may have an associated +/// cluster-specific code. +/// +/// However some actions specifically may return additional errors for +/// chunking (i.e. out of space, retry or send existing partial data) hence +/// the exitance of this class: +/// +/// - encapsulates a ClusterStatusCode for an actual action result +/// - encapsulates a underlying CHIP_ERROR for reporting purposes +/// and to check for out-of-space specific errors +/// +/// The class is directly constructible from statuses (non-exlicit) to make +/// returning of values easy. +class ActionReturnStatus +{ +public: + ActionReturnStatus(CHIP_ERROR error): mError(error) {} + ActionReturnStatus(Protocols::InteractionModel::Status status): mStatusCode(status) {} + ActionReturnStatus(Protocols::InteractionModel::ClusterStatusCode status): mStatusCode(status) {} + + /// Constructs a status code. Either returns the underlying code directly + /// or converts the underlying CHIP_ERROR into a cluster status code. + Protocols::InteractionModel::ClusterStatusCode GetStatusCode() const; + + /// Considers if the underlying error is an error or not (CHIP_NO_ERROR is the only non-erro) + /// or if the underlying statuscode is not an error (success and cluster specific successes + /// are not an error). + bool IsError() const; + + /// Checks if the underlying error is an out of space condition (i.e. something that + /// chunking can handle by sending partial list data). + bool IsOutOfSpaceError() const; + + /// Logs the underlying code as a ChipLogError. If the underlying data contains + /// a CHIP_ERROR, the error is reported. Otherwise the cluster status code data + /// is logged. + void LogError(const char *prefix) const; + +private: + std::optional mError; + std::optional mStatusCode; +}; + +} // namespace DataModel +} // namespace app +} // namespace chip diff --git a/src/app/data-model-provider/BUILD.gn b/src/app/data-model-provider/BUILD.gn index 5093cce2f4124d..24930601cb669a 100644 --- a/src/app/data-model-provider/BUILD.gn +++ b/src/app/data-model-provider/BUILD.gn @@ -16,6 +16,8 @@ import("//build_overrides/chip.gni") source_set("data-model-provider") { sources = [ "ActionContext.h", + "ActionReturnStatus.cpp", + "ActionReturnStatus.h", "Context.h", "EventsGenerator.h", "MetadataTypes.cpp", diff --git a/src/app/data-model-provider/Provider.h b/src/app/data-model-provider/Provider.h index f5d550c806fd89..b7069f36e21253 100644 --- a/src/app/data-model-provider/Provider.h +++ b/src/app/data-model-provider/Provider.h @@ -26,6 +26,7 @@ #include #include #include +#include namespace chip { namespace app { From bfe92359635799c774c590b3131acf45c478099e Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 1 Aug 2024 09:24:05 -0400 Subject: [PATCH 02/31] ActionReturnStatus implementation --- .../ActionReturnStatus.cpp | 77 ++++++++++++++----- .../data-model-provider/ActionReturnStatus.h | 11 ++- 2 files changed, 64 insertions(+), 24 deletions(-) diff --git a/src/app/data-model-provider/ActionReturnStatus.cpp b/src/app/data-model-provider/ActionReturnStatus.cpp index 7978a3acba70e0..95da38e989dac7 100644 --- a/src/app/data-model-provider/ActionReturnStatus.cpp +++ b/src/app/data-model-provider/ActionReturnStatus.cpp @@ -14,6 +14,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +#include "lib/support/CodeUtils.h" #include #include @@ -22,51 +23,94 @@ namespace chip { namespace app { namespace DataModel { -Protocols::InteractionModel::ClusterStatusCode ActionReturnStatus::GetStatusCode() const +using Protocols::InteractionModel::ClusterStatusCode; +using Protocols::InteractionModel::Status; + +ClusterStatusCode ActionReturnStatus::GetStatusCode() const { - // FIXME + if (const ClusterStatusCode * status = std::get_if(&mReturnStatus)) + { + return *status; + } + + if (const CHIP_ERROR * err = std::get_if(&mReturnStatus)) + { + if (err->IsPart(ChipError::SdkPart::kIMClusterStatus)) + { + return ClusterStatusCode::ClusterSpecificFailure(err->GetSdkCode()); + } + + if (*err == CHIP_NO_ERROR) + { + return ClusterStatusCode(Status::Success); + } + + if (err->IsPart(ChipError::SdkPart::kIMGlobalStatus)) + { + return ClusterStatusCode(static_cast(err->GetSdkCode())); + } + + return ClusterStatusCode(Status::Failure); + } + + // all std::variant cases exhausted + chipDie(); } bool ActionReturnStatus::IsError() const { + if (const CHIP_ERROR * err = std::get_if(&mReturnStatus)) + { + return (*err != CHIP_NO_ERROR); + } + + if (const ClusterStatusCode * status = std::get_if(&mReturnStatus)) + { + return !status->IsSuccess(); + } - // FIXME + // all std::variant cases exhausted + chipDie(); } bool ActionReturnStatus::IsOutOfSpaceError() const { + if (const CHIP_ERROR * err = std::get_if(&mReturnStatus)) + { + return (*err == CHIP_ERROR_NO_MEMORY) || (*err == CHIP_ERROR_BUFFER_TOO_SMALL); + } - // FIXME + return false; } void ActionReturnStatus::LogError(const char * prefix) const { - if (mError.has_value()) + if (const CHIP_ERROR * err = std::get_if(&mReturnStatus)) { - ChipLogError(InteractionModel, "%s: %" CHIP_ERROR_FORMAT, prefix, mError->Format()); + ChipLogError(InteractionModel, "%s: %" CHIP_ERROR_FORMAT, prefix, err->Format()); } - if (mStatusCode.has_value()) + if (const ClusterStatusCode * status = std::get_if(&mReturnStatus)) { StringBuilder<48> txt; #if CHIP_CONFIG_IM_STATUS_CODE_VERBOSE_FORMAT - txt.AddFormat("%s(%d)", Protocols::InteractionModel::StatusName(mStatusCode->GetStatus()), - static_cast(mStatusCode->GetStatus())); + txt.AddFormat("%s(%d)", Protocols::InteractionModel::StatusName(status->GetStatus()), + static_cast(status->GetStatus())); #else - if (mStatusCode->IsSuccess()) + if (status->IsSuccess()) { txt.Add("Success"); } else { - txt.AddFormat("Status<%d>", static_cast(mStatusCode->GetStatus())); + txt.AddFormat("Status<%d>", static_cast(status->GetStatus())); } #endif - chip::Optional clusterCode = mStatusCode->GetClusterSpecificCode(); - if (mStatusCode.has_value()) + chip::Optional clusterCode = status->GetClusterSpecificCode(); + if (clusterCode.HasValue()) { txt.AddFormat(", Code %d", static_cast(clusterCode.Value())); } @@ -74,11 +118,8 @@ void ActionReturnStatus::LogError(const char * prefix) const ChipLogError(InteractionModel, "%s: %s", prefix, txt.c_str()); } - if (!mError.has_value() && !mStatusCode.has_value()) - { - // This should be impossible - we enforce this in constructors ... - ChipLogError(InteractionModel, "%s: Unknown/Invalid action return status", prefix); - } + // all std::variant cases exhausted + chipDie(); } } // namespace DataModel diff --git a/src/app/data-model-provider/ActionReturnStatus.h b/src/app/data-model-provider/ActionReturnStatus.h index 2eb83a584d12cd..046e3e7a790bc0 100644 --- a/src/app/data-model-provider/ActionReturnStatus.h +++ b/src/app/data-model-provider/ActionReturnStatus.h @@ -19,7 +19,7 @@ #include #include -#include +#include namespace chip { namespace app { @@ -44,9 +44,9 @@ namespace DataModel { class ActionReturnStatus { public: - ActionReturnStatus(CHIP_ERROR error): mError(error) {} - ActionReturnStatus(Protocols::InteractionModel::Status status): mStatusCode(status) {} - ActionReturnStatus(Protocols::InteractionModel::ClusterStatusCode status): mStatusCode(status) {} + ActionReturnStatus(CHIP_ERROR error): mReturnStatus(error) {} + ActionReturnStatus(Protocols::InteractionModel::Status status): mReturnStatus(Protocols::InteractionModel::ClusterStatusCode(status)) {} + ActionReturnStatus(Protocols::InteractionModel::ClusterStatusCode status): mReturnStatus(status) {} /// Constructs a status code. Either returns the underlying code directly /// or converts the underlying CHIP_ERROR into a cluster status code. @@ -67,8 +67,7 @@ class ActionReturnStatus void LogError(const char *prefix) const; private: - std::optional mError; - std::optional mStatusCode; + std::variant mReturnStatus; }; } // namespace DataModel From 224c8f9cfa785ea49155b426c9e60612f5e5a7df Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 1 Aug 2024 09:35:08 -0400 Subject: [PATCH 03/31] Start making use of ActionReturnStatus --- .../CodegenDataModelProvider.cpp | 4 +-- .../CodegenDataModelProvider.h | 11 ++++--- .../CodegenDataModelProvider_Read.cpp | 15 +++++---- .../CodegenDataModelProvider_Write.cpp | 16 +++++----- .../EmberMetadata.cpp | 18 ++++++----- .../EmberMetadata.h | 15 ++++----- .../ActionReturnStatus.cpp | 6 ++-- .../data-model-provider/ActionReturnStatus.h | 17 +++++++--- src/app/data-model-provider/Provider.h | 31 +++++++------------ 9 files changed, 72 insertions(+), 61 deletions(-) diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider.cpp b/src/app/codegen-data-model-provider/CodegenDataModelProvider.cpp index e3b9823efeef9a..cea75c8d0c1ccc 100644 --- a/src/app/codegen-data-model-provider/CodegenDataModelProvider.cpp +++ b/src/app/codegen-data-model-provider/CodegenDataModelProvider.cpp @@ -229,8 +229,8 @@ bool CodegenDataModelProvider::EmberCommandListIterator::Exists(const CommandId return (*mCurrentHint == toCheck); } -CHIP_ERROR CodegenDataModelProvider::Invoke(const DataModel::InvokeRequest & request, TLV::TLVReader & input_arguments, - CommandHandler * handler) +DataModel::ActionReturnStatus CodegenDataModelProvider::Invoke(const DataModel::InvokeRequest & request, + TLV::TLVReader & input_arguments, CommandHandler * handler) { // TODO: CommandHandlerInterface support is currently // residing in InteractionModelEngine itself. We may want to separate this out diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider.h b/src/app/codegen-data-model-provider/CodegenDataModelProvider.h index 21dc0cc87e1a31..b486b953976328 100644 --- a/src/app/codegen-data-model-provider/CodegenDataModelProvider.h +++ b/src/app/codegen-data-model-provider/CodegenDataModelProvider.h @@ -16,6 +16,7 @@ */ #pragma once +#include "app/data-model-provider/ActionReturnStatus.h" #include #include @@ -68,10 +69,12 @@ class CodegenDataModelProvider : public chip::app::DataModel::Provider /// Generic model implementations CHIP_ERROR Shutdown() override { return CHIP_NO_ERROR; } - CHIP_ERROR ReadAttribute(const DataModel::ReadAttributeRequest & request, AttributeValueEncoder & encoder) override; - CHIP_ERROR WriteAttribute(const DataModel::WriteAttributeRequest & request, AttributeValueDecoder & decoder) override; - CHIP_ERROR Invoke(const DataModel::InvokeRequest & request, chip::TLV::TLVReader & input_arguments, - CommandHandler * handler) override; + DataModel::ActionReturnStatus ReadAttribute(const DataModel::ReadAttributeRequest & request, + AttributeValueEncoder & encoder) override; + DataModel::ActionReturnStatus WriteAttribute(const DataModel::WriteAttributeRequest & request, + AttributeValueDecoder & decoder) override; + DataModel::ActionReturnStatus Invoke(const DataModel::InvokeRequest & request, chip::TLV::TLVReader & input_arguments, + CommandHandler * handler) override; /// attribute tree iteration EndpointId FirstEndpoint() override; diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider_Read.cpp b/src/app/codegen-data-model-provider/CodegenDataModelProvider_Read.cpp index d27d18964dccf7..6ff4b730812d16 100644 --- a/src/app/codegen-data-model-provider/CodegenDataModelProvider_Read.cpp +++ b/src/app/codegen-data-model-provider/CodegenDataModelProvider_Read.cpp @@ -47,7 +47,9 @@ namespace chip { namespace app { namespace { + using namespace chip::app::Compatibility::Internal; +using Protocols::InteractionModel::Status; /// Attempts to read via an attribute access interface (AAI) /// @@ -258,7 +260,8 @@ CHIP_ERROR EncodeEmberValue(ByteSpan data, const EmberAfAttributeMetadata * meta /// - validate ACL (only for non-internal requests) /// - Try to read attribute via the AttributeAccessInterface /// - Try to read the value from ember RAM storage -CHIP_ERROR CodegenDataModelProvider::ReadAttribute(const DataModel::ReadAttributeRequest & request, AttributeValueEncoder & encoder) +DataModel::ActionReturnStatus CodegenDataModelProvider::ReadAttribute(const DataModel::ReadAttributeRequest & request, + AttributeValueEncoder & encoder) { ChipLogDetail(DataManagement, "Reading attribute: Cluster=" ChipLogFormatMEI " Endpoint=0x%x AttributeId=" ChipLogFormatMEI " (expanded=%d)", @@ -290,12 +293,12 @@ CHIP_ERROR CodegenDataModelProvider::ReadAttribute(const DataModel::ReadAttribut auto metadata = Ember::FindAttributeMetadata(request.path); // Explicit failure in finding a suitable metadata - if (const CHIP_ERROR * err = std::get_if(&metadata)) + if (const Status * status = std::get_if(&metadata)) { - VerifyOrDie((*err == CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint)) || // - (*err == CHIP_IM_GLOBAL_STATUS(UnsupportedCluster)) || // - (*err == CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute))); - return *err; + VerifyOrDie((*status == Status::UnsupportedEndpoint) || // + (*status == Status::UnsupportedCluster) || // + (*status == Status::UnsupportedAttribute)); + return *status; } // Read via AAI diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp b/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp index 962355e8e47b08..61639419de9e8b 100644 --- a/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp +++ b/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp @@ -43,6 +43,7 @@ namespace app { namespace { using namespace chip::app::Compatibility::Internal; +using Protocols::InteractionModel::Status; /// Attempts to write via an attribute access interface (AAI) /// @@ -266,8 +267,8 @@ CHIP_ERROR DecodeValueIntoEmberBuffer(AttributeValueDecoder & decoder, const Emb } // namespace -CHIP_ERROR CodegenDataModelProvider::WriteAttribute(const DataModel::WriteAttributeRequest & request, - AttributeValueDecoder & decoder) +DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const DataModel::WriteAttributeRequest & request, + AttributeValueDecoder & decoder) { ChipLogDetail(DataManagement, "Writing attribute: Cluster=" ChipLogFormatMEI " Endpoint=0x%x AttributeId=" ChipLogFormatMEI, ChipLogValueMEI(request.path.mClusterId), request.path.mEndpointId, ChipLogValueMEI(request.path.mAttributeId)); @@ -292,12 +293,13 @@ CHIP_ERROR CodegenDataModelProvider::WriteAttribute(const DataModel::WriteAttrib auto metadata = Ember::FindAttributeMetadata(request.path); - if (const CHIP_ERROR * err = std::get_if(&metadata)) + // Explicit failure in finding a suitable metadata + if (const Status * status = std::get_if(&metadata)) { - VerifyOrDie((*err == CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint)) || // - (*err == CHIP_IM_GLOBAL_STATUS(UnsupportedCluster)) || // - (*err == CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute))); - return *err; + VerifyOrDie((*status == Status::UnsupportedEndpoint) || // + (*status == Status::UnsupportedCluster) || // + (*status == Status::UnsupportedAttribute)); + return *status; } const EmberAfAttributeMetadata ** attributeMetadata = std::get_if(&metadata); diff --git a/src/app/codegen-data-model-provider/EmberMetadata.cpp b/src/app/codegen-data-model-provider/EmberMetadata.cpp index b8d998d29f0b7e..8853c562dd40ce 100644 --- a/src/app/codegen-data-model-provider/EmberMetadata.cpp +++ b/src/app/codegen-data-model-provider/EmberMetadata.cpp @@ -24,9 +24,11 @@ namespace chip { namespace app { namespace Ember { -std::variant FindAttributeMetadata(const ConcreteAttributePath & aPath) { @@ -41,8 +43,8 @@ FindAttributeMetadata(const ConcreteAttributePath & aPath) const EmberAfCluster * cluster = emberAfFindServerCluster(aPath.mEndpointId, aPath.mClusterId); if (cluster == nullptr) { - return (emberAfFindEndpointType(aPath.mEndpointId) == nullptr) ? CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint) - : CHIP_IM_GLOBAL_STATUS(UnsupportedCluster); + return (emberAfFindEndpointType(aPath.mEndpointId) == nullptr) ? Status::UnsupportedEndpoint + : Status::UnsupportedCluster; } return cluster; @@ -57,18 +59,18 @@ FindAttributeMetadata(const ConcreteAttributePath & aPath) const EmberAfEndpointType * type = emberAfFindEndpointType(aPath.mEndpointId); if (type == nullptr) { - return CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint); + return Status::UnsupportedEndpoint; } const EmberAfCluster * cluster = emberAfFindClusterInType(type, aPath.mClusterId, CLUSTER_MASK_SERVER); if (cluster == nullptr) { - return CHIP_IM_GLOBAL_STATUS(UnsupportedCluster); + return Status::UnsupportedCluster; } // Since we know the attribute is unsupported and the endpoint/cluster are // OK, this is the only option left. - return CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute); + return Status::UnsupportedAttribute; } return metadata; diff --git a/src/app/codegen-data-model-provider/EmberMetadata.h b/src/app/codegen-data-model-provider/EmberMetadata.h index f8f41312f1f7c9..64c0bfe736a25f 100644 --- a/src/app/codegen-data-model-provider/EmberMetadata.h +++ b/src/app/codegen-data-model-provider/EmberMetadata.h @@ -18,6 +18,7 @@ #include #include +#include #include @@ -31,13 +32,13 @@ namespace Ember { /// Possible return values: /// - EmberAfCluster (NEVER null) - Only for GlobalAttributesNotInMetaData /// - EmberAfAttributeMetadata (NEVER null) - if the attribute is known to ember datastore -/// - CHIP_ERROR, only specifically for unknown attributes, may only be one of: -/// - CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint); -/// - CHIP_IM_GLOBAL_STATUS(UnsupportedCluster); -/// - CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute); -std::variant FindAttributeMetadata(const ConcreteAttributePath & aPath); diff --git a/src/app/data-model-provider/ActionReturnStatus.cpp b/src/app/data-model-provider/ActionReturnStatus.cpp index 95da38e989dac7..b7176931d81840 100644 --- a/src/app/data-model-provider/ActionReturnStatus.cpp +++ b/src/app/data-model-provider/ActionReturnStatus.cpp @@ -57,16 +57,16 @@ ClusterStatusCode ActionReturnStatus::GetStatusCode() const chipDie(); } -bool ActionReturnStatus::IsError() const +bool ActionReturnStatus::IsSuccess() const { if (const CHIP_ERROR * err = std::get_if(&mReturnStatus)) { - return (*err != CHIP_NO_ERROR); + return (*err == CHIP_NO_ERROR); } if (const ClusterStatusCode * status = std::get_if(&mReturnStatus)) { - return !status->IsSuccess(); + return status->IsSuccess(); } // all std::variant cases exhausted diff --git a/src/app/data-model-provider/ActionReturnStatus.h b/src/app/data-model-provider/ActionReturnStatus.h index 046e3e7a790bc0..e8688bdc55af3d 100644 --- a/src/app/data-model-provider/ActionReturnStatus.h +++ b/src/app/data-model-provider/ActionReturnStatus.h @@ -44,27 +44,34 @@ namespace DataModel { class ActionReturnStatus { public: - ActionReturnStatus(CHIP_ERROR error): mReturnStatus(error) {} - ActionReturnStatus(Protocols::InteractionModel::Status status): mReturnStatus(Protocols::InteractionModel::ClusterStatusCode(status)) {} - ActionReturnStatus(Protocols::InteractionModel::ClusterStatusCode status): mReturnStatus(status) {} + ActionReturnStatus(CHIP_ERROR error) : mReturnStatus(error) {} + ActionReturnStatus(Protocols::InteractionModel::Status status) : + mReturnStatus(Protocols::InteractionModel::ClusterStatusCode(status)) + {} + ActionReturnStatus(Protocols::InteractionModel::ClusterStatusCode status) : mReturnStatus(status) {} /// Constructs a status code. Either returns the underlying code directly /// or converts the underlying CHIP_ERROR into a cluster status code. Protocols::InteractionModel::ClusterStatusCode GetStatusCode() const; + /// If this is a CHIP_NO_ERROR or a Status::Success + bool IsSuccess() const; + /// Considers if the underlying error is an error or not (CHIP_NO_ERROR is the only non-erro) /// or if the underlying statuscode is not an error (success and cluster specific successes /// are not an error). - bool IsError() const; + bool IsError() const { return !IsSuccess(); } /// Checks if the underlying error is an out of space condition (i.e. something that /// chunking can handle by sending partial list data). + /// + /// Generally this is when the return is based on CHIP_ERROR_NO_MEMORY or CHIP_ERROR_BUFFER_TOO_SMALL bool IsOutOfSpaceError() const; /// Logs the underlying code as a ChipLogError. If the underlying data contains /// a CHIP_ERROR, the error is reported. Otherwise the cluster status code data /// is logged. - void LogError(const char *prefix) const; + void LogError(const char * prefix) const; private: std::variant mReturnStatus; diff --git a/src/app/data-model-provider/Provider.h b/src/app/data-model-provider/Provider.h index b7069f36e21253..4cded7612ff561 100644 --- a/src/app/data-model-provider/Provider.h +++ b/src/app/data-model-provider/Provider.h @@ -16,6 +16,7 @@ */ #pragma once +#include "lib/core/CHIPError.h" #include #include @@ -67,18 +68,12 @@ class Provider : public ProviderMetadataTree /// > Else if reading from the attribute in the path requires a privilege that is not /// granted to access the cluster in the path, then the path SHALL be discarded. /// - /// Return codes: - /// CHIP_ERROR_NO_MEMORY or CHIP_ERROR_BUFFER_TOO_SMALL: + /// Return value notes: + /// ActionReturnStatus::IsOutOfSpaceError(i.e. CHIP_ERROR_NO_MEMORY or CHIP_ERROR_BUFFER_TOO_SMALL): /// - Indicates that list encoding had insufficient buffer space to encode elements. /// - encoder::GetState().AllowPartialData() determines if these errors are permanent (no partial /// data allowed) or further encoding can be retried (AllowPartialData true for list encoding) - /// CHIP_IM_GLOBAL_STATUS(code): - /// - error codes that are translatable in IM status codes (otherwise we expect Failure to be reported) - /// - to check for this, CHIP_ERROR provides: - /// - ::IsPart(ChipError::SdkPart::kIMGlobalStatus) -> bool - /// - ::GetSdkCode() -> uint8_t to translate to the actual code - /// other internal falures - virtual CHIP_ERROR ReadAttribute(const ReadAttributeRequest & request, AttributeValueEncoder & encoder) = 0; + virtual ActionReturnStatus ReadAttribute(const ReadAttributeRequest & request, AttributeValueEncoder & encoder) = 0; /// Requests a write of an attribute. /// @@ -91,18 +86,16 @@ class Provider : public ProviderMetadataTree /// - Validation of readability/writability (also controlled by OperationFlags::kInternal) /// - Validation of timed interaction required (also controlled by OperationFlags::kInternal) /// - /// Return codes - /// CHIP_IM_GLOBAL_STATUS(code): - /// - error codes that are translatable to specific IM codes - /// - in particular, the following codes are interesting/expected - /// - `UnsupportedWrite` for attempts to write read-only data - /// - `UnsupportedAccess` for ACL failures - /// - `NeedsTimedInteraction` for writes that are not timed however are required to be so - virtual CHIP_ERROR WriteAttribute(const WriteAttributeRequest & request, AttributeValueDecoder & decoder) = 0; + /// Return notes: + /// - The following codes are interesting/expected to be possible: + /// - `UnsupportedWrite` for attempts to write read-only data + /// - `UnsupportedAccess` for ACL failures + /// - `NeedsTimedInteraction` for writes that are not timed however are required to be so + virtual ActionReturnStatus WriteAttribute(const WriteAttributeRequest & request, AttributeValueDecoder & decoder) = 0; /// `handler` is used to send back the reply. - /// - returning a value other than CHIP_NO_ERROR implies an error reply (error and data are mutually exclusive) - virtual CHIP_ERROR Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments, CommandHandler * handler) = 0; + /// - returning a value other than Success implies an error reply (error and data are mutually exclusive) + virtual ActionReturnStatus Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments, CommandHandler * handler) = 0; private: InteractionModelContext mContext = { nullptr }; From 4e02d167d6f65f928b9f8176cf5a2205bb780fa2 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 1 Aug 2024 09:59:35 -0400 Subject: [PATCH 04/31] Things seem to compile (but not yet passing) --- .../CodegenDataModelProvider_Write.cpp | 18 ++++++------ .../ActionReturnStatus.cpp | 26 +++++++++++++++++ .../data-model-provider/ActionReturnStatus.h | 10 +++++++ src/app/reporting/Engine.cpp | 22 +++++++++----- src/app/reporting/Read-Checked.cpp | 27 +++++++++-------- src/app/reporting/Read-Checked.h | 3 +- src/app/reporting/Read-DataModel.cpp | 29 ++++++++----------- src/app/reporting/Read-DataModel.h | 9 ++++-- src/app/reporting/Read-Ember.cpp | 8 +++-- src/app/reporting/Read-Ember.h | 3 +- src/app/tests/test-interaction-model-api.cpp | 7 +++-- src/app/tests/test-interaction-model-api.h | 10 ++++--- .../tests/data_model/DataModelFixtures.cpp | 7 +++-- .../tests/data_model/DataModelFixtures.h | 10 ++++--- src/lib/core/CHIPError.h | 12 ++++++-- 15 files changed, 131 insertions(+), 70 deletions(-) diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp b/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp index 61639419de9e8b..925d5cce61bc87 100644 --- a/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp +++ b/src/app/codegen-data-model-provider/CodegenDataModelProvider_Write.cpp @@ -276,7 +276,7 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const Dat // ACL check for non-internal requests if (!request.operationFlags.Has(DataModel::OperationFlags::kInternal)) { - ReturnErrorCodeIf(!request.subjectDescriptor.has_value(), CHIP_IM_GLOBAL_STATUS(UnsupportedAccess)); + ReturnErrorCodeIf(!request.subjectDescriptor.has_value(), Status::UnsupportedAccess); Access::RequestPath requestPath{ .cluster = request.path.mClusterId, .endpoint = request.path.mEndpointId }; CHIP_ERROR err = Access::GetAccessControl().Check(*request.subjectDescriptor, requestPath, @@ -287,7 +287,7 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const Dat ReturnErrorCodeIf(err != CHIP_ERROR_ACCESS_DENIED, err); // TODO: when wildcard/group writes are supported, handle them to discard rather than fail with status - return CHIP_IM_GLOBAL_STATUS(UnsupportedAccess); + return Status::UnsupportedAccess; } } @@ -318,15 +318,15 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const Dat // Internal is allowed to bypass timed writes and read-only. if (!request.operationFlags.Has(DataModel::OperationFlags::kInternal)) { - VerifyOrReturnError(!isReadOnly, CHIP_IM_GLOBAL_STATUS(UnsupportedWrite)); + VerifyOrReturnError(!isReadOnly, Status::UnsupportedWrite); VerifyOrReturnError(!(*attributeMetadata)->MustUseTimedWrite() || request.writeFlags.Has(DataModel::WriteFlags::kTimed), - CHIP_IM_GLOBAL_STATUS(NeedsTimedInteraction)); + Status::NeedsTimedInteraction); } // Extra check: internal requests can bypass the read only check, however global attributes // have no underlying storage, so write still cannot be done - VerifyOrReturnError(attributeMetadata != nullptr, CHIP_IM_GLOBAL_STATUS(UnsupportedWrite)); + VerifyOrReturnError(attributeMetadata != nullptr, Status::UnsupportedWrite); if (request.path.mDataVersion.HasValue()) { @@ -335,14 +335,14 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const Dat { ChipLogError(DataManagement, "Unable to get cluster info for Endpoint 0x%x, Cluster " ChipLogFormatMEI, request.path.mEndpointId, ChipLogValueMEI(request.path.mClusterId)); - return CHIP_IM_GLOBAL_STATUS(DataVersionMismatch); + return Status::DataVersionMismatch; } if (request.path.mDataVersion.Value() != clusterInfo->dataVersion) { ChipLogError(DataManagement, "Write Version mismatch for Endpoint 0x%x, Cluster " ChipLogFormatMEI, request.path.mEndpointId, ChipLogValueMEI(request.path.mClusterId)); - return CHIP_IM_GLOBAL_STATUS(DataVersionMismatch); + return Status::DataVersionMismatch; } } @@ -369,7 +369,7 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const Dat if (dataBuffer.size() > (*attributeMetadata)->size) { ChipLogDetail(Zcl, "Data to write exceeds the attribute size claimed."); - return CHIP_IM_GLOBAL_STATUS(InvalidValue); + return Status::InvalidValue; } if (request.operationFlags.Has(DataModel::OperationFlags::kInternal)) @@ -388,7 +388,7 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::WriteAttribute(const Dat if (status != Protocols::InteractionModel::Status::Success) { - return CHIP_ERROR_IM_GLOBAL_STATUS_VALUE(status); + return status; } // TODO: this WILL requre updates diff --git a/src/app/data-model-provider/ActionReturnStatus.cpp b/src/app/data-model-provider/ActionReturnStatus.cpp index b7176931d81840..44f8e376f73930 100644 --- a/src/app/data-model-provider/ActionReturnStatus.cpp +++ b/src/app/data-model-provider/ActionReturnStatus.cpp @@ -14,7 +14,9 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +#include "lib/core/CHIPError.h" #include "lib/support/CodeUtils.h" +#include "protocols/interaction_model/StatusCode.h" #include #include @@ -26,6 +28,30 @@ namespace DataModel { using Protocols::InteractionModel::ClusterStatusCode; using Protocols::InteractionModel::Status; +CHIP_ERROR ActionReturnStatus::GetUnderlyingError() const +{ + + if (const CHIP_ERROR * err = std::get_if(&mReturnStatus)) + { + return *err; + } + + if (const ClusterStatusCode * status = std::get_if(&mReturnStatus)) + { + if (status->IsSuccess()) + { + return CHIP_NO_ERROR; + } + + chip::Optional code = status->GetClusterSpecificCode(); + + return code.HasValue() ? CHIP_ERROR_IM_CLUSTER_STATUS_VALUE(code.Value()) + : CHIP_ERROR_IM_GLOBAL_STATUS_VALUE(status->GetStatus()); + } + + chipDie(); +} + ClusterStatusCode ActionReturnStatus::GetStatusCode() const { if (const ClusterStatusCode * status = std::get_if(&mReturnStatus)) diff --git a/src/app/data-model-provider/ActionReturnStatus.h b/src/app/data-model-provider/ActionReturnStatus.h index e8688bdc55af3d..efb8e8d5f82425 100644 --- a/src/app/data-model-provider/ActionReturnStatus.h +++ b/src/app/data-model-provider/ActionReturnStatus.h @@ -54,6 +54,12 @@ class ActionReturnStatus /// or converts the underlying CHIP_ERROR into a cluster status code. Protocols::InteractionModel::ClusterStatusCode GetStatusCode() const; + /// Gets the underlying CHIP_ERROR if it exists, otherwise it will + /// return a CHIP_ERROR corresponding to the underlying return status. + /// + /// Success statusess will result in CHIP_NO_ERROR (i.e. cluster specitic success codes are lost) + CHIP_ERROR GetUnderlyingError() const; + /// If this is a CHIP_NO_ERROR or a Status::Success bool IsSuccess() const; @@ -73,6 +79,10 @@ class ActionReturnStatus /// is logged. void LogError(const char * prefix) const; + + bool operator==(const ActionReturnStatus & other) const { return mReturnStatus == other.mReturnStatus; } + bool operator!=(const ActionReturnStatus & other) const { return mReturnStatus != other.mReturnStatus; } + private: std::variant mReturnStatus; }; diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp index faf76e031e177e..ac7c8366c9cc79 100644 --- a/src/app/reporting/Engine.cpp +++ b/src/app/reporting/Engine.cpp @@ -23,6 +23,7 @@ * */ +#include "app/data-model-provider/ActionReturnStatus.h" #include #if CHIP_CONFIG_ENABLE_ICD_SERVER #include // nogncheck @@ -182,15 +183,20 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu ConcreteReadAttributePath pathForRetrieval(readPath); // Load the saved state from previous encoding session for chunking of one single attribute (list chunking). AttributeEncodeState encodeState = apReadHandler->GetAttributeEncodeState(); - err = Impl::RetrieveClusterData(mpImEngine->GetDataModelProvider(), apReadHandler->GetSubjectDescriptor(), - apReadHandler->IsFabricFiltered(), attributeReportIBs, pathForRetrieval, &encodeState); - if (err != CHIP_NO_ERROR) + DataModel::ActionReturnStatus status = + Impl::RetrieveClusterData(mpImEngine->GetDataModelProvider(), apReadHandler->GetSubjectDescriptor(), + apReadHandler->IsFabricFiltered(), attributeReportIBs, pathForRetrieval, &encodeState); + if (status.IsError()) { + // Operation error set, since this will affect early return or override on status encoding + // it will also be used for error reporting below. + err = status.GetUnderlyingError(); + // If error is not an "out of writer space" error, rollback and encode status. // Otherwise, if partial data allowed, save the encode state. // Otherwise roll back. If we have already encoded some chunks, we are done; otherwise encode status. - if (encodeState.AllowPartialData() && IsOutOfWriterSpaceError(err)) + if (encodeState.AllowPartialData() && status.IsOutOfSpaceError()) { ChipLogDetail(DataManagement, "List does not fit in packet, chunk between list items for clusterId: " ChipLogFormatMEI @@ -210,7 +216,7 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu attributeReportIBs.Rollback(attributeBackup); apReadHandler->SetAttributeEncodeState(AttributeEncodeState()); - if (!IsOutOfWriterSpaceError(err)) + if (status.IsOutOfSpaceError()) { ChipLogError(DataManagement, "Fail to retrieve data, roll back and encode status on clusterId: " ChipLogFormatMEI @@ -218,7 +224,7 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu ChipLogValueMEI(pathForRetrieval.mClusterId), ChipLogValueMEI(pathForRetrieval.mAttributeId), err.Format()); // Try to encode our error as a status response. - err = attributeReportIBs.EncodeAttributeStatus(pathForRetrieval, StatusIB(err)); + err = attributeReportIBs.EncodeAttributeStatus(pathForRetrieval, StatusIB(status.GetStatusCode())); if (err != CHIP_NO_ERROR) { // OK, just roll back again and give up; if we still ran out of space we @@ -1005,6 +1011,6 @@ void Engine::ScheduleUrgentEventDeliverySync(Optional fabricIndex) // TODO: MatterReportingAttributeChangeCallback should just live in libCHIP, // instead of being in ember-compatibility-functions. It does not depend on any // app-specific generated bits. -void __attribute__((weak)) -MatterReportingAttributeChangeCallback(chip::EndpointId endpoint, chip::ClusterId clusterId, chip::AttributeId attributeId) +void __attribute__((weak)) MatterReportingAttributeChangeCallback(chip::EndpointId endpoint, chip::ClusterId clusterId, + chip::AttributeId attributeId) {} diff --git a/src/app/reporting/Read-Checked.cpp b/src/app/reporting/Read-Checked.cpp index 3b434d5da5f624..2fad1b7f60d681 100644 --- a/src/app/reporting/Read-Checked.cpp +++ b/src/app/reporting/Read-Checked.cpp @@ -14,6 +14,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +#include "app/data-model-provider/ActionReturnStatus.h" #include #include @@ -26,6 +27,8 @@ namespace reporting { namespace CheckedImpl { namespace { +using DataModel::ActionReturnStatus; + /// Checkpoints and saves the state (including error state) for a /// AttributeReportIBs::Builder class ScopedAttributeReportIBsBuilderState @@ -50,16 +53,16 @@ class ScopedAttributeReportIBsBuilderState } // namespace -CHIP_ERROR RetrieveClusterData(DataModel::Provider * dataModel, const Access::SubjectDescriptor & subjectDescriptor, - bool isFabricFiltered, AttributeReportIBs::Builder & reportBuilder, - const ConcreteReadAttributePath & path, AttributeEncodeState * encoderState) +ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataModel, const Access::SubjectDescriptor & subjectDescriptor, + bool isFabricFiltered, AttributeReportIBs::Builder & reportBuilder, + const ConcreteReadAttributePath & path, AttributeEncodeState * encoderState) { ChipLogDetail(DataManagement, " Cluster %" PRIx32 ", Attribute %" PRIx32 " is dirty", path.mClusterId, path.mAttributeId); DataModelCallbacks::GetInstance()->AttributeOperation(DataModelCallbacks::OperationType::Read, DataModelCallbacks::OperationOrder::Pre, path); - CHIP_ERROR errEmber = CHIP_NO_ERROR; + ActionReturnStatus statusEmber(CHIP_NO_ERROR); uint32_t lengthWrittenEmber = 0; // a copy for DM logic only. Ember changes state directly @@ -68,21 +71,21 @@ CHIP_ERROR RetrieveClusterData(DataModel::Provider * dataModel, const Access::Su { ScopedAttributeReportIBsBuilderState builderState(reportBuilder); // temporary only - errEmber = + statusEmber = EmberImpl::RetrieveClusterData(dataModel, subjectDescriptor, isFabricFiltered, reportBuilder, path, encoderState); lengthWrittenEmber = reportBuilder.GetWriter()->GetLengthWritten(); } - CHIP_ERROR errDM = DataModelImpl::RetrieveClusterData(dataModel, subjectDescriptor, isFabricFiltered, reportBuilder, path, - encoderState != nullptr ? &stateDm : nullptr); + ActionReturnStatus statusDm = DataModelImpl::RetrieveClusterData(dataModel, subjectDescriptor, isFabricFiltered, reportBuilder, + path, encoderState != nullptr ? &stateDm : nullptr); - if (errEmber != errDM) + if (statusEmber != statusDm) { // Note log + chipDie instead of VerifyOrDie so that breakpoints (and usage of rr) // is easier to debug. ChipLogError(Test, "Different return codes between ember and DM"); - ChipLogError(Test, " Ember error: %" CHIP_ERROR_FORMAT, errEmber.Format()); - ChipLogError(Test, " DM error: %" CHIP_ERROR_FORMAT, errDM.Format()); + statusEmber.LogError(" Ember status: "); + statusDm.LogError(" DM status: "); // For time-dependent data, we may have size differences here: one data fitting in buffer // while another not, resulting in different errors (success vs out of space). @@ -120,7 +123,7 @@ CHIP_ERROR RetrieveClusterData(DataModel::Provider * dataModel, const Access::Su // For chunked reads, the encoder state MUST be identical (since this is what controls // where chunking resumes). - if ((errEmber == CHIP_ERROR_NO_MEMORY) || (errEmber == CHIP_ERROR_BUFFER_TOO_SMALL)) + if ((statusEmber == CHIP_ERROR_NO_MEMORY) || (statusEmber == CHIP_ERROR_BUFFER_TOO_SMALL)) { // Encoder state MUST match on partial reads (used by chunking) // specifically ReadViaAccessInterface in ember-compatibility-functions only @@ -151,7 +154,7 @@ CHIP_ERROR RetrieveClusterData(DataModel::Provider * dataModel, const Access::Su DataModelCallbacks::GetInstance()->AttributeOperation(DataModelCallbacks::OperationType::Read, DataModelCallbacks::OperationOrder::Post, path); - return errDM; + return statusDm; } } // namespace CheckedImpl diff --git a/src/app/reporting/Read-Checked.h b/src/app/reporting/Read-Checked.h index 2652d4f98c86d7..d6a5355f483c9d 100644 --- a/src/app/reporting/Read-Checked.h +++ b/src/app/reporting/Read-Checked.h @@ -16,6 +16,7 @@ */ #pragma once +#include "app/data-model-provider/ActionReturnStatus.h" #include #include #include @@ -27,7 +28,7 @@ namespace app { namespace reporting { namespace CheckedImpl { -CHIP_ERROR RetrieveClusterData(DataModel::Provider * dataModel, const Access::SubjectDescriptor & subjectDescriptor, +DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataModel, const Access::SubjectDescriptor & subjectDescriptor, bool isFabricFiltered, AttributeReportIBs::Builder & reportBuilder, const ConcreteReadAttributePath & path, AttributeEncodeState * encoderState); diff --git a/src/app/reporting/Read-DataModel.cpp b/src/app/reporting/Read-DataModel.cpp index 9364d5c5fde9ce..f8ccbf67f277d7 100644 --- a/src/app/reporting/Read-DataModel.cpp +++ b/src/app/reporting/Read-DataModel.cpp @@ -14,6 +14,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +#include "app/data-model-provider/ActionReturnStatus.h" #include #include @@ -24,18 +25,11 @@ namespace chip { namespace app { namespace reporting { namespace DataModelImpl { -namespace { -bool IsOutOfSpaceError(CHIP_ERROR err) -{ - return (err == CHIP_ERROR_NO_MEMORY || err == CHIP_ERROR_BUFFER_TOO_SMALL); -} - -} // namespace - -CHIP_ERROR RetrieveClusterData(DataModel::Provider * dataModel, const Access::SubjectDescriptor & subjectDescriptor, - bool isFabricFiltered, AttributeReportIBs::Builder & reportBuilder, - const ConcreteReadAttributePath & path, AttributeEncodeState * encoderState) +DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataModel, + const Access::SubjectDescriptor & subjectDescriptor, bool isFabricFiltered, + AttributeReportIBs::Builder & reportBuilder, + const ConcreteReadAttributePath & path, AttributeEncodeState * encoderState) { // Odd ifdef is to only do this if the `Read-Check` does not do it already. #if !CHIP_CONFIG_USE_EMBER_DATA_MODEL @@ -68,9 +62,10 @@ CHIP_ERROR RetrieveClusterData(DataModel::Provider * dataModel, const Access::Su reportBuilder.Checkpoint(checkpoint); AttributeValueEncoder attributeValueEncoder(reportBuilder, subjectDescriptor, path, version, isFabricFiltered, encoderState); - CHIP_ERROR err = dataModel->ReadAttribute(readRequest, attributeValueEncoder); - if (err == CHIP_NO_ERROR) + DataModel::ActionReturnStatus status = dataModel->ReadAttribute(readRequest, attributeValueEncoder); + + if (status.IsSuccess()) { // Odd ifdef is to only do this if the `Read-Check` does not do it already. #if !CHIP_CONFIG_USE_EMBER_DATA_MODEL @@ -82,7 +77,7 @@ CHIP_ERROR RetrieveClusterData(DataModel::Provider * dataModel, const Access::Su DataModelCallbacks::GetInstance()->AttributeOperation(DataModelCallbacks::OperationType::Read, DataModelCallbacks::OperationOrder::Post, path); #endif // !CHIP_CONFIG_USE_EMBER_DATA_MODEL - return CHIP_NO_ERROR; + return status; } // Encoder state is relevant for errors in case they are retryable. @@ -97,11 +92,11 @@ CHIP_ERROR RetrieveClusterData(DataModel::Provider * dataModel, const Access::Su // Out of space errors may be chunked data, reporting those cases would be very confusing // as they are not fully errors. Report only others (which presumably are not recoverable // and will be sent to the client as well). - if (!IsOutOfSpaceError(err)) + if (!status.IsOutOfSpaceError()) { - ChipLogError(DataManagement, "Failed to read attribute: %" CHIP_ERROR_FORMAT, err.Format()); + status.LogError("Failed to read attribute: "); } - return err; + return status; } } // namespace DataModelImpl diff --git a/src/app/reporting/Read-DataModel.h b/src/app/reporting/Read-DataModel.h index 346ebb2340f6d7..3a629e66e398d0 100644 --- a/src/app/reporting/Read-DataModel.h +++ b/src/app/reporting/Read-DataModel.h @@ -16,9 +16,11 @@ */ #pragma once +#include "app/data-model-provider/ActionReturnStatus.h" #include #include #include +#include #include #include @@ -27,9 +29,10 @@ namespace app { namespace reporting { namespace DataModelImpl { -CHIP_ERROR RetrieveClusterData(DataModel::Provider * dataModel, const Access::SubjectDescriptor & subjectDescriptor, - bool isFabricFiltered, AttributeReportIBs::Builder & reportBuilder, - const ConcreteReadAttributePath & path, AttributeEncodeState * encoderState); +DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataModel, + const Access::SubjectDescriptor & subjectDescriptor, bool isFabricFiltered, + AttributeReportIBs::Builder & reportBuilder, + const ConcreteReadAttributePath & path, AttributeEncodeState * encoderState); } // namespace DataModelImpl } // namespace reporting diff --git a/src/app/reporting/Read-Ember.cpp b/src/app/reporting/Read-Ember.cpp index 3e9ad543a6e2b4..a7bf1023b3d841 100644 --- a/src/app/reporting/Read-Ember.cpp +++ b/src/app/reporting/Read-Ember.cpp @@ -14,6 +14,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +#include "app/data-model-provider/ActionReturnStatus.h" #include #include @@ -25,9 +26,10 @@ namespace app { namespace reporting { namespace EmberImpl { -CHIP_ERROR RetrieveClusterData(DataModel::Provider * dataModel, const Access::SubjectDescriptor & subjectDescriptor, - bool isFabricFiltered, AttributeReportIBs::Builder & reportBuilder, - const ConcreteReadAttributePath & path, AttributeEncodeState * encoderState) +DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataModel, + const Access::SubjectDescriptor & subjectDescriptor, bool isFabricFiltered, + AttributeReportIBs::Builder & reportBuilder, + const ConcreteReadAttributePath & path, AttributeEncodeState * encoderState) { // Odd ifdef is to only do this if the `Read-Check` does not do it already. #if !CHIP_CONFIG_USE_DATA_MODEL_INTERFACE diff --git a/src/app/reporting/Read-Ember.h b/src/app/reporting/Read-Ember.h index 5ff3fff4103dc5..5c192a7a392896 100644 --- a/src/app/reporting/Read-Ember.h +++ b/src/app/reporting/Read-Ember.h @@ -20,6 +20,7 @@ #include #include #include +#include #include namespace chip { @@ -27,7 +28,7 @@ namespace app { namespace reporting { namespace EmberImpl { -CHIP_ERROR RetrieveClusterData(DataModel::Provider * dataModel, const Access::SubjectDescriptor & subjectDescriptor, +DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataModel, const Access::SubjectDescriptor & subjectDescriptor, bool isFabricFiltered, AttributeReportIBs::Builder & reportBuilder, const ConcreteReadAttributePath & path, AttributeEncodeState * encoderState); diff --git a/src/app/tests/test-interaction-model-api.cpp b/src/app/tests/test-interaction-model-api.cpp index f626d061c2b107..4d3b6c3f37c9e2 100644 --- a/src/app/tests/test-interaction-model-api.cpp +++ b/src/app/tests/test-interaction-model-api.cpp @@ -13,6 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +#include "app/data-model-provider/ActionReturnStatus.h" #include #include @@ -153,7 +154,7 @@ TestImCustomDataModel & TestImCustomDataModel::Instance() return model; } -CHIP_ERROR TestImCustomDataModel::ReadAttribute(const ReadAttributeRequest & request, AttributeValueEncoder & encoder) +ActionReturnStatus TestImCustomDataModel::ReadAttribute(const ReadAttributeRequest & request, AttributeValueEncoder & encoder) { AttributeEncodeState mutableState(&encoder.GetState()); // provide a state copy to start. @@ -167,12 +168,12 @@ CHIP_ERROR TestImCustomDataModel::ReadAttribute(const ReadAttributeRequest & req return err; } -CHIP_ERROR TestImCustomDataModel::WriteAttribute(const WriteAttributeRequest & request, AttributeValueDecoder & decoder) +ActionReturnStatus TestImCustomDataModel::WriteAttribute(const WriteAttributeRequest & request, AttributeValueDecoder & decoder) { return CHIP_ERROR_NOT_IMPLEMENTED; } -CHIP_ERROR TestImCustomDataModel::Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments, +ActionReturnStatus TestImCustomDataModel::Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments, CommandHandler * handler) { return CHIP_ERROR_NOT_IMPLEMENTED; diff --git a/src/app/tests/test-interaction-model-api.h b/src/app/tests/test-interaction-model-api.h index ccaf6514890546..6de6c9c34a2e21 100644 --- a/src/app/tests/test-interaction-model-api.h +++ b/src/app/tests/test-interaction-model-api.h @@ -118,10 +118,12 @@ class TestImCustomDataModel : public DataModel::Provider CHIP_ERROR Shutdown() override { return CHIP_NO_ERROR; } - CHIP_ERROR ReadAttribute(const DataModel::ReadAttributeRequest & request, AttributeValueEncoder & encoder) override; - CHIP_ERROR WriteAttribute(const DataModel::WriteAttributeRequest & request, AttributeValueDecoder & decoder) override; - CHIP_ERROR Invoke(const DataModel::InvokeRequest & request, chip::TLV::TLVReader & input_arguments, - CommandHandler * handler) override; + DataModel::ActionReturnStatus ReadAttribute(const DataModel::ReadAttributeRequest & request, + AttributeValueEncoder & encoder) override; + DataModel::ActionReturnStatus WriteAttribute(const DataModel::WriteAttributeRequest & request, + AttributeValueDecoder & decoder) override; + DataModel::ActionReturnStatus Invoke(const DataModel::InvokeRequest & request, chip::TLV::TLVReader & input_arguments, + CommandHandler * handler) override; EndpointId FirstEndpoint() override; EndpointId NextEndpoint(EndpointId before) override; diff --git a/src/controller/tests/data_model/DataModelFixtures.cpp b/src/controller/tests/data_model/DataModelFixtures.cpp index ff22f2063e6e62..f4b1e43b12ed87 100644 --- a/src/controller/tests/data_model/DataModelFixtures.cpp +++ b/src/controller/tests/data_model/DataModelFixtures.cpp @@ -17,6 +17,7 @@ */ #include "DataModelFixtures.h" +#include "app/data-model-provider/ActionReturnStatus.h" #include #include @@ -491,7 +492,7 @@ CustomDataModel & CustomDataModel::Instance() return model; } -CHIP_ERROR CustomDataModel::ReadAttribute(const ReadAttributeRequest & request, AttributeValueEncoder & encoder) +ActionReturnStatus CustomDataModel::ReadAttribute(const ReadAttributeRequest & request, AttributeValueEncoder & encoder) { AttributeEncodeState mutableState(&encoder.GetState()); // provide a state copy to start. @@ -519,12 +520,12 @@ CHIP_ERROR CustomDataModel::ReadAttribute(const ReadAttributeRequest & request, return err; } -CHIP_ERROR CustomDataModel::WriteAttribute(const WriteAttributeRequest & request, AttributeValueDecoder & decoder) +ActionReturnStatus CustomDataModel::WriteAttribute(const WriteAttributeRequest & request, AttributeValueDecoder & decoder) { return CHIP_ERROR_NOT_IMPLEMENTED; } -CHIP_ERROR CustomDataModel::Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments, CommandHandler * handler) +ActionReturnStatus CustomDataModel::Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments, CommandHandler * handler) { return CHIP_ERROR_NOT_IMPLEMENTED; } diff --git a/src/controller/tests/data_model/DataModelFixtures.h b/src/controller/tests/data_model/DataModelFixtures.h index a36e905c23ce41..cfb2edf5db83f3 100644 --- a/src/controller/tests/data_model/DataModelFixtures.h +++ b/src/controller/tests/data_model/DataModelFixtures.h @@ -115,10 +115,12 @@ class CustomDataModel : public DataModel::Provider CHIP_ERROR Shutdown() override { return CHIP_NO_ERROR; } - CHIP_ERROR ReadAttribute(const DataModel::ReadAttributeRequest & request, AttributeValueEncoder & encoder) override; - CHIP_ERROR WriteAttribute(const DataModel::WriteAttributeRequest & request, AttributeValueDecoder & decoder) override; - CHIP_ERROR Invoke(const DataModel::InvokeRequest & request, chip::TLV::TLVReader & input_arguments, - CommandHandler * handler) override; + DataModel::ActionReturnStatus ReadAttribute(const DataModel::ReadAttributeRequest & request, + AttributeValueEncoder & encoder) override; + DataModel::ActionReturnStatus WriteAttribute(const DataModel::WriteAttributeRequest & request, + AttributeValueDecoder & decoder) override; + DataModel::ActionReturnStatus Invoke(const DataModel::InvokeRequest & request, chip::TLV::TLVReader & input_arguments, + CommandHandler * handler) override; EndpointId FirstEndpoint() override; EndpointId NextEndpoint(EndpointId before) override; diff --git a/src/lib/core/CHIPError.h b/src/lib/core/CHIPError.h index 23d83d868ea3df..7e856ed4408d77 100644 --- a/src/lib/core/CHIPError.h +++ b/src/lib/core/CHIPError.h @@ -157,8 +157,7 @@ class ChipError {} #else constexpr ChipError(SdkPart part, uint8_t code, const char * file, unsigned int line) : - mError(MakeInteger(part, code)) CHIP_INITIALIZE_ERROR_SOURCE(file, line, /*loc=*/nullptr) - {} + mError(MakeInteger(part, code)) CHIP_INITIALIZE_ERROR_SOURCE(file, line, /*loc=*/nullptr){} #endif // CHIP_CONFIG_ERROR_SOURCE && __cplusplus >= 202002L /** @@ -453,6 +452,15 @@ using CHIP_ERROR = ::chip::ChipError; // #define CHIP_IM_CLUSTER_STATUS(type) CHIP_SDK_ERROR(::chip::ChipError::SdkPart::kIMClusterStatus, type) +// Defines a runtime-value for a chip-error that contains a cluster Status. +#if CHIP_CONFIG_ERROR_SOURCE +#define CHIP_ERROR_IM_CLUSTER_STATUS_VALUE(status_value) \ + ::chip::ChipError(::chip::ChipError::SdkPart::kIMClusterStatus, status_value, __FILE__, __LINE__) +#else +#define CHIP_ERROR_IM_CLUSTER_STATUS_VALUE(status_value) \ + ::chip::ChipError(::chip::ChipError::SdkPart::kIMClusterStatus, status_value) +#endif // CHIP_CONFIG_ERROR_SOURCE + // clang-format off /** From 047321b369ab8b4df5b602ce657649095b2427a8 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 1 Aug 2024 10:41:26 -0400 Subject: [PATCH 05/31] nice log formatting --- .../tests/BUILD.gn | 5 +- .../tests/TestCodegenModelViaMocks.cpp | 55 ++++++++++--------- .../ActionReturnStatus.cpp | 39 +++++++++---- .../data-model-provider/ActionReturnStatus.h | 13 +++-- src/app/data-model-provider/BUILD.gn | 14 +++++ src/app/reporting/Read-Checked.cpp | 8 ++- src/app/reporting/Read-DataModel.cpp | 5 +- 7 files changed, 92 insertions(+), 47 deletions(-) diff --git a/src/app/codegen-data-model-provider/tests/BUILD.gn b/src/app/codegen-data-model-provider/tests/BUILD.gn index 451a276c0367d2..1de428c16c8903 100644 --- a/src/app/codegen-data-model-provider/tests/BUILD.gn +++ b/src/app/codegen-data-model-provider/tests/BUILD.gn @@ -57,5 +57,8 @@ chip_test_suite("tests") { cflags = [ "-Wconversion" ] - public_deps = [ ":mock_model" ] + public_deps = [ + ":mock_model", + "${chip_root}/src/app/data-model-provider:string-builder-adapters", + ] } diff --git a/src/app/codegen-data-model-provider/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model-provider/tests/TestCodegenModelViaMocks.cpp index 947d0aecd9684d..a8862bd9036963 100644 --- a/src/app/codegen-data-model-provider/tests/TestCodegenModelViaMocks.cpp +++ b/src/app/codegen-data-model-provider/tests/TestCodegenModelViaMocks.cpp @@ -19,9 +19,6 @@ #include -#include "app/ConcreteCommandPath.h" -#include - #include #include #include @@ -35,9 +32,12 @@ #include #include #include +#include #include #include +#include #include +#include #include #include #include @@ -58,6 +58,7 @@ #include #include #include +#include using namespace chip; using namespace chip::Test; @@ -65,6 +66,8 @@ using namespace chip::app; using namespace chip::app::DataModel; using namespace chip::app::Clusters::Globals::Attributes; +using chip::Protocols::InteractionModel::Status; + namespace { constexpr FabricIndex kTestFabrixIndex = kMinValidFabricIndex; @@ -787,7 +790,7 @@ void TestEmberScalarTypeWrite(const typename NumericAttributeTraits::WorkingT AttributeValueDecoder decoder = test.DecoderFor(value); // write should succeed - ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); + ASSERT_TRUE(model.WriteAttribute(test.request, decoder).IsSuccess()); // Validate data after write chip::ByteSpan writtenData = Test::GetEmberBuffer(); @@ -844,7 +847,7 @@ void TestEmberScalarNullWrite() AttributeValueDecoder decoder = test.DecoderFor(NullableType()); // write should succeed - ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); + ASSERT_TRUE(model.WriteAttribute(test.request, decoder).IsSuccess()); // Validate data after write chip::ByteSpan writtenData = Test::GetEmberBuffer(); @@ -1298,7 +1301,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeReadAclDeny) ConcreteAttributePath(kMockEndpoint1, MockClusterId(1), MockAttributeId(10))); std::unique_ptr encoder = testRequest.StartEncoding(&model); - ASSERT_EQ(model.ReadAttribute(testRequest.request, *encoder), CHIP_IM_GLOBAL_STATUS(UnsupportedAccess)); + ASSERT_EQ(model.ReadAttribute(testRequest.request, *encoder), Status::UnsupportedAccess); } TEST(TestCodegenModelViaMocks, ReadForInvalidGlobalAttributePath) @@ -1311,14 +1314,14 @@ TEST(TestCodegenModelViaMocks, ReadForInvalidGlobalAttributePath) TestReadRequest testRequest(kAdminSubjectDescriptor, ConcreteAttributePath(kEndpointIdThatIsMissing, MockClusterId(1), AttributeList::Id)); std::unique_ptr encoder = testRequest.StartEncoding(&model); - ASSERT_EQ(model.ReadAttribute(testRequest.request, *encoder), CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint)); + ASSERT_EQ(model.ReadAttribute(testRequest.request, *encoder), Status::UnsupportedEndpoint); } { TestReadRequest testRequest(kAdminSubjectDescriptor, ConcreteAttributePath(kMockEndpoint1, kInvalidClusterId, AttributeList::Id)); std::unique_ptr encoder = testRequest.StartEncoding(&model); - ASSERT_EQ(model.ReadAttribute(testRequest.request, *encoder), CHIP_IM_GLOBAL_STATUS(UnsupportedCluster)); + ASSERT_EQ(model.ReadAttribute(testRequest.request, *encoder), Status::UnsupportedCluster); } } @@ -1334,7 +1337,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeInvalidRead) ConcreteAttributePath(kMockEndpoint1, MockClusterId(1), MockAttributeId(10))); std::unique_ptr encoder = testRequest.StartEncoding(&model); - ASSERT_EQ(model.ReadAttribute(testRequest.request, *encoder), CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute)); + ASSERT_EQ(model.ReadAttribute(testRequest.request, *encoder), Status::UnsupportedAttribute); } // Invalid cluster @@ -1343,7 +1346,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeInvalidRead) ConcreteAttributePath(kMockEndpoint1, MockClusterId(100), MockAttributeId(1))); std::unique_ptr encoder = testRequest.StartEncoding(&model); - ASSERT_EQ(model.ReadAttribute(testRequest.request, *encoder), CHIP_IM_GLOBAL_STATUS(UnsupportedCluster)); + ASSERT_EQ(model.ReadAttribute(testRequest.request, *encoder), Status::UnsupportedCluster); } // Invalid endpoint @@ -1352,7 +1355,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeInvalidRead) ConcreteAttributePath(kEndpointIdThatIsMissing, MockClusterId(1), MockAttributeId(1))); std::unique_ptr encoder = testRequest.StartEncoding(&model); - ASSERT_EQ(model.ReadAttribute(testRequest.request, *encoder), CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint)); + ASSERT_EQ(model.ReadAttribute(testRequest.request, *encoder), Status::UnsupportedEndpoint); } } @@ -1494,7 +1497,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeReadErrorReading) // Actual read via an encoder std::unique_ptr encoder = testRequest.StartEncoding(&model); - ASSERT_EQ(model.ReadAttribute(testRequest.request, *encoder), CHIP_IM_GLOBAL_STATUS(Failure)); + ASSERT_EQ(model.ReadAttribute(testRequest.request, *encoder), Status::Failure); } { @@ -1507,7 +1510,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeReadErrorReading) // Actual read via an encoder std::unique_ptr encoder = testRequest.StartEncoding(&model); - ASSERT_EQ(model.ReadAttribute(testRequest.request, *encoder), CHIP_IM_GLOBAL_STATUS(Busy)); + ASSERT_EQ(model.ReadAttribute(testRequest.request, *encoder), Status::Busy); } // reset things to success to not affect other tests @@ -1994,7 +1997,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteAclDeny) TestWriteRequest test(kDenySubjectDescriptor, ConcreteDataAttributePath(kMockEndpoint1, MockClusterId(1), MockAttributeId(10))); AttributeValueDecoder decoder = test.DecoderFor(1234); - ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_IM_GLOBAL_STATUS(UnsupportedAccess)); + ASSERT_EQ(model.WriteAttribute(test.request, decoder), Status::UnsupportedAccess); ASSERT_TRUE(model.ChangeListener().DirtyList().empty()); } @@ -2065,7 +2068,7 @@ TEST(TestCodegenModelViaMocks, EmberTestWriteReservedNullPlaceholderToNullable) AttributeValueDecoder decoder = test.DecoderFor(0xFFFFFFFF); // write should fail: we are trying to write null which is out of range - ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_IM_GLOBAL_STATUS(ConstraintError)); + ASSERT_EQ(model.WriteAttribute(test.request, decoder), Status::ConstraintError); } TEST(TestCodegenModelViaMocks, EmberTestWriteOutOfRepresentableRangeOddIntegerNonNullable) @@ -2173,7 +2176,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteLongStringOutOfBounds) AttributeValueDecoder decoder = test.DecoderFor( "this is a very long string that will be longer than the default attribute size for our mocks"_span); - ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_IM_GLOBAL_STATUS(InvalidValue)); + ASSERT_EQ(model.WriteAttribute(test.request, decoder), Status::InvalidValue); } TEST(TestCodegenModelViaMocks, EmberAttributeWriteLongString) @@ -2292,7 +2295,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteTimedWrite) TestWriteRequest test(kAdminSubjectDescriptor, ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), kAttributeIdTimedWrite)); AttributeValueDecoder decoder = test.DecoderFor(1234); - ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_IM_GLOBAL_STATUS(NeedsTimedInteraction)); + ASSERT_EQ(model.WriteAttribute(test.request, decoder), Status::NeedsTimedInteraction); // writing as timed should be fine test.request.writeFlags.Set(WriteFlags::kTimed); @@ -2308,7 +2311,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteReadOnlyAttribute) TestWriteRequest test(kAdminSubjectDescriptor, ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), kAttributeIdReadOnly)); AttributeValueDecoder decoder = test.DecoderFor(1234); - ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_IM_GLOBAL_STATUS(UnsupportedWrite)); + ASSERT_EQ(model.WriteAttribute(test.request, decoder), Status::UnsupportedWrite); // Internal writes bypass the read only requirement test.request.operationFlags.Set(OperationFlags::kInternal); @@ -2335,7 +2338,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteDataVersion) AttributeValueDecoder decoder = test.DecoderFor(1234); - ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_IM_GLOBAL_STATUS(DataVersionMismatch)); + ASSERT_EQ(model.WriteAttribute(test.request, decoder), Status::DataVersionMismatch); // Write passes if we set the right version for the data test.request.path.mDataVersion = MakeOptional(GetVersion()); @@ -2351,18 +2354,18 @@ TEST(TestCodegenModelViaMocks, WriteToInvalidPath) { TestWriteRequest test(kAdminSubjectDescriptor, ConcreteAttributePath(kInvalidEndpointId, MockClusterId(1234), 1234)); AttributeValueDecoder decoder = test.DecoderFor(1234); - ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint)); + ASSERT_EQ(model.WriteAttribute(test.request, decoder), Status::UnsupportedEndpoint); } { TestWriteRequest test(kAdminSubjectDescriptor, ConcreteAttributePath(kMockEndpoint1, MockClusterId(1234), 1234)); AttributeValueDecoder decoder = test.DecoderFor(1234); - ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_IM_GLOBAL_STATUS(UnsupportedCluster)); + ASSERT_EQ(model.WriteAttribute(test.request, decoder), Status::UnsupportedCluster); } { TestWriteRequest test(kAdminSubjectDescriptor, ConcreteAttributePath(kMockEndpoint1, MockClusterId(1), 1234)); AttributeValueDecoder decoder = test.DecoderFor(1234); - ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute)); + ASSERT_EQ(model.WriteAttribute(test.request, decoder), Status::UnsupportedAttribute); } } @@ -2374,7 +2377,7 @@ TEST(TestCodegenModelViaMocks, WriteToGlobalAttribute) TestWriteRequest test(kAdminSubjectDescriptor, ConcreteAttributePath(kMockEndpoint1, MockClusterId(1), AttributeList::Id)); AttributeValueDecoder decoder = test.DecoderFor(1234); - ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_IM_GLOBAL_STATUS(UnsupportedWrite)); + ASSERT_EQ(model.WriteAttribute(test.request, decoder), Status::UnsupportedWrite); } TEST(TestCodegenModelViaMocks, EmberWriteFailure) @@ -2390,12 +2393,12 @@ TEST(TestCodegenModelViaMocks, EmberWriteFailure) { AttributeValueDecoder decoder = test.DecoderFor(1234); chip::Test::SetEmberReadOutput(Protocols::InteractionModel::Status::Failure); - ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_IM_GLOBAL_STATUS(Failure)); + ASSERT_EQ(model.WriteAttribute(test.request, decoder), Status::Failure); } { AttributeValueDecoder decoder = test.DecoderFor(1234); chip::Test::SetEmberReadOutput(Protocols::InteractionModel::Status::Busy); - ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_IM_GLOBAL_STATUS(Busy)); + ASSERT_EQ(model.WriteAttribute(test.request, decoder), Status::Busy); } // reset things to success to not affect other tests chip::Test::SetEmberReadOutput(ByteSpan()); @@ -2524,6 +2527,6 @@ TEST(TestCodegenModelViaMocks, EmberWriteInvalidDataType) // Embed specifically DOES NOT support structures. // Without AAI, we expect a data type error (translated to failure) - ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_IM_GLOBAL_STATUS(Failure)); + ASSERT_EQ(model.WriteAttribute(test.request, decoder), Status::Failure); ASSERT_TRUE(model.ChangeListener().DirtyList().empty()); } diff --git a/src/app/data-model-provider/ActionReturnStatus.cpp b/src/app/data-model-provider/ActionReturnStatus.cpp index 44f8e376f73930..2b1a431c3dc3b0 100644 --- a/src/app/data-model-provider/ActionReturnStatus.cpp +++ b/src/app/data-model-provider/ActionReturnStatus.cpp @@ -28,6 +28,26 @@ namespace DataModel { using Protocols::InteractionModel::ClusterStatusCode; using Protocols::InteractionModel::Status; +bool ActionReturnStatus::operator==(Protocols::InteractionModel::Status status) +{ + if (const CHIP_ERROR * err = std::get_if(&mReturnStatus)) + { + return *err == CHIP_ERROR_IM_GLOBAL_STATUS_VALUE(status); + } + + if (const ClusterStatusCode * internal_status = std::get_if(&mReturnStatus)) + { + if (internal_status->HasClusterSpecificCode()) + { + return false; // status has no cluster-specific code, so reject equality + } + + return internal_status->GetStatus() == status; + } + + chipDie(); +} + CHIP_ERROR ActionReturnStatus::GetUnderlyingError() const { @@ -109,39 +129,36 @@ bool ActionReturnStatus::IsOutOfSpaceError() const return false; } -void ActionReturnStatus::LogError(const char * prefix) const +void ActionReturnStatus::AddTo(StringBuilderBase & buffer) const { - if (const CHIP_ERROR * err = std::get_if(&mReturnStatus)) { - ChipLogError(InteractionModel, "%s: %" CHIP_ERROR_FORMAT, prefix, err->Format()); + buffer.AddFormat("%" CHIP_ERROR_FORMAT, err->Format()); + return; } if (const ClusterStatusCode * status = std::get_if(&mReturnStatus)) { - StringBuilder<48> txt; - #if CHIP_CONFIG_IM_STATUS_CODE_VERBOSE_FORMAT - txt.AddFormat("%s(%d)", Protocols::InteractionModel::StatusName(status->GetStatus()), + buffer.AddFormat("%s(%d)", Protocols::InteractionModel::StatusName(status->GetStatus()), static_cast(status->GetStatus())); #else if (status->IsSuccess()) { - txt.Add("Success"); + buffer.Add("Success"); } else { - txt.AddFormat("Status<%d>", static_cast(status->GetStatus())); + buffer.AddFormat("Status<%d>", static_cast(status->GetStatus())); } #endif chip::Optional clusterCode = status->GetClusterSpecificCode(); if (clusterCode.HasValue()) { - txt.AddFormat(", Code %d", static_cast(clusterCode.Value())); + buffer.AddFormat(", Code %d", static_cast(clusterCode.Value())); } - - ChipLogError(InteractionModel, "%s: %s", prefix, txt.c_str()); + return; } // all std::variant cases exhausted diff --git a/src/app/data-model-provider/ActionReturnStatus.h b/src/app/data-model-provider/ActionReturnStatus.h index efb8e8d5f82425..b1ea15b3203697 100644 --- a/src/app/data-model-provider/ActionReturnStatus.h +++ b/src/app/data-model-provider/ActionReturnStatus.h @@ -17,6 +17,7 @@ #pragma once #include +#include #include #include @@ -74,15 +75,15 @@ class ActionReturnStatus /// Generally this is when the return is based on CHIP_ERROR_NO_MEMORY or CHIP_ERROR_BUFFER_TOO_SMALL bool IsOutOfSpaceError() const; - /// Logs the underlying code as a ChipLogError. If the underlying data contains - /// a CHIP_ERROR, the error is reported. Otherwise the cluster status code data - /// is logged. - void LogError(const char * prefix) const; - - bool operator==(const ActionReturnStatus & other) const { return mReturnStatus == other.mReturnStatus; } bool operator!=(const ActionReturnStatus & other) const { return mReturnStatus != other.mReturnStatus; } + // specialization that allows compare with both ClusterStatusCode AND CHIP_ERROR global IM status + bool operator==(Protocols::InteractionModel::Status status); + + /// Get the formatted string of this status. + void AddTo(StringBuilderBase & buffer) const; + private: std::variant mReturnStatus; }; diff --git a/src/app/data-model-provider/BUILD.gn b/src/app/data-model-provider/BUILD.gn index 24930601cb669a..40f34db127d501 100644 --- a/src/app/data-model-provider/BUILD.gn +++ b/src/app/data-model-provider/BUILD.gn @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. import("//build_overrides/chip.gni") +import("//build_overrides/pigweed.gni") source_set("data-model-provider") { sources = [ @@ -42,3 +43,16 @@ source_set("data-model-provider") { "${chip_root}/src/messaging", ] } + +source_set("string-builder-adapters") { + sources = [ + "StringBuilderAdapters.cpp", + "StringBuilderAdapters.h", + ] + + public_deps = [ + ":data-model-provider", + "$dir_pw_string", + "${chip_root}/src/lib/core:string-builder-adapters", + ] +} diff --git a/src/app/reporting/Read-Checked.cpp b/src/app/reporting/Read-Checked.cpp index 2fad1b7f60d681..40940b46dee657 100644 --- a/src/app/reporting/Read-Checked.cpp +++ b/src/app/reporting/Read-Checked.cpp @@ -15,6 +15,7 @@ * limitations under the License. */ #include "app/data-model-provider/ActionReturnStatus.h" +#include "lib/support/StringBuilder.h" #include #include @@ -81,11 +82,14 @@ ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataModel, const Ac if (statusEmber != statusDm) { + StringBuilder<128> buffer; // Note log + chipDie instead of VerifyOrDie so that breakpoints (and usage of rr) // is easier to debug. ChipLogError(Test, "Different return codes between ember and DM"); - statusEmber.LogError(" Ember status: "); - statusDm.LogError(" DM status: "); + statusEmber.AddTo(buffer.Reset()); + ChipLogError(Test, " Ember status: %s", buffer.c_str()); + statusDm.AddTo(buffer.Reset()); + ChipLogError(Test, " DM status: %s", buffer.c_str()); // For time-dependent data, we may have size differences here: one data fitting in buffer // while another not, resulting in different errors (success vs out of space). diff --git a/src/app/reporting/Read-DataModel.cpp b/src/app/reporting/Read-DataModel.cpp index f8ccbf67f277d7..36c58b1ec247cd 100644 --- a/src/app/reporting/Read-DataModel.cpp +++ b/src/app/reporting/Read-DataModel.cpp @@ -15,6 +15,7 @@ * limitations under the License. */ #include "app/data-model-provider/ActionReturnStatus.h" +#include "lib/support/logging/TextOnlyLogging.h" #include #include @@ -94,7 +95,9 @@ DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataMode // and will be sent to the client as well). if (!status.IsOutOfSpaceError()) { - status.LogError("Failed to read attribute: "); + StringBuilder<128> buffer; + status.AddTo(buffer); + ChipLogError(DataManagement, "Failed to read attribute: %s", buffer.c_str()); } return status; } From 67a676c64bac4cd079d9c57791a5a262d1fa015b Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 1 Aug 2024 10:56:48 -0400 Subject: [PATCH 06/31] Propper formatting and comparisons in tests. Mock tests pass --- .../ActionReturnStatus.cpp | 48 ++++++++++++++----- .../data-model-provider/ActionReturnStatus.h | 9 ++-- 2 files changed, 41 insertions(+), 16 deletions(-) diff --git a/src/app/data-model-provider/ActionReturnStatus.cpp b/src/app/data-model-provider/ActionReturnStatus.cpp index 2b1a431c3dc3b0..a694df66d3685a 100644 --- a/src/app/data-model-provider/ActionReturnStatus.cpp +++ b/src/app/data-model-provider/ActionReturnStatus.cpp @@ -28,24 +28,50 @@ namespace DataModel { using Protocols::InteractionModel::ClusterStatusCode; using Protocols::InteractionModel::Status; -bool ActionReturnStatus::operator==(Protocols::InteractionModel::Status status) +namespace { + +bool StatusIsTheSameAsError(const ClusterStatusCode & status, const CHIP_ERROR & err) { - if (const CHIP_ERROR * err = std::get_if(&mReturnStatus)) + auto cluster_code = status.GetClusterSpecificCode(); + if (!cluster_code.HasValue()) { - return *err == CHIP_ERROR_IM_GLOBAL_STATUS_VALUE(status); + return err == CHIP_ERROR_IM_GLOBAL_STATUS_VALUE(status.GetStatus()); } - if (const ClusterStatusCode * internal_status = std::get_if(&mReturnStatus)) + if (status.GetStatus() != Status::Failure) { - if (internal_status->HasClusterSpecificCode()) - { - return false; // status has no cluster-specific code, so reject equality - } + return false; + } + + return err == CHIP_ERROR_IM_CLUSTER_STATUS_VALUE(cluster_code.Value()); +} + +} // namespace + +bool ActionReturnStatus::operator==(const ActionReturnStatus & other) const +{ + if (mReturnStatus == other.mReturnStatus) + { + return true; + } + + const ClusterStatusCode * thisStatus = std::get_if(&mReturnStatus); + const ClusterStatusCode * otherStatus = std::get_if(&other.mReturnStatus); + + const CHIP_ERROR * thisErr = std::get_if(&mReturnStatus); + const CHIP_ERROR * otherErr = std::get_if(&other.mReturnStatus); + + if (thisStatus && otherErr) + { + return StatusIsTheSameAsError(*thisStatus, *otherErr); + } - return internal_status->GetStatus() == status; + if (otherStatus && thisErr) + { + return StatusIsTheSameAsError(*otherStatus, *thisErr); } - chipDie(); + return false; } CHIP_ERROR ActionReturnStatus::GetUnderlyingError() const @@ -141,7 +167,7 @@ void ActionReturnStatus::AddTo(StringBuilderBase & buffer) const { #if CHIP_CONFIG_IM_STATUS_CODE_VERBOSE_FORMAT buffer.AddFormat("%s(%d)", Protocols::InteractionModel::StatusName(status->GetStatus()), - static_cast(status->GetStatus())); + static_cast(status->GetStatus())); #else if (status->IsSuccess()) { diff --git a/src/app/data-model-provider/ActionReturnStatus.h b/src/app/data-model-provider/ActionReturnStatus.h index b1ea15b3203697..a19ca000dcdd48 100644 --- a/src/app/data-model-provider/ActionReturnStatus.h +++ b/src/app/data-model-provider/ActionReturnStatus.h @@ -75,11 +75,10 @@ class ActionReturnStatus /// Generally this is when the return is based on CHIP_ERROR_NO_MEMORY or CHIP_ERROR_BUFFER_TOO_SMALL bool IsOutOfSpaceError() const; - bool operator==(const ActionReturnStatus & other) const { return mReturnStatus == other.mReturnStatus; } - bool operator!=(const ActionReturnStatus & other) const { return mReturnStatus != other.mReturnStatus; } - - // specialization that allows compare with both ClusterStatusCode AND CHIP_ERROR global IM status - bool operator==(Protocols::InteractionModel::Status status); + // NOTE: operator== will assume statues the same between CHIP_GLOBAL_IM_ERROR and a raw cluster status + // even though a CHIP_ERROR has some formatting info like file/line + bool operator==(const ActionReturnStatus & other) const; + bool operator!=(const ActionReturnStatus & other) const { return !(*this == other); } /// Get the formatted string of this status. void AddTo(StringBuilderBase & buffer) const; From d6b95e050fdc3530c6a4a22fe3b390d3535b5440 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 1 Aug 2024 10:59:13 -0400 Subject: [PATCH 07/31] Restyle --- src/app/codegen-data-model-provider/tests/BUILD.gn | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/codegen-data-model-provider/tests/BUILD.gn b/src/app/codegen-data-model-provider/tests/BUILD.gn index 1de428c16c8903..247685a91a0e7b 100644 --- a/src/app/codegen-data-model-provider/tests/BUILD.gn +++ b/src/app/codegen-data-model-provider/tests/BUILD.gn @@ -57,7 +57,7 @@ chip_test_suite("tests") { cflags = [ "-Wconversion" ] - public_deps = [ + public_deps = [ ":mock_model", "${chip_root}/src/app/data-model-provider:string-builder-adapters", ] From 035aa06dd2e2dfadd8344d087894bba221c3b0d2 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 1 Aug 2024 11:00:28 -0400 Subject: [PATCH 08/31] Restyle --- src/app/data-model-provider/Provider.h | 5 +++-- src/app/reporting/Engine.cpp | 4 ++-- src/app/reporting/Read-Checked.h | 7 ++++--- src/app/reporting/Read-Ember.h | 9 +++++---- src/app/tests/test-interaction-model-api.cpp | 2 +- src/controller/tests/data_model/DataModelFixtures.cpp | 3 ++- src/lib/core/CHIPError.h | 3 ++- 7 files changed, 19 insertions(+), 14 deletions(-) diff --git a/src/app/data-model-provider/Provider.h b/src/app/data-model-provider/Provider.h index 4cded7612ff561..068adbe275ccbd 100644 --- a/src/app/data-model-provider/Provider.h +++ b/src/app/data-model-provider/Provider.h @@ -24,10 +24,10 @@ #include #include +#include #include #include #include -#include namespace chip { namespace app { @@ -95,7 +95,8 @@ class Provider : public ProviderMetadataTree /// `handler` is used to send back the reply. /// - returning a value other than Success implies an error reply (error and data are mutually exclusive) - virtual ActionReturnStatus Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments, CommandHandler * handler) = 0; + virtual ActionReturnStatus Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments, + CommandHandler * handler) = 0; private: InteractionModelContext mContext = { nullptr }; diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp index ac7c8366c9cc79..e3461749bdae9f 100644 --- a/src/app/reporting/Engine.cpp +++ b/src/app/reporting/Engine.cpp @@ -1011,6 +1011,6 @@ void Engine::ScheduleUrgentEventDeliverySync(Optional fabricIndex) // TODO: MatterReportingAttributeChangeCallback should just live in libCHIP, // instead of being in ember-compatibility-functions. It does not depend on any // app-specific generated bits. -void __attribute__((weak)) MatterReportingAttributeChangeCallback(chip::EndpointId endpoint, chip::ClusterId clusterId, - chip::AttributeId attributeId) +void __attribute__((weak)) +MatterReportingAttributeChangeCallback(chip::EndpointId endpoint, chip::ClusterId clusterId, chip::AttributeId attributeId) {} diff --git a/src/app/reporting/Read-Checked.h b/src/app/reporting/Read-Checked.h index d6a5355f483c9d..742b1e2fef8c3d 100644 --- a/src/app/reporting/Read-Checked.h +++ b/src/app/reporting/Read-Checked.h @@ -28,9 +28,10 @@ namespace app { namespace reporting { namespace CheckedImpl { -DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataModel, const Access::SubjectDescriptor & subjectDescriptor, - bool isFabricFiltered, AttributeReportIBs::Builder & reportBuilder, - const ConcreteReadAttributePath & path, AttributeEncodeState * encoderState); +DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataModel, + const Access::SubjectDescriptor & subjectDescriptor, bool isFabricFiltered, + AttributeReportIBs::Builder & reportBuilder, + const ConcreteReadAttributePath & path, AttributeEncodeState * encoderState); } // namespace CheckedImpl } // namespace reporting diff --git a/src/app/reporting/Read-Ember.h b/src/app/reporting/Read-Ember.h index 5c192a7a392896..129746baf1f9c8 100644 --- a/src/app/reporting/Read-Ember.h +++ b/src/app/reporting/Read-Ember.h @@ -19,8 +19,8 @@ #include #include #include -#include #include +#include #include namespace chip { @@ -28,9 +28,10 @@ namespace app { namespace reporting { namespace EmberImpl { -DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataModel, const Access::SubjectDescriptor & subjectDescriptor, - bool isFabricFiltered, AttributeReportIBs::Builder & reportBuilder, - const ConcreteReadAttributePath & path, AttributeEncodeState * encoderState); +DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataModel, + const Access::SubjectDescriptor & subjectDescriptor, bool isFabricFiltered, + AttributeReportIBs::Builder & reportBuilder, + const ConcreteReadAttributePath & path, AttributeEncodeState * encoderState); } // namespace EmberImpl } // namespace reporting diff --git a/src/app/tests/test-interaction-model-api.cpp b/src/app/tests/test-interaction-model-api.cpp index 4d3b6c3f37c9e2..f483f4b3de88a4 100644 --- a/src/app/tests/test-interaction-model-api.cpp +++ b/src/app/tests/test-interaction-model-api.cpp @@ -174,7 +174,7 @@ ActionReturnStatus TestImCustomDataModel::WriteAttribute(const WriteAttributeReq } ActionReturnStatus TestImCustomDataModel::Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments, - CommandHandler * handler) + CommandHandler * handler) { return CHIP_ERROR_NOT_IMPLEMENTED; } diff --git a/src/controller/tests/data_model/DataModelFixtures.cpp b/src/controller/tests/data_model/DataModelFixtures.cpp index f4b1e43b12ed87..05fa16b956bb0c 100644 --- a/src/controller/tests/data_model/DataModelFixtures.cpp +++ b/src/controller/tests/data_model/DataModelFixtures.cpp @@ -525,7 +525,8 @@ ActionReturnStatus CustomDataModel::WriteAttribute(const WriteAttributeRequest & return CHIP_ERROR_NOT_IMPLEMENTED; } -ActionReturnStatus CustomDataModel::Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments, CommandHandler * handler) +ActionReturnStatus CustomDataModel::Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments, + CommandHandler * handler) { return CHIP_ERROR_NOT_IMPLEMENTED; } diff --git a/src/lib/core/CHIPError.h b/src/lib/core/CHIPError.h index 7e856ed4408d77..dc9bad511006f7 100644 --- a/src/lib/core/CHIPError.h +++ b/src/lib/core/CHIPError.h @@ -157,7 +157,8 @@ class ChipError {} #else constexpr ChipError(SdkPart part, uint8_t code, const char * file, unsigned int line) : - mError(MakeInteger(part, code)) CHIP_INITIALIZE_ERROR_SOURCE(file, line, /*loc=*/nullptr){} + mError(MakeInteger(part, code)) CHIP_INITIALIZE_ERROR_SOURCE(file, line, /*loc=*/nullptr) + {} #endif // CHIP_CONFIG_ERROR_SOURCE && __cplusplus >= 202002L /** From 37d688fd53548cdcdae2f1f4ade36b6b9dc894b2 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 1 Aug 2024 11:08:10 -0400 Subject: [PATCH 09/31] Fix typo --- src/app/reporting/Engine.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp index e3461749bdae9f..788a77635c1c17 100644 --- a/src/app/reporting/Engine.cpp +++ b/src/app/reporting/Engine.cpp @@ -216,7 +216,7 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu attributeReportIBs.Rollback(attributeBackup); apReadHandler->SetAttributeEncodeState(AttributeEncodeState()); - if (status.IsOutOfSpaceError()) + if (!status.IsOutOfSpaceError()) { ChipLogError(DataManagement, "Fail to retrieve data, roll back and encode status on clusterId: " ChipLogFormatMEI From cdacd184f834244969eef65536fad150ca80a222 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 1 Aug 2024 12:17:46 -0400 Subject: [PATCH 10/31] Add missing files --- .../StringBuilderAdapters.cpp | 32 ++++++++++++ .../StringBuilderAdapters.h | 46 +++++++++++++++++ src/app/data-model-provider/tests/BUILD.gn | 7 ++- .../tests/TestActionReturnStatus.cpp | 49 +++++++++++++++++++ 4 files changed, 133 insertions(+), 1 deletion(-) create mode 100644 src/app/data-model-provider/StringBuilderAdapters.cpp create mode 100644 src/app/data-model-provider/StringBuilderAdapters.h create mode 100644 src/app/data-model-provider/tests/TestActionReturnStatus.cpp diff --git a/src/app/data-model-provider/StringBuilderAdapters.cpp b/src/app/data-model-provider/StringBuilderAdapters.cpp new file mode 100644 index 00000000000000..ec55f3b71c0a17 --- /dev/null +++ b/src/app/data-model-provider/StringBuilderAdapters.cpp @@ -0,0 +1,32 @@ +/* + * Copyright (c) 2024 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#include + +#include + +namespace pw { + +template <> +StatusWithSize ToString(const chip::app::DataModel::ActionReturnStatus & status, + pw::span buffer) +{ + chip::StringBuilder<256> formatted; + status.AddTo(formatted); + + return pw::string::Format(buffer, "ActionReturnStatus<%s>", formatted.c_str()); +} + +} // namespace pw diff --git a/src/app/data-model-provider/StringBuilderAdapters.h b/src/app/data-model-provider/StringBuilderAdapters.h new file mode 100644 index 00000000000000..32da18f43f1681 --- /dev/null +++ b/src/app/data-model-provider/StringBuilderAdapters.h @@ -0,0 +1,46 @@ +/* + * Copyright (c) 2024 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#pragma once + +/// This header includes pigweed stringbuilder adaptations for various chip types. +/// You can see https://pigweed.dev/pw_string/guide.html as a reference. +/// +/// In particular, pigweed code generally looks like: +/// +/// pw::StringBuffer<42> sb; +/// sb << "Here is a value: "; +/// sb << value; +/// +/// Where specific formatters exist for "value". In particular these are used when +/// reporting unit test assertions such as ASSERT_EQ/EXPECT_EQ so if you write code +/// like: +/// +/// ASSERT_EQ(SomeCall(), CHIP_NO_ERROR); +/// +/// On failure without adapters, the objects are reported as "24-byte object at 0x....." +/// which is not as helpful as a full formatted output. + +#include + +#include + +namespace pw { + +template <> +StatusWithSize ToString(const chip::app::DataModel::ActionReturnStatus & status, + pw::span buffer); + +} // namespace pw diff --git a/src/app/data-model-provider/tests/BUILD.gn b/src/app/data-model-provider/tests/BUILD.gn index 23107b9b5b1fd8..679119f018a0b3 100644 --- a/src/app/data-model-provider/tests/BUILD.gn +++ b/src/app/data-model-provider/tests/BUILD.gn @@ -17,12 +17,17 @@ import("${chip_root}/build/chip/chip_test_suite.gni") chip_test_suite("tests") { output_name = "libIMInterfaceTests" - test_sources = [ "TestEventEmitting.cpp" ] + test_sources = [ + "TestEventEmitting.cpp", + "TestActionReturnStatus.cpp", + + ] cflags = [ "-Wconversion" ] public_deps = [ "${chip_root}/src/app/data-model-provider", + "${chip_root}/src/app/data-model-provider:string-builder-adapters", "${chip_root}/src/lib/core:string-builder-adapters", ] } diff --git a/src/app/data-model-provider/tests/TestActionReturnStatus.cpp b/src/app/data-model-provider/tests/TestActionReturnStatus.cpp new file mode 100644 index 00000000000000..bc5257a06fe399 --- /dev/null +++ b/src/app/data-model-provider/tests/TestActionReturnStatus.cpp @@ -0,0 +1,49 @@ +/* + * + * Copyright (c) 2024 Project CHIP Authors + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "lib/core/CHIPError.h" +#include "protocols/interaction_model/StatusCode.h" +#include +#include +#include + +#include + +using chip::app::DataModel::ActionReturnStatus; +using chip::Protocols::InteractionModel::Status; +using chip::Protocols::InteractionModel::ClusterStatusCode; + +TEST(TestActionReturnStatus, TestEquality) +{ + // equality should happen between equivalent statuses and chip_errors + ASSERT_EQ(ActionReturnStatus(Status::UnsupportedRead), Status::UnsupportedRead); + ASSERT_EQ(ActionReturnStatus(Status::UnsupportedWrite), CHIP_IM_GLOBAL_STATUS(UnsupportedRead)); + + ASSERT_EQ(ActionReturnStatus(CHIP_IM_GLOBAL_STATUS(Busy)), Status::Busy); + ASSERT_EQ(ActionReturnStatus(CHIP_IM_GLOBAL_STATUS(Busy)), CHIP_IM_GLOBAL_STATUS(Busy)); + + ASSERT_EQ(ActionReturnStatus(CHIP_IM_CLUSTER_STATUS(123)), CHIP_IM_CLUSTER_STATUS(123)); + ASSERT_EQ(ActionReturnStatus(ClusterStatusCode::ClusterSpecificFailure(123)), CHIP_IM_CLUSTER_STATUS(123)); + ASSERT_EQ(ActionReturnStatus(ClusterStatusCode::ClusterSpecificFailure(123)), ClusterStatusCode::ClusterSpecificFailure(123)); + ASSERT_EQ(ActionReturnStatus(ClusterStatusCode::ClusterSpecificSuccess(123)), ClusterStatusCode::ClusterSpecificSuccess(123)); +} + +TEST(TestActionReturnStatus, TestIsError) +{ + AS +} From 0ef9987ed2ea75a14b90f26f4457867c9651286e Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 1 Aug 2024 12:22:16 -0400 Subject: [PATCH 11/31] Added some unit tests that pass --- .../tests/TestActionReturnStatus.cpp | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/app/data-model-provider/tests/TestActionReturnStatus.cpp b/src/app/data-model-provider/tests/TestActionReturnStatus.cpp index bc5257a06fe399..26a859d59c4405 100644 --- a/src/app/data-model-provider/tests/TestActionReturnStatus.cpp +++ b/src/app/data-model-provider/tests/TestActionReturnStatus.cpp @@ -18,6 +18,7 @@ #include "lib/core/CHIPError.h" #include "protocols/interaction_model/StatusCode.h" +#include "pw_unit_test/framework_backend.h" #include #include #include @@ -32,7 +33,7 @@ TEST(TestActionReturnStatus, TestEquality) { // equality should happen between equivalent statuses and chip_errors ASSERT_EQ(ActionReturnStatus(Status::UnsupportedRead), Status::UnsupportedRead); - ASSERT_EQ(ActionReturnStatus(Status::UnsupportedWrite), CHIP_IM_GLOBAL_STATUS(UnsupportedRead)); + ASSERT_EQ(ActionReturnStatus(Status::UnsupportedWrite), CHIP_IM_GLOBAL_STATUS(UnsupportedWrite)); ASSERT_EQ(ActionReturnStatus(CHIP_IM_GLOBAL_STATUS(Busy)), Status::Busy); ASSERT_EQ(ActionReturnStatus(CHIP_IM_GLOBAL_STATUS(Busy)), CHIP_IM_GLOBAL_STATUS(Busy)); @@ -45,5 +46,13 @@ TEST(TestActionReturnStatus, TestEquality) TEST(TestActionReturnStatus, TestIsError) { - AS + ASSERT_TRUE(ActionReturnStatus(CHIP_IM_CLUSTER_STATUS(123)).IsError()); + ASSERT_TRUE(ActionReturnStatus(CHIP_ERROR_INTERNAL).IsError()); + ASSERT_TRUE(ActionReturnStatus(CHIP_ERROR_NO_MEMORY).IsError()); + ASSERT_TRUE(ActionReturnStatus(Status::UnsupportedRead).IsError()); + ASSERT_TRUE(ActionReturnStatus(ClusterStatusCode::ClusterSpecificFailure(123)).IsError()); + + ASSERT_FALSE(ActionReturnStatus(Status::Success).IsError()); + ASSERT_FALSE(ActionReturnStatus(ClusterStatusCode::ClusterSpecificSuccess(123)).IsError()); + ASSERT_FALSE(ActionReturnStatus(CHIP_NO_ERROR).IsError()); } From 54980d088ae7146e383cf6709b41e890a2c12924 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 1 Aug 2024 12:28:41 -0400 Subject: [PATCH 12/31] More tests --- .../tests/TestActionReturnStatus.cpp | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/src/app/data-model-provider/tests/TestActionReturnStatus.cpp b/src/app/data-model-provider/tests/TestActionReturnStatus.cpp index 26a859d59c4405..7787ab6d2af315 100644 --- a/src/app/data-model-provider/tests/TestActionReturnStatus.cpp +++ b/src/app/data-model-provider/tests/TestActionReturnStatus.cpp @@ -26,8 +26,8 @@ #include using chip::app::DataModel::ActionReturnStatus; -using chip::Protocols::InteractionModel::Status; using chip::Protocols::InteractionModel::ClusterStatusCode; +using chip::Protocols::InteractionModel::Status; TEST(TestActionReturnStatus, TestEquality) { @@ -56,3 +56,23 @@ TEST(TestActionReturnStatus, TestIsError) ASSERT_FALSE(ActionReturnStatus(ClusterStatusCode::ClusterSpecificSuccess(123)).IsError()); ASSERT_FALSE(ActionReturnStatus(CHIP_NO_ERROR).IsError()); } + +TEST(TestActionReturnStatus, TestUnderlyingError) +{ + ASSERT_EQ(ActionReturnStatus(ClusterStatusCode::ClusterSpecificFailure(123)).GetUnderlyingError(), CHIP_IM_CLUSTER_STATUS(123)); + ASSERT_EQ(ActionReturnStatus(ClusterStatusCode::ClusterSpecificSuccess(123)).GetUnderlyingError(), CHIP_NO_ERROR); + ASSERT_EQ(ActionReturnStatus(Status::Busy).GetUnderlyingError(), CHIP_IM_GLOBAL_STATUS(Busy)); + ASSERT_EQ(ActionReturnStatus(CHIP_ERROR_INTERNAL).GetUnderlyingError(), CHIP_ERROR_INTERNAL); +} + +TEST(TestActionReturnStatus, TestStatusCode) +{ + ASSERT_EQ(ActionReturnStatus(CHIP_ERROR_INTERNAL).GetStatusCode(), ClusterStatusCode(Status::Failure)); + ASSERT_EQ(ActionReturnStatus(Status::Busy).GetStatusCode(), ClusterStatusCode(Status::Busy)); + ASSERT_EQ(ActionReturnStatus(ClusterStatusCode::ClusterSpecificSuccess(123)).GetStatusCode(), + ClusterStatusCode::ClusterSpecificSuccess(123)); + ASSERT_EQ(ActionReturnStatus(ClusterStatusCode::ClusterSpecificFailure(123)).GetStatusCode(), + ClusterStatusCode::ClusterSpecificFailure(123)); + ASSERT_EQ(ActionReturnStatus(CHIP_IM_CLUSTER_STATUS(0x12)).GetStatusCode(), ClusterStatusCode::ClusterSpecificFailure(0x12)); + ASSERT_EQ(ActionReturnStatus(CHIP_IM_GLOBAL_STATUS(Timeout)).GetStatusCode(), ClusterStatusCode(Status::Timeout)); +} From c7aa8cefb0d2f4e2393c49a8d85f70ba3b15e1fb Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 1 Aug 2024 12:30:20 -0400 Subject: [PATCH 13/31] Restyle --- src/app/data-model-provider/tests/BUILD.gn | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/app/data-model-provider/tests/BUILD.gn b/src/app/data-model-provider/tests/BUILD.gn index 679119f018a0b3..9829a2f4622514 100644 --- a/src/app/data-model-provider/tests/BUILD.gn +++ b/src/app/data-model-provider/tests/BUILD.gn @@ -18,9 +18,8 @@ chip_test_suite("tests") { output_name = "libIMInterfaceTests" test_sources = [ - "TestEventEmitting.cpp", "TestActionReturnStatus.cpp", - + "TestEventEmitting.cpp", ] cflags = [ "-Wconversion" ] From e87b6eaf476031336c452d13b9a9abec03efa972 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 1 Aug 2024 13:21:40 -0400 Subject: [PATCH 14/31] Update src/app/codegen-data-model-provider/EmberMetadata.cpp Co-authored-by: Boris Zbarsky --- src/app/codegen-data-model-provider/EmberMetadata.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/codegen-data-model-provider/EmberMetadata.cpp b/src/app/codegen-data-model-provider/EmberMetadata.cpp index 8853c562dd40ce..08a2724dc847f8 100644 --- a/src/app/codegen-data-model-provider/EmberMetadata.cpp +++ b/src/app/codegen-data-model-provider/EmberMetadata.cpp @@ -28,7 +28,7 @@ using Protocols::InteractionModel::Status; std::variant FindAttributeMetadata(const ConcreteAttributePath & aPath) { From e5c1120ff141bfcd81632ff43e052d4e18238597 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 1 Aug 2024 13:21:48 -0400 Subject: [PATCH 15/31] Update src/app/data-model-provider/ActionReturnStatus.h Co-authored-by: Boris Zbarsky --- src/app/data-model-provider/ActionReturnStatus.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/app/data-model-provider/ActionReturnStatus.h b/src/app/data-model-provider/ActionReturnStatus.h index a19ca000dcdd48..f4d6e5390f2ed4 100644 --- a/src/app/data-model-provider/ActionReturnStatus.h +++ b/src/app/data-model-provider/ActionReturnStatus.h @@ -32,9 +32,8 @@ namespace DataModel { /// which is a code (InteractionModel::Status) and may have an associated /// cluster-specific code. /// -/// However some actions specifically may return additional errors for -/// chunking (i.e. out of space, retry or send existing partial data) hence -/// the exitance of this class: +/// However some actions specifically may return additional information for +/// chunking, hence the existence of this class: /// /// - encapsulates a ClusterStatusCode for an actual action result /// - encapsulates a underlying CHIP_ERROR for reporting purposes From c82e8fe15fe9e0bae36970a15c501fb5c86477b8 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 1 Aug 2024 13:22:03 -0400 Subject: [PATCH 16/31] Update src/app/data-model-provider/ActionReturnStatus.h Co-authored-by: Boris Zbarsky --- src/app/data-model-provider/ActionReturnStatus.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/data-model-provider/ActionReturnStatus.h b/src/app/data-model-provider/ActionReturnStatus.h index f4d6e5390f2ed4..1f5db5fab4d2f8 100644 --- a/src/app/data-model-provider/ActionReturnStatus.h +++ b/src/app/data-model-provider/ActionReturnStatus.h @@ -37,7 +37,7 @@ namespace DataModel { /// /// - encapsulates a ClusterStatusCode for an actual action result /// - encapsulates a underlying CHIP_ERROR for reporting purposes -/// and to check for out-of-space specific errors +/// - has a way to check for "chunking needed" status. /// /// The class is directly constructible from statuses (non-exlicit) to make /// returning of values easy. From bdc065ea860724f93e48fb33def57ed9a68e99ff Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 1 Aug 2024 13:23:17 -0400 Subject: [PATCH 17/31] Update src/app/data-model-provider/ActionReturnStatus.h Co-authored-by: Boris Zbarsky --- src/app/data-model-provider/ActionReturnStatus.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/data-model-provider/ActionReturnStatus.h b/src/app/data-model-provider/ActionReturnStatus.h index 1f5db5fab4d2f8..dc5057107eae34 100644 --- a/src/app/data-model-provider/ActionReturnStatus.h +++ b/src/app/data-model-provider/ActionReturnStatus.h @@ -74,7 +74,7 @@ class ActionReturnStatus /// Generally this is when the return is based on CHIP_ERROR_NO_MEMORY or CHIP_ERROR_BUFFER_TOO_SMALL bool IsOutOfSpaceError() const; - // NOTE: operator== will assume statues the same between CHIP_GLOBAL_IM_ERROR and a raw cluster status + // NOTE: operator== will treat a CHIP_GLOBAL_IM_ERROR and a raw cluster status as equal if the statuses match, // even though a CHIP_ERROR has some formatting info like file/line bool operator==(const ActionReturnStatus & other) const; bool operator!=(const ActionReturnStatus & other) const { return !(*this == other); } From 739f792c1d6d73b94178b320aa1c74ae5b109c24 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 1 Aug 2024 13:24:54 -0400 Subject: [PATCH 18/31] Update src/lib/core/CHIPError.h Co-authored-by: Boris Zbarsky --- src/lib/core/CHIPError.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/lib/core/CHIPError.h b/src/lib/core/CHIPError.h index dc9bad511006f7..a55849a3da2e0f 100644 --- a/src/lib/core/CHIPError.h +++ b/src/lib/core/CHIPError.h @@ -453,7 +453,8 @@ using CHIP_ERROR = ::chip::ChipError; // #define CHIP_IM_CLUSTER_STATUS(type) CHIP_SDK_ERROR(::chip::ChipError::SdkPart::kIMClusterStatus, type) -// Defines a runtime-value for a chip-error that contains a cluster Status. +// Defines a runtime-value for a chip-error that contains a cluster-specific error status. +// Must not be used with cluster-specific success status codes. #if CHIP_CONFIG_ERROR_SOURCE #define CHIP_ERROR_IM_CLUSTER_STATUS_VALUE(status_value) \ ::chip::ChipError(::chip::ChipError::SdkPart::kIMClusterStatus, status_value, __FILE__, __LINE__) From 5ab01905f17c0f99253c8d5a335cce2dc31ae786 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 1 Aug 2024 15:36:08 -0400 Subject: [PATCH 19/31] Rename IsOutOfSpaceError and do not document specifics --- src/app/data-model-provider/ActionReturnStatus.cpp | 2 +- src/app/data-model-provider/ActionReturnStatus.h | 2 +- src/app/data-model-provider/Provider.h | 8 +------- src/app/reporting/Engine.cpp | 4 ++-- src/app/reporting/Read-DataModel.cpp | 4 ++-- 5 files changed, 7 insertions(+), 13 deletions(-) diff --git a/src/app/data-model-provider/ActionReturnStatus.cpp b/src/app/data-model-provider/ActionReturnStatus.cpp index a694df66d3685a..c1ba20ce62a1a1 100644 --- a/src/app/data-model-provider/ActionReturnStatus.cpp +++ b/src/app/data-model-provider/ActionReturnStatus.cpp @@ -145,7 +145,7 @@ bool ActionReturnStatus::IsSuccess() const chipDie(); } -bool ActionReturnStatus::IsOutOfSpaceError() const +bool ActionReturnStatus::IsOutOfSpaceEncodingResponse() const { if (const CHIP_ERROR * err = std::get_if(&mReturnStatus)) { diff --git a/src/app/data-model-provider/ActionReturnStatus.h b/src/app/data-model-provider/ActionReturnStatus.h index a19ca000dcdd48..0b6fbcddcf8c1c 100644 --- a/src/app/data-model-provider/ActionReturnStatus.h +++ b/src/app/data-model-provider/ActionReturnStatus.h @@ -73,7 +73,7 @@ class ActionReturnStatus /// chunking can handle by sending partial list data). /// /// Generally this is when the return is based on CHIP_ERROR_NO_MEMORY or CHIP_ERROR_BUFFER_TOO_SMALL - bool IsOutOfSpaceError() const; + bool IsOutOfSpaceEncodingResponse() const; // NOTE: operator== will assume statues the same between CHIP_GLOBAL_IM_ERROR and a raw cluster status // even though a CHIP_ERROR has some formatting info like file/line diff --git a/src/app/data-model-provider/Provider.h b/src/app/data-model-provider/Provider.h index 068adbe275ccbd..8ac1bda736c7ba 100644 --- a/src/app/data-model-provider/Provider.h +++ b/src/app/data-model-provider/Provider.h @@ -69,7 +69,7 @@ class Provider : public ProviderMetadataTree /// granted to access the cluster in the path, then the path SHALL be discarded. /// /// Return value notes: - /// ActionReturnStatus::IsOutOfSpaceError(i.e. CHIP_ERROR_NO_MEMORY or CHIP_ERROR_BUFFER_TOO_SMALL): + /// ActionReturnStatus::IsOutOfSpaceEncodingResponse /// - Indicates that list encoding had insufficient buffer space to encode elements. /// - encoder::GetState().AllowPartialData() determines if these errors are permanent (no partial /// data allowed) or further encoding can be retried (AllowPartialData true for list encoding) @@ -85,12 +85,6 @@ class Provider : public ProviderMetadataTree /// - ACL validation (see notes on OperationFlags::kInternal) /// - Validation of readability/writability (also controlled by OperationFlags::kInternal) /// - Validation of timed interaction required (also controlled by OperationFlags::kInternal) - /// - /// Return notes: - /// - The following codes are interesting/expected to be possible: - /// - `UnsupportedWrite` for attempts to write read-only data - /// - `UnsupportedAccess` for ACL failures - /// - `NeedsTimedInteraction` for writes that are not timed however are required to be so virtual ActionReturnStatus WriteAttribute(const WriteAttributeRequest & request, AttributeValueDecoder & decoder) = 0; /// `handler` is used to send back the reply. diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp index 788a77635c1c17..b5621e94bd9f47 100644 --- a/src/app/reporting/Engine.cpp +++ b/src/app/reporting/Engine.cpp @@ -196,7 +196,7 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu // Otherwise, if partial data allowed, save the encode state. // Otherwise roll back. If we have already encoded some chunks, we are done; otherwise encode status. - if (encodeState.AllowPartialData() && status.IsOutOfSpaceError()) + if (encodeState.AllowPartialData() && status.IsOutOfSpaceEncodingResponse()) { ChipLogDetail(DataManagement, "List does not fit in packet, chunk between list items for clusterId: " ChipLogFormatMEI @@ -216,7 +216,7 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu attributeReportIBs.Rollback(attributeBackup); apReadHandler->SetAttributeEncodeState(AttributeEncodeState()); - if (!status.IsOutOfSpaceError()) + if (!status.IsOutOfSpaceEncodingResponse()) { ChipLogError(DataManagement, "Fail to retrieve data, roll back and encode status on clusterId: " ChipLogFormatMEI diff --git a/src/app/reporting/Read-DataModel.cpp b/src/app/reporting/Read-DataModel.cpp index 36c58b1ec247cd..accffabec0c150 100644 --- a/src/app/reporting/Read-DataModel.cpp +++ b/src/app/reporting/Read-DataModel.cpp @@ -83,7 +83,7 @@ DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataMode // Encoder state is relevant for errors in case they are retryable. // - // Generally only IsOutOfSpaceError(err) would be retryable, however we save the state + // Generally only out of space encoding errors would be retryable, however we save the state // for all errors in case this is information that is useful (retry or error position). if (encoderState != nullptr) { @@ -93,7 +93,7 @@ DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataMode // Out of space errors may be chunked data, reporting those cases would be very confusing // as they are not fully errors. Report only others (which presumably are not recoverable // and will be sent to the client as well). - if (!status.IsOutOfSpaceError()) + if (!status.IsOutOfSpaceEncodingResponse()) { StringBuilder<128> buffer; status.AddTo(buffer); From 5974a91ba7ce51e0fd3db71a0844b069e43b1752 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 1 Aug 2024 15:41:17 -0400 Subject: [PATCH 20/31] Document invoke return codes --- src/app/data-model-provider/Provider.h | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/app/data-model-provider/Provider.h b/src/app/data-model-provider/Provider.h index 8ac1bda736c7ba..51106ba34b7d3a 100644 --- a/src/app/data-model-provider/Provider.h +++ b/src/app/data-model-provider/Provider.h @@ -89,6 +89,15 @@ class Provider : public ProviderMetadataTree /// `handler` is used to send back the reply. /// - returning a value other than Success implies an error reply (error and data are mutually exclusive) + /// + /// Returning anything other than CHIP_NO_ERROR or Status::Success (i.e. success without a return code) + /// means that the invoke will be considered to be returning the given status WITHOUT any data (any data + /// that was sent via Handler is to be rolled back/discarded). + /// + /// This is because only one of the following may be encoded in a response: + /// - data (as CommandDataIB) which is assumed a "response as a success" + /// - status (as a CommandStatusIB) which is considered a final status, usually an error however + /// cluster-specific success statuses also exist. virtual ActionReturnStatus Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments, CommandHandler * handler) = 0; From 1e0c949c713bfb1be0f48cf8e1c1ba9e4a43cfba Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 1 Aug 2024 15:45:15 -0400 Subject: [PATCH 21/31] Use the new out of space method in checked --- src/app/reporting/Read-Checked.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/reporting/Read-Checked.cpp b/src/app/reporting/Read-Checked.cpp index 40940b46dee657..b0ad9ff30096e4 100644 --- a/src/app/reporting/Read-Checked.cpp +++ b/src/app/reporting/Read-Checked.cpp @@ -127,7 +127,7 @@ ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataModel, const Ac // For chunked reads, the encoder state MUST be identical (since this is what controls // where chunking resumes). - if ((statusEmber == CHIP_ERROR_NO_MEMORY) || (statusEmber == CHIP_ERROR_BUFFER_TOO_SMALL)) + if (statusEmber.IsOutOfSpaceEncodingResponse()) { // Encoder state MUST match on partial reads (used by chunking) // specifically ReadViaAccessInterface in ember-compatibility-functions only From 5bf5dfe13ff835b5f0405fdcc055bf858c7c3bef Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 1 Aug 2024 15:50:42 -0400 Subject: [PATCH 22/31] Restyle --- src/app/codegen-data-model-provider/EmberMetadata.cpp | 6 +++--- src/app/data-model-provider/Provider.h | 2 +- src/lib/core/CHIPError.h | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/app/codegen-data-model-provider/EmberMetadata.cpp b/src/app/codegen-data-model-provider/EmberMetadata.cpp index 08a2724dc847f8..69f9e3e889369c 100644 --- a/src/app/codegen-data-model-provider/EmberMetadata.cpp +++ b/src/app/codegen-data-model-provider/EmberMetadata.cpp @@ -26,9 +26,9 @@ namespace Ember { using Protocols::InteractionModel::Status; -std::variant FindAttributeMetadata(const ConcreteAttributePath & aPath) { diff --git a/src/app/data-model-provider/Provider.h b/src/app/data-model-provider/Provider.h index 51106ba34b7d3a..6828bf43952590 100644 --- a/src/app/data-model-provider/Provider.h +++ b/src/app/data-model-provider/Provider.h @@ -90,7 +90,7 @@ class Provider : public ProviderMetadataTree /// `handler` is used to send back the reply. /// - returning a value other than Success implies an error reply (error and data are mutually exclusive) /// - /// Returning anything other than CHIP_NO_ERROR or Status::Success (i.e. success without a return code) + /// Returning anything other than CHIP_NO_ERROR or Status::Success (i.e. success without a return code) /// means that the invoke will be considered to be returning the given status WITHOUT any data (any data /// that was sent via Handler is to be rolled back/discarded). /// diff --git a/src/lib/core/CHIPError.h b/src/lib/core/CHIPError.h index a55849a3da2e0f..84b0de1a47f3ba 100644 --- a/src/lib/core/CHIPError.h +++ b/src/lib/core/CHIPError.h @@ -453,7 +453,7 @@ using CHIP_ERROR = ::chip::ChipError; // #define CHIP_IM_CLUSTER_STATUS(type) CHIP_SDK_ERROR(::chip::ChipError::SdkPart::kIMClusterStatus, type) -// Defines a runtime-value for a chip-error that contains a cluster-specific error status. +// Defines a runtime-value for a chip-error that contains a cluster-specific error status. // Must not be used with cluster-specific success status codes. #if CHIP_CONFIG_ERROR_SOURCE #define CHIP_ERROR_IM_CLUSTER_STATUS_VALUE(status_value) \ From 20410b3d77a26ea3fa9b7726e81df9356c79ace4 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 1 Aug 2024 16:27:08 -0400 Subject: [PATCH 23/31] Allow ClusterStatusCode to be constructed from a CHIP_ERROR --- .../ActionReturnStatus.cpp | 17 +------------ .../interaction_model/StatusCode.cpp | 24 +++++++++++++++++++ src/protocols/interaction_model/StatusCode.h | 1 + 3 files changed, 26 insertions(+), 16 deletions(-) diff --git a/src/app/data-model-provider/ActionReturnStatus.cpp b/src/app/data-model-provider/ActionReturnStatus.cpp index c1ba20ce62a1a1..2891f5e2d196eb 100644 --- a/src/app/data-model-provider/ActionReturnStatus.cpp +++ b/src/app/data-model-provider/ActionReturnStatus.cpp @@ -107,22 +107,7 @@ ClusterStatusCode ActionReturnStatus::GetStatusCode() const if (const CHIP_ERROR * err = std::get_if(&mReturnStatus)) { - if (err->IsPart(ChipError::SdkPart::kIMClusterStatus)) - { - return ClusterStatusCode::ClusterSpecificFailure(err->GetSdkCode()); - } - - if (*err == CHIP_NO_ERROR) - { - return ClusterStatusCode(Status::Success); - } - - if (err->IsPart(ChipError::SdkPart::kIMGlobalStatus)) - { - return ClusterStatusCode(static_cast(err->GetSdkCode())); - } - - return ClusterStatusCode(Status::Failure); + return ClusterStatusCode(*err); } // all std::variant cases exhausted diff --git a/src/protocols/interaction_model/StatusCode.cpp b/src/protocols/interaction_model/StatusCode.cpp index 36e1274f4e6c1e..b91817a0290e4a 100644 --- a/src/protocols/interaction_model/StatusCode.cpp +++ b/src/protocols/interaction_model/StatusCode.cpp @@ -37,6 +37,30 @@ const char * StatusName(Status status) return "Unallocated"; } #endif // CHIP_CONFIG_IM_STATUS_CODE_VERBOSE_FORMAT +// +ClusterStatusCode::ClusterStatusCode(CHIP_ERROR err) +{ + if (err.IsPart(ChipError::SdkPart::kIMClusterStatus)) + { + mStatus = Status::Failure; + mClusterSpecificCode = chip::MakeOptional(err.GetSdkCode()); + return; + } + + if (err == CHIP_NO_ERROR) + { + mStatus = Status::Success; + return; + } + + if (err.IsPart(ChipError::SdkPart::kIMGlobalStatus)) + { + mStatus = static_cast(err.GetSdkCode()); + return; + } + + mStatus = Status::Failure; +} } // namespace InteractionModel } // namespace Protocols diff --git a/src/protocols/interaction_model/StatusCode.h b/src/protocols/interaction_model/StatusCode.h index 9326c86653759c..b30ef95ea2d1e2 100644 --- a/src/protocols/interaction_model/StatusCode.h +++ b/src/protocols/interaction_model/StatusCode.h @@ -68,6 +68,7 @@ class ClusterStatusCode { public: explicit ClusterStatusCode(Status status) : mStatus(status) {} + explicit ClusterStatusCode(CHIP_ERROR err); // We only have simple copyable members, so we should be trivially copyable. ClusterStatusCode(const ClusterStatusCode & other) = default; From 17573d000ec47962ee8c39f82d2fb3831e4dab6b Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 1 Aug 2024 16:45:18 -0400 Subject: [PATCH 24/31] Format action statuses as c_str. HOWEVER this wastes 32 bytes of BSS --- .../ActionReturnStatus.cpp | 28 +++++++++++++------ .../data-model-provider/ActionReturnStatus.h | 8 +++++- .../StringBuilderAdapters.cpp | 5 +--- src/app/reporting/Read-Checked.cpp | 6 ++-- src/app/reporting/Read-DataModel.cpp | 4 +-- 5 files changed, 30 insertions(+), 21 deletions(-) diff --git a/src/app/data-model-provider/ActionReturnStatus.cpp b/src/app/data-model-provider/ActionReturnStatus.cpp index 2891f5e2d196eb..5955ab35a37aef 100644 --- a/src/app/data-model-provider/ActionReturnStatus.cpp +++ b/src/app/data-model-provider/ActionReturnStatus.cpp @@ -14,6 +14,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +#include "lib/core/CHIPConfig.h" #include "lib/core/CHIPError.h" #include "lib/support/CodeUtils.h" #include "protocols/interaction_model/StatusCode.h" @@ -140,36 +141,45 @@ bool ActionReturnStatus::IsOutOfSpaceEncodingResponse() const return false; } -void ActionReturnStatus::AddTo(StringBuilderBase & buffer) const +const char * ActionReturnStatus::c_str() const { + + // Generally size should be sufficient. + // len("Status<123>, Code 255") == 21 (and then 22 for null terminator. We have slack.) + static chip::StringBuilder<32> sFormatBuffer; + if (const CHIP_ERROR * err = std::get_if(&mReturnStatus)) { - buffer.AddFormat("%" CHIP_ERROR_FORMAT, err->Format()); - return; +#if CHIP_CONFIG_ERROR_FORMAT_AS_STRING + return err->Format(); // any length +#else + sFormatBuffer.Reset().AddFormat("%" CHIP_ERROR_FORMAT, *err); + return sFormatBuffer.c_str(); +#endif } if (const ClusterStatusCode * status = std::get_if(&mReturnStatus)) { #if CHIP_CONFIG_IM_STATUS_CODE_VERBOSE_FORMAT - buffer.AddFormat("%s(%d)", Protocols::InteractionModel::StatusName(status->GetStatus()), - static_cast(status->GetStatus())); + sFormatBuffer.AddFormat("%s(%d)", Protocols::InteractionModel::StatusName(status->GetStatus()), + static_cast(status->GetStatus())); #else if (status->IsSuccess()) { - buffer.Add("Success"); + sFormatBuffer.Add("Success"); } else { - buffer.AddFormat("Status<%d>", static_cast(status->GetStatus())); + sFormatBuffer.AddFormat("Status<%d>", static_cast(status->GetStatus())); } #endif chip::Optional clusterCode = status->GetClusterSpecificCode(); if (clusterCode.HasValue()) { - buffer.AddFormat(", Code %d", static_cast(clusterCode.Value())); + sFormatBuffer.AddFormat(", Code %d", static_cast(clusterCode.Value())); } - return; + return sFormatBuffer.c_str(); } // all std::variant cases exhausted diff --git a/src/app/data-model-provider/ActionReturnStatus.h b/src/app/data-model-provider/ActionReturnStatus.h index 6af2e96acd63b6..7f78cc55b241bc 100644 --- a/src/app/data-model-provider/ActionReturnStatus.h +++ b/src/app/data-model-provider/ActionReturnStatus.h @@ -80,7 +80,13 @@ class ActionReturnStatus bool operator!=(const ActionReturnStatus & other) const { return !(*this == other); } /// Get the formatted string of this status. - void AddTo(StringBuilderBase & buffer) const; + /// + /// NOTE: this is NOT thread safe in the general case, however the safety guarantees + /// are similar to chip::ErrorStr which also assumes a static buffer. + /// + /// Use this in the chip main event loop (and since that is a single thread, + /// there should be no races) + const char *c_str() const; private: std::variant mReturnStatus; diff --git a/src/app/data-model-provider/StringBuilderAdapters.cpp b/src/app/data-model-provider/StringBuilderAdapters.cpp index ec55f3b71c0a17..93ad9863fc9762 100644 --- a/src/app/data-model-provider/StringBuilderAdapters.cpp +++ b/src/app/data-model-provider/StringBuilderAdapters.cpp @@ -23,10 +23,7 @@ template <> StatusWithSize ToString(const chip::app::DataModel::ActionReturnStatus & status, pw::span buffer) { - chip::StringBuilder<256> formatted; - status.AddTo(formatted); - - return pw::string::Format(buffer, "ActionReturnStatus<%s>", formatted.c_str()); + return pw::string::Format(buffer, "ActionReturnStatus<%s>", status.c_str()); } } // namespace pw diff --git a/src/app/reporting/Read-Checked.cpp b/src/app/reporting/Read-Checked.cpp index b0ad9ff30096e4..cd821bc67ae4f6 100644 --- a/src/app/reporting/Read-Checked.cpp +++ b/src/app/reporting/Read-Checked.cpp @@ -86,10 +86,8 @@ ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataModel, const Ac // Note log + chipDie instead of VerifyOrDie so that breakpoints (and usage of rr) // is easier to debug. ChipLogError(Test, "Different return codes between ember and DM"); - statusEmber.AddTo(buffer.Reset()); - ChipLogError(Test, " Ember status: %s", buffer.c_str()); - statusDm.AddTo(buffer.Reset()); - ChipLogError(Test, " DM status: %s", buffer.c_str()); + ChipLogError(Test, " Ember status: %s", statusEmber.c_str()); + ChipLogError(Test, " DM status: %s", statusDm.c_str()); // For time-dependent data, we may have size differences here: one data fitting in buffer // while another not, resulting in different errors (success vs out of space). diff --git a/src/app/reporting/Read-DataModel.cpp b/src/app/reporting/Read-DataModel.cpp index accffabec0c150..7df19e2c89ecfc 100644 --- a/src/app/reporting/Read-DataModel.cpp +++ b/src/app/reporting/Read-DataModel.cpp @@ -95,9 +95,7 @@ DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataMode // and will be sent to the client as well). if (!status.IsOutOfSpaceEncodingResponse()) { - StringBuilder<128> buffer; - status.AddTo(buffer); - ChipLogError(DataManagement, "Failed to read attribute: %s", buffer.c_str()); + ChipLogError(DataManagement, "Failed to read attribute: %s", status.c_str()); } return status; } From 46155f26ca5a2d91d08eb8be3278bd60507f9610 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 1 Aug 2024 16:50:44 -0400 Subject: [PATCH 25/31] Fix error formatting --- src/app/data-model-provider/ActionReturnStatus.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/data-model-provider/ActionReturnStatus.cpp b/src/app/data-model-provider/ActionReturnStatus.cpp index 5955ab35a37aef..82a1992bc45e84 100644 --- a/src/app/data-model-provider/ActionReturnStatus.cpp +++ b/src/app/data-model-provider/ActionReturnStatus.cpp @@ -153,7 +153,7 @@ const char * ActionReturnStatus::c_str() const #if CHIP_CONFIG_ERROR_FORMAT_AS_STRING return err->Format(); // any length #else - sFormatBuffer.Reset().AddFormat("%" CHIP_ERROR_FORMAT, *err); + sFormatBuffer.Reset().AddFormat("%" CHIP_ERROR_FORMAT, err->Format()); return sFormatBuffer.c_str(); #endif } From a089ee21bff991311a4ed70cea4db6dddf200d35 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 1 Aug 2024 16:52:25 -0400 Subject: [PATCH 26/31] Restyle --- src/app/data-model-provider/ActionReturnStatus.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/data-model-provider/ActionReturnStatus.h b/src/app/data-model-provider/ActionReturnStatus.h index 7f78cc55b241bc..516541483ad068 100644 --- a/src/app/data-model-provider/ActionReturnStatus.h +++ b/src/app/data-model-provider/ActionReturnStatus.h @@ -83,10 +83,10 @@ class ActionReturnStatus /// /// NOTE: this is NOT thread safe in the general case, however the safety guarantees /// are similar to chip::ErrorStr which also assumes a static buffer. - /// - /// Use this in the chip main event loop (and since that is a single thread, + /// + /// Use this in the chip main event loop (and since that is a single thread, /// there should be no races) - const char *c_str() const; + const char * c_str() const; private: std::variant mReturnStatus; From a19f2bf371c415fdd11f36f8e16ba44c90cc7486 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 1 Aug 2024 16:53:48 -0400 Subject: [PATCH 27/31] Fix includes to be system paths --- src/app/data-model-provider/ActionReturnStatus.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/app/data-model-provider/ActionReturnStatus.cpp b/src/app/data-model-provider/ActionReturnStatus.cpp index 82a1992bc45e84..3af05b2d184f36 100644 --- a/src/app/data-model-provider/ActionReturnStatus.cpp +++ b/src/app/data-model-provider/ActionReturnStatus.cpp @@ -14,11 +14,11 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -#include "lib/core/CHIPConfig.h" -#include "lib/core/CHIPError.h" -#include "lib/support/CodeUtils.h" -#include "protocols/interaction_model/StatusCode.h" #include +#include +#include +#include +#include #include From 245898b54e2d4ce0dd20be5c4e5c6ee5fd241f83 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 2 Aug 2024 11:15:21 -0400 Subject: [PATCH 28/31] Update src/app/data-model-provider/Provider.h Co-authored-by: Boris Zbarsky --- src/app/data-model-provider/Provider.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/data-model-provider/Provider.h b/src/app/data-model-provider/Provider.h index 6828bf43952590..48eeef7db0da61 100644 --- a/src/app/data-model-provider/Provider.h +++ b/src/app/data-model-provider/Provider.h @@ -92,7 +92,7 @@ class Provider : public ProviderMetadataTree /// /// Returning anything other than CHIP_NO_ERROR or Status::Success (i.e. success without a return code) /// means that the invoke will be considered to be returning the given status WITHOUT any data (any data - /// that was sent via Handler is to be rolled back/discarded). + /// that was sent via CommandHandler is to be rolled back/discarded). /// /// This is because only one of the following may be encoded in a response: /// - data (as CommandDataIB) which is assumed a "response as a success" From 316f6ec82c85779cbce8aa5dd6ceb7944dabf1c7 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 2 Aug 2024 11:15:28 -0400 Subject: [PATCH 29/31] Update src/app/data-model-provider/Provider.h Co-authored-by: Boris Zbarsky --- src/app/data-model-provider/Provider.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/data-model-provider/Provider.h b/src/app/data-model-provider/Provider.h index 48eeef7db0da61..f38568319eb425 100644 --- a/src/app/data-model-provider/Provider.h +++ b/src/app/data-model-provider/Provider.h @@ -91,7 +91,7 @@ class Provider : public ProviderMetadataTree /// - returning a value other than Success implies an error reply (error and data are mutually exclusive) /// /// Returning anything other than CHIP_NO_ERROR or Status::Success (i.e. success without a return code) - /// means that the invoke will be considered to be returning the given status WITHOUT any data (any data + /// means that the invoke will be considered to be returning the given path-specific status WITHOUT any data (any data /// that was sent via CommandHandler is to be rolled back/discarded). /// /// This is because only one of the following may be encoded in a response: From 417afc50c1992f869d6bee793fa883bcde260722 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 2 Aug 2024 12:14:35 -0400 Subject: [PATCH 30/31] Fix status success and chip_no_error equivalence and add unit tests --- src/app/data-model-provider/ActionReturnStatus.cpp | 8 ++++++++ .../tests/TestActionReturnStatus.cpp | 12 ++++++++++++ 2 files changed, 20 insertions(+) diff --git a/src/app/data-model-provider/ActionReturnStatus.cpp b/src/app/data-model-provider/ActionReturnStatus.cpp index 3af05b2d184f36..7857246a0e861f 100644 --- a/src/app/data-model-provider/ActionReturnStatus.cpp +++ b/src/app/data-model-provider/ActionReturnStatus.cpp @@ -36,6 +36,14 @@ bool StatusIsTheSameAsError(const ClusterStatusCode & status, const CHIP_ERROR & auto cluster_code = status.GetClusterSpecificCode(); if (!cluster_code.HasValue()) { + // there exist Status::Success, however that may not be encoded + // as a CHIP_ERROR_IM_GLOBAL_STATUS_VALUE as it is just as well a CHIP_NO_ERROR. + // handle that separately + if ((status.GetStatus() == Status::Success) && (err == CHIP_NO_ERROR)) + { + return true; + } + return err == CHIP_ERROR_IM_GLOBAL_STATUS_VALUE(status.GetStatus()); } diff --git a/src/app/data-model-provider/tests/TestActionReturnStatus.cpp b/src/app/data-model-provider/tests/TestActionReturnStatus.cpp index 7787ab6d2af315..1c32439c5f79b6 100644 --- a/src/app/data-model-provider/tests/TestActionReturnStatus.cpp +++ b/src/app/data-model-provider/tests/TestActionReturnStatus.cpp @@ -42,6 +42,18 @@ TEST(TestActionReturnStatus, TestEquality) ASSERT_EQ(ActionReturnStatus(ClusterStatusCode::ClusterSpecificFailure(123)), CHIP_IM_CLUSTER_STATUS(123)); ASSERT_EQ(ActionReturnStatus(ClusterStatusCode::ClusterSpecificFailure(123)), ClusterStatusCode::ClusterSpecificFailure(123)); ASSERT_EQ(ActionReturnStatus(ClusterStatusCode::ClusterSpecificSuccess(123)), ClusterStatusCode::ClusterSpecificSuccess(123)); + + // Successes (without cluster-specific codes) are equivalent + ASSERT_EQ(ActionReturnStatus(Status::Success), Status::Success); + ASSERT_EQ(ActionReturnStatus(CHIP_NO_ERROR), Status::Success); + ASSERT_EQ(ActionReturnStatus(Status::Success), CHIP_NO_ERROR); + + // status specific success is has more data, so there is no equality (i.e. an action return + // with specific success codes has more data than a simple success) + ASSERT_NE(ActionReturnStatus(ClusterStatusCode::ClusterSpecificSuccess(123)), CHIP_NO_ERROR); + ASSERT_NE(ActionReturnStatus(ClusterStatusCode::ClusterSpecificSuccess(123)), Status::Success); + ASSERT_NE(ActionReturnStatus(CHIP_NO_ERROR), ClusterStatusCode::ClusterSpecificSuccess(123)); + ASSERT_NE(ActionReturnStatus(Status::Success), ClusterStatusCode::ClusterSpecificSuccess(123)); } TEST(TestActionReturnStatus, TestIsError) From f2e0812b4b758ee6d816056e7ff97576d2347e3f Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 2 Aug 2024 12:23:51 -0400 Subject: [PATCH 31/31] Added more tests --- .../tests/TestActionReturnStatus.cpp | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/app/data-model-provider/tests/TestActionReturnStatus.cpp b/src/app/data-model-provider/tests/TestActionReturnStatus.cpp index 1c32439c5f79b6..a194c0838d7d12 100644 --- a/src/app/data-model-provider/tests/TestActionReturnStatus.cpp +++ b/src/app/data-model-provider/tests/TestActionReturnStatus.cpp @@ -54,6 +54,29 @@ TEST(TestActionReturnStatus, TestEquality) ASSERT_NE(ActionReturnStatus(ClusterStatusCode::ClusterSpecificSuccess(123)), Status::Success); ASSERT_NE(ActionReturnStatus(CHIP_NO_ERROR), ClusterStatusCode::ClusterSpecificSuccess(123)); ASSERT_NE(ActionReturnStatus(Status::Success), ClusterStatusCode::ClusterSpecificSuccess(123)); + + // things that are just not equal + ASSERT_NE(ActionReturnStatus(ClusterStatusCode::ClusterSpecificSuccess(11)), ClusterStatusCode::ClusterSpecificSuccess(22)); + ASSERT_NE(ActionReturnStatus(ClusterStatusCode::ClusterSpecificFailure(11)), ClusterStatusCode::ClusterSpecificSuccess(11)); + ASSERT_NE(ActionReturnStatus(ClusterStatusCode::ClusterSpecificFailure(11)), ClusterStatusCode::ClusterSpecificFailure(22)); + ASSERT_NE(ActionReturnStatus(CHIP_NO_ERROR), CHIP_ERROR_NOT_FOUND); + ASSERT_NE(ActionReturnStatus(CHIP_ERROR_INVALID_ARGUMENT), CHIP_ERROR_NOT_FOUND); + ASSERT_NE(ActionReturnStatus(CHIP_ERROR_INVALID_ARGUMENT), CHIP_NO_ERROR); + ASSERT_NE(ActionReturnStatus(CHIP_ERROR_INVALID_ARGUMENT), Status::Success); + ASSERT_NE(ActionReturnStatus(CHIP_ERROR_INVALID_ARGUMENT), Status::UnsupportedRead); + ASSERT_NE(ActionReturnStatus(Status::Success), Status::UnsupportedRead); + ASSERT_NE(ActionReturnStatus(Status::Success), CHIP_ERROR_INVALID_ARGUMENT); + ASSERT_NE(ActionReturnStatus(CHIP_ERROR_NOT_FOUND), Status::Failure); + ASSERT_NE(ActionReturnStatus(Status::Failure), CHIP_NO_ERROR); + ASSERT_NE(ActionReturnStatus(Status::Failure), CHIP_ERROR_INVALID_ARGUMENT); + ASSERT_NE(ActionReturnStatus(Status::Failure), ClusterStatusCode::ClusterSpecificSuccess(1)); + ASSERT_NE(ActionReturnStatus(Status::Failure), ClusterStatusCode::ClusterSpecificFailure(2)); + ASSERT_NE(ActionReturnStatus(ClusterStatusCode::ClusterSpecificSuccess(1)), CHIP_NO_ERROR); + ASSERT_NE(ActionReturnStatus(ClusterStatusCode::ClusterSpecificFailure(2)), CHIP_NO_ERROR); + ASSERT_NE(ActionReturnStatus(ClusterStatusCode::ClusterSpecificSuccess(3)), Status::Failure); + ASSERT_NE(ActionReturnStatus(ClusterStatusCode::ClusterSpecificFailure(4)), Status::Failure); + ASSERT_NE(ActionReturnStatus(ClusterStatusCode::ClusterSpecificSuccess(3)), Status::Success); + ASSERT_NE(ActionReturnStatus(ClusterStatusCode::ClusterSpecificFailure(4)), Status::Success); } TEST(TestActionReturnStatus, TestIsError)