From eb084ed6717bb5d4301e4754505f9ebb0d7263f2 Mon Sep 17 00:00:00 2001 From: Paul Zander <69502639+pjzander-signify@users.noreply.github.com> Date: Mon, 18 Oct 2021 19:17:57 +0200 Subject: [PATCH] Global Attribute ClusterRevision looks broken for clusters of bridged devices (#9823) (#10412) * Return an error when trying to read/write internal stored attributes when the endpoint is dynamic * Change dynamic endpoints so that the cluster revision is read externally --- examples/bridge-app/esp32/main/main.cpp | 58 ++++++++++++++++++++----- examples/bridge-app/linux/main.cpp | 47 ++++++++++++++++---- src/app/util/attribute-storage.cpp | 48 +++++++++++++------- src/app/util/attribute-storage.h | 4 +- 4 files changed, 119 insertions(+), 38 deletions(-) diff --git a/examples/bridge-app/esp32/main/main.cpp b/examples/bridge-app/esp32/main/main.cpp index 58a2f5e9c8c189..579b382441cd23 100644 --- a/examples/bridge-app/esp32/main/main.cpp +++ b/examples/bridge-app/esp32/main/main.cpp @@ -75,7 +75,7 @@ static uint8_t gDescriptorAttrStorage[DYNAMIC_ENDPOINT_COUNT][kDescriptorAttribu // Declare On/Off cluster attributes DECLARE_DYNAMIC_ATTRIBUTE_LIST_BEGIN(onOffAttrs) DECLARE_DYNAMIC_ATTRIBUTE(ZCL_ON_OFF_ATTRIBUTE_ID, BOOLEAN, 1, 0) /* on/off */ -DECLARE_DYNAMIC_ATTRIBUTE_LIST_END(0x0001); +DECLARE_DYNAMIC_ATTRIBUTE_LIST_END(); // Declare Descriptor cluster attributes DECLARE_DYNAMIC_ATTRIBUTE_LIST_BEGIN(descriptorAttrs) @@ -83,18 +83,18 @@ DECLARE_DYNAMIC_ATTRIBUTE(ZCL_DEVICE_LIST_ATTRIBUTE_ID, ARRAY, kDescriptorAttrib DECLARE_DYNAMIC_ATTRIBUTE(ZCL_SERVER_LIST_ATTRIBUTE_ID, ARRAY, kDescriptorAttributeArraySize, 0), /* server list */ DECLARE_DYNAMIC_ATTRIBUTE(ZCL_CLIENT_LIST_ATTRIBUTE_ID, ARRAY, kDescriptorAttributeArraySize, 0), /* client list */ DECLARE_DYNAMIC_ATTRIBUTE(ZCL_PARTS_LIST_ATTRIBUTE_ID, ARRAY, kDescriptorAttributeArraySize, 0) /* parts list */ - DECLARE_DYNAMIC_ATTRIBUTE_LIST_END(0x0001); + DECLARE_DYNAMIC_ATTRIBUTE_LIST_END(); // Declare Bridged Device Basic information cluster attributes DECLARE_DYNAMIC_ATTRIBUTE_LIST_BEGIN(bridgedDeviceBasicAttrs) DECLARE_DYNAMIC_ATTRIBUTE(ZCL_USER_LABEL_ATTRIBUTE_ID, CHAR_STRING, kUserLabelSize, 0), /* UserLabel */ DECLARE_DYNAMIC_ATTRIBUTE(ZCL_REACHABLE_ATTRIBUTE_ID, BOOLEAN, 1, 0) /* Reachable */ - DECLARE_DYNAMIC_ATTRIBUTE_LIST_END(0x0003); + DECLARE_DYNAMIC_ATTRIBUTE_LIST_END(); // Declare Fixed Label cluster attributes DECLARE_DYNAMIC_ATTRIBUTE_LIST_BEGIN(fixedLabelAttrs) DECLARE_DYNAMIC_ATTRIBUTE(ZCL_LABEL_LIST_ATTRIBUTE_ID, ARRAY, kFixedLabelAttributeArraySize, 0) /* label list */ -DECLARE_DYNAMIC_ATTRIBUTE_LIST_END(0x0001); +DECLARE_DYNAMIC_ATTRIBUTE_LIST_END(); // Declare Cluster List for Bridged Light endpoint DECLARE_DYNAMIC_CLUSTER_LIST_BEGIN(bridgedLightClusters) @@ -105,6 +105,14 @@ DECLARE_DYNAMIC_CLUSTER(ZCL_ON_OFF_CLUSTER_ID, onOffAttrs), DECLARE_DYNAMIC_CLUS // Declare Bridged Light endpoint DECLARE_DYNAMIC_ENDPOINT(bridgedLightEndpoint, bridgedLightClusters); +/* REVISION definitions: + */ + +#define ZCL_DESCRIPTOR_CLUSTER_REVISION (1u) +#define ZCL_BRIDGED_DEVICE_BASIC_CLUSTER_REVISION (1u) +#define ZCL_FIXED_LABEL_CLUSTER_REVISION (1u) +#define ZCL_ON_OFF_CLUSTER_REVISION (4u) + CHIP_ERROR AddDeviceEndpoint(Device * dev, EmberAfEndpointType * ep, uint16_t deviceType) { uint8_t index = 0; @@ -191,6 +199,10 @@ EmberAfStatus HandleReadBridgedDeviceBasicAttribute(Device * dev, chip::Attribut { ToZclCharString(buffer, dev->GetName(), static_cast(maxReadLength - 1)); } + else if ((attributeId == ZCL_CLUSTER_REVISION_SERVER_ATTRIBUTE_ID) && (maxReadLength == 2)) + { + *buffer = (uint16_t) ZCL_BRIDGED_DEVICE_BASIC_CLUSTER_REVISION; + } else { return EMBER_ZCL_STATUS_FAILURE; @@ -205,10 +217,17 @@ EmberAfStatus HandleReadDescriptorAttribute(uint16_t endpointIndex, chip::Attrib if ((maxReadLength <= kDescriptorAttributeArraySize) && (attributeId < kDescriptorAttributeCount)) { memcpy(buffer, &gDescriptorAttrStorage[endpointIndex][attributeId][0], maxReadLength); - return EMBER_ZCL_STATUS_SUCCESS; + } + else if ((attributeId == ZCL_CLUSTER_REVISION_SERVER_ATTRIBUTE_ID) && (maxReadLength == 2)) + { + *buffer = (uint16_t) ZCL_DESCRIPTOR_CLUSTER_REVISION; + } + else + { + return EMBER_ZCL_STATUS_FAILURE; } - return EMBER_ZCL_STATUS_FAILURE; + return EMBER_ZCL_STATUS_SUCCESS; } EmberAfStatus HandleReadFixedLabelAttribute(Device * dev, EmberAfAttributeMetadata * am, uint8_t * buffer, uint16_t maxReadLength) @@ -216,19 +235,36 @@ EmberAfStatus HandleReadFixedLabelAttribute(Device * dev, EmberAfAttributeMetada if ((am->attributeId == ZCL_LABEL_LIST_ATTRIBUTE_ID) && (maxReadLength <= kFixedLabelAttributeArraySize)) { EncodeFixedLabel("room", dev->GetLocation(), buffer, maxReadLength, am); - - return EMBER_ZCL_STATUS_SUCCESS; + } + else if ((am->attributeId == ZCL_CLUSTER_REVISION_SERVER_ATTRIBUTE_ID) && (maxReadLength == 2)) + { + *buffer = (uint16_t) ZCL_FIXED_LABEL_CLUSTER_REVISION; + } + else + { + return EMBER_ZCL_STATUS_FAILURE; } - return EMBER_ZCL_STATUS_FAILURE; + return EMBER_ZCL_STATUS_SUCCESS; } EmberAfStatus HandleReadOnOffAttribute(Device * dev, chip::AttributeId attributeId, uint8_t * buffer, uint16_t maxReadLength) { ChipLogProgress(DeviceLayer, "HandleReadOnOffAttribute: attrId=%d, maxReadLength=%d", attributeId, maxReadLength); - ReturnErrorCodeIf((attributeId != ZCL_ON_OFF_ATTRIBUTE_ID) || (maxReadLength != 1), EMBER_ZCL_STATUS_FAILURE); - *buffer = dev->IsOn() ? 1 : 0; + if ((attributeId == ZCL_ON_OFF_ATTRIBUTE_ID) && (maxReadLength == 1)) + { + *buffer = dev->IsOn() ? 1 : 0; + } + else if ((attributeId == ZCL_CLUSTER_REVISION_SERVER_ATTRIBUTE_ID) && (maxReadLength == 2)) + { + *buffer = (uint16_t) ZCL_ON_OFF_CLUSTER_REVISION; + } + else + { + return EMBER_ZCL_STATUS_FAILURE; + } + return EMBER_ZCL_STATUS_SUCCESS; } diff --git a/examples/bridge-app/linux/main.cpp b/examples/bridge-app/linux/main.cpp index e053008b1a0ced..3d4d34c0d4c6a9 100644 --- a/examples/bridge-app/linux/main.cpp +++ b/examples/bridge-app/linux/main.cpp @@ -99,7 +99,7 @@ static uint8_t gDescriptorAttrStorage[DYNAMIC_ENDPOINT_COUNT][kDescriptorAttribu // Declare On/Off cluster attributes DECLARE_DYNAMIC_ATTRIBUTE_LIST_BEGIN(onOffAttrs) DECLARE_DYNAMIC_ATTRIBUTE(ZCL_ON_OFF_ATTRIBUTE_ID, BOOLEAN, 1, 0) /* on/off */ -DECLARE_DYNAMIC_ATTRIBUTE_LIST_END(0x0001); +DECLARE_DYNAMIC_ATTRIBUTE_LIST_END(); // Declare Descriptor cluster attributes DECLARE_DYNAMIC_ATTRIBUTE_LIST_BEGIN(descriptorAttrs) @@ -107,18 +107,18 @@ DECLARE_DYNAMIC_ATTRIBUTE(ZCL_DEVICE_LIST_ATTRIBUTE_ID, ARRAY, kDescriptorAttrib DECLARE_DYNAMIC_ATTRIBUTE(ZCL_SERVER_LIST_ATTRIBUTE_ID, ARRAY, kDescriptorAttributeArraySize, 0), /* server list */ DECLARE_DYNAMIC_ATTRIBUTE(ZCL_CLIENT_LIST_ATTRIBUTE_ID, ARRAY, kDescriptorAttributeArraySize, 0), /* client list */ DECLARE_DYNAMIC_ATTRIBUTE(ZCL_PARTS_LIST_ATTRIBUTE_ID, ARRAY, kDescriptorAttributeArraySize, 0) /* parts list */ - DECLARE_DYNAMIC_ATTRIBUTE_LIST_END(0x0001); + DECLARE_DYNAMIC_ATTRIBUTE_LIST_END(); // Declare Bridged Device Basic information cluster attributes DECLARE_DYNAMIC_ATTRIBUTE_LIST_BEGIN(bridgedDeviceBasicAttrs) DECLARE_DYNAMIC_ATTRIBUTE(ZCL_USER_LABEL_ATTRIBUTE_ID, CHAR_STRING, kUserLabelSize, 0), /* UserLabel */ DECLARE_DYNAMIC_ATTRIBUTE(ZCL_REACHABLE_ATTRIBUTE_ID, BOOLEAN, 1, 0) /* Reachable */ - DECLARE_DYNAMIC_ATTRIBUTE_LIST_END(0x0003); + DECLARE_DYNAMIC_ATTRIBUTE_LIST_END(); // Declare Fixed Label cluster attributes DECLARE_DYNAMIC_ATTRIBUTE_LIST_BEGIN(fixedLabelAttrs) DECLARE_DYNAMIC_ATTRIBUTE(ZCL_LABEL_LIST_ATTRIBUTE_ID, ARRAY, kFixedLabelAttributeArraySize, 0) /* label list */ -DECLARE_DYNAMIC_ATTRIBUTE_LIST_END(0x0001); +DECLARE_DYNAMIC_ATTRIBUTE_LIST_END(); // Declare Cluster List for Bridged Light endpoint DECLARE_DYNAMIC_CLUSTER_LIST_BEGIN(bridgedLightClusters) @@ -129,6 +129,14 @@ DECLARE_DYNAMIC_CLUSTER(ZCL_ON_OFF_CLUSTER_ID, onOffAttrs), DECLARE_DYNAMIC_CLUS // Declare Bridged Light endpoint DECLARE_DYNAMIC_ENDPOINT(bridgedLightEndpoint, bridgedLightClusters); +// REVISION DEFINITIONS: +// ================================================================================= + +#define ZCL_DESCRIPTOR_CLUSTER_REVISION (1u) +#define ZCL_BRIDGED_DEVICE_BASIC_CLUSTER_REVISION (1u) +#define ZCL_FIXED_LABEL_CLUSTER_REVISION (1u) +#define ZCL_ON_OFF_CLUSTER_REVISION (4u) + // --------------------------------------------------------------------------- int AddDeviceEndpoint(Device * dev, EmberAfEndpointType * ep, uint16_t deviceType) @@ -258,6 +266,10 @@ EmberAfStatus HandleReadBridgedDeviceBasicAttribute(Device * dev, chip::Attribut MakeZclCharString(zclString, dev->GetName()); buffer = zclString.data(); } + else if ((attributeId == ZCL_CLUSTER_REVISION_SERVER_ATTRIBUTE_ID) && (maxReadLength == 2)) + { + *buffer = (uint16_t) ZCL_BRIDGED_DEVICE_BASIC_CLUSTER_REVISION; + } else { return EMBER_ZCL_STATUS_FAILURE; @@ -274,6 +286,10 @@ EmberAfStatus HandleReadOnOffAttribute(Device * dev, chip::AttributeId attribute { *buffer = dev->IsOn() ? 1 : 0; } + else if ((attributeId == ZCL_CLUSTER_REVISION_SERVER_ATTRIBUTE_ID) && (maxReadLength == 2)) + { + *buffer = (uint16_t) ZCL_ON_OFF_CLUSTER_REVISION; + } else { return EMBER_ZCL_STATUS_FAILURE; @@ -311,10 +327,17 @@ EmberAfStatus HandleReadDescriptorAttribute(uint16_t endpointIndex, chip::Attrib if ((maxReadLength <= kDescriptorAttributeArraySize) && (attributeId < kDescriptorAttributeCount)) { memcpy(buffer, &gDescriptorAttrStorage[endpointIndex][attributeId][0], maxReadLength); - return EMBER_ZCL_STATUS_SUCCESS; + } + else if ((attributeId == ZCL_CLUSTER_REVISION_SERVER_ATTRIBUTE_ID) && (maxReadLength == 2)) + { + *buffer = (uint16_t) ZCL_DESCRIPTOR_CLUSTER_REVISION; + } + else + { + return EMBER_ZCL_STATUS_FAILURE; } - return EMBER_ZCL_STATUS_FAILURE; + return EMBER_ZCL_STATUS_SUCCESS; } EmberAfStatus HandleReadFixedLabelAttribute(Device * dev, EmberAfAttributeMetadata * am, uint8_t * buffer, uint16_t maxReadLength) @@ -322,11 +345,17 @@ EmberAfStatus HandleReadFixedLabelAttribute(Device * dev, EmberAfAttributeMetada if ((am->attributeId == ZCL_LABEL_LIST_ATTRIBUTE_ID) && (maxReadLength <= kFixedLabelAttributeArraySize)) { EncodeFixedLabel("room", dev->GetLocation(), buffer, maxReadLength, am); - - return EMBER_ZCL_STATUS_SUCCESS; + } + else if ((am->attributeId == ZCL_CLUSTER_REVISION_SERVER_ATTRIBUTE_ID) && (maxReadLength == 2)) + { + *buffer = (uint16_t) ZCL_FIXED_LABEL_CLUSTER_REVISION; + } + else + { + return EMBER_ZCL_STATUS_FAILURE; } - return EMBER_ZCL_STATUS_FAILURE; + return EMBER_ZCL_STATUS_SUCCESS; } EmberAfStatus HandleWriteDescriptorAttribute(uint16_t endpointIndex, EmberAfAttributeMetadata * am, uint8_t * buffer, diff --git a/src/app/util/attribute-storage.cpp b/src/app/util/attribute-storage.cpp index 01695496ca8dc7..93b47c8b0d14e2 100644 --- a/src/app/util/attribute-storage.cpp +++ b/src/app/util/attribute-storage.cpp @@ -586,16 +586,18 @@ bool emAfMatchAttribute(EmberAfCluster * cluster, EmberAfAttributeMetadata * am, EmberAfStatus emAfReadOrWriteAttribute(EmberAfAttributeSearchRecord * attRecord, EmberAfAttributeMetadata ** metadata, uint8_t * buffer, uint16_t readLength, bool write, int32_t index) { - uint8_t i; uint16_t attributeOffsetIndex = 0; - for (i = 0; i < emberAfEndpointCount(); i++) + for (uint8_t ep = 0; ep < emberAfEndpointCount(); ep++) { - if (emAfEndpoints[i].endpoint == attRecord->endpoint) + // Is this a dynamic endpoint? + bool isDynamicEndpoint = (ep >= emberAfFixedEndpointCount()); + + if (emAfEndpoints[ep].endpoint == attRecord->endpoint) { - EmberAfEndpointType * endpointType = emAfEndpoints[i].endpointType; + EmberAfEndpointType * endpointType = emAfEndpoints[ep].endpointType; uint8_t clusterIndex; - if (!emberAfEndpointIndexIsEnabled(i)) + if (!emberAfEndpointIndexIsEnabled(ep)) { continue; } @@ -649,15 +651,29 @@ EmberAfStatus emAfReadOrWriteAttribute(EmberAfAttributeSearchRecord * attRecord, } } - return (am->mask & ATTRIBUTE_MASK_EXTERNAL_STORAGE - ? (write) ? emberAfExternalAttributeWriteCallback( - attRecord->endpoint, attRecord->clusterId, am, - emAfGetManufacturerCodeForAttribute(cluster, am), buffer, index) - : emberAfExternalAttributeReadCallback( - attRecord->endpoint, attRecord->clusterId, am, - emAfGetManufacturerCodeForAttribute(cluster, am), buffer, - emberAfAttributeSize(am), index) - : typeSensitiveMemCopy(attRecord->clusterId, dst, src, am, write, readLength, index)); + // Is the attribute externally stored? + if (am->mask & ATTRIBUTE_MASK_EXTERNAL_STORAGE) + { + return (write ? emberAfExternalAttributeWriteCallback( + attRecord->endpoint, attRecord->clusterId, am, + emAfGetManufacturerCodeForAttribute(cluster, am), buffer, index) + : emberAfExternalAttributeReadCallback( + attRecord->endpoint, attRecord->clusterId, am, + emAfGetManufacturerCodeForAttribute(cluster, am), buffer, + emberAfAttributeSize(am), index)); + } + else + { + // Internal storage is only supported for fixed endpoints + if (!isDynamicEndpoint) + { + return typeSensitiveMemCopy(attRecord->clusterId, dst, src, am, write, readLength, index); + } + else + { + return EMBER_ZCL_STATUS_FAILURE; + } + } } } else @@ -679,9 +695,9 @@ EmberAfStatus emAfReadOrWriteAttribute(EmberAfAttributeSearchRecord * attRecord, else { // Not the endpoint we are looking for // Dynamic endpoints are external and don't factor into storage size - if (i < emberAfFixedEndpointCount()) + if (!isDynamicEndpoint) { - attributeOffsetIndex = static_cast(attributeOffsetIndex + emAfEndpoints[i].endpointType->endpointSize); + attributeOffsetIndex = static_cast(attributeOffsetIndex + emAfEndpoints[ep].endpointType->endpointSize); } } } diff --git a/src/app/util/attribute-storage.h b/src/app/util/attribute-storage.h index 80b5083955718a..84a9f84ab06052 100644 --- a/src/app/util/attribute-storage.h +++ b/src/app/util/attribute-storage.h @@ -90,8 +90,8 @@ #define DECLARE_DYNAMIC_ATTRIBUTE_LIST_BEGIN(attrListName) EmberAfAttributeMetadata attrListName[] = { -#define DECLARE_DYNAMIC_ATTRIBUTE_LIST_END(clusterRevision) \ - , { 0xFFFD, ZAP_TYPE(INT16U), 2, 0, ZAP_SIMPLE_DEFAULT(clusterRevision) } /* cluster revision */ \ +#define DECLARE_DYNAMIC_ATTRIBUTE_LIST_END() \ + , { 0xFFFD, ZAP_TYPE(INT16U), 2, ZAP_ATTRIBUTE_MASK(EXTERNAL_STORAGE), ZAP_EMPTY_DEFAULT() } /* cluster revision */ \ } #define DECLARE_DYNAMIC_ATTRIBUTE(attId, attType, attSizeBytes, attrMask) \