Skip to content

Fixes for an ideal change #38485

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

Closed
wants to merge 11 commits into from
Closed

Fixes for an ideal change #38485

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
ca35731
Fixes for an ideal change
ratgr Apr 18, 2025
6159e80
Command handler interface fix
ratgr Apr 21, 2025
a076ff2
Restyled by clang-format
restyled-commits Apr 21, 2025
4541017
removed generator on build.gn
ratgr Apr 21, 2025
7da47df
append entries
ratgr Apr 21, 2025
fb65598
moree fixes
ratgr Apr 21, 2025
eeeed99
Fixing network commissioning
ratgr Apr 21, 2025
af668aa
network commisioning fixes
ratgr Apr 21, 2025
4fc481c
Adding Metadata.h to clusters
ratgr Apr 21, 2025
aeed6df
Fixed dependencies?
ratgr Apr 21, 2025
32f6703
More fixes
ratgr Apr 22, 2025
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
84 changes: 80 additions & 4 deletions src/app/CommandHandlerInterface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,13 @@
#include <app/CommandHandler.h>
#include <app/ConcreteClusterPath.h>
#include <app/ConcreteCommandPath.h>
#include <app/data-model-provider/MetadataTypes.h>
#include <app/data-model/Decode.h>
#include <app/data-model/List.h> // So we can encode lists
#include <lib/core/DataModelTypes.h>
#include <lib/support/Iterators.h>
#include <lib/support/ReadOnlyBuffer.h>
#include <lib/support/SplitLambda.h>

namespace chip {
namespace app {
Expand Down Expand Up @@ -101,8 +104,6 @@ class CommandHandlerInterface
*/
virtual void InvokeCommand(HandlerContext & handlerContext) = 0;

typedef Loop (*CommandIdCallback)(CommandId id, void * context);

/**
* Function that may be implemented to enumerate accepted (client-to-server)
* commands for the given cluster.
Expand All @@ -122,7 +123,8 @@ class CommandHandlerInterface
* This is used by callbacks that just look for a particular value in the
* list.
*/
virtual CHIP_ERROR EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context)
virtual CHIP_ERROR EnumerateAcceptedCommands(const ConcreteClusterPath & cluster,
ReadOnlyBufferBuilder<DataModel::AcceptedCommandEntry> & builder)
{
return CHIP_ERROR_NOT_IMPLEMENTED;
}
Expand All @@ -146,7 +148,7 @@ class CommandHandlerInterface
* This is used by callbacks that just look for a particular value in the
* list.
*/
virtual CHIP_ERROR EnumerateGeneratedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context)
virtual CHIP_ERROR EnumerateGeneratedCommands(const ConcreteClusterPath & cluster, ReadOnlyBufferBuilder<CommandId> & builder)
{
return CHIP_ERROR_NOT_IMPLEMENTED;
}
Expand Down Expand Up @@ -232,5 +234,79 @@ class CommandHandlerInterface
CommandHandlerInterface * mNext = nullptr;
};

class CommandHandlerInterfaceShim : public CommandHandlerInterface
{
public:
using CommandHandlerInterface::CommandHandlerInterface;
typedef Loop (*CommandIdCallback)(CommandId id, void * context);
typedef Loop (*CommandEntryCallback)(DataModel::AcceptedCommandEntry id, void * context);
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 not what the old interface is though: old interface only returned an id and this is the error we are trying to fix.

If we provide a compatibility shim, we have to provide one that converts the id to an entry ... and that is the hard/awkard part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this change focuses on minimizing code size, but the user will have to change from ::Id to ::Metadata, a very simple task however needs a programmer to do


inline CHIP_ERROR EnumerateAcceptedCommands(const ConcreteClusterPath & cluster,
ReadOnlyBufferBuilder<DataModel::AcceptedCommandEntry> & builder) override
{
size_t commandCount = 0;
CHIP_ERROR err = CHIP_NO_ERROR;

auto counter = SplitLambda([&](DataModel::AcceptedCommandEntry commandId) {
commandCount++;
return Loop::Continue;
});

ReturnErrorOnFailure(EnumerateAcceptedCommands(cluster, counter.Caller(), counter.Context()));
ReturnErrorOnFailure(builder.EnsureAppendCapacity(commandCount));

auto appender = SplitLambda([&](DataModel::AcceptedCommandEntry entry) {
err = builder.Append(entry);
return err == CHIP_NO_ERROR ? Loop::Continue : Loop::Break;
});

ReturnErrorOnFailure(EnumerateAcceptedCommands(cluster, appender.Caller(), appender.Context()));
ReturnErrorOnFailure(err);
// the two invocations MUST return the same sizes
VerifyOrReturnError(builder.Size() == commandCount, CHIP_ERROR_INTERNAL);
return CHIP_NO_ERROR;
}

// [[deprecated("This interface is only provided to make the transition simpler,"
// "and it might be removed on any subsequent releases")]]
virtual CHIP_ERROR EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandEntryCallback callback, void * context)
{
return CHIP_ERROR_NOT_IMPLEMENTED;
}

