Buffered List Chunk Reader #11705
Merged
Buffered List Chunk Reader #11705
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
andy31415,
anush-apple,
austinh0,
balducci-apple,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chulspro,
Damian-Nordic,
electrocucaracha,
franck-apple,
harimau-qirex,
hawk248,
holbrookt,
jelderton,
jepenven-silabs,
jmartinez-silabs,
LuDuda,
mlepage-google,
msandstedt,
pan-apple,
robszewczyk,
sagar-apple,
saurabhst,
selissia,
tcarmelveilleux,
tecimovic and
turon
bzbarsky-apple
requested changes
Nov 17, 2021
mrjerryjohns
force-pushed
the
buffered-read-client
branch
from
5016128 to
f2d7300
Compare
|
PR #11705: Size comparison from ebadd33 to f2d7300 Increases above 0.2%:
Increases (21 builds for efr32, esp32, k32w, linux, p6, qpg, telink)
Full report (23 builds for efr32, esp32, k32w, linux, p6, qpg, telink)
|
|
PR #11705: Size comparison from a5636fa to 4e6af09 Increases above 0.2%:
Increases (24 builds for efr32, esp32, k32w, linux, mbed, p6, qpg, telink)
Full report (28 builds for efr32, esp32, k32w, linux, mbed, p6, qpg, telink)
|
mrjerryjohns
force-pushed
the
buffered-read-client
branch
from
4e6af09 to
08922da
Compare
|
PR #11705: Size comparison from 1adaf7a to 08922da Increases above 0.2%:
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)
|
bzbarsky-apple
approved these changes
Nov 19, 2021
|
PR #11705: Size comparison from 1adaf7a to 48594ad Increases above 0.2%:
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)
|
|
The Darwin test failure reproduces for me locally, and is due to a bug in this PR. We are getting a crash with the following stack: and indeed which is wrong: it should pass |
|
PR #11705: Size comparison from 5f847e0 to 70835d2 Increases above 0.2%:
Increases (15 builds for k32w, linux, p6, qpg, telink)
Full report (17 builds for k32w, linux, p6, qpg, telink)
|
bzbarsky-apple
force-pushed
the
buffered-read-client
branch
from
70835d2 to
e2a05c0
Compare
|
PR #11705: Size comparison from 6cb75f4 to e2a05c0 Increases above 0.2%:
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.
Once list chunking is implemented on the server, clients will need to be updated to deal the the fact that lists will be delivered across separate AttributeDataIBs and potentially, across multiple messages.
To reduce the burden of doing this on clients, this commit creates an adapter class (
BufferedReadCallback) that sits in between the application and theReadClientand buffers up each list chunk before reconstituting them into a contiguous list and dispatching that up to the application.This buffering is done natively in TLV. This avoids the need to understand the specifics of any data model types that the TLV will be decoded into.
We cannot do this buffering at the next level up (i.e at the cluster object level) since those objects are merely 'view' objects and not proper containers. Consequently, there is no way to 'patch' the state in those objects incrementally. By doing it at the TLV level, we can preserve the existing cluster object APIs, which is a significant win.
This PR does not actually fix the ReadClient to correctly handle
AttributeDataIBsthat contain list operations other than replace (i.e append). That should happen in a separate PR.Tests: A TestBufferedReadCallback test that exercises all possible permutations of list and non list data to ensure the state machine is validated.