Skip to content

Commit

Permalink
Make global attributes be part of IM instead of part of DataModel::Pr…
Browse files Browse the repository at this point in the history
…ovider (#37345)

* A first pass to add global attributes as part of IME instead of datamodel::provider

* Some updates

* Update some tests and reformat... the checks were PAINFUL,kept debug logs

* Bump test: we now support the extra 3 global attributes not in metadata

* More fixes

* Fix compile error about parameter shadowing

* More updates on casts to make xtensa compile happy

* Restyled by clang-format

* Fix tests

* Make the test for unsupported write consistent with #37322

* Fix includes

* Smaller delta

* Attribute metadata is now guaranteed

* Update src/app/data-model-provider/MetadataLookup.h

Co-authored-by: Boris Zbarsky <[email protected]>

* Update src/app/data-model-provider/MetadataLookup.h

Co-authored-by: Boris Zbarsky <[email protected]>

* Update src/app/reporting/Engine.cpp

Co-authored-by: Boris Zbarsky <[email protected]>

* Update src/app/reporting/Engine.cpp

Co-authored-by: Boris Zbarsky <[email protected]>

* Address some code review comments

* Remove the ember metadata public cluster path validation as it is not needed anymore as a public API

* Do not enforce ordering in attribute list encoding

* Comment about items returned or not returned in various calls

* Correct the unsupported attribute call

* Restyle

* Update order dependent test in java ... this is somewhat broken...

* Fix indent

* Fix include

* Fix typo

* Allow TODO comment: I am not willing to re-build the kotlin code right now for this PR

* Using uint64_t for encoding saves some flash

* Fix test ... although this is NOT ok as we need order independent test

* Fix the real list now

* Switch the basic information constraints as a set compare (use python)

* Remove extra space

* Restyled by prettier-yaml

* Fix asserts in DGWIFI - wrong method used

* Restyled by autopep8

* Update src/app/data-model-provider/Provider.h

Co-authored-by: Boris Zbarsky <[email protected]>

* Update src/app/data-model-provider/ProviderMetadataTree.h

Co-authored-by: Boris Zbarsky <[email protected]>

* Update src/data-model-providers/codegen/CodegenDataModelProvider_Write.cpp

Co-authored-by: Boris Zbarsky <[email protected]>

* Update src/app/data-model-provider/ProviderMetadataTree.h

Co-authored-by: Boris Zbarsky <[email protected]>

* Undo DFWIFI changes, leave it up to #37382.

* Update ordering of attribute list to match unsorted (and smaller) implementation in the SDK

* Update src/app/GlobalAttributes.cpp

Co-authored-by: Terence Hampson <[email protected]>

* Update src/app/GlobalAttributes.h

Co-authored-by: Terence Hampson <[email protected]>

* Add comment about oddify in path validation

* Added comments about why we do the casts ... it is ugly

* Add integration test for writing read only attributes and getting the correct error

* Restyled by isort

* Fix unused imports

---------

Co-authored-by: Andrei Litvin <[email protected]>
Co-authored-by: Restyled.io <[email protected]>
Co-authored-by: Boris Zbarsky <[email protected]>
Co-authored-by: Terence Hampson <[email protected]>
  • Loading branch information
5 people authored Feb 6, 2025
1 parent 48fc426 commit 09833eb
Show file tree
Hide file tree
Showing 31 changed files with 562 additions and 428 deletions.
2 changes: 0 additions & 2 deletions .github/workflows/lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,6 @@ jobs:
--known-failure app/util/config.h \
--known-failure app/util/DataModelHandler.cpp \
--known-failure app/util/DataModelHandler.h \
--known-failure app/util/ember-global-attribute-access-interface.h \
--known-failure app/util/ember-io-storage.h \
--known-failure app/util/endpoint-config-api.h \
--known-failure app/util/generic-callbacks.h \
Expand All @@ -133,7 +132,6 @@ jobs:
# for them. Keeping them as a list as they still need review ...
# --known-failure app/util/attribute-table.cpp \
# --known-failure app/util/ember-io-storage.cpp \
# --known-failure app/util/ember-global-attribute-access-interface.cpp \
# --known-failure app/util/attribute-storage.cpp \
- name: Check for matter lint errors
if: always()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,12 @@ class PairOnNetworkLongImReadCommand(
event.getJson().toString() == """{"0:STRUCT":{"0:UINT":1}}"""

fun checkAllAttributesJsonForFixedLabel(cluster: String): Boolean {
// TODO: this hard-codes the array and as a result it is order-dependend. This should
// be changed to be order-independent.
val expected =
"""{"65528:ARRAY-?":[],"0:ARRAY-STRUCT":[{"0:STRING":"room","1:STRING":"bedroom 2"},""" +
"""{"0:STRING":"orientation","1:STRING":"North"},{"0:STRING":"floor","1:STRING":"2"},""" +
"""{"0:STRING":"direction","1:STRING":"up"}],"65531:ARRAY-UINT":[0,65528,65529,65531,65532,65533],""" +
"""{"0:STRING":"direction","1:STRING":"up"}],"65531:ARRAY-UINT":[0,65532,65533,65528,65529,65531],""" +
""""65533:UINT":1,"65529:ARRAY-?":[],"65532:UINT":0}"""
return cluster.equals(expected)
}
Expand Down
1 change: 1 addition & 0 deletions kotlin-detect-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ style:
- "**/examples/android/CHIPTool/app/src/main/java/com/google/chip/chiptool/clusterclient/MultiAdminClientFragment.kt"
- "**/examples/android/CHIPTool/app/src/main/java/com/google/chip/chiptool/clusterclient/clusterinteraction/ClusterInteractionFragment.kt"
- "**/examples/android/CHIPTool/app/src/main/java/com/google/chip/chiptool/provisioning/AddressCommissioningFragment.kt"
- "**/examples/java-matter-controller/java/src/com/matter/controller/commands/pairing/PairOnNetworkLongImReadCommand.kt"
- "**/src/controller/java/src/matter/onboardingpayload/QRCodeOnboardingPayloadParser.kt"
ExplicitItLambdaParameter:
excludes:
Expand Down
6 changes: 3 additions & 3 deletions scripts/build_coverage.sh
Original file line number Diff line number Diff line change
Expand Up @@ -176,9 +176,9 @@ if [ "$skip_gn" == false ]; then
fi

mkdir -p "$COVERAGE_ROOT"
lcov --initial --capture --directory "$OUTPUT_ROOT/obj/src" --exclude="$PWD"/zzz_generated/* --exclude="$PWD"/third_party/* --exclude=/usr/include/* --output-file "$COVERAGE_ROOT/lcov_base.info"
lcov --capture --directory "$OUTPUT_ROOT/obj/src" --exclude="$PWD"/zzz_generated/* --exclude="$PWD"/third_party/* --exclude=/usr/include/* --output-file "$COVERAGE_ROOT/lcov_test.info"
lcov --add-tracefile "$COVERAGE_ROOT/lcov_base.info" --add-tracefile "$COVERAGE_ROOT/lcov_test.info" --output-file "$COVERAGE_ROOT/lcov_final.info"
lcov --initial --capture --ignore-errors inconsistent --directory "$OUTPUT_ROOT/obj/src" --exclude="$PWD"/zzz_generated/* --exclude="$PWD"/third_party/* --exclude=/usr/include/* --output-file "$COVERAGE_ROOT/lcov_base.info"
lcov --capture --ignore-errors inconsistent --directory "$OUTPUT_ROOT/obj/src" --exclude="$PWD"/zzz_generated/* --exclude="$PWD"/third_party/* --exclude=/usr/include/* --output-file "$COVERAGE_ROOT/lcov_test.info"
lcov --ignore-errors inconsistent --add-tracefile "$COVERAGE_ROOT/lcov_base.info" --add-tracefile "$COVERAGE_ROOT/lcov_test.info" --output-file "$COVERAGE_ROOT/lcov_final.info"
genhtml "$COVERAGE_ROOT/lcov_final.info" --output-directory "$COVERAGE_ROOT/html" --title "SHA:$(git rev-parse HEAD)" --header-title "Matter SDK Coverage Report"

# Copy webapp's YAML file to the coverage output directory
Expand Down
8 changes: 6 additions & 2 deletions src/app/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -107,17 +107,22 @@ source_set("paths") {
"${chip_root}/src/app/util:types",
"${chip_root}/src/lib/core",
"${chip_root}/src/lib/core:types",
"${chip_root}/src/lib/support",
]
}

source_set("global-attributes") {
sources = [ "GlobalAttributes.h" ]
sources = [
"GlobalAttributes.cpp",
"GlobalAttributes.h",
]

# This also depends on zap-generated code which is currently impossible to split out
# as a dependency
public_deps = [
":app_config",
"${chip_root}/src/app/common:ids",
"${chip_root}/src/app/data-model-provider",
"${chip_root}/src/lib/support",
]
}
Expand Down Expand Up @@ -263,7 +268,6 @@ static_library("interaction-model") {
# We likely should formalize and change this with a proper DataModel::Provider that
# is consistent instead
sources += [
"${chip_root}/src/app/util/ember-global-attribute-access-interface.cpp",
"${chip_root}/src/app/util/ember-io-storage.cpp",
"dynamic_server/DynamicDispatcher.cpp",
]
Expand Down
132 changes: 132 additions & 0 deletions src/app/GlobalAttributes.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
/*
* Copyright (c) 2022-2025 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 <app/GlobalAttributes.h>
#include <app/data-model-provider/MetadataLookup.h>
#include <protocols/interaction_model/StatusCode.h>

using chip::Protocols::InteractionModel::Status;

namespace chip {
namespace app {

bool IsSupportedGlobalAttributeNotInMetadata(AttributeId attributeId)
{
for (auto & attr : GlobalAttributesNotInMetadata)
{
if (attr == attributeId)
{
return true;
}
}

return false;
}

DataModel::ActionReturnStatus ReadGlobalAttributeFromMetadata(DataModel::Provider * provider, const ConcreteAttributePath & path,
AttributeValueEncoder & encoder)
{
CHIP_ERROR err;

switch (path.mAttributeId)
{
case Clusters::Globals::Attributes::GeneratedCommandList::Id: {
DataModel::ListBuilder<CommandId> builder;
err = provider->GeneratedCommands(path, builder);
if (err != CHIP_NO_ERROR)
{
break;
}
auto buffer = builder.TakeBuffer();

return encoder.EncodeList([&buffer](const auto & listEncodeHelper) {
for (auto id : buffer)
{
// NOTE: cast to u64 because TLV encodes all numbers the same (no TLV sideffects)
// and this reduces template variants for Encode, saving flash.
ReturnErrorOnFailure(listEncodeHelper.Encode(static_cast<uint64_t>(id)));
}
return CHIP_NO_ERROR;
});
}
case Clusters::Globals::Attributes::AcceptedCommandList::Id: {
DataModel::ListBuilder<DataModel::AcceptedCommandEntry> builder;
err = provider->AcceptedCommands(path, builder);
if (err != CHIP_NO_ERROR)
{
break;
}
auto buffer = builder.TakeBuffer();

return encoder.EncodeList([&buffer](const auto & listEncodeHelper) {
for (auto entry : buffer)
{
// NOTE: cast to u64 because TLV encodes all numbers the same (no TLV sideffects)
// and this reduces template variants for Encode, saving flash.
ReturnErrorOnFailure(listEncodeHelper.Encode(static_cast<uint64_t>(entry.commandId)));
}
return CHIP_NO_ERROR;
});
}
case Clusters::Globals::Attributes::AttributeList::Id: {
DataModel::ListBuilder<DataModel::AttributeEntry> builder;
err = provider->Attributes(path, builder);
if (err != CHIP_NO_ERROR)
{
break;
}
auto buffer = builder.TakeBuffer();

return encoder.EncodeList([&buffer](const auto & listEncodeHelper) {
for (auto entry : buffer)
{
// NOTE: cast to u64 because TLV encodes all numbers the same (no TLV sideffects)
// and this reduces template variants for Encode, saving flash.
ReturnErrorOnFailure(listEncodeHelper.Encode(static_cast<uint64_t>(entry.attributeId)));
}

for (auto id : GlobalAttributesNotInMetadata)
{
// NOTE: cast to u64 because TLV encodes all numbers the same (no TLV sideffects)
// and this reduces template variants for Encode, saving flash.
ReturnErrorOnFailure(listEncodeHelper.Encode(static_cast<uint64_t>(id)));
}

return CHIP_NO_ERROR;
});
}
default:
return CHIP_ERROR_INVALID_ARGUMENT;
}

// if we get here, the path was NOT valid
if (err == CHIP_ERROR_NOT_FOUND)
{
// The `Failure` here is arbitrary: we expect ReadGlobalAttributeFromMetadata to be
// an internal API used for global attributes only and call preconditions say that
// should never happen.
//
// Code only takes this path if one of
// `GeneratedCommands`/`AcceptedCommands`/`Attribute` return a NOT_FOUND and
// that would indicate an invalid cluster (which should have been pre-validated by
// the caller).
return DataModel::ValidateClusterPath(provider, path, Status::Failure);
}
return err;
}

} // namespace app
} // namespace chip
28 changes: 16 additions & 12 deletions src/app/GlobalAttributes.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@

#include <app-common/zap-generated/ids/Attributes.h>
#include <app/AppConfig.h>
#include <app/AttributeValueEncoder.h>
#include <app/ConcreteAttributePath.h>
#include <app/data-model-provider/ActionReturnStatus.h>
#include <app/data-model-provider/Provider.h>
#include <lib/core/DataModelTypes.h>
#include <lib/support/CodeUtils.h>

namespace chip {
Expand All @@ -37,18 +42,17 @@ constexpr AttributeId GlobalAttributesNotInMetadata[] = {

static_assert(ArrayIsSorted(GlobalAttributesNotInMetadata), "Array of global attribute ids must be sorted");

inline bool IsSupportedGlobalAttributeNotInMetadata(AttributeId attributeId)
{
for (auto & attr : GlobalAttributesNotInMetadata)
{
if (attr == attributeId)
{
return true;
}
}

return false;
}
bool IsSupportedGlobalAttributeNotInMetadata(AttributeId attributeId);

/**
* Reads a `IsSupportedGlobalAttributeNotInMetadata` attribute into `encoder`.
*
* Preconditions:
* - `path` MUST be a valid cluster path inside `provider` and its mAttributeID
* MUST be `IsSupportedGlobalAttributeNotInMetadata`
*/
DataModel::ActionReturnStatus ReadGlobalAttributeFromMetadata(DataModel::Provider * provider, const ConcreteAttributePath & path,
AttributeValueEncoder & encoder);

} // namespace app
} // namespace chip
1 change: 0 additions & 1 deletion src/app/chip_data_model.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,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-global-attribute-access-interface.cpp
${CHIP_APP_BASE_DIR}/util/ember-io-storage.cpp
${CHIP_APP_BASE_DIR}/util/generic-callback-stubs.cpp
${CHIP_APP_BASE_DIR}/util/privilege-storage.cpp
Expand Down
1 change: 0 additions & 1 deletion src/app/chip_data_model.gni
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,6 @@ 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-global-attribute-access-interface.cpp",
"${_app_root}/util/ember-io-storage.cpp",
"${_app_root}/util/util.cpp",
]
Expand Down
21 changes: 21 additions & 0 deletions src/app/data-model-provider/MetadataLookup.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,27 @@ std::optional<EndpointEntry> EndpointFinder::Find(EndpointId endpointId)
return std::nullopt;
}

