[IM] Calculate interaction model timeout accoring to RMP timeouts #18744
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,
harimau-qirex,
hawk248,
harsha-rajendran,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple,
kghost,
lazarkov,
LuDuda,
mlepage-google and
mrjerryjohns
|
PR #18744: Size comparison from 352333a to 39f06d5 Increases (35 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Decreases (19 builds for cc13x2_26x2, esp32, linux, nrfconnect)
Full report (35 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
bzbarsky-apple
approved these changes
May 24, 2022
Co-authored-by: Boris Zbarsky <[email protected]>
mrjerryjohns
suggested changes
May 25, 2022
|
Had a long chat with @tcarmelveilleux, and he made a great suggestion that I think will make this a lot simpler, while still providing applications the ability to do transport-adjusted timeouts. Here's what we eventually arrived at:
@erjiaqing let me know what you think. |
|
The above does mean however that we'll need to modify chip-tool and the REPL to then permit specifying an extra "adjust for transport" parameter to alter the underlying timeout computation... |
|
So, after thinking about this a bit more, I'm starting to come around to the premise of the original solution in this PR. It seems clear that applications have to choose between specifying an absolute timeout, vs a transport-adjusted timeout. You cannot live with just either option alone. There are going to be applications that are about interactiveness, and do not want to be subject to the variance induced by a sleepy target when deciding a timeout for that application. There are also going to be applications that do want to interact with these classes of devices (consciously), and want to bias towards interaction success and not responsiveness. The use of 0 as a way to select 'auto transport adjusted' and non-zero to select 'absolute timeout' seems like a fine way to select this modality. My original concern was the inability to then specify any adjustments when selecting 'auto', but in 99% of interactions, the amount of time it takes to execute a command on the receiver is always quite low (i.e fairly quick). In the cases where it isn't, applications can manually make adjustments if need-be. However, I think it would be useful if the computation of the actual absolute timeout is not specific to the IM. Instead, that should be happening I think, in the exchange layer when Thoughts @erjiaqing @tcarmelveilleux ? |
Infact, I do have some considerations about the use of '0' vs Optional for the default auto timeout setting. However, since
"execute a command" => infact, the timeout is used calculate the timeout of "time to first byte" of the payload e.g. when it comes to reads, it is the timeout for receiving first chunk of reports instead of the whole transaction (I don't suggest the timeout for the whole transaction, since it depends on the actual size of report which cannot be foreseen by the client)
Yes, I will move |
erjiaqing
changed the title
|
PR #18744: Size comparison from f188fd9 to 4c13a6e Increases (49 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Decreases (3 builds for cc13x2_26x2)
Full report (49 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
mrjerryjohns
approved these changes
Jun 1, 2022
|
PR #18744: Size comparison from 131b13c to ab6facf Increases (44 builds for cc13x2_26x2, cyw30739, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Decreases (3 builds for cc13x2_26x2)
Full report (44 builds for cc13x2_26x2, cyw30739, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
| static constexpr System::Clock::Timeout kImMessageTimeout = System::Clock::Seconds16(12); | ||
| // This is an expected upper bound of time-to-first-byte for IM transactions, the actual processing time should never exceed this | ||
| // value unless specified elsewhere (e.g. async command handling for some clusters). | ||
| static constexpr System::Clock::Timeout kExpectedIMProcessingTime = System::Clock::Seconds16(2); |
There was a problem hiding this comment.
Is 2 seconds long enough for crypto-heavy commands like AddNOC/UpdateNOC?
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
Fixed #16797
Change overview
kImMessageTimeoutkExpectedIMProcessingTimeInteractionModelTimeoutForSessionTesting
Testing steps: