From f301851d93863e337803a9037fe476283b1e3b11 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Mon, 3 Mar 2025 09:30:16 -0500 Subject: [PATCH 01/24] Server cluster interface --- docs/ids_and_codes/ERROR_CODES.md | 1 + scripts/build_coverage.sh | 5 +- src/BUILD.gn | 12 +- src/app/InteractionModelEngine.cpp | 2 +- src/app/data-model-provider/Provider.h | 4 +- .../tests/TestActionReturnStatus.cpp | 6 +- src/app/server-cluster/BUILD.gn | 35 ++++ .../server-cluster/DefaultServerCluster.cpp | 110 ++++++++++++ src/app/server-cluster/DefaultServerCluster.h | 84 +++++++++ .../server-cluster/ServerClusterInterface.h | 159 ++++++++++++++++++ src/app/server-cluster/tests/BUILD.gn | 31 ++++ .../tests/TestDefaultServerCluster.cpp | 152 +++++++++++++++++ src/app/tests/TestCommandInteraction.cpp | 7 +- src/app/tests/test-interaction-model-api.cpp | 5 +- src/app/tests/test-interaction-model-api.h | 5 +- .../tests/data_model/DataModelFixtures.cpp | 4 +- .../tests/data_model/DataModelFixtures.h | 5 +- .../codegen/CodegenDataModelProvider.cpp | 6 +- .../codegen/CodegenDataModelProvider.h | 5 +- .../tests/TestCodegenModelViaMocks.cpp | 4 +- src/lib/core/CHIPError.cpp | 3 + src/lib/core/CHIPError.h | 11 +- 22 files changed, 627 insertions(+), 29 deletions(-) create mode 100644 src/app/server-cluster/BUILD.gn create mode 100644 src/app/server-cluster/DefaultServerCluster.cpp create mode 100644 src/app/server-cluster/DefaultServerCluster.h create mode 100644 src/app/server-cluster/ServerClusterInterface.h create mode 100644 src/app/server-cluster/tests/BUILD.gn create mode 100644 src/app/server-cluster/tests/TestDefaultServerCluster.cpp diff --git a/docs/ids_and_codes/ERROR_CODES.md b/docs/ids_and_codes/ERROR_CODES.md index ff549e6df6b6dc..5bd5807e4ed442 100644 --- a/docs/ids_and_codes/ERROR_CODES.md +++ b/docs/ids_and_codes/ERROR_CODES.md @@ -55,6 +55,7 @@ This file was **AUTOMATICALLY** generated by | 37 | 0x25 | `CHIP_ERROR_UNKNOWN_IMPLICIT_TLV_TAG` | | 38 | 0x26 | `CHIP_ERROR_WRONG_TLV_TYPE` | | 39 | 0x27 | `CHIP_ERROR_TLV_CONTAINER_OPEN` | +| 40 | 0x28 | `CHIP_ERROR_IN_USE` | | 42 | 0x2A | `CHIP_ERROR_INVALID_MESSAGE_TYPE` | | 43 | 0x2B | `CHIP_ERROR_UNEXPECTED_TLV_ELEMENT` | | 45 | 0x2D | `CHIP_ERROR_NOT_IMPLEMENTED` | diff --git a/scripts/build_coverage.sh b/scripts/build_coverage.sh index d21aef5a77eb7e..5c29e7a9482649 100755 --- a/scripts/build_coverage.sh +++ b/scripts/build_coverage.sh @@ -222,6 +222,7 @@ lcov --initial --capture --directory "$OUTPUT_ROOT/obj/src" \ --exclude="$PWD"/zzz_generated/* \ --exclude="$PWD"/third_party/* \ --exclude=/usr/include/* \ + --ignore-errors inconsistent \ --output-file "$COVERAGE_ROOT/lcov_base.info" lcov --capture --directory "$OUTPUT_ROOT/obj/src" \ @@ -229,9 +230,11 @@ lcov --capture --directory "$OUTPUT_ROOT/obj/src" \ --exclude="$PWD"/zzz_generated/* \ --exclude="$PWD"/third_party/* \ --exclude=/usr/include/* \ + --ignore-errors inconsistent \ --output-file "$COVERAGE_ROOT/lcov_test.info" -lcov --add-tracefile "$COVERAGE_ROOT/lcov_base.info" \ +lcov --ignore-errors inconsistent \ + --add-tracefile "$COVERAGE_ROOT/lcov_base.info" \ --add-tracefile "$COVERAGE_ROOT/lcov_test.info" \ --ignore-errors inconsistent \ --output-file "$COVERAGE_ROOT/lcov_final.info" diff --git a/src/BUILD.gn b/src/BUILD.gn index d290cf04cf2737..2a8fa7c388d206 100644 --- a/src/BUILD.gn +++ b/src/BUILD.gn @@ -51,13 +51,14 @@ if (chip_build_tests) { deps = [] tests = [ "${chip_root}/src/access/tests", - "${chip_root}/src/app/data-model/tests", "${chip_root}/src/app/cluster-building-blocks/tests", "${chip_root}/src/app/data-model-provider/tests", + "${chip_root}/src/app/data-model/tests", "${chip_root}/src/app/icd/server/tests", "${chip_root}/src/crypto/tests", "${chip_root}/src/inet/tests", "${chip_root}/src/lib/address_resolve/tests", + "${chip_root}/src/app/server-cluster/tests", "${chip_root}/src/lib/asn1/tests", "${chip_root}/src/lib/core/tests", "${chip_root}/src/lib/format/tests", @@ -94,12 +95,19 @@ if (chip_build_tests) { # symbols for ember mocks which are used by other tests. tests += [ - "${chip_root}/src/data-model-providers/codegen/tests", "${chip_root}/src/setup_payload/tests", "${chip_root}/src/transport/raw/tests", ] } + if (current_os != "zephyr" && current_os != "mbed" && + chip_device_platform != "efr32") { + # Disabled from "one single binary" since the "mocks" contain duplicate sybmols + # with ember + # Disabled on EFR32 since "include " fails with `::fabs` not defined + tests += [ "${chip_root}/src/data-model-providers/codegen/tests" ] + } + if (chip_device_platform != "efr32") { tests += [ "${chip_root}/src/app/tests", diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index d92c459a94d438..9a64332aea3fa9 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -1734,7 +1734,7 @@ void InteractionModelEngine::DispatchCommand(CommandHandlerImpl & apCommandObj, request.invokeFlags.Set(DataModel::InvokeFlags::kTimed, apCommandObj.IsTimedInvoke()); request.subjectDescriptor = &subjectDescriptor; - std::optional status = GetDataModelProvider()->Invoke(request, apPayload, &apCommandObj); + std::optional status = GetDataModelProvider()->InvokeCommand(request, apPayload, &apCommandObj); // Provider indicates that handler status or data was already set (or will be set asynchronously) by // returning std::nullopt. If any other value is returned, it is requesting that a status is set. This diff --git a/src/app/data-model-provider/Provider.h b/src/app/data-model-provider/Provider.h index 27a86b63fa0cf2..67a8ccabb12e24 100644 --- a/src/app/data-model-provider/Provider.h +++ b/src/app/data-model-provider/Provider.h @@ -114,8 +114,8 @@ class Provider : public ProviderMetadataTree /// - if a value is returned (not nullopt) then the handler response MUST NOT be filled. The caller /// will then issue `handler->AddStatus(request.path, ->GetStatusCode())`. This is a /// convenience to make writing Invoke calls easier. - virtual std::optional Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments, - CommandHandler * handler) = 0; + virtual std::optional InvokeCommand(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments, + CommandHandler * handler) = 0; private: InteractionModelContext mContext = { nullptr }; diff --git a/src/app/data-model-provider/tests/TestActionReturnStatus.cpp b/src/app/data-model-provider/tests/TestActionReturnStatus.cpp index 8ca1993d149e1a..451c65640c55d7 100644 --- a/src/app/data-model-provider/tests/TestActionReturnStatus.cpp +++ b/src/app/data-model-provider/tests/TestActionReturnStatus.cpp @@ -15,13 +15,11 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - -#include "lib/core/CHIPError.h" -#include "protocols/interaction_model/StatusCode.h" -#include "pw_unit_test/framework_backend.h" #include #include +#include #include +#include #include diff --git a/src/app/server-cluster/BUILD.gn b/src/app/server-cluster/BUILD.gn new file mode 100644 index 00000000000000..cd8b7e3b48f816 --- /dev/null +++ b/src/app/server-cluster/BUILD.gn @@ -0,0 +1,35 @@ +# Copyright (c) 2025 Project CHIP Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +import("//build_overrides/chip.gni") + +source_set("server-cluster") { + sources = [ + "DefaultServerCluster.cpp", + "DefaultServerCluster.h", + "ServerClusterInterface.h", + ] + + public_deps = [ + "${chip_root}/src/app:attribute-access", + "${chip_root}/src/app:command-handler-interface", + "${chip_root}/src/app:paths", + "${chip_root}/src/app/common:ids", + "${chip_root}/src/app/data-model-provider", + "${chip_root}/src/app/data-model-provider:metadata", + "${chip_root}/src/crypto", + "${chip_root}/src/lib/core:error", + "${chip_root}/src/lib/core:types", + "${chip_root}/src/lib/support", + ] +} diff --git a/src/app/server-cluster/DefaultServerCluster.cpp b/src/app/server-cluster/DefaultServerCluster.cpp new file mode 100644 index 00000000000000..db2b7d3b3cf4ae --- /dev/null +++ b/src/app/server-cluster/DefaultServerCluster.cpp @@ -0,0 +1,110 @@ +/* + * Copyright (c) 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 + +#include +#include +#include +#include +#include +#include +#include + +namespace chip { +namespace app { +namespace { + +using namespace chip::app::Clusters; +using namespace chip::app::DataModel; + +constexpr std::array kGlobalAttributeEntries{ { + { + Globals::Attributes::ClusterRevision::Id, + BitFlags(), + Access::Privilege::kView, + std::nullopt, + }, + { + Globals::Attributes::FeatureMap::Id, + BitFlags(), + Access::Privilege::kView, + std::nullopt, + }, + { + Globals::Attributes::AttributeList::Id, + BitFlags(AttributeQualityFlags::kListAttribute), + Access::Privilege::kView, + std::nullopt, + }, + { + Globals::Attributes::AcceptedCommandList::Id, + BitFlags(AttributeQualityFlags::kListAttribute), + Access::Privilege::kView, + std::nullopt, + }, + { + Globals::Attributes::GeneratedCommandList::Id, + BitFlags(AttributeQualityFlags::kListAttribute), + Access::Privilege::kView, + std::nullopt, + }, +} }; + +} // namespace + +DefaultServerCluster::DefaultServerCluster() +{ + // SPEC - 7.10.3. Cluster Data Version + // A cluster data version SHALL be initialized randomly when it is first published. + mDataVersion = Crypto::GetRandU32(); +} + +CHIP_ERROR DefaultServerCluster::Attributes(const ConcreteClusterPath & path, DataModel::ListBuilder & builder) +{ + + return builder.ReferenceExisting(kGlobalAttributeEntries); +} + +BitFlags DefaultServerCluster::GetClusterFlags() const +{ + return {}; +} + +ActionReturnStatus DefaultServerCluster::WriteAttribute(const WriteAttributeRequest & request, AttributeValueDecoder & decoder) +{ + return Protocols::InteractionModel::Status::UnsupportedWrite; +} + +std::optional +DefaultServerCluster::InvokeCommand(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments, CommandHandler * handler) +{ + return Protocols::InteractionModel::Status::UnsupportedCommand; +} + +CHIP_ERROR DefaultServerCluster::AcceptedCommands(const ConcreteClusterPath & path, + DataModel::ListBuilder & builder) +{ + return CHIP_NO_ERROR; +} + +CHIP_ERROR DefaultServerCluster::GeneratedCommands(const ConcreteClusterPath & path, DataModel::ListBuilder & builder) +{ + return CHIP_NO_ERROR; +} + +} // namespace app +} // namespace chip diff --git a/src/app/server-cluster/DefaultServerCluster.h b/src/app/server-cluster/DefaultServerCluster.h new file mode 100644 index 00000000000000..3463d39882bddc --- /dev/null +++ b/src/app/server-cluster/DefaultServerCluster.h @@ -0,0 +1,84 @@ +/* + * Copyright (c) 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. + */ +#pragma once + +#include + +namespace chip { +namespace app { + +/// Provides an implementation of most methods for a `ServerClusterInterface` +/// to make it easier to implement spec-compliant classes. +/// +/// In particular it does: +/// - maintains a data version and provides `IncreaseDataVersion`. Ensures this +/// version is spec-compliant initialized (with a random value) +/// - Provides default implementations for most virtual methods EXCEPT: +/// - ReadAttribute (since that one needs to handle featuremap and revision) +/// - GetClusterId (since every implementation is for different clusters) +/// +/// +class DefaultServerCluster : public ServerClusterInterface +{ +public: + DefaultServerCluster(); + ~DefaultServerCluster() override = default; + + //////////////////////////// ServerClusterInterface implementation //////////////////////////////////////// + + [[nodiscard]] DataVersion GetDataVersion() const override { return mDataVersion; } + [[nodiscard]] BitFlags GetClusterFlags() const override; + + /// Default implementation errors out with an unsupported write on every attribute. + DataModel::ActionReturnStatus WriteAttribute(const DataModel::WriteAttributeRequest & request, + AttributeValueDecoder & decoder) override; + + /// Must only be implemented if support for any non-global attributes + /// is required. + /// + /// Default implementation just returns the global attributes required by the API contract. + CHIP_ERROR Attributes(const ConcreteClusterPath & path, DataModel::ListBuilder & builder) override; + + ///////////////////////////////////// Command Support ///////////////////////////////////////////////////////// + + /// Must only be implemented if commands are supported by the cluster + /// + /// Default implementation errors out with an UnsupportedCommand error. + std::optional InvokeCommand(const DataModel::InvokeRequest & request, + chip::TLV::TLVReader & input_arguments, + CommandHandler * handler) override; + + /// Must only be implemented if commands are supported by the cluster + /// + /// Default implementation is a NOOP (no list items generated) + CHIP_ERROR AcceptedCommands(const ConcreteClusterPath & path, + DataModel::ListBuilder & builder) override; + + /// Must only be implemented if commands that return values are supported by the cluster. + /// + /// Default implementation is a NOOP (no list items generated) + CHIP_ERROR GeneratedCommands(const ConcreteClusterPath & path, DataModel::ListBuilder & builder) override; + +protected: + void IncreaseDataVersion() { mDataVersion++; } + +private: + DataVersion mDataVersion; // will be random-initialized as per spec +}; + +} // namespace app +} // namespace chip diff --git a/src/app/server-cluster/ServerClusterInterface.h b/src/app/server-cluster/ServerClusterInterface.h new file mode 100644 index 00000000000000..086a37cbab529b --- /dev/null +++ b/src/app/server-cluster/ServerClusterInterface.h @@ -0,0 +1,159 @@ +/* + * Copyright (c) 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. + */ +#pragma once + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace chip { +namespace app { + +/// Handles cluster interactions for a specific cluster id. +/// +/// A `ServerClusterInterface` instance is associated with a single endpointId and represents +/// a cluster that exists at a given `endpointId/clusterId` path. +/// +/// Provides metadata as well as interaction processing (attribute read/write and command handling). +class ServerClusterInterface +{ +public: + virtual ~ServerClusterInterface() = default; + + ///////////////////////////////////// Cluster Metadata Support ////////////////////////////////////////////////// + [[nodiscard]] virtual ClusterId GetClusterId() const = 0; + + /// Gets the data version for this cluster instance. + /// + /// Every cluster instance must have a data version. + /// + /// SPEC - 7.10.3. Cluster Data Version + /// A cluster data version is a metadata increment-only counter value, maintained for each cluster instance. + /// [...] + /// A cluster data version SHALL increment or be set (wrap) to zero if incrementing would exceed its + /// maximum value. A cluster data version SHALL be maintained for each cluster instance. + /// [...] + /// A cluster data version SHALL be incremented if any attribute data changes. + [[nodiscard]] virtual DataVersion GetDataVersion() const = 0; + + [[nodiscard]] virtual BitFlags GetClusterFlags() const = 0; + + ///////////////////////////////////// Attribute Support //////////////////////////////////////////////////////// + + /// Indicates the start/end of a series of list operations. This function will be called either before the first + /// Write operation or after the last one of a series of consecutive attribute data values received for the same attribute. + /// + /// 1) This function will be called if the client tries to set a nullable list attribute to null. + /// 2) This function will only be called at the beginning and end of a series of consecutive attribute data + /// blocks for the same attribute, no matter what list operations those data blocks represent. + /// 3) The opType argument indicates the type of notification (Start, Failure, Success). + virtual void ListAttributeWriteNotification(const ConcreteAttributePath & aPath, DataModel::ListWriteOperation opType) {} + + /// Reads the value of an existing attribute. + /// + /// ReadAttribute MUST be done on an "existent" attribute path: only on attributes that are + /// returned in an `Attributes` call for this cluster. ReadAttribute is not expected to perform + /// that verification; the caller is responsible for it. + /// + /// `request.path` is expected to have `GetClusterId` as the cluster id as well as an attribute that is + /// included in an `Attributes` call. + /// + /// This MUST HANDLE the following global attributes: + /// - FeatureMap::Id + /// - ClusterRevision::Id + /// + /// This function WILL NOT be called for attributes that can be derived from cluster metadata. + /// Specifically this WILL NOT be called (and does not need to implement handling for) the + /// following attribute IDs: + /// - AcceptedCommandList::Id + /// - AttributeList::Id + /// - GeneratedCommandList::Id + virtual DataModel::ActionReturnStatus ReadAttribute(const DataModel::ReadAttributeRequest & request, + AttributeValueEncoder & encoder) = 0; + + /// Writes a value to an existing attribute. + /// + /// WriteAttribute MUST be done on an "existent" attribute path: only on attributes that are + /// returned in an `Attributes` call for this cluster. WriteAttribute is not expected to perform + /// that verification; the caller is responsible for it. + /// + /// `request.path` is expected to have `GetClusterId` as the cluster id as well as an attribute that is + /// included in a `Attributes` call. + virtual DataModel::ActionReturnStatus WriteAttribute(const DataModel::WriteAttributeRequest & request, + AttributeValueDecoder & decoder) = 0; + + /// Retrieves the list of attributes supported by this cluster. + /// + /// Attribute list MUST contain global attributes. + /// + /// Specifically these attributes MUST always exist in the list for all clusters: + /// - ClusterRevision::Id + /// - FeatureMap::Id + /// - AcceptedCommandList::Id + /// - AttributeList::Id + /// - GeneratedCommandList::Id + /// See SPEC 7.13 Global Elements: `Global Attributes` table + virtual CHIP_ERROR Attributes(const ConcreteClusterPath & path, + DataModel::ListBuilder & builder) = 0; + + ///////////////////////////////////// Command Support ///////////////////////////////////////////////////////// + + /// Handles the invocation of a command. + /// + /// `handler` is used to send back the response. + /// - returning `nullopt` means that return value was placed in handler directly. + /// This includes cases where command handling and value return will be done asynchronously. + /// - returning a value other than Success implies an error reply (error and data are mutually exclusive) + /// + /// InvokeCommand MUST be done on an "existent" attribute path: only on commands that are + /// returned in an `AcceptedCommand` call for this cluster. + /// + /// Return value expectations: + /// - if a response has been placed into `handler` then std::nullopt MUST be returned. In particular + /// note that CHIP_NO_ERROR is NOT the same as std::nullopt: + /// > CHIP_NO_ERROR means handler had no status set and we expect the caller to AddStatus(success) + /// > std::nullopt means that handler has added an appropriate data/status response + /// - if a value is returned (not nullopt) then the handler response MUST NOT be filled. The caller + /// will then issue `handler->AddStatus(request.path, ->GetStatusCode())`. This is a + /// convenience to make writing Invoke calls easier. + virtual std::optional + InvokeCommand(const DataModel::InvokeRequest & request, chip::TLV::TLVReader & input_arguments, CommandHandler * handler) = 0; + + /// Retrieves a list of commands accepted by this cluster. + /// + /// Returning `CHIP_NO_ERROR` without adding anything to the `builder` list is expected + /// if no commands are supported by the cluster. + virtual CHIP_ERROR AcceptedCommands(const ConcreteClusterPath & path, + DataModel::ListBuilder & builder) = 0; + + /// Retrieves a list of commands generated by this cluster. + /// + /// Returning `CHIP_NO_ERROR` without adding anything to the `builder` list is expected + /// if no commands are generated by processing accepted commands. + virtual CHIP_ERROR GeneratedCommands(const ConcreteClusterPath & path, DataModel::ListBuilder & builder) = 0; +}; + +} // namespace app +} // namespace chip diff --git a/src/app/server-cluster/tests/BUILD.gn b/src/app/server-cluster/tests/BUILD.gn new file mode 100644 index 00000000000000..d9f7af30ec5e45 --- /dev/null +++ b/src/app/server-cluster/tests/BUILD.gn @@ -0,0 +1,31 @@ +# Copyright (c) 2025 Project CHIP Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +import("//build_overrides/build.gni") +import("//build_overrides/chip.gni") +import("${chip_root}/build/chip/chip_test_suite.gni") + +chip_test_suite("tests") { + output_name = "libServerClusterInterfaceTests" + + test_sources = [ "TestDefaultServerCluster.cpp" ] + + cflags = [ "-Wconversion" ] + + public_deps = [ + "${chip_root}/src/app/common:ids", + "${chip_root}/src/app/data-model-provider/tests:encode-decode", + "${chip_root}/src/app/server-cluster", + "${chip_root}/src/lib/core:string-builder-adapters", + ] +} diff --git a/src/app/server-cluster/tests/TestDefaultServerCluster.cpp b/src/app/server-cluster/tests/TestDefaultServerCluster.cpp new file mode 100644 index 00000000000000..a44791625d4a5b --- /dev/null +++ b/src/app/server-cluster/tests/TestDefaultServerCluster.cpp @@ -0,0 +1,152 @@ +/* + * Copyright (c) 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 + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include + +using namespace chip; +using namespace chip::app; +using namespace chip::app::Clusters; +using namespace chip::app::DataModel; +using namespace chip::app::Testing; +using namespace chip::Protocols::InteractionModel; + +namespace { + +class FakeDefaultServerCluster : public DefaultServerCluster +{ +public: + FakeDefaultServerCluster(ClusterId id) : mClusterId(id) {} + + ClusterId GetClusterId() const override { return mClusterId; } + + DataModel::ActionReturnStatus ReadAttribute(const DataModel::ReadAttributeRequest & request, + AttributeValueEncoder & encoder) override + { + switch (request.path.mAttributeId) + { + case Globals::Attributes::FeatureMap::Id: + return encoder.Encode(0); + case Globals::Attributes::ClusterRevision::Id: + return encoder.Encode(123); + } + return CHIP_ERROR_INVALID_ARGUMENT; + } + + void TestIncreaseDataVersion() { IncreaseDataVersion(); } + +private: + ClusterId mClusterId; +}; + +} // namespace + +TEST(TestDefaultServerCluster, TestDataVersion) +{ + FakeDefaultServerCluster cluster(1); + + DataVersion v1 = cluster.GetDataVersion(); + cluster.TestIncreaseDataVersion(); + ASSERT_EQ(cluster.GetDataVersion(), v1 + 1); +} + +TEST(TestDefaultServerCluster, TestFlagsDefault) +{ + FakeDefaultServerCluster cluster(1); + ASSERT_EQ(cluster.GetClusterFlags().Raw(), 0u); +} + +TEST(TestDefaultServerCluster, AttributesDefault) +{ + FakeDefaultServerCluster cluster(1); + + DataModel::ListBuilder attributes; + + ASSERT_EQ(cluster.Attributes({ 1, 1 }, attributes), CHIP_NO_ERROR); + + ReadOnlyBuffer data = attributes.TakeBuffer(); + + // 5 global attributes are currently supported. Ensure they are returned. + ASSERT_EQ(data.size(), 5u); + + ASSERT_EQ(data[0].attributeId, Globals::Attributes::ClusterRevision::Id); + ASSERT_EQ(data[1].attributeId, Globals::Attributes::FeatureMap::Id); + ASSERT_EQ(data[2].attributeId, Globals::Attributes::AttributeList::Id); + ASSERT_EQ(data[3].attributeId, Globals::Attributes::AcceptedCommandList::Id); + ASSERT_EQ(data[4].attributeId, Globals::Attributes::GeneratedCommandList::Id); + + // first 2 are normal, the rest are list + for (size_t i = 0; i < 5; i++) + { + ASSERT_EQ(data[i].flags.Has(AttributeQualityFlags::kListAttribute), (i >= 2)); + ASSERT_EQ(data[i].readPrivilege, Access::Privilege::kView); + ASSERT_FALSE(data[i].writePrivilege.has_value()); + } +} + +TEST(TestDefaultServerCluster, CommandsDefault) +{ + FakeDefaultServerCluster cluster(1); + + DataModel::ListBuilder acceptedCommands; + ASSERT_EQ(cluster.AcceptedCommands({ 1, 1 }, acceptedCommands), CHIP_NO_ERROR); + ASSERT_TRUE(acceptedCommands.TakeBuffer().empty()); + + DataModel::ListBuilder generatedCommands; + ASSERT_EQ(cluster.GeneratedCommands({ 1, 1 }, generatedCommands), CHIP_NO_ERROR); + ASSERT_TRUE(generatedCommands.TakeBuffer().empty()); +} + +TEST(TestDefaultServerCluster, WriteAttributeDefault) +{ + FakeDefaultServerCluster cluster(1); + + WriteOperation test(0 /* endpoint */, 1 /* cluster */, 1234 /* attribute */); + test.SetSubjectDescriptor(kAdminSubjectDescriptor); + + AttributeValueDecoder decoder = test.DecoderFor(12345); + + ASSERT_EQ(cluster.WriteAttribute(test.GetRequest(), decoder), Status::UnsupportedWrite); + ASSERT_FALSE(decoder.TriedDecode()); +} + +TEST(TestDefaultServerCluster, InvokeDefault) +{ + FakeDefaultServerCluster cluster(1); + + TLV::TLVReader tlvReader; + InvokeRequest request; + + request.path = { 0 /* endpoint */, 1 /* cluster */, 1234 /* command */ }; + + ASSERT_EQ(cluster.InvokeCommand(request, tlvReader, nullptr /* command handler, assumed unused here */), + Status::UnsupportedCommand); +} diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index 363b43ccb4d5cf..65cab41b4848f9 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -403,10 +403,11 @@ class TestCommandInteractionModel : public TestImCustomDataModel return &instance; } - TestCommandInteractionModel() {} + TestCommandInteractionModel() = default; - std::optional Invoke(const DataModel::InvokeRequest & request, - chip::TLV::TLVReader & input_arguments, CommandHandler * handler) + std::optional InvokeCommand(const DataModel::InvokeRequest & request, + chip::TLV::TLVReader & input_arguments, + CommandHandler * handler) override { DispatchSingleClusterCommand(request.path, input_arguments, handler); return std::nullopt; // handler status is set by the dispatch diff --git a/src/app/tests/test-interaction-model-api.cpp b/src/app/tests/test-interaction-model-api.cpp index d5659420dd6a8a..2c7f393856b825 100644 --- a/src/app/tests/test-interaction-model-api.cpp +++ b/src/app/tests/test-interaction-model-api.cpp @@ -143,8 +143,9 @@ ActionReturnStatus TestImCustomDataModel::WriteAttribute(const WriteAttributeReq return CHIP_NO_ERROR; } -std::optional TestImCustomDataModel::Invoke(const InvokeRequest & request, - chip::TLV::TLVReader & input_arguments, CommandHandler * handler) +std::optional TestImCustomDataModel::InvokeCommand(const InvokeRequest & request, + chip::TLV::TLVReader & input_arguments, + CommandHandler * handler) { return std::make_optional(CHIP_ERROR_NOT_IMPLEMENTED); } diff --git a/src/app/tests/test-interaction-model-api.h b/src/app/tests/test-interaction-model-api.h index 54efc316da95a1..f3ad060286ee50 100644 --- a/src/app/tests/test-interaction-model-api.h +++ b/src/app/tests/test-interaction-model-api.h @@ -108,8 +108,9 @@ class TestImCustomDataModel : public CodegenDataModelProvider AttributeValueEncoder & encoder) override; DataModel::ActionReturnStatus WriteAttribute(const DataModel::WriteAttributeRequest & request, AttributeValueDecoder & decoder) override; - std::optional Invoke(const DataModel::InvokeRequest & request, - chip::TLV::TLVReader & input_arguments, CommandHandler * handler) override; + std::optional InvokeCommand(const DataModel::InvokeRequest & request, + chip::TLV::TLVReader & input_arguments, + CommandHandler * handler) override; }; } // namespace app diff --git a/src/controller/tests/data_model/DataModelFixtures.cpp b/src/controller/tests/data_model/DataModelFixtures.cpp index 930c74690392e2..55bc4e33fe06db 100644 --- a/src/controller/tests/data_model/DataModelFixtures.cpp +++ b/src/controller/tests/data_model/DataModelFixtures.cpp @@ -470,8 +470,8 @@ ActionReturnStatus CustomDataModel::WriteAttribute(const WriteAttributeRequest & return CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE; } -std::optional CustomDataModel::Invoke(const InvokeRequest & request, chip::TLV::TLVReader & input_arguments, - CommandHandler * handler) +std::optional CustomDataModel::InvokeCommand(const InvokeRequest & request, + chip::TLV::TLVReader & input_arguments, CommandHandler * handler) { DispatchSingleClusterCommand(request.path, input_arguments, handler); return std::nullopt; // handler status is set by the dispatch diff --git a/src/controller/tests/data_model/DataModelFixtures.h b/src/controller/tests/data_model/DataModelFixtures.h index d847e4b57439cf..c11cb7bfa92c5f 100644 --- a/src/controller/tests/data_model/DataModelFixtures.h +++ b/src/controller/tests/data_model/DataModelFixtures.h @@ -120,8 +120,9 @@ class CustomDataModel : public CodegenDataModelProvider AttributeValueEncoder & encoder) override; DataModel::ActionReturnStatus WriteAttribute(const DataModel::WriteAttributeRequest & request, AttributeValueDecoder & decoder) override; - std::optional Invoke(const DataModel::InvokeRequest & request, - chip::TLV::TLVReader & input_arguments, CommandHandler * handler) override; + std::optional InvokeCommand(const DataModel::InvokeRequest & request, + chip::TLV::TLVReader & input_arguments, + CommandHandler * handler) override; }; } // namespace DataModelTests diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp index 6913214ac803cc..8c84fa1740374d 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp @@ -158,9 +158,9 @@ CHIP_ERROR CodegenDataModelProvider::Startup(DataModel::InteractionModelContext return CHIP_NO_ERROR; } -std::optional CodegenDataModelProvider::Invoke(const DataModel::InvokeRequest & request, - TLV::TLVReader & input_arguments, - CommandHandler * handler) +std::optional CodegenDataModelProvider::InvokeCommand(const DataModel::InvokeRequest & request, + TLV::TLVReader & input_arguments, + CommandHandler * handler) { CommandHandlerInterface * handler_interface = CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(request.path.mEndpointId, request.path.mClusterId); diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.h b/src/data-model-providers/codegen/CodegenDataModelProvider.h index 9edea20ed6a7a7..64d6b9511ef8e8 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.h +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.h @@ -63,9 +63,10 @@ class CodegenDataModelProvider : public DataModel::Provider AttributeValueEncoder & encoder) override; DataModel::ActionReturnStatus WriteAttribute(const DataModel::WriteAttributeRequest & request, AttributeValueDecoder & decoder) override; + void ListAttributeWriteNotification(const ConcreteAttributePath & aPath, DataModel::ListWriteOperation opType) override; - std::optional Invoke(const DataModel::InvokeRequest & request, TLV::TLVReader & input_arguments, - CommandHandler * handler) override; + std::optional InvokeCommand(const DataModel::InvokeRequest & request, + TLV::TLVReader & input_arguments, CommandHandler * handler) override; /// attribute tree iteration CHIP_ERROR Endpoints(DataModel::ListBuilder & out) override; diff --git a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp index bcd9d64d88d86d..b3dfcbff972d02 100644 --- a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp +++ b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp @@ -2351,7 +2351,7 @@ TEST_F(TestCodegenModelViaMocks, EmberInvokeTest) const uint32_t kDispatchCountPre = chip::Test::DispatchCount(); // Using a handler set to nullptr as it is not used by the impl - ASSERT_EQ(model.Invoke(kInvokeRequest, tlvReader, /* handler = */ nullptr), std::nullopt); + ASSERT_EQ(model.InvokeCommand(kInvokeRequest, tlvReader, /* handler = */ nullptr), std::nullopt); EXPECT_EQ(chip::Test::DispatchCount(), kDispatchCountPre + 1); // single dispatch EXPECT_EQ(chip::Test::GetLastDispatchPath(), kCommandPath); // for the right path @@ -2365,7 +2365,7 @@ TEST_F(TestCodegenModelViaMocks, EmberInvokeTest) const uint32_t kDispatchCountPre = chip::Test::DispatchCount(); // Using a handler set to nullpotr as it is not used by the impl - ASSERT_EQ(model.Invoke(kInvokeRequest, tlvReader, /* handler = */ nullptr), std::nullopt); + ASSERT_EQ(model.InvokeCommand(kInvokeRequest, tlvReader, /* handler = */ nullptr), std::nullopt); EXPECT_EQ(chip::Test::DispatchCount(), kDispatchCountPre + 1); // single dispatch EXPECT_EQ(chip::Test::GetLastDispatchPath(), kCommandPath); // for the right path diff --git a/src/lib/core/CHIPError.cpp b/src/lib/core/CHIPError.cpp index 2068a9fbc68c42..de1f0b3862c5e3 100644 --- a/src/lib/core/CHIPError.cpp +++ b/src/lib/core/CHIPError.cpp @@ -178,6 +178,9 @@ bool FormatCHIPError(char * buf, uint16_t bufSize, CHIP_ERROR err) case CHIP_ERROR_TLV_CONTAINER_OPEN.AsInteger(): desc = "TLV container open"; break; + case CHIP_ERROR_IN_USE.AsInteger(): + desc = "In use"; + break; case CHIP_ERROR_INVALID_MESSAGE_TYPE.AsInteger(): desc = "Invalid message type"; break; diff --git a/src/lib/core/CHIPError.h b/src/lib/core/CHIPError.h index 03217378eecb87..513967d1501726 100644 --- a/src/lib/core/CHIPError.h +++ b/src/lib/core/CHIPError.h @@ -837,7 +837,16 @@ using CHIP_ERROR = ::chip::ChipError; */ #define CHIP_ERROR_TLV_CONTAINER_OPEN CHIP_CORE_ERROR(0x27) -// AVAILABLE: 0x28 +/** + * @def CHIP_ERROR_IN_USE + * + * @brief + * A value is already used. Generally indicates an unavailable resource. + * As opposed to CHIP_ERROR_BUSY, the use is not considered transient/temporary. + * + */ +#define CHIP_ERROR_IN_USE CHIP_CORE_ERROR(0x28) + // AVAILABLE: 0x29 /** From bb8909395295c0f7825a6691216ede5cd62e499f Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Mon, 3 Mar 2025 09:30:46 -0500 Subject: [PATCH 02/24] ServerClusterInterfaceRegistry adding --- src/data-model-providers/codegen/BUILD.gn | 14 + .../ServerClusterInterfaceRegistry.cpp | 224 +++++++++++++ .../codegen/ServerClusterInterfaceRegistry.h | 138 ++++++++ src/data-model-providers/codegen/model.gni | 2 + .../codegen/tests/BUILD.gn | 1 + .../TestServerClusterInterfaceRegistry.cpp | 307 ++++++++++++++++++ 6 files changed, 686 insertions(+) create mode 100644 src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp create mode 100644 src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h create mode 100644 src/data-model-providers/codegen/tests/TestServerClusterInterfaceRegistry.cpp diff --git a/src/data-model-providers/codegen/BUILD.gn b/src/data-model-providers/codegen/BUILD.gn index 3b414c595b9f67..d6517dea8366c0 100644 --- a/src/data-model-providers/codegen/BUILD.gn +++ b/src/data-model-providers/codegen/BUILD.gn @@ -45,3 +45,17 @@ source_set("instance-header") { # generally being unit tests or data_model.gni/data_model.cmake files) sources = [ "Instance.h" ] } + +source_set("registry") { + sources = [ + "ServerClusterInterfaceRegistry.cpp", + "ServerClusterInterfaceRegistry.h", + ] + + public_deps = [ + "${chip_root}/src/app:paths", + "${chip_root}/src/app/server-cluster", + "${chip_root}/src/lib/core:types", + "${chip_root}/src/lib/support", + ] +} diff --git a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp new file mode 100644 index 00000000000000..3d8f3b5fc2e720 --- /dev/null +++ b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp @@ -0,0 +1,224 @@ +/* + * Copyright (c) 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 + +#include +#include +#include +#include +#include +#include + +namespace chip { +namespace app { + +void ServerClusterInterfaceRegistry::ClearSingleLinkedList(RegisteredServerClusterInterface * clusters) +{ + while (clusters != nullptr) + { + RegisteredServerClusterInterface * next = clusters->next; + Platform::Delete(clusters); + clusters = next; + } +} + +ServerClusterInterfaceRegistry::~ServerClusterInterfaceRegistry() +{ + while (mEndpoints != nullptr) + { + UnregisterAllFromEndpoint(mEndpoints->endpointId); + } +} + +CHIP_ERROR ServerClusterInterfaceRegistry::AllocateNewEndpointClusters(EndpointId endpointId, EndpointClusters *& dest) +{ + auto result = Platform::New(); + VerifyOrReturnError(result != nullptr, CHIP_ERROR_NO_MEMORY); + + result->endpointId = endpointId; + result->firstCluster = nullptr; + result->next = mEndpoints; + mEndpoints = result; + dest = result; + + return CHIP_NO_ERROR; +} + +CHIP_ERROR ServerClusterInterfaceRegistry::Register(EndpointId endpointId, ServerClusterInterface * cluster) +{ + VerifyOrReturnError(cluster != nullptr, CHIP_ERROR_INVALID_ARGUMENT); + VerifyOrReturnError(endpointId != kInvalidEndpointId, CHIP_ERROR_INVALID_ARGUMENT); + VerifyOrReturnError(cluster->GetClusterId() != kInvalidClusterId, CHIP_ERROR_INVALID_ARGUMENT); + + // duplicate registrations are disallowed + VerifyOrReturnError(Get(ConcreteClusterPath(endpointId, cluster->GetClusterId())) == nullptr, CHIP_ERROR_DUPLICATE_KEY_ID); + + EndpointClusters * endpointClusters = FindClusters(endpointId); + if (endpointClusters == nullptr) + { + ReturnErrorOnFailure(AllocateNewEndpointClusters(endpointId, endpointClusters)); + } + + auto entry = Platform::New(cluster, endpointClusters->firstCluster); + VerifyOrReturnError(entry != nullptr, CHIP_ERROR_NO_MEMORY); + + endpointClusters->firstCluster = entry; + + return CHIP_NO_ERROR; +} + +ServerClusterInterface * ServerClusterInterfaceRegistry::Unregister(const ConcreteClusterPath & path) +{ + EndpointClusters * endpointClusters = FindClusters(path.mEndpointId); + VerifyOrReturnValue(endpointClusters != nullptr, nullptr); + VerifyOrReturnValue(endpointClusters->firstCluster != nullptr, nullptr); + + RegisteredServerClusterInterface * prev = nullptr; + RegisteredServerClusterInterface * current = endpointClusters->firstCluster; + + while (current != nullptr) + { + if (current->serverClusterInterface->GetClusterId() == path.mClusterId) + { + // take the item out of the current list and return it. + RegisteredServerClusterInterface * next = current->next; + + if (prev == nullptr) + { + endpointClusters->firstCluster = next; + } + else + { + prev->next = next; + } + + if (mCachedInterface == current->serverClusterInterface) + { + mCachedClusterEndpointId = kInvalidEndpointId; + mCachedInterface = nullptr; + } + + ServerClusterInterface * result = current->serverClusterInterface; + Platform::MemoryFree(current); + + return result; + } + + prev = current; + current = current->next; + } + + // Not found. + return nullptr; +} + +ServerClusterInterfaceRegistry::ClustersList ServerClusterInterfaceRegistry::ClustersOnEndpoint(EndpointId endpointId) +{ + EndpointClusters * clusters = FindClusters(endpointId); + + if (clusters == nullptr) + { + return nullptr; + } + + return clusters->firstCluster; +} + +void ServerClusterInterfaceRegistry::UnregisterAllFromEndpoint(EndpointId endpointId) +{ + if (mCachedClusterEndpointId == endpointId) + { + // all clusters on the given endpoint will be unregistered. + mCachedInterface = nullptr; + mCachedClusterEndpointId = kInvalidEndpointId; + } + + EndpointClusters * prev = nullptr; + EndpointClusters * current = mEndpoints; + while (current != nullptr) + { + if (current->endpointId == endpointId) + { + if (prev == nullptr) + { + mEndpoints = current->next; + } + else + { + prev->next = current->next; + } + ClearSingleLinkedList(current->firstCluster); + Platform::Delete(current); + return; + } + + prev = current; + current = current->next; + } +} + +ServerClusterInterface * ServerClusterInterfaceRegistry::Get(const ConcreteClusterPath & path) +{ + EndpointClusters * endpointClusters = FindClusters(path.mEndpointId); + VerifyOrReturnValue(endpointClusters != nullptr, nullptr); + VerifyOrReturnValue(endpointClusters->firstCluster != nullptr, nullptr); + + // Check the cache to speed things up + if ((mCachedClusterEndpointId == path.mEndpointId) && (mCachedInterface != nullptr) && + (mCachedInterface->GetClusterId() == path.mClusterId)) + { + return mCachedInterface; + } + + // The cluster searched for is not cached, do a linear search for it + RegisteredServerClusterInterface * current = endpointClusters->firstCluster; + + while (current != nullptr) + { + if (current->serverClusterInterface->GetClusterId() == path.mClusterId) + { + mCachedClusterEndpointId = path.mEndpointId; + mCachedInterface = current->serverClusterInterface; + return mCachedInterface; + } + + current = current->next; + } + + // not found + return nullptr; +} + +ServerClusterInterfaceRegistry::EndpointClusters * ServerClusterInterfaceRegistry::FindClusters(EndpointId endpointId) +{ + // invalid cluster id is NOT acceptable (since static allocation uses the + // invalid cluster id as a marker of "not used") + VerifyOrReturnValue(endpointId != kInvalidEndpointId, nullptr); + + for (EndpointClusters * p = mEndpoints; p != nullptr; p = p->next) + { + if (p->endpointId == endpointId) + { + return p; + } + } + + return nullptr; +} + +} // namespace app +} // namespace chip diff --git a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h new file mode 100644 index 00000000000000..a72ccd49bbe95c --- /dev/null +++ b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h @@ -0,0 +1,138 @@ +/* + * Copyright (c) 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. + */ +#pragma once + +#include +#include +#include +#include + +namespace chip { +namespace app { + +/// Allows registering and retrieving ServerClusterInterface instances for specific cluster paths. +class ServerClusterInterfaceRegistry +{ +private: + /// A single-linked list of registered server cluster interfaces + struct RegisteredServerClusterInterface + { + // A single-linked list of clusters registered for the given `endpointId` + ServerClusterInterface * serverClusterInterface = nullptr; + RegisteredServerClusterInterface * next = nullptr; + + constexpr RegisteredServerClusterInterface(ServerClusterInterface * cluster, RegisteredServerClusterInterface * n) : + serverClusterInterface(cluster), next(n) + {} + }; + +public: + /// represents an iterable list of clusters + class ClustersList + { + public: + class Iterator + { + public: + Iterator(RegisteredServerClusterInterface * interface) : mInterface(interface) {} + + Iterator & operator++() + { + if (mInterface != nullptr) + { + mInterface = mInterface->next; + } + return *this; + } + bool operator==(const Iterator & other) const { return mInterface == other.mInterface; } + bool operator!=(const Iterator & other) const { return mInterface != other.mInterface; } + ServerClusterInterface * operator*() { return mInterface->serverClusterInterface; } + + private: + RegisteredServerClusterInterface * mInterface; + }; + + ClustersList(RegisteredServerClusterInterface * start) : mStart(start) {} + Iterator begin() { return mStart; } + Iterator end() { return nullptr; } + + private: + RegisteredServerClusterInterface * mStart; + }; + + ~ServerClusterInterfaceRegistry(); + + /// Associate a specific interface with the given endpoint. + /// + /// There can be only a single registration for a given `endpointId/clusterId` path. + /// A registration will return an error if a registration already exists on + /// the given `endpointId/cluster->GetClusterID()` already exists + /// + /// Registrations need a valid endpointId and cluster MUST return a valid cluster id. + [[nodiscard]] CHIP_ERROR Register(EndpointId endpointId, ServerClusterInterface * cluster); + + /// Remove an existing registration for a given endpoint/cluster path. + /// + /// Returns the previous registration if any exists (or nullptr if nothing + /// to unregister) + ServerClusterInterface * Unregister(const ConcreteClusterPath & path); + + /// Return the interface registered for the given cluster path or nullptr if one does not exist + ServerClusterInterface * Get(const ConcreteClusterPath & path); + + /// ClustersList is valid as a snapshot only and should not be saved. + ClustersList ClustersOnEndpoint(EndpointId endpointId); + + /// Unregister all registrations for the given endpoint. + void UnregisterAllFromEndpoint(EndpointId endpointId); + +private: + /// tracks clusters registered to a particular endpoint + struct EndpointClusters + { + // The endpointId for this registration. kInvalidEndpointId means + // not allocated/used + EndpointId endpointId = kInvalidEndpointId; + + // A single-linked list of clusters registered for the given `endpointId` + RegisteredServerClusterInterface * firstCluster = nullptr; + + /// A single-linked list of endpoint clusters that is dynamically + /// allocated. + EndpointClusters * next; + }; + + // Dynamic allocated endpoint cluters. + EndpointClusters * mEndpoints = nullptr; + + // A one-element cache to speed up finding a cluster within an endpoint. + // The endpointId specifies which endpoint the cache belongs to. + ClusterId mCachedClusterEndpointId = kInvalidEndpointId; + ServerClusterInterface * mCachedInterface = nullptr; + + /// returns nullptr if not found + EndpointClusters * FindClusters(EndpointId endpointId); + + /// Get a new usable endpoint cluster + CHIP_ERROR AllocateNewEndpointClusters(EndpointId endpointId, EndpointClusters *& dest); + + /// Clear and free memory for the given linked list + static void ClearSingleLinkedList(RegisteredServerClusterInterface * clusters); +}; + +} // namespace app +} // namespace chip diff --git a/src/data-model-providers/codegen/model.gni b/src/data-model-providers/codegen/model.gni index 51305a5fe30b5a..f9a8295fcb8593 100644 --- a/src/data-model-providers/codegen/model.gni +++ b/src/data-model-providers/codegen/model.gni @@ -39,6 +39,8 @@ codegen_data_model_SOURCES = [ codegen_data_model_PUBLIC_DEPS = [ "${chip_root}/src/app/common:attribute-type", "${chip_root}/src/app/data-model-provider", + "${chip_root}/src/app/server-cluster", "${chip_root}/src/data-model-providers/codegen:instance-header", + "${chip_root}/src/data-model-providers/codegen:registry", "${chip_root}/src/app/util/persistence", ] diff --git a/src/data-model-providers/codegen/tests/BUILD.gn b/src/data-model-providers/codegen/tests/BUILD.gn index 57facbee09ec10..a2d4fdec2cbee1 100644 --- a/src/data-model-providers/codegen/tests/BUILD.gn +++ b/src/data-model-providers/codegen/tests/BUILD.gn @@ -54,6 +54,7 @@ chip_test_suite("tests") { test_sources = [ "TestCodegenModelViaMocks.cpp", "TestEmberAttributeDataBuffer.cpp", + "TestServerClusterInterfaceRegistry.cpp", ] cflags = [ "-Wconversion" ] diff --git a/src/data-model-providers/codegen/tests/TestServerClusterInterfaceRegistry.cpp b/src/data-model-providers/codegen/tests/TestServerClusterInterfaceRegistry.cpp new file mode 100644 index 00000000000000..6731cf3a757f1d --- /dev/null +++ b/src/data-model-providers/codegen/tests/TestServerClusterInterfaceRegistry.cpp @@ -0,0 +1,307 @@ +/* + * Copyright (c) 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 + +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include + +using namespace chip; +using namespace chip::app; +using namespace chip::app::DataModel; +using namespace chip::app::Clusters; + +namespace { + +constexpr chip::EndpointId kEp1 = 1; +constexpr chip::EndpointId kEp2 = 2; +constexpr chip::EndpointId kEp3 = 3; +constexpr chip::ClusterId kCluster1 = 1; +constexpr chip::ClusterId kCluster2 = 2; +constexpr chip::ClusterId kCluster3 = 3; + +class FakeServerClusterInterface : public DefaultServerCluster +{ +public: + FakeServerClusterInterface(ClusterId id) : mClusterId(id) {} + + ClusterId GetClusterId() const override { return mClusterId; } + + DataModel::ActionReturnStatus ReadAttribute(const DataModel::ReadAttributeRequest & request, + AttributeValueEncoder & encoder) override + { + switch (request.path.mAttributeId) + { + case Globals::Attributes::FeatureMap::Id: + return encoder.Encode(0); + case Globals::Attributes::ClusterRevision::Id: + return encoder.Encode(123); + } + return CHIP_ERROR_INVALID_ARGUMENT; + } + +private: + ClusterId mClusterId; +}; + +struct TestServerClusterInterfaceRegistry : public ::testing::Test +{ + static void SetUpTestSuite() { ASSERT_EQ(chip::Platform::MemoryInit(), CHIP_NO_ERROR); } + static void TearDownTestSuite() { chip::Platform::MemoryShutdown(); } +}; + +} // namespace + +TEST_F(TestServerClusterInterfaceRegistry, BasicTest) +{ + // NOTE: tests DO NOT use the global registry and validate implementation + // details. + + ServerClusterInterfaceRegistry registry; + + FakeServerClusterInterface cluster1(kCluster1); + FakeServerClusterInterface cluster2(kCluster2); + FakeServerClusterInterface cluster3(kCluster3); + + // there should be nothing registered to start with. + EXPECT_EQ(registry.Get(ConcreteClusterPath(kEp1, kCluster1)), nullptr); + EXPECT_EQ(registry.Get(ConcreteClusterPath(kEp1, kCluster2)), nullptr); + EXPECT_EQ(registry.Get(ConcreteClusterPath(kEp2, kCluster2)), nullptr); + EXPECT_EQ(registry.Get(ConcreteClusterPath(kEp2, kCluster3)), nullptr); + EXPECT_EQ(registry.Get(ConcreteClusterPath(kInvalidEndpointId, kCluster2)), nullptr); + EXPECT_EQ(registry.Get(ConcreteClusterPath(kEp1, kInvalidClusterId)), nullptr); + + // registration of invalid values is not acceptable + EXPECT_EQ(registry.Register(kEp1, nullptr), CHIP_ERROR_INVALID_ARGUMENT); + EXPECT_EQ(registry.Register(kInvalidEndpointId, nullptr), CHIP_ERROR_INVALID_ARGUMENT); + EXPECT_EQ(registry.Register(kInvalidEndpointId, &cluster1), CHIP_ERROR_INVALID_ARGUMENT); + EXPECT_EQ(registry.Register(kInvalidEndpointId, &cluster2), CHIP_ERROR_INVALID_ARGUMENT); + { + FakeServerClusterInterface badCluster(kInvalidClusterId); + EXPECT_EQ(registry.Register(kInvalidEndpointId, &badCluster), CHIP_ERROR_INVALID_ARGUMENT); + } + + // should be able to register + EXPECT_EQ(registry.Register(kEp1, &cluster1), CHIP_NO_ERROR); + EXPECT_EQ(registry.Register(kEp2, &cluster2), CHIP_NO_ERROR); + EXPECT_EQ(registry.Register(kEp2, &cluster3), CHIP_NO_ERROR); + + // cannot register two implementations on the same path + { + FakeServerClusterInterface another1(kCluster1); + EXPECT_EQ(registry.Register(kEp1, &another1), CHIP_ERROR_DUPLICATE_KEY_ID); + } + + // Items can be found back + EXPECT_EQ(registry.Get({ kEp1, kCluster1 }), &cluster1); + EXPECT_EQ(registry.Get({ kEp2, kCluster2 }), &cluster2); + EXPECT_EQ(registry.Get({ kEp2, kCluster3 }), &cluster3); + + EXPECT_EQ(registry.Get({ kEp2, kCluster1 }), nullptr); + EXPECT_EQ(registry.Get({ kEp1, kCluster2 }), nullptr); + + // repeated calls work + EXPECT_EQ(registry.Get({ kEp1, kCluster2 }), nullptr); + EXPECT_EQ(registry.Get({ kEp1, kCluster2 }), nullptr); + EXPECT_EQ(registry.Get({ kEp1, kCluster2 }), nullptr); + EXPECT_EQ(registry.Get({ kEp2, kCluster1 }), nullptr); + EXPECT_EQ(registry.Get({ kEp2, kCluster1 }), nullptr); + EXPECT_EQ(registry.Get({ kEp2, kCluster1 }), nullptr); + + // remove registrations + EXPECT_EQ(registry.Unregister({ kEp2, kCluster2 }), &cluster2); + EXPECT_EQ(registry.Unregister({ kEp2, kCluster2 }), nullptr); + + // Re-adding works exactly once. + EXPECT_EQ(registry.Get({ kEp2, kCluster2 }), nullptr); + EXPECT_EQ(registry.Register(kEp3, &cluster2), CHIP_NO_ERROR); + EXPECT_EQ(registry.Get({ kEp3, kCluster2 }), &cluster2); + EXPECT_EQ(registry.Unregister({ kEp3, kCluster2 }), &cluster2); + EXPECT_EQ(registry.Get({ kEp2, kCluster2 }), nullptr); + EXPECT_EQ(registry.Unregister({ kEp3, kCluster2 }), nullptr); + EXPECT_EQ(registry.Get({ kEp2, kCluster2 }), nullptr); + + // cannot get it anymore once removed, others are still valid + EXPECT_EQ(registry.Get({ kEp2, kCluster2 }), nullptr); + EXPECT_EQ(registry.Get({ kEp2, kCluster3 }), &cluster3); + + // clean of an entire endpoint works + registry.UnregisterAllFromEndpoint(kEp2); + EXPECT_EQ(registry.Get({ kEp1, kCluster1 }), &cluster1); + EXPECT_EQ(registry.Get({ kEp2, kCluster2 }), nullptr); + EXPECT_EQ(registry.Get({ kEp2, kCluster3 }), nullptr); + + registry.UnregisterAllFromEndpoint(kEp1); + EXPECT_EQ(registry.Get({ kEp1, kCluster1 }), nullptr); + EXPECT_EQ(registry.Get({ kEp2, kCluster2 }), nullptr); + EXPECT_EQ(registry.Get({ kEp2, kCluster3 }), nullptr); +} + +TEST_F(TestServerClusterInterfaceRegistry, StressTest) +{ + // make the test repeatable + srand(1234); + + std::vector items; + + static constexpr ClusterId kClusterTestCount = 200; + static constexpr EndpointId kEndpointTestCount = 10; + static constexpr size_t kTestIterations = 4; + + static_assert(kInvalidClusterId > kClusterTestCount, "Tests assume all clusters IDs [0...] are valid"); + static_assert(kTestIterations > 1, "Tests use different unregister methods. Need 2 or more passes."); + + items.reserve(kClusterTestCount); + for (ClusterId i = 0; i < kClusterTestCount; i++) + { + items.emplace_back(i); + } + + ServerClusterInterfaceRegistry registry; + + for (size_t test = 0; test < kTestIterations; test++) + { + std::vector endpoint_placement; + endpoint_placement.reserve(kClusterTestCount); + + for (ClusterId i = 0; i < kClusterTestCount; i++) + { + auto endpointId = static_cast(rand() % kEndpointTestCount); + endpoint_placement.push_back(endpointId); + ASSERT_EQ(registry.Register(endpointId, &items[i]), CHIP_NO_ERROR); + } + + // test that getters work + for (ClusterId cluster = 0; cluster < kClusterTestCount; cluster++) + { + for (EndpointId ep = 0; ep < kEndpointTestCount; ep++) + { + if (endpoint_placement[cluster] == ep) + { + ASSERT_EQ(registry.Get({ ep, cluster }), &items[cluster]); + } + else + { + ASSERT_EQ(registry.Get({ ep, cluster }), nullptr); + } + } + } + + // clear endpoints. Stress test, unregister in different ways (bulk vs individual) + if (test % 2 == 1) + { + // shuffle unregister + std::vector unregister_order; + unregister_order.reserve(kClusterTestCount); + for (size_t i = 0; i < kClusterTestCount; i++) + { + unregister_order.push_back(i); + } + + std::default_random_engine eng(static_cast(rand())); + std::shuffle(unregister_order.begin(), unregister_order.end(), eng); + + // unregister + for (auto cluster : unregister_order) + { + // item MUST exist and be accessible + ASSERT_EQ(registry.Get({ endpoint_placement[cluster], static_cast(cluster) }), &items[cluster]); + ASSERT_EQ(registry.Unregister({ endpoint_placement[cluster], static_cast(cluster) }), &items[cluster]); + + // once unregistered, it is not there anymore + ASSERT_EQ(registry.Get({ endpoint_placement[cluster], static_cast(cluster) }), nullptr); + ASSERT_EQ(registry.Unregister({ endpoint_placement[cluster], static_cast(cluster) }), nullptr); + } + } + else + { + // bulk unregister + for (EndpointId ep = 0; ep < kEndpointTestCount; ep++) + { + registry.UnregisterAllFromEndpoint(ep); + } + } + + // all endpoints should be clear + for (ClusterId cluster = 0; cluster < kClusterTestCount; cluster++) + { + for (EndpointId ep = 0; ep < kEndpointTestCount; ep++) + { + ASSERT_EQ(registry.Get({ ep, cluster }), nullptr); + } + } + } +} + +TEST_F(TestServerClusterInterfaceRegistry, ClustersOnEndpoint) +{ + std::vector items; + + static constexpr ClusterId kClusterTestCount = 200; + static constexpr EndpointId kEndpointTestCount = 10; + + static_assert(kInvalidClusterId > kClusterTestCount, "Tests assume all clusters IDs [0...] are valid"); + + items.reserve(kClusterTestCount); + for (ClusterId i = 0; i < kClusterTestCount; i++) + { + items.emplace_back(i); + } + + ServerClusterInterfaceRegistry registry; + + // place the clusters on the respecitve endpoints + for (ClusterId i = 0; i < kClusterTestCount; i++) + { + ASSERT_EQ(registry.Register(static_cast(i % kEndpointTestCount), &items[i]), CHIP_NO_ERROR); + } + + // this IS implementation defined: we always register at "HEAD" so the listing is in + // INVERSE order of registering. + for (EndpointId ep = 0; ep < kEndpointTestCount; ep++) + { + // Move to the end since we iterate in reverse order + ClusterId expectedClusterId = ep + kEndpointTestCount * (kClusterTestCount / kEndpointTestCount); + if (expectedClusterId >= kClusterTestCount) + { + expectedClusterId -= kEndpointTestCount; + } + + // ensure that iteration happens exactly as we expect: reverse order and complete + for (auto cluster : registry.ClustersOnEndpoint(ep)) + { + ASSERT_LT(expectedClusterId, kClusterTestCount); + ASSERT_EQ(cluster->GetClusterId(), expectedClusterId); + expectedClusterId -= kEndpointTestCount; // next expected/registered cluster + } + + // Iterated through all : we overflowed and got a large number + ASSERT_GE(expectedClusterId, kClusterTestCount); + } + + // invalid index works and iteration on empty lists is ok + auto clusters = registry.ClustersOnEndpoint(kEndpointTestCount + 1); + ASSERT_EQ(clusters.begin(), clusters.end()); +} From 7fdd6b4e14e2ad4cb5ed47dc35710662ed2a41ca Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 4 Mar 2025 10:52:38 -0500 Subject: [PATCH 03/24] Update src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h Co-authored-by: Terence Hampson --- .../codegen/ServerClusterInterfaceRegistry.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h index a72ccd49bbe95c..274b267705bbbc 100644 --- a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h +++ b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h @@ -82,7 +82,7 @@ class ServerClusterInterfaceRegistry /// A registration will return an error if a registration already exists on /// the given `endpointId/cluster->GetClusterID()` already exists /// - /// Registrations need a valid endpointId and cluster MUST return a valid cluster id. + /// Registrations need a valid `endpointId` and `cluster->GetClusterID()` MUST be a valid cluster id. [[nodiscard]] CHIP_ERROR Register(EndpointId endpointId, ServerClusterInterface * cluster); /// Remove an existing registration for a given endpoint/cluster path. From 0a6a7f9198a78be3b6c1ac3324714ed5eab28ec8 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 4 Mar 2025 10:59:00 -0500 Subject: [PATCH 04/24] Code review: forward declare --- .../codegen/ServerClusterInterfaceRegistry.h | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h index 274b267705bbbc..1f7ea7d2f6e0fb 100644 --- a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h +++ b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h @@ -28,18 +28,7 @@ namespace app { class ServerClusterInterfaceRegistry { private: - /// A single-linked list of registered server cluster interfaces - struct RegisteredServerClusterInterface - { - // A single-linked list of clusters registered for the given `endpointId` - ServerClusterInterface * serverClusterInterface = nullptr; - RegisteredServerClusterInterface * next = nullptr; - - constexpr RegisteredServerClusterInterface(ServerClusterInterface * cluster, RegisteredServerClusterInterface * n) : - serverClusterInterface(cluster), next(n) - {} - }; - + struct RegisteredServerClusterInterface; public: /// represents an iterable list of clusters class ClustersList @@ -101,6 +90,18 @@ class ServerClusterInterfaceRegistry void UnregisterAllFromEndpoint(EndpointId endpointId); private: + /// A single-linked list of registered server cluster interfaces + struct RegisteredServerClusterInterface + { + // A single-linked list of clusters registered for the given `endpointId` + ServerClusterInterface * serverClusterInterface = nullptr; + RegisteredServerClusterInterface * next = nullptr; + + constexpr RegisteredServerClusterInterface(ServerClusterInterface * cluster, RegisteredServerClusterInterface * n) : + serverClusterInterface(cluster), next(n) + {} + }; + /// tracks clusters registered to a particular endpoint struct EndpointClusters { From ab8a9b2e8e5ba059f8897cfdc2bbfaae809e6fdf Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 4 Mar 2025 11:00:00 -0500 Subject: [PATCH 05/24] Review comment: switch order of members --- .../codegen/ServerClusterInterfaceRegistry.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h index 1f7ea7d2f6e0fb..dd1b3098d77431 100644 --- a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h +++ b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h @@ -117,14 +117,6 @@ class ServerClusterInterfaceRegistry EndpointClusters * next; }; - // Dynamic allocated endpoint cluters. - EndpointClusters * mEndpoints = nullptr; - - // A one-element cache to speed up finding a cluster within an endpoint. - // The endpointId specifies which endpoint the cache belongs to. - ClusterId mCachedClusterEndpointId = kInvalidEndpointId; - ServerClusterInterface * mCachedInterface = nullptr; - /// returns nullptr if not found EndpointClusters * FindClusters(EndpointId endpointId); @@ -133,6 +125,14 @@ class ServerClusterInterfaceRegistry /// Clear and free memory for the given linked list static void ClearSingleLinkedList(RegisteredServerClusterInterface * clusters); + + // Dynamic allocated endpoint cluters. + EndpointClusters * mEndpoints = nullptr; + + // A one-element cache to speed up finding a cluster within an endpoint. + // The endpointId specifies which endpoint the cache belongs to. + ClusterId mCachedClusterEndpointId = kInvalidEndpointId; + ServerClusterInterface * mCachedInterface = nullptr; }; } // namespace app From d9aa48efd4e38784e730691f2dcee04b6f2e0646 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 4 Mar 2025 11:01:33 -0500 Subject: [PATCH 06/24] Updated logic: the condition is not relevant anymore --- .../codegen/ServerClusterInterfaceRegistry.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp index 3d8f3b5fc2e720..f5d38fda1b2575 100644 --- a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp +++ b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp @@ -205,10 +205,6 @@ ServerClusterInterface * ServerClusterInterfaceRegistry::Get(const ConcreteClust ServerClusterInterfaceRegistry::EndpointClusters * ServerClusterInterfaceRegistry::FindClusters(EndpointId endpointId) { - // invalid cluster id is NOT acceptable (since static allocation uses the - // invalid cluster id as a marker of "not used") - VerifyOrReturnValue(endpointId != kInvalidEndpointId, nullptr); - for (EndpointClusters * p = mEndpoints; p != nullptr; p = p->next) { if (p->endpointId == endpointId) From a062ca2ada21ab988896b3fb90d6c2f3df5ddda0 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 4 Mar 2025 11:06:35 -0500 Subject: [PATCH 07/24] Code review: added comment --- .../codegen/ServerClusterInterfaceRegistry.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp index f5d38fda1b2575..e550777bd35042 100644 --- a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp +++ b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp @@ -74,6 +74,12 @@ CHIP_ERROR ServerClusterInterfaceRegistry::Register(EndpointId endpointId, Serve } auto entry = Platform::New(cluster, endpointClusters->firstCluster); + + // If this fails, we still have the `AllocateNewEndpointClusters` succeeding, + // so an empty list for this cluster exists. No cleanup for now (smaller code) + // as an empty list will probably not cause problems (this is the same as all + // clusters on an endpoint getting unregistered - we do not clean mEndpoints except + // in UnregisterAllFromEndpoint) VerifyOrReturnError(entry != nullptr, CHIP_ERROR_NO_MEMORY); endpointClusters->firstCluster = entry; From a92317ca7f182f078e0bedcc3dafa5d2d4665819 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 4 Mar 2025 11:07:05 -0500 Subject: [PATCH 08/24] Restyle --- .../codegen/ServerClusterInterfaceRegistry.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h index dd1b3098d77431..6488cb8ff413ab 100644 --- a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h +++ b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h @@ -29,6 +29,7 @@ class ServerClusterInterfaceRegistry { private: struct RegisteredServerClusterInterface; + public: /// represents an iterable list of clusters class ClustersList From b60ebad6c5ebbee5de6ea9f68b5b0c0946010e8e Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 5 Mar 2025 16:45:11 -0500 Subject: [PATCH 09/24] Comment update --- .../codegen/ServerClusterInterfaceRegistry.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h index 6488cb8ff413ab..c0f837a61d2e4c 100644 --- a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h +++ b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h @@ -84,7 +84,10 @@ class ServerClusterInterfaceRegistry /// Return the interface registered for the given cluster path or nullptr if one does not exist ServerClusterInterface * Get(const ConcreteClusterPath & path); - /// ClustersList is valid as a snapshot only and should not be saved. + /// Provides a list of clusters that are registered for the given endpoint. + /// + /// As ClustersList points inside the internal registrations of the registry, + /// the list is only valid as long as the registry is not modified. ClustersList ClustersOnEndpoint(EndpointId endpointId); /// Unregister all registrations for the given endpoint. From 2daa37c9df3ee13ae7d4ad0f8e98dc89684f45a4 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 6 Mar 2025 10:13:10 -0500 Subject: [PATCH 10/24] Updates to make callers use memory allocation --- .../server-cluster/ServerClusterInterface.h | 7 +- .../tests/TestDefaultServerCluster.cpp | 19 ++- .../ServerClusterInterfaceRegistry.cpp | 154 +++++------------- .../codegen/ServerClusterInterfaceRegistry.h | 108 ++++++------ .../TestServerClusterInterfaceRegistry.cpp | 123 ++++++++------ 5 files changed, 179 insertions(+), 232 deletions(-) diff --git a/src/app/server-cluster/ServerClusterInterface.h b/src/app/server-cluster/ServerClusterInterface.h index 086a37cbab529b..53460f763547f6 100644 --- a/src/app/server-cluster/ServerClusterInterface.h +++ b/src/app/server-cluster/ServerClusterInterface.h @@ -43,7 +43,12 @@ class ServerClusterInterface virtual ~ServerClusterInterface() = default; ///////////////////////////////////// Cluster Metadata Support ////////////////////////////////////////////////// - [[nodiscard]] virtual ClusterId GetClusterId() const = 0; + + /// The path where this cluster operates on. + /// + /// This path (endpointid,clusterid) is expected to be fixed once the server + /// cluster interface is in use. + [[nodiscard]] virtual ConcreteClusterPath GetPath() const = 0; /// Gets the data version for this cluster instance. /// diff --git a/src/app/server-cluster/tests/TestDefaultServerCluster.cpp b/src/app/server-cluster/tests/TestDefaultServerCluster.cpp index a44791625d4a5b..d34ccff1794033 100644 --- a/src/app/server-cluster/tests/TestDefaultServerCluster.cpp +++ b/src/app/server-cluster/tests/TestDefaultServerCluster.cpp @@ -14,6 +14,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +#include "app/ConcreteClusterPath.h" #include #include @@ -44,9 +45,9 @@ namespace { class FakeDefaultServerCluster : public DefaultServerCluster { public: - FakeDefaultServerCluster(ClusterId id) : mClusterId(id) {} + FakeDefaultServerCluster(ConcreteClusterPath path) : mPath(path) {} - ClusterId GetClusterId() const override { return mClusterId; } + [[nodiscard]] ConcreteClusterPath GetPath() const override { return mPath; } DataModel::ActionReturnStatus ReadAttribute(const DataModel::ReadAttributeRequest & request, AttributeValueEncoder & encoder) override @@ -64,14 +65,14 @@ class FakeDefaultServerCluster : public DefaultServerCluster void TestIncreaseDataVersion() { IncreaseDataVersion(); } private: - ClusterId mClusterId; + ConcreteClusterPath mPath; }; } // namespace TEST(TestDefaultServerCluster, TestDataVersion) { - FakeDefaultServerCluster cluster(1); + FakeDefaultServerCluster cluster({ 1, 2 }); DataVersion v1 = cluster.GetDataVersion(); cluster.TestIncreaseDataVersion(); @@ -80,13 +81,13 @@ TEST(TestDefaultServerCluster, TestDataVersion) TEST(TestDefaultServerCluster, TestFlagsDefault) { - FakeDefaultServerCluster cluster(1); + FakeDefaultServerCluster cluster({ 1, 2 }); ASSERT_EQ(cluster.GetClusterFlags().Raw(), 0u); } TEST(TestDefaultServerCluster, AttributesDefault) { - FakeDefaultServerCluster cluster(1); + FakeDefaultServerCluster cluster({ 1, 2 }); DataModel::ListBuilder attributes; @@ -114,7 +115,7 @@ TEST(TestDefaultServerCluster, AttributesDefault) TEST(TestDefaultServerCluster, CommandsDefault) { - FakeDefaultServerCluster cluster(1); + FakeDefaultServerCluster cluster({ 1, 2 }); DataModel::ListBuilder acceptedCommands; ASSERT_EQ(cluster.AcceptedCommands({ 1, 1 }, acceptedCommands), CHIP_NO_ERROR); @@ -127,7 +128,7 @@ TEST(TestDefaultServerCluster, CommandsDefault) TEST(TestDefaultServerCluster, WriteAttributeDefault) { - FakeDefaultServerCluster cluster(1); + FakeDefaultServerCluster cluster({ 1, 2 }); WriteOperation test(0 /* endpoint */, 1 /* cluster */, 1234 /* attribute */); test.SetSubjectDescriptor(kAdminSubjectDescriptor); @@ -140,7 +141,7 @@ TEST(TestDefaultServerCluster, WriteAttributeDefault) TEST(TestDefaultServerCluster, InvokeDefault) { - FakeDefaultServerCluster cluster(1); + FakeDefaultServerCluster cluster({ 1, 2 }); TLV::TLVReader tlvReader; InvokeRequest request; diff --git a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp index e550777bd35042..65aad7a53c986a 100644 --- a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp +++ b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp @@ -26,86 +26,52 @@ namespace chip { namespace app { -void ServerClusterInterfaceRegistry::ClearSingleLinkedList(RegisteredServerClusterInterface * clusters) -{ - while (clusters != nullptr) - { - RegisteredServerClusterInterface * next = clusters->next; - Platform::Delete(clusters); - clusters = next; - } -} - ServerClusterInterfaceRegistry::~ServerClusterInterfaceRegistry() { - while (mEndpoints != nullptr) + while (mRegistrations != nullptr) { - UnregisterAllFromEndpoint(mEndpoints->endpointId); + ServerClusterRegistration * next = mRegistrations->next; + mRegistrations->next = nullptr; + mRegistrations = next; } } -CHIP_ERROR ServerClusterInterfaceRegistry::AllocateNewEndpointClusters(EndpointId endpointId, EndpointClusters *& dest) +CHIP_ERROR ServerClusterInterfaceRegistry::Register(ServerClusterRegistration & entry) { - auto result = Platform::New(); - VerifyOrReturnError(result != nullptr, CHIP_ERROR_NO_MEMORY); + // we have no strong way to check if entry is already registered somewhere else, so we use "next" as some + // form of double-check + VerifyOrReturnError(entry.next == nullptr, CHIP_ERROR_INVALID_ARGUMENT); + VerifyOrReturnError(entry.serverClusterInterface != nullptr, CHIP_ERROR_INVALID_ARGUMENT); - result->endpointId = endpointId; - result->firstCluster = nullptr; - result->next = mEndpoints; - mEndpoints = result; - dest = result; + ConcreteClusterPath path = entry.serverClusterInterface->GetPath(); - return CHIP_NO_ERROR; -} - -CHIP_ERROR ServerClusterInterfaceRegistry::Register(EndpointId endpointId, ServerClusterInterface * cluster) -{ - VerifyOrReturnError(cluster != nullptr, CHIP_ERROR_INVALID_ARGUMENT); - VerifyOrReturnError(endpointId != kInvalidEndpointId, CHIP_ERROR_INVALID_ARGUMENT); - VerifyOrReturnError(cluster->GetClusterId() != kInvalidClusterId, CHIP_ERROR_INVALID_ARGUMENT); + VerifyOrReturnError(path.HasValidIds(), CHIP_ERROR_INVALID_ARGUMENT); - // duplicate registrations are disallowed - VerifyOrReturnError(Get(ConcreteClusterPath(endpointId, cluster->GetClusterId())) == nullptr, CHIP_ERROR_DUPLICATE_KEY_ID); + // Double-checking for duplicates makes the checks O(n^2) on the total number of registered + // items. We preserve this however we may want to make this optional at some point in time. + VerifyOrReturnError(Get(path) == nullptr, CHIP_ERROR_DUPLICATE_KEY_ID); - EndpointClusters * endpointClusters = FindClusters(endpointId); - if (endpointClusters == nullptr) - { - ReturnErrorOnFailure(AllocateNewEndpointClusters(endpointId, endpointClusters)); - } - - auto entry = Platform::New(cluster, endpointClusters->firstCluster); - - // If this fails, we still have the `AllocateNewEndpointClusters` succeeding, - // so an empty list for this cluster exists. No cleanup for now (smaller code) - // as an empty list will probably not cause problems (this is the same as all - // clusters on an endpoint getting unregistered - we do not clean mEndpoints except - // in UnregisterAllFromEndpoint) - VerifyOrReturnError(entry != nullptr, CHIP_ERROR_NO_MEMORY); - - endpointClusters->firstCluster = entry; + entry.next = mRegistrations; + mRegistrations = &entry; return CHIP_NO_ERROR; } ServerClusterInterface * ServerClusterInterfaceRegistry::Unregister(const ConcreteClusterPath & path) { - EndpointClusters * endpointClusters = FindClusters(path.mEndpointId); - VerifyOrReturnValue(endpointClusters != nullptr, nullptr); - VerifyOrReturnValue(endpointClusters->firstCluster != nullptr, nullptr); - - RegisteredServerClusterInterface * prev = nullptr; - RegisteredServerClusterInterface * current = endpointClusters->firstCluster; + ServerClusterRegistration * prev = nullptr; + ServerClusterRegistration * current = mRegistrations; while (current != nullptr) { - if (current->serverClusterInterface->GetClusterId() == path.mClusterId) + if (current->serverClusterInterface->GetPath() == path) { // take the item out of the current list and return it. - RegisteredServerClusterInterface * next = current->next; + ServerClusterRegistration * next = current->next; if (prev == nullptr) { - endpointClusters->firstCluster = next; + mRegistrations = next; } else { @@ -114,14 +80,11 @@ ServerClusterInterface * ServerClusterInterfaceRegistry::Unregister(const Concre if (mCachedInterface == current->serverClusterInterface) { - mCachedClusterEndpointId = kInvalidEndpointId; - mCachedInterface = nullptr; + mCachedInterface = nullptr; } - ServerClusterInterface * result = current->serverClusterInterface; - Platform::MemoryFree(current); - - return result; + current->next = nullptr; // some clearing + return current->serverClusterInterface; } prev = current; @@ -134,71 +97,57 @@ ServerClusterInterface * ServerClusterInterfaceRegistry::Unregister(const Concre ServerClusterInterfaceRegistry::ClustersList ServerClusterInterfaceRegistry::ClustersOnEndpoint(EndpointId endpointId) { - EndpointClusters * clusters = FindClusters(endpointId); - - if (clusters == nullptr) - { - return nullptr; - } - - return clusters->firstCluster; + return { mRegistrations, endpointId }; } void ServerClusterInterfaceRegistry::UnregisterAllFromEndpoint(EndpointId endpointId) { - if (mCachedClusterEndpointId == endpointId) - { - // all clusters on the given endpoint will be unregistered. - mCachedInterface = nullptr; - mCachedClusterEndpointId = kInvalidEndpointId; - } - - EndpointClusters * prev = nullptr; - EndpointClusters * current = mEndpoints; + ServerClusterRegistration * prev = nullptr; + ServerClusterRegistration * current = mRegistrations; while (current != nullptr) { - if (current->endpointId == endpointId) + if (current->serverClusterInterface->GetPath().mEndpointId == endpointId) { + if (mCachedInterface == current->serverClusterInterface) + { + mCachedInterface = nullptr; + } if (prev == nullptr) { - mEndpoints = current->next; + mRegistrations = current->next; } else { prev->next = current->next; } - ClearSingleLinkedList(current->firstCluster); - Platform::Delete(current); - return; + ServerClusterRegistration * actual_next = current->next; + current->next = nullptr; // some clearing + current = actual_next; + } + else + { + prev = current; + current = current->next; } - - prev = current; - current = current->next; } } ServerClusterInterface * ServerClusterInterfaceRegistry::Get(const ConcreteClusterPath & path) { - EndpointClusters * endpointClusters = FindClusters(path.mEndpointId); - VerifyOrReturnValue(endpointClusters != nullptr, nullptr); - VerifyOrReturnValue(endpointClusters->firstCluster != nullptr, nullptr); - // Check the cache to speed things up - if ((mCachedClusterEndpointId == path.mEndpointId) && (mCachedInterface != nullptr) && - (mCachedInterface->GetClusterId() == path.mClusterId)) + if ((mCachedInterface != nullptr) && (mCachedInterface->GetPath() == path)) { return mCachedInterface; } // The cluster searched for is not cached, do a linear search for it - RegisteredServerClusterInterface * current = endpointClusters->firstCluster; + ServerClusterRegistration * current = mRegistrations; while (current != nullptr) { - if (current->serverClusterInterface->GetClusterId() == path.mClusterId) + if (current->serverClusterInterface->GetPath() == path) { - mCachedClusterEndpointId = path.mEndpointId; - mCachedInterface = current->serverClusterInterface; + mCachedInterface = current->serverClusterInterface; return mCachedInterface; } @@ -209,18 +158,5 @@ ServerClusterInterface * ServerClusterInterfaceRegistry::Get(const ConcreteClust return nullptr; } -ServerClusterInterfaceRegistry::EndpointClusters * ServerClusterInterfaceRegistry::FindClusters(EndpointId endpointId) -{ - for (EndpointClusters * p = mEndpoints; p != nullptr; p = p->next) - { - if (p->endpointId == endpointId) - { - return p; - } - } - - return nullptr; -} - } // namespace app } // namespace chip diff --git a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h index c0f837a61d2e4c..f7ad720421e92d 100644 --- a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h +++ b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h @@ -24,12 +24,25 @@ namespace chip { namespace app { +/// Represents an entry in the server cluster interface registry for +/// a specific interface. +/// +/// A single-linked list element +struct ServerClusterRegistration +{ + // A single-linked list of clusters registered for the given `endpointId` + ServerClusterInterface * serverClusterInterface = nullptr; + ServerClusterRegistration * next = nullptr; + + constexpr ServerClusterRegistration() = default; + constexpr ServerClusterRegistration(ServerClusterInterface * interface, ServerClusterRegistration * next_item = nullptr) : + serverClusterInterface(interface), next(next_item) + {} +}; + /// Allows registering and retrieving ServerClusterInterface instances for specific cluster paths. class ServerClusterInterfaceRegistry { -private: - struct RegisteredServerClusterInterface; - public: /// represents an iterable list of clusters class ClustersList @@ -38,42 +51,57 @@ class ServerClusterInterfaceRegistry class Iterator { public: - Iterator(RegisteredServerClusterInterface * interface) : mInterface(interface) {} + constexpr Iterator(ServerClusterRegistration * interface, EndpointId endpoint) : + mRegistration(interface), mEndpointId(endpoint) + { + AdvanceUntilMatchingEndpoint(); + } Iterator & operator++() { - if (mInterface != nullptr) + if (mRegistration != nullptr) { - mInterface = mInterface->next; + mRegistration = mRegistration->next; } + AdvanceUntilMatchingEndpoint(); return *this; } - bool operator==(const Iterator & other) const { return mInterface == other.mInterface; } - bool operator!=(const Iterator & other) const { return mInterface != other.mInterface; } - ServerClusterInterface * operator*() { return mInterface->serverClusterInterface; } + bool operator==(const Iterator & other) const { return mRegistration == other.mRegistration; } + bool operator!=(const Iterator & other) const { return mRegistration != other.mRegistration; } + ServerClusterInterface * operator*() { return mRegistration->serverClusterInterface; } private: - RegisteredServerClusterInterface * mInterface; + ServerClusterRegistration * mRegistration; + EndpointId mEndpointId; + + void AdvanceUntilMatchingEndpoint() + { + while ((mRegistration != nullptr) && (mRegistration->serverClusterInterface->GetPath().mEndpointId != mEndpointId)) + { + mRegistration = mRegistration->next; + } + } }; - ClustersList(RegisteredServerClusterInterface * start) : mStart(start) {} - Iterator begin() { return mStart; } - Iterator end() { return nullptr; } + constexpr ClustersList(ServerClusterRegistration * start, EndpointId endpointId) : mStart(start), mEndpointId(endpointId) {} + Iterator begin() { return { mStart, mEndpointId }; } + Iterator end() { return { nullptr, mEndpointId }; } private: - RegisteredServerClusterInterface * mStart; + ServerClusterRegistration * mStart; + EndpointId mEndpointId; }; ~ServerClusterInterfaceRegistry(); - /// Associate a specific interface with the given endpoint. + /// Add the given entry to the registry. /// - /// There can be only a single registration for a given `endpointId/clusterId` path. - /// A registration will return an error if a registration already exists on - /// the given `endpointId/cluster->GetClusterID()` already exists + /// Requirements: + /// - entry MUST NOT be part of any other registration + /// - LIFETIME of entry must outlive the Registry (or entry must be unregistered) /// - /// Registrations need a valid `endpointId` and `cluster->GetClusterID()` MUST be a valid cluster id. - [[nodiscard]] CHIP_ERROR Register(EndpointId endpointId, ServerClusterInterface * cluster); + /// There can be only a single registration for a given `endpointId/clusterId` path. + [[nodiscard]] CHIP_ERROR Register(ServerClusterRegistration & entry); /// Remove an existing registration for a given endpoint/cluster path. /// @@ -94,48 +122,10 @@ class ServerClusterInterfaceRegistry void UnregisterAllFromEndpoint(EndpointId endpointId); private: - /// A single-linked list of registered server cluster interfaces - struct RegisteredServerClusterInterface - { - // A single-linked list of clusters registered for the given `endpointId` - ServerClusterInterface * serverClusterInterface = nullptr; - RegisteredServerClusterInterface * next = nullptr; - - constexpr RegisteredServerClusterInterface(ServerClusterInterface * cluster, RegisteredServerClusterInterface * n) : - serverClusterInterface(cluster), next(n) - {} - }; - - /// tracks clusters registered to a particular endpoint - struct EndpointClusters - { - // The endpointId for this registration. kInvalidEndpointId means - // not allocated/used - EndpointId endpointId = kInvalidEndpointId; - - // A single-linked list of clusters registered for the given `endpointId` - RegisteredServerClusterInterface * firstCluster = nullptr; - - /// A single-linked list of endpoint clusters that is dynamically - /// allocated. - EndpointClusters * next; - }; - - /// returns nullptr if not found - EndpointClusters * FindClusters(EndpointId endpointId); - - /// Get a new usable endpoint cluster - CHIP_ERROR AllocateNewEndpointClusters(EndpointId endpointId, EndpointClusters *& dest); - - /// Clear and free memory for the given linked list - static void ClearSingleLinkedList(RegisteredServerClusterInterface * clusters); - - // Dynamic allocated endpoint cluters. - EndpointClusters * mEndpoints = nullptr; + ServerClusterRegistration * mRegistrations = nullptr; // A one-element cache to speed up finding a cluster within an endpoint. // The endpointId specifies which endpoint the cache belongs to. - ClusterId mCachedClusterEndpointId = kInvalidEndpointId; ServerClusterInterface * mCachedInterface = nullptr; }; diff --git a/src/data-model-providers/codegen/tests/TestServerClusterInterfaceRegistry.cpp b/src/data-model-providers/codegen/tests/TestServerClusterInterfaceRegistry.cpp index 6731cf3a757f1d..7d6c972db16bac 100644 --- a/src/data-model-providers/codegen/tests/TestServerClusterInterfaceRegistry.cpp +++ b/src/data-model-providers/codegen/tests/TestServerClusterInterfaceRegistry.cpp @@ -45,9 +45,10 @@ constexpr chip::ClusterId kCluster3 = 3; class FakeServerClusterInterface : public DefaultServerCluster { public: - FakeServerClusterInterface(ClusterId id) : mClusterId(id) {} + FakeServerClusterInterface(EndpointId endpoint, ClusterId cluster) : mPath({ endpoint, cluster }) {} + FakeServerClusterInterface(const ConcreteClusterPath & path) : mPath(path) {} - ClusterId GetClusterId() const override { return mClusterId; } + [[nodiscard]] ConcreteClusterPath GetPath() const override { return mPath; } DataModel::ActionReturnStatus ReadAttribute(const DataModel::ReadAttributeRequest & request, AttributeValueEncoder & encoder) override @@ -63,7 +64,7 @@ class FakeServerClusterInterface : public DefaultServerCluster } private: - ClusterId mClusterId; + ConcreteClusterPath mPath; }; struct TestServerClusterInterfaceRegistry : public ::testing::Test @@ -76,42 +77,55 @@ struct TestServerClusterInterfaceRegistry : public ::testing::Test TEST_F(TestServerClusterInterfaceRegistry, BasicTest) { - // NOTE: tests DO NOT use the global registry and validate implementation - // details. - ServerClusterInterfaceRegistry registry; - FakeServerClusterInterface cluster1(kCluster1); - FakeServerClusterInterface cluster2(kCluster2); - FakeServerClusterInterface cluster3(kCluster3); + FakeServerClusterInterface cluster1(kEp1, kCluster1); + FakeServerClusterInterface cluster2(kEp2, kCluster2); + FakeServerClusterInterface cluster3(kEp2, kCluster3); // there should be nothing registered to start with. - EXPECT_EQ(registry.Get(ConcreteClusterPath(kEp1, kCluster1)), nullptr); - EXPECT_EQ(registry.Get(ConcreteClusterPath(kEp1, kCluster2)), nullptr); - EXPECT_EQ(registry.Get(ConcreteClusterPath(kEp2, kCluster2)), nullptr); - EXPECT_EQ(registry.Get(ConcreteClusterPath(kEp2, kCluster3)), nullptr); - EXPECT_EQ(registry.Get(ConcreteClusterPath(kInvalidEndpointId, kCluster2)), nullptr); - EXPECT_EQ(registry.Get(ConcreteClusterPath(kEp1, kInvalidClusterId)), nullptr); + EXPECT_EQ(registry.Get({ kEp1, kCluster1 }), nullptr); + EXPECT_EQ(registry.Get({ kEp1, kCluster2 }), nullptr); + EXPECT_EQ(registry.Get({ kEp2, kCluster2 }), nullptr); + EXPECT_EQ(registry.Get({ kEp2, kCluster3 }), nullptr); + EXPECT_EQ(registry.Get({ kInvalidEndpointId, kCluster2 }), nullptr); + EXPECT_EQ(registry.Get({ kEp1, kInvalidClusterId }), nullptr); // registration of invalid values is not acceptable - EXPECT_EQ(registry.Register(kEp1, nullptr), CHIP_ERROR_INVALID_ARGUMENT); - EXPECT_EQ(registry.Register(kInvalidEndpointId, nullptr), CHIP_ERROR_INVALID_ARGUMENT); - EXPECT_EQ(registry.Register(kInvalidEndpointId, &cluster1), CHIP_ERROR_INVALID_ARGUMENT); - EXPECT_EQ(registry.Register(kInvalidEndpointId, &cluster2), CHIP_ERROR_INVALID_ARGUMENT); { - FakeServerClusterInterface badCluster(kInvalidClusterId); - EXPECT_EQ(registry.Register(kInvalidEndpointId, &badCluster), CHIP_ERROR_INVALID_ARGUMENT); + // registration has NULL interface + ServerClusterRegistration registration; + EXPECT_EQ(registry.Register(registration), CHIP_ERROR_INVALID_ARGUMENT); + + // next is not null (meaning registration2 looks like already registered) + ServerClusterRegistration registration2(&cluster1, ®istration); + EXPECT_EQ(registry.Register(registration2), CHIP_ERROR_INVALID_ARGUMENT); + + // invalid path in cluster + FakeServerClusterInterface invalidPathInterface(kInvalidEndpointId, kCluster1); + ServerClusterRegistration registration3(&invalidPathInterface); + EXPECT_EQ(registry.Register(registration3), CHIP_ERROR_INVALID_ARGUMENT); + + // invalid path in cluster + FakeServerClusterInterface invalidPathInterface2(kEp1, kInvalidClusterId); + ServerClusterRegistration registration4(&invalidPathInterface); + EXPECT_EQ(registry.Register(registration4), CHIP_ERROR_INVALID_ARGUMENT); } + ServerClusterRegistration registration1(&cluster1); + ServerClusterRegistration registration2(&cluster2); + ServerClusterRegistration registration3(&cluster3); + // should be able to register - EXPECT_EQ(registry.Register(kEp1, &cluster1), CHIP_NO_ERROR); - EXPECT_EQ(registry.Register(kEp2, &cluster2), CHIP_NO_ERROR); - EXPECT_EQ(registry.Register(kEp2, &cluster3), CHIP_NO_ERROR); + EXPECT_EQ(registry.Register(registration1), CHIP_NO_ERROR); + EXPECT_EQ(registry.Register(registration2), CHIP_NO_ERROR); + EXPECT_EQ(registry.Register(registration3), CHIP_NO_ERROR); // cannot register two implementations on the same path { - FakeServerClusterInterface another1(kCluster1); - EXPECT_EQ(registry.Register(kEp1, &another1), CHIP_ERROR_DUPLICATE_KEY_ID); + FakeServerClusterInterface another1(kEp1, kCluster1); + ServerClusterRegistration anotherRegisration1(&another1); + EXPECT_EQ(registry.Register(anotherRegisration1), CHIP_ERROR_DUPLICATE_KEY_ID); } // Items can be found back @@ -121,6 +135,7 @@ TEST_F(TestServerClusterInterfaceRegistry, BasicTest) EXPECT_EQ(registry.Get({ kEp2, kCluster1 }), nullptr); EXPECT_EQ(registry.Get({ kEp1, kCluster2 }), nullptr); + EXPECT_EQ(registry.Get({ kEp3, kCluster2 }), nullptr); // repeated calls work EXPECT_EQ(registry.Get({ kEp1, kCluster2 }), nullptr); @@ -134,28 +149,19 @@ TEST_F(TestServerClusterInterfaceRegistry, BasicTest) EXPECT_EQ(registry.Unregister({ kEp2, kCluster2 }), &cluster2); EXPECT_EQ(registry.Unregister({ kEp2, kCluster2 }), nullptr); - // Re-adding works exactly once. + // Re-adding works EXPECT_EQ(registry.Get({ kEp2, kCluster2 }), nullptr); - EXPECT_EQ(registry.Register(kEp3, &cluster2), CHIP_NO_ERROR); - EXPECT_EQ(registry.Get({ kEp3, kCluster2 }), &cluster2); - EXPECT_EQ(registry.Unregister({ kEp3, kCluster2 }), &cluster2); - EXPECT_EQ(registry.Get({ kEp2, kCluster2 }), nullptr); - EXPECT_EQ(registry.Unregister({ kEp3, kCluster2 }), nullptr); - EXPECT_EQ(registry.Get({ kEp2, kCluster2 }), nullptr); - - // cannot get it anymore once removed, others are still valid - EXPECT_EQ(registry.Get({ kEp2, kCluster2 }), nullptr); - EXPECT_EQ(registry.Get({ kEp2, kCluster3 }), &cluster3); + EXPECT_EQ(registry.Register(registration2), CHIP_NO_ERROR); + EXPECT_EQ(registry.Get({ kEp2, kCluster2 }), &cluster2); // clean of an entire endpoint works + EXPECT_EQ(registry.Get({ kEp2, kCluster3 }), &cluster3); registry.UnregisterAllFromEndpoint(kEp2); EXPECT_EQ(registry.Get({ kEp1, kCluster1 }), &cluster1); - EXPECT_EQ(registry.Get({ kEp2, kCluster2 }), nullptr); EXPECT_EQ(registry.Get({ kEp2, kCluster3 }), nullptr); registry.UnregisterAllFromEndpoint(kEp1); EXPECT_EQ(registry.Get({ kEp1, kCluster1 }), nullptr); - EXPECT_EQ(registry.Get({ kEp2, kCluster2 }), nullptr); EXPECT_EQ(registry.Get({ kEp2, kCluster3 }), nullptr); } @@ -165,6 +171,7 @@ TEST_F(TestServerClusterInterfaceRegistry, StressTest) srand(1234); std::vector items; + std::vector registrations; static constexpr ClusterId kClusterTestCount = 200; static constexpr EndpointId kEndpointTestCount = 10; @@ -176,21 +183,23 @@ TEST_F(TestServerClusterInterfaceRegistry, StressTest) items.reserve(kClusterTestCount); for (ClusterId i = 0; i < kClusterTestCount; i++) { - items.emplace_back(i); + auto endpointId = static_cast(rand() % kEndpointTestCount); + items.emplace_back(endpointId, i); + } + + registrations.reserve(kClusterTestCount); + for (ClusterId i = 0; i < kClusterTestCount; i++) + { + registrations.emplace_back(&items[i]); } ServerClusterInterfaceRegistry registry; for (size_t test = 0; test < kTestIterations; test++) { - std::vector endpoint_placement; - endpoint_placement.reserve(kClusterTestCount); - for (ClusterId i = 0; i < kClusterTestCount; i++) { - auto endpointId = static_cast(rand() % kEndpointTestCount); - endpoint_placement.push_back(endpointId); - ASSERT_EQ(registry.Register(endpointId, &items[i]), CHIP_NO_ERROR); + ASSERT_EQ(registry.Register(registrations[i]), CHIP_NO_ERROR); } // test that getters work @@ -198,7 +207,7 @@ TEST_F(TestServerClusterInterfaceRegistry, StressTest) { for (EndpointId ep = 0; ep < kEndpointTestCount; ep++) { - if (endpoint_placement[cluster] == ep) + if (items[cluster].GetPath().mEndpointId == ep) { ASSERT_EQ(registry.Get({ ep, cluster }), &items[cluster]); } @@ -227,12 +236,12 @@ TEST_F(TestServerClusterInterfaceRegistry, StressTest) for (auto cluster : unregister_order) { // item MUST exist and be accessible - ASSERT_EQ(registry.Get({ endpoint_placement[cluster], static_cast(cluster) }), &items[cluster]); - ASSERT_EQ(registry.Unregister({ endpoint_placement[cluster], static_cast(cluster) }), &items[cluster]); + ASSERT_EQ(registry.Get(items[cluster].GetPath()), &items[cluster]); + ASSERT_EQ(registry.Unregister(items[cluster].GetPath()), &items[cluster]); // once unregistered, it is not there anymore - ASSERT_EQ(registry.Get({ endpoint_placement[cluster], static_cast(cluster) }), nullptr); - ASSERT_EQ(registry.Unregister({ endpoint_placement[cluster], static_cast(cluster) }), nullptr); + ASSERT_EQ(registry.Get(items[cluster].GetPath()), nullptr); + ASSERT_EQ(registry.Unregister(items[cluster].GetPath()), nullptr); } } else @@ -258,6 +267,7 @@ TEST_F(TestServerClusterInterfaceRegistry, StressTest) TEST_F(TestServerClusterInterfaceRegistry, ClustersOnEndpoint) { std::vector items; + std::vector registrations; static constexpr ClusterId kClusterTestCount = 200; static constexpr EndpointId kEndpointTestCount = 10; @@ -267,7 +277,12 @@ TEST_F(TestServerClusterInterfaceRegistry, ClustersOnEndpoint) items.reserve(kClusterTestCount); for (ClusterId i = 0; i < kClusterTestCount; i++) { - items.emplace_back(i); + items.emplace_back(static_cast(i % kEndpointTestCount), i); + } + registrations.reserve(kClusterTestCount); + for (ClusterId i = 0; i < kClusterTestCount; i++) + { + registrations.emplace_back(&items[i]); } ServerClusterInterfaceRegistry registry; @@ -275,7 +290,7 @@ TEST_F(TestServerClusterInterfaceRegistry, ClustersOnEndpoint) // place the clusters on the respecitve endpoints for (ClusterId i = 0; i < kClusterTestCount; i++) { - ASSERT_EQ(registry.Register(static_cast(i % kEndpointTestCount), &items[i]), CHIP_NO_ERROR); + ASSERT_EQ(registry.Register(registrations[i]), CHIP_NO_ERROR); } // this IS implementation defined: we always register at "HEAD" so the listing is in @@ -293,7 +308,7 @@ TEST_F(TestServerClusterInterfaceRegistry, ClustersOnEndpoint) for (auto cluster : registry.ClustersOnEndpoint(ep)) { ASSERT_LT(expectedClusterId, kClusterTestCount); - ASSERT_EQ(cluster->GetClusterId(), expectedClusterId); + ASSERT_EQ(cluster->GetPath(), ConcreteClusterPath(ep, expectedClusterId)); expectedClusterId -= kEndpointTestCount; // next expected/registered cluster } From 7a2bb762d80982464c4c8fbf74d075b90f2b22ae Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 6 Mar 2025 10:20:29 -0500 Subject: [PATCH 11/24] Self review: fix an include --- src/app/server-cluster/tests/TestDefaultServerCluster.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/server-cluster/tests/TestDefaultServerCluster.cpp b/src/app/server-cluster/tests/TestDefaultServerCluster.cpp index d34ccff1794033..299408f7d0ae60 100644 --- a/src/app/server-cluster/tests/TestDefaultServerCluster.cpp +++ b/src/app/server-cluster/tests/TestDefaultServerCluster.cpp @@ -14,11 +14,11 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -#include "app/ConcreteClusterPath.h" #include #include #include +#include #include #include #include From 4e819fa13453d8dd1911da63e691fb31a7f2c83a Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 6 Mar 2025 10:28:29 -0500 Subject: [PATCH 12/24] Fix constexpr logic --- .../codegen/ServerClusterInterfaceRegistry.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h index f7ad720421e92d..efa719ce313d80 100644 --- a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h +++ b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h @@ -51,7 +51,7 @@ class ServerClusterInterfaceRegistry class Iterator { public: - constexpr Iterator(ServerClusterRegistration * interface, EndpointId endpoint) : + Iterator(ServerClusterRegistration * interface, EndpointId endpoint) : mRegistration(interface), mEndpointId(endpoint) { AdvanceUntilMatchingEndpoint(); From 7da6312c8d98cc78404ba9e19701b388a24cfc46 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 6 Mar 2025 10:29:07 -0500 Subject: [PATCH 13/24] Restyle --- .../codegen/ServerClusterInterfaceRegistry.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h index efa719ce313d80..34300eaae35401 100644 --- a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h +++ b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h @@ -51,8 +51,7 @@ class ServerClusterInterfaceRegistry class Iterator { public: - Iterator(ServerClusterRegistration * interface, EndpointId endpoint) : - mRegistration(interface), mEndpointId(endpoint) + Iterator(ServerClusterRegistration * interface, EndpointId endpoint) : mRegistration(interface), mEndpointId(endpoint) { AdvanceUntilMatchingEndpoint(); } From 0c1bc2672269284e16e61f8609923f7c7d70e615 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 6 Mar 2025 11:08:38 -0500 Subject: [PATCH 14/24] minor comment update so github is not confused about head SHA --- .../codegen/ServerClusterInterfaceRegistry.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h index 34300eaae35401..6e61bded061f0a 100644 --- a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h +++ b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h @@ -27,7 +27,7 @@ namespace app { /// Represents an entry in the server cluster interface registry for /// a specific interface. /// -/// A single-linked list element +/// In practice this is a single-linked list element. struct ServerClusterRegistration { // A single-linked list of clusters registered for the given `endpointId` From ea3e4d0922b12bf24637d28bad562c2caad79f3a Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 6 Mar 2025 11:16:40 -0500 Subject: [PATCH 15/24] Added useless unit test to get coverage --- .../server-cluster/tests/TestDefaultServerCluster.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/app/server-cluster/tests/TestDefaultServerCluster.cpp b/src/app/server-cluster/tests/TestDefaultServerCluster.cpp index 299408f7d0ae60..e61d45b8e10d50 100644 --- a/src/app/server-cluster/tests/TestDefaultServerCluster.cpp +++ b/src/app/server-cluster/tests/TestDefaultServerCluster.cpp @@ -85,6 +85,16 @@ TEST(TestDefaultServerCluster, TestFlagsDefault) ASSERT_EQ(cluster.GetClusterFlags().Raw(), 0u); } +TEST(TestDefaultServerCluster, ListWriteNotification) +{ + FakeDefaultServerCluster cluster({ 1, 2 }); + + // this does not test anything really, except we get 100% coverage and we see that we do not crash + cluster.ListAttributeWriteNotification({1,2,3}, DataModel::ListWriteOperation::kListWriteBegin); + cluster.ListAttributeWriteNotification({1,2,3}, DataModel::ListWriteOperation::kListWriteFailure); +} + + TEST(TestDefaultServerCluster, AttributesDefault) { FakeDefaultServerCluster cluster({ 1, 2 }); From 86aeb052ce7ff67a2b4bf131b0e6dbd69d25f489 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 6 Mar 2025 11:17:25 -0500 Subject: [PATCH 16/24] Restyle --- src/app/server-cluster/tests/TestDefaultServerCluster.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/app/server-cluster/tests/TestDefaultServerCluster.cpp b/src/app/server-cluster/tests/TestDefaultServerCluster.cpp index e61d45b8e10d50..8e880ae86c301a 100644 --- a/src/app/server-cluster/tests/TestDefaultServerCluster.cpp +++ b/src/app/server-cluster/tests/TestDefaultServerCluster.cpp @@ -90,11 +90,10 @@ TEST(TestDefaultServerCluster, ListWriteNotification) FakeDefaultServerCluster cluster({ 1, 2 }); // this does not test anything really, except we get 100% coverage and we see that we do not crash - cluster.ListAttributeWriteNotification({1,2,3}, DataModel::ListWriteOperation::kListWriteBegin); - cluster.ListAttributeWriteNotification({1,2,3}, DataModel::ListWriteOperation::kListWriteFailure); + cluster.ListAttributeWriteNotification({ 1, 2, 3 }, DataModel::ListWriteOperation::kListWriteBegin); + cluster.ListAttributeWriteNotification({ 1, 2, 3 }, DataModel::ListWriteOperation::kListWriteFailure); } - TEST(TestDefaultServerCluster, AttributesDefault) { FakeDefaultServerCluster cluster({ 1, 2 }); From 4fec4f1259ad1ed30a9e8cc37a81ea9d48034d06 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 6 Mar 2025 11:19:29 -0500 Subject: [PATCH 17/24] Update src/app/server-cluster/ServerClusterInterface.h Co-authored-by: Boris Zbarsky --- src/app/server-cluster/ServerClusterInterface.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/server-cluster/ServerClusterInterface.h b/src/app/server-cluster/ServerClusterInterface.h index 53460f763547f6..b1e9b311ba47a8 100644 --- a/src/app/server-cluster/ServerClusterInterface.h +++ b/src/app/server-cluster/ServerClusterInterface.h @@ -44,7 +44,7 @@ class ServerClusterInterface ///////////////////////////////////// Cluster Metadata Support ////////////////////////////////////////////////// - /// The path where this cluster operates on. + /// The path to this cluster instance. /// /// This path (endpointid,clusterid) is expected to be fixed once the server /// cluster interface is in use. From 868d8a9bbb2b206aa707c15a63ed71063faddee1 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 6 Mar 2025 11:19:40 -0500 Subject: [PATCH 18/24] Update src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp Co-authored-by: Boris Zbarsky --- .../codegen/ServerClusterInterfaceRegistry.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp index 65aad7a53c986a..b59a95e24661b1 100644 --- a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp +++ b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp @@ -83,7 +83,7 @@ ServerClusterInterface * ServerClusterInterfaceRegistry::Unregister(const Concre mCachedInterface = nullptr; } - current->next = nullptr; // some clearing + current->next = nullptr; // Make sure current does not look like part of a list. return current->serverClusterInterface; } From 30f5679de8a5ce8cb19354eee031f7be7320961e Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 6 Mar 2025 11:19:50 -0500 Subject: [PATCH 19/24] Update src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp Co-authored-by: Boris Zbarsky --- .../codegen/ServerClusterInterfaceRegistry.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp index b59a95e24661b1..feb4ec716742b0 100644 --- a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp +++ b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp @@ -121,7 +121,7 @@ void ServerClusterInterfaceRegistry::UnregisterAllFromEndpoint(EndpointId endpoi prev->next = current->next; } ServerClusterRegistration * actual_next = current->next; - current->next = nullptr; // some clearing + current->next = nullptr; // Make sure current does not look like part of a list. current = actual_next; } else From c88e835382bc2374fee22bcbc53ff8db5334295b Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 6 Mar 2025 11:20:03 -0500 Subject: [PATCH 20/24] Update src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h Co-authored-by: Boris Zbarsky --- .../codegen/ServerClusterInterfaceRegistry.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h index 6e61bded061f0a..490168a3402dfe 100644 --- a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h +++ b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h @@ -31,7 +31,7 @@ namespace app { struct ServerClusterRegistration { // A single-linked list of clusters registered for the given `endpointId` - ServerClusterInterface * serverClusterInterface = nullptr; + ServerClusterInterface * const serverClusterInterface = nullptr; ServerClusterRegistration * next = nullptr; constexpr ServerClusterRegistration() = default; From 4ea82ec7322838cf13390cab7d96d371bf90d400 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 6 Mar 2025 11:20:13 -0500 Subject: [PATCH 21/24] Update src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h Co-authored-by: Boris Zbarsky --- .../codegen/ServerClusterInterfaceRegistry.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h index 490168a3402dfe..acb0aeee9735e1 100644 --- a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h +++ b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h @@ -113,7 +113,7 @@ class ServerClusterInterfaceRegistry /// Provides a list of clusters that are registered for the given endpoint. /// - /// As ClustersList points inside the internal registrations of the registry, + /// ClustersList points inside the internal registrations of the registry, so /// the list is only valid as long as the registry is not modified. ClustersList ClustersOnEndpoint(EndpointId endpointId); From af23a93304661f5216bdb442396d30a4fb5c716f Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 6 Mar 2025 11:32:16 -0500 Subject: [PATCH 22/24] Disallow null ptr registrations --- .../codegen/ServerClusterInterfaceRegistry.h | 15 +++++++---- .../TestServerClusterInterfaceRegistry.cpp | 25 ++++++++----------- 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h index 6e61bded061f0a..26ee722c93c342 100644 --- a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h +++ b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h @@ -31,13 +31,18 @@ namespace app { struct ServerClusterRegistration { // A single-linked list of clusters registered for the given `endpointId` - ServerClusterInterface * serverClusterInterface = nullptr; - ServerClusterRegistration * next = nullptr; + ServerClusterInterface * serverClusterInterface; + ServerClusterRegistration * next; - constexpr ServerClusterRegistration() = default; - constexpr ServerClusterRegistration(ServerClusterInterface * interface, ServerClusterRegistration * next_item = nullptr) : - serverClusterInterface(interface), next(next_item) + constexpr ServerClusterRegistration(ServerClusterInterface &interface, ServerClusterRegistration * next_item = nullptr) : + serverClusterInterface(&interface), next(next_item) {} + ServerClusterRegistration(ServerClusterRegistration &&other) = default; + ServerClusterRegistration& operator=(ServerClusterRegistration &&other) = default; + + // we generally do not want to allow copies as those may have different "next" entries. + ServerClusterRegistration(const ServerClusterRegistration &other) = delete; + ServerClusterRegistration& operator=(const ServerClusterRegistration &other) = delete; }; /// Allows registering and retrieving ServerClusterInterface instances for specific cluster paths. diff --git a/src/data-model-providers/codegen/tests/TestServerClusterInterfaceRegistry.cpp b/src/data-model-providers/codegen/tests/TestServerClusterInterfaceRegistry.cpp index 7d6c972db16bac..5824568c8eab01 100644 --- a/src/data-model-providers/codegen/tests/TestServerClusterInterfaceRegistry.cpp +++ b/src/data-model-providers/codegen/tests/TestServerClusterInterfaceRegistry.cpp @@ -94,27 +94,26 @@ TEST_F(TestServerClusterInterfaceRegistry, BasicTest) // registration of invalid values is not acceptable { // registration has NULL interface - ServerClusterRegistration registration; - EXPECT_EQ(registry.Register(registration), CHIP_ERROR_INVALID_ARGUMENT); - // next is not null (meaning registration2 looks like already registered) - ServerClusterRegistration registration2(&cluster1, ®istration); + ServerClusterRegistration registration1(cluster1); + ServerClusterRegistration registration2(cluster2); + registration2.next = ®istration1; EXPECT_EQ(registry.Register(registration2), CHIP_ERROR_INVALID_ARGUMENT); // invalid path in cluster FakeServerClusterInterface invalidPathInterface(kInvalidEndpointId, kCluster1); - ServerClusterRegistration registration3(&invalidPathInterface); + ServerClusterRegistration registration3(invalidPathInterface); EXPECT_EQ(registry.Register(registration3), CHIP_ERROR_INVALID_ARGUMENT); // invalid path in cluster FakeServerClusterInterface invalidPathInterface2(kEp1, kInvalidClusterId); - ServerClusterRegistration registration4(&invalidPathInterface); + ServerClusterRegistration registration4(invalidPathInterface); EXPECT_EQ(registry.Register(registration4), CHIP_ERROR_INVALID_ARGUMENT); } - ServerClusterRegistration registration1(&cluster1); - ServerClusterRegistration registration2(&cluster2); - ServerClusterRegistration registration3(&cluster3); + ServerClusterRegistration registration1(cluster1); + ServerClusterRegistration registration2(cluster2); + ServerClusterRegistration registration3(cluster3); // should be able to register EXPECT_EQ(registry.Register(registration1), CHIP_NO_ERROR); @@ -124,7 +123,7 @@ TEST_F(TestServerClusterInterfaceRegistry, BasicTest) // cannot register two implementations on the same path { FakeServerClusterInterface another1(kEp1, kCluster1); - ServerClusterRegistration anotherRegisration1(&another1); + ServerClusterRegistration anotherRegisration1(another1); EXPECT_EQ(registry.Register(anotherRegisration1), CHIP_ERROR_DUPLICATE_KEY_ID); } @@ -187,10 +186,9 @@ TEST_F(TestServerClusterInterfaceRegistry, StressTest) items.emplace_back(endpointId, i); } - registrations.reserve(kClusterTestCount); for (ClusterId i = 0; i < kClusterTestCount; i++) { - registrations.emplace_back(&items[i]); + registrations.emplace_back(items[i]); } ServerClusterInterfaceRegistry registry; @@ -279,10 +277,9 @@ TEST_F(TestServerClusterInterfaceRegistry, ClustersOnEndpoint) { items.emplace_back(static_cast(i % kEndpointTestCount), i); } - registrations.reserve(kClusterTestCount); for (ClusterId i = 0; i < kClusterTestCount; i++) { - registrations.emplace_back(&items[i]); + registrations.emplace_back(items[i]); } ServerClusterInterfaceRegistry registry; From 4463e95cc15d8778ba5ea6b9a8c56aa17004a817 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Thu, 6 Mar 2025 16:33:54 +0000 Subject: [PATCH 23/24] Restyled by clang-format --- .../codegen/ServerClusterInterfaceRegistry.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h index 034a0a1b292b02..aa8db91628b5a4 100644 --- a/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h +++ b/src/data-model-providers/codegen/ServerClusterInterfaceRegistry.h @@ -34,14 +34,14 @@ struct ServerClusterRegistration ServerClusterInterface * const serverClusterInterface; ServerClusterRegistration * next; - constexpr ServerClusterRegistration(ServerClusterInterface &interface, ServerClusterRegistration * next_item = nullptr) : + constexpr ServerClusterRegistration(ServerClusterInterface & interface, ServerClusterRegistration * next_item = nullptr) : serverClusterInterface(&interface), next(next_item) {} - ServerClusterRegistration(ServerClusterRegistration &&other) = default; + ServerClusterRegistration(ServerClusterRegistration && other) = default; // we generally do not want to allow copies as those may have different "next" entries. - ServerClusterRegistration(const ServerClusterRegistration &other) = delete; - ServerClusterRegistration& operator=(const ServerClusterRegistration &other) = delete; + ServerClusterRegistration(const ServerClusterRegistration & other) = delete; + ServerClusterRegistration & operator=(const ServerClusterRegistration & other) = delete; }; /// Allows registering and retrieving ServerClusterInterface instances for specific cluster paths. From 2fa212502ac11634418bbb7dc932f2dbcffc9e78 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 6 Mar 2025 12:56:27 -0500 Subject: [PATCH 24/24] Reworded based on code review --- src/app/server-cluster/ServerClusterInterface.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/server-cluster/ServerClusterInterface.h b/src/app/server-cluster/ServerClusterInterface.h index b1e9b311ba47a8..e8f227576f5a00 100644 --- a/src/app/server-cluster/ServerClusterInterface.h +++ b/src/app/server-cluster/ServerClusterInterface.h @@ -46,7 +46,7 @@ class ServerClusterInterface /// The path to this cluster instance. /// - /// This path (endpointid,clusterid) is expected to be fixed once the server + /// This path (endpointid,clusterid) is expected to remain constant once the server /// cluster interface is in use. [[nodiscard]] virtual ConcreteClusterPath GetPath() const = 0;