Fix wildcard reads + chunking #12140
Merged
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
pullapprove
bot
requested review from
anush-apple,
austinh0,
balducci-apple,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chulspro,
Damian-Nordic,
electrocucaracha,
emargolis,
erjiaqing,
franck-apple,
harimau-qirex,
hawk248,
holbrookt,
jelderton,
jepenven-silabs,
jmartinez-silabs,
kghost,
LuDuda,
msandstedt,
mspang,
pan-apple,
sagar-apple,
saurabhst,
selissia,
tcarmelveilleux and
tecimovic
erjiaqing
reviewed
Nov 23, 2021
erjiaqing
reviewed
Nov 23, 2021
|
PR #12140: Size comparison from b83502b to 554cc03 Increases (31 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
|
Unfortunately, there are a bunch more bugs on the server side that the tests I've added to Python are un-earthing. So, will need to fix these before this PR is in a shape to merge. |
andy31415
approved these changes
Nov 25, 2021
|
fast track: change up for a while, created by a domain owner. Most changes are python-related which is not affecting main functionality. |
Using the Matter REPL, this PR fixes a number of critical bugs in both the server-side logic as well as the Python APIs to validate all permutations of wildcard reads as well as basic chunking. On the server side, it fixes: - The AttributeExpandPathIterator was incorrectly using emberAfClusterIndex to iterate over clusters on an endpoint, but that isn't its API contract. That function returns something else completely. Fixed that implementation as well as created a new, better named function called emberAfClusterIndexInMatchingEndpoints that actually better describes what it does. - In ReadSingleClusterData, we were chaining a number of MessageDef builder calls one after another and checking the error at the end (by calling GetError()). However, this resulted in the original error encountered in the first call in that chain being lost and replaced with something else by the time it got to the end. This was especially problematic during chunking since the errors CHIP_ERROR_BUFFER_TOO_SMALL and CHIP_ERROR_NO_MEMORY are what the higher level calls in Engine::BuildSingleReportDataAttributeReportIBs expect to correclty handle chunking. Since the error got lost and converted, it resulted in the Engine treating it like a critical failure and stopping the generation of reports entirely. On the client side in Python: - Reading Clusters.OnOff.Attributes.SampleMfgSpecificAttribute0x00000x1049 broke because in the Python code, we were generating attribute IDs that were 16-bit values instead of 32-bits. This was a combination of not generating them correclty, as well as the C++ bindings expecting a c_uint16 instead of c_uint32 - Fixed up ChipDeviceController.ReadAttribute to actually work correctly with wildcards instead of just erroring out due to parsing issues. Validation: - Tested all wildcard read variations using the Python ReadAttribute API in the REPL and ensured it completed successfully. - Added all variations as tests to the mobile device test suite.
erjiaqing
force-pushed
the
wildcard-reads
branch
from
554cc03 to
259cb84
Compare
erjiaqing
force-pushed
the
wildcard-reads
branch
from
259cb84 to
ebd102f
Compare
|
PR #12140: Size comparison from 659190d to ebd102f Increases (3 builds for qpg, telink)
Full report (4 builds for qpg, telink)
|
|
PR #12140: Size comparison from 659190d to b2690ef Increases (29 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
|
PR #12140: Size comparison from 659190d to 0711b2c Increases (22 builds for efr32, esp32, k32w, linux, mbed, p6, qpg, telink)
Full report (28 builds for efr32, esp32, k32w, linux, mbed, p6, qpg, telink)
|
|
PR #12140: Size comparison from 67c8f1d to de666a7 Increases (29 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
|
PR #12140: Size comparison from ab5734c to c0475e0 Increases (29 builds for efr32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Full report (36 builds for efr32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
|
PR #12140: Size comparison from ab5734c to f403549 Increases (31 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Full report (38 builds for efr32, 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.
Using the Matter REPL as a validation tool, this PR fixes a number of critical bugs in both the server-side logic as well as the Python APIs to validate all permutations of wildcard reads as well as basic chunking.
On the server side, it fixes:
The
AttributeExpandPathIteratorwas incorrectly usingemberAfClusterIndexto iterate over clusters on an endpoint, but that isn't its API contract. That function returns something else completely. Fixed that implementation as well as created a new, better named function calledemberAfClusterIndexInMatchingEndpointsthat actually better describes what it does.In
ReadSingleClusterData, we were chaining a number of MessageDef builder calls one after another and checking the error at the end (by callingGetError()). However, this resulted in the original error encountered in the first call in that chain being lost and replaced with something else by the time it got to the end. This was especially problematic during chunking since the errorsCHIP_ERROR_BUFFER_TOO_SMALLandCHIP_ERROR_NO_MEMORYare what the higher level calls inEngine::BuildSingleReportDataAttributeReportIBsexpect to correctly handle chunking. Since the error got lost and converted, it resulted in the Engine treating it like a critical failure and stopping the generation of reports entirely.Many places where we were assuming we could close out DataIB after encountering a TLV full error when writing data, which was incorrectly and resulted in the subscription failing. Instead, doing the correct reservation of space for these end-of-container tags was done in the fix to prevent encountering any subsequent errors on closure.
On the client side in Python:
Reading
Clusters.OnOff.Attributes.SampleMfgSpecificAttribute0x00000x1049broke because in the Python code, we were generating attribute IDs that were 16-bit values instead of 32-bits. This was a combination of not generating them correctly, as well as the C++ bindings expecting a c_uint16 instead of c_uint32Fixed up
ChipDeviceController.ReadAttributeto actually work correctly with wildcards instead of just erroring out due to parsing issues.Made the data returned by ReadAttribute actually return the value as strongly typed values, making it easier to read.
Validation:
Tested all wildcard read variations using the Python ReadAttribute API in the REPL and ensured it completed successfully, including reading every single attribute on the device! and validating that each and every attribute was of the right type.
Created a definitive, robust chunking validation unit test that artificially limits the size of a packet buffer and sweeps through a range of packet buffer sizes to empirically fit in N to N+1 attributes in a report, exposing all corner cases. This was used to not only ensure the above bugs were caught by the test, but to also validate the fixes.
Added all variations as tests to the mobile device test suite.