Fix mismatched expectations on index retrieval in Ember cluster lookup #12750
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
pullapprove
bot
requested review from
andy31415,
anush-apple,
balducci-apple,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chulspro,
Damian-Nordic,
electrocucaracha,
franck-apple,
hawk248,
jelderton,
jepenven-silabs,
jmartinez-silabs,
kpschoedel,
LuDuda,
mlepage-google,
msandstedt,
mspang,
pan-apple,
robszewczyk,
sagar-apple,
saurabhst,
selissia,
tcarmelveilleux,
tecimovic and
turon
pullapprove
bot
requested review from
vijs,
vivien-apple,
wbschiller,
woody-apple,
yufengwangca and
yunhanw-google
robszewczyk
approved these changes
Dec 8, 2021
|
I was puzzled why such a 'index-shifting-bug' would not have resulted in large, obvious failures since the data returned would have been for a different cluster than what was requested. This is especially true since in the large list of clusters in However, we have a variety of Read tests in both YAML and Python to cover this. So why didn't they break?
in AttributePathExpandIterator.cpp:125
Filed #12752 to at least handle some of those cases. |
|
Wild, thanks for digging into it! |
yunhanw-google
approved these changes
Dec 8, 2021
bzbarsky-apple
approved these changes
Dec 8, 2021
turon
approved these changes
Dec 8, 2021
|
PR #12750: Size comparison from e4f3246 to 6f42abd Increases (18 builds for esp32, k32w, linux, nrfconnect, p6, qpg, telink)
Full report (29 builds for esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
AttributePathExpandIteratorusesemberAfClusterIndexto look up the index of a cluster within a given endpoint. This in turn calls intoemberAfFindClusterInTypeWithMfgCodeto find that index. This returns an index that indexes into all clusters in that endpoint regardless of type.Later on however, it uses
emberAfGetNthClusterIdto retrieve the cluster ID 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
emberAfFindClusterInTypeWithMfgCodeto 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.