Skip to content

Commit 2daa37c

Browse files
andreilitvinandreilitvin
committed
Updates to make callers use memory allocation
1 parent b60ebad commit 2daa37c

File tree

5 files changed

+179
-232
lines changed

5 files changed

+179
-232
lines changed

‎src/app/server-cluster/ServerClusterInterface.h

+6-1
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,12 @@ class ServerClusterInterface
4343
virtual ~ServerClusterInterface() = default;
4444

4545
///////////////////////////////////// Cluster Metadata Support //////////////////////////////////////////////////
46-
[[nodiscard]] virtual ClusterId GetClusterId() const = 0;
46+
47+
/// The path where this cluster operates on.
48+
///
49+
/// This path (endpointid,clusterid) is expected to be fixed once the server
50+
/// cluster interface is in use.
51+
[[nodiscard]] virtual ConcreteClusterPath GetPath() const = 0;
4752

4853
/// Gets the data version for this cluster instance.
4954
///

‎src/app/server-cluster/tests/TestDefaultServerCluster.cpp

+10-9
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
* See the License for the specific language governing permissions and
1515
* limitations under the License.
1616
*/
17+
#include "app/ConcreteClusterPath.h"
1718
#include <pw_unit_test/framework.h>
1819

1920
#include <access/Privilege.h>
@@ -44,9 +45,9 @@ namespace {
4445
class FakeDefaultServerCluster : public DefaultServerCluster
4546
{
4647
public:
47-
FakeDefaultServerCluster(ClusterId id) : mClusterId(id) {}
48+
FakeDefaultServerCluster(ConcreteClusterPath path) : mPath(path) {}
4849

49-
ClusterId GetClusterId() const override { return mClusterId; }
50+
[[nodiscard]] ConcreteClusterPath GetPath() const override { return mPath; }
5051

5152
DataModel::ActionReturnStatus ReadAttribute(const DataModel::ReadAttributeRequest & request,
5253
AttributeValueEncoder & encoder) override
@@ -64,14 +65,14 @@ class FakeDefaultServerCluster : public DefaultServerCluster
6465
void TestIncreaseDataVersion() { IncreaseDataVersion(); }
6566

6667
private:
67-
ClusterId mClusterId;
68+
ConcreteClusterPath mPath;
6869
};
6970

7071
} // namespace
7172

