Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Power source: Implementation of dynamic endpoint list setter #28110

Merged
merged 15 commits into from
Aug 4, 2023
Merged

Power source: Implementation of dynamic endpoint list setter #28110

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
15d0964
Implementation of dynamic endpoint list setter
cecille Jul 17, 2023
b5276c1
Restyled by whitespace
restyled-commits Jul 20, 2023
ac8c518
Restyled by gn
restyled-commits Jul 20, 2023
169874c
Remove the define
cecille Jul 20, 2023
4470acd
Address review comments
cecille Jul 21, 2023
ee7f611
free is being called, man, where's the leak?
cecille Jul 21, 2023
b14cf2d
Restyled by clang-format
restyled-commits Jul 21, 2023
3819589
Address some review comments.
cecille Aug 1, 2023
a573371
Fix leak.
cecille Aug 1, 2023
dfea4b2
Deal with zero-length arrays.
cecille Aug 1, 2023
0ba91b3
shutdown -> clear
cecille Aug 2, 2023
a0e802b
Use EncodeList for list
cecille Aug 3, 2023
1d52db3
Merge remote-tracking branch 'upstream/master' into power_source_endp…
cecille Aug 3, 2023
f011ec1
test fix.
cecille Aug 3, 2023
bbe2790
types need to match
cecille Aug 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 69 additions & 8 deletions src/app/clusters/power-source-server/power-source-server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include <app/util/attribute-storage.h>
#include <lib/support/CodeUtils.h>
#include <lib/support/logging/CHIPLogging.h>
#include <zap-generated/gen_config.h>

using namespace chip;
using namespace app;
Expand All @@ -34,10 +35,55 @@ using namespace app::Clusters::PowerSource::Attributes;

namespace {

struct PowerSourceClusterInfo
{
PowerSourceClusterInfo() : mClusterEndpoint(kInvalidEndpointId) {}
explicit PowerSourceClusterInfo(EndpointId powerClusterEndpointId) : mClusterEndpoint(powerClusterEndpointId) {}
void Shutdown()
{
mBuf.Free();
mEndpointList = Span<EndpointId>();
}
CHIP_ERROR SetEndpointList(Span<EndpointId> endpointList)
{
Shutdown();
if (endpointList.size() == 0)
{
mEndpointList = Span<EndpointId>();
return CHIP_NO_ERROR;
}
mBuf.Calloc(endpointList.size());
if (mBuf.Get() == nullptr)
{
return CHIP_ERROR_NO_MEMORY;
}
memcpy(mBuf.Get(), endpointList.data(), endpointList.size() * sizeof(EndpointId));
mEndpointList = Span<EndpointId>(mBuf.Get(), endpointList.size());
return CHIP_NO_ERROR;
}
EndpointId mClusterEndpoint = kInvalidEndpointId;
Platform::ScopedMemoryBuffer<EndpointId> mBuf;
Span<EndpointId> mEndpointList;
};

PowerSourceServer gPowerSourceServer;

PowerSourceAttrAccess gAttrAccess;

#ifdef ZCL_USING_POWER_SOURCE_CLUSTER_SERVER
#define POWER_SERVER_NUM_SUPPORTED_ENDPOINTS \
(EMBER_AF_POWER_SOURCE_CLUSTER_SERVER_ENDPOINT_COUNT + CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT)
#else
#define POWER_SERVER_NUM_SUPPORTED_ENDPOINTS CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT
#endif
static constexpr size_t kNumSupportedEndpoints = POWER_SERVER_NUM_SUPPORTED_ENDPOINTS;

#if POWER_SERVER_NUM_SUPPORTED_ENDPOINTS > 0
PowerSourceClusterInfo sPowerSourceClusterInfo[kNumSupportedEndpoints] = {};
#else
PowerSourceClusterInfo * sPowerSourceClusterInfo = nullptr;
#endif

} // anonymous namespace

