Fix command timeouts during commissioning. #19137
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,
arkq,
Byungjoo-Lee,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
erjiaqing,
franck-apple,
gjc13,
harimau-qirex,
hawk248,
harsha-rajendran,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple,
kghost,
kpschoedel,
lazarkov,
LuDuda,
mlepage-google,
mrjerryjohns and
msandstedt
pullapprove
bot
requested review from
sagar-apple,
saurabhst,
selissia,
tcarmelveilleux,
tecimovic,
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21 and
yunhanw-google
|
PR #19137: Size comparison from e8c2a6f to 49e6038 Increases (1 build for k32w)
Full report (2 builds for k32w)
|
Several fixes here:
1. AutoCommissioner has a comment about how per spec everything during
commissioning needs at least a 30s timeout, and it was passing that to
PerformCommissioningStep, but DeviceCommissioner was ignoring the "timeout"
parameter for a bunch of the cases, including crucially for AddNOC and
CSRRequest. Those can take a while to run, and were hitting the
now-much-lower default 2s timeout. The fix here is to stop ignoring the
passed-in value.
2. The passed-in timeout value computation in AutoCommissioner was not quite
right. It was set to max(30s, network-connect-time), but the network connect
time is the processing time on the server, not the total time including
transport latency. Fix the computation of the timeout to:
a. Take the network connect times for the network enable steps, a "slow
crypto" time of 15s for the AddNOC and CSRRequest steps, and the default
IM timeout for all other steps and treat that as the server processing
time.
b. Add the transport timeout bits from our device's session to that server
processing time.
c. If the result is less than the spec-mandated 30s timeout, use 30s,
otherwise use the result we computed.
3. Assuming that BLE has 0 transport latency is wrong. Not clear what the right
value is, but for now setting it to the same as TCP.
Fixes project-chip#19135
bzbarsky-apple
force-pushed
the
fix-commissioning-timeouts
branch
from
49e6038
to
30cc40f
Compare
|
PR #19137: Size comparison from e8c2a6f to 30cc40f Increases (28 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, nrfconnect, p6, telink)
Decreases (2 builds for cc13x2_26x2)
Full report (41 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
andy31415
approved these changes
Jun 3, 2022
msandstedt
reviewed
Jun 3, 2022
msandstedt
approved these changes
Jun 3, 2022
jmartinez-silabs
approved these changes
Jun 3, 2022
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.
Several fixes here:
AutoCommissioner has a comment about how per spec everything during
commissioning needs at least a 30s timeout, and it was passing that to
PerformCommissioningStep, but DeviceCommissioner was ignoring the "timeout"
parameter for a bunch of the cases, including crucially for AddNOC and
CSRRequest. Those can take a while to run, and were hitting the
now-much-lower default 2s timeout. The fix here is to stop ignoring the
passed-in value.
The passed-in timeout value computation in AutoCommissioner was not quite
right. It was set to max(30s, network-connect-time), but the network connect
time is the processing time on the server, not the total time including
transport latency. Fix the computation of the timeout to:
a. Take the network connect times for the network enable steps, a "slow
crypto" time of 15s for the AddNOC and CSRRequest steps, and the default
IM timeout for all other steps and treat that as the server processing
time.
b. Add the transport timeout bits from our device's session to that server
processing time.
c. If the result is less than the spec-mandated 30s timeout, use 30s,
otherwise use the result we computed.
Assuming that BLE has 0 transport latency is wrong. Not clear what the right
value is, but for now setting it to the same as TCP.
Fixes #19135
Problem
See above.
Change overview
See above.
Testing
Verified via logging that sane timeout values are computed, and checked that BLE commissioning of an M5Stack in fact works now.