Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add contraint checks on Color contol arguments value ranges #36202

Closed
wants to merge 2 commits into from
Closed

Add contraint checks on Color contol arguments value ranges #36202

wants to merge 2 commits into from

Conversation

jmartinez-silabs
Copy link
Member

@jmartinez-silabs jmartinez-silabs commented Oct 22, 2024 •
edited
Loading

fixes #34721

This PR adds some constraint checks on some Color control command arguments, most notably the TransitionTime.

It also rework the command handlers to :

  • Clean up and unify all command handlers of the color control. There were multiple implementation schemes across handlers
  • remove the goto mechanism and in favour of early return conditions (VerifyOrReturnValue)

I verified that there is no regression by running all automated TC-CC tests.

@semanticdiff-com SemanticDiff.com
Copy link

Review changes with SemanticDiff.

@github-actions github-actions bot added the app label Oct 22, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 22, 2024 •
edited
Loading

PR #36202: Size comparison from 9041169 to 29a9366

Full report (66 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink)
platform target config section 9041169 29a9366 change % change
bl602 lighting-app bl602+mfd+littlefs+rpc FLASH 1350008 1349496 -512 -0.0
RAM 104120 104120 0 0.0
bl702 lighting-app bl702+eth FLASH 647710 647966 256 0.0
RAM 25233 25233 0 0.0
bl702+wifi FLASH 825274 825530 256 0.0
RAM 13965 13965 0 0.0
bl706+mfd+rpc+littlefs FLASH 1054200 1054456 256 0.0
RAM 23821 23821 0 0.0
bl702l lighting-app bl702l+mfd+littlefs FLASH 975048 975304 256 0.0
RAM 16468 16468 0 0.0
cc13x4_26x4 lighting-app LP_EM_CC1354P10_6 FLASH 829564 829612 48 0.0
RAM 123452 123452 0 0.0
lock-ftd LP_EM_CC1354P10_6 FLASH 814872 814872 0 0.0
RAM 125332 125332 0 0.0
pump-app LP_EM_CC1354P10_6 FLASH 761460 761460 0 0.0
RAM 113824 113824 0 0.0
pump-controller-app LP_EM_CC1354P10_6 FLASH 745704 745704 0 0.0
RAM 114016 114016 0 0.0
cc32xx air-purifier CC3235SF_LAUNCHXL FLASH 617162 617162 0 0.0
RAM 205908 205908 0 0.0
lock CC3235SF_LAUNCHXL FLASH 657210 657210 0 0.0
RAM 206060 206060 0 0.0
cyw30739 light CYW30739B2-P5-EVK-01 unknown 2040 2040 0 0.0
FLASH 678589 678645 56 0.0
RAM 78668 78668 0 0.0
CYW30739B2-P5-EVK-02 unknown 2040 2040 0 0.0
FLASH 698433 698489 56 0.0
RAM 81300 81300 0 0.0
CYW30739B2-P5-EVK-03 unknown 2040 2040 0 0.0
FLASH 698433 698489 56 0.0
RAM 81300 81300 0 0.0
CYW930739M2EVB-02 unknown 2040 2040 0 0.0
FLASH 655377 655433 56 0.0
RAM 73736 73736 0 0.0
light-switch CYW30739B2-P5-EVK-01 unknown 2040 2040 0 0.0
FLASH 614933 614933 0 0.0
RAM 71628 71628 0 0.0
CYW30739B2-P5-EVK-02 unknown 2040 2040 0 0.0
FLASH 634569 634569 0 0.0
RAM 74180 74180 0 0.0
CYW30739B2-P5-EVK-03 unknown 2040 2040 0 0.0
FLASH 634569 634569 0 0.0
RAM 74180 74180 0 0.0
lock CYW30739B2-P5-EVK-01 unknown 2040 2040 0 0.0
FLASH 634213 634213 0 0.0
RAM 74676 74676 0 0.0
CYW30739B2-P5-EVK-02 unknown 2040 2040 0 0.0
FLASH 653921 653921 0 0.0
RAM 77228 77228 0 0.0
CYW30739B2-P5-EVK-03 unknown 2040 2040 0 0.0
FLASH 653921 653921 0 0.0
RAM 77228 77228 0 0.0
thermostat CYW30739B2-P5-EVK-01 unknown 2040 2040 0 0.0
FLASH 609525 609525 0 0.0
RAM 68764 68764 0 0.0
CYW30739B2-P5-EVK-02 unknown 2040 2040 0 0.0
FLASH 629385 629385 0 0.0
RAM 71396 71396 0 0.0
CYW30739B2-P5-EVK-03 unknown 2040 2040 0 0.0
FLASH 629385 629385 0 0.0
RAM 71396 71396 0 0.0
efr32 lock-app BRD4187C FLASH 925248 925248 0 0.0
RAM 159708 159708 0 0.0
BRD4338a FLASH 742264 742264 0 0.0
RAM 232076 232076 0 0.0
window-app BRD4187C FLASH 1018236 1018236 0 0.0
RAM 128052 128052 0 0.0
esp32 all-clusters-app c3devkit DRAM 95256 95256 0 0.0
FLASH 1539960 1540182 222 0.0
IRAM 82538 82538 0 0.0
m5stack DRAM 116192 116192 0 0.0
FLASH 1550166 1549990 -176 -0.0
IRAM 117039 117039 0 0.0
linux air-purifier-app debug unknown 4688 4688 0 0.0
FLASH 2781395 2781395 0 0.0
RAM 129520 129520 0 0.0
all-clusters-app debug unknown 5528 5528 0 0.0
FLASH 6092520 6092312 -208 -0.0
RAM 523840 523840 0 0.0
all-clusters-minimal-app debug unknown 5424 5424 0 0.0
FLASH 5423064 5423120 56 0.0
RAM 242416 242416 0 0.0
bridge-app debug unknown 5408 5408 0 0.0
FLASH 4751992 4751992 0 0.0
RAM 218384 218384 0 0.0
chip-tool debug unknown 5960 5960 0 0.0
FLASH 13096588 13096588 0 0.0
RAM 584402 584402 0 0.0
chip-tool-ipv6only arm64 unknown 21408 21408 0 0.0
FLASH 11666896 11666896 0 0.0
RAM 635344 635344 0 0.0
fabric-admin debug unknown 5792 5792 0 0.0
FLASH 11355369 11355369 0 0.0
RAM 584890 584890 0 0.0
fabric-bridge-app debug unknown 4632 4632 0 0.0
FLASH 4578306 4578306 0 0.0
RAM 205336 205336 0 0.0
fabric-sync debug unknown 4816 4816 0 0.0
FLASH 5290773 5290773 0 0.0
RAM 465416 465416 0 0.0
lighting-app debug+rpc+ui unknown 6056 6056 0 0.0
FLASH 5693921 5693713 -208 -0.0
RAM 228488 228488 0 0.0
lock-app debug unknown 5344 5344 0 0.0
FLASH 4801538 4801538 0 0.0
RAM 204472 204472 0 0.0
ota-provider-app debug unknown 4720 4720 0 0.0
FLASH 4430808 4430808 0 0.0
RAM 198192 198192 0 0.0
ota-requestor-app debug unknown 4656 4656 0 0.0
FLASH 4569580 4569580 0 0.0
RAM 202760 202760 0 0.0
shell debug unknown 4216 4216 0 0.0
FLASH 3115981 3115789 -192 -0.0
RAM 160368 160368 0 0.0
thermostat-no-ble arm64 unknown 9448 9448 0 0.0
FLASH 4319760 4319760 0 0.0
RAM 242896 242896 0 0.0
tv-app debug unknown 5624 5624 0 0.0
FLASH 6032885 6032885 0 0.0
RAM 596256 596256 0 0.0
tv-casting-app debug unknown 5208 5208 0 0.0
FLASH 11300989 11300989 0 0.0
RAM 675776 675776 0 0.0
nrfconnect all-clusters-app nrf52840dk_nrf52840 FLASH 915528 915580 52 0.0
RAM 143357 143357 0 0.0
nrf7002dk_nrf5340_cpuapp FLASH 885992 885932 -60 -0.0
RAM 141496 141496 0 0.0
all-clusters-minimal-app nrf52840dk_nrf52840 FLASH 848936 848960 24 0.0
RAM 142265 142265 0 0.0
nxp contact k32w0+release FLASH 582296 582296 0 0.0
RAM 70948 70948 0 0.0
mcxw71+release FLASH 596928 596928 0 0.0
RAM 63184 63184 0 0.0
light k32w0+release FLASH 618932 618932 0 0.0
RAM 70412 70412 0 0.0
k32w1+release FLASH 683160 683160 0 0.0
RAM 48816 48816 0 0.0
lock mcxw71+release FLASH 746712 746712 0 0.0
RAM 67340 67340 0 0.0
psoc6 all-clusters cy8ckit_062s2_43012 FLASH 1647604 1647364 -240 -0.0
RAM 212408 212408 0 0.0
all-clusters-minimal cy8ckit_062s2_43012 FLASH 1553684 1553700 16 0.0
RAM 209208 209208 0 0.0
light cy8ckit_062s2_43012 FLASH 1468004 1467764 -240 -0.0
RAM 201200 201200 0 0.0
lock cy8ckit_062s2_43012 FLASH 1464980 1464980 0 0.0
RAM 225560 225560 0 0.0
qpg lighting-app qpg6105+debug FLASH 660600 660648 48 0.0
RAM 105396 105396 0 0.0
lock-app qpg6105+debug FLASH 618612 618612 0 0.0
RAM 99864 99864 0 0.0
stm32 light STM32WB5MM-DK FLASH 481936 481992 56 0.0
RAM 144844 144844 0 0.0
telink bridge-app tlsr9258a FLASH 681164 681164 0 0.0
RAM 91304 91304 0 0.0
contact-sensor-app tlsr9528a_retention FLASH 620814 620814 0 0.0
RAM 50600 50600 0 0.0
light-switch-app-ota-shell-factory-data tlsr9528a FLASH 708794 708794 0 0.0
RAM 73940 73940 0 0.0
lighting-app-ota-factory-data tlsr9118bdk40d FLASH 625764 626102 338 0.1
RAM 144468 144468 0 0.0
lighting-app-ota-rpc-factory-data-4mb tlsr9518adk80d FLASH 811772 812110 338 0.0
RAM 99100 99100 0 0.0

bzbarsky-apple
bzbarsky-apple requested changes Oct 26, 2024
View reviewed changes
src/app/clusters/color-control-server/color-control-server.cpp
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do "reorganization with no functional change" and "behavior changes" as separate PRs. Reviewing this and figuring out which behavior changes are intended and which ones are bugs in the refactoring is very difficult.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will split it in a few pr this week. Thanks.

This was referenced Nov 5, 2024
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bzbarsky-apple bzbarsky-apple bzbarsky-apple requested changes

Assignees
No one assigned
Labels
app review - pending
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[1.3] Out-of-Range Problems in Various color control cluster
2 participants
@jmartinez-silabs @bzbarsky-apple