void MatterPowerSourcePluginServerInitCallback()
Expand Down Expand Up @@ -96,22 +142,24 @@ CHIP_ERROR PowerSourceServer::SetEndpointList(EndpointId powerSourceClusterEndpo
// TODO: should check here that the power source cluster exists on the endpoint, but for now let's take the caller's word for it

size_t idx = PowerSourceClusterEndpointIndex(powerSourceClusterEndpoint);
if (idx == std::numeric_limits<size_t>::max())
if (idx >= kNumSupportedEndpoints)
{
idx = NextEmptyIndex();
}
if (idx == std::numeric_limits<size_t>::max())
if (idx >= kNumSupportedEndpoints)
{
return CHIP_ERROR_NO_MEMORY;
}

sPowerSourceClusterInfo[idx].Shutdown();
if (endpointList.size() == 0)
{
mPowerSourceClusterInfo[idx] = PowerSourceClusterInfo();
sPowerSourceClusterInfo[idx] = PowerSourceClusterInfo();
}
else
{
mPowerSourceClusterInfo[idx] = PowerSourceClusterInfo(powerSourceClusterEndpoint);
mPowerSourceClusterInfo[idx].SetEndpointList(endpointList);
sPowerSourceClusterInfo[idx] = PowerSourceClusterInfo(powerSourceClusterEndpoint);
sPowerSourceClusterInfo[idx].SetEndpointList(endpointList);
}
return CHIP_NO_ERROR;
}
Expand All @@ -120,16 +168,29 @@ const Span<EndpointId> * PowerSourceServer::GetEndpointList(EndpointId powerSour
size_t idx = PowerSourceClusterEndpointIndex(powerSourceClusterEndpoint);
if (idx != std::numeric_limits<size_t>::max())
{
return &mPowerSourceClusterInfo[idx].mEndpointList;
return &sPowerSourceClusterInfo[idx].mEndpointList;
}
return nullptr;
}

void PowerSourceServer::Shutdown()
{
for (size_t i = 0; i < kNumSupportedEndpoints; ++i)
{
sPowerSourceClusterInfo[i].Shutdown();
}
}

size_t PowerSourceServer::GetNumSupportedEndpointLists() const
{
return kNumSupportedEndpoints;
}

size_t PowerSourceServer::PowerSourceClusterEndpointIndex(EndpointId endpointId) const
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably OK, but we have existing utilities (emberAfGetClusterServerEndpointIndex) for things like "map endpoint id to a list index, taking into account the fixed endpoints that we know have this cluster and the fact that all dynamic endpoints might have it"... Is there a reason we're not just using that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The endpoint count is different.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cecille Why is it different, exactly?

