Skip to content

Commit d37a175

Browse files
andy31415andreilitvinCopilotbzbarsky-applesoares-sergio
authored andcommitted
[nrf fromtree] Provide a AttributeListBuilder to facilitate code-reuse for server cluster implementations and reduce flash overhead (#40202)
* Fix very common case of adding mandatory and optional attributes * Fix bugs * Restyle * Fix typo * Remove obsolete code * Some logic cleanup * Remove extra method * Restyle * Update src/app/server-cluster/DefaultServerCluster.cpp Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update src/app/server-cluster/DefaultServerCluster.h Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Restyle * use reference * Fix logic error * Update src/app/clusters/administrator-commissioning-server/AdministratorCommissioningCluster.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/app/server-cluster/DefaultServerCluster.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Add comments * Update src/app/server-cluster/DefaultServerCluster.cpp Co-authored-by: Sergio Soares <sergiosoares@google.com> * Refactor code into a separate AttributeListBuilder * Enable unit tests * Update src/app/server-cluster/AttributeListBuilder.cpp Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Restyle * fix cmake --------- Co-authored-by: Andrei Litvin <andreilitvin@google.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> Co-authored-by: Sergio Soares <sergiosoares@google.com>
1 parent 7e92bbb commit d37a175

9 files changed

Lines changed: 264 additions & 29 deletions

File tree

src/app/clusters/administrator-commissioning-server/AdministratorCommissioningCluster.cpp

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
#include "AdministratorCommissioningCluster.h"
1717

1818
#include <app/data-model-provider/MetadataTypes.h>
19+
#include <app/server-cluster/AttributeListBuilder.h>
1920
#include <clusters/AdministratorCommissioning/Commands.h>
2021
#include <lib/support/CodeUtils.h>
2122

@@ -40,7 +41,7 @@ constexpr DataModel::AcceptedCommandEntry kAcceptedCommandsWithBasicCommissionin
4041
AdministratorCommissioning::Commands::OpenBasicCommissioningWindow::kMetadataEntry,
4142
};
4243

