From 07a28ac0bed1c54843f73590a15b09c661733969 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Mon, 7 Oct 2024 20:29:39 -0400 Subject: [PATCH] One more decouple from ember from src/app and ensure we can compile without `ember-compatibility-functions.cpp` (#35919) * Add ability to skip compiling of ember-compatibility-functions * Fix build and make sure that CI will validate a data-model-enabled build * Restyle * Fix compilation * Restyled by clang-format * Typo fix * Fix includes * Rename method to something that seems a bit clearer * Fix pwrpc usage to use datamodel interface * Restyle * Remove todo * Update for attribute is list to support not know value * Update src/app/WriteHandler.h Co-authored-by: Boris Zbarsky --------- Co-authored-by: Restyled.io Co-authored-by: Boris Zbarsky --- .github/workflows/examples-linux-arm.yaml | 2 +- .../common/pigweed/rpc_services/Attributes.h | 38 ++++++++++++++++ src/app/WriteHandler.cpp | 44 ++++++++++++++----- src/app/WriteHandler.h | 6 +++ src/app/chip_data_model.cmake | 14 +++++- src/app/chip_data_model.gni | 6 ++- 6 files changed, 95 insertions(+), 15 deletions(-) diff --git a/.github/workflows/examples-linux-arm.yaml b/.github/workflows/examples-linux-arm.yaml index ee226b12dc2881..7000082f03e32e 100644 --- a/.github/workflows/examples-linux-arm.yaml +++ b/.github/workflows/examples-linux-arm.yaml @@ -65,7 +65,7 @@ jobs: --target linux-arm64-chip-tool-nodeps-ipv6only \ --target linux-arm64-lock-clang \ --target linux-arm64-minmdns-clang \ - --target linux-arm64-light-rpc-ipv6only-clang \ + --target linux-arm64-light-data-model-enabled-rpc-ipv6only-clang \ --target linux-arm64-thermostat-no-ble-clang \ --target linux-arm64-lit-icd-no-ble-clang \ --target linux-arm64-fabric-admin-clang-rpc \ diff --git a/examples/common/pigweed/rpc_services/Attributes.h b/examples/common/pigweed/rpc_services/Attributes.h index e246c5361bd281..3ff1200be2edeb 100644 --- a/examples/common/pigweed/rpc_services/Attributes.h +++ b/examples/common/pigweed/rpc_services/Attributes.h @@ -22,6 +22,7 @@ #include "pigweed/rpc_services/internal/StatusUtils.h" #include +#include #include #include #include @@ -32,6 +33,13 @@ #include #include +#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE +#include +#include +#include +#include +#endif + namespace chip { namespace rpc { @@ -202,7 +210,37 @@ class Attributes : public pw_rpc::nanopb::Attributes::Service writer.Init(tlvBuffer); PW_TRY(ChipErrorToPwStatus(writer.StartContainer(TLV::AnonymousTag(), TLV::kTLVType_Structure, outer))); PW_TRY(ChipErrorToPwStatus(attributeReports.Init(&writer, kReportContextTag))); + +#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE + // TODO: this assumes a singleton data model provider + app::DataModel::Provider * provider = app::InteractionModelEngine::GetInstance()->GetDataModelProvider(); + + app::DataModel::ReadAttributeRequest request; + request.path = path; + request.operationFlags.Set(app::DataModel::OperationFlags::kInternal); + request.subjectDescriptor = subjectDescriptor; + + std::optional info = provider->GetClusterInfo(path); + if (!info.has_value()) + { + return ::pw::Status::NotFound(); + } + + app::AttributeValueEncoder encoder(attributeReports, subjectDescriptor, path, info->dataVersion, + false /* isFabricFiltered */, nullptr /* attributeEncodingState */); + app::DataModel::ActionReturnStatus result = provider->ReadAttribute(request, encoder); + + if (!result.IsSuccess()) + { + app::DataModel::ActionReturnStatus::StringStorage storage; + ChipLogError(Support, "Failed to read data: %s", result.c_str(storage)); + return ::pw::Status::Internal(); + } + +#else PW_TRY(ChipErrorToPwStatus(app::ReadSingleClusterData(subjectDescriptor, false, path, attributeReports, nullptr))); +#endif + attributeReports.EndOfContainer(); PW_TRY(ChipErrorToPwStatus(writer.EndContainer(outer))); PW_TRY(ChipErrorToPwStatus(writer.Finalize())); diff --git a/src/app/WriteHandler.cpp b/src/app/WriteHandler.cpp index 5050cbbee2fcb5..d2fde339304f8f 100644 --- a/src/app/WriteHandler.cpp +++ b/src/app/WriteHandler.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include @@ -32,6 +33,7 @@ #include #include #include +#include #include #include @@ -41,8 +43,7 @@ namespace chip { namespace app { using namespace Protocols::InteractionModel; -using Status = Protocols::InteractionModel::Status; -constexpr uint8_t kListAttributeType = 0x48; +using Status = Protocols::InteractionModel::Status; CHIP_ERROR WriteHandler::Init(DataModel::Provider * apProvider, WriteHandlerDelegate * apWriteHandlerDelegate) { @@ -79,6 +80,30 @@ void WriteHandler::Close() MoveToState(State::Uninitialized); } +std::optional WriteHandler::IsListAttributePath(const ConcreteAttributePath & path) +{ +#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE + VerifyOrReturnValue(mDataModelProvider != nullptr, std::nullopt, + ChipLogError(DataManagement, "Null data model while checking attribute properties.")); + + auto info = mDataModelProvider->GetAttributeInfo(path); + if (!info.has_value()) + { + return std::nullopt; + } + + return info->flags.Has(DataModel::AttributeQualityFlags::kListAttribute); +#else + constexpr uint8_t kListAttributeType = 0x48; + const auto attributeMetadata = GetAttributeMetadata(path); + if (attributeMetadata == nullptr) + { + return std::nullopt; + } + return (attributeMetadata->attributeType == kListAttributeType); +#endif +} + Status WriteHandler::HandleWriteRequestMessage(Messaging::ExchangeContext * apExchangeContext, System::PacketBufferHandle && aPayload, bool aIsTimedWrite) { @@ -317,10 +342,7 @@ CHIP_ERROR WriteHandler::ProcessAttributeDataIBs(TLV::TLVReader & aAttributeData err = element.GetData(&dataReader); SuccessOrExit(err); - const auto attributeMetadata = GetAttributeMetadata(dataAttributePath); - bool currentAttributeIsList = (attributeMetadata != nullptr && attributeMetadata->attributeType == kListAttributeType); - - if (!dataAttributePath.IsListOperation() && currentAttributeIsList) + if (!dataAttributePath.IsListOperation() && IsListAttributePath(dataAttributePath).value_or(false)) { dataAttributePath.mListOp = ConcreteDataAttributePath::ListOperation::ReplaceAll; } @@ -446,7 +468,7 @@ CHIP_ERROR WriteHandler::ProcessGroupAttributeDataIBs(TLV::TLVReader & aAttribut mProcessingAttributePath, mStateFlags.Has(StateBits::kProcessingAttributeIsList), dataAttributePath); bool shouldReportListWriteBegin = false; // This will be set below. - const EmberAfAttributeMetadata * attributeMetadata = nullptr; + std::optional isListAttribute = std::nullopt; while (iterator->Next(mapping)) { @@ -460,11 +482,11 @@ CHIP_ERROR WriteHandler::ProcessGroupAttributeDataIBs(TLV::TLVReader & aAttribut // Try to get the metadata from for the attribute from one of the expanded endpoints (it doesn't really matter which // endpoint we pick, as long as it's valid) and update the path info according to it and recheck if we need to report // list write begin. - if (attributeMetadata == nullptr) + if (!isListAttribute.has_value()) { - attributeMetadata = GetAttributeMetadata(dataAttributePath); - bool currentAttributeIsList = - (attributeMetadata != nullptr && attributeMetadata->attributeType == kListAttributeType); + isListAttribute = IsListAttributePath(dataAttributePath); + bool currentAttributeIsList = isListAttribute.value_or(false); + if (!dataAttributePath.IsListOperation() && currentAttributeIsList) { dataAttributePath.mListOp = ConcreteDataAttributePath::ListOperation::ReplaceAll; diff --git a/src/app/WriteHandler.h b/src/app/WriteHandler.h index fe63e028b8dfee..33c068b8cd02ea 100644 --- a/src/app/WriteHandler.h +++ b/src/app/WriteHandler.h @@ -189,6 +189,12 @@ class WriteHandler : public Messaging::ExchangeDelegate CHIP_ERROR WriteClusterData(const Access::SubjectDescriptor & aSubject, const ConcreteDataAttributePath & aPath, TLV::TLVReader & aData); + /// Checks whether the given path corresponds to a list attribute + /// Return values: + /// true/false: valid attribute path, known if list or not + /// std::nulloptr - path not available/valid, unknown if attribute is a list or not + std::optional IsListAttributePath(const ConcreteAttributePath & path); + Messaging::ExchangeHolder mExchangeCtx; WriteResponseMessage::Builder mWriteResponseBuilder; Optional mProcessingAttributePath; diff --git a/src/app/chip_data_model.cmake b/src/app/chip_data_model.cmake index e2d05adb01e4a1..11db6d4ec6c97b 100644 --- a/src/app/chip_data_model.cmake +++ b/src/app/chip_data_model.cmake @@ -70,12 +70,17 @@ endfunction() # function(chip_configure_data_model APP_TARGET) set(SCOPE PRIVATE) - cmake_parse_arguments(ARG "" "SCOPE;ZAP_FILE;IDL" "EXTERNAL_CLUSTERS" ${ARGN}) + set(ADD_EMBER_INTERFACE_FILES TRUE) + cmake_parse_arguments(ARG "SKIP_EMBER_INTERFACE" "SCOPE;ZAP_FILE;IDL" "EXTERNAL_CLUSTERS" ${ARGN}) if(ARG_SCOPE) set(SCOPE ${ARG_SCOPE}) endif() + if(ARG_SKIP_EMBER_INTERFACE) + set(ADD_EMBER_INTERFACE_FILES FALSE) + endif() + # CMAKE data model auto-includes the server side implementation target_sources(${APP_TARGET} ${SCOPE} ${CHIP_APP_BASE_DIR}/server/AclStorage.cpp @@ -159,7 +164,6 @@ function(chip_configure_data_model APP_TARGET) ${CHIP_APP_BASE_DIR}/util/attribute-table.cpp ${CHIP_APP_BASE_DIR}/util/binding-table.cpp ${CHIP_APP_BASE_DIR}/util/DataModelHandler.cpp - ${CHIP_APP_BASE_DIR}/util/ember-compatibility-functions.cpp ${CHIP_APP_BASE_DIR}/util/ember-global-attribute-access-interface.cpp ${CHIP_APP_BASE_DIR}/util/ember-io-storage.cpp ${CHIP_APP_BASE_DIR}/util/generic-callback-stubs.cpp @@ -169,4 +173,10 @@ function(chip_configure_data_model APP_TARGET) ${APP_GEN_FILES} ${APP_TEMPLATES_GEN_FILES} ) + + if(ADD_EMBER_INTERFACE_FILES) + target_sources(${APP_TARGET} ${SCOPE} + ${CHIP_APP_BASE_DIR}/util/ember-compatibility-functions.cpp + ) + endif() endfunction() diff --git a/src/app/chip_data_model.gni b/src/app/chip_data_model.gni index 42b221c1a98836..cc998948d835e7 100644 --- a/src/app/chip_data_model.gni +++ b/src/app/chip_data_model.gni @@ -209,11 +209,15 @@ template("chip_data_model") { "${_app_root}/util/DataModelHandler.cpp", "${_app_root}/util/attribute-storage.cpp", "${_app_root}/util/attribute-table.cpp", - "${_app_root}/util/ember-compatibility-functions.cpp", "${_app_root}/util/ember-global-attribute-access-interface.cpp", "${_app_root}/util/ember-io-storage.cpp", "${_app_root}/util/util.cpp", ] + + if (chip_use_data_model_interface != "enabled") { + # This applies to "check" or "disabled" (i.e. use ember-only or use ember for double-checking) + sources += [ "${_app_root}/util/ember-compatibility-functions.cpp" ] + } } if (defined(invoker.zap_file)) {