Skip to content

Commit

Permalink
Move client cluster to use iterators, see how flash behaves after that
Browse files Browse the repository at this point in the history
  • Loading branch information
@andy31415
andy31415 committed Dec 18, 2024
1 parent 27b9ca0 commit ef07811
Show file tree
Hide file tree
Showing 9 changed files with 69 additions and 117 deletions.
9 changes: 4 additions & 5 deletions src/app/clusters/descriptor/descriptor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,12 +183,11 @@ CHIP_ERROR DescriptorAttrAccess::ReadClientServerAttribute(EndpointId endpoint,
}
else
{
ConcreteClusterPath clusterPath =
InteractionModelEngine::GetInstance()->GetDataModelProvider()->FirstClientCluster(endpoint);
while (clusterPath.HasValidIds())
auto it = InteractionModelEngine::GetInstance()->GetDataModelProvider()->GetClientClusters(endpoint);

for (auto clusterID = it->Next(); clusterID.has_value(); clusterID = it->Next())
{
ReturnErrorOnFailure(encoder.Encode(clusterPath.mClusterId));
clusterPath = InteractionModelEngine::GetInstance()->GetDataModelProvider()->NextClientCluster(clusterPath);
ReturnErrorOnFailure(encoder.Encode(*clusterID));
}
}

Expand Down
18 changes: 5 additions & 13 deletions src/app/data-model-provider/MetadataTypes.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -203,23 +203,21 @@ class ProviderMetadataTree
// - Lookups should be performed using `Get...` and `SeekTo`.
//
/////////////////////////////////////////////////////////////////////////
virtual std::unique_ptr<ElementIterator<DeviceTypeEntry>> GetDeviceTypes(EndpointId endpointId) = 0;
virtual std::unique_ptr<ElementIterator<SemanticTag>> GetSemanticTags(EndpointId endpointId) = 0;
virtual std::unique_ptr<ElementIterator<ClusterId>> GetClientClusters(EndpointId endpointId) = 0;

// TODO: below items MUST transition to pure virtual and have implementations everywhere
virtual std::unique_ptr<MetaDataIterator<EndpointId, EndpointInfo>> GetEndpoints()
{
return std::make_unique<NullMetadataIterator<EndpointId, EndpointInfo>>();
}
virtual std::unique_ptr<ElementIterator<DeviceTypeEntry>> GetDeviceTypes(EndpointId endpointId) = 0;
virtual std::unique_ptr<ElementIterator<SemanticTag>> GetSemanticTags(EndpointId endpointId) = 0;

virtual std::unique_ptr<MetaDataIterator<ClusterId, ClusterInfo>> GetServerClusters(EndpointId endpointId)
{
return std::make_unique<NullMetadataIterator<ClusterId, ClusterInfo>>();
}

virtual std::unique_ptr<ElementIterator<ClusterId>> GetClientClusters(EndpointId endpointId)
{
return std::make_unique<NullElementIterator<ClusterId>>();
}