43-
constexpr DataModel::AttributeEntry kAttributes[] = {
44+
constexpr DataModel::AttributeEntry kMandatoryAttributes[] = {
4445
AdministratorCommissioning::Attributes::WindowStatus::kMetadataEntry,
4546
AdministratorCommissioning::Attributes::AdminFabricIndex::kMetadataEntry,
4647
AdministratorCommissioning::Attributes::AdminVendorId::kMetadataEntry,
@@ -102,8 +103,9 @@ CHIP_ERROR AdministratorCommissioningCluster::AcceptedCommands(const ConcreteClu
102103
CHIP_ERROR AdministratorCommissioningCluster::Attributes(const ConcreteClusterPath & path,
103104
ReadOnlyBufferBuilder<DataModel::AttributeEntry> & builder)
104105
{
105-
ReturnErrorOnFailure(builder.ReferenceExisting(kAttributes));
106-
return builder.AppendElements(DefaultServerCluster::GlobalAttributes());
106+
107+
AttributeListBuilder listBuilder(builder);
108+
return listBuilder.Append(Span(kMandatoryAttributes), {});
107109
}
108110

109111
std::optional<DataModel::ActionReturnStatus> AdministratorCommissioningWithBasicCommissioningWindowCluster::InvokeCommand(

src/app/clusters/administrator-commissioning-server/tests/TestAdministratorCommissioningCluster.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,13 +51,13 @@ TEST_F(TestAdministratorCommissioningCluster, TestAttributes)
5151
ASSERT_EQ(cluster.Attributes({ kRootEndpointId, AdministratorCommissioning::Id }, builder), CHIP_NO_ERROR);
5252

5353
ReadOnlyBufferBuilder<AttributeEntry> expectedBuilder;
54-
ASSERT_EQ(expectedBuilder.ReferenceExisting(app::DefaultServerCluster::GlobalAttributes()), CHIP_NO_ERROR);
5554
ASSERT_EQ(expectedBuilder.AppendElements({
5655
AdministratorCommissioning::Attributes::WindowStatus::kMetadataEntry,
5756
AdministratorCommissioning::Attributes::AdminFabricIndex::kMetadataEntry,
5857
AdministratorCommissioning::Attributes::AdminVendorId::kMetadataEntry,
5958
}),
6059
CHIP_NO_ERROR);
60+
ASSERT_EQ(expectedBuilder.ReferenceExisting(app::DefaultServerCluster::GlobalAttributes()), CHIP_NO_ERROR);
6161

6262
ASSERT_TRUE(Testing::EqualAttributeSets(builder.TakeBuffer(), expectedBuilder.TakeBuffer()));
6363
}
@@ -70,13 +70,13 @@ TEST_F(TestAdministratorCommissioningCluster, TestAttributes)
7070
ASSERT_EQ(cluster.Attributes({ kRootEndpointId, AdministratorCommissioning::Id }, builder), CHIP_NO_ERROR);
7171

7272
ReadOnlyBufferBuilder<AttributeEntry> expectedBuilder;
73-
ASSERT_EQ(expectedBuilder.ReferenceExisting(app::DefaultServerCluster::GlobalAttributes()), CHIP_NO_ERROR);
7473
ASSERT_EQ(expectedBuilder.AppendElements({
7574
AdministratorCommissioning::Attributes::WindowStatus::kMetadataEntry,
7675
AdministratorCommissioning::Attributes::AdminFabricIndex::kMetadataEntry,
7776
AdministratorCommissioning::Attributes::AdminVendorId::kMetadataEntry,
7877
}),
7978
CHIP_NO_ERROR);
79+
ASSERT_EQ(expectedBuilder.ReferenceExisting(app::DefaultServerCluster::GlobalAttributes()), CHIP_NO_ERROR);
8080

8181
ASSERT_TRUE(Testing::EqualAttributeSets(builder.TakeBuffer(), expectedBuilder.TakeBuffer()));
8282
}

src/app/clusters/software-diagnostics-server/software-diagnostics-logic.cpp

Lines changed: 9 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
*/
1717
#include <app/clusters/software-diagnostics-server/software-diagnostics-logic.h>
1818

19+
#include <app/server-cluster/AttributeListBuilder.h>
1920
#include <app/server-cluster/DefaultServerCluster.h>
2021
#include <lib/support/CodeUtils.h>
2122

@@ -97,31 +98,16 @@ CHIP_ERROR SoftwareDiagnosticsLogic::AcceptedCommands(ReadOnlyBufferBuilder<Data
9798

9899
CHIP_ERROR SoftwareDiagnosticsLogic::Attributes(ReadOnlyBufferBuilder<DataModel::AttributeEntry> & builder)
99100
{
100-
// ensure we have space for all attributes. We may add at most 4 attributes (which are ALL optional or
101-
// guarded by feature maps)
102-
ReturnErrorOnFailure(builder.EnsureAppendCapacity(4 + DefaultServerCluster::GlobalAttributes().size()));
101+
AttributeListBuilder listBuilder(builder);
103102

104-
if (mEnabledAttributes.enableThreadMetrics)
105-
{
106-
ReturnErrorOnFailure(builder.Append(Attributes::ThreadMetrics::kMetadataEntry));
107-
}
108-
109-
if (mEnabledAttributes.enableCurrentHeapFree)
110-
{
111-
ReturnErrorOnFailure(builder.Append(Attributes::CurrentHeapFree::kMetadataEntry));
112-
}
113-
114-
if (mEnabledAttributes.enableCurrentHeapUsed)
115-
{
116-
ReturnErrorOnFailure(builder.Append(Attributes::CurrentHeapUsed::kMetadataEntry));
117-
}
118-
119-
if (mEnabledAttributes.enableCurrentWatermarks)
120-
{
121-
ReturnErrorOnFailure(builder.Append(Attributes::CurrentHeapHighWatermark::kMetadataEntry));
122-
}
103+
const AttributeListBuilder::OptionalAttributeEntry optionalEntries[] = {
104+
{ mEnabledAttributes.enableThreadMetrics, Attributes::ThreadMetrics::kMetadataEntry },
105+
{ mEnabledAttributes.enableCurrentHeapFree, Attributes::CurrentHeapFree::kMetadataEntry },
106+
{ mEnabledAttributes.enableCurrentHeapUsed, Attributes::CurrentHeapUsed::kMetadataEntry },
107+
{ mEnabledAttributes.enableCurrentWatermarks, Attributes::CurrentHeapHighWatermark::kMetadataEntry },
108+
};
123109

124-
return builder.AppendElements(DefaultServerCluster::GlobalAttributes());
110+
return listBuilder.Append(Span<const DataModel::AttributeEntry>() /* mandatory */, Span(optionalEntries));
125111
}
126112

127113
} // namespace Clusters
Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
/*
2+
* Copyright (c) 2025 Project CHIP Authors
3+
* All rights reserved.
4+
*
5+
* Licensed under the Apache License, Version 2.0 (the "License");
6+
* you may not use this file except in compliance with the License.
7+
* You may obtain a copy of the License at
8+
*
9+
* http://www.apache.org/licenses/LICENSE-2.0
10+
*
11+
* Unless required by applicable law or agreed to in writing, software
12+
* distributed under the License is distributed on an "AS IS" BASIS,
13+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
* See the License for the specific language governing permissions and
15+
* limitations under the License.
16+
*/
17+
#include <app/server-cluster/AttributeListBuilder.h>
18+
19+
#include <app/server-cluster/DefaultServerCluster.h>
20+
21+
namespace chip {
22+
namespace app {
23+
24+
CHIP_ERROR AttributeListBuilder::Append(Span<const DataModel::AttributeEntry> mandatoryAttributes,
25+
Span<const OptionalAttributeEntry> optionalAttributes)
26+
{
27+
// determine how much data to append. This should only be called if generally we have something to append
28+
size_t append_size = mandatoryAttributes.size();
29+
for (const auto & entry : optionalAttributes)
30+
{
31+
if (entry.enabled)
32+
{
33+
append_size++;
34+
}
35+
}
36+
37+
if (append_size > 0)
38+
{
39+
// NOTE: ReferenceExisting will APPEND data (and use heap) when some data already
40+
// exists in the builder. This is why we ensure AppendCapacity for everything
41+
// so that we do not perform extra allocations.
42+
ReturnErrorOnFailure(mBuilder.EnsureAppendCapacity(append_size + DefaultServerCluster::GlobalAttributes().size()));
43+
ReturnErrorOnFailure(mBuilder.ReferenceExisting(mandatoryAttributes));
44+
45+
for (const auto & entry : optionalAttributes)
46+
{
47+
if (entry.enabled)
48+
{
49+
ReturnErrorOnFailure(mBuilder.Append(entry.metadata));
50+
}
51+
}
52+
}
53+
54+
// NOTE: ReferenceExisting will APPEND data (and use heap) when some data already
55+
// exists in the builder.
56+
return mBuilder.ReferenceExisting(DefaultServerCluster::GlobalAttributes());
57+
}
58+
59+
} // namespace app
60+
} // namespace chip
Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
/*
2+
* Copyright (c) 2025 Project CHIP Authors
3+
* All rights reserved.
4+
*
5+
* Licensed under the Apache License, Version 2.0 (the "License");
6+
* you may not use this file except in compliance with the License.
7+
* You may obtain a copy of the License at
8+
*
9+
* http://www.apache.org/licenses/LICENSE-2.0
10+
*
11+
* Unless required by applicable law or agreed to in writing, software
12+
* distributed under the License is distributed on an "AS IS" BASIS,
13+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
* See the License for the specific language governing permissions and
15+
* limitations under the License.
16+
*/
17+
#pragma once
18+
19+
#include <app/data-model-provider/MetadataTypes.h>
20+
#include <lib/core/CHIPError.h>
21+
#include <lib/support/ReadOnlyBuffer.h>
22+
23+
#include <initializer_list>
24+
25+
namespace chip {
26+
namespace app {
27+
28+
/// Provides a centralized implementation of the very common operation of
29+
/// appending a list of attributes to a ReadOnlyBufferBuilder.
30+
///
31+
/// The intent is that the more complex logic of `Append` to be shared across
32+
/// cluster implementations, so that flash size is kept small.
33+
///
34+
/// Append handles both mandatory and optional attributes and also handles
35+
/// the required auto-add of `GlobalAttributes`.
36+
class AttributeListBuilder
37+
{
38+
public:
39+
AttributeListBuilder(ReadOnlyBufferBuilder<DataModel::AttributeEntry> & builder) : mBuilder(builder) {}
40+
41+
struct OptionalAttributeEntry
42+
{
43+
bool enabled; // Is this optional attribute enabled?
44+
const DataModel::AttributeEntry & metadata; // Metadata for the attribute
45+
};
46+
47+
/// Appends the given attributes to the builder.
48+
///
49+
/// It is very common to have a set of mandatory and a set of optional attributes for a
50+
/// cluster. This method allows for a single call to set up all of the given attributes in `builder`:
51+
/// - mandatoryAttributes
52+
/// - optionalAttributes IF AND ONLY IF they are enabled
53+
/// - all of `GlobalAttributes()`
54+
CHIP_ERROR Append(Span<const DataModel::AttributeEntry> mandatoryAttributes,
55+
Span<const OptionalAttributeEntry> optionalAttributes);
56+
57+
private:
58+
ReadOnlyBufferBuilder<DataModel::AttributeEntry> & mBuilder;
59+
};
60+
61+
} // namespace app
62+
} // namespace chip

src/app/server-cluster/BUILD.gn

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@ import("//build_overrides/chip.gni")
1515

1616
source_set("server-cluster") {
1717
sources = [
18+
"AttributeListBuilder.cpp",
19+
"AttributeListBuilder.h",
1820
"DefaultServerCluster.cpp",
1921
"DefaultServerCluster.h",
2022
"ServerClusterContext.h",

src/app/server-cluster/tests/BUILD.gn

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,10 @@ import("${chip_root}/build/chip/chip_test_suite.gni")
1818
chip_test_suite("tests") {
1919
output_name = "libServerClusterInterfaceTests"
2020

21-
test_sources = [ "TestDefaultServerCluster.cpp" ]
21+
test_sources = [
22+
"TestAttributeListBuilder.cpp",
23+
"TestDefaultServerCluster.cpp",
24+
]
2225

2326
cflags = [ "-Wconversion" ]
2427

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
2+
/*
3+
* Copyright (c) 2025 Project CHIP Authors
4+
* All rights reserved.
5+
*
6+
* Licensed under the Apache License, Version 2.0 (the "License");
7+
* you may not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing, software
13+
* distributed under the License is distributed on an "AS IS" BASIS,
14+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
* See the License for the specific language governing permissions and
16+
* limitations under the License.
17+
*/
18+
#include <pw_unit_test/framework.h>
19+
20+
#include <app-common/zap-generated/ids/Attributes.h>
21+
#include <app/server-cluster/AttributeListBuilder.h>
22+
#include <app/server-cluster/DefaultServerCluster.h>
23+
24+
#include <cstdlib>
25+
#include <optional>
26+
27+
using namespace chip;
28+
using namespace chip::app;
29+
using namespace chip::app::DataModel;
30+
using namespace chip::app::Clusters;
31+
32+
namespace {
33+
34+
// Initialize memory as ReadOnlyBufferBuilder may allocate
35+
struct TestAttributeListBuilder : public ::testing::Test
36+
{
37+
static void SetUpTestSuite() { ASSERT_EQ(chip::Platform::MemoryInit(), CHIP_NO_ERROR); }
38+
static void TearDownTestSuite() { chip::Platform::MemoryShutdown(); }
39+
};
40+
41+
} // namespace
42+
43+
TEST_F(TestAttributeListBuilder, Append)
44+
{
45+
const size_t global_attribute_count = DefaultServerCluster::GlobalAttributes().size();
46+
47+
constexpr BitFlags<DataModel::AttributeQualityFlags> kNoFlags;
48+
49+
// Only mandatory attributes
50+
{
51+
const AttributeEntry mandatory[] = {
52+
{ 1, kNoFlags, Access::Privilege::kView, std::nullopt },
53+
{ 2, kNoFlags, Access::Privilege::kView, std::nullopt },
54+
};
55+
56+
ReadOnlyBufferBuilder<AttributeEntry> builder;
57+
ASSERT_EQ(AttributeListBuilder(builder).Append(Span(mandatory), {}), CHIP_NO_ERROR);
58+
59+
ReadOnlyBuffer<AttributeEntry> result = builder.TakeBuffer();
60+
ASSERT_EQ(result.size(), 2 + global_attribute_count);
61+
ASSERT_EQ(result[0].attributeId, 1u);
62+
ASSERT_EQ(result[1].attributeId, 2u);
63+
ASSERT_EQ(result[2].attributeId, Globals::Attributes::ClusterRevision::Id);
64+
}
65+
66+
// Only optional attributes
67+
{
68+
const AttributeEntry optional1_meta(10, kNoFlags, Access::Privilege::kView, std::nullopt);
69+
const AttributeEntry optional2_meta(11, kNoFlags, Access::Privilege::kView, std::nullopt);
70+
const AttributeEntry optional3_meta(12, kNoFlags, Access::Privilege::kView, std::nullopt);
71+
72+
ReadOnlyBufferBuilder<AttributeEntry> builder;
73+
const AttributeListBuilder::OptionalAttributeEntry optionalEntries[] = {
74+
{ .enabled = true, .metadata = optional1_meta },
75+
{ .enabled = false, .metadata = optional2_meta },
76+
{ .enabled = true, .metadata = optional3_meta },
77+
};
78+
ASSERT_EQ(AttributeListBuilder(builder).Append({}, Span(optionalEntries)), CHIP_NO_ERROR);
79+
80+
ReadOnlyBuffer<AttributeEntry> result = builder.TakeBuffer();
81+
ASSERT_EQ(result.size(), 2 + global_attribute_count);
82+
ASSERT_EQ(result[0].attributeId, 10u);
83+
ASSERT_EQ(result[1].attributeId, 12u);
84+
ASSERT_EQ(result[2].attributeId, Globals::Attributes::ClusterRevision::Id);
85+
}
86+
87+
// Mix of mandatory and optional attributes
88+
{
89+
const AttributeEntry mandatory[] = {
90+
{ 1, kNoFlags, Access::Privilege::kView, std::nullopt },
91+
};
92+
const AttributeEntry optional1_meta(10, kNoFlags, Access::Privilege::kView, std::nullopt);
93+
const AttributeEntry optional2_meta(11, kNoFlags, Access::Privilege::kView, std::nullopt);
94+
95+
ReadOnlyBufferBuilder<AttributeEntry> builder;
96+
const AttributeListBuilder::OptionalAttributeEntry optionalEntries[] = {
97+
{ .enabled = true, .metadata = optional1_meta },
98+
{ .enabled = false, .metadata = optional2_meta },
99+
};
100+
ASSERT_EQ(AttributeListBuilder(builder).Append(Span(mandatory), Span(optionalEntries)), CHIP_NO_ERROR);
101+
102+
ReadOnlyBuffer<AttributeEntry> result = builder.TakeBuffer();
103+
ASSERT_EQ(result.size(), 2 + global_attribute_count);
104+
ASSERT_EQ(result[0].attributeId, 1u);
105+
ASSERT_EQ(result[1].attributeId, 10u);
106+
ASSERT_EQ(result[2].attributeId, Globals::Attributes::ClusterRevision::Id);
107+
}
108+
109+
// No attributes
110+
{
111+
ReadOnlyBufferBuilder<AttributeEntry> builder;
112+
ASSERT_EQ(AttributeListBuilder(builder).Append({}, {}), CHIP_NO_ERROR);
113+
114+
ReadOnlyBuffer<AttributeEntry> result = builder.TakeBuffer();
115+
ASSERT_EQ(result.size(), global_attribute_count);
116+
ASSERT_EQ(result[0].attributeId, Globals::Attributes::ClusterRevision::Id);
117+
}
118+
}

src/data-model-providers/codegen/model.cmake

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ SET(CODEGEN_DATA_MODEL_SOURCES
2727

2828
# These are dependencies from model.gni that are not included directly in cmake
2929
# "${chip_root}/src/app/server-cluster",
30+
"${BASE_DIR}/../../app/server-cluster/AttributeListBuilder.cpp"
31+
"${BASE_DIR}/../../app/server-cluster/AttributeListBuilder.h"
3032
"${BASE_DIR}/../../app/server-cluster/DefaultServerCluster.cpp"
3133
"${BASE_DIR}/../../app/server-cluster/DefaultServerCluster.h"
3234
"${BASE_DIR}/../../app/server-cluster/ServerClusterContext.h"

0 commit comments

Comments
 (0)