7273
TEST(TestDefaultServerCluster, TestDataVersion)
7374
{
74-
FakeDefaultServerCluster cluster(1);
75+
FakeDefaultServerCluster cluster({ 1, 2 });
7576

7677
DataVersion v1 = cluster.GetDataVersion();
7778
cluster.TestIncreaseDataVersion();
@@ -80,13 +81,13 @@ TEST(TestDefaultServerCluster, TestDataVersion)
8081

8182
TEST(TestDefaultServerCluster, TestFlagsDefault)
8283
{
83-
FakeDefaultServerCluster cluster(1);
84+
FakeDefaultServerCluster cluster({ 1, 2 });
8485
ASSERT_EQ(cluster.GetClusterFlags().Raw(), 0u);
8586
}
8687

8788
TEST(TestDefaultServerCluster, AttributesDefault)
8889
{
89-
FakeDefaultServerCluster cluster(1);
90+
FakeDefaultServerCluster cluster({ 1, 2 });
9091

9192
DataModel::ListBuilder<AttributeEntry> attributes;
9293

@@ -114,7 +115,7 @@ TEST(TestDefaultServerCluster, AttributesDefault)
114115

115116
TEST(TestDefaultServerCluster, CommandsDefault)
116117
{
117-
FakeDefaultServerCluster cluster(1);
118+
FakeDefaultServerCluster cluster({ 1, 2 });
118119

119120
DataModel::ListBuilder<AcceptedCommandEntry> acceptedCommands;
120121
ASSERT_EQ(cluster.AcceptedCommands({ 1, 1 }, acceptedCommands), CHIP_NO_ERROR);
@@ -127,7 +128,7 @@ TEST(TestDefaultServerCluster, CommandsDefault)
127128

128129
TEST(TestDefaultServerCluster, WriteAttributeDefault)
129130
{
130-
FakeDefaultServerCluster cluster(1);
131+
FakeDefaultServerCluster cluster({ 1, 2 });
131132

132133
WriteOperation test(0 /* endpoint */, 1 /* cluster */, 1234 /* attribute */);
133134
test.SetSubjectDescriptor(kAdminSubjectDescriptor);
@@ -140,7 +141,7 @@ TEST(TestDefaultServerCluster, WriteAttributeDefault)
140141

141142
TEST(TestDefaultServerCluster, InvokeDefault)
142143
{
143-
FakeDefaultServerCluster cluster(1);
144+
FakeDefaultServerCluster cluster({ 1, 2 });
144145

145146
TLV::TLVReader tlvReader;
146147
InvokeRequest request;

‎src/data-model-providers/codegen/ServerClusterInterfaceRegistry.cpp

+45-109
Original file line numberDiff line numberDiff line change
@@ -26,86 +26,52 @@
2626
namespace chip {
2727
namespace app {
2828

29-
void ServerClusterInterfaceRegistry::ClearSingleLinkedList(RegisteredServerClusterInterface * clusters)
30-
{
31-
while (clusters != nullptr)
32-
{
33-
RegisteredServerClusterInterface * next = clusters->next;
34-
Platform::Delete(clusters);
35-
clusters = next;
36-
}
37-
}
38-
3929
ServerClusterInterfaceRegistry::~ServerClusterInterfaceRegistry()
4030
{
41-
while (mEndpoints != nullptr)
31+
while (mRegistrations != nullptr)
4232
{
43-
UnregisterAllFromEndpoint(mEndpoints->endpointId);
33+
ServerClusterRegistration * next = mRegistrations->next;
34+
mRegistrations->next = nullptr;
35+
mRegistrations = next;
4436
}
4537
}
4638

47-
CHIP_ERROR ServerClusterInterfaceRegistry::AllocateNewEndpointClusters(EndpointId endpointId, EndpointClusters *& dest)
39+
CHIP_ERROR ServerClusterInterfaceRegistry::Register(ServerClusterRegistration & entry)
4840
{
49-
auto result = Platform::New<EndpointClusters>();
50-
VerifyOrReturnError(result != nullptr, CHIP_ERROR_NO_MEMORY);
41+
// we have no strong way to check if entry is already registered somewhere else, so we use "next" as some
42+
// form of double-check
43+
VerifyOrReturnError(entry.next == nullptr, CHIP_ERROR_INVALID_ARGUMENT);
44+
VerifyOrReturnError(entry.serverClusterInterface != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
5145

52-
result->endpointId = endpointId;
53-
result->firstCluster = nullptr;
54-
result->next = mEndpoints;
55-
mEndpoints = result;
56-
dest = result;
46+
ConcreteClusterPath path = entry.serverClusterInterface->GetPath();
5747

58-
return CHIP_NO_ERROR;
59-
}
60-
61-
CHIP_ERROR ServerClusterInterfaceRegistry::Register(EndpointId endpointId, ServerClusterInterface * cluster)
62-
{
63-
VerifyOrReturnError(cluster != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
64-
VerifyOrReturnError(endpointId != kInvalidEndpointId, CHIP_ERROR_INVALID_ARGUMENT);
65-
VerifyOrReturnError(cluster->GetClusterId() != kInvalidClusterId, CHIP_ERROR_INVALID_ARGUMENT);
48+
VerifyOrReturnError(path.HasValidIds(), CHIP_ERROR_INVALID_ARGUMENT);
6649

67-
// duplicate registrations are disallowed
68-
VerifyOrReturnError(Get(ConcreteClusterPath(endpointId, cluster->GetClusterId())) == nullptr, CHIP_ERROR_DUPLICATE_KEY_ID);
50+
// Double-checking for duplicates makes the checks O(n^2) on the total number of registered
51+
// items. We preserve this however we may want to make this optional at some point in time.
52+
VerifyOrReturnError(Get(path) == nullptr, CHIP_ERROR_DUPLICATE_KEY_ID);
6953

70-
EndpointClusters * endpointClusters = FindClusters(endpointId);
71-
if (endpointClusters == nullptr)
72-
{
73-
ReturnErrorOnFailure(AllocateNewEndpointClusters(endpointId, endpointClusters));
74-
}
75-
76-
auto entry = Platform::New<RegisteredServerClusterInterface>(cluster, endpointClusters->firstCluster);
77-
78-
// If this fails, we still have the `AllocateNewEndpointClusters` succeeding,
79-
// so an empty list for this cluster exists. No cleanup for now (smaller code)
80-
// as an empty list will probably not cause problems (this is the same as all
81-
// clusters on an endpoint getting unregistered - we do not clean mEndpoints except
82-
// in UnregisterAllFromEndpoint)
83-
VerifyOrReturnError(entry != nullptr, CHIP_ERROR_NO_MEMORY);
84-
85-
endpointClusters->firstCluster = entry;
54+
entry.next = mRegistrations;
55+
mRegistrations = &entry;
8656

8757
return CHIP_NO_ERROR;
8858
}
8959

9060
ServerClusterInterface * ServerClusterInterfaceRegistry::Unregister(const ConcreteClusterPath & path)
9161
{
92-
EndpointClusters * endpointClusters = FindClusters(path.mEndpointId);
93-
VerifyOrReturnValue(endpointClusters != nullptr, nullptr);
94-
VerifyOrReturnValue(endpointClusters->firstCluster != nullptr, nullptr);
95-
96-
RegisteredServerClusterInterface * prev = nullptr;
97-
RegisteredServerClusterInterface * current = endpointClusters->firstCluster;
62+
ServerClusterRegistration * prev = nullptr;
63+
ServerClusterRegistration * current = mRegistrations;
9864

9965
while (current != nullptr)
10066
{
101-
if (current->serverClusterInterface->GetClusterId() == path.mClusterId)
67+
if (current->serverClusterInterface->GetPath() == path)
10268
{
10369
// take the item out of the current list and return it.
104-
RegisteredServerClusterInterface * next = current->next;
70+
ServerClusterRegistration * next = current->next;
10571

10672
if (prev == nullptr)
10773
{
108-
endpointClusters->firstCluster = next;
74+
mRegistrations = next;
10975
}
11076
else
11177
{
@@ -114,14 +80,11 @@ ServerClusterInterface * ServerClusterInterfaceRegistry::Unregister(const Concre
11480

11581
if (mCachedInterface == current->serverClusterInterface)
11682
{
117-
mCachedClusterEndpointId = kInvalidEndpointId;
118-
mCachedInterface = nullptr;
83+
mCachedInterface = nullptr;
11984
}
12085

121-
ServerClusterInterface * result = current->serverClusterInterface;
122-
Platform::MemoryFree(current);
123-
124-
return result;
86+
current->next = nullptr; // some clearing
87+
return current->serverClusterInterface;
12588
}
12689

12790
prev = current;
@@ -134,71 +97,57 @@ ServerClusterInterface * ServerClusterInterfaceRegistry::Unregister(const Concre
13497

13598
ServerClusterInterfaceRegistry::ClustersList ServerClusterInterfaceRegistry::ClustersOnEndpoint(EndpointId endpointId)
13699
{
137-
EndpointClusters * clusters = FindClusters(endpointId);
138-
139-
if (clusters == nullptr)
140-
{
141-
return nullptr;
142-
}
143-
144-
return clusters->firstCluster;
100+
return { mRegistrations, endpointId };
145101
}
146102

147103
void ServerClusterInterfaceRegistry::UnregisterAllFromEndpoint(EndpointId endpointId)
148104
{
149-
if (mCachedClusterEndpointId == endpointId)
150-
{
151-
// all clusters on the given endpoint will be unregistered.
152-
mCachedInterface = nullptr;
153-
mCachedClusterEndpointId = kInvalidEndpointId;
154-
}
155-
156-
EndpointClusters * prev = nullptr;
157-
EndpointClusters * current = mEndpoints;
105+
ServerClusterRegistration * prev = nullptr;
106+
ServerClusterRegistration * current = mRegistrations;
158107
while (current != nullptr)
159108
{
160-
if (current->endpointId == endpointId)
109+
if (current->serverClusterInterface->GetPath().mEndpointId == endpointId)
161110
{
111+
if (mCachedInterface == current->serverClusterInterface)
112+
{
113+
mCachedInterface = nullptr;
114+
}
162115
if (prev == nullptr)
163116
{
164-
mEndpoints = current->next;
117+
mRegistrations = current->next;
165118
}
166119
else
167120
{
168121
prev->next = current->next;
169122
}
170-
ClearSingleLinkedList(current->firstCluster);
171-
Platform::Delete(current);
172-
return;
123+
ServerClusterRegistration * actual_next = current->next;
124+
current->next = nullptr; // some clearing
125+
current = actual_next;
126+
}
127+
else
128+
{
129+
prev = current;
130+
current = current->next;
173131
}
174-
175-
prev = current;
176-
current = current->next;
177132
}
178133
}
179134

180135
ServerClusterInterface * ServerClusterInterfaceRegistry::Get(const ConcreteClusterPath & path)
181136
{
182-
EndpointClusters * endpointClusters = FindClusters(path.mEndpointId);
183-
VerifyOrReturnValue(endpointClusters != nullptr, nullptr);
184-
VerifyOrReturnValue(endpointClusters->firstCluster != nullptr, nullptr);
185-
186137
// Check the cache to speed things up
187-
if ((mCachedClusterEndpointId == path.mEndpointId) && (mCachedInterface != nullptr) &&
188-
(mCachedInterface->GetClusterId() == path.mClusterId))
138+
if ((mCachedInterface != nullptr) && (mCachedInterface->GetPath() == path))
189139
{
190140
return mCachedInterface;
191141
}
192142

193143
// The cluster searched for is not cached, do a linear search for it
194-
RegisteredServerClusterInterface * current = endpointClusters->firstCluster;
144+
ServerClusterRegistration * current = mRegistrations;
195145

196146
while (current != nullptr)
197147
{
198-
if (current->serverClusterInterface->GetClusterId() == path.mClusterId)
148+
if (current->serverClusterInterface->GetPath() == path)
199149
{
200-
mCachedClusterEndpointId = path.mEndpointId;
201-
mCachedInterface = current->serverClusterInterface;
150+
mCachedInterface = current->serverClusterInterface;
202151
return mCachedInterface;
203152
}
204153

@@ -209,18 +158,5 @@ ServerClusterInterface * ServerClusterInterfaceRegistry::Get(const ConcreteClust
209158
return nullptr;
210159
}
211160

212-
ServerClusterInterfaceRegistry::EndpointClusters * ServerClusterInterfaceRegistry::FindClusters(EndpointId endpointId)
213-
{
214-
for (EndpointClusters * p = mEndpoints; p != nullptr; p = p->next)
215-
{
216-
if (p->endpointId == endpointId)
217-
{
218-
return p;
219-
}
220-
}
221-
222-
return nullptr;
223-
}
224-
225161
} // namespace app
226162
} // namespace chip

0 commit comments

Comments
 (0)