virtual std::unique_ptr<MetaDataIterator<AttributeId, AttributeInfo>> GetAttributes(ConcreteClusterPath clusterPath)
{
return std::make_unique<NullMetadataIterator<AttributeId, AttributeInfo>>();
Expand All @@ -244,12 +242,6 @@ class ProviderMetadataTree
virtual ClusterEntry NextServerCluster(const ConcreteClusterPath & before) = 0;
virtual std::optional<ClusterInfo> GetServerClusterInfo(const ConcreteClusterPath & path) = 0;

// This iteration will list all client clusters on a given endpoint
// As the client cluster is only a client without any attributes/commands,
// these functions only return the cluster path.
virtual ConcreteClusterPath FirstClientCluster(EndpointId endpoint) = 0;
virtual ConcreteClusterPath NextClientCluster(const ConcreteClusterPath & before) = 0;

// Attribute iteration and accessors provide cluster-level access over
// attributes
virtual AttributeEntry FirstAttribute(const ConcreteClusterPath & cluster) = 0;
Expand Down
9 changes: 2 additions & 7 deletions src/app/tests/test-interaction-model-api.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,14 +179,9 @@ std::optional<ClusterInfo> TestImCustomDataModel::GetServerClusterInfo(const Con
return CodegenDataModelProviderInstance(nullptr /* delegate */)->GetServerClusterInfo(path);
}

ConcreteClusterPath TestImCustomDataModel::FirstClientCluster(EndpointId endpoint)
std::unique_ptr<DataModel::ElementIterator<ClusterId>> TestImCustomDataModel::GetClientClusters(EndpointId endpointId)
{
return CodegenDataModelProviderInstance(nullptr /* delegate */)->FirstClientCluster(endpoint);
}

ConcreteClusterPath TestImCustomDataModel::NextClientCluster(const ConcreteClusterPath & before)
{
return CodegenDataModelProviderInstance(nullptr /* delegate */)->NextClientCluster(before);
return CodegenDataModelProviderInstance(nullptr /* delegate */)->GetClientClusters(endpointId);
}

AttributeEntry TestImCustomDataModel::FirstAttribute(const ConcreteClusterPath & cluster)
Expand Down
3 changes: 1 addition & 2 deletions src/app/tests/test-interaction-model-api.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,14 +110,13 @@ class TestImCustomDataModel : public DataModel::Provider
std::optional<DataModel::ActionReturnStatus> Invoke(const DataModel::InvokeRequest & request,
chip::TLV::TLVReader & input_arguments, CommandHandler * handler) override;

std::unique_ptr<DataModel::ElementIterator<ClusterId>> GetClientClusters(EndpointId endpointId) override;
DataModel::EndpointEntry FirstEndpoint() override;
DataModel::EndpointEntry NextEndpoint(EndpointId before) override;
std::optional<DataModel::EndpointInfo> GetEndpointInfo(EndpointId endpoint) override;
DataModel::ClusterEntry FirstServerCluster(EndpointId endpoint) override;
DataModel::ClusterEntry NextServerCluster(const ConcreteClusterPath & before) override;
std::optional<DataModel::ClusterInfo> GetServerClusterInfo(const ConcreteClusterPath & path) override;
ConcreteClusterPath FirstClientCluster(EndpointId endpoint) override;
ConcreteClusterPath NextClientCluster(const ConcreteClusterPath & before) override;
DataModel::AttributeEntry FirstAttribute(const ConcreteClusterPath & cluster) override;
DataModel::AttributeEntry NextAttribute(const ConcreteAttributePath & before) override;
std::optional<DataModel::AttributeInfo> GetAttributeInfo(const ConcreteAttributePath & path) override;
Expand Down
9 changes: 2 additions & 7 deletions src/controller/tests/data_model/DataModelFixtures.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -507,14 +507,9 @@ std::optional<ClusterInfo> CustomDataModel::GetServerClusterInfo(const ConcreteC
return CodegenDataModelProviderInstance(nullptr /* delegate */)->GetServerClusterInfo(path);
}

ConcreteClusterPath CustomDataModel::FirstClientCluster(EndpointId endpoint)
std::unique_ptr<DataModel::ElementIterator<ClusterId>> CustomDataModel::GetClientClusters(EndpointId endpointId)
{
return CodegenDataModelProviderInstance(nullptr /* delegate */)->FirstClientCluster(endpoint);
}

ConcreteClusterPath CustomDataModel::NextClientCluster(const ConcreteClusterPath & before)
{
return CodegenDataModelProviderInstance(nullptr /* delegate */)->NextClientCluster(before);
return CodegenDataModelProviderInstance(nullptr /* delegate */)->GetClientClusters(endpointId);
}

AttributeEntry CustomDataModel::FirstAttribute(const ConcreteClusterPath & cluster)
Expand Down
3 changes: 1 addition & 2 deletions src/controller/tests/data_model/DataModelFixtures.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,14 +122,13 @@ class CustomDataModel : public DataModel::Provider
std::optional<DataModel::ActionReturnStatus> Invoke(const DataModel::InvokeRequest & request,
chip::TLV::TLVReader & input_arguments, CommandHandler * handler) override;

std::unique_ptr<DataModel::ElementIterator<ClusterId>> GetClientClusters(EndpointId endpointId) override;
DataModel::EndpointEntry FirstEndpoint() override;
DataModel::EndpointEntry NextEndpoint(EndpointId before) override;
std::optional<DataModel::EndpointInfo> GetEndpointInfo(EndpointId endpoint) override;
DataModel::ClusterEntry FirstServerCluster(EndpointId endpoint) override;
DataModel::ClusterEntry NextServerCluster(const ConcreteClusterPath & before) override;
std::optional<DataModel::ClusterInfo> GetServerClusterInfo(const ConcreteClusterPath & path) override;
ConcreteClusterPath FirstClientCluster(EndpointId endpoint) override;
ConcreteClusterPath NextClientCluster(const ConcreteClusterPath & before) override;
DataModel::AttributeEntry FirstAttribute(const ConcreteClusterPath & cluster) override;
DataModel::AttributeEntry NextAttribute(const ConcreteAttributePath & before) override;
std::optional<DataModel::AttributeInfo> GetAttributeInfo(const ConcreteAttributePath & path) override;
Expand Down
74 changes: 34 additions & 40 deletions src/data-model-providers/codegen/CodegenDataModelProvider.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,23 +211,6 @@ DataModel::ClusterEntry FirstServerClusterEntry(EndpointId endpointId, const Emb
return DataModel::ClusterEntry::kInvalid;
}

ClusterId FirstClientClusterId(const EmberAfEndpointType * endpoint, unsigned start_index, unsigned & found_index)
{
for (unsigned cluster_idx = start_index; cluster_idx < endpoint->clusterCount; cluster_idx++)
{
const EmberAfCluster & cluster = endpoint->cluster[cluster_idx];
if (!cluster.IsClient())
{
continue;
}

found_index = cluster_idx;
return cluster.clusterId;
}

return kInvalidClusterId;
}

/// Load the attribute information into the specified destination
///
/// `info` is assumed to be default-constructed/clear (i.e. this sets flags, but does not reset them).
Expand Down Expand Up @@ -367,6 +350,35 @@ class SemanticTagIterator : public DataModel::ElementIterator<DataModel::Provide
size_t mIndex = 0;
};

