Skip to content

Commit

Permalink
Fix mismatched expectations on index retrieval in Ember cluster lookup (
Browse files Browse the repository at this point in the history
#12750)

AttributePathExpandIterator uses emberAfClusterIndex to look up the
index of a cluster within a given endpoint. This in turn calls into
emberAfFindClusterInTypeWithMfgCode to find that index. This returns an
index that indexes into all clusters in that endpoint regardless of
type.

Later on however, it uses emberAfGetNthClusterId to retrieve the actual
cluster index for a given index. However, this function applies the
index as though it's indexing into a list of clusters that match the
masking criteria (i.e server or client). This results in it retrieving
the wrong cluster in cases where there is a mix of client and server
clusters on a given endpoint.

Since most places use this 'scoped index', this PR fixes
emberAfFindClusterInTypeWithMfgCode to return a scoped index instead.

Problem:

Marc noted that when reading the WifiDiagnostics cluster, that on the
server, the logs indicated a retrieval of a ClusterId 0x36, but the
subsequent logs in the reporting engine seemed to indicate it was
picking up ClusterId 0x37 oddly enough.

Testing:

Validated that the above bug no longer happens, as well as reading out
the entire device using wildcards in the REPL to ensure everything still
works.
  • Loading branch information
mrjerryjohns authored Dec 9, 2021
1 parent 1423df5 commit bba082b
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 12 deletions.
25 changes: 13 additions & 12 deletions src/app/util/attribute-storage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -643,32 +643,33 @@ EmberAfStatus emAfReadOrWriteAttribute(EmberAfAttributeSearchRecord * attRecord,
return EMBER_ZCL_STATUS_UNSUPPORTED_ATTRIBUTE; // Sorry, attribute was not found.
}

// Check if a cluster is implemented or not. If yes, the cluster is returned.
// If the cluster is not manufacturerSpecific [ClusterId < FC00] then
// manufacturerCode argument is ignored otherwise checked.
//
// mask = 0 -> find either client or server
// mask = CLUSTER_MASK_CLIENT -> find client
// mask = CLUSTER_MASK_SERVER -> find server
EmberAfCluster * emberAfFindClusterInTypeWithMfgCode(EmberAfEndpointType * endpointType, ClusterId clusterId,
EmberAfClusterMask mask, uint16_t manufacturerCode, uint8_t * index)
{
uint8_t i;
uint8_t scopedIndex = 0;

for (i = 0; i < endpointType->clusterCount; i++)
{
EmberAfCluster * cluster = &(endpointType->cluster[i]);
if (cluster->clusterId == clusterId &&
(mask == 0 || (mask == CLUSTER_MASK_CLIENT && emberAfClusterIsClient(cluster)) ||

if ((mask == 0 || (mask == CLUSTER_MASK_CLIENT && emberAfClusterIsClient(cluster)) ||
(mask == CLUSTER_MASK_SERVER && emberAfClusterIsServer(cluster))))
{
if (index)
if (cluster->clusterId == clusterId)
{
*index = i;
if (index)
{
*index = scopedIndex;
}

return cluster;
}

return cluster;
scopedIndex++;
}
}

return NULL;
}

Expand Down
11 changes: 11 additions & 0 deletions src/app/util/attribute-storage.h
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,17 @@ EmberAfStatus emAfReadOrWriteAttribute(EmberAfAttributeSearchRecord * attRecord,
bool emAfMatchCluster(EmberAfCluster * cluster, EmberAfAttributeSearchRecord * attRecord);
bool emAfMatchAttribute(EmberAfCluster * cluster, EmberAfAttributeMetadata * am, EmberAfAttributeSearchRecord * attRecord);

// Check if a cluster is implemented or not. If yes, the cluster is returned.
// If the cluster is not manufacturerSpecific [ClusterId < FC00] then
// manufacturerCode argument is ignored otherwise checked.
//
// mask = 0 -> find either client or server
// mask = CLUSTER_MASK_CLIENT -> find client
// mask = CLUSTER_MASK_SERVER -> find server
//
// If a pointer to an index is provided, it will be updated to point to the relative index of the cluster
// within the set of clusters that match the mask criteria.
//
EmberAfCluster * emberAfFindClusterInTypeWithMfgCode(EmberAfEndpointType * endpointType, chip::ClusterId clusterId,
EmberAfClusterMask mask, uint16_t manufacturerCode, uint8_t * index = nullptr);

Expand Down

0 comments on commit bba082b

Please sign in to comment.