{
for (size_t i = 0; i < kNumSupportedEndpoints; ++i)
{
if (mPowerSourceClusterInfo[i].mClusterEndpoint == endpointId)
if (sPowerSourceClusterInfo[i].mClusterEndpoint == endpointId)
{
return i;
}
Expand All @@ -141,7 +202,7 @@ size_t PowerSourceServer::NextEmptyIndex() const
{
for (size_t i = 0; i < kNumSupportedEndpoints; ++i)
{
if (mPowerSourceClusterInfo[i].mClusterEndpoint == kInvalidEndpointId)
if (sPowerSourceClusterInfo[i].mClusterEndpoint == kInvalidEndpointId)
{
return i;
}
Expand Down
50 changes: 6 additions & 44 deletions src/app/clusters/power-source-server/power-source-server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,61 +28,23 @@ namespace chip {
namespace app {
namespace Clusters {

struct PowerSourceClusterInfo
{
PowerSourceClusterInfo() : mClusterEndpoint(kInvalidEndpointId) {}
explicit PowerSourceClusterInfo(EndpointId powerClusterEndpointId) : mClusterEndpoint(powerClusterEndpointId) {}
void Shutdown()
{
mEndpointList = Span<EndpointId>();
mBuf.Free();
}
CHIP_ERROR SetEndpointList(Span<EndpointId> endpointList)
{
Shutdown();
if (endpointList.size() == 0)
{
mEndpointList = Span<EndpointId>();
return CHIP_NO_ERROR;
}
mBuf.Calloc(endpointList.size());
if (mBuf.Get() == nullptr)
{
return CHIP_ERROR_NO_MEMORY;
}
memcpy(mBuf.Get(), endpointList.data(), endpointList.size() * sizeof(EndpointId));
mEndpointList = Span<EndpointId>(mBuf.Get(), endpointList.size());
return CHIP_NO_ERROR;
}
EndpointId mClusterEndpoint = kInvalidEndpointId;
Platform::ScopedMemoryBuffer<EndpointId> mBuf;
Span<EndpointId> mEndpointList;
};

class PowerSourceServer
{
public:
static PowerSourceServer & Instance();

// Caller does not need to retain the span past the call point as these are copied into an internal storage
CHIP_ERROR SetEndpointList(EndpointId powerSourceClusterEndpoint, Span<EndpointId> endpointList);
// returns nullptr if there's not endpoint list set for this power source cluster endpoint id.
const Span<EndpointId> * GetEndpointList(EndpointId powerSourceClusterEndpoint) const;
void Shutdown()
CHIP_ERROR ClearEndpointList(EndpointId powerSourceClusterEndpoint)
{
for (size_t i = 0; i < kNumSupportedEndpoints; ++i)
{
mPowerSourceClusterInfo[i].Shutdown();
}
return SetEndpointList(powerSourceClusterEndpoint, Span<EndpointId>());
}
size_t GetNumSupportedEndpointLists() const { return kNumSupportedEndpoints; };
// returns nullptr if there's not endpoint list set for this power source cluster endpoint id.
const Span<EndpointId> * GetEndpointList(EndpointId powerSourceClusterEndpoint) const;
void Shutdown();
size_t GetNumSupportedEndpointLists() const;

private:
// There's probably a good way to properly set this from zap or have this grow dynamically as dynamic endpoints are
// finalized, but for now, let's just say 10 power clusters is probably ok.
// TODO: is there a way to get a static count of endpoints? CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT?
static constexpr size_t kNumSupportedEndpoints = 10;
PowerSourceClusterInfo mPowerSourceClusterInfo[kNumSupportedEndpoints] = {};
// Both return std::numeric_limits<size_t>::max() for not found
size_t PowerSourceClusterEndpointIndex(EndpointId endpointId) const;
size_t NextEmptyIndex() const;
Expand Down
7 changes: 4 additions & 3 deletions src/app/tests/TestPowerSourceCluster.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ void TestPowerSourceCluster::TestEndpointList(nlTestSuite * apSuite, void * apCo
}

// Remaining endpoints - list of 1
for (EndpointId ep = 2; ep < 10; ++ep)
for (EndpointId ep = 2; ep < powerSourceServer.GetNumSupportedEndpointLists(); ++ep)
{
err = powerSourceServer.SetEndpointList(ep, Span<EndpointId>(listRest));
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
Expand All @@ -173,7 +173,8 @@ void TestPowerSourceCluster::TestEndpointList(nlTestSuite * apSuite, void * apCo
// *****************
// Check for out of memory error when setting too many endpoints
// *****************
err = powerSourceServer.SetEndpointList(11, Span<EndpointId>(listRest));
// pick a random endpoint number for the power cluster - it doesn't matter, we don't have space anyway.
err = powerSourceServer.SetEndpointList(55, Span<EndpointId>(listRest));
NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_NO_MEMORY);

// *****************
Expand Down Expand Up @@ -265,7 +266,6 @@ void TestPowerSourceCluster::TestEndpointList(nlTestSuite * apSuite, void * apCo
std::vector<EndpointId> vec = ReadEndpointsThroughAttributeReader(apSuite, ep);
NL_TEST_ASSERT(apSuite, vec.size() == 0);
}
powerSourceServer.Shutdown();
}

} // namespace app
Expand Down Expand Up @@ -301,6 +301,7 @@ int TestPowerSourceClusterContext_Setup(void * inContext)
*/
int TestPowerSourceClusterContext_Teardown(void * inContext)
{
chip::app::Clusters::PowerSourceServer::Instance().Shutdown();
chip::Platform::MemoryShutdown();
return SUCCESS;
}
Expand Down