class ClientClusterIdIterator : public DataModel::ElementIterator<ClusterId>
{
public:
ClientClusterIdIterator() = default;
ClientClusterIdIterator(Span<const EmberAfCluster> clusters) : mClusters(clusters) {}

std::optional<ClusterId> Next() override
{
while (true)
{
if (mClusters.empty())
{
return std::nullopt;
}

if (mClusters.front().IsClient())
{
ClusterId id = mClusters.front().clusterId;
mClusters = mClusters.SubSpan(1);
return id;
}
mClusters = mClusters.SubSpan(1);
}
}

private:
Span<const EmberAfCluster> mClusters;
};

DefaultAttributePersistenceProvider gDefaultAttributePersistence;

} // namespace
Expand Down Expand Up @@ -629,33 +641,15 @@ std::optional<DataModel::ClusterInfo> CodegenDataModelProvider::GetServerCluster
return std::make_optional(std::get<DataModel::ClusterInfo>(info));
}

ConcreteClusterPath CodegenDataModelProvider::FirstClientCluster(EndpointId endpointId)
std::unique_ptr<DataModel::ElementIterator<ClusterId>> CodegenDataModelProvider::GetClientClusters(EndpointId endpointId)
{
const EmberAfEndpointType * endpoint = emberAfFindEndpointType(endpointId);
VerifyOrReturnValue(endpoint != nullptr, ConcreteClusterPath(endpointId, kInvalidClusterId));
VerifyOrReturnValue(endpoint->clusterCount > 0, ConcreteClusterPath(endpointId, kInvalidClusterId));
VerifyOrReturnValue(endpoint->cluster != nullptr, ConcreteClusterPath(endpointId, kInvalidClusterId));

return ConcreteClusterPath(endpointId, FirstClientClusterId(endpoint, 0, mClientClusterIterationHint));
}

