From cc6c773f62431936fe305848c2a1fd121d24551f Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 3 Oct 2024 13:28:57 -0400 Subject: [PATCH] Add ability to report endpoint device types in the DataModel::Provider interface (#35861) * Add separate listing for device types * Make unit tests work * Restyle * Fixup include * Fixup include * Add clang-tidy comments * Added a comment about future todo * more comments * Attempt to make the code more readable * Fix cast * Add comment for newly created issue. --------- Co-authored-by: Andrei Litvin --- .../CodegenDataModelProvider.cpp | 103 ++++++++++++++++++ .../CodegenDataModelProvider.h | 11 +- .../tests/TestCodegenModelViaMocks.cpp | 61 +++++++++++ src/app/data-model-provider/MetadataTypes.h | 17 +++ src/app/tests/test-interaction-model-api.cpp | 11 ++ src/app/tests/test-interaction-model-api.h | 3 + src/app/util/attribute-storage.cpp | 8 +- src/app/util/attribute-storage.h | 1 + src/app/util/mock/MockNodeConfig.cpp | 11 +- src/app/util/mock/MockNodeConfig.h | 8 +- src/app/util/mock/attribute-storage.cpp | 22 ++++ .../tests/data_model/DataModelFixtures.cpp | 11 ++ .../tests/data_model/DataModelFixtures.h | 3 + 13 files changed, 260 insertions(+), 10 deletions(-) diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider.cpp b/src/app/codegen-data-model-provider/CodegenDataModelProvider.cpp index afba240b419258..972e0565131dd6 100644 --- a/src/app/codegen-data-model-provider/CodegenDataModelProvider.cpp +++ b/src/app/codegen-data-model-provider/CodegenDataModelProvider.cpp @@ -292,6 +292,57 @@ std::optional EnumeratorCommandFinder::FindCommandEntry return (*id == kInvalidCommandId) ? DataModel::CommandEntry::kInvalid : CommandEntryFrom(path, *id); } +// TODO: DeviceTypeEntry content is IDENTICAL to EmberAfDeviceType, so centralizing +// to a common type is probably better. Need to figure out dependencies since +// this would make ember return datamodel-provider types. +// See: https://github.com/project-chip/connectedhomeip/issues/35889 +DataModel::DeviceTypeEntry DeviceTypeEntryFromEmber(const EmberAfDeviceType & other) +{ + DataModel::DeviceTypeEntry entry; + + entry.deviceTypeId = other.deviceId; + entry.deviceTypeVersion = other.deviceVersion; + + return entry; +} + +// Explicitly compare for identical entries. note that types are different, +// so you must do `a == b` and the `b == a` will not work. +bool operator==(const DataModel::DeviceTypeEntry & a, const EmberAfDeviceType & b) +{ + return (a.deviceTypeId == b.deviceId) && (a.deviceTypeVersion == b.deviceVersion); +} + +/// Find the `index` where one of the following holds: +/// - types[index - 1] == previous OR +/// - index == types.size() // i.e. not found or there is no next +/// +/// hintWherePreviousMayBe represents a search hint where previous may exist. +unsigned FindNextDeviceTypeIndex(Span types, const DataModel::DeviceTypeEntry previous, + unsigned hintWherePreviousMayBe) +{ + if (hintWherePreviousMayBe < types.size()) + { + // this is a valid hint ... see if we are lucky + if (previous == types[hintWherePreviousMayBe]) + { + return hintWherePreviousMayBe + 1; // return the next index + } + } + + // hint was not useful. We have to do a full search + for (unsigned idx = 0; idx < types.size(); idx++) + { + if (previous == types[idx]) + { + return idx + 1; + } + } + + // cast should be safe as we know we do not have that many types + return static_cast(types.size()); +} + const ConcreteCommandPath kInvalidCommandPath(kInvalidEndpointId, kInvalidClusterId, kInvalidCommandId); } // namespace @@ -731,6 +782,58 @@ ConcreteCommandPath CodegenDataModelProvider::NextGeneratedCommand(const Concret return ConcreteCommandPath(before.mEndpointId, before.mClusterId, *commandId); } +std::optional CodegenDataModelProvider::FirstDeviceType(EndpointId endpoint) +{ + // Use the `Index` version even though `emberAfDeviceTypeListFromEndpoint` would work because + // index finding is cached in TryFindEndpointIndex and this avoids an extra `emberAfIndexFromEndpoint` + // during `Next` loops. This avoids O(n^2) on number of indexes when iterating over all device types. + // + // Not actually needed for `First`, however this makes First and Next consistent. + std::optional endpoint_index = TryFindEndpointIndex(endpoint); + if (!endpoint_index.has_value()) + { + return std::nullopt; + } + + CHIP_ERROR err = CHIP_NO_ERROR; + Span deviceTypes = emberAfDeviceTypeListFromEndpointIndex(*endpoint_index, err); + + if (deviceTypes.empty()) + { + return std::nullopt; + } + + // we start at the beginning + mDeviceTypeIterationHint = 0; + return DeviceTypeEntryFromEmber(deviceTypes[0]); +} + +std::optional CodegenDataModelProvider::NextDeviceType(EndpointId endpoint, + const DataModel::DeviceTypeEntry & previous) +{ + // Use the `Index` version even though `emberAfDeviceTypeListFromEndpoint` would work because + // index finding is cached in TryFindEndpointIndex and this avoids an extra `emberAfIndexFromEndpoint` + // during `Next` loops. This avoids O(n^2) on number of indexes when iterating over all device types. + std::optional endpoint_index = TryFindEndpointIndex(endpoint); + if (!endpoint_index.has_value()) + { + return std::nullopt; + } + + CHIP_ERROR err = CHIP_NO_ERROR; + Span deviceTypes = emberAfDeviceTypeListFromEndpointIndex(*endpoint_index, err); + + unsigned idx = FindNextDeviceTypeIndex(deviceTypes, previous, mDeviceTypeIterationHint); + + if (idx >= deviceTypes.size()) + { + return std::nullopt; + } + + mDeviceTypeIterationHint = idx; + return DeviceTypeEntryFromEmber(deviceTypes[idx]); +} + bool CodegenDataModelProvider::EventPathIncludesAccessibleConcretePath(const EventPathParams & path, const Access::SubjectDescriptor & descriptor) { diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider.h b/src/app/codegen-data-model-provider/CodegenDataModelProvider.h index 500b491a39c8e7..f6d6e4e4ed4be3 100644 --- a/src/app/codegen-data-model-provider/CodegenDataModelProvider.h +++ b/src/app/codegen-data-model-provider/CodegenDataModelProvider.h @@ -98,6 +98,10 @@ class CodegenDataModelProvider : public chip::app::DataModel::Provider EndpointId NextEndpoint(EndpointId before) override; bool EndpointExists(EndpointId endpoint) override; + std::optional FirstDeviceType(EndpointId endpoint) override; + std::optional NextDeviceType(EndpointId endpoint, + const DataModel::DeviceTypeEntry & previous) override; + DataModel::ClusterEntry FirstCluster(EndpointId endpoint) override; DataModel::ClusterEntry NextCluster(const ConcreteClusterPath & before) override; std::optional GetClusterInfo(const ConcreteClusterPath & path) override; @@ -116,9 +120,10 @@ class CodegenDataModelProvider : public chip::app::DataModel::Provider private: // Iteration is often done in a tight loop going through all values. // To avoid N^2 iterations, cache a hint of where something is positioned - uint16_t mEndpointIterationHint = 0; - unsigned mClusterIterationHint = 0; - unsigned mAttributeIterationHint = 0; + uint16_t mEndpointIterationHint = 0; + unsigned mClusterIterationHint = 0; + unsigned mAttributeIterationHint = 0; + unsigned mDeviceTypeIterationHint = 0; EmberCommandListIterator mAcceptedCommandsIterator; EmberCommandListIterator mGeneratedCommandsIterator; diff --git a/src/app/codegen-data-model-provider/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model-provider/tests/TestCodegenModelViaMocks.cpp index 5314684fc6f9a4..3840de9c8151dc 100644 --- a/src/app/codegen-data-model-provider/tests/TestCodegenModelViaMocks.cpp +++ b/src/app/codegen-data-model-provider/tests/TestCodegenModelViaMocks.cpp @@ -63,6 +63,7 @@ #include #include +#include #include using namespace chip; @@ -86,6 +87,15 @@ constexpr EndpointId kEndpointIdThatIsMissing = kMockEndpointMin - 1; constexpr AttributeId kReadOnlyAttributeId = 0x5001; +constexpr DeviceTypeId kDeviceTypeId1 = 123; +constexpr uint8_t kDeviceTypeId1Version = 10; + +constexpr DeviceTypeId kDeviceTypeId2 = 1122; +constexpr uint8_t kDeviceTypeId2Version = 11; + +constexpr DeviceTypeId kDeviceTypeId3 = 3; +constexpr uint8_t kDeviceTypeId3Version = 33; + static_assert(kEndpointIdThatIsMissing != kInvalidEndpointId); static_assert(kEndpointIdThatIsMissing != kMockEndpoint1); static_assert(kEndpointIdThatIsMissing != kMockEndpoint2); @@ -270,6 +280,10 @@ const MockNodeConfig gTestNodeConfig({ MockClusterConfig(MockClusterId(2), { ClusterRevision::Id, FeatureMap::Id, MockAttributeId(1), }), + }, { + { kDeviceTypeId1, kDeviceTypeId1Version}, + { kDeviceTypeId2, kDeviceTypeId2Version}, + { kDeviceTypeId3, kDeviceTypeId3Version}, }), MockEndpointConfig(kMockEndpoint2, { MockClusterConfig(MockClusterId(1), { @@ -296,6 +310,8 @@ const MockNodeConfig gTestNodeConfig({ {11}, /* acceptedCommands */ {4, 6} /* generatedCommands */ ), + }, { + { kDeviceTypeId2, kDeviceTypeId2Version}, }), MockEndpointConfig(kMockEndpoint3, { MockClusterConfig(MockClusterId(1), { @@ -2580,3 +2596,48 @@ TEST(TestCodegenModelViaMocks, EmberWriteInvalidDataType) ASSERT_EQ(model.WriteAttribute(test.GetRequest(), decoder), Status::Failure); ASSERT_TRUE(model.ChangeListener().DirtyList().empty()); } + +TEST(TestCodegenModelViaMocks, DeviceTypeIteration) +{ + UseMockNodeConfig config(gTestNodeConfig); + CodegenDataModelProviderWithContext model; + + // Mock endpoint 1 has 3 device types + std::optional entry = model.FirstDeviceType(kMockEndpoint1); + ASSERT_EQ(entry, + std::make_optional(DeviceTypeEntry{ .deviceTypeId = kDeviceTypeId1, .deviceTypeVersion = kDeviceTypeId1Version })); + // NOLINTNEXTLINE(bugprone-unchecked-optional-access): Assert above that this is not none + entry = model.NextDeviceType(kMockEndpoint1, *entry); + ASSERT_EQ(entry, + std::make_optional(DeviceTypeEntry{ .deviceTypeId = kDeviceTypeId2, .deviceTypeVersion = kDeviceTypeId2Version })); + // NOLINTNEXTLINE(bugprone-unchecked-optional-access): Assert above that this is not none + entry = model.NextDeviceType(kMockEndpoint1, *entry); + ASSERT_EQ(entry, + std::make_optional(DeviceTypeEntry{ .deviceTypeId = kDeviceTypeId3, .deviceTypeVersion = kDeviceTypeId3Version })); + // NOLINTNEXTLINE(bugprone-unchecked-optional-access): Assert above that this is not none + entry = model.NextDeviceType(kMockEndpoint1, *entry); + ASSERT_FALSE(entry.has_value()); + + // Mock endpoint 2 has 1 device types + entry = model.FirstDeviceType(kMockEndpoint2); + ASSERT_EQ(entry, + std::make_optional(DeviceTypeEntry{ .deviceTypeId = kDeviceTypeId2, .deviceTypeVersion = kDeviceTypeId2Version })); + // NOLINTNEXTLINE(bugprone-unchecked-optional-access): Assert above that this is not none + entry = model.NextDeviceType(kMockEndpoint2, *entry); + ASSERT_FALSE(entry.has_value()); + + // out of order query works + entry = std::make_optional(DeviceTypeEntry{ .deviceTypeId = kDeviceTypeId2, .deviceTypeVersion = kDeviceTypeId2Version }); + entry = model.NextDeviceType(kMockEndpoint1, *entry); + ASSERT_EQ(entry, + std::make_optional(DeviceTypeEntry{ .deviceTypeId = kDeviceTypeId3, .deviceTypeVersion = kDeviceTypeId3Version })); + + // invalid query fails + entry = std::make_optional(DeviceTypeEntry{ .deviceTypeId = kDeviceTypeId1, .deviceTypeVersion = kDeviceTypeId1Version }); + entry = model.NextDeviceType(kMockEndpoint2, *entry); + ASSERT_FALSE(entry.has_value()); + + // empty endpoint works + entry = model.FirstDeviceType(kMockEndpoint3); + ASSERT_FALSE(entry.has_value()); +} diff --git a/src/app/data-model-provider/MetadataTypes.h b/src/app/data-model-provider/MetadataTypes.h index 65c89da1c2d42f..a3541faeabbf25 100644 --- a/src/app/data-model-provider/MetadataTypes.h +++ b/src/app/data-model-provider/MetadataTypes.h @@ -105,6 +105,18 @@ struct CommandEntry static const CommandEntry kInvalid; }; +/// Represents a device type that resides on an endpoint +struct DeviceTypeEntry +{ + DeviceTypeId deviceTypeId; + uint8_t deviceTypeVersion; + + bool operator==(const DeviceTypeEntry & other) const + { + return (deviceTypeId == other.deviceTypeId) && (deviceTypeVersion == other.deviceTypeVersion); + } +}; + /// Provides metadata information for a data model /// /// The data model can be viewed as a tree of endpoint/cluster/(attribute+commands+events) @@ -130,6 +142,11 @@ class ProviderMetadataTree virtual EndpointId NextEndpoint(EndpointId before) = 0; virtual bool EndpointExists(EndpointId id); + // This iteration describes device types registered on an endpoint + virtual std::optional FirstDeviceType(EndpointId endpoint) = 0; + virtual std::optional NextDeviceType(EndpointId endpoint, const DeviceTypeEntry & previous) = 0; + + // This iteration will list all clusters on a given endpoint virtual ClusterEntry FirstCluster(EndpointId endpoint) = 0; virtual ClusterEntry NextCluster(const ConcreteClusterPath & before) = 0; virtual std::optional GetClusterInfo(const ConcreteClusterPath & path) = 0; diff --git a/src/app/tests/test-interaction-model-api.cpp b/src/app/tests/test-interaction-model-api.cpp index 5750e098b995f3..b69c4234273cb9 100644 --- a/src/app/tests/test-interaction-model-api.cpp +++ b/src/app/tests/test-interaction-model-api.cpp @@ -216,6 +216,17 @@ EndpointId TestImCustomDataModel::NextEndpoint(EndpointId before) return CodegenDataModelProviderInstance()->NextEndpoint(before); } +std::optional TestImCustomDataModel::FirstDeviceType(EndpointId endpoint) +{ + return std::nullopt; +} + +std::optional TestImCustomDataModel::NextDeviceType(EndpointId endpoint, + const DataModel::DeviceTypeEntry & previous) +{ + return std::nullopt; +} + ClusterEntry TestImCustomDataModel::FirstCluster(EndpointId endpoint) { return CodegenDataModelProviderInstance()->FirstCluster(endpoint); diff --git a/src/app/tests/test-interaction-model-api.h b/src/app/tests/test-interaction-model-api.h index 8523e4b88c8157..ae5cc63df7293a 100644 --- a/src/app/tests/test-interaction-model-api.h +++ b/src/app/tests/test-interaction-model-api.h @@ -131,6 +131,9 @@ class TestImCustomDataModel : public DataModel::Provider EndpointId FirstEndpoint() override; EndpointId NextEndpoint(EndpointId before) override; + std::optional FirstDeviceType(EndpointId endpoint) override; + std::optional NextDeviceType(EndpointId endpoint, + const DataModel::DeviceTypeEntry & previous) override; DataModel::ClusterEntry FirstCluster(EndpointId endpoint) override; DataModel::ClusterEntry NextCluster(const ConcreteClusterPath & before) override; std::optional GetClusterInfo(const ConcreteClusterPath & path) override; diff --git a/src/app/util/attribute-storage.cpp b/src/app/util/attribute-storage.cpp index 7247031b071042..0d1b3827e8b365 100644 --- a/src/app/util/attribute-storage.cpp +++ b/src/app/util/attribute-storage.cpp @@ -1007,13 +1007,15 @@ uint8_t emberAfGetClusterCountForEndpoint(EndpointId endpoint) Span emberAfDeviceTypeListFromEndpoint(EndpointId endpoint, CHIP_ERROR & err) { - uint16_t endpointIndex = emberAfIndexFromEndpoint(endpoint); - Span ret; + return emberAfDeviceTypeListFromEndpointIndex(emberAfIndexFromEndpoint(endpoint), err); +} +chip::Span emberAfDeviceTypeListFromEndpointIndex(unsigned endpointIndex, CHIP_ERROR & err) +{ if (endpointIndex == 0xFFFF) { err = CHIP_ERROR_INVALID_ARGUMENT; - return ret; + return Span(); } err = CHIP_NO_ERROR; diff --git a/src/app/util/attribute-storage.h b/src/app/util/attribute-storage.h index 23a9b9b5fd20c3..9d2dcc60bb3f05 100644 --- a/src/app/util/attribute-storage.h +++ b/src/app/util/attribute-storage.h @@ -281,6 +281,7 @@ const EmberAfCluster * emberAfGetNthCluster(chip::EndpointId endpoint, uint8_t n // Retrieve the device type list associated with a specific endpoint. // chip::Span emberAfDeviceTypeListFromEndpoint(chip::EndpointId endpoint, CHIP_ERROR & err); +chip::Span emberAfDeviceTypeListFromEndpointIndex(unsigned endpointIndex, CHIP_ERROR & err); // // Override the device type list current associated with an endpoint with a user-provided list. The buffers backing diff --git a/src/app/util/mock/MockNodeConfig.cpp b/src/app/util/mock/MockNodeConfig.cpp index b48c846e65072d..c6572ed0aef075 100644 --- a/src/app/util/mock/MockNodeConfig.cpp +++ b/src/app/util/mock/MockNodeConfig.cpp @@ -16,10 +16,12 @@ * limitations under the License. */ +#include "app/util/af-types.h" #include #include #include +#include #include #include @@ -174,8 +176,10 @@ const MockAttributeConfig * MockClusterConfig::attributeById(AttributeId attribu return findById(attributes, attributeId, outIndex); } -MockEndpointConfig::MockEndpointConfig(EndpointId aId, std::initializer_list aClusters) : - id(aId), clusters(aClusters), mEmberEndpoint{} +MockEndpointConfig::MockEndpointConfig(EndpointId aId, std::initializer_list aClusters, + std::initializer_list aDeviceTypes) : + id(aId), + clusters(aClusters), mDeviceTypes(aDeviceTypes), mEmberEndpoint{} { VerifyOrDie(aClusters.size() < UINT8_MAX); @@ -189,7 +193,8 @@ MockEndpointConfig::MockEndpointConfig(EndpointId aId, std::initializer_list aClusters = {}); + MockEndpointConfig(EndpointId aId, std::initializer_list aClusters = {}, + std::initializer_list aDeviceTypes = {}); // Endpoint-config is self-referential: mEmberEndpoint.clusters references mEmberClusters.data() MockEndpointConfig(const MockEndpointConfig & other); @@ -109,12 +110,17 @@ struct MockEndpointConfig const MockClusterConfig * clusterById(ClusterId clusterId, ptrdiff_t * outIndex = nullptr) const; const EmberAfEndpointType * emberEndpoint() const { return &mEmberEndpoint; } + Span deviceTypes() const + { + return Span(mDeviceTypes.data(), mDeviceTypes.size()); + } const EndpointId id; const std::vector clusters; private: std::vector mEmberClusters; + std::vector mDeviceTypes; EmberAfEndpointType mEmberEndpoint; }; diff --git a/src/app/util/mock/attribute-storage.cpp b/src/app/util/mock/attribute-storage.cpp index 4447640dc1960c..256d0a3a880886 100644 --- a/src/app/util/mock/attribute-storage.cpp +++ b/src/app/util/mock/attribute-storage.cpp @@ -313,6 +313,28 @@ DataVersion * emberAfDataVersionStorage(const chip::app::ConcreteClusterPath & a return &dataVersion; } +chip::Span emberAfDeviceTypeListFromEndpoint(chip::EndpointId endpointId, CHIP_ERROR & err) +{ + auto endpoint = GetMockNodeConfig().endpointById(endpointId); + + if (endpoint == nullptr) + { + return chip::Span(); + } + + return endpoint->deviceTypes(); +} + +chip::Span emberAfDeviceTypeListFromEndpointIndex(unsigned index, CHIP_ERROR & err) +{ + if (index >= GetMockNodeConfig().endpoints.size()) + { + return chip::Span(); + } + + return GetMockNodeConfig().endpoints[index].deviceTypes(); +} + void emberAfAttributeChanged(EndpointId endpoint, ClusterId clusterId, AttributeId attributeId, AttributesChangedListener * listener) { diff --git a/src/controller/tests/data_model/DataModelFixtures.cpp b/src/controller/tests/data_model/DataModelFixtures.cpp index e30641fc530532..a5533dc51de57c 100644 --- a/src/controller/tests/data_model/DataModelFixtures.cpp +++ b/src/controller/tests/data_model/DataModelFixtures.cpp @@ -679,6 +679,17 @@ EndpointId CustomDataModel::NextEndpoint(EndpointId before) return CodegenDataModelProviderInstance()->NextEndpoint(before); } +std::optional CustomDataModel::FirstDeviceType(EndpointId endpoint) +{ + return std::nullopt; +} + +std::optional CustomDataModel::NextDeviceType(EndpointId endpoint, + const DataModel::DeviceTypeEntry & previous) +{ + return std::nullopt; +} + ClusterEntry CustomDataModel::FirstCluster(EndpointId endpoint) { return CodegenDataModelProviderInstance()->FirstCluster(endpoint); diff --git a/src/controller/tests/data_model/DataModelFixtures.h b/src/controller/tests/data_model/DataModelFixtures.h index 68d2cc103192cf..bdc251ecb7d9eb 100644 --- a/src/controller/tests/data_model/DataModelFixtures.h +++ b/src/controller/tests/data_model/DataModelFixtures.h @@ -129,6 +129,9 @@ class CustomDataModel : public DataModel::Provider EndpointId FirstEndpoint() override; EndpointId NextEndpoint(EndpointId before) override; + std::optional FirstDeviceType(EndpointId endpoint) override; + std::optional NextDeviceType(EndpointId endpoint, + const DataModel::DeviceTypeEntry & previous) override; DataModel::ClusterEntry FirstCluster(EndpointId endpoint) override; DataModel::ClusterEntry NextCluster(const ConcreteClusterPath & before) override; std::optional GetClusterInfo(const ConcreteClusterPath & path) override;