From 103730319d18936837f7a2a351e0301d3f39d77c Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 18 Jan 2024 10:50:12 -0500 Subject: [PATCH] Move emAfLoadAttributeDefaults to private inside attribute-storage.cpp (#31427) * Move emAfLoadAttributeDefaults to private. Reduce the API surface for ember attribute storage: - remove unused ability to "load defaults without loading persistence" - make the full loadDefautls + cluster private for now as it is not used Looking to reduce the API surface of ember attribute storage to consider making it pluggable/modular (to allow for better unit testing support). * Add extra static to method * Replace a usage of emAfLoadAttributeDefaults * Update test cluster docs * Fix verifyordie logic * Remove one more unused (not even implemented) function * Fix macro usage * Move 2 more methods outside public API * Restyle * More strict const in readorwriteattribute --- .../all-clusters-app.matter | 3 +- .../all-clusters-minimal-app.matter | 3 +- .../test-cluster-server.cpp | 2 +- src/app/util/attribute-storage.cpp | 34 +++++++++---------- src/app/util/attribute-storage.h | 18 ++-------- .../zcl/data-model/chip/test-cluster.xml | 3 +- .../data_model/controller-clusters.matter | 3 +- .../CHIP/zap-generated/MTRBaseClusters.h | 3 +- 8 files changed, 30 insertions(+), 39 deletions(-) diff --git a/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter b/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter index 6a817823dd4667..1964ac899e9ff7 100644 --- a/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter +++ b/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter @@ -6742,7 +6742,8 @@ internal cluster UnitTesting = 4294048773 { int8u fillCharacter = 2; } - /** Simple command without any parameters and without a specific response */ + /** Simple command without any parameters and without a specific response. + To aid in unit testing, this command will re-initialize attribute storage to defaults. */ command Test(): DefaultSuccess = 0; /** Simple command without any parameters and without a specific response not handled by the server */ command TestNotHandled(): DefaultSuccess = 1; diff --git a/examples/all-clusters-minimal-app/all-clusters-common/all-clusters-minimal-app.matter b/examples/all-clusters-minimal-app/all-clusters-common/all-clusters-minimal-app.matter index dd01a2fb356f22..ee5f558431657e 100644 --- a/examples/all-clusters-minimal-app/all-clusters-common/all-clusters-minimal-app.matter +++ b/examples/all-clusters-minimal-app/all-clusters-common/all-clusters-minimal-app.matter @@ -5812,7 +5812,8 @@ internal cluster UnitTesting = 4294048773 { int8u fillCharacter = 2; } - /** Simple command without any parameters and without a specific response */ + /** Simple command without any parameters and without a specific response. + To aid in unit testing, this command will re-initialize attribute storage to defaults. */ command Test(): DefaultSuccess = 0; /** Simple command without any parameters and without a specific response not handled by the server */ command TestNotHandled(): DefaultSuccess = 1; diff --git a/src/app/clusters/test-cluster-server/test-cluster-server.cpp b/src/app/clusters/test-cluster-server/test-cluster-server.cpp index de41c286f15f2d..cfecf37ec14929 100644 --- a/src/app/clusters/test-cluster-server/test-cluster-server.cpp +++ b/src/app/clusters/test-cluster-server/test-cluster-server.cpp @@ -720,7 +720,7 @@ bool emberAfUnitTestingClusterTestCallback(app::CommandHandler * commandObj, con const Clusters::UnitTesting::Commands::Test::DecodableType & commandData) { // Setup the test variables - emAfLoadAttributeDefaults(commandPath.mEndpointId, true, MakeOptional(commandPath.mClusterId)); + emberAfInitializeAttributes(commandPath.mEndpointId); for (int i = 0; i < kAttributeListLength; ++i) { gListUint8Data[i] = 0; diff --git a/src/app/util/attribute-storage.cpp b/src/app/util/attribute-storage.cpp index ce37246c4b3d9f..67b15686e18282 100644 --- a/src/app/util/attribute-storage.cpp +++ b/src/app/util/attribute-storage.cpp @@ -56,6 +56,15 @@ EmberAfDefinedEndpoint emAfEndpoints[MAX_ENDPOINT_COUNT]; uint8_t attributeData[ACTUAL_ATTRIBUTE_SIZE]; +// ----- internal-only methods, not part of the external API ----- + +// Loads the attributes from built-in default and storage. +static void emAfLoadAttributeDefaults(chip::EndpointId endpoint, chip::Optional = chip::NullOptional); + +static bool emAfMatchCluster(const EmberAfCluster * cluster, const EmberAfAttributeSearchRecord * attRecord); +static bool emAfMatchAttribute(const EmberAfCluster * cluster, const EmberAfAttributeMetadata * am, + const EmberAfAttributeSearchRecord * attRecord); + namespace { #if (!defined(ATTRIBUTE_SINGLETONS_SIZE)) || (ATTRIBUTE_SINGLETONS_SIZE == 0) @@ -536,7 +545,7 @@ static EmberAfStatus typeSensitiveMemCopy(ClusterId clusterId, uint8_t * dest, u * 1. Cluster ids match AND * 2. Cluster is a server cluster (because there are no client attributes). */ -bool emAfMatchCluster(const EmberAfCluster * cluster, EmberAfAttributeSearchRecord * attRecord) +bool emAfMatchCluster(const EmberAfCluster * cluster, const EmberAfAttributeSearchRecord * attRecord) { return (cluster->clusterId == attRecord->clusterId && (cluster->mask & CLUSTER_MASK_SERVER)); } @@ -549,7 +558,7 @@ bool emAfMatchCluster(const EmberAfCluster * cluster, EmberAfAttributeSearchReco * Attributes match if attr ids match. */ bool emAfMatchAttribute(const EmberAfCluster * cluster, const EmberAfAttributeMetadata * am, - EmberAfAttributeSearchRecord * attRecord) + const EmberAfAttributeSearchRecord * attRecord) { return (am->attributeId == attRecord->attributeId); } @@ -567,7 +576,7 @@ bool emAfMatchAttribute(const EmberAfCluster * cluster, const EmberAfAttributeMe // type. For strings, the function will copy as many bytes as will fit in the // attribute. This means the resulting string may be truncated. The length // byte(s) in the resulting string will reflect any truncated. -EmberAfStatus emAfReadOrWriteAttribute(EmberAfAttributeSearchRecord * attRecord, const EmberAfAttributeMetadata ** metadata, +EmberAfStatus emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord, const EmberAfAttributeMetadata ** metadata, uint8_t * buffer, uint16_t readLength, bool write) { assertChipStackLockedByCurrentThread(); @@ -1187,15 +1196,10 @@ uint8_t emberAfGetClustersFromEndpoint(EndpointId endpoint, ClusterId * clusterL void emberAfInitializeAttributes(EndpointId endpoint) { - emAfLoadAttributeDefaults(endpoint, false); + emAfLoadAttributeDefaults(endpoint); } -void emberAfResetAttributes(EndpointId endpoint) -{ - emAfLoadAttributeDefaults(endpoint, true); -} - -void emAfLoadAttributeDefaults(EndpointId endpoint, bool ignoreStorage, Optional clusterId) +void emAfLoadAttributeDefaults(EndpointId endpoint, Optional clusterId) { uint16_t ep; uint8_t clusterI; @@ -1203,7 +1207,7 @@ void emAfLoadAttributeDefaults(EndpointId endpoint, bool ignoreStorage, Optional uint8_t * ptr; uint16_t epCount = emberAfEndpointCount(); uint8_t attrData[ATTRIBUTE_LARGEST]; - auto * attrStorage = ignoreStorage ? nullptr : app::GetAttributePersistenceProvider(); + auto * attrStorage = app::GetAttributePersistenceProvider(); // Don't check whether we actually have an attrStorage here, because it's OK // to have one if none of our attributes have NVM storage. @@ -1245,9 +1249,9 @@ void emAfLoadAttributeDefaults(EndpointId endpoint, bool ignoreStorage, Optional ptr = nullptr; // Will get set to the value to write, as needed. // First check for a persisted value. - if (!ignoreStorage && am->IsAutomaticallyPersisted()) + if (am->IsAutomaticallyPersisted()) { - VerifyOrDie(attrStorage && "Attribute persistence needs a persistence provider"); + VerifyOrDieWithMsg(attrStorage != nullptr, Zcl, "Attribute persistence needs a persistence provider"); MutableByteSpan bytes(attrData); CHIP_ERROR err = attrStorage->ReadValue( app::ConcreteAttributePath(de->endpoint, cluster->clusterId, am->attributeId), am, bytes); @@ -1328,10 +1332,6 @@ void emAfLoadAttributeDefaults(EndpointId endpoint, bool ignoreStorage, Optional ptr, 0, // buffer size - unused true); // write? - if (ignoreStorage) - { - emAfSaveAttributeToStorageIfNeeded(ptr, de->endpoint, record.clusterId, am); - } } } } diff --git a/src/app/util/attribute-storage.h b/src/app/util/attribute-storage.h index 184282a1e466c8..be965da42ee357 100644 --- a/src/app/util/attribute-storage.h +++ b/src/app/util/attribute-storage.h @@ -95,13 +95,9 @@ void emAfCallInits(void); // Initial configuration void emberAfEndpointConfigure(void); -EmberAfStatus emAfReadOrWriteAttribute(EmberAfAttributeSearchRecord * attRecord, const EmberAfAttributeMetadata ** metadata, +EmberAfStatus emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord, const EmberAfAttributeMetadata ** metadata, uint8_t * buffer, uint16_t readLength, bool write); -bool emAfMatchCluster(const EmberAfCluster * cluster, EmberAfAttributeSearchRecord * attRecord); -bool emAfMatchAttribute(const EmberAfCluster * cluster, const EmberAfAttributeMetadata * am, - EmberAfAttributeSearchRecord * attRecord); - // Check if a cluster is implemented or not. If yes, the cluster is returned. // // mask = 0 -> find either client or server @@ -156,14 +152,8 @@ const EmberAfCluster * emberAfFindClusterIncludingDisabledEndpoints(chip::Endpoi // cast it. EmberAfGenericClusterFunction emberAfFindClusterFunction(const EmberAfCluster * cluster, EmberAfClusterMask functionMask); -// Public APIs for loading attributes +// Loads attribute defaults and any non-volatile attributes stored void emberAfInitializeAttributes(chip::EndpointId endpoint); -void emberAfResetAttributes(chip::EndpointId endpoint); - -// Loads the attributes from built-in default and / or storage. If -// ignoreStorage is true, only defaults will be read, and the storage for -// non-volatile attributes will be overwritten with those defaults. -void emAfLoadAttributeDefaults(chip::EndpointId endpoint, bool ignoreStorage, chip::Optional = chip::NullOptional); // After the RAM value has changed, code should call this function. If this // attribute has been tagged as non-volatile, its value will be stored. @@ -177,10 +167,6 @@ void emAfClusterAttributeChangedCallback(const chip::app::ConcreteAttributePath EmberAfStatus emAfClusterPreAttributeChangedCallback(const chip::app::ConcreteAttributePath & attributePath, EmberAfAttributeType attributeType, uint16_t size, uint8_t * value); -// Checks a cluster mask byte against ticks passed bitmask -// returns true if the mask matches a passed interval -bool emberAfCheckTick(EmberAfClusterMask mask, uint8_t passedMask); - // Check whether there is an endpoint defined with the given endpoint id that is // enabled. bool emberAfEndpointIsEnabled(chip::EndpointId endpoint); diff --git a/src/app/zap-templates/zcl/data-model/chip/test-cluster.xml b/src/app/zap-templates/zcl/data-model/chip/test-cluster.xml index f043c49074ec0e..7fdc585a0c82e9 100644 --- a/src/app/zap-templates/zcl/data-model/chip/test-cluster.xml +++ b/src/app/zap-templates/zcl/data-model/chip/test-cluster.xml @@ -240,7 +240,8 @@ limitations under the License. - Simple command without any parameters and without a specific response + Simple command without any parameters and without a specific response. + To aid in unit testing, this command will re-initialize attribute storage to defaults. diff --git a/src/controller/data_model/controller-clusters.matter b/src/controller/data_model/controller-clusters.matter index bc8127ac45ba28..d1bd05a82d21d4 100644 --- a/src/controller/data_model/controller-clusters.matter +++ b/src/controller/data_model/controller-clusters.matter @@ -8929,7 +8929,8 @@ internal cluster UnitTesting = 4294048773 { int8u fillCharacter = 2; } - /** Simple command without any parameters and without a specific response */ + /** Simple command without any parameters and without a specific response. + To aid in unit testing, this command will re-initialize attribute storage to defaults. */ command Test(): DefaultSuccess = 0; /** Simple command without any parameters and without a specific response not handled by the server */ command TestNotHandled(): DefaultSuccess = 1; diff --git a/src/darwin/Framework/CHIP/zap-generated/MTRBaseClusters.h b/src/darwin/Framework/CHIP/zap-generated/MTRBaseClusters.h index a3c5fc32992c07..741bca21657ec4 100644 --- a/src/darwin/Framework/CHIP/zap-generated/MTRBaseClusters.h +++ b/src/darwin/Framework/CHIP/zap-generated/MTRBaseClusters.h @@ -14848,7 +14848,8 @@ MTR_AVAILABLE(ios(16.4), macos(13.3), watchos(9.4), tvos(16.4)) /** * Command Test * - * Simple command without any parameters and without a specific response + * Simple command without any parameters and without a specific response. + To aid in unit testing, this command will re-initialize attribute storage to defaults. */ - (void)testWithParams:(MTRUnitTestingClusterTestParams * _Nullable)params completion:(MTRStatusCompletion)completion MTR_AVAILABLE(ios(16.4), macos(13.3), watchos(9.4), tvos(16.4)); - (void)testWithCompletion:(MTRStatusCompletion)completion