[Scenes] Level control handler bugfix #29076
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
…ssary typing and added doc on on-off and level control handlers interactions
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
gjc13,
harsha-rajendran and
hawk248
pullapprove
bot
requested review from
turon,
tobiasgraf,
vivien-apple,
woody-apple,
younghak-hwang,
yufengwangca and
yunhanw-google
|
PR #29076: Size comparison from 14cbb32 to 63bd2cb Increases (12 builds for bl702, bl702l, esp32, linux, psoc6, telink)
Decreases (11 builds for cc32xx, efr32, esp32, nrfconnect, psoc6, telink)
Full report (67 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
jmartinez-silabs
approved these changes
Sep 6, 2023
andy31415
reviewed
Sep 6, 2023
There was a problem hiding this comment.
@lpbeliveau-silabs could we update the summary to contain things a bit clearer on what to expect in the PR?
Title says "bugfix" but it never says what bug... so the summary links to an issue and if opening the issue, that links to a PR review.
That is a lot of steps for reviewers and when this log becomes part of the git history it will likely not be useful either.
Could we explain what the bug was and what the fix is?
Co-authored-by: Junior Martinez <[email protected]>
|
Summary updated. |
|
PR #29076: Size comparison from 14cbb32 to e32c08f Increases (15 builds for bl702, bl702l, efr32, esp32, linux, nrfconnect, psoc6, telink)
Decreases (12 builds for cc32xx, efr32, esp32, nrfconnect, psoc6, telink)
Full report (67 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
andy31415
approved these changes
Sep 7, 2023
| } | ||
| else | ||
| { | ||
| chip::app::NumericAttributeTraits<uint32_t>::SetNull(pairs[0].attributeValue); |
There was a problem hiding this comment.
But ApplyScene was not fixed, so this is broken.... How was this tested?
There was a problem hiding this comment.
Currently, If null value (255) for a uint8_t is stored, this will cause the ApplyScene to fail as we are asking to move to a value outside of the boundaries.
This will return failure to the Apply Scene. Is the expected behavior here to silently fail instead of explicitly letting the user know the scene wasn't applied?
There was a problem hiding this comment.
I would think the expected behavior is to move to the values that were stored in the scene, including a null level if that's what was stored.
There was a problem hiding this comment.
I see, so we do not want that recall scene to end up in a failure.
How do we expect the level to behave to a null level? Value capped at maximum or lowest possible value, remaining at current level?
So basically, we either ignore the level command silently, ignore the whole scene returning an error (current implementation), or move to an unexpected level.
Say we explicitly add a scene with a null value, we could say this behaves as expected, but what if null is added from a save scene. This means the user just saved the current scene while the level value was null. This probably means the user was not expecting the level to have an impact when restoring the scene.
By that reasoning, should we decide that silently ignoring the move to null seems like the expected behavior?
There was a problem hiding this comment.
How do we expect the level to behave to a null level?
Well, how was it behaving when the scene was saved?
The questions you are asking are spec questions, not implementation questions, no? If the spec does not define this, it needs to...
There was a problem hiding this comment.
The MoveToLevel Command states that the command SHALL have the Level data field, constrained between 0 to 254. We do not respect that, therefore this should result in an error.
However the RecallScene states that the status of RecallScene at step 3 (applying EFS) SHALL be SUCCESS, so my initial interpretation seems wrong, silent failure seems to be the way here.
There was a problem hiding this comment.
Where does the spec say that recalling a scene involves a MoveToLevel command? Seems like if that's the intent the spec needs to say it...
There was a problem hiding this comment.
set the attributes and corresponding state of the cluster accordingly.
The "corresponding state of the cluster" bits.
Either way, setting the attributes to null without taking an action on the actual level would mean read level won't reflect the actual state of the cluster if no action is taken.
Now does it have to be the actual moveToLevel command, the spec doesn't precise it. So we could technically create a separate function that allows us to move the level to "null" but that is still not defined right?
There was a problem hiding this comment.
@lpbeliveau-silabs Indeed.
abpoth
pushed a commit
to abpoth/connectedhomeip
that referenced
this pull request
Sep 12, 2023
* Used NumericAttributesTraits for storage null values, removed un-necessary typing and added doc on on-off and level control handlers interactions * Update src/app/clusters/on-off-server/on-off-server.cpp Co-authored-by: Junior Martinez <[email protected]> --------- Co-authored-by: Junior Martinez <[email protected]>
HunsupJung
pushed a commit
to HunsupJung/connectedhomeip
that referenced
this pull request
Oct 23, 2023
* Used NumericAttributesTraits for storage null values, removed un-necessary typing and added doc on on-off and level control handlers interactions * Update src/app/clusters/on-off-server/on-off-server.cpp Co-authored-by: Junior Martinez <[email protected]> --------- Co-authored-by: Junior Martinez <[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.
Addresses : #29033
Solves broken null value storage by adding proper null handling for storing the current level.
Also removes un-necessary explicit typing and documents the behavior of the on-off handler when the level-control handler is registered.