Fixes for an ideal change #38485

ratgr · 2025-04-18T23:44:43Z

This is one of 3 proposals I'm drafting for #37139

This is the "Ideal change", Ideal as the user only includes what it needs, and no extra generation will be needed

Advantages

No extra bloat, simple
Reduced size

Disadvantages

User must change multiple pieces manually
Simple, yet complex enough for disallowing automation
Previous change to just provide the metadata is very similar to this one

@@ -22,6 +22,7 @@
 #include <app/AttributeAccessInterface.h>
 #include <app/CommandHandlerInterface.h>
 #include <app/data-model/Nullable.h>
+#include <clusters/NetworkCommissioning/Metadata.h>
 #include <lib/support/IntrusiveList.h>
 #include <lib/support/ThreadOperationalDataset.h>
 #include <lib/support/Variant.h>

-class Instance : public CommandHandlerInterface,
+class Instance : public CommandHandlerInterfaceShim,
                  public AttributeAccessInterface,
 #if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
                  private InstanceListNode,
@@ -59,7 +60,8 @@ public:
 
     // CommandHandlerInterface
     void InvokeCommand(HandlerContext & ctx) override;
-    CHIP_ERROR EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) override;
+    CHIP_ERROR EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandEntryCallback callback,
+                                         void * context) override;
     CHIP_ERROR EnumerateGeneratedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) override;
 
     // AttributeAccessInterface

@@ -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>
@@ -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),
+    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),
+    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),
+    CommandHandlerInterfaceShim(Optional<EndpointId>(aEndpointId), Id), AttributeAccessInterface(Optional<EndpointId>(aEndpointId), Id)
    mEndpointId(aEndpointId), mFeatureFlags(Feature::kEthernetNetworkInterface), mpWirelessDriver(nullptr), mpBaseDriver(apDelegate)
 {}
 
 CHIP_ERROR Instance::Init()
@@ -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, /**/);
@@ -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, /**/);
@@ -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:

Mitigation

Not introducing the SHIM but keeping the callback based interface might keep it simple by one line, not sure worth the cost, this change reduces the binary size on most cases

Testing

all UT's still pass

github-actions · 2025-04-21T19:31:45Z

PR #38485: Size comparison from aa3d16f to af668aa

Full report (1 build for stm32)

platform	target	config	section	`aa3d16f`	`af668aa`	change	% change
stm32	light	STM32WB5MM-DK	FLASH	463016	463344	328	0.1
			RAM	141488	141488	0	0.0

github-actions · 2025-04-22T01:44:03Z

PR #38485: Size comparison from d1b46b0 to aeed6df

Full report (9 builds for telink)

platform	target	config	section	`d1b46b0`	`aeed6df`	change	% change
telink	light-app-ota-compress-lzma-factory-data	tl3218x	FLASH	777962	778148	186	0.0
			RAM	50120	50120	0	0.0
	light-app-ota-compress-lzma-shell-factory-data	tl3218x	FLASH	768224	768410	186	0.0
			RAM	40420	40420	0	0.0
	light-switch-app-ota-shell-factory-data	tl3218x_retention	FLASH	709710	709896	186	0.0
			RAM	37044	37044	0	0.0
	bridge-app	tl7218x	FLASH	696146	696332	186	0.0
			RAM	101860	101860	0	0.0
	light-app-ota-shell-factory-data	tl7218x	FLASH	784598	756870	-27728	-3.5
			RAM	109308	97540	-11768	-10.8
	light-switch-app-ota-compress-lzma-factory-data	tl7218x_retention	FLASH	705824	683900	-21924	-3.1
			RAM	62620	51588	-11032	-17.6
	lighting-app-ota-factory-data	tlsr9118bdk40d	FLASH	603726	603912	186	0.0
			RAM	148704	148704	0	0.0
	lighting-app-ota-rpc-factory-data-4mb	tlsr9518adk80d	FLASH	819212	791872	-27340	-3.3
			RAM	107568	96396	-11172	-10.4
	light-switch-app-ota-compress-lzma-shell-factory-data	tlsr9528a	FLASH	742006	712350	-29656	-4.0
			RAM	85808	73408	-12400	-14.5

github-actions · 2025-04-22T22:08:09Z

PR #38485: Size comparison from 7f94ac6 to 32f6703

Full report (14 builds for esp32, nrfconnect, telink)

platform	target	config	section	`7f94ac6`	`32f6703`	change	% change
esp32	all-clusters-app	c3devkit	DRAM	103408	103408	0	0.0
			FLASH	`1800048`	1800674	626	0.0
			IRAM	83846	83846	0	0.0
		m5stack	DRAM	121980	121980	0	0.0
			FLASH	1765610	`1766114`	504	0.0
			IRAM	117043	117043	0	0.0