Protocols::InteractionModel::Status ValidateClusterPath(ProviderMetadataTree * provider, const ConcreteClusterPath & path,
Protocols::InteractionModel::Status successStatus)
{
if (ServerClusterFinder(provider).Find(path).has_value())
{
return successStatus;
}

auto endpoints = provider->EndpointsIgnoreError();
for (auto & endpointEntry : endpoints)
{
if (endpointEntry.id == path.mEndpointId)
{
// endpoint is valid
return Protocols::InteractionModel::Status::UnsupportedCluster;
}
}

return Protocols::InteractionModel::Status::UnsupportedEndpoint;
}

} // namespace DataModel
} // namespace app
} // namespace chip
8 changes: 8 additions & 0 deletions src/app/data-model-provider/MetadataLookup.h
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,14 @@ class EndpointFinder
ReadOnlyBuffer<EndpointEntry> mEndpoints;
};

/// Validates that the cluster identified by `path` exists within the given provider.
/// If the endpoint does not exist, will return Status::UnsupportedEndpoint.
/// If the endpoint exists but does not have the cluster identified by the path, will return Status::UnsupportedCluster.
///
/// Otherwise, will return successStatus.
Protocols::InteractionModel::Status ValidateClusterPath(ProviderMetadataTree * provider, const ConcreteClusterPath & path,
Protocols::InteractionModel::Status successStatus);

} // namespace DataModel
} // namespace app
} // namespace chip
8 changes: 6 additions & 2 deletions src/app/data-model-provider/Provider.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,12 @@ class Provider : public ProviderMetadataTree
// event emitting, path marking and other operations
virtual InteractionModelContext CurrentContext() const { return mContext; }