ConcreteClusterPath CodegenDataModelProvider::NextClientCluster(const ConcreteClusterPath & before)
{
// TODO: This search still seems slow (ember will loop). Should use index hints as long
// as ember API supports it
const EmberAfEndpointType * endpoint = emberAfFindEndpointType(before.mEndpointId);

VerifyOrReturnValue(endpoint != nullptr, ConcreteClusterPath(before.mEndpointId, kInvalidClusterId));
VerifyOrReturnValue(endpoint->clusterCount > 0, ConcreteClusterPath(before.mEndpointId, kInvalidClusterId));
VerifyOrReturnValue(endpoint->cluster != nullptr, ConcreteClusterPath(before.mEndpointId, kInvalidClusterId));

std::optional<unsigned> cluster_idx = TryFindClusterIndex(endpoint, before.mClusterId, ClusterSide::kClient);
if (!cluster_idx.has_value())
{
return ConcreteClusterPath(before.mEndpointId, kInvalidClusterId);
}
VerifyOrReturnValue(endpoint != nullptr, std::make_unique<ClientClusterIdIterator>());
VerifyOrReturnValue(endpoint->clusterCount > 0, std::make_unique<ClientClusterIdIterator>());
VerifyOrReturnValue(endpoint->cluster != nullptr, std::make_unique<ClientClusterIdIterator>());

return ConcreteClusterPath(before.mEndpointId, FirstClientClusterId(endpoint, *cluster_idx + 1, mClientClusterIterationHint));
return std::make_unique<ClientClusterIdIterator>(Span(endpoint->cluster, endpoint->clusterCount));
}

DataModel::AttributeEntry CodegenDataModelProvider::FirstAttribute(const ConcreteClusterPath & path)
Expand Down
4 changes: 1 addition & 3 deletions src/data-model-providers/codegen/CodegenDataModelProvider.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ class CodegenDataModelProvider : public DataModel::Provider
/// attribute tree iteration
std::unique_ptr<DataModel::ElementIterator<DataModel::DeviceTypeEntry>> GetDeviceTypes(EndpointId endpointId) override;
std::unique_ptr<DataModel::ElementIterator<SemanticTag>> GetSemanticTags(EndpointId endpointId) override;
std::unique_ptr<DataModel::ElementIterator<ClusterId>> GetClientClusters(EndpointId endpointId) override;

DataModel::EndpointEntry FirstEndpoint() override;
DataModel::EndpointEntry NextEndpoint(EndpointId before) override;
Expand All @@ -167,9 +168,6 @@ class CodegenDataModelProvider : public DataModel::Provider
DataModel::ClusterEntry NextServerCluster(const ConcreteClusterPath & before) override;
std::optional<DataModel::ClusterInfo> GetServerClusterInfo(const ConcreteClusterPath & path) override;

ConcreteClusterPath FirstClientCluster(EndpointId endpoint) override;
ConcreteClusterPath NextClientCluster(const ConcreteClusterPath & before) override;

DataModel::AttributeEntry FirstAttribute(const ConcreteClusterPath & cluster) override;
DataModel::AttributeEntry NextAttribute(const ConcreteAttributePath & before) override;
std::optional<DataModel::AttributeInfo> GetAttributeInfo(const ConcreteAttributePath & path) override;
Expand Down
57 changes: 19 additions & 38 deletions src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
* limitations under the License.
*/

#include "pw_unit_test/framework.h"
#include <pw_unit_test/framework.h>

#include <data-model-providers/codegen/tests/EmberInvokeOverride.h>
Expand Down Expand Up @@ -1079,53 +1080,33 @@ TEST(TestCodegenModelViaMocks, IterateOverClientClusters)
UseMockNodeConfig config(gTestNodeConfig);
CodegenDataModelProviderWithContext model;

