Change mrp-parameter-struct to hold 32-bit milliseconds #17978
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
andy31415,
anush-apple,
arkq,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
erjiaqing,
franck-apple,
gjc13,
hawk248,
harsha-rajendran,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
kghost,
kpschoedel,
lazarkov,
LuDuda,
mlepage-google,
mrjerryjohns,
mspang and
sagar-apple
pullapprove
bot
requested review from
saurabhst,
selissia,
tcarmelveilleux,
tecimovic,
turon,
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21 and
yunhanw-google
tcarmelveilleux
approved these changes
May 2, 2022
Nodes store and advertise MRP parameters as 32-bit values. However, the mrp-parameter-struct had been specified to only hold 16-bit values on the wire. This would lead to session establishment failures with nodes attempting to exchange values in excess of 65536 milliseconds, despite the fact that values up to 360,000 milliseconds are legal. This corrects the problem to allow up-to 32-bit values per the spec change here: https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/5173 In most cases, peers will be using smaller MRP values and and will therefore still exchange 1 or 2-byte fields on the wire, making this change mostly backward compatible. Testing: verification of successful exchange of larger MRP values up to 360,000 has been added to TestCASESession.cpp. TestTxtFields.cpp already has coverage for advertisement of large values. Fixes project-chip#17812
bzbarsky-apple
approved these changes
May 2, 2022
|
PR #17978: Size comparison from 769264a to 3812be3 Increases (4 builds for cc13x2_26x2)
Decreases (33 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, nrfconnect, p6, telink)
Full report (34 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, 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.
Problem
Nodes store and advertise MRP parameters as 32-bit values. However,
the mrp-parameter-struct had been specified to only hold 16-bit values
on the wire. This would lead to session establishment failures with
nodes attempting to exchange values in excess of 65536 milliseconds,
despite the fact that values up to 360,000 milliseconds are legal.
Change overview
This corrects the problem to allow up-to 32-bit values per the spec
change here:
https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/5173
In most cases, peers will be using smaller MRP values and and will
therefore still exchange 1 or 2-byte fields on the wire, making this
change mostly backward compatible.
Fixes #17812
Testing
Testing: verification of successful exchange of larger MRP values up
to 360,000 has been added to TestCASESession.cpp. TestTxtFields.cpp
already has coverage for advertisement of large values.