From 48435ae5e271362f6270e3dbb7407945f0d58ee8 Mon Sep 17 00:00:00 2001 From: Lukas Zeller Date: Wed, 26 Jul 2023 19:40:35 +0200 Subject: [PATCH 01/14] [attribute storage] add automatic attribute storage for dynamic endpoints Dynamically defined endpoints can now have their own block of storage (to be provided by the caller of emberAfSetDynamicEndpoint()). If no storage is set for a dynamic endpoint, it behaves exactly as before, which is treating all attributes as "external", regardless of the respective bit in the attribute's metadata. With a storage provided, attributes behave the same way as static endpoint's do, that is, only those marked "external" need to be handled programmatically in the respective callbacks, other attributes' storage is automatic. --- src/app/util/af-types.h | 14 +++++- src/app/util/attribute-storage.cpp | 75 +++++++++++++++++++++++++----- src/app/util/attribute-storage.h | 7 ++- 3 files changed, 83 insertions(+), 13 deletions(-) diff --git a/src/app/util/af-types.h b/src/app/util/af-types.h index 04275a2515c23c..1284068ad24fd7 100644 --- a/src/app/util/af-types.h +++ b/src/app/util/af-types.h @@ -24,6 +24,8 @@ */ #include "att-storage.h" +#include // For CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT + #include // For bool #include // For various uint*_t types @@ -180,7 +182,7 @@ typedef struct */ uint8_t clusterCount; /** - * Size of all non-external, non-singlet attribute in this endpoint type. + * Size of all non-external, non-singleton attributes in this endpoint type. */ uint16_t endpointSize; } EmberAfEndpointType; @@ -221,6 +223,16 @@ struct EmberAfDefinedEndpoint */ chip::DataVersion * dataVersions = nullptr; +#if CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT > 0 + /** + * Pointer to the memory block to be used for automatic attribute storage if + * this is a dynamic endpoint. If set, the memory block pointed at + * must have a size equal to endpointType->endpointSize (which is + * the sum of all endpointType->clusters[*].clustersize). + */ + uint8_t * dynamicAttributeStorage = nullptr; +#endif + /** * Root endpoint id for composed device type. */ diff --git a/src/app/util/attribute-storage.cpp b/src/app/util/attribute-storage.cpp index 4d9cc2368f6900..664bc0aa9eb104 100644 --- a/src/app/util/attribute-storage.cpp +++ b/src/app/util/attribute-storage.cpp @@ -268,7 +268,7 @@ uint16_t emberAfGetDynamicIndexFromEndpoint(EndpointId id) CHIP_ERROR emberAfSetDynamicEndpoint(uint16_t index, EndpointId id, const EmberAfEndpointType * ep, const Span & dataVersionStorage, Span deviceTypeList, - EndpointId parentEndpointId) + EndpointId parentEndpointId, uint8_t * dynamicAttributeStorage) { auto realIndex = index + FIXED_ENDPOINT_COUNT; @@ -303,6 +303,9 @@ CHIP_ERROR emberAfSetDynamicEndpoint(uint16_t index, EndpointId id, const EmberA // Start the endpoint off as disabled. emAfEndpoints[index].bitmask.Clear(EmberAfEndpointOptions::isEnabled); emAfEndpoints[index].parentEndpointId = parentEndpointId; +#if CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT > 0 + emAfEndpoints[index].dynamicAttributeStorage = dynamicAttributeStorage; +#endif emberAfSetDynamicEndpointCount(MAX_ENDPOINT_COUNT - FIXED_ENDPOINT_COUNT); @@ -317,6 +320,18 @@ CHIP_ERROR emberAfSetDynamicEndpoint(uint16_t index, EndpointId id, const EmberA } } +#if CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT > 0 + if (dynamicAttributeStorage != nullptr && ep->endpointSize > 0) + { + // Flag the endpoint as enabled here, because otherwise loading attributes cannot work + emAfEndpoints[index].bitmask.Set(EmberAfEndpointOptions::isEnabled); + // Load attributes from NVstorage or set defaults + emberAfInitializeAttributes(id); + // set disabled again, so enabling below will detect a transition + emAfEndpoints[index].bitmask.Clear(EmberAfEndpointOptions::isEnabled); + } +#endif + // Now enable the endpoint. emberAfEndpointEnableDisable(id, true); @@ -580,7 +595,10 @@ Status emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord, { assertChipStackLockedByCurrentThread(); - uint16_t attributeOffsetIndex = 0; + // offset relative to the storage block, which is: + // - for static endpoints: attributeData[] global + // - for dynamic endpoints: dynamicAttributeStorage (unless nullPtr, then all attributes must be external) + uint16_t attributeStorageOffset = 0; for (uint16_t ep = 0; ep < emberAfEndpointCount(); ep++) { @@ -588,13 +606,31 @@ Status emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord, bool isDynamicEndpoint = (ep >= emberAfFixedEndpointCount()); if (emAfEndpoints[ep].endpoint == attRecord->endpoint) - { + { // Got the endpoint const EmberAfEndpointType * endpointType = emAfEndpoints[ep].endpointType; uint8_t clusterIndex; if (!emberAfEndpointIndexIsEnabled(ep)) { + // TODO: I think this is wrong, and should be a break + // It does not harm because usually no other endpointindex will contain + // an endpoint with the same ID, but it would cause catastrophic mess in + // attribute data because attributeOffsetIndex does not get incremented - + // endpoint enabling/disabling is dynamic, so enabled/disabled state + // MUST NOT change the data layout! continue; } + +#if CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT > 0 + bool hasDynamicAttributeStorage = emAfEndpoints[ep].dynamicAttributeStorage != nullptr; + if (hasDynamicAttributeStorage) + { + // endpoint storage is not in the static global `attributeData`, but offset + // from the per-endpoint dynamicAttributeStorage pointer. + // Endpoint processing starts here, so reset the offset. + attributeStorageOffset = 0; + } +#endif + for (clusterIndex = 0; clusterIndex < endpointType->clusterCount; clusterIndex++) { const EmberAfCluster * cluster = &(endpointType->cluster[clusterIndex]); @@ -613,9 +649,20 @@ Status emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord, } { + // attribute storage can be + // - singleton: statically allocated in singletonAttributeData global + // - static endpoint: statically allocated in attributeData global + // - dynamic endpoint with dynamic storage: in memory block provided at endpoint instantiation uint8_t * attributeLocation = - (am->mask & ATTRIBUTE_MASK_SINGLETON ? singletonAttributeLocation(am) - : attributeData + attributeOffsetIndex); + (am->mask & ATTRIBUTE_MASK_SINGLETON + ? singletonAttributeLocation(am) +#if CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT > 0 + : (hasDynamicAttributeStorage ? emAfEndpoints[ep].dynamicAttributeStorage : attributeData) +#else + : attributeData +#endif + + attributeStorageOffset); + uint8_t *src, *dst; if (write) { @@ -631,6 +678,7 @@ Status emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord, { if (buffer == nullptr) { + // only getting metadata return Status::Success; } @@ -653,7 +701,12 @@ Status emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord, } // Internal storage is only supported for fixed endpoints - if (!isDynamicEndpoint) + // and dynamic ones with dynamicAttributeStorage assigned. + if (!isDynamicEndpoint +#if CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT > 0 + || hasDynamicAttributeStorage +#endif + ) { return typeSensitiveMemCopy(attRecord->clusterId, dst, src, am, write, readLength); } @@ -666,7 +719,7 @@ Status emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord, // Increase the index if attribute is not externally stored if (!(am->mask & ATTRIBUTE_MASK_EXTERNAL_STORAGE) && !(am->mask & ATTRIBUTE_MASK_SINGLETON)) { - attributeOffsetIndex = static_cast(attributeOffsetIndex + emberAfAttributeSize(am)); + attributeStorageOffset = static_cast(attributeStorageOffset + emberAfAttributeSize(am)); } } } @@ -676,7 +729,7 @@ Status emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord, } // Not the cluster we are looking for - attributeOffsetIndex = static_cast(attributeOffsetIndex + cluster->clusterSize); + attributeStorageOffset = static_cast(attributeStorageOffset + cluster->clusterSize); } // Cluster is not in the endpoint. @@ -684,10 +737,10 @@ Status emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord, } // Not the endpoint we are looking for - // Dynamic endpoints are external and don't factor into storage size + // Dynamic endpoints are external and don't factor into statically allocated global storage size if (!isDynamicEndpoint) { - attributeOffsetIndex = static_cast(attributeOffsetIndex + emAfEndpoints[ep].endpointType->endpointSize); + attributeStorageOffset = static_cast(attributeStorageOffset + emAfEndpoints[ep].endpointType->endpointSize); } } return Status::UnsupportedEndpoint; // Sorry, endpoint was not found. @@ -1153,7 +1206,7 @@ void emAfLoadAttributeDefaults(EndpointId endpoint, Optional clusterI uint8_t attrData[ATTRIBUTE_LARGEST]; auto * attrStorage = 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. + // to have none if none of our attributes have NVM storage. for (ep = 0; ep < epCount; ep++) { diff --git a/src/app/util/attribute-storage.h b/src/app/util/attribute-storage.h index 711f6a7cff768d..2e19fcc1368c44 100644 --- a/src/app/util/attribute-storage.h +++ b/src/app/util/attribute-storage.h @@ -251,6 +251,10 @@ void emberAfEndpointConfigure(); // // An optional parent endpoint id should be passed for child endpoints of composed device. // +// An optional dynamicAttributeStorage can be passed to allow automatic attribute storage. +// This must point to a memory block of ep->endpointSize bytes size. If provided, the memory +// needs to remain allocated until this dynamic endpoint is cleared. +// // Returns CHIP_NO_ERROR No error. // CHIP_ERROR_NO_MEMORY MAX_ENDPOINT_COUNT is reached or when no storage is left for clusters // CHIP_ERROR_INVALID_ARGUMENT The EndpointId value passed is kInvalidEndpointId @@ -259,7 +263,8 @@ void emberAfEndpointConfigure(); CHIP_ERROR emberAfSetDynamicEndpoint(uint16_t index, chip::EndpointId id, const EmberAfEndpointType * ep, const chip::Span & dataVersionStorage, chip::Span deviceTypeList = {}, - chip::EndpointId parentEndpointId = chip::kInvalidEndpointId); + chip::EndpointId parentEndpointId = chip::kInvalidEndpointId, + uint8_t * dynamicAttributeStorage = nullptr); chip::EndpointId emberAfClearDynamicEndpoint(uint16_t index); uint16_t emberAfGetDynamicIndexFromEndpoint(chip::EndpointId id); /** From 61beb318c6d105ca14f81186b27c98a6e122252f Mon Sep 17 00:00:00 2001 From: Lukas Zeller Date: Tue, 23 Apr 2024 18:15:58 +0200 Subject: [PATCH 02/14] [attribute storage] allow setting up dynamic endpoints from templates Instead of re-creating dynamic endpoint's cluster declarations using the DECLARE_DYNAMIC_xxx macros, the new `setupDynamicEndpointDeclaration()` function allows setting up a dynamic endpoint by using clusters defined in another endpoint as templates. Usually that is a disabled endpoint created in ZAP with all possibly dynamically used clusters assigned. In combination with dynamic endpoint attribute storage, this greatly simplifies implementing dynamic endpoints and allows configuring them using the ZAP tool to ensure specs conformance, much the same way as with statically defined endpoints. --- src/app/util/attribute-storage.cpp | 34 ++++++++++++++++++++++++++++++ src/app/util/attribute-storage.h | 12 +++++++++++ 2 files changed, 46 insertions(+) diff --git a/src/app/util/attribute-storage.cpp b/src/app/util/attribute-storage.cpp index 664bc0aa9eb104..99ba78d7ffece7 100644 --- a/src/app/util/attribute-storage.cpp +++ b/src/app/util/attribute-storage.cpp @@ -266,6 +266,40 @@ uint16_t emberAfGetDynamicIndexFromEndpoint(EndpointId id) return kEmberInvalidEndpointIndex; } +const EmberAfCluster * getClusterTypeDefinition(EndpointId endpointId, ClusterId clusterId, EmberAfClusterMask mask) +{ + uint16_t index = emberAfIndexFromEndpointIncludingDisabledEndpoints(endpointId); + if (index != kEmberInvalidEndpointIndex) + { + return emberAfFindClusterInType(emAfEndpoints[index].endpointType, clusterId, mask, nullptr); + } + // not found + return nullptr; +} + +CHIP_ERROR setupDynamicEndpointDeclaration(EmberAfEndpointType & endpointType, EndpointId templateEndpointId, + const Span & templateClusterIds) +{ + // allocate cluster list + endpointType.clusterCount = static_cast(templateClusterIds.size()); + endpointType.cluster = new EmberAfCluster[endpointType.clusterCount]; + endpointType.endpointSize = 0; + // get the actual cluster pointers and sum up memory size + for (size_t i = 0; i < templateClusterIds.size(); i++) + { + auto cluster = getClusterTypeDefinition(templateEndpointId, templateClusterIds.data()[i], 0); + VerifyOrDieWithMsg(cluster, Support, "cluster 0x%04x template in endpoint %u does not exist", + (unsigned int) templateClusterIds.data()[i], (unsigned int) templateEndpointId); + // for now, we need to copy the cluster definition, unfortunately. + // TODO: make endpointType use a pointer to a list of EmberAfCluster* instead, so we can re-use cluster definitions + // instead of duplicating them here once for every instance. + memcpy((void *) &endpointType.cluster[i], cluster, sizeof(EmberAfCluster)); + // sum up the needed storage + endpointType.endpointSize = (uint16_t) (endpointType.endpointSize + cluster->clusterSize); + } + return CHIP_NO_ERROR; +} + CHIP_ERROR emberAfSetDynamicEndpoint(uint16_t index, EndpointId id, const EmberAfEndpointType * ep, const Span & dataVersionStorage, Span deviceTypeList, EndpointId parentEndpointId, uint8_t * dynamicAttributeStorage) diff --git a/src/app/util/attribute-storage.h b/src/app/util/attribute-storage.h index 2e19fcc1368c44..cf01e845b5f587 100644 --- a/src/app/util/attribute-storage.h +++ b/src/app/util/attribute-storage.h @@ -235,6 +235,18 @@ const EmberAfCluster * emberAfFindClusterInType(const EmberAfEndpointType * endp // Initial configuration void emberAfEndpointConfigure(); +// setup a dynamic endpoint's EmberAfEndpointType from a list of template clusters. +// +// This is a alternative to declaring dynamic endpoint metadata using DECLARE_DYNAMIC_* macros. +// +// As clusters to be used in dynamic endpoint setup need to be defined in ZAP anyway +// (usually on a special endpoint which remains always disabled), the cluster's +// metadata including all attributes already exists and can be re-used this way, +// without error prone manual duplicating with DECLARE_DYNAMIC_* +// +CHIP_ERROR setupDynamicEndpointDeclaration(EmberAfEndpointType & endpointType, chip::EndpointId templateEndpointId, + const chip::Span & templateClusterIds); + // Register a dynamic endpoint. This involves registering descriptors that describe // the composition of the endpoint (encapsulated in the 'ep' argument) as well as providing // storage for data versions. From 3a318042d467a8502c24bd7938a258a7a326f309 Mon Sep 17 00:00:00 2001 From: Lukas Zeller Date: Mon, 14 Oct 2024 15:12:54 +0200 Subject: [PATCH 03/14] [attribute storage] apply suggestions from PR review --- src/app/util/attribute-storage.cpp | 29 ++++++++++++++++++++++++----- src/app/util/attribute-storage.h | 26 ++++++++++++++++++++++++-- 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/src/app/util/attribute-storage.cpp b/src/app/util/attribute-storage.cpp index 99ba78d7ffece7..6c03fa1ab6dfaf 100644 --- a/src/app/util/attribute-storage.cpp +++ b/src/app/util/attribute-storage.cpp @@ -266,6 +266,8 @@ uint16_t emberAfGetDynamicIndexFromEndpoint(EndpointId id) return kEmberInvalidEndpointIndex; } +namespace { + const EmberAfCluster * getClusterTypeDefinition(EndpointId endpointId, ClusterId clusterId, EmberAfClusterMask mask) { uint16_t index = emberAfIndexFromEndpointIncludingDisabledEndpoints(endpointId); @@ -277,27 +279,44 @@ const EmberAfCluster * getClusterTypeDefinition(EndpointId endpointId, ClusterId return nullptr; } -CHIP_ERROR setupDynamicEndpointDeclaration(EmberAfEndpointType & endpointType, EndpointId templateEndpointId, - const Span & templateClusterIds) +} // anonymous namespace + +void emberAfSetupDynamicEndpointDeclaration(EmberAfEndpointType & endpointType, EndpointId templateEndpointId, + const Span & templateClusterIds) { // allocate cluster list - endpointType.clusterCount = static_cast(templateClusterIds.size()); + size_t clusterCount = templateClusterIds.size(); + VerifyOrDieWithMsg(clusterCount <= 255, Support, "max 255 clusters per endpoint!"); + endpointType.clusterCount = static_cast(clusterCount); endpointType.cluster = new EmberAfCluster[endpointType.clusterCount]; endpointType.endpointSize = 0; // get the actual cluster pointers and sum up memory size for (size_t i = 0; i < templateClusterIds.size(); i++) { - auto cluster = getClusterTypeDefinition(templateEndpointId, templateClusterIds.data()[i], 0); + auto cluster = getClusterTypeDefinition(templateEndpointId, templateClusterIds[i], 0); // 0 = not filtered by a mask VerifyOrDieWithMsg(cluster, Support, "cluster 0x%04x template in endpoint %u does not exist", (unsigned int) templateClusterIds.data()[i], (unsigned int) templateEndpointId); // for now, we need to copy the cluster definition, unfortunately. // TODO: make endpointType use a pointer to a list of EmberAfCluster* instead, so we can re-use cluster definitions // instead of duplicating them here once for every instance. + // Note: we cannot struct assignment here, because cluster in EmberAfEndpointType is const due + // to the way static cluster code generation works. So we have to resort to memcpy as a workaround + // until the much more intrusive changes as suggested by the TODO above can be applied. memcpy((void *) &endpointType.cluster[i], cluster, sizeof(EmberAfCluster)); // sum up the needed storage endpointType.endpointSize = (uint16_t) (endpointType.endpointSize + cluster->clusterSize); } - return CHIP_NO_ERROR; +} + +void emberAfResetDynamicEndpointDeclaration(EmberAfEndpointType & endpointType) +{ + if (endpointType.cluster) + { + delete[] endpointType.cluster; + endpointType.cluster = nullptr; + } + endpointType.clusterCount = 0; + endpointType.endpointSize = 0; } CHIP_ERROR emberAfSetDynamicEndpoint(uint16_t index, EndpointId id, const EmberAfEndpointType * ep, diff --git a/src/app/util/attribute-storage.h b/src/app/util/attribute-storage.h index cf01e845b5f587..7cbe184382ed69 100644 --- a/src/app/util/attribute-storage.h +++ b/src/app/util/attribute-storage.h @@ -244,8 +244,30 @@ void emberAfEndpointConfigure(); // metadata including all attributes already exists and can be re-used this way, // without error prone manual duplicating with DECLARE_DYNAMIC_* // -CHIP_ERROR setupDynamicEndpointDeclaration(EmberAfEndpointType & endpointType, chip::EndpointId templateEndpointId, - const chip::Span & templateClusterIds); +// templateEndpointId specifies a endpoint which is usually disabled, but containing +// cluster definitions that should be used for instantiating active endpoints. +// +// templateClusterIds is a list of clusterIds to be used for the new endpoint. +// +// endpointType will be setup with the specified clusters and their storage size so +// it can be used in a subsequent call to emberAfSetDynamicEndpoint instead of +// an endpoint manually constructed with DECLARE_DYNAMIC_*. +// +// Note: passing invalid templateEndpointId/templateClusterIds combinations, i.e. clusters +// not present in the specified template endpoint, will cause the function will die with +// an error message as this indicates severe internal inconsistency of the setup. +// +// Note: function may allocate memory for the endpoint declaration. +// Use emberAfResetEndpointDeclaration to properly dispose of an endpoint declaration. +void emberAfSetupDynamicEndpointDeclaration(EmberAfEndpointType & endpointType, chip::EndpointId templateEndpointId, + const chip::Span & templateClusterIds); + +// reset an endpoint declaration that was setup with emberAfSetupDynamicEndpointDeclaration +// to free all extra memory that might have been allocated. +// +// Warning: passing endpoint declarations that are not set up with +// emberAfSetupDynamicEndpointDeclaration is NOT allowed and likely causes undefined crashes. +void emberAfResetDynamicEndpointDeclaration(EmberAfEndpointType & endpointType); // Register a dynamic endpoint. This involves registering descriptors that describe // the composition of the endpoint (encapsulated in the 'ep' argument) as well as providing From 28377bb63001549e23c2f18f858e5d261605b315 Mon Sep 17 00:00:00 2001 From: Lukas Zeller Date: Mon, 14 Oct 2024 15:40:59 +0200 Subject: [PATCH 04/14] [attribute storage] const_casted with operator= instead of memcpy --- src/app/util/attribute-storage.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/util/attribute-storage.cpp b/src/app/util/attribute-storage.cpp index 6c03fa1ab6dfaf..6d4a40b46e4440 100644 --- a/src/app/util/attribute-storage.cpp +++ b/src/app/util/attribute-storage.cpp @@ -302,7 +302,7 @@ void emberAfSetupDynamicEndpointDeclaration(EmberAfEndpointType & endpointType, // Note: we cannot struct assignment here, because cluster in EmberAfEndpointType is const due // to the way static cluster code generation works. So we have to resort to memcpy as a workaround // until the much more intrusive changes as suggested by the TODO above can be applied. - memcpy((void *) &endpointType.cluster[i], cluster, sizeof(EmberAfCluster)); + const_cast(endpointType.cluster[i]) = *cluster; // sum up the needed storage endpointType.endpointSize = (uint16_t) (endpointType.endpointSize + cluster->clusterSize); } From f214311910bbc668c320481c2af65a56627ad3bd Mon Sep 17 00:00:00 2001 From: Lukas Zeller Date: Fri, 18 Oct 2024 19:14:39 +0200 Subject: [PATCH 05/14] [attribute storage] template cluster specification now includes mask... ...to allow differentiating server and client sides of the same cluster type --- src/app/util/af-types.h | 9 +++++++++ src/app/util/attribute-storage.cpp | 21 +++++++++++---------- src/app/util/attribute-storage.h | 5 +++-- 3 files changed, 23 insertions(+), 12 deletions(-) diff --git a/src/app/util/af-types.h b/src/app/util/af-types.h index 1284068ad24fd7..8b20c0ab251339 100644 --- a/src/app/util/af-types.h +++ b/src/app/util/af-types.h @@ -50,6 +50,15 @@ */ typedef uint8_t EmberAfClusterMask; +/** + * @brief Type for specifiying cluster including mask, to differentiate server & client + */ +typedef struct +{ + chip::ClusterId clusterId; + EmberAfClusterMask mask; +} EmberAfClusterSpec; + /** * @brief Generic function type, used for either of the cluster function. * diff --git a/src/app/util/attribute-storage.cpp b/src/app/util/attribute-storage.cpp index 6d4a40b46e4440..d075567624cfd5 100644 --- a/src/app/util/attribute-storage.cpp +++ b/src/app/util/attribute-storage.cpp @@ -282,27 +282,28 @@ const EmberAfCluster * getClusterTypeDefinition(EndpointId endpointId, ClusterId } // anonymous namespace void emberAfSetupDynamicEndpointDeclaration(EmberAfEndpointType & endpointType, EndpointId templateEndpointId, - const Span & templateClusterIds) + const Span & templateClusterSpecs) { // allocate cluster list - size_t clusterCount = templateClusterIds.size(); + size_t clusterCount = templateClusterSpecs.size(); VerifyOrDieWithMsg(clusterCount <= 255, Support, "max 255 clusters per endpoint!"); endpointType.clusterCount = static_cast(clusterCount); endpointType.cluster = new EmberAfCluster[endpointType.clusterCount]; endpointType.endpointSize = 0; // get the actual cluster pointers and sum up memory size - for (size_t i = 0; i < templateClusterIds.size(); i++) + for (size_t i = 0; i < templateClusterSpecs.size(); i++) { - auto cluster = getClusterTypeDefinition(templateEndpointId, templateClusterIds[i], 0); // 0 = not filtered by a mask - VerifyOrDieWithMsg(cluster, Support, "cluster 0x%04x template in endpoint %u does not exist", - (unsigned int) templateClusterIds.data()[i], (unsigned int) templateEndpointId); + auto cluster = + getClusterTypeDefinition(templateEndpointId, templateClusterSpecs[i].clusterId, templateClusterSpecs[i].mask); + VerifyOrDieWithMsg(cluster, Support, "cluster 0x%04x with mask %x could not be found in template endpoint %u", + (unsigned int) templateClusterSpecs[i].clusterId, templateClusterSpecs[i].mask, + (unsigned int) templateEndpointId); // for now, we need to copy the cluster definition, unfortunately. // TODO: make endpointType use a pointer to a list of EmberAfCluster* instead, so we can re-use cluster definitions // instead of duplicating them here once for every instance. - // Note: we cannot struct assignment here, because cluster in EmberAfEndpointType is const due - // to the way static cluster code generation works. So we have to resort to memcpy as a workaround - // until the much more intrusive changes as suggested by the TODO above can be applied. - const_cast(endpointType.cluster[i]) = *cluster; + // Note: we need const cast here, because cluster in EmberAfEndpointType is const due + // to the way static cluster code generation works. + const_cast(endpointType.cluster[i]) = *cluster; // sum up the needed storage endpointType.endpointSize = (uint16_t) (endpointType.endpointSize + cluster->clusterSize); } diff --git a/src/app/util/attribute-storage.h b/src/app/util/attribute-storage.h index 7cbe184382ed69..ae2b53343a2a05 100644 --- a/src/app/util/attribute-storage.h +++ b/src/app/util/attribute-storage.h @@ -247,7 +247,8 @@ void emberAfEndpointConfigure(); // templateEndpointId specifies a endpoint which is usually disabled, but containing // cluster definitions that should be used for instantiating active endpoints. // -// templateClusterIds is a list of clusterIds to be used for the new endpoint. +// templateClusterSpecs is a list of clusterId/mask to specify the clusters to +// be used from the template for the new endpoint. // // endpointType will be setup with the specified clusters and their storage size so // it can be used in a subsequent call to emberAfSetDynamicEndpoint instead of @@ -260,7 +261,7 @@ void emberAfEndpointConfigure(); // Note: function may allocate memory for the endpoint declaration. // Use emberAfResetEndpointDeclaration to properly dispose of an endpoint declaration. void emberAfSetupDynamicEndpointDeclaration(EmberAfEndpointType & endpointType, chip::EndpointId templateEndpointId, - const chip::Span & templateClusterIds); + const chip::Span & templateClusterSpecs); // reset an endpoint declaration that was setup with emberAfSetupDynamicEndpointDeclaration // to free all extra memory that might have been allocated. From 0f3074adcedf70f9b028bcc94f2219f718327f95 Mon Sep 17 00:00:00 2001 From: Lukas Zeller Date: Fri, 18 Oct 2024 19:19:00 +0200 Subject: [PATCH 06/14] [attribute storage] dynamic attribute storage is now passed as Span... to allow checking that it has sufficient size --- src/app/util/af-types.h | 9 +++++---- src/app/util/attribute-storage.cpp | 15 +++++++++++---- src/app/util/attribute-storage.h | 6 +++--- 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/src/app/util/af-types.h b/src/app/util/af-types.h index 8b20c0ab251339..a7cd77ea67e8eb 100644 --- a/src/app/util/af-types.h +++ b/src/app/util/af-types.h @@ -234,12 +234,13 @@ struct EmberAfDefinedEndpoint #if CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT > 0 /** - * Pointer to the memory block to be used for automatic attribute storage if - * this is a dynamic endpoint. If set, the memory block pointed at - * must have a size equal to endpointType->endpointSize (which is + * Span describing a memory block to be used for automatic attribute storage if + * this is a dynamic endpoint. If not empty, the Span must have + * a size at least equal to endpointType->endpointSize (which is * the sum of all endpointType->clusters[*].clustersize). */ - uint8_t * dynamicAttributeStorage = nullptr; + chip::Span dynamicAttributeStorage; + #endif /** diff --git a/src/app/util/attribute-storage.cpp b/src/app/util/attribute-storage.cpp index d075567624cfd5..fd325130d7bd5f 100644 --- a/src/app/util/attribute-storage.cpp +++ b/src/app/util/attribute-storage.cpp @@ -321,8 +321,9 @@ void emberAfResetDynamicEndpointDeclaration(EmberAfEndpointType & endpointType) } CHIP_ERROR emberAfSetDynamicEndpoint(uint16_t index, EndpointId id, const EmberAfEndpointType * ep, - const Span & dataVersionStorage, Span deviceTypeList, - EndpointId parentEndpointId, uint8_t * dynamicAttributeStorage) + const chip::Span & dataVersionStorage, + chip::Span deviceTypeList, EndpointId parentEndpointId, + Span dynamicAttributeStorage) { auto realIndex = index + FIXED_ENDPOINT_COUNT; @@ -350,6 +351,12 @@ CHIP_ERROR emberAfSetDynamicEndpoint(uint16_t index, EndpointId id, const EmberA } } +#if CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT > 0 + if (!dynamicAttributeStorage.empty() && dynamicAttributeStorage.size()endpointSize) { + return CHIP_ERROR_NO_MEMORY; // not enough memory provided for dynamic attribute storage + } +#endif + emAfEndpoints[index].endpoint = id; emAfEndpoints[index].deviceTypeList = deviceTypeList; emAfEndpoints[index].endpointType = ep; @@ -375,7 +382,7 @@ CHIP_ERROR emberAfSetDynamicEndpoint(uint16_t index, EndpointId id, const EmberA } #if CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT > 0 - if (dynamicAttributeStorage != nullptr && ep->endpointSize > 0) + if (!dynamicAttributeStorage.empty() && ep->endpointSize > 0) { // Flag the endpoint as enabled here, because otherwise loading attributes cannot work emAfEndpoints[index].bitmask.Set(EmberAfEndpointOptions::isEnabled); @@ -675,7 +682,7 @@ Status emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord, } #if CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT > 0 - bool hasDynamicAttributeStorage = emAfEndpoints[ep].dynamicAttributeStorage != nullptr; + bool hasDynamicAttributeStorage = !emAfEndpoints[ep].dynamicAttributeStorage.empty(); if (hasDynamicAttributeStorage) { // endpoint storage is not in the static global `attributeData`, but offset diff --git a/src/app/util/attribute-storage.h b/src/app/util/attribute-storage.h index ae2b53343a2a05..4a1bd61a226115 100644 --- a/src/app/util/attribute-storage.h +++ b/src/app/util/attribute-storage.h @@ -286,8 +286,8 @@ void emberAfResetDynamicEndpointDeclaration(EmberAfEndpointType & endpointType); // // An optional parent endpoint id should be passed for child endpoints of composed device. // -// An optional dynamicAttributeStorage can be passed to allow automatic attribute storage. -// This must point to a memory block of ep->endpointSize bytes size. If provided, the memory +// An optional dynamicAttributeStorage Span can be passed to allow automatic attribute storage. +// This must describe a memory block of at least ep->endpointSize bytes size. If provided, the memory // needs to remain allocated until this dynamic endpoint is cleared. // // Returns CHIP_NO_ERROR No error. @@ -299,7 +299,7 @@ CHIP_ERROR emberAfSetDynamicEndpoint(uint16_t index, chip::EndpointId id, const const chip::Span & dataVersionStorage, chip::Span deviceTypeList = {}, chip::EndpointId parentEndpointId = chip::kInvalidEndpointId, - uint8_t * dynamicAttributeStorage = nullptr); + chip::Span dynamicAttributeStorage = chip::Span()); chip::EndpointId emberAfClearDynamicEndpoint(uint16_t index); uint16_t emberAfGetDynamicIndexFromEndpoint(chip::EndpointId id); /** From 8fef412615c594271417c2b95137241079d290eb Mon Sep 17 00:00:00 2001 From: Lukas Zeller Date: Fri, 18 Oct 2024 19:19:48 +0200 Subject: [PATCH 07/14] [attribute storage] apply suggestion from PR review more readable attributeLocation calculation, remove unneeded ugly C cast --- src/app/util/attribute-storage.cpp | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/src/app/util/attribute-storage.cpp b/src/app/util/attribute-storage.cpp index fd325130d7bd5f..511356a33faa74 100644 --- a/src/app/util/attribute-storage.cpp +++ b/src/app/util/attribute-storage.cpp @@ -305,7 +305,7 @@ void emberAfSetupDynamicEndpointDeclaration(EmberAfEndpointType & endpointType, // to the way static cluster code generation works. const_cast(endpointType.cluster[i]) = *cluster; // sum up the needed storage - endpointType.endpointSize = (uint16_t) (endpointType.endpointSize + cluster->clusterSize); + endpointType.endpointSize += cluster->clusterSize; } } @@ -352,7 +352,8 @@ CHIP_ERROR emberAfSetDynamicEndpoint(uint16_t index, EndpointId id, const EmberA } #if CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT > 0 - if (!dynamicAttributeStorage.empty() && dynamicAttributeStorage.size()endpointSize) { + if (!dynamicAttributeStorage.empty() && dynamicAttributeStorage.size() < ep->endpointSize) + { return CHIP_ERROR_NO_MEMORY; // not enough memory provided for dynamic attribute storage } #endif @@ -714,15 +715,21 @@ Status emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord, // - singleton: statically allocated in singletonAttributeData global // - static endpoint: statically allocated in attributeData global // - dynamic endpoint with dynamic storage: in memory block provided at endpoint instantiation - uint8_t * attributeLocation = - (am->mask & ATTRIBUTE_MASK_SINGLETON - ? singletonAttributeLocation(am) + uint8_t * attributeLocation; + if (am->mask & ATTRIBUTE_MASK_SINGLETON) + { + attributeLocation = singletonAttributeLocation(am); + } #if CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT > 0 - : (hasDynamicAttributeStorage ? emAfEndpoints[ep].dynamicAttributeStorage : attributeData) -#else - : attributeData + else if (hasDynamicAttributeStorage) + { + attributeLocation = emAfEndpoints[ep].dynamicAttributeStorage.data(); + } #endif - + attributeStorageOffset); + else + { + attributeLocation = attributeData; + } uint8_t *src, *dst; if (write) From b3110089f9f578909cdf6eb8dd6afbd0dd8972f0 Mon Sep 17 00:00:00 2001 From: Lukas Zeller Date: Fri, 25 Oct 2024 10:43:29 +0200 Subject: [PATCH 08/14] [attribute storage] BUILD.gn: apply suggestions from PR review - af-types needs platform/CHIPDeviceConfig.h for CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT - reflect this in BUILD.gn by adding the platform_config_header sourceset as dependency - remove unneeded comment lines in agreement with original author --- src/app/util/BUILD.gn | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/app/util/BUILD.gn b/src/app/util/BUILD.gn index 29b3bc9563c487..fa16b7e3d927c7 100644 --- a/src/app/util/BUILD.gn +++ b/src/app/util/BUILD.gn @@ -29,7 +29,6 @@ source_set("nullable-primitives") { public_configs = [ "${chip_root}/src:includes" ] } -# These headers/cpp only depend on core/common source_set("types") { sources = [ "att-storage.h", @@ -52,7 +51,6 @@ source_set("types") { public_configs = [ "${chip_root}/src:includes" ] } -# This source set also depends on data-model source_set("af-types") { sources = [ "af-types.h" ] deps = [ @@ -60,6 +58,7 @@ source_set("af-types") { "${chip_root}/src/app:paths", "${chip_root}/src/app/data-model", "${chip_root}/src/messaging", + "${chip_root}/src/platform:platform_config_header", "${chip_root}/src/protocols/interaction_model", ] } From f8257d75da72c2e0b4e34a41c4f71c7b731aba83 Mon Sep 17 00:00:00 2001 From: Lukas Zeller Date: Fri, 25 Oct 2024 19:24:07 +0200 Subject: [PATCH 09/14] [attribute storage] apply help and review suggestions - check passed in endpointType is explicitly "empty" (zeroed out) - use CanCastTo instead of literally testing against 255 - return appropriate errors - only return completely set-up or untouched endpointType - no const cast needed --- src/app/util/attribute-storage.cpp | 54 ++++++++++++++++++------------ src/app/util/attribute-storage.h | 12 ++++--- 2 files changed, 39 insertions(+), 27 deletions(-) diff --git a/src/app/util/attribute-storage.cpp b/src/app/util/attribute-storage.cpp index 511356a33faa74..7a3d6d746b9dce 100644 --- a/src/app/util/attribute-storage.cpp +++ b/src/app/util/attribute-storage.cpp @@ -30,6 +30,7 @@ #include #include #include +#include #include #include #include @@ -281,32 +282,45 @@ const EmberAfCluster * getClusterTypeDefinition(EndpointId endpointId, ClusterId } // anonymous namespace -void emberAfSetupDynamicEndpointDeclaration(EmberAfEndpointType & endpointType, EndpointId templateEndpointId, - const Span & templateClusterSpecs) +CHIP_ERROR emberAfSetupDynamicEndpointDeclaration(EmberAfEndpointType & endpointType, EndpointId templateEndpointId, + const Span & templateClusterSpecs) { - // allocate cluster list + // we want an explicitly empty endpoint to begin with, to make sure no already set-up endpoint is passed in + VerifyOrReturnError(endpointType.cluster == nullptr, CHIP_ERROR_INVALID_ARGUMENT); + VerifyOrReturnError(endpointType.endpointSize == 0, CHIP_ERROR_INVALID_ARGUMENT); + VerifyOrReturnError(endpointType.clusterCount == 0, CHIP_ERROR_INVALID_ARGUMENT); + // check cluster count fits target struct's clusterCount field size_t clusterCount = templateClusterSpecs.size(); - VerifyOrDieWithMsg(clusterCount <= 255, Support, "max 255 clusters per endpoint!"); - endpointType.clusterCount = static_cast(clusterCount); - endpointType.cluster = new EmberAfCluster[endpointType.clusterCount]; - endpointType.endpointSize = 0; + VerifyOrReturnError(CanCastTo(clusterCount), CHIP_ERROR_NO_MEMORY); + // allocate new cluster array + auto newClusters = new EmberAfCluster[clusterCount]; + VerifyOrReturnError(newClusters != nullptr, CHIP_ERROR_NO_MEMORY); + uint16_t endpointSize = 0; // get the actual cluster pointers and sum up memory size - for (size_t i = 0; i < templateClusterSpecs.size(); i++) + for (size_t i = 0; i < clusterCount; i++) { auto cluster = getClusterTypeDefinition(templateEndpointId, templateClusterSpecs[i].clusterId, templateClusterSpecs[i].mask); - VerifyOrDieWithMsg(cluster, Support, "cluster 0x%04x with mask %x could not be found in template endpoint %u", - (unsigned int) templateClusterSpecs[i].clusterId, templateClusterSpecs[i].mask, - (unsigned int) templateEndpointId); + if (!cluster) + { + delete[] newClusters; + ChipLogError(DataManagement, "cluster 0x%04x with mask %x could not be found in template endpoint %u", + (unsigned int) templateClusterSpecs[i].clusterId, templateClusterSpecs[i].mask, + (unsigned int) templateEndpointId); + return CHIP_ERROR_NOT_FOUND; + } // for now, we need to copy the cluster definition, unfortunately. // TODO: make endpointType use a pointer to a list of EmberAfCluster* instead, so we can re-use cluster definitions // instead of duplicating them here once for every instance. - // Note: we need const cast here, because cluster in EmberAfEndpointType is const due - // to the way static cluster code generation works. - const_cast(endpointType.cluster[i]) = *cluster; + newClusters[i] = *cluster; // sum up the needed storage - endpointType.endpointSize += cluster->clusterSize; + endpointSize += cluster->clusterSize; } + // set up dynamic endpoint + endpointType.clusterCount = static_cast(clusterCount); + endpointType.cluster = newClusters; + endpointType.endpointSize = endpointSize; + return CHIP_NO_ERROR; } void emberAfResetDynamicEndpointDeclaration(EmberAfEndpointType & endpointType) @@ -691,6 +705,8 @@ Status emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord, // Endpoint processing starts here, so reset the offset. attributeStorageOffset = 0; } +#else + constexpr bool hasDynamicAttributeStorage = false; #endif for (clusterIndex = 0; clusterIndex < endpointType->clusterCount; clusterIndex++) @@ -720,12 +736,10 @@ Status emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord, { attributeLocation = singletonAttributeLocation(am); } -#if CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT > 0 else if (hasDynamicAttributeStorage) { attributeLocation = emAfEndpoints[ep].dynamicAttributeStorage.data(); } -#endif else { attributeLocation = attributeData; @@ -770,11 +784,7 @@ Status emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord, // Internal storage is only supported for fixed endpoints // and dynamic ones with dynamicAttributeStorage assigned. - if (!isDynamicEndpoint -#if CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT > 0 - || hasDynamicAttributeStorage -#endif - ) + if (!isDynamicEndpoint || hasDynamicAttributeStorage) { return typeSensitiveMemCopy(attRecord->clusterId, dst, src, am, write, readLength); } diff --git a/src/app/util/attribute-storage.h b/src/app/util/attribute-storage.h index 4a1bd61a226115..8f3e738c94cb43 100644 --- a/src/app/util/attribute-storage.h +++ b/src/app/util/attribute-storage.h @@ -250,18 +250,20 @@ void emberAfEndpointConfigure(); // templateClusterSpecs is a list of clusterId/mask to specify the clusters to // be used from the template for the new endpoint. // +// endpointType must be passed in with all members zero (cluster, clusterCount, endpointSize) +// to express the endpoint has not yet been configured before. // endpointType will be setup with the specified clusters and their storage size so // it can be used in a subsequent call to emberAfSetDynamicEndpoint instead of // an endpoint manually constructed with DECLARE_DYNAMIC_*. // // Note: passing invalid templateEndpointId/templateClusterIds combinations, i.e. clusters -// not present in the specified template endpoint, will cause the function will die with -// an error message as this indicates severe internal inconsistency of the setup. +// not present in the specified template endpoint, will cause the function to +// return CHIP_ERROR_NOT_FOUND and endpointType unmodified. // // Note: function may allocate memory for the endpoint declaration. -// Use emberAfResetEndpointDeclaration to properly dispose of an endpoint declaration. -void emberAfSetupDynamicEndpointDeclaration(EmberAfEndpointType & endpointType, chip::EndpointId templateEndpointId, - const chip::Span & templateClusterSpecs); +// Use emberAfResetEndpointDeclaration to properly dispose of a dynamic endpoint declaration. +CHIP_ERROR emberAfSetupDynamicEndpointDeclaration(EmberAfEndpointType & endpointType, chip::EndpointId templateEndpointId, + const chip::Span & templateClusterSpecs); // reset an endpoint declaration that was setup with emberAfSetupDynamicEndpointDeclaration // to free all extra memory that might have been allocated. From 8279865146be7685be95089030026686842a07e7 Mon Sep 17 00:00:00 2001 From: Lukas Zeller Date: Fri, 25 Oct 2024 22:24:05 +0200 Subject: [PATCH 10/14] [attribute storage] revert attempt to use constexpr to skip compiling code Turns out that while this works well on current compilers, it does not with many embedded toolchains. - idea was that an if testing an always false constexpr bool should skip *compiling* its body. - we need that because that body refers to a struct field that is non-existing in the case when the if condition is constexpr false - note that we can't use a real >=C++17 constexpr if, because in the CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT>0 case, the condition cannot be a constexpr Bottom line: there seems no way around the uglier #ifdef around the `else if` clause. We can avoid an #ifdef in the middle of a condition expression. --- src/app/util/attribute-storage.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/app/util/attribute-storage.cpp b/src/app/util/attribute-storage.cpp index 7a3d6d746b9dce..a78116efb1cdeb 100644 --- a/src/app/util/attribute-storage.cpp +++ b/src/app/util/attribute-storage.cpp @@ -706,7 +706,7 @@ Status emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord, attributeStorageOffset = 0; } #else - constexpr bool hasDynamicAttributeStorage = false; + const bool hasDynamicAttributeStorage = false; #endif for (clusterIndex = 0; clusterIndex < endpointType->clusterCount; clusterIndex++) @@ -736,10 +736,12 @@ Status emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord, { attributeLocation = singletonAttributeLocation(am); } +#if CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT > 0 else if (hasDynamicAttributeStorage) { attributeLocation = emAfEndpoints[ep].dynamicAttributeStorage.data(); } +#endif else { attributeLocation = attributeData; From 1b99675ec495ec846c469d2a9f64150a95686a8d Mon Sep 17 00:00:00 2001 From: Lukas Zeller Date: Fri, 25 Oct 2024 22:53:45 +0200 Subject: [PATCH 11/14] [attribute storage] avoid problems with summing up size in restricted int size result on some platforms - took the chance to add safe error exit on excessive cumulative cluster storage size. - using `typeof()` and `CanCastTo` throughout to make code independent of struct field sizes. --- src/app/util/attribute-storage.cpp | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/app/util/attribute-storage.cpp b/src/app/util/attribute-storage.cpp index a78116efb1cdeb..859222b1d54e1e 100644 --- a/src/app/util/attribute-storage.cpp +++ b/src/app/util/attribute-storage.cpp @@ -295,7 +295,7 @@ CHIP_ERROR emberAfSetupDynamicEndpointDeclaration(EmberAfEndpointType & endpoint // allocate new cluster array auto newClusters = new EmberAfCluster[clusterCount]; VerifyOrReturnError(newClusters != nullptr, CHIP_ERROR_NO_MEMORY); - uint16_t endpointSize = 0; + size_t endpointSize = 0; // get the actual cluster pointers and sum up memory size for (size_t i = 0; i < clusterCount; i++) { @@ -313,13 +313,18 @@ CHIP_ERROR emberAfSetupDynamicEndpointDeclaration(EmberAfEndpointType & endpoint // TODO: make endpointType use a pointer to a list of EmberAfCluster* instead, so we can re-use cluster definitions // instead of duplicating them here once for every instance. newClusters[i] = *cluster; - // sum up the needed storage + // sum up the needed storage, result must endpointSize += cluster->clusterSize; + if (!CanCastTo(endpointSize)) + { + delete[] newClusters; + return CHIP_ERROR_NO_MEMORY; + } } // set up dynamic endpoint - endpointType.clusterCount = static_cast(clusterCount); + endpointType.clusterCount = static_cast(clusterCount); endpointType.cluster = newClusters; - endpointType.endpointSize = endpointSize; + endpointType.endpointSize = static_cast(endpointSize); return CHIP_NO_ERROR; } From 4f4a211a3e79d805eb252392bcc3108cd04e6dc1 Mon Sep 17 00:00:00 2001 From: Lukas Zeller Date: Mon, 28 Oct 2024 15:47:35 +0100 Subject: [PATCH 12/14] [attribute storage] apply comment clarifications from PR review Co-authored-by: Andrei Litvin --- src/app/util/attribute-storage.cpp | 2 +- src/app/util/attribute-storage.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/util/attribute-storage.cpp b/src/app/util/attribute-storage.cpp index 859222b1d54e1e..dc29f557d163a6 100644 --- a/src/app/util/attribute-storage.cpp +++ b/src/app/util/attribute-storage.cpp @@ -313,7 +313,7 @@ CHIP_ERROR emberAfSetupDynamicEndpointDeclaration(EmberAfEndpointType & endpoint // TODO: make endpointType use a pointer to a list of EmberAfCluster* instead, so we can re-use cluster definitions // instead of duplicating them here once for every instance. newClusters[i] = *cluster; - // sum up the needed storage, result must + // sum up the needed storage, result must fit into endpointSize member (which is smaller than size_t) endpointSize += cluster->clusterSize; if (!CanCastTo(endpointSize)) { diff --git a/src/app/util/attribute-storage.h b/src/app/util/attribute-storage.h index 8f3e738c94cb43..5206f6948c5c2e 100644 --- a/src/app/util/attribute-storage.h +++ b/src/app/util/attribute-storage.h @@ -260,7 +260,7 @@ void emberAfEndpointConfigure(); // not present in the specified template endpoint, will cause the function to // return CHIP_ERROR_NOT_FOUND and endpointType unmodified. // -// Note: function may allocate memory for the endpoint declaration. +// Note: function will allocate dynamic memory for the endpoint declaration. // Use emberAfResetEndpointDeclaration to properly dispose of a dynamic endpoint declaration. CHIP_ERROR emberAfSetupDynamicEndpointDeclaration(EmberAfEndpointType & endpointType, chip::EndpointId templateEndpointId, const chip::Span & templateClusterSpecs); From 265beea5b3712879c2b078ffe6033f1eff2bab9c Mon Sep 17 00:00:00 2001 From: Lukas Zeller Date: Thu, 31 Oct 2024 12:25:27 +0100 Subject: [PATCH 13/14] [attribute storage] use decltype (C++11) instead of typeof (C++23) --- src/app/util/attribute-storage.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/app/util/attribute-storage.cpp b/src/app/util/attribute-storage.cpp index dc29f557d163a6..c1c93e27f85d36 100644 --- a/src/app/util/attribute-storage.cpp +++ b/src/app/util/attribute-storage.cpp @@ -291,7 +291,7 @@ CHIP_ERROR emberAfSetupDynamicEndpointDeclaration(EmberAfEndpointType & endpoint VerifyOrReturnError(endpointType.clusterCount == 0, CHIP_ERROR_INVALID_ARGUMENT); // check cluster count fits target struct's clusterCount field size_t clusterCount = templateClusterSpecs.size(); - VerifyOrReturnError(CanCastTo(clusterCount), CHIP_ERROR_NO_MEMORY); + VerifyOrReturnError(CanCastTo(clusterCount), CHIP_ERROR_NO_MEMORY); // allocate new cluster array auto newClusters = new EmberAfCluster[clusterCount]; VerifyOrReturnError(newClusters != nullptr, CHIP_ERROR_NO_MEMORY); @@ -315,16 +315,16 @@ CHIP_ERROR emberAfSetupDynamicEndpointDeclaration(EmberAfEndpointType & endpoint newClusters[i] = *cluster; // sum up the needed storage, result must fit into endpointSize member (which is smaller than size_t) endpointSize += cluster->clusterSize; - if (!CanCastTo(endpointSize)) + if (!CanCastTo(endpointSize)) { delete[] newClusters; return CHIP_ERROR_NO_MEMORY; } } // set up dynamic endpoint - endpointType.clusterCount = static_cast(clusterCount); + endpointType.clusterCount = static_cast(clusterCount); endpointType.cluster = newClusters; - endpointType.endpointSize = static_cast(endpointSize); + endpointType.endpointSize = static_cast(endpointSize); return CHIP_NO_ERROR; } From 6cd2a7ef2d87a08a365226465a6b2cafb4fbaf03 Mon Sep 17 00:00:00 2001 From: Lukas Zeller Date: Fri, 22 Nov 2024 17:49:45 +0100 Subject: [PATCH 14/14] [attribute storage] fix bad bug introduced in refacoring during the PR review process In "[attribute storage] apply suggestion from PR review" the somewhat hard-to-read conditional expression was refactored to an if/else, but unfortunately left out adding the actual attribute offset. This ruined the attribute storage pretty badly (all attributes written to the same memory location), but still passed surprisingly many tests. --- src/app/util/attribute-storage.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/app/util/attribute-storage.cpp b/src/app/util/attribute-storage.cpp index c1c93e27f85d36..17829e0b28ddb9 100644 --- a/src/app/util/attribute-storage.cpp +++ b/src/app/util/attribute-storage.cpp @@ -732,7 +732,8 @@ Status emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord, } { - // attribute storage can be + // Determine the attribute storage base address + // Attribute storage can be // - singleton: statically allocated in singletonAttributeData global // - static endpoint: statically allocated in attributeData global // - dynamic endpoint with dynamic storage: in memory block provided at endpoint instantiation @@ -751,6 +752,8 @@ Status emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord, { attributeLocation = attributeData; } + // Apply the offset to the attribute + attributeLocation += attributeStorageOffset; uint8_t *src, *dst; if (write)