Immediately perform colour transitions for ColorControl commands where TransitionTime is zero #26987
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
Commands to the colour control server that had a transition time of zero were being deferred for 100 ms. Instead they should be handled immediately.
When the transition time is 0, the remaining time should also be 0.
pullapprove
bot
requested review from
amitnj,
andy31415,
anush-apple,
arkq,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
CodeChronos928,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
franck-apple,
gjc13,
harsha-rajendran,
hawk248,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo,
jtung-apple,
kcoppock and
kkasperczyk-no
|
PR #26987: Size comparison from 77b1596 to 3db9546 Increases (29 builds for bl602, bl702, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, psoc6, qpg, telink)
Decreases (5 builds for psoc6, telink)
Full report (58 builds for bl602, bl702, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
|
I believe we can streamline those changes in a cleaner way. I am not convinced we need the new vars in each function I agreed with removing the But setHsvRemainingtime() and similar should remain the focal point where to update the RemainingTime attribute Lastly the only change left would be |
…the number of steps remaining. This ensures the calculation for time remaining is correct when both a hue and a saturation command are running simultaneously.
Calculate the amount of time to delay the first step where it's used instead of at the start of the functions.
Calculate the value where it's used instead.
|
I've added the time remaining in the transition states so it's distinct from the number of steps. This keeps SetHSVRemainingTime as the place where the RemainingTime attribute is set for hue and saturation commands. |
bzbarsky-apple
approved these changes
Jun 2, 2023
|
PR #26987: Size comparison from 9828293 to c34cf1f Increases (30 builds for bl602, bl702, cc32xx, cyw30739, efr32, esp32, k32w, linux, nrfconnect, psoc6, qpg, telink)
Decreases (10 builds for bl702, k32w, psoc6, qpg, telink)
Full report (58 builds for bl602, bl702, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
cecille
reviewed
Jun 6, 2023
|
General Q: Level control and on/off have a similar mechanism - do they also suffer from this problem? |
jmartinez-silabs
approved these changes
Jun 6, 2023
Level control's implementation will correctly schedule the work to occur immediately when the transition time is zero. I'm not familiar with the on/off cluster, but I think the OnWithTimedOff command might have a similar issue? In some cases it schedules a timer for 100 ms after the command is received, and I believe it's possible it could do so while OnTime is set to zero. |
|
PR #26987: Size comparison from a73c8e9 to f1a3ed7 Increases (30 builds for bl602, bl702, cyw30739, efr32, esp32, linux, nrfconnect, psoc6, qpg, telink)
Decreases (10 builds for bl702, cc32xx, psoc6, qpg, telink)
Full report (57 builds for bl602, bl702, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
cecille
approved these changes
Jun 6, 2023
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.
Fixes #26929
Problem
When TransitionTime is zero, the ColorControl cluster treats it as one instead so commands that should apply instantly instead take 100 ms.
When followed by another command (e.g. MoveToColorTemperature followed by On), the introduced delay would cause the later command to be applied before the colour-changing command.
Change overview