From ba43ab71e2a74ef7e5fa68f652d2587cde75393a Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 28 Jun 2024 13:27:39 -0400 Subject: [PATCH 01/74] Initial copy/merge of the codegendatamodel::write support --- src/app/codegen-data-model/BUILD.gn | 5 +- .../codegen-data-model/CodegenDataModel.cpp | 7 - .../CodegenDataModel_Read.cpp | 54 +- .../CodegenDataModel_Write.cpp | 346 +++++++++++++ src/app/codegen-data-model/EmberMetadata.cpp | 75 +++ src/app/codegen-data-model/EmberMetadata.h | 40 ++ src/app/codegen-data-model/model.gni | 3 + .../tests/EmberReadWriteOverride.cpp | 26 +- .../tests/EmberReadWriteOverride.h | 3 + .../tests/TestCodegenModelViaMocks.cpp | 479 +++++++++++++++++- 10 files changed, 969 insertions(+), 69 deletions(-) create mode 100644 src/app/codegen-data-model/CodegenDataModel_Write.cpp create mode 100644 src/app/codegen-data-model/EmberMetadata.cpp create mode 100644 src/app/codegen-data-model/EmberMetadata.h diff --git a/src/app/codegen-data-model/BUILD.gn b/src/app/codegen-data-model/BUILD.gn index 5803f01a37778e..cc856539d001e7 100644 --- a/src/app/codegen-data-model/BUILD.gn +++ b/src/app/codegen-data-model/BUILD.gn @@ -20,8 +20,11 @@ import("//build_overrides/chip.gni") # # Use `model.gni` to get access to: # CodegenDataModel.cpp -# CodegenDataModel_Read.cpp # CodegenDataModel.h +# CodegenDataModel_Read.cpp +# CodegenDataModel_Write.cpp +# EmberMetadata.cpp +# EmberMetadata.h # # The above list of files exists to satisfy the "dependency linter" # since those files should technically be "visible to gn" even though we diff --git a/src/app/codegen-data-model/CodegenDataModel.cpp b/src/app/codegen-data-model/CodegenDataModel.cpp index d46deeeddaa1e0..8c76c9b66d8ec1 100644 --- a/src/app/codegen-data-model/CodegenDataModel.cpp +++ b/src/app/codegen-data-model/CodegenDataModel.cpp @@ -231,13 +231,6 @@ bool CodegenDataModel::EmberCommandListIterator::Exists(const CommandId * list, return (*mCurrentHint == toCheck); } -CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttributeRequest & request, - AttributeValueDecoder & decoder) -{ - // TODO: this needs an implementation - return CHIP_ERROR_NOT_IMPLEMENTED; -} - CHIP_ERROR CodegenDataModel::Invoke(const InteractionModel::InvokeRequest & request, TLV::TLVReader & input_arguments, InteractionModel::InvokeReply & reply) { diff --git a/src/app/codegen-data-model/CodegenDataModel_Read.cpp b/src/app/codegen-data-model/CodegenDataModel_Read.cpp index 04265d37f8623f..d4d89a474ca0fc 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Read.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Read.cpp @@ -14,7 +14,6 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -#include "lib/core/CHIPError.h" #include #include @@ -29,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -49,56 +49,6 @@ namespace app { namespace { using namespace chip::app::Compatibility::Internal; -// Fetch the source for the given attribute path: either a cluster (for global ones) or attribute -// path. -// -// if returning a CHIP_ERROR, it will NEVER be CHIP_NO_ERROR. -std::variant -FindAttributeMetadata(const ConcreteAttributePath & aPath) -{ - for (auto & attr : GlobalAttributesNotInMetadata) - { - - if (attr == aPath.mAttributeId) - { - 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 cluster; - } - } - const EmberAfAttributeMetadata * metadata = - emberAfLocateAttributeMetadata(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId); - - if (metadata == nullptr) - { - const EmberAfEndpointType * type = emberAfFindEndpointType(aPath.mEndpointId); - if (type == nullptr) - { - return CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint); - } - - const EmberAfCluster * cluster = emberAfFindClusterInType(type, aPath.mClusterId, CLUSTER_MASK_SERVER); - if (cluster == nullptr) - { - return CHIP_IM_GLOBAL_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 metadata; -} - /// Attempts to read via an attribute access interface (AAI) /// /// If it returns a CHIP_ERROR, then this is a FINAL result (i.e. either failure or success). @@ -320,7 +270,7 @@ CHIP_ERROR CodegenDataModel::ReadAttribute(const InteractionModel::ReadAttribute } } - auto metadata = FindAttributeMetadata(request.path); + auto metadata = Ember::FindAttributeMetadata(request.path); // Explicit failure in finding a suitable metadata if (const CHIP_ERROR * err = std::get_if(&metadata)) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp new file mode 100644 index 00000000000000..2b5d3efec99e5d --- /dev/null +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -0,0 +1,346 @@ +/* + * 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 +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +namespace chip { +namespace app { +namespace { + +using namespace chip::app::Compatibility::Internal; + +/// Attempts to write via an attribute access interface (AAI) +/// +/// If it returns a CHIP_ERROR, then this is a FINAL result (i.e. either failure or success): +/// - in particular, CHIP_ERROR_ACCESS_DENIED will be used for UnsupportedRead AII returns +/// +/// If it returns std::nullopt, then there is no AAI to handle the given path +/// and processing should figure out the value otherwise (generally from other ember data) +std::optional TryWriteViaAccessInterface(const ConcreteAttributePath & path, AttributeAccessInterface * aai, + AttributeValueDecoder & decoder) +{ + + // Processing can happen only if an attribute access interface actually exists.. + if (aai == nullptr) + { + return std::nullopt; + } + + CHIP_ERROR err = aai->Write(path, decoder); + + // explict translate UnsupportedRead to Access denied. This is to allow callers to determine a + // translation for this: usually wildcard subscriptions MAY just ignore these where as direct reads + // MUST translate them to UnsupportedAccess + ReturnErrorCodeIf(err == CHIP_IM_GLOBAL_STATUS(UnsupportedWrite), CHIP_ERROR_ACCESS_DENIED); + + if (err != CHIP_NO_ERROR) + { + return std::make_optional(err); + } + + // If the decoder tried to decode, then a value should have been read for processing. + // - if decode, assueme DONE (i.e. FINAL CHIP_NO_ERROR) + // - if no encode, say that processing must continue + return decoder.TriedDecode() ? std::make_optional(CHIP_NO_ERROR) : std::nullopt; +} +/// Metadata of what a ember/pascal short string means (prepended by a u8 length) +struct ShortPascalString +{ + using LengthType = uint8_t; + static constexpr LengthType kNullLength = 0xFF; +}; + +/// Metadata of what a ember/pascal LONG string means (prepended by a u16 length) +struct LongPascalString +{ + using LengthType = uint16_t; + static constexpr LengthType kNullLength = 0xFFFF; +}; + +// ember assumptions ... should just work +static_assert(sizeof(ShortPascalString::LengthType) == 1); +static_assert(sizeof(LongPascalString::LengthType) == 2); + +/// Encode the value stored in 'decoder' into an ember format string 'out' +/// +/// The value encoded will be of type T (e.g. CharSpan or ByteSpan) and it will be encoded +/// via the given ENCODING (i.e. ShortPascalString or LongPascalString) +/// +/// isNullable defines if the value of NULL is allowed to be encoded. +template +CHIP_ERROR DecodeStringLikeIntoEmberBuffer(AttributeValueDecoder decoder, bool isNullable, MutableByteSpan out) +{ + T workingValue; + + if (isNullable) + { + typename DataModel::Nullable nullableWorkingValue; + ReturnErrorOnFailure(decoder.Decode(nullableWorkingValue)); + + if (nullableWorkingValue.IsNull()) + { + VerifyOrReturnError(out.size() >= sizeof(typename ENCODING::LengthType), CHIP_ERROR_BUFFER_TOO_SMALL); + + typename ENCODING::LengthType nullLength = ENCODING::kNullLength; + memcpy(out.data(), &nullLength, sizeof(nullLength)); + return CHIP_NO_ERROR; + } + + // continue encoding non-null value + workingValue = nullableWorkingValue.Value(); + } + else + { + ReturnErrorOnFailure(decoder.Decode(workingValue)); + } + + typename ENCODING::LengthType len = workingValue.size(); + VerifyOrReturnError(out.size() >= sizeof(len) + len, CHIP_ERROR_BUFFER_TOO_SMALL); + + memcpy(out.data(), &len, sizeof(len)); + memcpy(out.data() + sizeof(len), workingValue.data(), workingValue.size()); + return CHIP_NO_ERROR; +} + +/// Encodes a numeric data value of type T from the given ember-encoded buffer `data`. +/// +/// isNullable defines if the value of NULL is allowed to be encoded. +template +CHIP_ERROR DecodeIntoEmberBuffer(AttributeValueDecoder & decoder, bool isNullable, MutableByteSpan out) +{ + if (isNullable) + { + using Traits = NumericAttributeTraits; + + DataModel::Nullable workingValue; + ReturnErrorOnFailure(decoder.Decode(workingValue)); + + typename Traits::StorageType storageValue; + if (workingValue.IsNull()) + { + Traits::SetNull(storageValue); + } + else + { + VerifyOrReturnError(Traits::CanRepresentValue(isNullable, *workingValue), CHIP_ERROR_INVALID_ARGUMENT); + Traits::WorkingToStorage(*workingValue, storageValue); + } + + VerifyOrReturnError(out.size() >= sizeof(storageValue), CHIP_ERROR_INVALID_ARGUMENT); + + const uint8_t * data = Traits::ToAttributeStoreRepresentation(storageValue); + memcpy(out.data(), data, sizeof(storageValue)); + } + else + { + using Traits = NumericAttributeTraits; + + typename Traits::WorkingType workingValue; + ReturnErrorOnFailure(decoder.Decode(workingValue)); + + typename Traits::StorageType storageValue; + Traits::WorkingToStorage(workingValue, storageValue); + + VerifyOrReturnError(out.size() >= sizeof(storageValue), CHIP_ERROR_INVALID_ARGUMENT); + + // This guards against trying to encode something that overlaps nullable, for example + // Nullable(0xFF) is not representable because 0xFF is the encoding of NULL in ember + VerifyOrReturnError(Traits::CanRepresentValue(isNullable, workingValue), CHIP_ERROR_INVALID_ARGUMENT); + + const uint8_t * data = Traits::ToAttributeStoreRepresentation(storageValue); + memcpy(out.data(), data, sizeof(storageValue)); + } + + return CHIP_NO_ERROR; +} + +/// Read the data from "decoder" into an ember-formatted buffer "out" +/// +/// Uses the attribute `metadata` to determine how the data is to be encoded into out. +CHIP_ERROR DecodeValueIntoEmberBuffer(AttributeValueDecoder & decoder, const EmberAfAttributeMetadata * metadata, + MutableByteSpan out) +{ + VerifyOrReturnError(metadata != nullptr, CHIP_ERROR_INVALID_ARGUMENT); + + const bool isNullable = metadata->IsNullable(); + + switch (AttributeBaseType(metadata->attributeType)) + { + case ZCL_BOOLEAN_ATTRIBUTE_TYPE: // Boolean + return DecodeIntoEmberBuffer(decoder, isNullable, out); + case ZCL_INT8U_ATTRIBUTE_TYPE: // Unsigned 8-bit integer + return DecodeIntoEmberBuffer(decoder, isNullable, out); + case ZCL_INT16U_ATTRIBUTE_TYPE: // Unsigned 16-bit integer + return DecodeIntoEmberBuffer(decoder, isNullable, out); + case ZCL_INT24U_ATTRIBUTE_TYPE: // Unsigned 24-bit integer + return DecodeIntoEmberBuffer>(decoder, isNullable, out); + case ZCL_INT32U_ATTRIBUTE_TYPE: // Unsigned 32-bit integer + return DecodeIntoEmberBuffer(decoder, isNullable, out); + case ZCL_INT40U_ATTRIBUTE_TYPE: // Unsigned 40-bit integer + return DecodeIntoEmberBuffer>(decoder, isNullable, out); + case ZCL_INT48U_ATTRIBUTE_TYPE: // Unsigned 48-bit integer + return DecodeIntoEmberBuffer>(decoder, isNullable, out); + case ZCL_INT56U_ATTRIBUTE_TYPE: // Unsigned 56-bit integer + return DecodeIntoEmberBuffer>(decoder, isNullable, out); + case ZCL_INT64U_ATTRIBUTE_TYPE: // Unsigned 64-bit integer + return DecodeIntoEmberBuffer(decoder, isNullable, out); + case ZCL_INT8S_ATTRIBUTE_TYPE: // Signed 8-bit integer + return DecodeIntoEmberBuffer(decoder, isNullable, out); + case ZCL_INT16S_ATTRIBUTE_TYPE: // Signed 16-bit integer + return DecodeIntoEmberBuffer(decoder, isNullable, out); + case ZCL_INT24S_ATTRIBUTE_TYPE: // Signed 24-bit integer + return DecodeIntoEmberBuffer>(decoder, isNullable, out); + case ZCL_INT32S_ATTRIBUTE_TYPE: // Signed 32-bit integer + return DecodeIntoEmberBuffer(decoder, isNullable, out); + case ZCL_INT40S_ATTRIBUTE_TYPE: // Signed 40-bit integer + return DecodeIntoEmberBuffer>(decoder, isNullable, out); + case ZCL_INT48S_ATTRIBUTE_TYPE: // Signed 48-bit integer + return DecodeIntoEmberBuffer>(decoder, isNullable, out); + case ZCL_INT56S_ATTRIBUTE_TYPE: // Signed 56-bit integer + return DecodeIntoEmberBuffer>(decoder, isNullable, out); + case ZCL_INT64S_ATTRIBUTE_TYPE: // Signed 64-bit integer + return DecodeIntoEmberBuffer(decoder, isNullable, out); + case ZCL_SINGLE_ATTRIBUTE_TYPE: // 32-bit float + return DecodeIntoEmberBuffer(decoder, isNullable, out); + case ZCL_DOUBLE_ATTRIBUTE_TYPE: // 64-bit float + return DecodeIntoEmberBuffer(decoder, isNullable, out); + case ZCL_CHAR_STRING_ATTRIBUTE_TYPE: // Char string + return DecodeStringLikeIntoEmberBuffer(decoder, isNullable, out); + case ZCL_LONG_CHAR_STRING_ATTRIBUTE_TYPE: + return DecodeStringLikeIntoEmberBuffer(decoder, isNullable, out); + case ZCL_OCTET_STRING_ATTRIBUTE_TYPE: // Octet string + return DecodeStringLikeIntoEmberBuffer(decoder, isNullable, out); + case ZCL_LONG_OCTET_STRING_ATTRIBUTE_TYPE: + return DecodeStringLikeIntoEmberBuffer(decoder, isNullable, out); + default: + ChipLogError(DataManagement, "Attribute type 0x%x not handled", static_cast(metadata->attributeType)); + return CHIP_IM_GLOBAL_STATUS(UnsupportedWrite); + } + return CHIP_IM_GLOBAL_STATUS(UnsupportedWrite); +} + +} // namespace + +CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttributeRequest & request, + AttributeValueDecoder & decoder) +{ + ChipLogDetail(DataManagement, + "Reading attribute: Cluster=" ChipLogFormatMEI " Endpoint=%x AttributeId=" ChipLogFormatMEI " (expanded=%d)", + ChipLogValueMEI(request.path.mClusterId), request.path.mEndpointId, ChipLogValueMEI(request.path.mAttributeId), + request.path.mExpanded); + + // ACL check for non-internal requests + if (!request.operationFlags.Has(InteractionModel::OperationFlags::kInternal)) + { + ReturnErrorCodeIf(!request.subjectDescriptor.has_value(), CHIP_ERROR_INVALID_ARGUMENT); + + Access::RequestPath requestPath{ .cluster = request.path.mClusterId, .endpoint = request.path.mEndpointId }; + ReturnErrorOnFailure(Access::GetAccessControl().Check(*request.subjectDescriptor, requestPath, + RequiredPrivilege::ForWriteAttribute(request.path))); + } + + auto metadata = Ember::FindAttributeMetadata(request.path); + + // Explicit failure in finding a suitable metadata + if (const CHIP_ERROR * err = std::get_if(&metadata)) + { + VerifyOrDie(*err != CHIP_NO_ERROR); + return *err; + } + + // All the global attributes that we do not have metadata for are + // read-only (i.e. cannot write atribute_list/event_list/accepted_cmds/generated_cmds) + // + // so if no metadata available, we wil lreturn an error + const EmberAfAttributeMetadata ** attributeMetadata = std::get_if(&metadata); + if (attributeMetadata == nullptr) + { + return CHIP_IM_GLOBAL_STATUS(UnsupportedWrite); + } + + if ((*attributeMetadata)->IsReadOnly() && !request.operationFlags.Has(InteractionModel::OperationFlags::kInternal)) + { + // Internal is allowed to try to bypass read-only updates, however otherwise we deny read-only + // updates + return CHIP_IM_GLOBAL_STATUS(UnsupportedWrite); + } + + if ((*attributeMetadata)->MustUseTimedWrite() && !request.writeFlags.Has(InteractionModel::WriteFlags::kTimed)) + { + return CHIP_IM_GLOBAL_STATUS(NeedsTimedInteraction); + } + + if (request.path.mDataVersion.HasValue()) + { + std::optional clusterInfo = GetClusterInfo(request.path); + if (!clusterInfo.has_value()) + { + ChipLogError(DataManagement, "Unable to get cluster info for Endpoint %x, Cluster " ChipLogFormatMEI, + request.path.mEndpointId, ChipLogValueMEI(request.path.mClusterId)); + return CHIP_IM_GLOBAL_STATUS(DataVersionMismatch); + } + + if (request.path.mDataVersion.Value() != clusterInfo->dataVersion) + { + ChipLogError(DataManagement, "Write Version mismatch for Endpoint %x, Cluster " ChipLogFormatMEI, + request.path.mEndpointId, ChipLogValueMEI(request.path.mClusterId)); + return CHIP_IM_GLOBAL_STATUS(DataVersionMismatch); + } + } + + std::optional aai_result = TryWriteViaAccessInterface( + request.path, GetAttributeAccessOverride(request.path.mEndpointId, request.path.mClusterId), decoder); + ReturnErrorCodeIf(aai_result.has_value(), *aai_result); + + ReturnErrorOnFailure(DecodeValueIntoEmberBuffer(decoder, *attributeMetadata, gEmberAttributeIOBufferSpan)); + + EmberAfAttributeSearchRecord record; + record.endpoint = request.path.mEndpointId; + record.clusterId = request.path.mClusterId; + record.attributeId = request.path.mAttributeId; + + Protocols::InteractionModel::Status status = + emAfReadOrWriteAttribute(&record, attributeMetadata, gEmberAttributeIOBufferSpan.data(), gEmberAttributeIOBufferSpan.size(), + /* write = */ true); + + if (status != Protocols::InteractionModel::Status::Success) + { + return ChipError(ChipError::SdkPart::kIMGlobalStatus, to_underlying(status), __FILE__, __LINE__); + } + + return CHIP_NO_ERROR; +} + +} // namespace app +} // namespace chip diff --git a/src/app/codegen-data-model/EmberMetadata.cpp b/src/app/codegen-data-model/EmberMetadata.cpp new file mode 100644 index 00000000000000..11fdef0515a94f --- /dev/null +++ b/src/app/codegen-data-model/EmberMetadata.cpp @@ -0,0 +1,75 @@ +/* + * 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 +#include +#include + +namespace chip { +namespace app { +namespace Ember { + +std::variant +FindAttributeMetadata(const ConcreteAttributePath & aPath) +{ + for (auto & attr : GlobalAttributesNotInMetadata) + { + + if (attr == aPath.mAttributeId) + { + 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 cluster; + } + } + const EmberAfAttributeMetadata * metadata = + emberAfLocateAttributeMetadata(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId); + + if (metadata == nullptr) + { + const EmberAfEndpointType * type = emberAfFindEndpointType(aPath.mEndpointId); + if (type == nullptr) + { + return CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint); + } + + const EmberAfCluster * cluster = emberAfFindClusterInType(type, aPath.mClusterId, CLUSTER_MASK_SERVER); + if (cluster == nullptr) + { + return CHIP_IM_GLOBAL_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 metadata; +} + +} // namespace Ember +} // namespace app +} // namespace chip diff --git a/src/app/codegen-data-model/EmberMetadata.h b/src/app/codegen-data-model/EmberMetadata.h new file mode 100644 index 00000000000000..848c2b9c11b566 --- /dev/null +++ b/src/app/codegen-data-model/EmberMetadata.h @@ -0,0 +1,40 @@ +/* + * 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 Ember { + +/// Fetch the source for the given attribute path: either a cluster (for global ones) or attribute +/// path. +/// +/// if returning a CHIP_ERROR, it will NEVER be CHIP_NO_ERROR. +std::variant +FindAttributeMetadata(const ConcreteAttributePath & aPath); + +} // namespace Ember +} // namespace app +} // namespace chip diff --git a/src/app/codegen-data-model/model.gni b/src/app/codegen-data-model/model.gni index 3be7b2d2610513..a2abf6377c07a4 100644 --- a/src/app/codegen-data-model/model.gni +++ b/src/app/codegen-data-model/model.gni @@ -28,6 +28,9 @@ codegen_data_model_SOURCES = [ "${chip_root}/src/app/codegen-data-model/CodegenDataModel.h", "${chip_root}/src/app/codegen-data-model/CodegenDataModel.cpp", "${chip_root}/src/app/codegen-data-model/CodegenDataModel_Read.cpp", + "${chip_root}/src/app/codegen-data-model/CodegenDataModel_Write.cpp", + "${chip_root}/src/app/codegen-data-model/EmberMetadata.cpp", + "${chip_root}/src/app/codegen-data-model/EmberMetadata.h", ] codegen_data_model_PUBLIC_DEPS = [ diff --git a/src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp b/src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp index 8c65ee29b05556..8d7f98b3997767 100644 --- a/src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp +++ b/src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp @@ -17,6 +17,7 @@ #include "EmberReadWriteOverride.h" #include +#include using chip::Protocols::InteractionModel::Status; @@ -63,6 +64,11 @@ void SetEmberReadOutput(std::variant what) gEmberStatusCode = Status::InvalidAction; } +ByteSpan GetEmberBuffer() +{ + return ByteSpan(gEmberIoBuffer, gEmberIoBufferFill); +} + } // namespace Test } // namespace chip @@ -76,12 +82,24 @@ Status emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord, return gEmberStatusCode; } - if (gEmberIoBufferFill > readLength) + if (write) + { + // copy over as much data as possible + // NOTE: we do NOT use (*metadata)->size since it is unclear if our mocks set that correctly + size_t len = std::min(sizeof(gEmberIoBuffer), readLength); + memcpy(gEmberIoBuffer, buffer, len); + gEmberIoBufferFill = len; + } + else { - ChipLogError(Test, "Internal TEST error: insufficient output buffer space."); - return Status::ResourceExhausted; + if (gEmberIoBufferFill > readLength) + { + ChipLogError(Test, "Internal TEST error: insufficient output buffer space."); + return Status::ResourceExhausted; + } + + memcpy(buffer, gEmberIoBuffer, gEmberIoBufferFill); } - memcpy(buffer, gEmberIoBuffer, gEmberIoBufferFill); return Status::Success; } diff --git a/src/app/codegen-data-model/tests/EmberReadWriteOverride.h b/src/app/codegen-data-model/tests/EmberReadWriteOverride.h index 527a6cfd0d18c7..5aeaeadc254086 100644 --- a/src/app/codegen-data-model/tests/EmberReadWriteOverride.h +++ b/src/app/codegen-data-model/tests/EmberReadWriteOverride.h @@ -29,5 +29,8 @@ namespace Test { /// It may return a value with success or some error. The byte span WILL BE COPIED. void SetEmberReadOutput(std::variant what); +/// Grab the data currently in the buffer +chip::ByteSpan GetEmberBuffer(); + } // namespace Test } // namespace chip diff --git a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp index 11264bbd33a36e..6320844b43bde8 100644 --- a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp +++ b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp @@ -19,29 +19,39 @@ #include #include +#include #include #include -#include #include #include #include #include +#include +#include #include -#include -#include #include #include +#include +#include +#include +#include +#include #include +#include #include #include #include #include #include #include +#include #include #include #include +#include +#include #include +#include #include #include @@ -57,6 +67,9 @@ namespace { constexpr FabricIndex kTestFabrixIndex = kMinValidFabricIndex; constexpr NodeId kTestNodeId = 0xFFFF'1234'ABCD'4321; +constexpr AttributeId kAttributeIdReadOnly = 0x3001; +constexpr AttributeId kAttributeIdTimedWrite = 0x3002; + constexpr EndpointId kEndpointIdThatIsMissing = kMockEndpointMin - 1; static_assert(kEndpointIdThatIsMissing != kInvalidEndpointId); @@ -341,6 +354,10 @@ const MockNodeConfig gTestNodeConfig({ MOCK_ATTRIBUTE_CONFIG_NULLABLE(ZCL_IPV6ADR_ATTRIBUTE_TYPE), MOCK_ATTRIBUTE_CONFIG_NULLABLE(ZCL_IPV6PRE_ATTRIBUTE_TYPE), MOCK_ATTRIBUTE_CONFIG_NULLABLE(ZCL_HWADR_ATTRIBUTE_TYPE), + + // Special case handling + MockAttributeConfig(kAttributeIdReadOnly, ZCL_INT32S_ATTRIBUTE_TYPE, 0), + MockAttributeConfig(kAttributeIdTimedWrite, ZCL_INT32S_ATTRIBUTE_TYPE, ATTRIBUTE_MASK_WRITABLE | ATTRIBUTE_MASK_MUST_USE_TIMED_WRITE), }), }), }); @@ -415,8 +432,19 @@ class StructAttributeAccessInterface : public AttributeAccessInterface return encoder.Encode(mData); } + CHIP_ERROR Write(const ConcreteDataAttributePath & path, AttributeValueDecoder & decoder) override + { + if (static_cast(path) != mPath) + { + // returning without trying to handle means "I do not handle this" + return CHIP_NO_ERROR; + } + + return decoder.Decode(mData); + } + void SetReturnedData(const Clusters::UnitTesting::Structs::SimpleStruct::Type & data) { mData = data; } - Clusters::UnitTesting::Structs::SimpleStruct::Type simpleStruct; + const Clusters::UnitTesting::Structs::SimpleStruct::Type & GetData() const { return mData; } private: ConcreteAttributePath mPath; @@ -451,7 +479,6 @@ class ListAttributeAcessInterface : public AttributeAccessInterface void SetReturnedData(const Clusters::UnitTesting::Structs::SimpleStruct::Type & data) { mData = data; } void SetReturnedDataCount(unsigned count) { mCount = count; } - Clusters::UnitTesting::Structs::SimpleStruct::Type simpleStruct; private: ConcreteAttributePath mPath; @@ -531,6 +558,55 @@ struct TestReadRequest CHIP_ERROR FinishEncoding() { return encodedIBs.FinishEncoding(reportBuilder); } }; +// Sets up data for writing +struct TestWriteRequest +{ + InteractionModel::WriteAttributeRequest request; + uint8_t tlvBuffer[128] = { 0 }; + TLV::TLVReader + tlvReader; /// tlv reader used for the returned AttributeValueDecoder (since attributeValueDecoder uses references) + + TestWriteRequest(const Access::SubjectDescriptor & subject, const ConcreteDataAttributePath & path) + { + request.subjectDescriptor = subject; + request.path = path; + } + + template + TLV::TLVReader ReadEncodedValue(const T & value) + { + TLV::TLVWriter writer; + writer.Init(tlvBuffer); + + // Encoding is within a structure: + // - BEGIN_STRUCT + // - 1: ..... + // - END_STRUCT + TLV::TLVType outerContainerType; + VerifyOrDie(writer.StartContainer(TLV::AnonymousTag(), TLV::kTLVType_Structure, outerContainerType) == CHIP_NO_ERROR); + VerifyOrDie(DataModel::Encode(writer, TLV::ContextTag(1), value) == CHIP_NO_ERROR); + VerifyOrDie(writer.EndContainer(outerContainerType) == CHIP_NO_ERROR); + VerifyOrDie(writer.Finalize() == CHIP_NO_ERROR); + + TLV::TLVReader reader; + reader.Init(tlvBuffer); + + // position the reader inside the buffer, on the encoded value + VerifyOrDie(reader.Next() == CHIP_NO_ERROR); + VerifyOrDie(reader.EnterContainer(outerContainerType) == CHIP_NO_ERROR); + VerifyOrDie(reader.Next() == CHIP_NO_ERROR); + + return reader; + } + + template + AttributeValueDecoder DecoderFor(const T & value) + { + tlvReader = ReadEncodedValue(value); + return AttributeValueDecoder(tlvReader, request.subjectDescriptor.value_or(kDenySubjectDescriptor)); + } +}; + template void TestEmberScalarTypeRead(typename NumericAttributeTraits::WorkingType value) { @@ -599,6 +675,61 @@ void TestEmberScalarNullRead() ASSERT_TRUE(actual.IsNull()); } +template +void TestEmberScalarTypeWrite(const typename NumericAttributeTraits::WorkingType value) +{ + UseMockNodeConfig config(gTestNodeConfig); + chip::app::CodegenDataModel model; + ScopedMockAccessControl accessControl; + + TestWriteRequest test( + kAdminSubjectDescriptor, + ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), MOCK_ATTRIBUTE_ID_FOR_NON_NULLABLE_TYPE(ZclType))); + AttributeValueDecoder decoder = test.DecoderFor(value); + + // write should succeed + ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); + + // Validate data after write + chip::ByteSpan writtenData = Test::GetEmberBuffer(); + + typename NumericAttributeTraits::StorageType storage; + ASSERT_GE(writtenData.size(), sizeof(storage)); + memcpy(&storage, writtenData.data(), sizeof(storage)); + typename NumericAttributeTraits::WorkingType actual = NumericAttributeTraits::StorageToWorking(storage); + + ASSERT_EQ(actual, value); +} + +template +void TestEmberScalarNullWrite() +{ + UseMockNodeConfig config(gTestNodeConfig); + chip::app::CodegenDataModel model; + ScopedMockAccessControl accessControl; + + TestWriteRequest test(kAdminSubjectDescriptor, + ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), MOCK_ATTRIBUTE_ID_FOR_NULLABLE_TYPE(ZclType))); + + using NumericType = NumericAttributeTraits; + using NullableType = DataModel::Nullable; + AttributeValueDecoder decoder = test.DecoderFor(NullableType()); + + // write should succeed + ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); + + // Validate data after write + chip::ByteSpan writtenData = Test::GetEmberBuffer(); + + using Traits = NumericAttributeTraits; + + typename Traits::StorageType storage; + ASSERT_GE(writtenData.size(), sizeof(storage)); + memcpy(&storage, writtenData.data(), sizeof(storage)); + ASSERT_TRUE(Traits::IsNullValue(storage)); +} + + } // namespace TEST(TestCodegenModelViaMocks, IterateOverEndpoints) @@ -1207,6 +1338,9 @@ TEST(TestCodegenModelViaMocks, EmberAttributeReadErrorReading) std::unique_ptr encoder = testRequest.StartEncoding(&model); ASSERT_EQ(model.ReadAttribute(testRequest.request, *encoder), CHIP_IM_GLOBAL_STATUS(Busy)); } + + // reset things to success to not affect other tests + chip::Test::SetEmberReadOutput(ByteSpan()); } TEST(TestCodegenModelViaMocks, EmberAttributeReadNullOctetString) @@ -1667,3 +1801,338 @@ TEST(TestCodegenModelViaMocks, ReadGlobalAttributeAttributeList) EXPECT_EQ(items[i], expected[i]); } } + +TEST(TestCodegenModelViaMocks, EmberAttributeWriteAclDeny) +{ + UseMockNodeConfig config(gTestNodeConfig); + chip::app::CodegenDataModel model; + ScopedMockAccessControl accessControl; + + TestWriteRequest test(kDenySubjectDescriptor, ConcreteDataAttributePath(kMockEndpoint1, MockClusterId(1), MockAttributeId(10))); + AttributeValueDecoder decoder = test.DecoderFor(1234); + + ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_ERROR_ACCESS_DENIED); +} + +TEST(TestCodegenModelViaMocks, EmberAttributeWriteBasicTypes) +{ + TestEmberScalarTypeWrite(0x12); + TestEmberScalarTypeWrite(0x1234); + TestEmberScalarTypeWrite, ZCL_INT24U_ATTRIBUTE_TYPE>(0x1234AB); + TestEmberScalarTypeWrite(0x1234ABCD); + TestEmberScalarTypeWrite, ZCL_INT40U_ATTRIBUTE_TYPE>(0x1122334455); + TestEmberScalarTypeWrite, ZCL_INT48U_ATTRIBUTE_TYPE>(0xAABB11223344); + TestEmberScalarTypeWrite, ZCL_INT56U_ATTRIBUTE_TYPE>(0xAABB11223344); + TestEmberScalarTypeWrite(0x1122334455667788L); + + TestEmberScalarTypeWrite(-10); + TestEmberScalarTypeWrite(-123); + TestEmberScalarTypeWrite, ZCL_INT24S_ATTRIBUTE_TYPE>(-1234); + TestEmberScalarTypeWrite(-12345); + TestEmberScalarTypeWrite, ZCL_INT40S_ATTRIBUTE_TYPE>(-123456); + TestEmberScalarTypeWrite, ZCL_INT48S_ATTRIBUTE_TYPE>(-1234567); + TestEmberScalarTypeWrite, ZCL_INT56S_ATTRIBUTE_TYPE>(-12345678); + TestEmberScalarTypeWrite(-123456789); + + TestEmberScalarTypeWrite(true); + TestEmberScalarTypeWrite(false); + TestEmberScalarTypeWrite(0.625); + TestEmberScalarTypeWrite(0.625); +} + +TEST(TestCodegenModelViaMocks, EmberAttributeWriteNulls) +{ + TestEmberScalarNullWrite(); + TestEmberScalarNullWrite(); + TestEmberScalarNullWrite, ZCL_INT24U_ATTRIBUTE_TYPE>(); + TestEmberScalarNullWrite(); + TestEmberScalarNullWrite, ZCL_INT40U_ATTRIBUTE_TYPE>(); + TestEmberScalarNullWrite, ZCL_INT48U_ATTRIBUTE_TYPE>(); + TestEmberScalarNullWrite, ZCL_INT56U_ATTRIBUTE_TYPE>(); + TestEmberScalarNullWrite(); + + TestEmberScalarNullWrite(); + TestEmberScalarNullWrite(); + TestEmberScalarNullWrite, ZCL_INT24S_ATTRIBUTE_TYPE>(); + TestEmberScalarNullWrite(); + TestEmberScalarNullWrite, ZCL_INT40S_ATTRIBUTE_TYPE>(); + TestEmberScalarNullWrite, ZCL_INT48S_ATTRIBUTE_TYPE>(); + TestEmberScalarNullWrite, ZCL_INT56S_ATTRIBUTE_TYPE>(); + TestEmberScalarNullWrite(); + TestEmberScalarNullWrite(); + TestEmberScalarNullWrite(); + TestEmberScalarNullWrite(); +} + +TEST(TestCodegenModelViaMocks, EmberAttributeWriteShortString) +{ + UseMockNodeConfig config(gTestNodeConfig); + chip::app::CodegenDataModel model; + ScopedMockAccessControl accessControl; + + TestWriteRequest test(kAdminSubjectDescriptor, + ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), + MOCK_ATTRIBUTE_ID_FOR_NON_NULLABLE_TYPE(ZCL_CHAR_STRING_ATTRIBUTE_TYPE))); + AttributeValueDecoder decoder = test.DecoderFor("hello world"_span); + + ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); + chip::ByteSpan writtenData = GetEmberBuffer(); + chip::CharSpan asCharSpan(reinterpret_cast(writtenData.data()), writtenData[0] + 1); + ASSERT_TRUE(asCharSpan.data_equal("\x0Bhello world"_span)); +} + +TEST(TestCodegenModelViaMocks, EmberAttributeWriteLongString) +{ + UseMockNodeConfig config(gTestNodeConfig); + chip::app::CodegenDataModel model; + ScopedMockAccessControl accessControl; + + TestWriteRequest test(kAdminSubjectDescriptor, + ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), + MOCK_ATTRIBUTE_ID_FOR_NON_NULLABLE_TYPE(ZCL_LONG_CHAR_STRING_ATTRIBUTE_TYPE))); + AttributeValueDecoder decoder = test.DecoderFor("text"_span); + + ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); + chip::ByteSpan writtenData = GetEmberBuffer(); + + uint16_t len; + memcpy(&len, writtenData.data(), 2); + EXPECT_EQ(len, 4); + chip::CharSpan asCharSpan(reinterpret_cast(writtenData.data() + 2), 4); + + ASSERT_TRUE(asCharSpan.data_equal("text"_span)); +} + +TEST(TestCodegenModelViaMocks, EmberAttributeWriteNullableLongStringValue) +{ + UseMockNodeConfig config(gTestNodeConfig); + chip::app::CodegenDataModel model; + ScopedMockAccessControl accessControl; + + TestWriteRequest test(kAdminSubjectDescriptor, + ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), + MOCK_ATTRIBUTE_ID_FOR_NULLABLE_TYPE(ZCL_LONG_CHAR_STRING_ATTRIBUTE_TYPE))); + AttributeValueDecoder decoder = test.DecoderFor>(DataModel::MakeNullable("text"_span)); + + ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); + chip::ByteSpan writtenData = GetEmberBuffer(); + + uint16_t len; + memcpy(&len, writtenData.data(), 2); + EXPECT_EQ(len, 4); + chip::CharSpan asCharSpan(reinterpret_cast(writtenData.data() + 2), 4); + + ASSERT_TRUE(asCharSpan.data_equal("text"_span)); +} + +TEST(TestCodegenModelViaMocks, EmberAttributeWriteLongNullableStringNull) +{ + UseMockNodeConfig config(gTestNodeConfig); + chip::app::CodegenDataModel model; + ScopedMockAccessControl accessControl; + + TestWriteRequest test(kAdminSubjectDescriptor, + ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), + MOCK_ATTRIBUTE_ID_FOR_NULLABLE_TYPE(ZCL_LONG_CHAR_STRING_ATTRIBUTE_TYPE))); + AttributeValueDecoder decoder = test.DecoderFor>(DataModel::Nullable()); + + ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); + chip::ByteSpan writtenData = GetEmberBuffer(); + ASSERT_EQ(writtenData[0], 0xFF); + ASSERT_EQ(writtenData[0], 0xFF); +} + +TEST(TestCodegenModelViaMocks, EmberAttributeWriteShortBytes) +{ + UseMockNodeConfig config(gTestNodeConfig); + chip::app::CodegenDataModel model; + ScopedMockAccessControl accessControl; + + TestWriteRequest test(kAdminSubjectDescriptor, + ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), + MOCK_ATTRIBUTE_ID_FOR_NON_NULLABLE_TYPE(ZCL_OCTET_STRING_ATTRIBUTE_TYPE))); + uint8_t buffer[] = { 11, 12, 13 }; + + AttributeValueDecoder decoder = test.DecoderFor(ByteSpan(buffer)); + + ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); + chip::ByteSpan writtenData = GetEmberBuffer(); + + EXPECT_EQ(writtenData[0], 3u); + EXPECT_EQ(writtenData[1], 11u); + EXPECT_EQ(writtenData[2], 12u); + EXPECT_EQ(writtenData[3], 13u); +} + +TEST(TestCodegenModelViaMocks, EmberAttributeWriteLongBytes) +{ + UseMockNodeConfig config(gTestNodeConfig); + chip::app::CodegenDataModel model; + ScopedMockAccessControl accessControl; + + TestWriteRequest test(kAdminSubjectDescriptor, + ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), + MOCK_ATTRIBUTE_ID_FOR_NON_NULLABLE_TYPE(ZCL_LONG_OCTET_STRING_ATTRIBUTE_TYPE))); + uint8_t buffer[] = { 11, 12, 13 }; + + AttributeValueDecoder decoder = test.DecoderFor(ByteSpan(buffer)); + + ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); + chip::ByteSpan writtenData = GetEmberBuffer(); + + uint16_t len; + memcpy(&len, writtenData.data(), 2); + EXPECT_EQ(len, 3); + + EXPECT_EQ(writtenData[2], 11u); + EXPECT_EQ(writtenData[3], 12u); + EXPECT_EQ(writtenData[4], 13u); +} + +TEST(TestCodegenModelViaMocks, EmberAttributeWriteTimedWrite) +{ + UseMockNodeConfig config(gTestNodeConfig); + chip::app::CodegenDataModel model; + ScopedMockAccessControl accessControl; + + 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)); + + // writing as timed should be fine + test.request.writeFlags.Set(WriteFlags::kTimed); + ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); +} + +TEST(TestCodegenModelViaMocks, EmberAttributeWriteReadOnlyAttribute) +{ + UseMockNodeConfig config(gTestNodeConfig); + chip::app::CodegenDataModel model; + ScopedMockAccessControl accessControl; + + 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)); + + // Internal writes bypass the read only requirement + test.request.operationFlags.Set(OperationFlags::kInternal); + ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); +} + +TEST(TestCodegenModelViaMocks, EmberAttributeWriteDataVersion) +{ + UseMockNodeConfig config(gTestNodeConfig); + chip::app::CodegenDataModel model; + ScopedMockAccessControl accessControl; + + TestWriteRequest test(kAdminSubjectDescriptor, + ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), + MOCK_ATTRIBUTE_ID_FOR_NON_NULLABLE_TYPE(ZCL_INT32S_ATTRIBUTE_TYPE))); + + // Initialize to some version + ResetVersion(); + BumpVersion(); + test.request.path.mDataVersion = MakeOptional(GetVersion()); + + // Make version invalid + BumpVersion(); + + AttributeValueDecoder decoder = test.DecoderFor(1234); + + ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_IM_GLOBAL_STATUS(DataVersionMismatch)); + + // Write passes if we set the right version for the data + test.request.path.mDataVersion = MakeOptional(GetVersion()); + ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); +} + +TEST(TestCodegenModelViaMocks, WriteToInvalidPath) +{ + UseMockNodeConfig config(gTestNodeConfig); + chip::app::CodegenDataModel model; + ScopedMockAccessControl accessControl; + + { + 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)); + } + { + 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)); + } + + { + 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)); + } +} + +TEST(TestCodegenModelViaMocks, WriteToGlobalAttribute) +{ + UseMockNodeConfig config(gTestNodeConfig); + chip::app::CodegenDataModel model; + ScopedMockAccessControl accessControl; + + 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)); +} + +TEST(TestCodegenModelViaMocks, EmberWriteFailure) +{ + UseMockNodeConfig config(gTestNodeConfig); + chip::app::CodegenDataModel model; + ScopedMockAccessControl accessControl; + + TestWriteRequest test(kAdminSubjectDescriptor, + ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), + MOCK_ATTRIBUTE_ID_FOR_NON_NULLABLE_TYPE(ZCL_INT32S_ATTRIBUTE_TYPE))); + + { + AttributeValueDecoder decoder = test.DecoderFor(1234); + chip::Test::SetEmberReadOutput(Protocols::InteractionModel::Status::Failure); + ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_IM_GLOBAL_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)); + } + // reset things to success to not affect other tests + chip::Test::SetEmberReadOutput(ByteSpan()); +} + +TEST(TestCodegenModelViaMocks, EmberWriteAttributeAccessInterfaceTest) +{ + UseMockNodeConfig config(gTestNodeConfig); + chip::app::CodegenDataModel model; + ScopedMockAccessControl accessControl; + + const ConcreteAttributePath kStructPath(kMockEndpoint3, MockClusterId(4), + MOCK_ATTRIBUTE_ID_FOR_NON_NULLABLE_TYPE(ZCL_STRUCT_ATTRIBUTE_TYPE)); + RegisteredAttributeAccessInterface aai(kStructPath); + + TestWriteRequest test(kAdminSubjectDescriptor, kStructPath); + Clusters::UnitTesting::Structs::SimpleStruct::Type testValue{ + .a = 112, + .b = true, + .e = "aai_write_test"_span, + .g = 0.5, + .h = 0.125, + }; + + AttributeValueDecoder decoder = test.DecoderFor(testValue); + ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); + + EXPECT_EQ(aai->GetData().a, 112); + EXPECT_TRUE(aai->GetData().e.data_equal("aai_write_test"_span)); + + // AAI does not prevent read/write of regular attributes + TestEmberScalarTypeWrite(1234); + TestEmberScalarNullWrite(); +} From c13f3ce148c989f77fdc89a6843b0bb8d61c36b5 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 28 Jun 2024 13:27:54 -0400 Subject: [PATCH 02/74] Restyle --- .../tests/TestCodegenModelViaMocks.cpp | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp index 6320844b43bde8..adce5b3ce75983 100644 --- a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp +++ b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp @@ -19,22 +19,21 @@ #include #include -#include #include #include +#include +#include #include #include #include #include -#include -#include #include +#include +#include +#include #include #include #include -#include -#include -#include #include #include #include @@ -729,7 +728,6 @@ void TestEmberScalarNullWrite() ASSERT_TRUE(Traits::IsNullValue(storage)); } - } // namespace TEST(TestCodegenModelViaMocks, IterateOverEndpoints) From 79f9603500e9567bc8c0476154bae212075b0384 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 28 Jun 2024 13:43:05 -0400 Subject: [PATCH 03/74] Remove the error translation for ACL checks for attribute writes --- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 6 +----- src/app/data-model-interface/DataModel.h | 8 +++++--- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index 2b5d3efec99e5d..8890ecadb9ccab 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -59,11 +59,6 @@ std::optional TryWriteViaAccessInterface(const ConcreteAttributePath CHIP_ERROR err = aai->Write(path, decoder); - // explict translate UnsupportedRead to Access denied. This is to allow callers to determine a - // translation for this: usually wildcard subscriptions MAY just ignore these where as direct reads - // MUST translate them to UnsupportedAccess - ReturnErrorCodeIf(err == CHIP_IM_GLOBAL_STATUS(UnsupportedWrite), CHIP_ERROR_ACCESS_DENIED); - if (err != CHIP_NO_ERROR) { return std::make_optional(err); @@ -74,6 +69,7 @@ std::optional TryWriteViaAccessInterface(const ConcreteAttributePath // - if no encode, say that processing must continue return decoder.TriedDecode() ? std::make_optional(CHIP_NO_ERROR) : std::nullopt; } + /// Metadata of what a ember/pascal short string means (prepended by a u8 length) struct ShortPascalString { diff --git a/src/app/data-model-interface/DataModel.h b/src/app/data-model-interface/DataModel.h index 04911fd75cccc3..0f4ce2121c7868 100644 --- a/src/app/data-model-interface/DataModel.h +++ b/src/app/data-model-interface/DataModel.h @@ -58,7 +58,7 @@ class DataModel : public DataModelMetadataTree /// TEMPORARY/TRANSITIONAL requirement for transitioning from ember-specific code /// ReadAttribute is REQUIRED to perform: /// - ACL validation (see notes on OperationFlags::kInternal) - /// - Validation of readability/writability + /// - Validation of readability/writability (also controlled by OperationFlags::kInternal) /// - use request.path.mExpanded to skip encoding replies for data according /// to 8.4.3.2 of the spec: /// > If the path indicates attribute data that is not readable, then the path SHALL @@ -84,8 +84,10 @@ class DataModel : public DataModelMetadataTree /// When this is invoked, caller is expected to have already done some validations: /// - cluster `data version` has been checked for the incoming request if applicable /// - /// When `request.writeFlags.Has(WriteFlags::kForceInternal)` the request is from an internal app update - /// and SHOULD bypass some internal checks (like timed enforcement, potentially read-only restrictions) + /// TEMPORARY/TRANSITIONAL requirement for transitioning from ember-specific code + /// WriteAttribute is REQUIRED to perform: + /// - ACL validation (see notes on OperationFlags::kInternal) + /// - Validation of readability/writability (also controlled by OperationFlags::kInternal) /// /// Return codes /// CHIP_IM_GLOBAL_STATUS(code): From 36488204156ccb10e1669b37d4d1d092ec3cfc3e Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 28 Jun 2024 13:49:04 -0400 Subject: [PATCH 04/74] Comment correction after special access error code guarantees were removed --- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index 8890ecadb9ccab..a6455b6e3642a5 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -42,8 +42,7 @@ using namespace chip::app::Compatibility::Internal; /// Attempts to write via an attribute access interface (AAI) /// -/// If it returns a CHIP_ERROR, then this is a FINAL result (i.e. either failure or success): -/// - in particular, CHIP_ERROR_ACCESS_DENIED will be used for UnsupportedRead AII returns +/// If it returns a CHIP_ERROR, then this is a FINAL result (i.e. either failure or success) /// /// If it returns std::nullopt, then there is no AAI to handle the given path /// and processing should figure out the value otherwise (generally from other ember data) From f2911ef854a9393924e7b05f7d7a881b079f0644 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 28 Jun 2024 14:05:04 -0400 Subject: [PATCH 05/74] Set the namespace for DataModel to resolve nameclash for android builds --- .../codegen-data-model/tests/TestCodegenModelViaMocks.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp index adce5b3ce75983..615671ee4c3978 100644 --- a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp +++ b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp @@ -583,7 +583,7 @@ struct TestWriteRequest // - END_STRUCT TLV::TLVType outerContainerType; VerifyOrDie(writer.StartContainer(TLV::AnonymousTag(), TLV::kTLVType_Structure, outerContainerType) == CHIP_NO_ERROR); - VerifyOrDie(DataModel::Encode(writer, TLV::ContextTag(1), value) == CHIP_NO_ERROR); + VerifyOrDie(chip::app::DataModel::Encode(writer, TLV::ContextTag(1), value) == CHIP_NO_ERROR); VerifyOrDie(writer.EndContainer(outerContainerType) == CHIP_NO_ERROR); VerifyOrDie(writer.Finalize() == CHIP_NO_ERROR); @@ -711,7 +711,7 @@ void TestEmberScalarNullWrite() ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), MOCK_ATTRIBUTE_ID_FOR_NULLABLE_TYPE(ZclType))); using NumericType = NumericAttributeTraits; - using NullableType = DataModel::Nullable; + using NullableType = chip::app::DataModel::Nullable; AttributeValueDecoder decoder = test.DecoderFor(NullableType()); // write should succeed @@ -1910,7 +1910,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteNullableLongStringValue) TestWriteRequest test(kAdminSubjectDescriptor, ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), MOCK_ATTRIBUTE_ID_FOR_NULLABLE_TYPE(ZCL_LONG_CHAR_STRING_ATTRIBUTE_TYPE))); - AttributeValueDecoder decoder = test.DecoderFor>(DataModel::MakeNullable("text"_span)); + AttributeValueDecoder decoder = test.DecoderFor>(chip::app::DataModel::MakeNullable("text"_span)); ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); chip::ByteSpan writtenData = GetEmberBuffer(); @@ -1932,7 +1932,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteLongNullableStringNull) TestWriteRequest test(kAdminSubjectDescriptor, ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), MOCK_ATTRIBUTE_ID_FOR_NULLABLE_TYPE(ZCL_LONG_CHAR_STRING_ATTRIBUTE_TYPE))); - AttributeValueDecoder decoder = test.DecoderFor>(DataModel::Nullable()); + AttributeValueDecoder decoder = test.DecoderFor>(chip::app::DataModel::Nullable()); ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); chip::ByteSpan writtenData = GetEmberBuffer(); From fedfc79640d877643790233a300f789a446464dc Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 28 Jun 2024 14:05:19 -0400 Subject: [PATCH 06/74] Restyle --- .../codegen-data-model/tests/TestCodegenModelViaMocks.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp index 615671ee4c3978..5ac54b60799198 100644 --- a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp +++ b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp @@ -1910,7 +1910,8 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteNullableLongStringValue) TestWriteRequest test(kAdminSubjectDescriptor, ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), MOCK_ATTRIBUTE_ID_FOR_NULLABLE_TYPE(ZCL_LONG_CHAR_STRING_ATTRIBUTE_TYPE))); - AttributeValueDecoder decoder = test.DecoderFor>(chip::app::DataModel::MakeNullable("text"_span)); + AttributeValueDecoder decoder = + test.DecoderFor>(chip::app::DataModel::MakeNullable("text"_span)); ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); chip::ByteSpan writtenData = GetEmberBuffer(); @@ -1932,7 +1933,8 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteLongNullableStringNull) TestWriteRequest test(kAdminSubjectDescriptor, ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), MOCK_ATTRIBUTE_ID_FOR_NULLABLE_TYPE(ZCL_LONG_CHAR_STRING_ATTRIBUTE_TYPE))); - AttributeValueDecoder decoder = test.DecoderFor>(chip::app::DataModel::Nullable()); + AttributeValueDecoder decoder = + test.DecoderFor>(chip::app::DataModel::Nullable()); ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); chip::ByteSpan writtenData = GetEmberBuffer(); From 2a632f3162239e99b956c4885c0e33bd0cd58849 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 28 Jun 2024 16:33:53 -0400 Subject: [PATCH 07/74] Some changes to make darwin builds happy --- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index a6455b6e3642a5..0078b6b6fec005 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -120,7 +120,7 @@ CHIP_ERROR DecodeStringLikeIntoEmberBuffer(AttributeValueDecoder decoder, bool i ReturnErrorOnFailure(decoder.Decode(workingValue)); } - typename ENCODING::LengthType len = workingValue.size(); + typename ENCODING::LengthType len = static_cast(workingValue.size()); VerifyOrReturnError(out.size() >= sizeof(len) + len, CHIP_ERROR_BUFFER_TOO_SMALL); memcpy(out.data(), &len, sizeof(len)); @@ -325,9 +325,9 @@ CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttribu record.clusterId = request.path.mClusterId; record.attributeId = request.path.mAttributeId; - Protocols::InteractionModel::Status status = - emAfReadOrWriteAttribute(&record, attributeMetadata, gEmberAttributeIOBufferSpan.data(), gEmberAttributeIOBufferSpan.size(), - /* write = */ true); + Protocols::InteractionModel::Status status = emAfReadOrWriteAttribute( + &record, attributeMetadata, gEmberAttributeIOBufferSpan.data(), static_cast(gEmberAttributeIOBufferSpan.size()), + /* write = */ true); if (status != Protocols::InteractionModel::Status::Success) { From 97490f90878ca4f4948e262fb15d5cf90147b13b Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 2 Jul 2024 11:49:15 -0400 Subject: [PATCH 08/74] Do not clang-tidy on CodegenDataModel_Write --- .github/workflows/build.yaml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index c1073d90678ed2..dc152c1a714f19 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -204,11 +204,13 @@ jobs: run: | ./scripts/run_in_build_env.sh "./scripts/run_codegen_targets.sh out/sanitizers" - name: Clang-tidy validation + # NOTE: clang-tidy crashes on CodegenDataModel_Write due to Nullable/std::optional check. + # See https://github.com/llvm/llvm-project/issues/97426 run: | ./scripts/run_in_build_env.sh \ "./scripts/run-clang-tidy-on-compile-commands.py \ --compile-database out/sanitizers/compile_commands.json \ - --file-exclude-regex '/(repo|zzz_generated|lwip/standalone)/|-ReadImpl|-InvokeSubscribeImpl' \ + --file-exclude-regex '/(repo|zzz_generated|lwip/standalone)/|-ReadImpl|-InvokeSubscribeImpl|CodegenDataModel_Write' \ check \ " - name: Clean output @@ -422,10 +424,13 @@ jobs: run: | ./scripts/run_in_build_env.sh "./scripts/run_codegen_targets.sh out/default" - name: Clang-tidy validation + # NOTE: clang-tidy crashes on CodegenDataModel_Write due to Nullable/std::optional check. + # See https://github.com/llvm/llvm-project/issues/97426 run: | ./scripts/run_in_build_env.sh \ "./scripts/run-clang-tidy-on-compile-commands.py \ --compile-database out/default/compile_commands.json \ + --file-exclude-regex '/(repo|zzz_generated|lwip/standalone)/|CodegenDataModel_Write' \ check \ " - name: Uploading diagnostic logs From a0bdfd2d28adb7cc979b6711b45e6a7f3d62cf97 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 09:44:17 -0400 Subject: [PATCH 09/74] Update src/app/codegen-data-model/CodegenDataModel_Write.cpp Co-authored-by: Tennessee Carmel-Veilleux --- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index 0078b6b6fec005..81f66860815a7c 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -49,7 +49,6 @@ using namespace chip::app::Compatibility::Internal; std::optional TryWriteViaAccessInterface(const ConcreteAttributePath & path, AttributeAccessInterface * aai, AttributeValueDecoder & decoder) { - // Processing can happen only if an attribute access interface actually exists.. if (aai == nullptr) { From 7f3075536b1fde401ff29e1b06bbdeb7c5fdafc2 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 09:44:27 -0400 Subject: [PATCH 10/74] Update src/app/codegen-data-model/CodegenDataModel_Write.cpp Co-authored-by: Tennessee Carmel-Veilleux --- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index 81f66860815a7c..d36edf34fa2f56 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -63,7 +63,7 @@ std::optional TryWriteViaAccessInterface(const ConcreteAttributePath } // If the decoder tried to decode, then a value should have been read for processing. - // - if decode, assueme DONE (i.e. FINAL CHIP_NO_ERROR) + // - if decoding was done, assume DONE (i.e. final CHIP_NO_ERROR) // - if no encode, say that processing must continue return decoder.TriedDecode() ? std::make_optional(CHIP_NO_ERROR) : std::nullopt; } From 8a158ef564e0cda173bbfbc77faf8dac3b511fcc Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 09:44:36 -0400 Subject: [PATCH 11/74] Update src/app/codegen-data-model/CodegenDataModel_Write.cpp Co-authored-by: Tennessee Carmel-Veilleux --- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index d36edf34fa2f56..ae275a0df789eb 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -64,7 +64,7 @@ std::optional TryWriteViaAccessInterface(const ConcreteAttributePath // If the decoder tried to decode, then a value should have been read for processing. // - if decoding was done, assume DONE (i.e. final CHIP_NO_ERROR) - // - if no encode, say that processing must continue + // - otherwise, if no decoding done, return that processing must continue via nullopt return decoder.TriedDecode() ? std::make_optional(CHIP_NO_ERROR) : std::nullopt; } From 55e45d45b1c7b7a1beca4d7107306ea96b77f72e Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 09:58:26 -0400 Subject: [PATCH 12/74] Use little endian encoding for pascal long strings, since this is what ember-strings uses --- .../codegen-data-model/CodegenDataModel_Read.cpp | 13 +++++++------ .../codegen-data-model/CodegenDataModel_Write.cpp | 12 ++++++++---- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Read.cpp b/src/app/codegen-data-model/CodegenDataModel_Read.cpp index d4d89a474ca0fc..8328d9d44c8963 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Read.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Read.cpp @@ -37,6 +37,7 @@ #include #include #include +#include #include #include #include @@ -88,6 +89,8 @@ struct ShortPascalString { using LengthType = uint8_t; static constexpr LengthType kNullLength = 0xFF; + + static LengthType GetLength(const uint8_t *buffer) { return emberAfStringLength(buffer); } }; /// Metadata of what a ember/pascal LONG string means (prepended by a u16 length) @@ -95,6 +98,8 @@ struct LongPascalString { using LengthType = uint16_t; static constexpr LengthType kNullLength = 0xFFFF; + + static LengthType GetLength(const uint8_t *buffer) { return emberAfLongStringLength(buffer); } }; // ember assumptions ... should just work @@ -107,12 +112,8 @@ static_assert(sizeof(LongPascalString::LengthType) == 2); template std::optional ExtractEmberString(ByteSpan data) { - typename ENCODING::LengthType len; - - // Ember storage format for pascal-prefix data is specifically "native byte order", - // hence the use of memcpy. - VerifyOrDie(sizeof(len) <= data.size()); - memcpy(&len, data.data(), sizeof(len)); + VerifyOrDie(sizeof(typename ENCODING::LengthType) <= data.size()); + typename ENCODING::LengthType len = ENCODING::GetLength(data.data()); if (len == ENCODING::kNullLength) { diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index 0078b6b6fec005..1c2a7a280b3c5e 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -74,6 +75,8 @@ struct ShortPascalString { using LengthType = uint8_t; static constexpr LengthType kNullLength = 0xFF; + + static void SetLength(uint8_t * buffer, LengthType value) { *buffer = value; } }; /// Metadata of what a ember/pascal LONG string means (prepended by a u16 length) @@ -81,6 +84,9 @@ struct LongPascalString { using LengthType = uint16_t; static constexpr LengthType kNullLength = 0xFFFF; + + // Encoding for ember string lengths is little-endian (see ember-strings.cpp) + static void SetLength(uint8_t * buffer, LengthType value) { Encoding::LittleEndian::Put16(buffer, value); } }; // ember assumptions ... should just work @@ -106,9 +112,7 @@ CHIP_ERROR DecodeStringLikeIntoEmberBuffer(AttributeValueDecoder decoder, bool i if (nullableWorkingValue.IsNull()) { VerifyOrReturnError(out.size() >= sizeof(typename ENCODING::LengthType), CHIP_ERROR_BUFFER_TOO_SMALL); - - typename ENCODING::LengthType nullLength = ENCODING::kNullLength; - memcpy(out.data(), &nullLength, sizeof(nullLength)); + ENCODING::SetLength(out.data(), ENCODING::kNullLength); return CHIP_NO_ERROR; } @@ -123,7 +127,7 @@ CHIP_ERROR DecodeStringLikeIntoEmberBuffer(AttributeValueDecoder decoder, bool i typename ENCODING::LengthType len = static_cast(workingValue.size()); VerifyOrReturnError(out.size() >= sizeof(len) + len, CHIP_ERROR_BUFFER_TOO_SMALL); - memcpy(out.data(), &len, sizeof(len)); + ENCODING::SetLength(out.data(), len); memcpy(out.data() + sizeof(len), workingValue.data(), workingValue.size()); return CHIP_NO_ERROR; } From 1b54c1a7c290028d8ba84eef729de226ca0ce4ae Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 09:58:45 -0400 Subject: [PATCH 13/74] Restyle --- src/app/codegen-data-model/CodegenDataModel_Read.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Read.cpp b/src/app/codegen-data-model/CodegenDataModel_Read.cpp index 8328d9d44c8963..c23aadbdae355e 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Read.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Read.cpp @@ -90,7 +90,7 @@ struct ShortPascalString using LengthType = uint8_t; static constexpr LengthType kNullLength = 0xFF; - static LengthType GetLength(const uint8_t *buffer) { return emberAfStringLength(buffer); } + static LengthType GetLength(const uint8_t * buffer) { return emberAfStringLength(buffer); } }; /// Metadata of what a ember/pascal LONG string means (prepended by a u16 length) @@ -99,7 +99,7 @@ struct LongPascalString using LengthType = uint16_t; static constexpr LengthType kNullLength = 0xFFFF; - static LengthType GetLength(const uint8_t *buffer) { return emberAfLongStringLength(buffer); } + static LengthType GetLength(const uint8_t * buffer) { return emberAfLongStringLength(buffer); } }; // ember assumptions ... should just work From 2518cbaaf5055e48e14bd8bb44352dceff9fb308 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 10:09:11 -0400 Subject: [PATCH 14/74] Fix code to compile and pass tests --- src/app/codegen-data-model/CodegenDataModel_Read.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Read.cpp b/src/app/codegen-data-model/CodegenDataModel_Read.cpp index c23aadbdae355e..82037605911481 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Read.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Read.cpp @@ -99,7 +99,12 @@ struct LongPascalString using LengthType = uint16_t; static constexpr LengthType kNullLength = 0xFFFF; - static LengthType GetLength(const uint8_t * buffer) { return emberAfLongStringLength(buffer); } + static LengthType GetLength(const uint8_t * buffer) + { + // NOTE: we do NOT use emberAfLongStringLength because that will result in 0 length + // for null strings + return Encoding::LittleEndian::Read16(buffer); + } }; // ember assumptions ... should just work From 4fd538a35cbdce44d4d569f8680e2ab15f416be1 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 10:16:00 -0400 Subject: [PATCH 15/74] Code review comments --- .../CodegenDataModel_Write.cpp | 30 ++++++++++--------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index 08689b04ce6926..8ff8f9696d294d 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -126,8 +126,13 @@ CHIP_ERROR DecodeStringLikeIntoEmberBuffer(AttributeValueDecoder decoder, bool i typename ENCODING::LengthType len = static_cast(workingValue.size()); VerifyOrReturnError(out.size() >= sizeof(len) + len, CHIP_ERROR_BUFFER_TOO_SMALL); - ENCODING::SetLength(out.data(), len); - memcpy(out.data() + sizeof(len), workingValue.data(), workingValue.size()); + char * output_buffer = out.data(); + + ENCODING::SetLength(output_buffer, len); + output_buffer += sizeof(len); + + memcpy(output_buffer, workingValue.data(), workingValue.size()); + return CHIP_NO_ERROR; } @@ -137,14 +142,14 @@ CHIP_ERROR DecodeStringLikeIntoEmberBuffer(AttributeValueDecoder decoder, bool i template CHIP_ERROR DecodeIntoEmberBuffer(AttributeValueDecoder & decoder, bool isNullable, MutableByteSpan out) { + using Traits = NumericAttributeTraits; + typename Traits::StorageType storageValue; + if (isNullable) { - using Traits = NumericAttributeTraits; - DataModel::Nullable workingValue; ReturnErrorOnFailure(decoder.Decode(workingValue)); - typename Traits::StorageType storageValue; if (workingValue.IsNull()) { Traits::SetNull(storageValue); @@ -156,18 +161,12 @@ CHIP_ERROR DecodeIntoEmberBuffer(AttributeValueDecoder & decoder, bool isNullabl } VerifyOrReturnError(out.size() >= sizeof(storageValue), CHIP_ERROR_INVALID_ARGUMENT); - - const uint8_t * data = Traits::ToAttributeStoreRepresentation(storageValue); - memcpy(out.data(), data, sizeof(storageValue)); } else { - using Traits = NumericAttributeTraits; - typename Traits::WorkingType workingValue; ReturnErrorOnFailure(decoder.Decode(workingValue)); - typename Traits::StorageType storageValue; Traits::WorkingToStorage(workingValue, storageValue); VerifyOrReturnError(out.size() >= sizeof(storageValue), CHIP_ERROR_INVALID_ARGUMENT); @@ -175,11 +174,14 @@ CHIP_ERROR DecodeIntoEmberBuffer(AttributeValueDecoder & decoder, bool isNullabl // This guards against trying to encode something that overlaps nullable, for example // Nullable(0xFF) is not representable because 0xFF is the encoding of NULL in ember VerifyOrReturnError(Traits::CanRepresentValue(isNullable, workingValue), CHIP_ERROR_INVALID_ARGUMENT); - - const uint8_t * data = Traits::ToAttributeStoreRepresentation(storageValue); - memcpy(out.data(), data, sizeof(storageValue)); } + const uint8_t * data = Traits::ToAttributeStoreRepresentation(storageValue); + + // The decoding + ToAttributeStoreRepresentation will result in data being + // stored in native format/byteorder, suitable to directly be stored in the data store + memcpy(out.data(), data, sizeof(storageValue)); + return CHIP_NO_ERROR; } From 77bc4fe73be6eb30b8f129730df16d1598e4757a Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 10:21:23 -0400 Subject: [PATCH 16/74] Comment update --- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index 8ff8f9696d294d..d43360f42feede 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -156,6 +156,9 @@ CHIP_ERROR DecodeIntoEmberBuffer(AttributeValueDecoder & decoder, bool isNullabl } else { + // This guards against trying to encode something that overlaps nullable, for example + // Nullable(0xFF) is not representable because 0xFF is the encoding of NULL in ember + // as well as odd-sized integers. VerifyOrReturnError(Traits::CanRepresentValue(isNullable, *workingValue), CHIP_ERROR_INVALID_ARGUMENT); Traits::WorkingToStorage(*workingValue, storageValue); } @@ -171,8 +174,8 @@ CHIP_ERROR DecodeIntoEmberBuffer(AttributeValueDecoder & decoder, bool isNullabl VerifyOrReturnError(out.size() >= sizeof(storageValue), CHIP_ERROR_INVALID_ARGUMENT); - // This guards against trying to encode something that overlaps nullable, for example - // Nullable(0xFF) is not representable because 0xFF is the encoding of NULL in ember + // Even non-nullable values may be outside range: e.g. odd-sized integers have working values + // that are larger than the storage values (e.g. a uint32_t being stored as a 3-byte integer) VerifyOrReturnError(Traits::CanRepresentValue(isNullable, workingValue), CHIP_ERROR_INVALID_ARGUMENT); } From 929b5f691b55b6dfd5281fb822392692ab489364 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 10:49:18 -0400 Subject: [PATCH 17/74] Update based on code review feedback --- src/app/codegen-data-model/CodegenDataModel_Read.cpp | 6 +++++- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 6 ++++-- src/app/codegen-data-model/EmberMetadata.h | 10 ++++++++-- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Read.cpp b/src/app/codegen-data-model/CodegenDataModel_Read.cpp index 82037605911481..15f328419e78b7 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Read.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Read.cpp @@ -281,7 +281,11 @@ CHIP_ERROR CodegenDataModel::ReadAttribute(const InteractionModel::ReadAttribute // Explicit failure in finding a suitable metadata if (const CHIP_ERROR * err = std::get_if(&metadata)) { - VerifyOrDie(*err != CHIP_NO_ERROR); + VerifyOrDie( + (*err != CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint)) || + (*err != CHIP_IM_GLOBAL_STATUS(UnsupportedCluster)) || + (*err != CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute)) + ); return *err; } diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index d43360f42feede..12a90dfc72bbeb 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -126,7 +126,7 @@ CHIP_ERROR DecodeStringLikeIntoEmberBuffer(AttributeValueDecoder decoder, bool i typename ENCODING::LengthType len = static_cast(workingValue.size()); VerifyOrReturnError(out.size() >= sizeof(len) + len, CHIP_ERROR_BUFFER_TOO_SMALL); - char * output_buffer = out.data(); + uint8_t * output_buffer = out.data(); ENCODING::SetLength(output_buffer, len); output_buffer += sizeof(len); @@ -278,7 +278,9 @@ CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttribu // Explicit failure in finding a suitable metadata if (const CHIP_ERROR * err = std::get_if(&metadata)) { - VerifyOrDie(*err != CHIP_NO_ERROR); + VerifyOrDie((*err != CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint)) || // + (*err != CHIP_IM_GLOBAL_STATUS(UnsupportedCluster)) || // + (*err != CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute))); return *err; } diff --git a/src/app/codegen-data-model/EmberMetadata.h b/src/app/codegen-data-model/EmberMetadata.h index 848c2b9c11b566..f8f41312f1f7c9 100644 --- a/src/app/codegen-data-model/EmberMetadata.h +++ b/src/app/codegen-data-model/EmberMetadata.h @@ -28,10 +28,16 @@ namespace Ember { /// Fetch the source for the given attribute path: either a cluster (for global ones) or attribute /// path. /// -/// if returning a CHIP_ERROR, it will NEVER be CHIP_NO_ERROR. +/// 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); From 1c1564412dac02e5c0d880a1482ca9355e99406f Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 10:56:12 -0400 Subject: [PATCH 18/74] Wrong condition. Fixed --- src/app/codegen-data-model/CodegenDataModel_Read.cpp | 8 +++----- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 6 +++--- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Read.cpp b/src/app/codegen-data-model/CodegenDataModel_Read.cpp index 15f328419e78b7..bf613622457bcc 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Read.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Read.cpp @@ -281,11 +281,9 @@ CHIP_ERROR CodegenDataModel::ReadAttribute(const InteractionModel::ReadAttribute // Explicit failure in finding a suitable metadata if (const CHIP_ERROR * err = std::get_if(&metadata)) { - VerifyOrDie( - (*err != CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint)) || - (*err != CHIP_IM_GLOBAL_STATUS(UnsupportedCluster)) || - (*err != CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute)) - ); + VerifyOrDie((*err == CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint)) || // + (*err == CHIP_IM_GLOBAL_STATUS(UnsupportedCluster)) || // + (*err == CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute))); return *err; } diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index 12a90dfc72bbeb..18d73d4ed62c2e 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -278,9 +278,9 @@ CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttribu // Explicit failure in finding a suitable metadata if (const CHIP_ERROR * err = std::get_if(&metadata)) { - VerifyOrDie((*err != CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint)) || // - (*err != CHIP_IM_GLOBAL_STATUS(UnsupportedCluster)) || // - (*err != CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute))); + VerifyOrDie((*err == CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint)) || // + (*err == CHIP_IM_GLOBAL_STATUS(UnsupportedCluster)) || // + (*err == CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute))); return *err; } From b61b3aa318b4aad36ea7338aed519c0c3c4ea8e9 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 10:58:57 -0400 Subject: [PATCH 19/74] Return invalid value to match ember-compatibility-functions --- src/app/codegen-data-model/CodegenDataModel_Read.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Read.cpp b/src/app/codegen-data-model/CodegenDataModel_Read.cpp index bf613622457bcc..7bcbaebb84298c 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Read.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Read.cpp @@ -238,7 +238,7 @@ CHIP_ERROR EncodeEmberValue(ByteSpan data, const EmberAfAttributeMetadata * meta return EncodeStringLike(data, isNullable, encoder); default: ChipLogError(DataManagement, "Attribute type 0x%x not handled", static_cast(metadata->attributeType)); - return CHIP_IM_GLOBAL_STATUS(UnsupportedRead); + return CHIP_IM_GLOBAL_STATUS(InvalidValue); } } From 0992f6d60df9fd1583b98fcff2f7fe01d1052bb5 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 11:00:25 -0400 Subject: [PATCH 20/74] switch invalid data point to constraint error for return codes --- src/app/codegen-data-model/CodegenDataModel_Read.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Read.cpp b/src/app/codegen-data-model/CodegenDataModel_Read.cpp index 7bcbaebb84298c..33db6561df5600 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Read.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Read.cpp @@ -238,7 +238,7 @@ CHIP_ERROR EncodeEmberValue(ByteSpan data, const EmberAfAttributeMetadata * meta return EncodeStringLike(data, isNullable, encoder); default: ChipLogError(DataManagement, "Attribute type 0x%x not handled", static_cast(metadata->attributeType)); - return CHIP_IM_GLOBAL_STATUS(InvalidValue); + return CHIP_IM_GLOBAL_STATUS(ConstraintError); } } From 5b7d5366fd0d62c34e8c49449a5288a58ad73127 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 11:14:19 -0400 Subject: [PATCH 21/74] Fix code review comments: comments and return unsupportedaccess --- .../CodegenDataModel_Read.cpp | 5 ++++- .../CodegenDataModel_Write.cpp | 20 ++++++++++++------- .../tests/TestCodegenModelViaMocks.cpp | 4 ++-- 3 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Read.cpp b/src/app/codegen-data-model/CodegenDataModel_Read.cpp index 33db6561df5600..6325eb150c8078 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Read.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Read.cpp @@ -267,12 +267,15 @@ CHIP_ERROR CodegenDataModel::ReadAttribute(const InteractionModel::ReadAttribute RequiredPrivilege::ForReadAttribute(request.path)); if (err != CHIP_NO_ERROR) { + ReturnErrorCodeIf(err != CHIP_ERROR_ACCESS_DENIED, err); + // Implementation of 8.4.3.2 of the spec for path expansion if (request.path.mExpanded && (err == CHIP_ERROR_ACCESS_DENIED)) { return CHIP_NO_ERROR; } - return err; + // access denied has a specific code for IM + return CHIP_IM_GLOBAL_STATUS(UnsupportedAccess); } } diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index 18d73d4ed62c2e..8fae41bfbc391a 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -258,19 +258,25 @@ CHIP_ERROR DecodeValueIntoEmberBuffer(AttributeValueDecoder & decoder, const Emb CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttributeRequest & request, AttributeValueDecoder & decoder) { - ChipLogDetail(DataManagement, - "Reading attribute: Cluster=" ChipLogFormatMEI " Endpoint=%x AttributeId=" ChipLogFormatMEI " (expanded=%d)", - ChipLogValueMEI(request.path.mClusterId), request.path.mEndpointId, ChipLogValueMEI(request.path.mAttributeId), - request.path.mExpanded); + ChipLogDetail(DataManagement, "Writing attribute: Cluster=" ChipLogFormatMEI " Endpoint=%x AttributeId=" ChipLogFormatMEI, + ChipLogValueMEI(request.path.mClusterId), request.path.mEndpointId, ChipLogValueMEI(request.path.mAttributeId)); // ACL check for non-internal requests if (!request.operationFlags.Has(InteractionModel::OperationFlags::kInternal)) { - ReturnErrorCodeIf(!request.subjectDescriptor.has_value(), CHIP_ERROR_INVALID_ARGUMENT); + ReturnErrorCodeIf(!request.subjectDescriptor.has_value(), CHIP_IM_GLOBAL_STATUS(UnsupportedAccess)); Access::RequestPath requestPath{ .cluster = request.path.mClusterId, .endpoint = request.path.mEndpointId }; - ReturnErrorOnFailure(Access::GetAccessControl().Check(*request.subjectDescriptor, requestPath, - RequiredPrivilege::ForWriteAttribute(request.path))); + CHIP_ERROR err = Access::GetAccessControl().Check(*request.subjectDescriptor, requestPath, + RequiredPrivilege::ForWriteAttribute(request.path)); + + if (err != CHIP_NO_ERROR) + { + 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); + } } auto metadata = Ember::FindAttributeMetadata(request.path); diff --git a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp index 5ac54b60799198..0a6605c15c5c95 100644 --- a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp +++ b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp @@ -1125,7 +1125,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_ERROR_ACCESS_DENIED); + ASSERT_EQ(model.ReadAttribute(testRequest.request, *encoder), CHIP_IM_GLOBAL_STATUS(UnsupportedAccess)); } TEST(TestCodegenModelViaMocks, ReadForInvalidGlobalAttributePath) @@ -1809,7 +1809,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_ERROR_ACCESS_DENIED); + ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_IM_GLOBAL_STATUS(UnsupportedAccess)); } TEST(TestCodegenModelViaMocks, EmberAttributeWriteBasicTypes) From 65baf809bcf0de6c335c40756457aa4364a1aedf Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 11:16:25 -0400 Subject: [PATCH 22/74] Remove useless comment - error check should be clear enough --- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index 8fae41bfbc391a..6e507ca9a927ba 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -281,7 +281,6 @@ CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttribu auto metadata = Ember::FindAttributeMetadata(request.path); - // Explicit failure in finding a suitable metadata if (const CHIP_ERROR * err = std::get_if(&metadata)) { VerifyOrDie((*err == CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint)) || // From 76b7e7d222fd7cc18ed536f132b36cbb0d77ede5 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 11:20:42 -0400 Subject: [PATCH 23/74] Comment update --- .../codegen-data-model/CodegenDataModel_Write.cpp | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index 6e507ca9a927ba..cbd6a8d573ab6e 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -289,13 +289,18 @@ CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttribu return *err; } - // All the global attributes that we do not have metadata for are - // read-only (i.e. cannot write atribute_list/event_list/accepted_cmds/generated_cmds) - // - // so if no metadata available, we wil lreturn an error const EmberAfAttributeMetadata ** attributeMetadata = std::get_if(&metadata); if (attributeMetadata == nullptr) { + // All the global attributes that we do not have metadata for are + // read-only. Specifically only the following list-based attributes match the + // "global attributes not in metadata" (see GlobalAttributes.h :: GlobalAttributesNotInMetadat): + // - AttributeList + // - EventList + // - AcceptedCommands + // - GeneratedCommands + // + // Given the above, UnsupportedWrite should be correct (attempt to write to a read-only list) return CHIP_IM_GLOBAL_STATUS(UnsupportedWrite); } From 1f79a52d2b9988ec73fc88eb5bb4350f0e033048 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 11:24:22 -0400 Subject: [PATCH 24/74] Re-arrange code for read only and timed --- .../codegen-data-model/CodegenDataModel_Write.cpp | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index cbd6a8d573ab6e..afcf10d4ac65bc 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -304,16 +304,14 @@ CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttribu return CHIP_IM_GLOBAL_STATUS(UnsupportedWrite); } - if ((*attributeMetadata)->IsReadOnly() && !request.operationFlags.Has(InteractionModel::OperationFlags::kInternal)) + // Internal is allowed to bypass timed writes and read-only. + if (!request.operationFlags.Has(InteractionModel::OperationFlags::kInternal)) { - // Internal is allowed to try to bypass read-only updates, however otherwise we deny read-only - // updates - return CHIP_IM_GLOBAL_STATUS(UnsupportedWrite); - } + VerifyOrReturnError(!(*attributeMetadata)->IsReadOnly(), CHIP_IM_GLOBAL_STATUS(UnsupportedWrite)); - if ((*attributeMetadata)->MustUseTimedWrite() && !request.writeFlags.Has(InteractionModel::WriteFlags::kTimed)) - { - return CHIP_IM_GLOBAL_STATUS(NeedsTimedInteraction); + VerifyOrReturnError(!(*attributeMetadata)->MustUseTimedWrite() || + request.writeFlags.Has(InteractionModel::WriteFlags::kTimed), + CHIP_IM_GLOBAL_STATUS(NeedsTimedInteraction)); } if (request.path.mDataVersion.HasValue()) From a629992d82e71e36d80ce97bcf86c0d64e57f167 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 11:27:28 -0400 Subject: [PATCH 25/74] Re-format the read only checks a bit --- .../CodegenDataModel_Write.cpp | 30 ++++++++++--------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index afcf10d4ac65bc..6a7e4c790d7b21 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -290,30 +290,32 @@ CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttribu } const EmberAfAttributeMetadata ** attributeMetadata = std::get_if(&metadata); - if (attributeMetadata == nullptr) - { - // All the global attributes that we do not have metadata for are - // read-only. Specifically only the following list-based attributes match the - // "global attributes not in metadata" (see GlobalAttributes.h :: GlobalAttributesNotInMetadat): - // - AttributeList - // - EventList - // - AcceptedCommands - // - GeneratedCommands - // - // Given the above, UnsupportedWrite should be correct (attempt to write to a read-only list) - return CHIP_IM_GLOBAL_STATUS(UnsupportedWrite); - } + + // All the global attributes that we do not have metadata for are + // read-only. Specifically only the following list-based attributes match the + // "global attributes not in metadata" (see GlobalAttributes.h :: GlobalAttributesNotInMetadat): + // - AttributeList + // - EventList + // - AcceptedCommands + // - GeneratedCommands + // + // Given the above, UnsupportedWrite should be correct (attempt to write to a read-only list) + bool isReadOnly = (attributeMetadata == nullptr) || (*attributeMetadata)->IsReadOnly(); // Internal is allowed to bypass timed writes and read-only. if (!request.operationFlags.Has(InteractionModel::OperationFlags::kInternal)) { - VerifyOrReturnError(!(*attributeMetadata)->IsReadOnly(), CHIP_IM_GLOBAL_STATUS(UnsupportedWrite)); + VerifyOrReturnError(!isReadOnly, CHIP_IM_GLOBAL_STATUS(UnsupportedWrite)); VerifyOrReturnError(!(*attributeMetadata)->MustUseTimedWrite() || request.writeFlags.Has(InteractionModel::WriteFlags::kTimed), CHIP_IM_GLOBAL_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)); + if (request.path.mDataVersion.HasValue()) { std::optional clusterInfo = GetClusterInfo(request.path); From 3741c61af335613e0adddce8563677709f008dfb Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 11:32:56 -0400 Subject: [PATCH 26/74] Use CHIP_ERROR_NOT_FOUND --- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index 6a7e4c790d7b21..53e3a3540e2845 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -323,7 +323,7 @@ CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttribu { ChipLogError(DataManagement, "Unable to get cluster info for Endpoint %x, Cluster " ChipLogFormatMEI, request.path.mEndpointId, ChipLogValueMEI(request.path.mClusterId)); - return CHIP_IM_GLOBAL_STATUS(DataVersionMismatch); + return CHIP_ERROR_NOT_FOUND; } if (request.path.mDataVersion.Value() != clusterInfo->dataVersion) From e252bb06e4a722a2b7fdc1d5177e97fd706bf3bf Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 11:35:21 -0400 Subject: [PATCH 27/74] Separate out variable names --- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index 53e3a3540e2845..dd2d606e53d105 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -334,8 +334,8 @@ CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttribu } } - std::optional aai_result = TryWriteViaAccessInterface( - request.path, GetAttributeAccessOverride(request.path.mEndpointId, request.path.mClusterId), decoder); + AttributeAccessInterface * aai = GetAttributeAccessOverride(request.path.mEndpointId, request.path.mClusterId); + std::optional aai_result = TryWriteViaAccessInterface(request.path, aai, decoder); ReturnErrorCodeIf(aai_result.has_value(), *aai_result); ReturnErrorOnFailure(DecodeValueIntoEmberBuffer(decoder, *attributeMetadata, gEmberAttributeIOBufferSpan)); From 2b0bc7ac54457652ce806ba6b1fe84f6a6643ab9 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 11:37:24 -0400 Subject: [PATCH 28/74] Slight updated code layout --- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index dd2d606e53d105..621d08919d49a6 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -336,6 +336,12 @@ CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttribu AttributeAccessInterface * aai = GetAttributeAccessOverride(request.path.mEndpointId, request.path.mClusterId); std::optional aai_result = TryWriteViaAccessInterface(request.path, aai, decoder); + + if (aai_result.has_value()) + { + return *aai_result; + } + ReturnErrorCodeIf(aai_result.has_value(), *aai_result); ReturnErrorOnFailure(DecodeValueIntoEmberBuffer(decoder, *attributeMetadata, gEmberAttributeIOBufferSpan)); From 3296766a7e772e0a0c53089a3b2aba58178b7404 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 11:38:44 -0400 Subject: [PATCH 29/74] Updated return value for chip error --- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index 621d08919d49a6..fba6690e2e8bdc 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -357,7 +357,7 @@ CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttribu if (status != Protocols::InteractionModel::Status::Success) { - return ChipError(ChipError::SdkPart::kIMGlobalStatus, to_underlying(status), __FILE__, __LINE__); + return CHIP_ERROR_IM_GLOBAL_STATUS_VALUE(status); } return CHIP_NO_ERROR; From 16cc088eb86fdb770f6dcd49cec05e9561d3da32 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 11:43:06 -0400 Subject: [PATCH 30/74] Updated test to verifyordie instead of just logging errors --- src/app/codegen-data-model/EmberMetadata.cpp | 1 - .../codegen-data-model/tests/EmberReadWriteOverride.cpp | 7 +------ 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/src/app/codegen-data-model/EmberMetadata.cpp b/src/app/codegen-data-model/EmberMetadata.cpp index 11fdef0515a94f..a39c3cb1188e61 100644 --- a/src/app/codegen-data-model/EmberMetadata.cpp +++ b/src/app/codegen-data-model/EmberMetadata.cpp @@ -32,7 +32,6 @@ FindAttributeMetadata(const ConcreteAttributePath & aPath) { for (auto & attr : GlobalAttributesNotInMetadata) { - if (attr == aPath.mAttributeId) { const EmberAfCluster * cluster = emberAfFindServerCluster(aPath.mEndpointId, aPath.mClusterId); diff --git a/src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp b/src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp index 8d7f98b3997767..1dabd7d0777f1d 100644 --- a/src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp +++ b/src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp @@ -92,12 +92,7 @@ Status emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord, } else { - if (gEmberIoBufferFill > readLength) - { - ChipLogError(Test, "Internal TEST error: insufficient output buffer space."); - return Status::ResourceExhausted; - } - + VerifyOrDie(gEmberIoBufferFill <= readLength); memcpy(buffer, gEmberIoBuffer, gEmberIoBufferFill); } From 682b73a1e0b04f49df18fae6a05ee5ba79e0a178 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 11:44:21 -0400 Subject: [PATCH 31/74] Update src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp Co-authored-by: Tennessee Carmel-Veilleux --- .../tests/TestCodegenModelViaMocks.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp index 0a6605c15c5c95..70ef2c8e9fc98f 100644 --- a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp +++ b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp @@ -1816,11 +1816,11 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteBasicTypes) { TestEmberScalarTypeWrite(0x12); TestEmberScalarTypeWrite(0x1234); - TestEmberScalarTypeWrite, ZCL_INT24U_ATTRIBUTE_TYPE>(0x1234AB); - TestEmberScalarTypeWrite(0x1234ABCD); - TestEmberScalarTypeWrite, ZCL_INT40U_ATTRIBUTE_TYPE>(0x1122334455); - TestEmberScalarTypeWrite, ZCL_INT48U_ATTRIBUTE_TYPE>(0xAABB11223344); - TestEmberScalarTypeWrite, ZCL_INT56U_ATTRIBUTE_TYPE>(0xAABB11223344); + TestEmberScalarTypeWrite, ZCL_INT24U_ATTRIBUTE_TYPE>(0x112233); + TestEmberScalarTypeWrite(0x11223344); + TestEmberScalarTypeWrite, ZCL_INT40U_ATTRIBUTE_TYPE>(0x1122334455ULL); + TestEmberScalarTypeWrite, ZCL_INT48U_ATTRIBUTE_TYPE>(0x112233445566ULL); + TestEmberScalarTypeWrite, ZCL_INT56U_ATTRIBUTE_TYPE>(0x11223344556677ULL); TestEmberScalarTypeWrite(0x1122334455667788L); TestEmberScalarTypeWrite(-10); From 37f4298d382fbe878003b18281826549644ad3dd Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 11:45:05 -0400 Subject: [PATCH 32/74] Update based on review feedback --- src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp index 0a6605c15c5c95..2fd3e4790afb82 100644 --- a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp +++ b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp @@ -1939,7 +1939,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteLongNullableStringNull) ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); chip::ByteSpan writtenData = GetEmberBuffer(); ASSERT_EQ(writtenData[0], 0xFF); - ASSERT_EQ(writtenData[0], 0xFF); + ASSERT_EQ(writtenData[1], 0xFF); } TEST(TestCodegenModelViaMocks, EmberAttributeWriteShortBytes) From 03bb45fa6bacdeb2a674dab52881deeb5d1a70ee Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 12:33:28 -0400 Subject: [PATCH 33/74] Fix endianess and copying in test code --- .../tests/TestCodegenModelViaMocks.cpp | 36 +++++++++++-------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp index 843fa87a69cb24..7a15a2682993c7 100644 --- a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp +++ b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp @@ -728,6 +728,18 @@ void TestEmberScalarNullWrite() ASSERT_TRUE(Traits::IsNullValue(storage)); } +uint16_t ReadLe16(const void * buffer) +{ + const uint8_t * p = reinterpret_cast(buffer); + return chip::Encoding::LittleEndian::Read16(p); +} + +void WriteLe16(void * buffer, uint16_t value) +{ + uint8_t * p = reinterpret_cast(buffer); + chip::Encoding::LittleEndian::Write16(p, value); +} + } // namespace TEST(TestCodegenModelViaMocks, IterateOverEndpoints) @@ -1389,9 +1401,8 @@ TEST(TestCodegenModelViaMocks, EmberAttributeReadOctetString) // NOTE: This is a pascal string, so actual data is "test" // the longer encoding is to make it clear we do not encode the overflow - char data[] = "\0\0testing here with overflow"; - uint16_t len = 4; - memcpy(data, &len, sizeof(uint16_t)); + char data[] = "\0\0testing here with overflow"; + WriteLe16(data, 4); chip::Test::SetEmberReadOutput(ByteSpan(reinterpret_cast(data), sizeof(data))); // Actual read via an encoder @@ -1465,9 +1476,8 @@ TEST(TestCodegenModelViaMocks, EmberAttributeReadShortString) // NOTE: This is a pascal string, so actual data is "abcde" // the longer encoding is to make it clear we do not encode the overflow - char data[] = "\0abcdef...this is the alphabet"; - uint16_t len = 5; - memcpy(data, &len, sizeof(uint8_t)); + char data[] = "\0abcdef...this is the alphabet"; + *data = 5; chip::Test::SetEmberReadOutput(ByteSpan(reinterpret_cast(data), sizeof(data))); // Actual read via an encoder @@ -1503,9 +1513,8 @@ TEST(TestCodegenModelViaMocks, EmberAttributeReadLongString) // NOTE: This is a pascal string, so actual data is "abcde" // the longer encoding is to make it clear we do not encode the overflow - char data[] = "\0\0abcdef...this is the alphabet"; - uint16_t len = 5; - memcpy(data, &len, sizeof(uint16_t)); + char data[] = "\0\0abcdef...this is the alphabet"; + WriteLe16(data, 5); chip::Test::SetEmberReadOutput(ByteSpan(reinterpret_cast(data), sizeof(data))); // Actual read via an encoder @@ -1893,8 +1902,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteLongString) ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); chip::ByteSpan writtenData = GetEmberBuffer(); - uint16_t len; - memcpy(&len, writtenData.data(), 2); + uint16_t len = ReadLe16(writtenData.data()); EXPECT_EQ(len, 4); chip::CharSpan asCharSpan(reinterpret_cast(writtenData.data() + 2), 4); @@ -1916,8 +1924,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteNullableLongStringValue) ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); chip::ByteSpan writtenData = GetEmberBuffer(); - uint16_t len; - memcpy(&len, writtenData.data(), 2); + uint16_t len = ReadLe16(writtenData.data()); EXPECT_EQ(len, 4); chip::CharSpan asCharSpan(reinterpret_cast(writtenData.data() + 2), 4); @@ -1980,8 +1987,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteLongBytes) ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); chip::ByteSpan writtenData = GetEmberBuffer(); - uint16_t len; - memcpy(&len, writtenData.data(), 2); + uint16_t len = ReadLe16(writtenData.data()); EXPECT_EQ(len, 3); EXPECT_EQ(writtenData[2], 11u); From 8b630b0c831838f18289d1f06a64d497e3f971ce Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 12:33:42 -0400 Subject: [PATCH 34/74] Restyle --- src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp index 7a15a2682993c7..e5a073a0a529da 100644 --- a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp +++ b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp @@ -1477,7 +1477,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeReadShortString) // NOTE: This is a pascal string, so actual data is "abcde" // the longer encoding is to make it clear we do not encode the overflow char data[] = "\0abcdef...this is the alphabet"; - *data = 5; + *data = 5; chip::Test::SetEmberReadOutput(ByteSpan(reinterpret_cast(data), sizeof(data))); // Actual read via an encoder From 412d681e1c790cde312f1f941e1b73a8e2a1a8ac Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 12:35:07 -0400 Subject: [PATCH 35/74] Updated comment --- src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp index e5a073a0a529da..35a9045ab1b119 100644 --- a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp +++ b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp @@ -2139,6 +2139,8 @@ TEST(TestCodegenModelViaMocks, EmberWriteAttributeAccessInterfaceTest) EXPECT_TRUE(aai->GetData().e.data_equal("aai_write_test"_span)); // AAI does not prevent read/write of regular attributes + // validate that once AAI is added, we still can go through writing regular bits (i.e. + // AAI returning "unknown" has fallback to ember) TestEmberScalarTypeWrite(1234); TestEmberScalarNullWrite(); } From 413b5e7ea5488588f8d0aa6da5d8c3dcd92a1c27 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 12:35:35 -0400 Subject: [PATCH 36/74] Update src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp Co-authored-by: Tennessee Carmel-Veilleux --- src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp index 35a9045ab1b119..9e943bd0c11147 100644 --- a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp +++ b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp @@ -1830,7 +1830,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteBasicTypes) TestEmberScalarTypeWrite, ZCL_INT40U_ATTRIBUTE_TYPE>(0x1122334455ULL); TestEmberScalarTypeWrite, ZCL_INT48U_ATTRIBUTE_TYPE>(0x112233445566ULL); TestEmberScalarTypeWrite, ZCL_INT56U_ATTRIBUTE_TYPE>(0x11223344556677ULL); - TestEmberScalarTypeWrite(0x1122334455667788L); + TestEmberScalarTypeWrite(0x1122334455667788ULL); TestEmberScalarTypeWrite(-10); TestEmberScalarTypeWrite(-123); From 8b1a1b1f9e383e23279e2d2a1bf258d35e22b0a3 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 12:38:00 -0400 Subject: [PATCH 37/74] Add unit test for "lowest signed value write" --- .../tests/TestCodegenModelViaMocks.cpp | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp index 9e943bd0c11147..69819d179d8633 100644 --- a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp +++ b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp @@ -1847,6 +1847,18 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteBasicTypes) TestEmberScalarTypeWrite(0.625); } +TEST(TestCodegenModelViaMocks, EmberAttributeWriteBasicTypesLowestValue) +{ + TestEmberScalarTypeWrite(-127); + TestEmberScalarTypeWrite(-32767); + TestEmberScalarTypeWrite, ZCL_INT24S_ATTRIBUTE_TYPE>(-8388607); + TestEmberScalarTypeWrite(-2147483647); + TestEmberScalarTypeWrite, ZCL_INT40S_ATTRIBUTE_TYPE>(-549755813887); + TestEmberScalarTypeWrite, ZCL_INT48S_ATTRIBUTE_TYPE>(-140737488355327); + TestEmberScalarTypeWrite, ZCL_INT56S_ATTRIBUTE_TYPE>(-36028797018963967); + TestEmberScalarTypeWrite(-9223372036854775807); +} + TEST(TestCodegenModelViaMocks, EmberAttributeWriteNulls) { TestEmberScalarNullWrite(); From 516a1c98f7f3847d7380df201ab730efa1f9a4d6 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 12:38:54 -0400 Subject: [PATCH 38/74] Restyle --- src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp index 69819d179d8633..a5a88dfd3edb0f 100644 --- a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp +++ b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp @@ -2152,7 +2152,7 @@ TEST(TestCodegenModelViaMocks, EmberWriteAttributeAccessInterfaceTest) // AAI does not prevent read/write of regular attributes // validate that once AAI is added, we still can go through writing regular bits (i.e. - // AAI returning "unknown" has fallback to ember) + // AAI returning "unknown" has fallback to ember) TestEmberScalarTypeWrite(1234); TestEmberScalarNullWrite(); } From 2aed85dfabb3eeb9b09ccb884c2532e0187656f4 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 13:19:27 -0400 Subject: [PATCH 39/74] A constraint error update and better tests for AAI returning errors --- .../CodegenDataModel_Write.cpp | 3 +- .../tests/TestCodegenModelViaMocks.cpp | 71 +++++++++++++++++++ 2 files changed, 72 insertions(+), 2 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index fba6690e2e8bdc..ab06c12a991aa7 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -248,9 +248,8 @@ CHIP_ERROR DecodeValueIntoEmberBuffer(AttributeValueDecoder & decoder, const Emb return DecodeStringLikeIntoEmberBuffer(decoder, isNullable, out); default: ChipLogError(DataManagement, "Attribute type 0x%x not handled", static_cast(metadata->attributeType)); - return CHIP_IM_GLOBAL_STATUS(UnsupportedWrite); + return CHIP_IM_GLOBAL_STATUS(ConstraintError); } - return CHIP_IM_GLOBAL_STATUS(UnsupportedWrite); } } // namespace diff --git a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp index a5a88dfd3edb0f..a1f8643f982852 100644 --- a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp +++ b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp @@ -450,6 +450,39 @@ class StructAttributeAccessInterface : public AttributeAccessInterface Clusters::UnitTesting::Structs::SimpleStruct::Type mData; }; +class ErrorAccessInterface : public AttributeAccessInterface +{ +public: + ErrorAccessInterface(ConcreteAttributePath path, CHIP_ERROR err) : + AttributeAccessInterface(MakeOptional(path.mEndpointId), path.mClusterId), mPath(path), mError(err) + {} + ~ErrorAccessInterface() = default; + + CHIP_ERROR Read(const ConcreteReadAttributePath & path, AttributeValueEncoder & encoder) override + { + if (static_cast(path) != mPath) + { + // returning without trying to handle means "I do not handle this" + return CHIP_NO_ERROR; + } + return mError; + } + + CHIP_ERROR Write(const ConcreteDataAttributePath & path, AttributeValueDecoder & decoder) override + { + if (static_cast(path) != mPath) + { + // returning without trying to handle means "I do not handle this" + return CHIP_NO_ERROR; + } + return mError; + } + +private: + ConcreteAttributePath mPath; + CHIP_ERROR mError; +}; + class ListAttributeAcessInterface : public AttributeAccessInterface { public: @@ -1580,6 +1613,21 @@ TEST(TestCodegenModelViaMocks, AttributeAccessInterfaceStructRead) ASSERT_TRUE(actual.e.data_equal("foo"_span)); } +TEST(TestCodegenModelViaMocks, AttributeAccessInterfaceReadError) +{ + UseMockNodeConfig config(gTestNodeConfig); + chip::app::CodegenDataModel model; + ScopedMockAccessControl accessControl; + + const ConcreteAttributePath kStructPath(kMockEndpoint3, MockClusterId(4), + MOCK_ATTRIBUTE_ID_FOR_NON_NULLABLE_TYPE(ZCL_STRUCT_ATTRIBUTE_TYPE)); + + TestReadRequest testRequest(kAdminSubjectDescriptor, kStructPath); + RegisteredAttributeAccessInterface aai(kStructPath, CHIP_ERROR_KEY_NOT_FOUND); + std::unique_ptr encoder = testRequest.StartEncoding(&model); + ASSERT_EQ(model.ReadAttribute(testRequest.request, *encoder), CHIP_ERROR_KEY_NOT_FOUND); +} + TEST(TestCodegenModelViaMocks, AttributeAccessInterfaceListRead) { UseMockNodeConfig config(gTestNodeConfig); @@ -2156,3 +2204,26 @@ TEST(TestCodegenModelViaMocks, EmberWriteAttributeAccessInterfaceTest) TestEmberScalarTypeWrite(1234); TestEmberScalarNullWrite(); } + +TEST(TestCodegenModelViaMocks, EmberWriteAttributeAccessInterfaceReturningError) +{ + UseMockNodeConfig config(gTestNodeConfig); + chip::app::CodegenDataModel model; + ScopedMockAccessControl accessControl; + + const ConcreteAttributePath kStructPath(kMockEndpoint3, MockClusterId(4), + MOCK_ATTRIBUTE_ID_FOR_NON_NULLABLE_TYPE(ZCL_STRUCT_ATTRIBUTE_TYPE)); + RegisteredAttributeAccessInterface aai(kStructPath, CHIP_ERROR_KEY_NOT_FOUND); + + TestWriteRequest test(kAdminSubjectDescriptor, kStructPath); + Clusters::UnitTesting::Structs::SimpleStruct::Type testValue{ + .a = 112, + .b = true, + .e = "aai_write_test"_span, + .g = 0.5, + .h = 0.125, + }; + + AttributeValueDecoder decoder = test.DecoderFor(testValue); + ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_ERROR_KEY_NOT_FOUND); +} From 1a48fc9e7c8a35ec1ed5637cb7d30bfbf9abcfb5 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 13:22:12 -0400 Subject: [PATCH 40/74] One more test for invalid ember usage --- .../tests/TestCodegenModelViaMocks.cpp | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp index a1f8643f982852..86c6b3567f3314 100644 --- a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp +++ b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp @@ -2227,3 +2227,26 @@ TEST(TestCodegenModelViaMocks, EmberWriteAttributeAccessInterfaceReturningError) AttributeValueDecoder decoder = test.DecoderFor(testValue); ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_ERROR_KEY_NOT_FOUND); } + +TEST(TestCodegenModelViaMocks, EmberWriteInvalidDataType) +{ + UseMockNodeConfig config(gTestNodeConfig); + chip::app::CodegenDataModel model; + ScopedMockAccessControl accessControl; + + // Embed specifically DOES NOT support structures. Without AAI, we expect a constraint error + const ConcreteAttributePath kStructPath(kMockEndpoint3, MockClusterId(4), + MOCK_ATTRIBUTE_ID_FOR_NON_NULLABLE_TYPE(ZCL_STRUCT_ATTRIBUTE_TYPE)); + + TestWriteRequest test(kAdminSubjectDescriptor, kStructPath); + Clusters::UnitTesting::Structs::SimpleStruct::Type testValue{ + .a = 112, + .b = true, + .e = "aai_write_test"_span, + .g = 0.5, + .h = 0.125, + }; + + AttributeValueDecoder decoder = test.DecoderFor(testValue); + ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_IM_GLOBAL_STATUS(ConstraintError)); +} From 6fc8d17cb124661a5e9e86e196ff2d2bf817aad1 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 13:22:36 -0400 Subject: [PATCH 41/74] Restyle --- src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp index 86c6b3567f3314..9a6a3d12bee0c0 100644 --- a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp +++ b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp @@ -2234,7 +2234,7 @@ TEST(TestCodegenModelViaMocks, EmberWriteInvalidDataType) chip::app::CodegenDataModel model; ScopedMockAccessControl accessControl; - // Embed specifically DOES NOT support structures. Without AAI, we expect a constraint error + // Embed specifically DOES NOT support structures. Without AAI, we expect a constraint error const ConcreteAttributePath kStructPath(kMockEndpoint3, MockClusterId(4), MOCK_ATTRIBUTE_ID_FOR_NON_NULLABLE_TYPE(ZCL_STRUCT_ATTRIBUTE_TYPE)); From e93665b033ce9ff17a4619880418e5bc56f2b263 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 13:42:48 -0400 Subject: [PATCH 42/74] more tests for more coverage --- .../CodegenDataModel_Write.cpp | 3 + .../tests/TestCodegenModelViaMocks.cpp | 84 ++++++++++++++++++- 2 files changed, 86 insertions(+), 1 deletion(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index ab06c12a991aa7..365eb67a53536e 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -159,6 +159,9 @@ CHIP_ERROR DecodeIntoEmberBuffer(AttributeValueDecoder & decoder, bool isNullabl // This guards against trying to encode something that overlaps nullable, for example // Nullable(0xFF) is not representable because 0xFF is the encoding of NULL in ember // as well as odd-sized integers. + // + // It also guargs against out of range (e.g. full 32-bit value like 0x11223344 to written to 3-byte + // odd-sized integger) VerifyOrReturnError(Traits::CanRepresentValue(isNullable, *workingValue), CHIP_ERROR_INVALID_ARGUMENT); Traits::WorkingToStorage(*workingValue, storageValue); } diff --git a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp index 9a6a3d12bee0c0..4e7e58f1a969ca 100644 --- a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp +++ b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp @@ -761,6 +761,25 @@ void TestEmberScalarNullWrite() ASSERT_TRUE(Traits::IsNullValue(storage)); } +template +void TestEmberScalarTypeWriteNullValueToNullable() +{ + UseMockNodeConfig config(gTestNodeConfig); + chip::app::CodegenDataModel model; + ScopedMockAccessControl accessControl; + + TestWriteRequest test( + kAdminSubjectDescriptor, + ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), MOCK_ATTRIBUTE_ID_FOR_NON_NULLABLE_TYPE(ZclType))); + + using NumericType = NumericAttributeTraits; + using NullableType = chip::app::DataModel::Nullable; + AttributeValueDecoder decoder = test.DecoderFor(NullableType()); + + // write should fail: we are trying to write null + ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_ERROR_WRONG_TLV_TYPE); +} + uint16_t ReadLe16(const void * buffer) { const uint8_t * p = reinterpret_cast(buffer); @@ -1895,7 +1914,70 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteBasicTypes) TestEmberScalarTypeWrite(0.625); } -TEST(TestCodegenModelViaMocks, EmberAttributeWriteBasicTypesLowestValue) +TEST(TestCodegenModelViaMocks, EmberAttributeWriteInvalidValueToNullable) +{ + TestEmberScalarTypeWriteNullValueToNullable(); + TestEmberScalarTypeWriteNullValueToNullable(); + TestEmberScalarTypeWriteNullValueToNullable, ZCL_INT24U_ATTRIBUTE_TYPE>(); + TestEmberScalarTypeWriteNullValueToNullable(); + TestEmberScalarTypeWriteNullValueToNullable, ZCL_INT40U_ATTRIBUTE_TYPE>(); + TestEmberScalarTypeWriteNullValueToNullable, ZCL_INT48U_ATTRIBUTE_TYPE>(); + TestEmberScalarTypeWriteNullValueToNullable, ZCL_INT56U_ATTRIBUTE_TYPE>(); + TestEmberScalarTypeWriteNullValueToNullable(); + + TestEmberScalarTypeWriteNullValueToNullable(); + TestEmberScalarTypeWriteNullValueToNullable(); + TestEmberScalarTypeWriteNullValueToNullable, ZCL_INT24S_ATTRIBUTE_TYPE>(); + TestEmberScalarTypeWriteNullValueToNullable(); + TestEmberScalarTypeWriteNullValueToNullable, ZCL_INT40S_ATTRIBUTE_TYPE>(); + TestEmberScalarTypeWriteNullValueToNullable, ZCL_INT48S_ATTRIBUTE_TYPE>(); + TestEmberScalarTypeWriteNullValueToNullable, ZCL_INT56S_ATTRIBUTE_TYPE>(); + TestEmberScalarTypeWriteNullValueToNullable(); + + TestEmberScalarTypeWriteNullValueToNullable(); + TestEmberScalarTypeWriteNullValueToNullable(); + TestEmberScalarTypeWriteNullValueToNullable(); + TestEmberScalarTypeWriteNullValueToNullable(); +} + +TEST(TestCodegenModelViaMocks, EmberTestWriteReservedNullPlaceholderToNullable) +{ + UseMockNodeConfig config(gTestNodeConfig); + chip::app::CodegenDataModel model; + ScopedMockAccessControl accessControl; + + TestWriteRequest test( + kAdminSubjectDescriptor, + ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), MOCK_ATTRIBUTE_ID_FOR_NULLABLE_TYPE(ZCL_INT32U_ATTRIBUTE_TYPE))); + + using NumericType = NumericAttributeTraits; + using NullableType = chip::app::DataModel::Nullable; + 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)); +} + +TEST(TestCodegenModelViaMocks, EmberTestWriteOutOfRepresentableRangeOddInteger) +{ + UseMockNodeConfig config(gTestNodeConfig); + chip::app::CodegenDataModel model; + ScopedMockAccessControl accessControl; + + TestWriteRequest test(kAdminSubjectDescriptor, + ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), + MOCK_ATTRIBUTE_ID_FOR_NON_NULLABLE_TYPE(ZCL_INT24U_ATTRIBUTE_TYPE))); + + using NumericType = NumericAttributeTraits; + using NullableType = chip::app::DataModel::Nullable; + AttributeValueDecoder decoder = test.DecoderFor(0x1223344); + + // write should fail: written value is not in range + // NOTE: this matches legacy behaviour, however realistically maybe ConstraintError would be more correct + ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_ERROR_INVALID_ARGUMENT); +} + +TEST(TestCodegenModelViaMoceNullValueToNullables, EmberAttributeWriteBasicTypesLowestValue) { TestEmberScalarTypeWrite(-127); TestEmberScalarTypeWrite(-32767); From 0e4398b08b2cf28fda56529e40b79c7404a1a9ee Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 13:43:45 -0400 Subject: [PATCH 43/74] Comment update --- src/app/data-model-interface/DataModel.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/app/data-model-interface/DataModel.h b/src/app/data-model-interface/DataModel.h index 0f4ce2121c7868..d673b79aac72a9 100644 --- a/src/app/data-model-interface/DataModel.h +++ b/src/app/data-model-interface/DataModel.h @@ -88,6 +88,7 @@ class DataModel : public DataModelMetadataTree /// WriteAttribute is REQUIRED to perform: /// - 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 codes /// CHIP_IM_GLOBAL_STATUS(code): From 41403a46e27758635d4795dc6871ccf4d8e9bc02 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 13:44:53 -0400 Subject: [PATCH 44/74] Fix comment --- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index 365eb67a53536e..bb32a38fb61cde 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -158,10 +158,8 @@ CHIP_ERROR DecodeIntoEmberBuffer(AttributeValueDecoder & decoder, bool isNullabl { // This guards against trying to encode something that overlaps nullable, for example // Nullable(0xFF) is not representable because 0xFF is the encoding of NULL in ember - // as well as odd-sized integers. - // - // It also guargs against out of range (e.g. full 32-bit value like 0x11223344 to written to 3-byte - // odd-sized integger) + // as well as odd-sized integers (e.g. full 32-bit value like 0x11223344 cannot be written + // to a 3-byte odd-sized integger). VerifyOrReturnError(Traits::CanRepresentValue(isNullable, *workingValue), CHIP_ERROR_INVALID_ARGUMENT); Traits::WorkingToStorage(*workingValue, storageValue); } From 39da04fc3e836fbef61ea4c2f68776e8fb32f0bd Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 13:52:06 -0400 Subject: [PATCH 45/74] One more test for more coverage --- .../tests/TestCodegenModelViaMocks.cpp | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp index 4e7e58f1a969ca..874771d71c6da2 100644 --- a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp +++ b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp @@ -1958,7 +1958,7 @@ TEST(TestCodegenModelViaMocks, EmberTestWriteReservedNullPlaceholderToNullable) ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_IM_GLOBAL_STATUS(ConstraintError)); } -TEST(TestCodegenModelViaMocks, EmberTestWriteOutOfRepresentableRangeOddInteger) +TEST(TestCodegenModelViaMocks, EmberTestWriteOutOfRepresentableRangeOddIntegerNonNullable) { UseMockNodeConfig config(gTestNodeConfig); chip::app::CodegenDataModel model; @@ -1977,6 +1977,25 @@ TEST(TestCodegenModelViaMocks, EmberTestWriteOutOfRepresentableRangeOddInteger) ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_ERROR_INVALID_ARGUMENT); } +TEST(TestCodegenModelViaMocks, EmberTestWriteOutOfRepresentableRangeOddIntegerNullable) +{ + UseMockNodeConfig config(gTestNodeConfig); + chip::app::CodegenDataModel model; + ScopedMockAccessControl accessControl; + + TestWriteRequest test(kAdminSubjectDescriptor, + ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), + MOCK_ATTRIBUTE_ID_FOR_NULLABLE_TYPE(ZCL_INT24U_ATTRIBUTE_TYPE))); + + using NumericType = NumericAttributeTraits; + using NullableType = chip::app::DataModel::Nullable; + AttributeValueDecoder decoder = test.DecoderFor(0x1223344); + + // write should fail: written value is not in range + // NOTE: this matches legacy behaviour, however realistically maybe ConstraintError would be more correct + ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_ERROR_INVALID_ARGUMENT); +} + TEST(TestCodegenModelViaMoceNullValueToNullables, EmberAttributeWriteBasicTypesLowestValue) { TestEmberScalarTypeWrite(-127); From c5feac752ab62761b511fc100f1630bfe5cf8d6d Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 13:57:12 -0400 Subject: [PATCH 46/74] Also cover writing non-null value to nullable attribute --- .../tests/TestCodegenModelViaMocks.cpp | 56 +++++++++++++------ 1 file changed, 40 insertions(+), 16 deletions(-) diff --git a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp index 874771d71c6da2..37f368dc09beb2 100644 --- a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp +++ b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp @@ -714,23 +714,47 @@ void TestEmberScalarTypeWrite(const typename NumericAttributeTraits::WorkingT chip::app::CodegenDataModel model; ScopedMockAccessControl accessControl; - TestWriteRequest test( - kAdminSubjectDescriptor, - ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), MOCK_ATTRIBUTE_ID_FOR_NON_NULLABLE_TYPE(ZclType))); - AttributeValueDecoder decoder = test.DecoderFor(value); + // non-nullable test + { + TestWriteRequest test( + kAdminSubjectDescriptor, + ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), MOCK_ATTRIBUTE_ID_FOR_NON_NULLABLE_TYPE(ZclType))); + AttributeValueDecoder decoder = test.DecoderFor(value); - // write should succeed - ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); + // write should succeed + ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); - // Validate data after write - chip::ByteSpan writtenData = Test::GetEmberBuffer(); + // Validate data after write + chip::ByteSpan writtenData = Test::GetEmberBuffer(); - typename NumericAttributeTraits::StorageType storage; - ASSERT_GE(writtenData.size(), sizeof(storage)); - memcpy(&storage, writtenData.data(), sizeof(storage)); - typename NumericAttributeTraits::WorkingType actual = NumericAttributeTraits::StorageToWorking(storage); + typename NumericAttributeTraits::StorageType storage; + ASSERT_GE(writtenData.size(), sizeof(storage)); + memcpy(&storage, writtenData.data(), sizeof(storage)); + typename NumericAttributeTraits::WorkingType actual = NumericAttributeTraits::StorageToWorking(storage); - ASSERT_EQ(actual, value); + ASSERT_EQ(actual, value); + } + + // nullable test + { + TestWriteRequest test( + kAdminSubjectDescriptor, + ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), MOCK_ATTRIBUTE_ID_FOR_NULLABLE_TYPE(ZclType))); + AttributeValueDecoder decoder = test.DecoderFor(value); + + // write should succeed + ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); + + // Validate data after write + chip::ByteSpan writtenData = Test::GetEmberBuffer(); + + typename NumericAttributeTraits::StorageType storage; + ASSERT_GE(writtenData.size(), sizeof(storage)); + memcpy(&storage, writtenData.data(), sizeof(storage)); + typename NumericAttributeTraits::WorkingType actual = NumericAttributeTraits::StorageToWorking(storage); + + ASSERT_EQ(actual, value); + } } template @@ -1983,9 +2007,9 @@ TEST(TestCodegenModelViaMocks, EmberTestWriteOutOfRepresentableRangeOddIntegerNu chip::app::CodegenDataModel model; ScopedMockAccessControl accessControl; - TestWriteRequest test(kAdminSubjectDescriptor, - ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), - MOCK_ATTRIBUTE_ID_FOR_NULLABLE_TYPE(ZCL_INT24U_ATTRIBUTE_TYPE))); + TestWriteRequest test( + kAdminSubjectDescriptor, + ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), MOCK_ATTRIBUTE_ID_FOR_NULLABLE_TYPE(ZCL_INT24U_ATTRIBUTE_TYPE))); using NumericType = NumericAttributeTraits; using NullableType = chip::app::DataModel::Nullable; From 6b05337679ec269cb5f8fc37598780788ad27f9a Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 15:14:58 -0400 Subject: [PATCH 47/74] Fix the ember string usage --- src/app/codegen-data-model/CodegenDataModel_Read.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Read.cpp b/src/app/codegen-data-model/CodegenDataModel_Read.cpp index 6325eb150c8078..c0df05e71b2391 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Read.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Read.cpp @@ -37,7 +37,6 @@ #include #include #include -#include #include #include #include @@ -90,7 +89,7 @@ struct ShortPascalString using LengthType = uint8_t; static constexpr LengthType kNullLength = 0xFF; - static LengthType GetLength(const uint8_t * buffer) { return emberAfStringLength(buffer); } + static LengthType GetLength(const uint8_t * buffer) { return *buffer; } }; /// Metadata of what a ember/pascal LONG string means (prepended by a u16 length) From 46a5ad20b22a08c060c53cc5497ac4aa2d4146d3 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 15:15:37 -0400 Subject: [PATCH 48/74] Update src/app/codegen-data-model/CodegenDataModel_Read.cpp Co-authored-by: Boris Zbarsky --- src/app/codegen-data-model/CodegenDataModel_Read.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Read.cpp b/src/app/codegen-data-model/CodegenDataModel_Read.cpp index c0df05e71b2391..4beda29d2d6818 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Read.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Read.cpp @@ -117,7 +117,7 @@ template std::optional ExtractEmberString(ByteSpan data) { VerifyOrDie(sizeof(typename ENCODING::LengthType) <= data.size()); - typename ENCODING::LengthType len = ENCODING::GetLength(data.data()); + auto len = ENCODING::GetLength(data.data()); if (len == ENCODING::kNullLength) { From 8631b41cab91e2ca88284f65b87a2568928e4da6 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 15:16:18 -0400 Subject: [PATCH 49/74] Update src/app/codegen-data-model/CodegenDataModel_Read.cpp Co-authored-by: Boris Zbarsky --- src/app/codegen-data-model/CodegenDataModel_Read.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Read.cpp b/src/app/codegen-data-model/CodegenDataModel_Read.cpp index 4beda29d2d6818..70c4a98924ab7b 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Read.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Read.cpp @@ -269,7 +269,7 @@ CHIP_ERROR CodegenDataModel::ReadAttribute(const InteractionModel::ReadAttribute ReturnErrorCodeIf(err != CHIP_ERROR_ACCESS_DENIED, err); // Implementation of 8.4.3.2 of the spec for path expansion - if (request.path.mExpanded && (err == CHIP_ERROR_ACCESS_DENIED)) + if (request.path.mExpanded) { return CHIP_NO_ERROR; } From 5a4bd3cc5c6c6c8b285576a8beab310e330d401d Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 15:17:59 -0400 Subject: [PATCH 50/74] Remove duplicate code --- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index bb32a38fb61cde..ed6e1a3a475eae 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -336,14 +336,11 @@ CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttribu AttributeAccessInterface * aai = GetAttributeAccessOverride(request.path.mEndpointId, request.path.mClusterId); std::optional aai_result = TryWriteViaAccessInterface(request.path, aai, decoder); - if (aai_result.has_value()) { return *aai_result; } - ReturnErrorCodeIf(aai_result.has_value(), *aai_result); - ReturnErrorOnFailure(DecodeValueIntoEmberBuffer(decoder, *attributeMetadata, gEmberAttributeIOBufferSpan)); EmberAfAttributeSearchRecord record; From 6e0d9d154fc223cbec36e3d33d1615b283663924 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 15:19:05 -0400 Subject: [PATCH 51/74] Update src/app/codegen-data-model/CodegenDataModel_Write.cpp Co-authored-by: Boris Zbarsky --- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index ed6e1a3a475eae..9b5f613a1a8bd2 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -328,7 +328,7 @@ CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttribu if (request.path.mDataVersion.Value() != clusterInfo->dataVersion) { - ChipLogError(DataManagement, "Write Version mismatch for Endpoint %x, Cluster " ChipLogFormatMEI, + ChipLogError(DataManagement, "Write Version mismatch for Endpoint 0x%x, Cluster " ChipLogFormatMEI, request.path.mEndpointId, ChipLogValueMEI(request.path.mClusterId)); return CHIP_IM_GLOBAL_STATUS(DataVersionMismatch); } From 4bdc2eed3d6dd2f521e8cf026b6a0cf60ad531ed Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 15:21:12 -0400 Subject: [PATCH 52/74] Remove chip::app:: prefix in unit test since we have a top level using --- .../tests/TestCodegenModelViaMocks.cpp | 137 +++++++++--------- 1 file changed, 67 insertions(+), 70 deletions(-) diff --git a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp index 37f368dc09beb2..80f44f65652c2b 100644 --- a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp +++ b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp @@ -384,7 +384,7 @@ CHIP_ERROR DecodeList(TLV::TLVReader & reader, std::vector & out) ReturnErrorOnFailure(err); T value; - ReturnErrorOnFailure(chip::app::DataModel::Decode(reader, value)); + ReturnErrorOnFailure(DataModel::Decode(reader, value)); out.emplace_back(std::move(value)); } } @@ -560,7 +560,7 @@ struct TestReadRequest request.path = path; } - std::unique_ptr StartEncoding(chip::app::InteractionModel::DataModel * model, + std::unique_ptr StartEncoding(InteractionModel::DataModel * model, AttributeEncodeState state = AttributeEncodeState()) { std::optional info = model->GetClusterInfo(request.path); @@ -616,7 +616,7 @@ struct TestWriteRequest // - END_STRUCT TLV::TLVType outerContainerType; VerifyOrDie(writer.StartContainer(TLV::AnonymousTag(), TLV::kTLVType_Structure, outerContainerType) == CHIP_NO_ERROR); - VerifyOrDie(chip::app::DataModel::Encode(writer, TLV::ContextTag(1), value) == CHIP_NO_ERROR); + VerifyOrDie(DataModel::Encode(writer, TLV::ContextTag(1), value) == CHIP_NO_ERROR); VerifyOrDie(writer.EndContainer(outerContainerType) == CHIP_NO_ERROR); VerifyOrDie(writer.Finalize() == CHIP_NO_ERROR); @@ -643,7 +643,7 @@ template void TestEmberScalarTypeRead(typename NumericAttributeTraits::WorkingType value) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; TestReadRequest testRequest( @@ -669,8 +669,7 @@ void TestEmberScalarTypeRead(typename NumericAttributeTraits::WorkingType val ASSERT_EQ(encodedData.attributePath, testRequest.request.path); typename NumericAttributeTraits::WorkingType actual; - ASSERT_EQ(chip::app::DataModel::Decode::WorkingType>(encodedData.dataReader, actual), - CHIP_NO_ERROR); + ASSERT_EQ(DataModel::Decode::WorkingType>(encodedData.dataReader, actual), CHIP_NO_ERROR); ASSERT_EQ(actual, value); } @@ -678,7 +677,7 @@ template void TestEmberScalarNullRead() { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; TestReadRequest testRequest( @@ -702,8 +701,8 @@ void TestEmberScalarNullRead() DecodedAttributeData & encodedData = attribute_data[0]; ASSERT_EQ(encodedData.attributePath, testRequest.request.path); - chip::app::DataModel::Nullable::WorkingType> actual; - ASSERT_EQ(chip::app::DataModel::Decode(encodedData.dataReader, actual), CHIP_NO_ERROR); + DataModel::Nullable::WorkingType> actual; + ASSERT_EQ(DataModel::Decode(encodedData.dataReader, actual), CHIP_NO_ERROR); ASSERT_TRUE(actual.IsNull()); } @@ -711,7 +710,7 @@ template void TestEmberScalarTypeWrite(const typename NumericAttributeTraits::WorkingType value) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; // non-nullable test @@ -761,14 +760,14 @@ template void TestEmberScalarNullWrite() { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; TestWriteRequest test(kAdminSubjectDescriptor, ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), MOCK_ATTRIBUTE_ID_FOR_NULLABLE_TYPE(ZclType))); using NumericType = NumericAttributeTraits; - using NullableType = chip::app::DataModel::Nullable; + using NullableType = DataModel::Nullable; AttributeValueDecoder decoder = test.DecoderFor(NullableType()); // write should succeed @@ -789,7 +788,7 @@ template void TestEmberScalarTypeWriteNullValueToNullable() { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; TestWriteRequest test( @@ -797,7 +796,7 @@ void TestEmberScalarTypeWriteNullValueToNullable() ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), MOCK_ATTRIBUTE_ID_FOR_NON_NULLABLE_TYPE(ZclType))); using NumericType = NumericAttributeTraits; - using NullableType = chip::app::DataModel::Nullable; + using NullableType = DataModel::Nullable; AttributeValueDecoder decoder = test.DecoderFor(NullableType()); // write should fail: we are trying to write null @@ -821,7 +820,7 @@ void WriteLe16(void * buffer, uint16_t value) TEST(TestCodegenModelViaMocks, IterateOverEndpoints) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; // This iteration relies on the hard-coding that occurs when mock_ember is used EXPECT_EQ(model.FirstEndpoint(), kMockEndpoint1); @@ -849,7 +848,7 @@ TEST(TestCodegenModelViaMocks, IterateOverEndpoints) TEST(TestCodegenModelViaMocks, IterateOverClusters) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; chip::Test::ResetVersion(); @@ -912,7 +911,7 @@ TEST(TestCodegenModelViaMocks, GetClusterInfo) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; chip::Test::ResetVersion(); @@ -937,7 +936,7 @@ TEST(TestCodegenModelViaMocks, GetClusterInfo) TEST(TestCodegenModelViaMocks, IterateOverAttributes) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; // invalid paths should return in "no more data" ASSERT_FALSE(model.FirstAttribute(ConcreteClusterPath(kEndpointIdThatIsMissing, MockClusterId(1))).path.HasValidIds()); @@ -1008,7 +1007,7 @@ TEST(TestCodegenModelViaMocks, IterateOverAttributes) TEST(TestCodegenModelViaMocks, GetAttributeInfo) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; // various non-existent or invalid paths should return no info data ASSERT_FALSE( @@ -1035,7 +1034,7 @@ TEST(TestCodegenModelViaMocks, GetAttributeInfo) TEST(TestCodegenModelViaMocks, GlobalAttributeInfo) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; std::optional info = model.GetAttributeInfo( ConcreteAttributePath(kMockEndpoint1, MockClusterId(1), Clusters::Globals::Attributes::GeneratedCommandList::Id)); @@ -1050,7 +1049,7 @@ TEST(TestCodegenModelViaMocks, GlobalAttributeInfo) TEST(TestCodegenModelViaMocks, IterateOverAcceptedCommands) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; // invalid paths should return in "no more data" ASSERT_FALSE(model.FirstAcceptedCommand(ConcreteClusterPath(kEndpointIdThatIsMissing, MockClusterId(1))).path.HasValidIds()); @@ -1115,7 +1114,7 @@ TEST(TestCodegenModelViaMocks, IterateOverAcceptedCommands) TEST(TestCodegenModelViaMocks, AcceptedCommandInfo) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; // invalid paths should return in "no more data" ASSERT_FALSE(model.GetAcceptedCommandInfo(ConcreteCommandPath(kEndpointIdThatIsMissing, MockClusterId(1), 1)).has_value()); @@ -1147,7 +1146,7 @@ TEST(TestCodegenModelViaMocks, AcceptedCommandInfo) TEST(TestCodegenModelViaMocks, IterateOverGeneratedCommands) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; // invalid paths should return in "no more data" ASSERT_FALSE(model.FirstGeneratedCommand(ConcreteClusterPath(kEndpointIdThatIsMissing, MockClusterId(1))).HasValidIds()); @@ -1206,7 +1205,7 @@ TEST(TestCodegenModelViaMocks, IterateOverGeneratedCommands) TEST(TestCodegenModelViaMocks, EmberAttributeReadAclDeny) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; TestReadRequest testRequest(kDenySubjectDescriptor, @@ -1219,7 +1218,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeReadAclDeny) TEST(TestCodegenModelViaMocks, ReadForInvalidGlobalAttributePath) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; { @@ -1240,7 +1239,7 @@ TEST(TestCodegenModelViaMocks, ReadForInvalidGlobalAttributePath) TEST(TestCodegenModelViaMocks, EmberAttributeInvalidRead) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; // Invalid attribute @@ -1274,7 +1273,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeInvalidRead) TEST(TestCodegenModelViaMocks, EmberAttributePathExpansionAccessDeniedRead) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; TestReadRequest testRequest(kDenySubjectDescriptor, @@ -1292,7 +1291,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributePathExpansionAccessDeniedRead) TEST(TestCodegenModelViaMocks, AccessInterfaceUnsupportedRead) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; const ConcreteAttributePath kTestPath(kMockEndpoint3, MockClusterId(4), @@ -1396,7 +1395,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeReadNulls) TEST(TestCodegenModelViaMocks, EmberAttributeReadErrorReading) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; { @@ -1432,7 +1431,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeReadErrorReading) TEST(TestCodegenModelViaMocks, EmberAttributeReadNullOctetString) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; TestReadRequest testRequest(kAdminSubjectDescriptor, @@ -1459,15 +1458,15 @@ TEST(TestCodegenModelViaMocks, EmberAttributeReadNullOctetString) // data element should be null for the given 0xFFFF length ASSERT_EQ(encodedData.dataReader.GetType(), TLV::kTLVType_Null); - chip::app::DataModel::Nullable actual; - ASSERT_EQ(chip::app::DataModel::Decode(encodedData.dataReader, actual), CHIP_NO_ERROR); + DataModel::Nullable actual; + ASSERT_EQ(DataModel::Decode(encodedData.dataReader, actual), CHIP_NO_ERROR); ASSERT_TRUE(actual.IsNull()); } TEST(TestCodegenModelViaMocks, EmberAttributeReadOctetString) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; TestReadRequest testRequest( @@ -1506,7 +1505,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeReadOctetString) TEST(TestCodegenModelViaMocks, EmberAttributeReadLongOctetString) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; TestReadRequest testRequest(kAdminSubjectDescriptor, @@ -1543,7 +1542,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeReadLongOctetString) TEST(TestCodegenModelViaMocks, EmberAttributeReadShortString) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; TestReadRequest testRequest(kAdminSubjectDescriptor, @@ -1579,7 +1578,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeReadShortString) TEST(TestCodegenModelViaMocks, EmberAttributeReadLongString) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; TestReadRequest testRequest( @@ -1616,7 +1615,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeReadLongString) TEST(TestCodegenModelViaMocks, AttributeAccessInterfaceStructRead) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; const ConcreteAttributePath kStructPath(kMockEndpoint3, MockClusterId(4), @@ -1647,7 +1646,7 @@ TEST(TestCodegenModelViaMocks, AttributeAccessInterfaceStructRead) ASSERT_EQ(encodedData.dataReader.GetType(), TLV::kTLVType_Structure); Clusters::UnitTesting::Structs::SimpleStruct::DecodableType actual; - ASSERT_EQ(chip::app::DataModel::Decode(encodedData.dataReader, actual), CHIP_NO_ERROR); + ASSERT_EQ(DataModel::Decode(encodedData.dataReader, actual), CHIP_NO_ERROR); ASSERT_EQ(actual.a, 123); ASSERT_EQ(actual.b, true); @@ -1659,7 +1658,7 @@ TEST(TestCodegenModelViaMocks, AttributeAccessInterfaceStructRead) TEST(TestCodegenModelViaMocks, AttributeAccessInterfaceReadError) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; const ConcreteAttributePath kStructPath(kMockEndpoint3, MockClusterId(4), @@ -1674,7 +1673,7 @@ TEST(TestCodegenModelViaMocks, AttributeAccessInterfaceReadError) TEST(TestCodegenModelViaMocks, AttributeAccessInterfaceListRead) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; const ConcreteAttributePath kStructPath(kMockEndpoint3, MockClusterId(4), @@ -1726,7 +1725,7 @@ TEST(TestCodegenModelViaMocks, AttributeAccessInterfaceListRead) TEST(TestCodegenModelViaMocks, AttributeAccessInterfaceListOverflowRead) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; const ConcreteAttributePath kStructPath(kMockEndpoint3, MockClusterId(4), @@ -1785,7 +1784,7 @@ TEST(TestCodegenModelViaMocks, AttributeAccessInterfaceListOverflowRead) TEST(TestCodegenModelViaMocks, AttributeAccessInterfaceListIncrementalRead) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; const ConcreteAttributePath kStructPath(kMockEndpoint3, MockClusterId(4), @@ -1834,7 +1833,7 @@ TEST(TestCodegenModelViaMocks, AttributeAccessInterfaceListIncrementalRead) ASSERT_EQ(encodedData.dataReader.GetType(), TLV::kTLVType_Structure); Clusters::UnitTesting::Structs::SimpleStruct::DecodableType actual; - ASSERT_EQ(chip::app::DataModel::Decode(encodedData.dataReader, actual), CHIP_NO_ERROR); + ASSERT_EQ(DataModel::Decode(encodedData.dataReader, actual), CHIP_NO_ERROR); ASSERT_EQ(actual.a, static_cast((i + kEncodeIndexStart) & 0xFF)); ASSERT_EQ(actual.b, true); @@ -1847,7 +1846,7 @@ TEST(TestCodegenModelViaMocks, AttributeAccessInterfaceListIncrementalRead) TEST(TestCodegenModelViaMocks, ReadGlobalAttributeAttributeList) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; TestReadRequest testRequest(kAdminSubjectDescriptor, @@ -1903,7 +1902,7 @@ TEST(TestCodegenModelViaMocks, ReadGlobalAttributeAttributeList) TEST(TestCodegenModelViaMocks, EmberAttributeWriteAclDeny) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; TestWriteRequest test(kDenySubjectDescriptor, ConcreteDataAttributePath(kMockEndpoint1, MockClusterId(1), MockAttributeId(10))); @@ -1967,7 +1966,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteInvalidValueToNullable) TEST(TestCodegenModelViaMocks, EmberTestWriteReservedNullPlaceholderToNullable) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; TestWriteRequest test( @@ -1975,7 +1974,7 @@ TEST(TestCodegenModelViaMocks, EmberTestWriteReservedNullPlaceholderToNullable) ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), MOCK_ATTRIBUTE_ID_FOR_NULLABLE_TYPE(ZCL_INT32U_ATTRIBUTE_TYPE))); using NumericType = NumericAttributeTraits; - using NullableType = chip::app::DataModel::Nullable; + using NullableType = DataModel::Nullable; AttributeValueDecoder decoder = test.DecoderFor(0xFFFFFFFF); // write should fail: we are trying to write null which is out of range @@ -1985,7 +1984,7 @@ TEST(TestCodegenModelViaMocks, EmberTestWriteReservedNullPlaceholderToNullable) TEST(TestCodegenModelViaMocks, EmberTestWriteOutOfRepresentableRangeOddIntegerNonNullable) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; TestWriteRequest test(kAdminSubjectDescriptor, @@ -1993,7 +1992,7 @@ TEST(TestCodegenModelViaMocks, EmberTestWriteOutOfRepresentableRangeOddIntegerNo MOCK_ATTRIBUTE_ID_FOR_NON_NULLABLE_TYPE(ZCL_INT24U_ATTRIBUTE_TYPE))); using NumericType = NumericAttributeTraits; - using NullableType = chip::app::DataModel::Nullable; + using NullableType = DataModel::Nullable; AttributeValueDecoder decoder = test.DecoderFor(0x1223344); // write should fail: written value is not in range @@ -2004,7 +2003,7 @@ TEST(TestCodegenModelViaMocks, EmberTestWriteOutOfRepresentableRangeOddIntegerNo TEST(TestCodegenModelViaMocks, EmberTestWriteOutOfRepresentableRangeOddIntegerNullable) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; TestWriteRequest test( @@ -2012,7 +2011,7 @@ TEST(TestCodegenModelViaMocks, EmberTestWriteOutOfRepresentableRangeOddIntegerNu ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), MOCK_ATTRIBUTE_ID_FOR_NULLABLE_TYPE(ZCL_INT24U_ATTRIBUTE_TYPE))); using NumericType = NumericAttributeTraits; - using NullableType = chip::app::DataModel::Nullable; + using NullableType = DataModel::Nullable; AttributeValueDecoder decoder = test.DecoderFor(0x1223344); // write should fail: written value is not in range @@ -2059,7 +2058,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteNulls) TEST(TestCodegenModelViaMocks, EmberAttributeWriteShortString) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; TestWriteRequest test(kAdminSubjectDescriptor, @@ -2076,7 +2075,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteShortString) TEST(TestCodegenModelViaMocks, EmberAttributeWriteLongString) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; TestWriteRequest test(kAdminSubjectDescriptor, @@ -2097,14 +2096,13 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteLongString) TEST(TestCodegenModelViaMocks, EmberAttributeWriteNullableLongStringValue) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; TestWriteRequest test(kAdminSubjectDescriptor, ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), MOCK_ATTRIBUTE_ID_FOR_NULLABLE_TYPE(ZCL_LONG_CHAR_STRING_ATTRIBUTE_TYPE))); - AttributeValueDecoder decoder = - test.DecoderFor>(chip::app::DataModel::MakeNullable("text"_span)); + AttributeValueDecoder decoder = test.DecoderFor>(chip::app::DataModel::MakeNullable("text"_span)); ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); chip::ByteSpan writtenData = GetEmberBuffer(); @@ -2119,14 +2117,13 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteNullableLongStringValue) TEST(TestCodegenModelViaMocks, EmberAttributeWriteLongNullableStringNull) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; TestWriteRequest test(kAdminSubjectDescriptor, ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), MOCK_ATTRIBUTE_ID_FOR_NULLABLE_TYPE(ZCL_LONG_CHAR_STRING_ATTRIBUTE_TYPE))); - AttributeValueDecoder decoder = - test.DecoderFor>(chip::app::DataModel::Nullable()); + AttributeValueDecoder decoder = test.DecoderFor>(chip::app::DataModel::Nullable()); ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_NO_ERROR); chip::ByteSpan writtenData = GetEmberBuffer(); @@ -2137,7 +2134,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteLongNullableStringNull) TEST(TestCodegenModelViaMocks, EmberAttributeWriteShortBytes) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; TestWriteRequest test(kAdminSubjectDescriptor, @@ -2159,7 +2156,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteShortBytes) TEST(TestCodegenModelViaMocks, EmberAttributeWriteLongBytes) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; TestWriteRequest test(kAdminSubjectDescriptor, @@ -2183,7 +2180,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteLongBytes) TEST(TestCodegenModelViaMocks, EmberAttributeWriteTimedWrite) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; TestWriteRequest test(kAdminSubjectDescriptor, ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), kAttributeIdTimedWrite)); @@ -2199,7 +2196,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteTimedWrite) TEST(TestCodegenModelViaMocks, EmberAttributeWriteReadOnlyAttribute) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; TestWriteRequest test(kAdminSubjectDescriptor, ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), kAttributeIdReadOnly)); @@ -2215,7 +2212,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteReadOnlyAttribute) TEST(TestCodegenModelViaMocks, EmberAttributeWriteDataVersion) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; TestWriteRequest test(kAdminSubjectDescriptor, @@ -2242,7 +2239,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteDataVersion) TEST(TestCodegenModelViaMocks, WriteToInvalidPath) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; { @@ -2266,7 +2263,7 @@ TEST(TestCodegenModelViaMocks, WriteToInvalidPath) TEST(TestCodegenModelViaMocks, WriteToGlobalAttribute) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; TestWriteRequest test(kAdminSubjectDescriptor, ConcreteAttributePath(kMockEndpoint1, MockClusterId(1), AttributeList::Id)); @@ -2277,7 +2274,7 @@ TEST(TestCodegenModelViaMocks, WriteToGlobalAttribute) TEST(TestCodegenModelViaMocks, EmberWriteFailure) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; TestWriteRequest test(kAdminSubjectDescriptor, @@ -2301,7 +2298,7 @@ TEST(TestCodegenModelViaMocks, EmberWriteFailure) TEST(TestCodegenModelViaMocks, EmberWriteAttributeAccessInterfaceTest) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; const ConcreteAttributePath kStructPath(kMockEndpoint3, MockClusterId(4), @@ -2333,7 +2330,7 @@ TEST(TestCodegenModelViaMocks, EmberWriteAttributeAccessInterfaceTest) TEST(TestCodegenModelViaMocks, EmberWriteAttributeAccessInterfaceReturningError) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; const ConcreteAttributePath kStructPath(kMockEndpoint3, MockClusterId(4), @@ -2356,7 +2353,7 @@ TEST(TestCodegenModelViaMocks, EmberWriteAttributeAccessInterfaceReturningError) TEST(TestCodegenModelViaMocks, EmberWriteInvalidDataType) { UseMockNodeConfig config(gTestNodeConfig); - chip::app::CodegenDataModel model; + CodegenDataModel model; ScopedMockAccessControl accessControl; // Embed specifically DOES NOT support structures. Without AAI, we expect a constraint error From f6e1bb90203fea0b55c098b83ca68219c9575a4d Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 15:22:51 -0400 Subject: [PATCH 53/74] Fix copy & paste encode to decode --- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index 9b5f613a1a8bd2..1206fcd2804657 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -92,12 +92,12 @@ struct LongPascalString static_assert(sizeof(ShortPascalString::LengthType) == 1); static_assert(sizeof(LongPascalString::LengthType) == 2); -/// Encode the value stored in 'decoder' into an ember format string 'out' +/// Decode the value stored in 'decoder' into an ember format span 'out' /// -/// The value encoded will be of type T (e.g. CharSpan or ByteSpan) and it will be encoded +/// The value decoded will be of type T (e.g. CharSpan or ByteSpan) and it will be decoded /// via the given ENCODING (i.e. ShortPascalString or LongPascalString) /// -/// isNullable defines if the value of NULL is allowed to be encoded. +/// isNullable defines if the value of NULL is allowed to be decoded. template CHIP_ERROR DecodeStringLikeIntoEmberBuffer(AttributeValueDecoder decoder, bool isNullable, MutableByteSpan out) { From 4bd7e28ecdb05ad1ec87da9b16a77a9a3edeeac3 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 15:23:46 -0400 Subject: [PATCH 54/74] Replace decoded with converted --- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index 1206fcd2804657..b3d193b0a79a41 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -92,12 +92,12 @@ struct LongPascalString static_assert(sizeof(ShortPascalString::LengthType) == 1); static_assert(sizeof(LongPascalString::LengthType) == 2); -/// Decode the value stored in 'decoder' into an ember format span 'out' +/// Convert the value stored in 'decoder' into an ember format span 'out' /// -/// The value decoded will be of type T (e.g. CharSpan or ByteSpan) and it will be decoded +/// The value converted will be of type T (e.g. CharSpan or ByteSpan) and it will be converted /// via the given ENCODING (i.e. ShortPascalString or LongPascalString) /// -/// isNullable defines if the value of NULL is allowed to be decoded. +/// isNullable defines if the value of NULL is allowed to be converted. template CHIP_ERROR DecodeStringLikeIntoEmberBuffer(AttributeValueDecoder decoder, bool isNullable, MutableByteSpan out) { From 8e2383c9417a039e61d6b73a9bc6d2bb498852ac Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 15:24:28 -0400 Subject: [PATCH 55/74] Update src/app/codegen-data-model/CodegenDataModel_Write.cpp Co-authored-by: Boris Zbarsky --- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index b3d193b0a79a41..1db2befd7f4e41 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -123,7 +123,7 @@ CHIP_ERROR DecodeStringLikeIntoEmberBuffer(AttributeValueDecoder decoder, bool i ReturnErrorOnFailure(decoder.Decode(workingValue)); } - typename ENCODING::LengthType len = static_cast(workingValue.size()); + auto len = static_cast(workingValue.size()); VerifyOrReturnError(out.size() >= sizeof(len) + len, CHIP_ERROR_BUFFER_TOO_SMALL); uint8_t * output_buffer = out.data(); From 575b492e9d44081c4042db6fda0317fead5c6733 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 15:27:28 -0400 Subject: [PATCH 56/74] Start using Failure for invalid data type instead of unsupported read. I do not expect this code path to actually be hit much --- src/app/codegen-data-model/CodegenDataModel_Read.cpp | 2 +- src/app/util/ember-compatibility-functions.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Read.cpp b/src/app/codegen-data-model/CodegenDataModel_Read.cpp index 70c4a98924ab7b..3d9ad41ff4751a 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Read.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Read.cpp @@ -237,7 +237,7 @@ CHIP_ERROR EncodeEmberValue(ByteSpan data, const EmberAfAttributeMetadata * meta return EncodeStringLike(data, isNullable, encoder); default: ChipLogError(DataManagement, "Attribute type 0x%x not handled", static_cast(metadata->attributeType)); - return CHIP_IM_GLOBAL_STATUS(ConstraintError); + return CHIP_IM_GLOBAL_STATUS(Failure); } } diff --git a/src/app/util/ember-compatibility-functions.cpp b/src/app/util/ember-compatibility-functions.cpp index cc3185f78a5df6..676197be9ab2f2 100644 --- a/src/app/util/ember-compatibility-functions.cpp +++ b/src/app/util/ember-compatibility-functions.cpp @@ -570,7 +570,7 @@ CHIP_ERROR ReadSingleClusterData(const SubjectDescriptor & aSubjectDescriptor, b } default: ChipLogError(DataManagement, "Attribute type 0x%x not handled", static_cast(attributeType)); - status = Status::UnsupportedRead; + status = Status::Failure; } } From 7b5bb9f22208fb59eda973ae1d743bf0f6567ba9 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 15:30:10 -0400 Subject: [PATCH 57/74] Fix comments --- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index 1db2befd7f4e41..90bfe3da159d62 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -136,9 +136,9 @@ CHIP_ERROR DecodeStringLikeIntoEmberBuffer(AttributeValueDecoder decoder, bool i return CHIP_NO_ERROR; } -/// Encodes a numeric data value of type T from the given ember-encoded buffer `data`. +/// Decodes a numeric data value of type T from the `decoder` into a ember-encoded buffer `out` /// -/// isNullable defines if the value of NULL is allowed to be encoded. +/// isNullable defines if the value of NULL is allowed to be decoded. template CHIP_ERROR DecodeIntoEmberBuffer(AttributeValueDecoder & decoder, bool isNullable, MutableByteSpan out) { From da1aae4e168d2c696b0710e82a04573e1e0d2c28 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 15:34:09 -0400 Subject: [PATCH 58/74] Updated encode/decode comment --- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index 90bfe3da159d62..ebd155b2772138 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -156,7 +156,7 @@ CHIP_ERROR DecodeIntoEmberBuffer(AttributeValueDecoder & decoder, bool isNullabl } else { - // This guards against trying to encode something that overlaps nullable, for example + // This guards against trying to decode something that overlaps nullable, for example // Nullable(0xFF) is not representable because 0xFF is the encoding of NULL in ember // as well as odd-sized integers (e.g. full 32-bit value like 0x11223344 cannot be written // to a 3-byte odd-sized integger). From dc1c6586259b2d0303940c0868afcdeef8d26169 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 15:35:38 -0400 Subject: [PATCH 59/74] Use failure instead of constraint error --- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 2 +- .../codegen-data-model/tests/TestCodegenModelViaMocks.cpp | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index ebd155b2772138..ec135540f232d9 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -249,7 +249,7 @@ CHIP_ERROR DecodeValueIntoEmberBuffer(AttributeValueDecoder & decoder, const Emb return DecodeStringLikeIntoEmberBuffer(decoder, isNullable, out); default: ChipLogError(DataManagement, "Attribute type 0x%x not handled", static_cast(metadata->attributeType)); - return CHIP_IM_GLOBAL_STATUS(ConstraintError); + return CHIP_IM_GLOBAL_STATUS(Failure); } } diff --git a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp index 80f44f65652c2b..e9c7fe7a26ed6d 100644 --- a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp +++ b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp @@ -2356,7 +2356,6 @@ TEST(TestCodegenModelViaMocks, EmberWriteInvalidDataType) CodegenDataModel model; ScopedMockAccessControl accessControl; - // Embed specifically DOES NOT support structures. Without AAI, we expect a constraint error const ConcreteAttributePath kStructPath(kMockEndpoint3, MockClusterId(4), MOCK_ATTRIBUTE_ID_FOR_NON_NULLABLE_TYPE(ZCL_STRUCT_ATTRIBUTE_TYPE)); @@ -2370,5 +2369,8 @@ TEST(TestCodegenModelViaMocks, EmberWriteInvalidDataType) }; AttributeValueDecoder decoder = test.DecoderFor(testValue); - ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_IM_GLOBAL_STATUS(ConstraintError)); + + // 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)); } From 4dab89be685e7231cdaf4e0e331911ccdfb534e6 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 16:01:50 -0400 Subject: [PATCH 60/74] Update src/app/codegen-data-model/CodegenDataModel_Write.cpp Co-authored-by: Boris Zbarsky --- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index ec135540f232d9..2adbdaa85f3774 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -258,7 +258,7 @@ CHIP_ERROR DecodeValueIntoEmberBuffer(AttributeValueDecoder & decoder, const Emb CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttributeRequest & request, AttributeValueDecoder & decoder) { - ChipLogDetail(DataManagement, "Writing attribute: Cluster=" ChipLogFormatMEI " Endpoint=%x AttributeId=" ChipLogFormatMEI, + 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 From e39cb80903f282a83d95741dff8e59a6d4ff2115 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 16:02:04 -0400 Subject: [PATCH 61/74] Update src/app/codegen-data-model/CodegenDataModel_Write.cpp Co-authored-by: Boris Zbarsky --- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index 2adbdaa85f3774..10e78b252650d9 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -293,7 +293,7 @@ CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttribu // All the global attributes that we do not have metadata for are // read-only. Specifically only the following list-based attributes match the - // "global attributes not in metadata" (see GlobalAttributes.h :: GlobalAttributesNotInMetadat): + // "global attributes not in metadata" (see GlobalAttributes.h :: GlobalAttributesNotInMetadata): // - AttributeList // - EventList // - AcceptedCommands From f58fee8a97a8b719681c89ab17aef1b163ad6c95 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 16:02:21 -0400 Subject: [PATCH 62/74] Update src/app/codegen-data-model/CodegenDataModel_Write.cpp Co-authored-by: Boris Zbarsky --- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index 10e78b252650d9..cf63cf844e9d7b 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -321,7 +321,7 @@ CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttribu std::optional clusterInfo = GetClusterInfo(request.path); if (!clusterInfo.has_value()) { - ChipLogError(DataManagement, "Unable to get cluster info for Endpoint %x, Cluster " ChipLogFormatMEI, + ChipLogError(DataManagement, "Unable to get cluster info for Endpoint 0x%x, Cluster " ChipLogFormatMEI, request.path.mEndpointId, ChipLogValueMEI(request.path.mClusterId)); return CHIP_ERROR_NOT_FOUND; } From 8e87177a91402ec4c7722041d4702fb4d1515104 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 16:05:54 -0400 Subject: [PATCH 63/74] Use dataversion mismatch for write without a version --- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index cf63cf844e9d7b..023116057b6739 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -323,7 +323,7 @@ CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttribu { ChipLogError(DataManagement, "Unable to get cluster info for Endpoint 0x%x, Cluster " ChipLogFormatMEI, request.path.mEndpointId, ChipLogValueMEI(request.path.mClusterId)); - return CHIP_ERROR_NOT_FOUND; + return CHIP_IM_GLOBAL_STATUS(DataVersionMismatch); } if (request.path.mDataVersion.Value() != clusterInfo->dataVersion) From 572a7f3608297d55e65d99bab202198cbaf238ab Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 16:08:29 -0400 Subject: [PATCH 64/74] Add extra IsGlobalAttribute check --- src/app/codegen-data-model/EmberMetadata.cpp | 21 ++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/app/codegen-data-model/EmberMetadata.cpp b/src/app/codegen-data-model/EmberMetadata.cpp index a39c3cb1188e61..9114196a377906 100644 --- a/src/app/codegen-data-model/EmberMetadata.cpp +++ b/src/app/codegen-data-model/EmberMetadata.cpp @@ -30,18 +30,23 @@ std::variant FindAttributeMetadata(const ConcreteAttributePath & aPath) { - for (auto & attr : GlobalAttributesNotInMetadata) + if (IsGlobalAttribute(aPath.mAttributeId)) { - if (attr == aPath.mAttributeId) + // Global list attribute check first: during path expansion a lot of attributes + // will actually be global attributes (so not too much of a performance hit) + for (auto & attr : GlobalAttributesNotInMetadata) { - const EmberAfCluster * cluster = emberAfFindServerCluster(aPath.mEndpointId, aPath.mClusterId); - if (cluster == nullptr) + if (attr == aPath.mAttributeId) { - return (emberAfFindEndpointType(aPath.mEndpointId) == nullptr) ? CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint) - : CHIP_IM_GLOBAL_STATUS(UnsupportedCluster); - } + 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 cluster; + return cluster; + } } } const EmberAfAttributeMetadata * metadata = From 84fb496a67d49e77f9ccd9ad4eedec7384fb10dd Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 16:19:42 -0400 Subject: [PATCH 65/74] use external writes for the ember write logic, so that we have extra size and validations --- .../CodegenDataModel_Write.cpp | 25 +++++++++++++------ .../tests/EmberReadWriteOverride.cpp | 23 +++++++++++++++++ 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index 023116057b6739..aa32484c13e6ed 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -343,14 +344,24 @@ CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttribu ReturnErrorOnFailure(DecodeValueIntoEmberBuffer(decoder, *attributeMetadata, gEmberAttributeIOBufferSpan)); - EmberAfAttributeSearchRecord record; - record.endpoint = request.path.mEndpointId; - record.clusterId = request.path.mClusterId; - record.attributeId = request.path.mAttributeId; + Protocols::InteractionModel::Status status; - Protocols::InteractionModel::Status status = emAfReadOrWriteAttribute( - &record, attributeMetadata, gEmberAttributeIOBufferSpan.data(), static_cast(gEmberAttributeIOBufferSpan.size()), - /* write = */ true); + if (!request.operationFlags.Has(InteractionModel::OperationFlags::kInternal)) + { + + EmberAfAttributeSearchRecord record; + record.endpoint = request.path.mEndpointId; + record.clusterId = request.path.mClusterId; + record.attributeId = request.path.mAttributeId; + status = emAfReadOrWriteAttribute(&record, attributeMetadata, gEmberAttributeIOBufferSpan.data(), + static_cast(gEmberAttributeIOBufferSpan.size()), + /* write = */ true); + } + else + { + status = emAfWriteAttributeExternal(request.path.mEndpointId, request.path.mClusterId, request.path.mAttributeId, + gEmberAttributeIOBufferSpan.data(), (*attributeMetadata)->attributeType); + } if (status != Protocols::InteractionModel::Status::Success) { diff --git a/src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp b/src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp index 1dabd7d0777f1d..2fb00fab91db53 100644 --- a/src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp +++ b/src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp @@ -17,6 +17,7 @@ #include "EmberReadWriteOverride.h" #include +#include #include using chip::Protocols::InteractionModel::Status; @@ -98,3 +99,25 @@ Status emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord, return Status::Success; } + +Status emAfWriteAttributeExternal(chip::EndpointId endpoint, chip::ClusterId cluster, chip::AttributeId attributeID, + uint8_t * dataPtr, EmberAfAttributeType dataType) +{ + if (gEmberStatusCode != Status::Success) + { + return gEmberStatusCode; + } + + // ember here deduces the size of dataPtr. For testing however, we KNOW we read + // out of the ember IO buffer, so we try to use that + VerifyOrDie(dataPtr == chip::app::Compatibility::Internal::gEmberAttributeIOBufferSpan.data()); + + // In theory this should do type validation and sizes. This is NOT done for testing. + // copy over as much data as possible + // NOTE: we do NOT use (*metadata)->size since it is unclear if our mocks set that correctly + size_t len = std::min(sizeof(gEmberIoBuffer), chip::app::Compatibility::Internal::gEmberAttributeIOBufferSpan.size()); + memcpy(gEmberIoBuffer, dataPtr, len); + gEmberIoBufferFill = len; + + return Status::Success; +} From db487b4734d4a55ac6b9420cb3aff0b337eb34cb Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 16:30:01 -0400 Subject: [PATCH 66/74] Updated comments --- .../codegen-data-model/CodegenDataModel_Write.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index aa32484c13e6ed..580664540a547b 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -353,9 +353,15 @@ CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttribu record.endpoint = request.path.mEndpointId; record.clusterId = request.path.mClusterId; record.attributeId = request.path.mAttributeId; - status = emAfReadOrWriteAttribute(&record, attributeMetadata, gEmberAttributeIOBufferSpan.data(), - static_cast(gEmberAttributeIOBufferSpan.size()), - /* write = */ true); + // NOTE: gEmberAttributeIOBufferSpah is likely much larger than the attribute itself. Ember is supposed + // to be able to pick up the "valid size" out of this + // + // Specifically `When writing attributes, readLength is ignored.`. + // The actual length is used by unit tests to copy over data without needing "get attribute size" logic + // since metadata attribute sizes in our mocks is not generally correct. + status = emAfReadOrWriteAttribute(&record, attributeMetadata, gEmberAttributeIOBufferSpan.data(), + static_cast(gEmberAttributeIOBufferSpan.size()), + /* write = */ true); } else { From 2d216ab80638b9936c77d3db3089fba1870ece04 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 16:30:54 -0400 Subject: [PATCH 67/74] more comments --- src/app/codegen-data-model/CodegenDataModel_Write.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index 580664540a547b..e0eb66d2b49895 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -359,8 +359,8 @@ CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttribu // Specifically `When writing attributes, readLength is ignored.`. // The actual length is used by unit tests to copy over data without needing "get attribute size" logic // since metadata attribute sizes in our mocks is not generally correct. - status = emAfReadOrWriteAttribute(&record, attributeMetadata, gEmberAttributeIOBufferSpan.data(), - static_cast(gEmberAttributeIOBufferSpan.size()), + status = emAfReadOrWriteAttribute(&record, attributeMetadata, /* buffer = */ gEmberAttributeIOBufferSpan.data(), + /* readLength = */ static_cast(gEmberAttributeIOBufferSpan.size()), /* write = */ true); } else From 865b80a6a289f798cf0a5cebef5080f6167c1184 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 3 Jul 2024 16:32:31 -0400 Subject: [PATCH 68/74] Restyle --- src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp b/src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp index 2fb00fab91db53..36a70361973cd3 100644 --- a/src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp +++ b/src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp @@ -118,6 +118,6 @@ Status emAfWriteAttributeExternal(chip::EndpointId endpoint, chip::ClusterId clu size_t len = std::min(sizeof(gEmberIoBuffer), chip::app::Compatibility::Internal::gEmberAttributeIOBufferSpan.size()); memcpy(gEmberIoBuffer, dataPtr, len); gEmberIoBufferFill = len; - + return Status::Success; } From e542231276446b27452bd98e2af1196e2a96041f Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 4 Jul 2024 09:17:27 -0400 Subject: [PATCH 69/74] Use emberAfWriteAttribute --- .../CodegenDataModel_Write.cpp | 22 ++++++------------- .../tests/EmberReadWriteOverride.cpp | 5 +++++ 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index e0eb66d2b49895..d772b2c6f0202b 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -346,22 +347,13 @@ CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttribu Protocols::InteractionModel::Status status; - if (!request.operationFlags.Has(InteractionModel::OperationFlags::kInternal)) + if (request.operationFlags.Has(InteractionModel::OperationFlags::kInternal)) { - - EmberAfAttributeSearchRecord record; - record.endpoint = request.path.mEndpointId; - record.clusterId = request.path.mClusterId; - record.attributeId = request.path.mAttributeId; - // NOTE: gEmberAttributeIOBufferSpah is likely much larger than the attribute itself. Ember is supposed - // to be able to pick up the "valid size" out of this - // - // Specifically `When writing attributes, readLength is ignored.`. - // The actual length is used by unit tests to copy over data without needing "get attribute size" logic - // since metadata attribute sizes in our mocks is not generally correct. - status = emAfReadOrWriteAttribute(&record, attributeMetadata, /* buffer = */ gEmberAttributeIOBufferSpan.data(), - /* readLength = */ static_cast(gEmberAttributeIOBufferSpan.size()), - /* write = */ true); + // Internal requests use the non-External interface that has less enforcement + // than the external version (e.g. does not check/enforce writable settings, does not + // validate atribute types) - see attribute-table.h documentation for details. + status = emberAfWriteAttribute(request.path.mEndpointId, request.path.mClusterId, request.path.mAttributeId, + gEmberAttributeIOBufferSpan.data(), (*attributeMetadata)->attributeType); } else { diff --git a/src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp b/src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp index 36a70361973cd3..7956ef4ab354ac 100644 --- a/src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp +++ b/src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp @@ -121,3 +121,8 @@ Status emAfWriteAttributeExternal(chip::EndpointId endpoint, chip::ClusterId clu return Status::Success; } + +Status emberAfWriteAttribute(chip::EndpointId endpoint, chip::ClusterId cluster, chip::AttributeId attributeID, + uint8_t * dataPtr, EmberAfAttributeType dataType) { + return emAfWriteAttributeExternal(endpoint, cluster, attributeID, dataPtr, dataType); +} From 5664e7ba6f9a9764585cd6fee3098df167d5beed Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 4 Jul 2024 09:18:34 -0400 Subject: [PATCH 70/74] Add comment about ember-string --- src/app/codegen-data-model/CodegenDataModel_Read.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Read.cpp b/src/app/codegen-data-model/CodegenDataModel_Read.cpp index 3d9ad41ff4751a..867431084e51a2 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Read.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Read.cpp @@ -89,7 +89,12 @@ struct ShortPascalString using LengthType = uint8_t; static constexpr LengthType kNullLength = 0xFF; - static LengthType GetLength(const uint8_t * buffer) { return *buffer; } + static LengthType GetLength(const uint8_t * buffer) + { + // NOTE: we do NOT use emberAfLongStringLength because that will result in 0 length + // for null strings + return *buffer; + } }; /// Metadata of what a ember/pascal LONG string means (prepended by a u16 length) From 1df88a2efed0f665ec6a8a6b4c010fc4068bef2e Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 4 Jul 2024 09:23:42 -0400 Subject: [PATCH 71/74] Restyle --- .../codegen-data-model/tests/EmberReadWriteOverride.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp b/src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp index 7956ef4ab354ac..d3c3b9975f8176 100644 --- a/src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp +++ b/src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp @@ -122,7 +122,8 @@ Status emAfWriteAttributeExternal(chip::EndpointId endpoint, chip::ClusterId clu return Status::Success; } -Status emberAfWriteAttribute(chip::EndpointId endpoint, chip::ClusterId cluster, chip::AttributeId attributeID, - uint8_t * dataPtr, EmberAfAttributeType dataType) { - return emAfWriteAttributeExternal(endpoint, cluster, attributeID, dataPtr, dataType); +Status emberAfWriteAttribute(chip::EndpointId endpoint, chip::ClusterId cluster, chip::AttributeId attributeID, uint8_t * dataPtr, + EmberAfAttributeType dataType) +{ + return emAfWriteAttributeExternal(endpoint, cluster, attributeID, dataPtr, dataType); } From 14751932fe89271d008cafd6d692d8b24112ff7f Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 4 Jul 2024 09:48:07 -0400 Subject: [PATCH 72/74] Add context to unit tests, make write do the marking of dirty paths --- .../CodegenDataModel_Write.cpp | 19 +++ .../InteractionModelTemporaryOverrides.cpp | 6 + .../tests/TestCodegenModelViaMocks.cpp | 148 ++++++++++++------ .../DataModelChangeListener.h | 6 +- 4 files changed, 126 insertions(+), 53 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index d772b2c6f0202b..91506a9410eb3b 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -65,6 +66,17 @@ std::optional TryWriteViaAccessInterface(const ConcreteAttributePath return std::make_optional(err); } + if (decoder.TriedDecode()) + { + // TODO: change callbacks should likely be routed through the context `MarkDirty` + // however for now this is called directly because ember code does this call + // inside emberAfWriteAttribute. + // + // If we pull out the logic, the remaining `MarkDirty` calls should be + // sufficient. + MatterReportingAttributeChangeCallback(path); + } + // If the decoder tried to decode, then a value should have been read for processing. // - if decoding was done, assume DONE (i.e. final CHIP_NO_ERROR) // - otherwise, if no decoding done, return that processing must continue via nullopt @@ -340,6 +352,9 @@ CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttribu std::optional aai_result = TryWriteViaAccessInterface(request.path, aai, decoder); if (aai_result.has_value()) { + if (*aai_result == CHIP_NO_ERROR) { + CurrentContext().dataModelChangeListener->MarkDirty(request.path); + } return *aai_result; } @@ -366,6 +381,10 @@ CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttribu return CHIP_ERROR_IM_GLOBAL_STATUS_VALUE(status); } + // TODO: this may need more refinement: + // - should internal requests be able to decide if something is marked dirty or not? + // - changes-omitted paths should not be marked dirty (ember is not aware of these) + CurrentContext().dataModelChangeListener->MarkDirty(request.path); return CHIP_NO_ERROR; } diff --git a/src/app/codegen-data-model/tests/InteractionModelTemporaryOverrides.cpp b/src/app/codegen-data-model/tests/InteractionModelTemporaryOverrides.cpp index 868a26880d3ce7..f23702a4d34727 100644 --- a/src/app/codegen-data-model/tests/InteractionModelTemporaryOverrides.cpp +++ b/src/app/codegen-data-model/tests/InteractionModelTemporaryOverrides.cpp @@ -82,3 +82,9 @@ void DispatchSingleClusterCommand(const ConcreteCommandPath & aRequestCommandPat } // namespace app } // namespace chip + +void MatterReportingAttributeChangeCallback(const chip::app::ConcreteAttributePath & aPath) +{ + // TODO: should we add logic to track these calls for test purposes? +} + diff --git a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp index e9c7fe7a26ed6d..c0005f6dd0bd5c 100644 --- a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp +++ b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp @@ -117,6 +117,54 @@ bool operator==(const Access::SubjectDescriptor & a, const Access::SubjectDescri return true; } +class TestDataModelChangeListener : public DataModelChangeListener +{ +public: + void MarkDirty(const ConcreteAttributePath & path) override { mDirtyList.push_back(path); } + + std::vector & DirtyList() { return mDirtyList; } + const std::vector & DirtyList() const { return mDirtyList; } + +private: + std::vector mDirtyList; +}; + +class TestEventGenerator : public EventsGenerator +{ + CHIP_ERROR GenerateEvent(EventLoggingDelegate * eventPayloadWriter, const EventOptions & options, + EventNumber & generatedEventNumber) override + { + return CHIP_ERROR_NOT_IMPLEMENTED; + } +}; + +class TestActionContext : public ActionContext +{ +public: + Messaging::ExchangeContext * CurrentExchange() override { return nullptr; } +}; + +class CodegenDataModelWithContext : public CodegenDataModel +{ +public: + CodegenDataModelWithContext() + { + InteractionModelContext context{ + .eventsGenerator = &mEventGenerator, + .dataModelChangeListener = &mChangeListener, + .actionContext = &mActionContext, + }; + + Startup(context); + } + ~CodegenDataModelWithContext() { Shutdown(); } + +private: + TestEventGenerator mEventGenerator; + TestDataModelChangeListener mChangeListener; + TestActionContext mActionContext; +}; + class MockAccessControl : public Access::AccessControl::Delegate, public Access::AccessControl::DeviceTypeResolver { public: @@ -643,7 +691,7 @@ template void TestEmberScalarTypeRead(typename NumericAttributeTraits::WorkingType value) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; TestReadRequest testRequest( @@ -677,7 +725,7 @@ template void TestEmberScalarNullRead() { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; TestReadRequest testRequest( @@ -710,7 +758,7 @@ template void TestEmberScalarTypeWrite(const typename NumericAttributeTraits::WorkingType value) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; // non-nullable test @@ -760,7 +808,7 @@ template void TestEmberScalarNullWrite() { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; TestWriteRequest test(kAdminSubjectDescriptor, @@ -788,7 +836,7 @@ template void TestEmberScalarTypeWriteNullValueToNullable() { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; TestWriteRequest test( @@ -820,7 +868,7 @@ void WriteLe16(void * buffer, uint16_t value) TEST(TestCodegenModelViaMocks, IterateOverEndpoints) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; // This iteration relies on the hard-coding that occurs when mock_ember is used EXPECT_EQ(model.FirstEndpoint(), kMockEndpoint1); @@ -848,7 +896,7 @@ TEST(TestCodegenModelViaMocks, IterateOverEndpoints) TEST(TestCodegenModelViaMocks, IterateOverClusters) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; chip::Test::ResetVersion(); @@ -911,7 +959,7 @@ TEST(TestCodegenModelViaMocks, GetClusterInfo) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; chip::Test::ResetVersion(); @@ -936,7 +984,7 @@ TEST(TestCodegenModelViaMocks, GetClusterInfo) TEST(TestCodegenModelViaMocks, IterateOverAttributes) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; // invalid paths should return in "no more data" ASSERT_FALSE(model.FirstAttribute(ConcreteClusterPath(kEndpointIdThatIsMissing, MockClusterId(1))).path.HasValidIds()); @@ -1007,7 +1055,7 @@ TEST(TestCodegenModelViaMocks, IterateOverAttributes) TEST(TestCodegenModelViaMocks, GetAttributeInfo) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; // various non-existent or invalid paths should return no info data ASSERT_FALSE( @@ -1034,7 +1082,7 @@ TEST(TestCodegenModelViaMocks, GetAttributeInfo) TEST(TestCodegenModelViaMocks, GlobalAttributeInfo) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; std::optional info = model.GetAttributeInfo( ConcreteAttributePath(kMockEndpoint1, MockClusterId(1), Clusters::Globals::Attributes::GeneratedCommandList::Id)); @@ -1049,7 +1097,7 @@ TEST(TestCodegenModelViaMocks, GlobalAttributeInfo) TEST(TestCodegenModelViaMocks, IterateOverAcceptedCommands) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; // invalid paths should return in "no more data" ASSERT_FALSE(model.FirstAcceptedCommand(ConcreteClusterPath(kEndpointIdThatIsMissing, MockClusterId(1))).path.HasValidIds()); @@ -1114,7 +1162,7 @@ TEST(TestCodegenModelViaMocks, IterateOverAcceptedCommands) TEST(TestCodegenModelViaMocks, AcceptedCommandInfo) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; // invalid paths should return in "no more data" ASSERT_FALSE(model.GetAcceptedCommandInfo(ConcreteCommandPath(kEndpointIdThatIsMissing, MockClusterId(1), 1)).has_value()); @@ -1146,7 +1194,7 @@ TEST(TestCodegenModelViaMocks, AcceptedCommandInfo) TEST(TestCodegenModelViaMocks, IterateOverGeneratedCommands) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; // invalid paths should return in "no more data" ASSERT_FALSE(model.FirstGeneratedCommand(ConcreteClusterPath(kEndpointIdThatIsMissing, MockClusterId(1))).HasValidIds()); @@ -1205,7 +1253,7 @@ TEST(TestCodegenModelViaMocks, IterateOverGeneratedCommands) TEST(TestCodegenModelViaMocks, EmberAttributeReadAclDeny) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; TestReadRequest testRequest(kDenySubjectDescriptor, @@ -1218,7 +1266,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeReadAclDeny) TEST(TestCodegenModelViaMocks, ReadForInvalidGlobalAttributePath) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; { @@ -1239,7 +1287,7 @@ TEST(TestCodegenModelViaMocks, ReadForInvalidGlobalAttributePath) TEST(TestCodegenModelViaMocks, EmberAttributeInvalidRead) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; // Invalid attribute @@ -1273,7 +1321,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeInvalidRead) TEST(TestCodegenModelViaMocks, EmberAttributePathExpansionAccessDeniedRead) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; TestReadRequest testRequest(kDenySubjectDescriptor, @@ -1291,7 +1339,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributePathExpansionAccessDeniedRead) TEST(TestCodegenModelViaMocks, AccessInterfaceUnsupportedRead) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; const ConcreteAttributePath kTestPath(kMockEndpoint3, MockClusterId(4), @@ -1395,7 +1443,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeReadNulls) TEST(TestCodegenModelViaMocks, EmberAttributeReadErrorReading) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; { @@ -1431,7 +1479,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeReadErrorReading) TEST(TestCodegenModelViaMocks, EmberAttributeReadNullOctetString) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; TestReadRequest testRequest(kAdminSubjectDescriptor, @@ -1466,7 +1514,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeReadNullOctetString) TEST(TestCodegenModelViaMocks, EmberAttributeReadOctetString) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; TestReadRequest testRequest( @@ -1505,7 +1553,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeReadOctetString) TEST(TestCodegenModelViaMocks, EmberAttributeReadLongOctetString) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; TestReadRequest testRequest(kAdminSubjectDescriptor, @@ -1542,7 +1590,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeReadLongOctetString) TEST(TestCodegenModelViaMocks, EmberAttributeReadShortString) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; TestReadRequest testRequest(kAdminSubjectDescriptor, @@ -1578,7 +1626,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeReadShortString) TEST(TestCodegenModelViaMocks, EmberAttributeReadLongString) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; TestReadRequest testRequest( @@ -1615,7 +1663,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeReadLongString) TEST(TestCodegenModelViaMocks, AttributeAccessInterfaceStructRead) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; const ConcreteAttributePath kStructPath(kMockEndpoint3, MockClusterId(4), @@ -1658,7 +1706,7 @@ TEST(TestCodegenModelViaMocks, AttributeAccessInterfaceStructRead) TEST(TestCodegenModelViaMocks, AttributeAccessInterfaceReadError) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; const ConcreteAttributePath kStructPath(kMockEndpoint3, MockClusterId(4), @@ -1673,7 +1721,7 @@ TEST(TestCodegenModelViaMocks, AttributeAccessInterfaceReadError) TEST(TestCodegenModelViaMocks, AttributeAccessInterfaceListRead) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; const ConcreteAttributePath kStructPath(kMockEndpoint3, MockClusterId(4), @@ -1725,7 +1773,7 @@ TEST(TestCodegenModelViaMocks, AttributeAccessInterfaceListRead) TEST(TestCodegenModelViaMocks, AttributeAccessInterfaceListOverflowRead) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; const ConcreteAttributePath kStructPath(kMockEndpoint3, MockClusterId(4), @@ -1784,7 +1832,7 @@ TEST(TestCodegenModelViaMocks, AttributeAccessInterfaceListOverflowRead) TEST(TestCodegenModelViaMocks, AttributeAccessInterfaceListIncrementalRead) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; const ConcreteAttributePath kStructPath(kMockEndpoint3, MockClusterId(4), @@ -1846,7 +1894,7 @@ TEST(TestCodegenModelViaMocks, AttributeAccessInterfaceListIncrementalRead) TEST(TestCodegenModelViaMocks, ReadGlobalAttributeAttributeList) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; TestReadRequest testRequest(kAdminSubjectDescriptor, @@ -1902,7 +1950,7 @@ TEST(TestCodegenModelViaMocks, ReadGlobalAttributeAttributeList) TEST(TestCodegenModelViaMocks, EmberAttributeWriteAclDeny) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; TestWriteRequest test(kDenySubjectDescriptor, ConcreteDataAttributePath(kMockEndpoint1, MockClusterId(1), MockAttributeId(10))); @@ -1966,7 +2014,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteInvalidValueToNullable) TEST(TestCodegenModelViaMocks, EmberTestWriteReservedNullPlaceholderToNullable) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; TestWriteRequest test( @@ -1984,7 +2032,7 @@ TEST(TestCodegenModelViaMocks, EmberTestWriteReservedNullPlaceholderToNullable) TEST(TestCodegenModelViaMocks, EmberTestWriteOutOfRepresentableRangeOddIntegerNonNullable) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; TestWriteRequest test(kAdminSubjectDescriptor, @@ -2003,7 +2051,7 @@ TEST(TestCodegenModelViaMocks, EmberTestWriteOutOfRepresentableRangeOddIntegerNo TEST(TestCodegenModelViaMocks, EmberTestWriteOutOfRepresentableRangeOddIntegerNullable) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; TestWriteRequest test( @@ -2058,7 +2106,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteNulls) TEST(TestCodegenModelViaMocks, EmberAttributeWriteShortString) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; TestWriteRequest test(kAdminSubjectDescriptor, @@ -2075,7 +2123,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteShortString) TEST(TestCodegenModelViaMocks, EmberAttributeWriteLongString) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; TestWriteRequest test(kAdminSubjectDescriptor, @@ -2096,7 +2144,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteLongString) TEST(TestCodegenModelViaMocks, EmberAttributeWriteNullableLongStringValue) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; TestWriteRequest test(kAdminSubjectDescriptor, @@ -2117,7 +2165,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteNullableLongStringValue) TEST(TestCodegenModelViaMocks, EmberAttributeWriteLongNullableStringNull) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; TestWriteRequest test(kAdminSubjectDescriptor, @@ -2134,7 +2182,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteLongNullableStringNull) TEST(TestCodegenModelViaMocks, EmberAttributeWriteShortBytes) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; TestWriteRequest test(kAdminSubjectDescriptor, @@ -2156,7 +2204,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteShortBytes) TEST(TestCodegenModelViaMocks, EmberAttributeWriteLongBytes) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; TestWriteRequest test(kAdminSubjectDescriptor, @@ -2180,7 +2228,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteLongBytes) TEST(TestCodegenModelViaMocks, EmberAttributeWriteTimedWrite) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; TestWriteRequest test(kAdminSubjectDescriptor, ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), kAttributeIdTimedWrite)); @@ -2196,7 +2244,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteTimedWrite) TEST(TestCodegenModelViaMocks, EmberAttributeWriteReadOnlyAttribute) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; TestWriteRequest test(kAdminSubjectDescriptor, ConcreteAttributePath(kMockEndpoint3, MockClusterId(4), kAttributeIdReadOnly)); @@ -2212,7 +2260,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteReadOnlyAttribute) TEST(TestCodegenModelViaMocks, EmberAttributeWriteDataVersion) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; TestWriteRequest test(kAdminSubjectDescriptor, @@ -2239,7 +2287,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteDataVersion) TEST(TestCodegenModelViaMocks, WriteToInvalidPath) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; { @@ -2263,7 +2311,7 @@ TEST(TestCodegenModelViaMocks, WriteToInvalidPath) TEST(TestCodegenModelViaMocks, WriteToGlobalAttribute) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; TestWriteRequest test(kAdminSubjectDescriptor, ConcreteAttributePath(kMockEndpoint1, MockClusterId(1), AttributeList::Id)); @@ -2274,7 +2322,7 @@ TEST(TestCodegenModelViaMocks, WriteToGlobalAttribute) TEST(TestCodegenModelViaMocks, EmberWriteFailure) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; TestWriteRequest test(kAdminSubjectDescriptor, @@ -2298,7 +2346,7 @@ TEST(TestCodegenModelViaMocks, EmberWriteFailure) TEST(TestCodegenModelViaMocks, EmberWriteAttributeAccessInterfaceTest) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; const ConcreteAttributePath kStructPath(kMockEndpoint3, MockClusterId(4), @@ -2330,7 +2378,7 @@ TEST(TestCodegenModelViaMocks, EmberWriteAttributeAccessInterfaceTest) TEST(TestCodegenModelViaMocks, EmberWriteAttributeAccessInterfaceReturningError) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; const ConcreteAttributePath kStructPath(kMockEndpoint3, MockClusterId(4), @@ -2353,7 +2401,7 @@ TEST(TestCodegenModelViaMocks, EmberWriteAttributeAccessInterfaceReturningError) TEST(TestCodegenModelViaMocks, EmberWriteInvalidDataType) { UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModel model; + CodegenDataModelWithContext model; ScopedMockAccessControl accessControl; const ConcreteAttributePath kStructPath(kMockEndpoint3, MockClusterId(4), diff --git a/src/app/data-model-interface/DataModelChangeListener.h b/src/app/data-model-interface/DataModelChangeListener.h index 37c278c76b92c8..a5ba12684baffe 100644 --- a/src/app/data-model-interface/DataModelChangeListener.h +++ b/src/app/data-model-interface/DataModelChangeListener.h @@ -16,7 +16,7 @@ */ #pragma once -#include +#include namespace chip { namespace app { @@ -34,12 +34,12 @@ namespace InteractionModel { class DataModelChangeListener { public: - virtual ~DataModelChangeListener() = 0; + virtual ~DataModelChangeListener() = default; /// Mark all attributes matching the given path (which may be a wildcard) dirty. /// /// Wildcards are supported. - virtual void MarkDirty(const AttributePathParams & path) = 0; + virtual void MarkDirty(const ConcreteAttributePath & path) = 0; }; } // namespace InteractionModel From b26edf159ea06b2a8efdae502b727729caf98b08 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 4 Jul 2024 09:52:47 -0400 Subject: [PATCH 73/74] Add some unit tests for dirty path handling --- .../tests/TestCodegenModelViaMocks.cpp | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp index c0005f6dd0bd5c..bef13e0464c5dd 100644 --- a/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp +++ b/src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp @@ -159,6 +159,9 @@ class CodegenDataModelWithContext : public CodegenDataModel } ~CodegenDataModelWithContext() { Shutdown(); } + TestDataModelChangeListener & ChangeListener() { return mChangeListener; } + const TestDataModelChangeListener & ChangeListener() const { return mChangeListener; } + private: TestEventGenerator mEventGenerator; TestDataModelChangeListener mChangeListener; @@ -779,7 +782,12 @@ void TestEmberScalarTypeWrite(const typename NumericAttributeTraits::WorkingT memcpy(&storage, writtenData.data(), sizeof(storage)); typename NumericAttributeTraits::WorkingType actual = NumericAttributeTraits::StorageToWorking(storage); - ASSERT_EQ(actual, value); + EXPECT_EQ(actual, value); + ASSERT_EQ(model.ChangeListener().DirtyList().size(), 1u); + EXPECT_EQ(model.ChangeListener().DirtyList()[0], test.request.path); + + // reset for the next test + model.ChangeListener().DirtyList().clear(); } // nullable test @@ -801,6 +809,8 @@ void TestEmberScalarTypeWrite(const typename NumericAttributeTraits::WorkingT typename NumericAttributeTraits::WorkingType actual = NumericAttributeTraits::StorageToWorking(storage); ASSERT_EQ(actual, value); + ASSERT_EQ(model.ChangeListener().DirtyList().size(), 1u); + EXPECT_EQ(model.ChangeListener().DirtyList()[0], test.request.path); } } @@ -1957,6 +1967,7 @@ TEST(TestCodegenModelViaMocks, EmberAttributeWriteAclDeny) AttributeValueDecoder decoder = test.DecoderFor(1234); ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_IM_GLOBAL_STATUS(UnsupportedAccess)); + ASSERT_TRUE(model.ChangeListener().DirtyList().empty()); } TEST(TestCodegenModelViaMocks, EmberAttributeWriteBasicTypes) @@ -2368,6 +2379,10 @@ TEST(TestCodegenModelViaMocks, EmberWriteAttributeAccessInterfaceTest) EXPECT_EQ(aai->GetData().a, 112); EXPECT_TRUE(aai->GetData().e.data_equal("aai_write_test"_span)); + // AAI marks dirty paths + ASSERT_EQ(model.ChangeListener().DirtyList().size(), 1u); + EXPECT_EQ(model.ChangeListener().DirtyList()[0], kStructPath); + // AAI does not prevent read/write of regular attributes // validate that once AAI is added, we still can go through writing regular bits (i.e. // AAI returning "unknown" has fallback to ember) @@ -2396,6 +2411,7 @@ TEST(TestCodegenModelViaMocks, EmberWriteAttributeAccessInterfaceReturningError) AttributeValueDecoder decoder = test.DecoderFor(testValue); ASSERT_EQ(model.WriteAttribute(test.request, decoder), CHIP_ERROR_KEY_NOT_FOUND); + ASSERT_TRUE(model.ChangeListener().DirtyList().empty()); } TEST(TestCodegenModelViaMocks, EmberWriteInvalidDataType) @@ -2421,4 +2437,5 @@ 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_TRUE(model.ChangeListener().DirtyList().empty()); } From faac4fbdc2bd2c16245b7a769bdd99193892d05b Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 4 Jul 2024 09:53:57 -0400 Subject: [PATCH 74/74] Move the change callback around a bit --- .../CodegenDataModel_Write.cpp | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/app/codegen-data-model/CodegenDataModel_Write.cpp b/src/app/codegen-data-model/CodegenDataModel_Write.cpp index 91506a9410eb3b..b9a55114c0b30d 100644 --- a/src/app/codegen-data-model/CodegenDataModel_Write.cpp +++ b/src/app/codegen-data-model/CodegenDataModel_Write.cpp @@ -66,17 +66,6 @@ std::optional TryWriteViaAccessInterface(const ConcreteAttributePath return std::make_optional(err); } - if (decoder.TriedDecode()) - { - // TODO: change callbacks should likely be routed through the context `MarkDirty` - // however for now this is called directly because ember code does this call - // inside emberAfWriteAttribute. - // - // If we pull out the logic, the remaining `MarkDirty` calls should be - // sufficient. - MatterReportingAttributeChangeCallback(path); - } - // If the decoder tried to decode, then a value should have been read for processing. // - if decoding was done, assume DONE (i.e. final CHIP_NO_ERROR) // - otherwise, if no decoding done, return that processing must continue via nullopt @@ -352,7 +341,12 @@ CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttribu std::optional aai_result = TryWriteViaAccessInterface(request.path, aai, decoder); if (aai_result.has_value()) { - if (*aai_result == CHIP_NO_ERROR) { + if (*aai_result == CHIP_NO_ERROR) + { + // TODO: change callbacks should likely be routed through the context `MarkDirty` only + // however for now this is called directly because ember code does this call + // inside emberAfWriteAttribute. + MatterReportingAttributeChangeCallback(request.path); CurrentContext().dataModelChangeListener->MarkDirty(request.path); } return *aai_result;