Fix overlapping execution for TriggerEffect and Identify commands in Identify Server #21968
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,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
franck-apple,
gjc13,
harimau-qirex,
harsha-rajendran,
hawk248,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple,
kghost,
kpschoedel,
lazarkov,
LuDuda,
mlepage-google,
mrjerryjohns and
msandstedt
pullapprove
bot
requested review from
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21 and
yunhanw-google
|
PR #21968: Size comparison from 65176fa to d9eea2d Increases (16 builds for cc13x2_26x2, cyw30739, esp32, k32w, mbed, nrfconnect, telink)
Decreases (20 builds for bl602, cc13x2_26x2, efr32, linux, p6)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
woody-apple
approved these changes
Aug 17, 2022
andrei-menzopol
force-pushed
the
identify-triggereffect
branch
from
d9eea2d to
c611c6c
Compare
|
PR #21968: Size comparison from 1ea6bbe to c611c6c Increases (15 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, nrfconnect, telink)
Decreases (11 builds for bl602, cc13x2_26x2, efr32, linux, p6)
Full report (32 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
andrei-menzopol
force-pushed
the
identify-triggereffect
branch
from
c611c6c to
f9d06b7
Compare
|
PR #21968: Size comparison from 3f69587 to f9d06b7 Increases (15 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, nrfconnect, telink)
Decreases (19 builds for bl602, cc13x2_26x2, efr32, linux, psoc6)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, telink)
|
andrei-menzopol
force-pushed
the
identify-triggereffect
branch
2 times, most recently
from
f9d06b7 to
8483c74
Compare
|
PR #21968: Size comparison from a8d12af to 8483c74 Increases (13 builds for cc13x2_26x2, cyw30739, esp32, k32w, nrfconnect, telink)
Decreases (13 builds for bl602, cc13x2_26x2, efr32, linux, psoc6)
Full report (32 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, telink)
|
* [K32W0] Add TriggerEffect support, update readme * Fix behaviour when TriggerEffect called during Identify and vice versa * Update wordlist Signed-off-by: Andrei Menzopol <[email protected]>
andrei-menzopol
force-pushed
the
identify-triggereffect
branch
from
8483c74 to
724c472
Compare
|
PR #21968: Size comparison from 82d974b to 724c472 Increases (16 builds for cc13x2_26x2, cyw30739, esp32, k32w, nrfconnect, telink)
Decreases (20 builds for bl602, cc13x2_26x2, efr32, linux, psoc6)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, telink)
|
|
@jmartinez-silabs could you take a look, please? |
jmartinez-silabs
requested changes
Aug 27, 2022
jmartinez-silabs
approved these changes
Aug 29, 2022
isiu-apple
pushed a commit
to isiu-apple/connectedhomeip
that referenced
this pull request
Sep 16, 2022
…Identify Server (project-chip#21968) * Fix overlapping behaviour for TriggerEffect and Identify commands * [K32W0] Add TriggerEffect support, update readme * Fix behaviour when TriggerEffect called during Identify and vice versa * Update wordlist Signed-off-by: Andrei Menzopol <[email protected]> * Restyled by clang-format * Restyled by prettier-markdown Signed-off-by: Andrei Menzopol <[email protected]> Co-authored-by: Restyled.io <[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.
Problem
What is being fixed? Examples:
The Identify cluster server's Identify and TriggerEffect commands don't work well in overlapping execution especially when Identify is executing and TriggerEffect is called. This behaviour is not clarified on the spec either, I opened an issue.
In the identify server code the Identify command can be canceled by TriggerEffect but only if
identify->mTargetEffectIdentifier != identify->mCurrentEffectIdentifierwhich is correct if TriggerEffect command is called with all effect identifiers but one, the one used to initialize the mTargetEffectIdentifier and mCurrentEffectIdentifier at Identify instance creation in AppTask.cpp, in which case it will let Identify command continue. I fixed it on k32w0 by using invalid effect identifiers for initialization.Moreover, if TriggerEffect command is called with finish effect, it also won't stop the Identify command since the same callback
onIdentifyClusterTickis called. I know that finish effect lets the device complete the current effect sequence but is this the whole Identify command or the current effect (e.g current led flash period)?The other way around (Identify command called during TriggerEffect running) can be solved on the app task (e.g. reset internal flags, structures, enable/disable led effects though callbacks).
Change overview
What's in this PR
This PR tries to come up with a solution to the problem mentioned. It may need more work and clarifications.
Testing
How was this tested? (at least one bullet point required)
Tested on k32w0 with chip-tool commands.