// [[deprecated("This interface is only provided to make the transition simpler,"
// "and it might be removed on any subsequent releases")]]
virtual CHIP_ERROR EnumerateGeneratedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context)
{
return CHIP_ERROR_NOT_IMPLEMENTED;
}

inline CHIP_ERROR EnumerateGeneratedCommands(const ConcreteClusterPath & cluster,
ReadOnlyBufferBuilder<CommandId> & builder) override
{
size_t commandCount = 0;
CHIP_ERROR err = CHIP_NO_ERROR;

auto counter = SplitLambda([&](CommandId commandId) {
commandCount++;
return Loop::Continue;
});

ReturnErrorOnFailure(this->EnumerateGeneratedCommands(cluster, counter.Caller(), counter.Context()));
ReturnErrorOnFailure(builder.EnsureAppendCapacity(commandCount));

auto appender = SplitLambda([&](CommandId commandId) {
err = builder.Append(commandId);
return err == CHIP_NO_ERROR ? Loop::Continue : Loop::Break;
});

ReturnErrorOnFailure(this->EnumerateGeneratedCommands(cluster, appender.Caller(), appender.Context()));
ReturnErrorOnFailure(err);
// the two invocations MUST return the same sizes
VerifyOrReturnError(builder.Size() == commandCount, CHIP_ERROR_INTERNAL);
return CHIP_NO_ERROR;
}
};

} // namespace app
} // namespace chip
9 changes: 9 additions & 0 deletions src/app/chip_data_model.gni
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,10 @@ template("chip_data_model") {
"${_app_root}/clusters/${cluster}/resource-monitoring-cluster-objects.cpp",
"${_app_root}/clusters/${cluster}/resource-monitoring-cluster-objects.h",
]
} else if (cluster == "network-commissioning") {
deps += [ "${chip_root}/zzz_generated/app-common/clusters/NetworkCommissioning:metadata" ]
} else if (cluster == "software-diagnostics-server") {
deps += [ "${chip_root}/zzz_generated/app-common/clusters/SoftwareDiagnostics:metadata" ]
} else if (cluster == "concentration-measurement-server") {
sources += [
"${_app_root}/clusters/${cluster}/${cluster}.h",
Expand Down Expand Up @@ -391,6 +395,9 @@ template("chip_data_model") {
"${_app_root}/clusters/${cluster}/${cluster}.h",
"${_app_root}/clusters/${cluster}/EnergyEvseTestEventTriggerHandler.h",
]
deps += [
"${chip_root}/zzz_generated/app-common/clusters/EnergyEvse:metadata",
]
} else if (cluster == "diagnostic-logs-server") {
sources += [
"${_app_root}/clusters/${cluster}/${cluster}.cpp",
Expand All @@ -411,6 +418,8 @@ template("chip_data_model") {
"${_app_root}/clusters/${cluster}/${cluster}.h",
"${_app_root}/clusters/${cluster}/DeviceEnergyManagementTestEventTriggerHandler.h",
]

deps += [ "${chip_root}/zzz_generated/app-common/clusters/DeviceEnergyManagement:metadata" ]
} else if (cluster == "thread-network-diagnostics-server") {
sources += [
"${_app_root}/clusters/${cluster}/${cluster}.cpp",
Expand Down
27 changes: 12 additions & 15 deletions src/app/clusters/device-energy-management-server/device-energy-management-server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <app/ConcreteAttributePath.h>
#include <app/InteractionModelEngine.h>
#include <app/util/attribute-storage.h>
#include <clusters/DeviceEnergyManagement/Metadata.h>

using namespace chip;
using namespace chip::app;
Expand Down Expand Up @@ -104,49 +105,45 @@ CHIP_ERROR Instance::Read(const ConcreteReadAttributePath & aPath, AttributeValu
}

// CommandHandlerInterface
CHIP_ERROR Instance::EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context)
CHIP_ERROR Instance::EnumerateAcceptedCommands(const ConcreteClusterPath & cluster,
ReadOnlyBufferBuilder<DataModel::AcceptedCommandEntry> & builder)
{
using namespace Commands;

ReturnErrorOnFailure(builder.EnsureAppendCapacity(8)); // Ensure we have capacity for all possible commands

if (HasFeature(Feature::kPowerAdjustment))
{
for (auto && cmd : {
PowerAdjustRequest::Id,
CancelPowerAdjustRequest::Id,
})
{
VerifyOrExit(callback(cmd, context) == Loop::Continue, /**/);
}
ReturnErrorOnFailure(
builder.AppendElements({ PowerAdjustRequest::kMetadataEntry, CancelPowerAdjustRequest::kMetadataEntry }));
}

if (HasFeature(Feature::kStartTimeAdjustment))
{
VerifyOrExit(callback(StartTimeAdjustRequest::Id, context) == Loop::Continue, /**/);
ReturnErrorOnFailure(builder.Append(StartTimeAdjustRequest::kMetadataEntry));
}

if (HasFeature(Feature::kPausable))
{
VerifyOrExit(callback(PauseRequest::Id, context) == Loop::Continue, /**/);
VerifyOrExit(callback(ResumeRequest::Id, context) == Loop::Continue, /**/);
ReturnErrorOnFailure(builder.AppendElements({ PauseRequest::kMetadataEntry, ResumeRequest::kMetadataEntry }));
}

if (HasFeature(Feature::kForecastAdjustment))
{
VerifyOrExit(callback(ModifyForecastRequest::Id, context) == Loop::Continue, /**/);
ReturnErrorOnFailure(builder.Append(ModifyForecastRequest::kMetadataEntry));
}

if (HasFeature(Feature::kConstraintBasedAdjustment))
{
VerifyOrExit(callback(RequestConstraintBasedForecast::Id, context) == Loop::Continue, /**/);
ReturnErrorOnFailure(builder.Append(RequestConstraintBasedForecast::kMetadataEntry));
}

if (HasFeature(Feature::kStartTimeAdjustment) || HasFeature(Feature::kForecastAdjustment) ||
HasFeature(Feature::kConstraintBasedAdjustment))
{
VerifyOrExit(callback(CancelRequest::Id, context) == Loop::Continue, /**/);
ReturnErrorOnFailure(builder.Append(CancelRequest::kMetadataEntry));
}

exit:
return CHIP_NO_ERROR;
}

Expand Down
4 changes: 3 additions & 1 deletion src/app/clusters/device-energy-management-server/device-energy-management-server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include <app/reporting/reporting.h>
#include <app/util/attribute-storage.h>
#include <lib/core/CHIPError.h>
#include <lib/support/ReadOnlyBuffer.h>
#include <protocols/interaction_model/StatusCode.h>

namespace chip {
Expand Down Expand Up @@ -235,7 +236,8 @@ class Instance : public AttributeAccessInterface, public CommandHandlerInterface

// CommandHandlerInterface
void InvokeCommand(HandlerContext & handlerContext) override;
CHIP_ERROR EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) override;
CHIP_ERROR EnumerateAcceptedCommands(const ConcreteClusterPath & cluster,
ReadOnlyBufferBuilder<DataModel::AcceptedCommandEntry> & builder) override;

Protocols::InteractionModel::Status CheckOptOutAllowsRequest(AdjustmentCauseEnum adjustmentCause);
void HandlePowerAdjustRequest(HandlerContext & ctx, const Commands::PowerAdjustRequest::DecodableType & commandData);
Expand Down
29 changes: 9 additions & 20 deletions src/app/clusters/energy-evse-server/energy-evse-server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <app/ConcreteAttributePath.h>
#include <app/InteractionModelEngine.h>
#include <app/util/attribute-storage.h>
#include <clusters/EnergyEvse/Metadata.h>

using namespace chip;
using namespace chip::app;
Expand Down Expand Up @@ -182,41 +183,29 @@ CHIP_ERROR Instance::Write(const ConcreteDataAttributePath & aPath, AttributeVal
}

// CommandHandlerInterface
CHIP_ERROR Instance::EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context)
CHIP_ERROR Instance::EnumerateAcceptedCommands(const ConcreteClusterPath & cluster,
ReadOnlyBufferBuilder<DataModel::AcceptedCommandEntry> & builder)
{
using namespace Commands;
ReturnErrorOnFailure(builder.EnsureAppendCapacity(7));

for (auto && cmd : {
Disable::Id,
EnableCharging::Id,
})
{
VerifyOrExit(callback(cmd, context) == Loop::Continue, /**/);
}
ReturnErrorOnFailure(builder.AppendElements({ Disable::kMetadataEntry, EnableCharging::kMetadataEntry }));

if (HasFeature(Feature::kV2x))
{
VerifyOrExit(callback(EnableDischarging::Id, context) == Loop::Continue, /**/);
ReturnErrorOnFailure(builder.Append(EnableDischarging::kMetadataEntry));
}

if (HasFeature(Feature::kChargingPreferences))
{
for (auto && cmd : {
SetTargets::Id,
GetTargets::Id,
ClearTargets::Id,
})
{
VerifyOrExit(callback(cmd, context) == Loop::Continue, /**/);
}
ReturnErrorOnFailure(
builder.AppendElements({ SetTargets::kMetadataEntry, GetTargets::kMetadataEntry, ClearTargets::kMetadataEntry }));
}

if (SupportsOptCmd(OptionalCommands::kSupportsStartDiagnostics))
{
callback(StartDiagnostics::Id, context);
ReturnErrorOnFailure(builder.Append(StartDiagnostics::kMetadataEntry));
}

exit:
return CHIP_NO_ERROR;
}

Expand Down
3 changes: 2 additions & 1 deletion src/app/clusters/energy-evse-server/energy-evse-server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,8 @@ class Instance : public AttributeAccessInterface, public CommandHandlerInterface

// CommandHandlerInterface
void InvokeCommand(HandlerContext & handlerContext) override;
CHIP_ERROR EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) override;
CHIP_ERROR EnumerateAcceptedCommands(const ConcreteClusterPath & cluster,
ReadOnlyBufferBuilder<DataModel::AcceptedCommandEntry> & builder) override;

void HandleDisable(HandlerContext & ctx, const Commands::Disable::DecodableType & commandData);
void HandleEnableCharging(HandlerContext & ctx, const Commands::EnableCharging::DecodableType & commandData);
Expand Down
41 changes: 22 additions & 19 deletions src/app/clusters/network-commissioning/network-commissioning.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include <app/reporting/reporting.h>
#include <app/server/Server.h>
#include <app/util/attribute-storage.h>
#include <clusters/NetworkCommissioning/Metadata.h>
#include <credentials/CHIPCert.h>
#include <lib/core/CHIPConfig.h>
#include <lib/support/SafeInt.h>
Expand Down Expand Up @@ -341,23 +342,25 @@ Instance::NetworkInstanceList Instance::sInstances;
#endif

Instance::Instance(EndpointId aEndpointId, WiFiDriver * apDelegate) :
CommandHandlerInterface(Optional<EndpointId>(aEndpointId), Id), AttributeAccessInterface(Optional<EndpointId>(aEndpointId), Id),
mEndpointId(aEndpointId), mFeatureFlags(WiFiFeatures(apDelegate)), mpWirelessDriver(apDelegate), mpBaseDriver(apDelegate)
CommandHandlerInterfaceShim(Optional<EndpointId>(aEndpointId), Id),
AttributeAccessInterface(Optional<EndpointId>(aEndpointId), Id), mEndpointId(aEndpointId),
mFeatureFlags(WiFiFeatures(apDelegate)), mpWirelessDriver(apDelegate), mpBaseDriver(apDelegate)
{
mpDriver.Set<WiFiDriver *>(apDelegate);
}

Instance::Instance(EndpointId aEndpointId, ThreadDriver * apDelegate) :
CommandHandlerInterface(Optional<EndpointId>(aEndpointId), Id), AttributeAccessInterface(Optional<EndpointId>(aEndpointId), Id),
mEndpointId(aEndpointId), mFeatureFlags(Feature::kThreadNetworkInterface), mpWirelessDriver(apDelegate),
mpBaseDriver(apDelegate)
CommandHandlerInterfaceShim(Optional<EndpointId>(aEndpointId), Id),
AttributeAccessInterface(Optional<EndpointId>(aEndpointId), Id), mEndpointId(aEndpointId),
mFeatureFlags(Feature::kThreadNetworkInterface), mpWirelessDriver(apDelegate), mpBaseDriver(apDelegate)
{
mpDriver.Set<ThreadDriver *>(apDelegate);
}

Instance::Instance(EndpointId aEndpointId, EthernetDriver * apDelegate) :
CommandHandlerInterface(Optional<EndpointId>(aEndpointId), Id), AttributeAccessInterface(Optional<EndpointId>(aEndpointId), Id),
mEndpointId(aEndpointId), mFeatureFlags(Feature::kEthernetNetworkInterface), mpWirelessDriver(nullptr), mpBaseDriver(apDelegate)
CommandHandlerInterfaceShim(Optional<EndpointId>(aEndpointId), Id),
AttributeAccessInterface(Optional<EndpointId>(aEndpointId), Id), mEndpointId(aEndpointId),
mFeatureFlags(Feature::kEthernetNetworkInterface), mpWirelessDriver(nullptr), mpBaseDriver(apDelegate)
{}

CHIP_ERROR Instance::Init()
Expand Down Expand Up @@ -1360,18 +1363,18 @@ void Instance::OnFailSafeTimerExpired()
}
}

