Update GetAckTimeout to use the backoff algorithm #23307
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,
emargolis,
franck-apple,
gjc13,
harsha-rajendran,
hawk248,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple,
kghost,
lazarkov,
LuDuda and
mlepage-google
Hmm. What are the parameters involved there? I assume the connect is an async process, so it sends the ack, but the exchange timeout is being hit? What "time it will take the server" are we providing as input to that connect call? It must be at least 30 seconds, since this is during commissioning, right? So 30s + active MRP bits (34s) is not enough in this case? Maybe we should be using a higher timeout for ConnectNetwork in AutoCommissioner? |
vivien-apple
force-pushed
the
SessionEstablihsment_Timeouts
branch
from
68c6f86 to
566ddb5
Compare
|
PR #23307: Size comparison from 0c2f7c6 to 566ddb5 Increases (24 builds for bl602, bl702, cc13x2_26x2, k32w, linux, mbed, nrfconnect, qpg, telink)
Decreases (8 builds for cc13x2_26x2, linux)
Full report (24 builds for bl602, bl702, cc13x2_26x2, k32w, linux, mbed, nrfconnect, qpg, telink)
|
vivien-apple
force-pushed
the
SessionEstablihsment_Timeouts
branch
from
566ddb5 to
9db295b
Compare
Based on the logs I assume the Ack is received: What "time it will take the server" are we providing as input to that connect call? It must be at least 30 seconds, since this is during commissioning, right? So 30s + active MRP bits (34s) is not enough in this case? Maybe we should be using a higher timeout for ConnectNetwork in AutoCommissioner? The python test suite is not using the Connect Network command.
It ends up calling: https://github.com/project-chip/connectedhomeip/blob/b9d32ecefd004adb7cc4c70a5f4a567dca199da0/src/controller/python/chip/clusters/command.cpp which in turns call |
|
PR #23307: Size comparison from 0c2f7c6 to 9db295b Increases (36 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
Decreases (8 builds for cc13x2_26x2, linux)
Full report (36 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
Well, that would be the bug, the. It's violating the spec, if it allows less than 30s on the server for any commissioning-related command. @mrjerryjohns @tcarmelveilleux @yunhanw-google could you please take a look at the python bits there? |
|
@vivien-apple Does just adding |
bzbarsky-apple
changed the title
bzbarsky-apple
approved these changes
Oct 25, 2022
Yes, I have tried already to add |
vivien-apple
force-pushed
the
SessionEstablihsment_Timeouts
branch
2 times, most recently
from
7e58087 to
3b1cde1
Compare
|
PR #23307: Size comparison from 0c2f7c6 to 3b1cde1 Increases (38 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
Decreases (7 builds for cc13x2_26x2, esp32)
Full report (38 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
src/controller/python/test/test_scripts/network_commissioning.py
Outdated
Show resolved
Hide resolved
…fig such that an accurate maximum retransmission time could be calculated from the external world
… and use GetRetransmissionTimeout to be more accurate
…py ScanNetwork and ConnectNetwork tests with an explicit interactionTimeoutMs instead of relying on GetAckTimeout behavior
vivien-apple
force-pushed
the
SessionEstablihsment_Timeouts
branch
from
3b1cde1 to
d6fdc1c
Compare
|
PR #23307: Size comparison from 0c2f7c6 to d6fdc1c Increases (38 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
Decreases (7 builds for cc13x2_26x2, esp32)
Full report (38 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
Damian-Nordic
approved these changes
Oct 26, 2022
adbridge
pushed a commit
to ARM-software/connectedhomeip
that referenced
this pull request
Nov 18, 2022
* [MRP] Expose GetRetransmissionTimeout from ReliableMessageProtocolConfig such that an accurate maximum retransmission time could be calculated from the external world * [MRP] Update GetAckTimeout to take into account the PeerActive status and use GetRetransmissionTimeout to be more accurate * Update src/controller/python/test/test_scripts/network_commissioning.py ScanNetwork and ConnectNetwork tests with an explicit interactionTimeoutMs instead of relying on GetAckTimeout behavior
adbridge
pushed a commit
to ARM-software/connectedhomeip
that referenced
this pull request
Nov 18, 2022
* [MRP] Expose GetRetransmissionTimeout from ReliableMessageProtocolConfig such that an accurate maximum retransmission time could be calculated from the external world * [MRP] Update GetAckTimeout to take into account the PeerActive status and use GetRetransmissionTimeout to be more accurate * Update src/controller/python/test/test_scripts/network_commissioning.py ScanNetwork and ConnectNetwork tests with an explicit interactionTimeoutMs instead of relying on GetAckTimeout behavior
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
SecureSession::GetAckTimeoutandUnauthenticatedSessionTable::GetAckTimeoutalways use the idle specified by MRP without taking into account thePeerActiveMode.Also, the current formula calculation:
GetRemoteMRPConfig().mIdleRetransTimeout * (CHIP_CONFIG_RMP_DEFAULT_MAX_RETRANS + 1);represent a static time and not a time that changes such as represented bymrpBackoffTime.If someone uses
GetAckTimeoutto configure the exchange response timeout, it may end up into ending the exchange before all MRP retransmissions is done.