/// TEMPORARY/TRANSITIONAL requirement for transitioning from ember-specific code
/// ReadAttribute is REQUIRED to respond to GlobalAttribute read requests
/// NOTE: this code is NOT required to handle `List` global attributes:
/// AcceptedCommandsList, GeneratedCommandsList OR AttributeList
///
/// Users of DataModel::Provider are expected to get these lists
/// from ProviderMetadataTree (in particular IM Reads of these
/// attributes will the automatically filled from metadata).
///
/// Return value notes:
/// ActionReturnStatus::IsOutOfSpaceEncodingResponse
Expand Down
8 changes: 8 additions & 0 deletions src/app/data-model-provider/ProviderMetadataTree.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,14 @@ class ProviderMetadataTree
virtual CHIP_ERROR ClientClusters(EndpointId endpointId, ListBuilder<ClusterId> & builder) = 0;
virtual CHIP_ERROR ServerClusters(EndpointId endpointId, ListBuilder<ServerClusterEntry> & builder) = 0;

/// Attribute lists contain all attributes EXCEPT the list attributes that
/// are part of metadata. The output from this method MUST NOT contain:
/// - AttributeList::Id
/// - AcceptedCommandList::Id
/// - GeneratedCommandList::Id
/// However it MUST ALWAYS contain:
/// - ClusterRevision::Id
/// - FeatureMap::Id
virtual CHIP_ERROR Attributes(const ConcreteClusterPath & path, ListBuilder<AttributeEntry> & builder) = 0;
virtual CHIP_ERROR GeneratedCommands(const ConcreteClusterPath & path, ListBuilder<CommandId> & builder) = 0;
virtual CHIP_ERROR AcceptedCommands(const ConcreteClusterPath & path, ListBuilder<AcceptedCommandEntry> & builder) = 0;
Expand Down
12 changes: 12 additions & 0 deletions src/app/data-model-provider/StringBuilderAdapters.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,15 @@ StatusWithSize ToString<chip::app::DataModel::ActionReturnStatus>(const chip::ap
}

} // namespace pw
//
#if CHIP_CONFIG_TEST_GOOGLETEST
namespace chip {

void PrintTo(const chip::app::DataModel::ActionReturnStatus & status, std::ostream * os)
{
chip::app::DataModel::ActionReturnStatus::StringStorage storage;
*os << "ActionReturnStatus<" << status.c_str(storage) << ">";
}

} // namespace chip
#endif // CHIP_CONFIG_TEST_GOOGLETEST
9 changes: 9 additions & 0 deletions src/app/data-model-provider/StringBuilderAdapters.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
/// which is not as helpful as a full formatted output.

#include <pw_string/string_builder.h>
#include <pw_unit_test/framework.h>

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

Expand All @@ -44,3 +45,11 @@ StatusWithSize ToString<chip::app::DataModel::ActionReturnStatus>(const chip::ap
pw::span<char> buffer);

} // namespace pw

#if CHIP_CONFIG_TEST_GOOGLETEST
namespace chip {

void PrintTo(const chip::app::DataModel::ActionReturnStatus & status, std::ostream * os);

} // namespace chip
#endif // CHIP_CONFIG_TEST_GOOGLETEST
Loading

0 comments on commit 09833eb

Please sign in to comment.