Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define an ActionReturnStatus for DataModel::Provider action returns #34708

Merged
merged 35 commits into from
Aug 2, 2024
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
af81a81
In progress
andreilitvin Jul 31, 2024
bfe9235
ActionReturnStatus implementation
andreilitvin Aug 1, 2024
224c8f9
Start making use of ActionReturnStatus
andreilitvin Aug 1, 2024
4e02d16
Things seem to compile (but not yet passing)
andreilitvin Aug 1, 2024
047321b
nice log formatting
andreilitvin Aug 1, 2024
67a676c
Propper formatting and comparisons in tests. Mock tests pass
andreilitvin Aug 1, 2024
d6b95e0
Restyle
andreilitvin Aug 1, 2024
04584fe
Merge branch 'master' into imdm/action_returns
andreilitvin Aug 1, 2024
035aa06
Restyle
andreilitvin Aug 1, 2024
37d688f
Fix typo
andreilitvin Aug 1, 2024
cdacd18
Add missing files
andreilitvin Aug 1, 2024
0ef9987
Added some unit tests that pass
andreilitvin Aug 1, 2024
54980d0
More tests
andreilitvin Aug 1, 2024
c7aa8ce
Restyle
andreilitvin Aug 1, 2024
e87b6ea
Update src/app/codegen-data-model-provider/EmberMetadata.cpp
andy31415 Aug 1, 2024
e5c1120
Update src/app/data-model-provider/ActionReturnStatus.h
andy31415 Aug 1, 2024
c82e8fe
Update src/app/data-model-provider/ActionReturnStatus.h
andy31415 Aug 1, 2024
bdc065e
Update src/app/data-model-provider/ActionReturnStatus.h
andy31415 Aug 1, 2024
739f792
Update src/lib/core/CHIPError.h
andy31415 Aug 1, 2024
5ab0190
Rename IsOutOfSpaceError and do not document specifics
andreilitvin Aug 1, 2024
f38e13e
Merge branch 'imdm/action_returns' of github.com:andy31415/connectedh…
andreilitvin Aug 1, 2024
5974a91
Document invoke return codes
andreilitvin Aug 1, 2024
1e0c949
Use the new out of space method in checked
andreilitvin Aug 1, 2024
81e6d33
Merge branch 'master' into imdm/action_returns
andreilitvin Aug 1, 2024
5bf5dfe
Restyle
andreilitvin Aug 1, 2024
20410b3
Allow ClusterStatusCode to be constructed from a CHIP_ERROR
andreilitvin Aug 1, 2024
17573d0
Format action statuses as c_str. HOWEVER this wastes 32 bytes of BSS
andreilitvin Aug 1, 2024
46155f2
Fix error formatting
andreilitvin Aug 1, 2024
a089ee2
Restyle
andreilitvin Aug 1, 2024
a19f2bf
Fix includes to be system paths
andreilitvin Aug 1, 2024
245898b
Update src/app/data-model-provider/Provider.h
andy31415 Aug 2, 2024
316f6ec
Update src/app/data-model-provider/Provider.h
andy31415 Aug 2, 2024
3303ded
Merge branch 'imdm/action_returns' of github.com:andy31415/connectedh…
andreilitvin Aug 2, 2024
417afc5
Fix status success and chip_no_error equivalence and add unit tests
andreilitvin Aug 2, 2024
f2e0812
Added more tests
andreilitvin Aug 2, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 7 additions & 4 deletions src/app/codegen-data-model-provider/CodegenDataModelProvider.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/
#pragma once

#include "app/data-model-provider/ActionReturnStatus.h"
#include <app/data-model-provider/Provider.h>

#include <app/util/af-types.h>
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
///
Expand Down Expand Up @@ -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)",
Expand Down Expand Up @@ -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<CHIP_ERROR>(&metadata))
if (const Status * status = std::get_if<Status>(&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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
///
Expand Down Expand Up @@ -266,16 +267,16 @@ 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));

// 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,
Expand All @@ -286,18 +287,19 @@ CHIP_ERROR CodegenDataModelProvider::WriteAttribute(const DataModel::WriteAttrib
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;
}
}

auto metadata = Ember::FindAttributeMetadata(request.path);

if (const CHIP_ERROR * err = std::get_if<CHIP_ERROR>(&metadata))
// Explicit failure in finding a suitable metadata
if (const Status * status = std::get_if<Status>(&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<const EmberAfAttributeMetadata *>(&metadata);
Expand All @@ -316,15 +318,15 @@ CHIP_ERROR CodegenDataModelProvider::WriteAttribute(const DataModel::WriteAttrib
// 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())
{
Expand All @@ -333,14 +335,14 @@ CHIP_ERROR CodegenDataModelProvider::WriteAttribute(const DataModel::WriteAttrib
{
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;
}
}

Expand All @@ -367,7 +369,7 @@ CHIP_ERROR CodegenDataModelProvider::WriteAttribute(const DataModel::WriteAttrib
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))
Expand All @@ -386,7 +388,7 @@ CHIP_ERROR CodegenDataModelProvider::WriteAttribute(const DataModel::WriteAttrib

if (status != Protocols::InteractionModel::Status::Success)
{
return CHIP_ERROR_IM_GLOBAL_STATUS_VALUE(status);
return status;
}

// TODO: this WILL requre updates
Expand Down
14 changes: 8 additions & 6 deletions src/app/codegen-data-model-provider/EmberMetadata.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,11 @@ namespace chip {
namespace app {
namespace Ember {

using Protocols::InteractionModel::Status;

std::variant<const EmberAfCluster *, // global attribute, data from a cluster
const EmberAfAttributeMetadata *, // a specific attribute stored by ember
CHIP_ERROR // error, this will NEVER be CHIP_NO_ERROR
Status // one of Status::Unsupported*
>
FindAttributeMetadata(const ConcreteAttributePath & aPath)
{
Expand All @@ -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;
Expand All @@ -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;
Expand Down
15 changes: 8 additions & 7 deletions src/app/codegen-data-model-provider/EmberMetadata.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

#include <app/util/af-types.h>
#include <lib/core/CHIPError.h>
#include <protocols/interaction_model/StatusCode.h>

#include <variant>

Expand All @@ -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<const EmberAfCluster *, // global attribute, data from a cluster
const EmberAfAttributeMetadata *, // a specific attribute stored by ember
CHIP_ERROR // error, (CHIP_IM_GLOBAL_STATUS(Unsupported*))
/// - Status, only specifically for unknown attributes, may only be one of:
/// - Status::UnsupportedEndpoint
/// - Status::UnsupportedCluster
/// - Status::UnsupportedAttribute
std::variant<const EmberAfCluster *, // global attribute, data from a cluster
const EmberAfAttributeMetadata *, // a specific attribute stored by ember
Protocols::InteractionModel::Status // one of Status::Unsupported*
>
FindAttributeMetadata(const ConcreteAttributePath & aPath);

Expand Down
5 changes: 4 additions & 1 deletion src/app/codegen-data-model-provider/tests/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -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",
]
}
Loading
Loading