[chip-tool] Add delay commands module to chip-tool with busy-wait com… #24063
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
|
PR #24063: Size comparison from 64b2ca4 to 1fdc183 Increases (7 builds for bl602, linux, nrfconnect, qpg, telink)
Decreases (7 builds for bl602, bl702, esp32, psoc6, telink)
Full report (35 builds for bl602, bl702, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
andy31415
reviewed
Dec 13, 2022
|
@vivien-apple please add more details to the summary: why is this needed. The "hint" text does not seem to help me understand ... why is a busy loop better for interactive work? |
andy31415
previously requested changes
Dec 13, 2022
There was a problem hiding this comment.
The PR summary describes what is done (add a busy wait loop) but not why.
Please exaplain why a bit clearer. My instinct is to complain on any busy loop in our code - using 100% CPU to wait for a time seems wrong. It needs very strong justification in case this is the only thing possible.
bzbarsky-apple
approved these changes
Dec 14, 2022
vivien-apple
force-pushed
the
ChipTool_BusyWaitCommand
branch
from
1fdc183 to
7e330c5
Compare
pullapprove
bot
requested review from
arkq,
Byungjoo-Lee,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
gjc13,
harimau-qirex,
harsha-rajendran,
hawk248,
jelderton,
jepenven-silabs,
jmartinez-silabs,
lazarkov,
lpbeliveau-silabs,
LuDuda,
mlepage-google,
mrjerryjohns and
msandstedt
pullapprove
bot
requested review from
mspang,
robszewczyk,
saurabhst,
selissia,
tcarmelveilleux,
tecimovic,
tehampson,
vijs,
xylophone21,
yufengwangca and
yunhanw-google
vivien-apple
force-pushed
the
ChipTool_BusyWaitCommand
branch
from
7e330c5 to
7ff9db3
Compare
|
PR #24063: Size comparison from 8055d86 to 7ff9db3 Increases (9 builds for bl602, bl702, cc13x2_26x2, cyw30739, esp32, linux, psoc6, telink)
Decreases (5 builds for bl602, bl702, cc13x2_26x2, telink)
Full report (42 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
andy31415
dismissed
their stale review
Busy loop replaced with sleep. Still needs some comment and PR description updates.
|
@andy31415 I have updated the PR summary. |
bzbarsky-apple
approved these changes
Dec 15, 2022
vivien-apple
force-pushed
the
ChipTool_BusyWaitCommand
branch
from
7ff9db3 to
f6468fe
Compare
|
PR #24063: Size comparison from b49c3ed to f6468fe Increases above 0.2%:
Increases (11 builds for bl602, bl702, cc13x2_26x2, esp32, linux, psoc6, telink)
Decreases (7 builds for bl602, cc13x2_26x2, esp32, psoc6, telink)
Full report (53 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
andy31415
approved these changes
Jan 4, 2023
|
PR #24063: Size comparison from 0f3039f to 6ba19f1 Increases above 0.2%:
Increases (6 builds for bl702, linux, nrfconnect, psoc6, telink)
Decreases (7 builds for bl602, esp32, k32w, psoc6, telink)
Full report (53 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
lecndav
pushed a commit
to lecndav/connectedhomeip
that referenced
this pull request
Mar 22, 2023
…mand (project-chip#24063) Co-authored-by: Andrei Litvin <[email protected]>
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.
…mand
Problem
[hint] This is different than #23886 in the sense that it can be used at any time during an interactive session.
That makes it useful to try to not asked messages from the server, such as the messages from a subscription.
One of the thing this PR tries to achieved is to trigger an
INVALID_SUBSCRIPTIONstatus response when a subscription is in place. The mechanism used is to prevent any message processing from the stack for a duration longer than the subscription maximum interval + the mrp retries.As an example:
The messages are queued during this interval (and not dropped) and so they are delivered to the message processing code afterwards. Which in turn trigger the stack to ack the reports but the subscription is closed on the other side.