Skip to content

Commit

Permalink
Global Attribute ClusterRevision looks broken for clusters of bridged…
Browse files Browse the repository at this point in the history 
… 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
  • Loading branch information
@pjzander-signify @pull
pjzander-signify authored and pull[bot] committed Oct 19, 2021
1 parent 82a7dd6 commit eb084ed
Show file tree
Hide file tree
Showing 4 changed files with 119 additions and 38 deletions.
58 changes: 47 additions & 11 deletions examples/bridge-app/esp32/main/main.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,26 +75,26 @@ 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)
DECLARE_DYNAMIC_ATTRIBUTE(ZCL_DEVICE_LIST_ATTRIBUTE_ID, ARRAY, kDescriptorAttributeArraySize, 0), /* device list */
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)
Expand All @@ -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;
Expand Down Expand Up @@ -191,6 +199,10 @@ EmberAfStatus HandleReadBridgedDeviceBasicAttribute(Device * dev, chip::Attribut
{
ToZclCharString(buffer, dev->GetName(), static_cast<uint8_t>(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;
Expand All @@ -205,30 +217,54 @@ 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)
{
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;
}

Expand Down
47 changes: 38 additions & 9 deletions examples/bridge-app/linux/main.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,26 +99,26 @@ 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)
DECLARE_DYNAMIC_ATTRIBUTE(ZCL_DEVICE_LIST_ATTRIBUTE_ID, ARRAY, kDescriptorAttributeArraySize, 0), /* device list */
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)
Expand All @@ -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)
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -311,22 +327,35 @@ 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)
{
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,
Expand Down
48 changes: 32 additions & 16 deletions src/app/util/attribute-storage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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
Expand All @@ -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<uint16_t>(attributeOffsetIndex + emAfEndpoints[i].endpointType->endpointSize);
attributeOffsetIndex = static_cast<uint16_t>(attributeOffsetIndex + emAfEndpoints[ep].endpointType->endpointSize);
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/app/util/attribute-storage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) \
Expand Down

0 comments on commit eb084ed

Please sign in to comment.