[SVE1] Fix level control's CurrentLevel update when receiving an Off command #20788
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,
erjiaqing,
franck-apple,
gjc13,
harsha-rajendran,
hawk248,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple,
kpschoedel,
lazarkov,
LuDuda,
mlepage-google and
msandstedt
marius-alex-tache
force-pushed
the
level-control/fix-off-action
branch
from
322ebfb to
1c8081b
Compare
|
PR #20788: Size comparison from 83deb47 to 1c8081b Increases (29 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, nrfconnect, p6, telink)
Decreases (4 builds for cc13x2_26x2, telink)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
marius-alex-tache
force-pushed
the
level-control/fix-off-action
branch
from
1c8081b to
88a4bdc
Compare
|
PR #20788: Size comparison from b87244d to 88a4bdc Increases (29 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, nrfconnect, p6, telink)
Decreases (3 builds for cc13x2_26x2)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
jmartinez-silabs
requested changes
Jul 21, 2022
There was a problem hiding this comment.
You caught a bug indeed and your fix might work but I think the real issue and easy fix is the following.
The last parameter of this call in emberAfOnOffClusterLevelControlEffectCallback should not be temporaryCurrentLevelCache
moveToLevelHandler(endpoint, Commands::MoveToLevel::Id, minimumLevelAllowedForTheDevice, currentOnOffTransitionTime, 0xFF,
0xFF, temporaryCurrentLevelCache);
This only work when onLevel is Null
resolvedLevel.Value() should be used ( it is set to temporaryCurrentLevelCache if onLevel is null else it is the read value of onLevel).
moveToLevelHandler(endpoint, Commands::MoveToLevel::Id, minimumLevelAllowedForTheDevice, currentOnOffTransitionTime, 0xFF,
0xFF, resolvedLevel.Value() );
With this, nothing else has to change and emberAfLevelControlClusterServerTickCallback will set back the level to the right stored level.
|
@jmartinez-silabs Thank you for your answer! The actual issue is that the current level should not be restored if Furthermore, I don't think |
marius-alex-tache
force-pushed
the
level-control/fix-off-action
branch
from
88a4bdc to
34d0436
Compare
|
PR #20788: Size comparison from 8548f96 to 34d0436 Increases (26 builds for bl602, cc13x2_26x2, cyw30739, efr32, k32w, linux, nrfconnect, p6, telink)
Decreases (4 builds for cc13x2_26x2, telink)
Full report (41 builds for bl602, cc13x2_26x2, cyw30739, efr32, k32w, linux, mbed, nrfconnect, p6, telink)
|
marius-alex-tache
force-pushed
the
level-control/fix-off-action
branch
from
34d0436 to
e86ce2f
Compare
|
PR #20788: Size comparison from c3e71f2 to e86ce2f Increases (29 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, nrfconnect, p6, telink)
Decreases (3 builds for cc13x2_26x2)
Full report (42 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, nrfconnect, p6, telink)
|
|
marius-alex-tache Ok based on your last comment I re-read that portion of the spec. And I understand now. If on level is configured and not null it should stay at the minimal level after an off command. So wouldn't using INVALID_STORED_LEVEL as the last parameter of the moveToLevelHandler as in other places do exactly that? |
jmartinez-silabs
approved these changes
Jul 22, 2022
|
You are correct, but only in the case where should still happen. After reaching minimum level, the actual check of |
|
marius-alex-tache Yeah i understand. I meant that in the case where the action is turn off ( |
|
@jmartinez-silabs Sorry, I misunderstood. I think your solution is more elegant, because there is no need to use the This really looks like some deprecated functionality, especially since |
|
marius-alex-tache I think you are right. state->useOnLevel can be removed. |
|
@marius-alex-tache Please apply 6486a1f to fix the restyled and ZAP failures? |
marius-alex-tache
force-pushed
the
level-control/fix-off-action
branch
from
e86ce2f to
3e3c0de
Compare
According to the Application Clusters specification, when Level Control
cluster is used in association with the On/Off cluster, the action on
receipt for an Off command should be to change the current level to:
* the minimum level allowed for the device, if OnLevel is defined.
* the stored level, if OnLevel is not defined.
In the current implementation, when receiving an Off command, the
currentLevel attribute is always restored to the stored value.
Signed-off-by: Marius Tache <[email protected]>
Restyled by clang-format
marius-alex-tache
force-pushed
the
level-control/fix-off-action
branch
from
3e3c0de to
400ebfd
Compare
|
PR #20788: Size comparison from b647dfb to 400ebfd Increases (8 builds for cc13x2_26x2, esp32, linux, nrfconnect, telink)
Decreases (10 builds for bl602, cc13x2_26x2, efr32, esp32, nrfconnect, p6)
Full report (32 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
github-actions bot
pushed a commit
that referenced
this pull request
Jul 23, 2022
…#20788) According to the Application Clusters specification, when Level Control cluster is used in association with the On/Off cluster, the action on receipt for an Off command should be to change the current level to: * the minimum level allowed for the device, if OnLevel is defined. * the stored level, if OnLevel is not defined. In the current implementation, when receiving an Off command, the currentLevel attribute is always restored to the stored value. Signed-off-by: Marius Tache <[email protected]> Restyled by clang-format
woody-apple
added a commit
that referenced
this pull request
Jul 25, 2022
…#20788) (#21143) According to the Application Clusters specification, when Level Control cluster is used in association with the On/Off cluster, the action on receipt for an Off command should be to change the current level to: * the minimum level allowed for the device, if OnLevel is defined. * the stored level, if OnLevel is not defined. In the current implementation, when receiving an Off command, the currentLevel attribute is always restored to the stored value. Signed-off-by: Marius Tache <[email protected]> Restyled by clang-format Co-authored-by: marius-alex-tache <[email protected]>
isiu-apple
pushed a commit
to isiu-apple/connectedhomeip
that referenced
this pull request
Sep 16, 2022
…project-chip#20788) According to the Application Clusters specification, when Level Control cluster is used in association with the On/Off cluster, the action on receipt for an Off command should be to change the current level to: * the minimum level allowed for the device, if OnLevel is defined. * the stored level, if OnLevel is not defined. In the current implementation, when receiving an Off command, the currentLevel attribute is always restored to the stored value. Signed-off-by: Marius Tache <[email protected]> Restyled by clang-format
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
When using both
Level ControlandOn/Offclusters on the same endpoint, theCurrentLevelattribute is not correctly updated when receiving anOffcommand.When
emberAfOnOffClusterLevelControlEffectCallbackis called, ifnewValueis false (transitionOn -> Off), there is a comment which states that the actualOnLevelcheck will be done inemberAfLevelControlClusterServerTickCallback. There is no such check. Furthermore, due to the fact thattemporaryCurrentLevelCachecan never be equal toINVALID_STORED_LEVEL(0xFFFF) whenOnLevelis defined and it's valid, the check insideemberAfLevelControlClusterServerTickCallbackwill always pass and theCurrentLevelvalue will be restored.I have used section 1.6.4.1.1 from the Application Clusters document as reference.
Proposed Solution
Add a condition in
emberAfLevelControlClusterServerTickCallback, which checks if theOnLevelattribute is defined. If it is,the
CurrentLevelshould not be restored to the previously saved value.