EXPECT_FALSE(model.FirstClientCluster(kEndpointIdThatIsMissing).HasValidIds());
EXPECT_FALSE(model.FirstClientCluster(kInvalidEndpointId).HasValidIds());
EXPECT_FALSE(model.NextClientCluster(ConcreteClusterPath(kInvalidEndpointId, 123)).HasValidIds());
EXPECT_FALSE(model.NextClientCluster(ConcreteClusterPath(kMockEndpoint1, kInvalidClusterId)).HasValidIds());
EXPECT_FALSE(model.NextClientCluster(ConcreteClusterPath(kMockEndpoint1, 981u)).HasValidIds());
EXPECT_FALSE(model.GetClientClusters(kEndpointIdThatIsMissing)->Next().has_value());
EXPECT_FALSE(model.GetClientClusters(kInvalidEndpointId)->Next().has_value());

// mock endpoint 1 has 2 mock client clusters: 3 and 4
ConcreteClusterPath path = model.FirstClientCluster(kMockEndpoint1);
ASSERT_TRUE(path.HasValidIds());
EXPECT_EQ(path.mEndpointId, kMockEndpoint1);
EXPECT_EQ(path.mClusterId, MockClusterId(3));
auto it = model.GetClientClusters(kMockEndpoint1);
auto clusterId = it->Next();
ASSERT_TRUE(clusterId.has_value());
EXPECT_EQ(*clusterId, MockClusterId(3)); // NOLINT(bugprone-unchecked-optional-access)

path = model.NextClientCluster(path);
ASSERT_TRUE(path.HasValidIds());
EXPECT_EQ(path.mEndpointId, kMockEndpoint1);
EXPECT_EQ(path.mClusterId, MockClusterId(4));
clusterId = it->Next();
ASSERT_TRUE(clusterId.has_value());
EXPECT_EQ(*clusterId, MockClusterId(4)); // NOLINT(bugprone-unchecked-optional-access)

path = model.NextClientCluster(path);
EXPECT_FALSE(path.HasValidIds());
clusterId = it->Next();
EXPECT_FALSE(clusterId.has_value());

// mock endpoint 2 has 1 mock client clusters: 3(has server side at the same time) and 4
path = model.FirstClientCluster(kMockEndpoint2);
for (uint16_t clusterId = 3; clusterId <= 4; clusterId++)
it = model.GetClientClusters(kMockEndpoint2);
clusterId = it->Next();
for (uint16_t expectedId = 3; expectedId <= 4; expectedId++)
{
ASSERT_TRUE(path.HasValidIds());
EXPECT_EQ(path.mEndpointId, kMockEndpoint2);
EXPECT_EQ(path.mClusterId, MockClusterId(clusterId));
path = model.NextClientCluster(path);
}
EXPECT_FALSE(path.HasValidIds());
ASSERT_TRUE(clusterId.has_value());
EXPECT_EQ(*clusterId, MockClusterId(expectedId)); // NOLINT(bugprone-unchecked-optional-access)

// repeat calls should work
for (int i = 0; i < 10; i++)
{
path = model.FirstClientCluster(kMockEndpoint1);
ASSERT_TRUE(path.HasValidIds());
EXPECT_EQ(path.mEndpointId, kMockEndpoint1);
EXPECT_EQ(path.mClusterId, MockClusterId(3));
}

for (int i = 0; i < 10; i++)
{
ConcreteClusterPath nextPath = model.NextClientCluster(path);
ASSERT_TRUE(nextPath.HasValidIds());
EXPECT_EQ(nextPath.mEndpointId, kMockEndpoint1);
EXPECT_EQ(nextPath.mClusterId, MockClusterId(4));
clusterId = it->Next();
}
ASSERT_FALSE(clusterId.has_value());
}

TEST(TestCodegenModelViaMocks, IterateOverAttributes)
Expand Down

0 comments on commit ef07811

Please sign in to comment.