nrfconnect	all-clusters-app	nrf52840dk_nrf52840	FLASH	917312	917924	612	0.1
			RAM	167469	167469	0	0.0
		nrf7002dk_nrf5340_cpuapp	FLASH	909780	910276	496	0.1
			RAM	145713	145713	0	0.0
	all-clusters-minimal-app	nrf52840dk_nrf52840	FLASH	853840	854176	336	0.0
			RAM	141223	141223	0	0.0
telink	bridge-app	tl7218x	FLASH	673028	673214	186	0.0
				696146	696332	186	0.0
			RAM	90712	90712	0	0.0
				101860	101860	0	0.0
	light-app-ota-compress-lzma-factory-data	tl3218x	FLASH	777962	778148	186	0.0
			RAM	50120	50120	0	0.0
	light-app-ota-compress-lzma-shell-factory-data	tl3218x	FLASH	768224	768410	186	0.0
			RAM	40420	40420	0	0.0
	light-app-ota-shell-factory-data	tl7218x	FLASH	756684	756870	186	0.0
				784598	784784	186	0.0
			RAM	97540	97540	0	0.0
				109308	109308	0	0.0
	light-switch-app-ota-compress-lzma-factory-data	tl7218x_retention	FLASH	683714	683900	186	0.0
				705824	706010	186	0.0
			RAM	51588	51588	0	0.0
				62620	62620	0	0.0
	light-switch-app-ota-compress-lzma-shell-factory-data	tlsr9528a	FLASH	712164	712350	186	0.0
				742006	742192	186	0.0
			RAM	73408	73408	0	0.0
				85808	85808	0	0.0
	light-switch-app-ota-shell-factory-data	tl3218x_retention	FLASH	709710	709896	186	0.0
			RAM	37044	37044	0	0.0
	lighting-app-ota-factory-data	tlsr9118bdk40d	FLASH	604188	604374	186	0.0
				603726	603912	186	0.0
			RAM	138640	138640	0	0.0
				148704	148704	0	0.0
	lighting-app-ota-rpc-factory-data-4mb	tlsr9518adk80d	FLASH	791682	791872	190	0.0
				819212	819402	190	0.0
			RAM	96396	96396	0	0.0
				107568	107568	0	0.0

andy31415 · 2025-04-23T15:40:05Z

src/app/CommandHandlerInterface.h

+public:
+    using CommandHandlerInterface::CommandHandlerInterface;
+    typedef Loop (*CommandIdCallback)(CommandId id, void * context);
+    typedef Loop (*CommandEntryCallback)(DataModel::AcceptedCommandEntry id, void * context);


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.

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

Fixes for an ideal change

ca35731

github-actions bot added lib controller app support labels Apr 18, 2025

ratgr linked an issue Apr 21, 2025 that may be closed by this pull request

Convert CommandHandlerInterface command listing to provide full data #37139

Open

ratgr and others added 3 commits April 21, 2025 16:44

Command handler interface fix

6159e80

Restyled by clang-format

a076ff2

removed generator on build.gn

4541017

ratgr self-assigned this Apr 21, 2025

ratgr added 2 commits April 21, 2025 17:12

append entries

7da47df

moree fixes

fb65598

ratgr force-pushed the command-handler-interface-mixin-ideal-change branch from e07e7f1 to fb65598 Compare April 21, 2025 17:30

ratgr added 2 commits April 21, 2025 18:16

Fixing network commissioning

eeeed99

network commisioning fixes

af668aa

Adding Metadata.h to clusters

4fc481c

ratgr force-pushed the command-handler-interface-mixin-ideal-change branch 4 times, most recently from dc8316a to 7432672 Compare April 22, 2025 00:04

Fixed dependencies?

aeed6df

ratgr force-pushed the command-handler-interface-mixin-ideal-change branch from 7432672 to aeed6df Compare April 22, 2025 00:52

More fixes

32f6703

ratgr force-pushed the command-handler-interface-mixin-ideal-change branch from a9a8454 to 32f6703 Compare April 22, 2025 21:19

andy31415 reviewed Apr 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fixes for an ideal change #38485

Fixes for an ideal change #38485

Uh oh!

ratgr commented Apr 18, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Apr 21, 2025

Uh oh!

github-actions bot commented Apr 22, 2025

Uh oh!

github-actions bot commented Apr 22, 2025 •

edited

Loading

Uh oh!

andy31415 Apr 23, 2025

Uh oh!

ratgr Apr 23, 2025

Uh oh!

Uh oh!

Fixes for an ideal change #38485

Are you sure you want to change the base?

Fixes for an ideal change #38485

Uh oh!

Conversation

ratgr commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Advantages

Disadvantages

Mitigation

Testing

Uh oh!

github-actions bot commented Apr 21, 2025

Uh oh!

github-actions bot commented Apr 22, 2025

Uh oh!

github-actions bot commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andy31415 Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

ratgr Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ratgr commented Apr 18, 2025 •

edited

Loading

github-actions bot commented Apr 22, 2025 •

edited

Loading