CHIP_ERROR Instance::EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context)
CHIP_ERROR Instance::EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandEntryCallback callback, void * context)
{
using namespace Clusters::NetworkCommissioning::Commands;

if (mFeatureFlags.Has(Feature::kThreadNetworkInterface))
{
for (auto && cmd : {
ScanNetworks::Id,
AddOrUpdateThreadNetwork::Id,
RemoveNetwork::Id,
ConnectNetwork::Id,
ReorderNetwork::Id,
ScanNetworks::kMetadataEntry,
AddOrUpdateThreadNetwork::kMetadataEntry,
RemoveNetwork::kMetadataEntry,
ConnectNetwork::kMetadataEntry,
ReorderNetwork::kMetadataEntry,
})
{
VerifyOrExit(callback(cmd, context) == Loop::Continue, /**/);
Expand All @@ -1380,11 +1383,11 @@ CHIP_ERROR Instance::EnumerateAcceptedCommands(const ConcreteClusterPath & clust
else if (mFeatureFlags.Has(Feature::kWiFiNetworkInterface))
{
for (auto && cmd : {
ScanNetworks::Id,
AddOrUpdateWiFiNetwork::Id,
RemoveNetwork::Id,
ConnectNetwork::Id,
ReorderNetwork::Id,
ScanNetworks::kMetadataEntry,
AddOrUpdateWiFiNetwork::kMetadataEntry,
RemoveNetwork::kMetadataEntry,
ConnectNetwork::kMetadataEntry,
ReorderNetwork::kMetadataEntry,
})
{
VerifyOrExit(callback(cmd, context) == Loop::Continue, /**/);
Expand All @@ -1393,7 +1396,7 @@ CHIP_ERROR Instance::EnumerateAcceptedCommands(const ConcreteClusterPath & clust

if (mFeatureFlags.Has(Feature::kPerDeviceCredentials))
{
VerifyOrExit(callback(QueryIdentity::Id, context) == Loop::Continue, /**/);
VerifyOrExit(callback(QueryIdentity::kMetadataEntry, context) == Loop::Continue, /**/);
}

exit:
Expand Down
Loading
Loading