Skip to content

[Part3] Decouple ICD Management cluster to be code driven. #41499

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
Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

[Part3] Decouple ICD Management cluster to be code driven. #41499

wants to merge 11 commits into from
+764 −398

Conversation

@jadhavrohit924
Copy link
Contributor

@jadhavrohit924 jadhavrohit924 commented Oct 17, 2025 •
edited
Loading

Summary

Creates a DefaultServerCluster out of the ICD Management.

Related issues

Fixes: #40988

Testing

  • Manually tested lit-icd-app with basic functionality read attributes.
  • Unit tests.

Readability checklist

The checklist below will help the reviewer finish PR review in time and keep the
code readable:

  • PR title is
    descriptive
  • Apply the
    “When in Rome…”
    rule (coding style)
  • PR size is short
  • Try to avoid "squashing" and "force-update" in commit history
  • CI time didn't increase

See: Pull Request Guidelines

Copilot AI review requested due to automatic review settings October 17, 2025 05:55
@jadhavrohit924 jadhavrohit924 requested review from a team as code owners October 17, 2025 05:55
@github-actions github-actions bot added examples app examples chef Changes in examples/chef labels Oct 17, 2025
@pullapprove pullapprove bot added review - pending and removed examples app examples chef Changes in examples/chef labels Oct 17, 2025
Copilot AI reviewed Oct 17, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

The PR decouples the ICD Management cluster to be code driven by migrating from a ZAP/Ember-based implementation to a modern DefaultServerCluster approach. This transformation makes the cluster more maintainable and testable while aligning with the project's architectural goals.

Key changes:

  • Converted from AttributeAccessInterface pattern to DefaultServerCluster
  • Created separate ICDManagementCluster and ICDManagementClusterWithCIP classes for different feature sets
  • Added comprehensive unit tests for cluster functionality
  • Moved ClusterRevision from RAM-based storage to callback-based implementation

Reviewed Changes

Copilot reviewed 40 out of 40 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/app/clusters/icd-management-server/ICDManagementCluster.h Redesigned class hierarchy with base ICDManagementCluster and CIP-enabled subclass
src/app/clusters/icd-management-server/ICDManagementCluster.cpp Implemented new cluster logic with DefaultServerCluster pattern
src/app/clusters/icd-management-server/CodegenIntegration.cpp Added integration layer for codegen cluster registration
src/app/clusters/icd-management-server/tests/TestICDManagementCluster.cpp Added unit tests for cluster attributes and commands
zzz_generated/app-common/app-common/zap-generated/callback.h Removed legacy command callbacks
zzz_generated/app-common/app-common/zap-generated/attributes/Accessors.h Removed ClusterRevision accessor functions
examples/*.matter Updated ClusterRevision from RAM to callback attribute across all examples

@jadhavrohit924 jadhavrohit924 mentioned this pull request Oct 17, 2025
Closed
7 tasks
gemini-code-assist[bot]
gemini-code-assist bot reviewed Oct 17, 2025
View reviewed changes
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request effectively refactors the ICD Management cluster to be a code-driven implementation, which is a great improvement. The changes align well with the established patterns for code-driven clusters in this repository, including the addition of unit tests. The overall structure is clean and well-organized. I have a couple of minor suggestions to further improve code quality and test coverage.

@jadhavrohit924 jadhavrohit924 changed the title Decouple ICD Management cluster to be code driven. [Part3] Decouple ICD Management cluster to be code driven. Oct 17, 2025
@github-actions
Copy link

PR #41499: Size comparison from a95f163 to c50365d

Full report (1 build for stm32)
platform target config section a95f163 c50365d change % change
stm32 light STM32WB5MM-DK FLASH 469812 469812 0 0.0
RAM 141248 141248 0 0.0

@github-actions github-actions bot added examples app examples chef Changes in examples/chef labels Oct 17, 2025
@github-actions
Copy link

github-actions bot commented Oct 17, 2025 •
edited
Loading

PR #41499: Size comparison from a95f163 to b66acf8

Increases above 0.2%:

platform target config section a95f163 b66acf8 change % change
nxp contact mcxw71+release FLASH 691400 693240 1840 0.3
RAM 61424 61584 160 0.3
lock mcxw71+release FLASH 773168 775016 1848 0.2
RAM 61868 62020 152 0.2
telink light-switch-app-ota-compress-lzma-factory-data tl7218x_retention RAM 51736 51900 164 0.3
light-switch-app-ota-factory-data tl3218x_retention RAM 34484 34648 164 0.5
Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
platform target config section a95f163 b66acf8 change % change
bl602 lighting-app bl602+mfd+littlefs+rpc FLASH 1106366 1106366 0 0.0
RAM 178802 178802 0 0.0
bl702 lighting-app bl702+eth FLASH 660956 660956 0 0.0
RAM 134881 134881 0 0.0
bl702+wifi FLASH 837068 837068 0 0.0
RAM 124349 124349 0 0.0
bl706+mfd+rpc+littlefs FLASH 1070036 1070036 0 0.0
RAM 117189 117189 0 0.0
bl702l contact-sensor-app bl702l+mfd+littlefs FLASH 898878 899798 920 0.1
RAM 105468 105676 208 0.2
lighting-app bl702l+mfd+littlefs FLASH 983054 983054 0 0.0
RAM 109676 109676 0 0.0
cc13x4_26x4 lighting-app LP_EM_CC1354P10_6 FLASH 770364 770364 0 0.0
RAM 103240 103240 0 0.0
lock-ftd LP_EM_CC1354P10_6 FLASH 782120 783272 1152 0.1
RAM 108400 108560 160 0.1
pump-app LP_EM_CC1354P10_6 FLASH 727940 727940 0 0.0
RAM 97308 97308 0 0.0
pump-controller-app LP_EM_CC1354P10_6 FLASH 712384 712384 0 0.0
RAM 97508 97508 0 0.0
cc32xx air-purifier CC3235SF_LAUNCHXL FLASH 554066 554066 0 0.0
RAM 205504 205504 0 0.0
lock CC3235SF_LAUNCHXL FLASH 587318 588486 1168 0.2
RAM 205768 205920 152 0.1
efr32 lock-app BRD4187C FLASH 962192 963640 1448 0.2
RAM 126268 126428 160 0.1
BRD4338a FLASH 757728 759616 1888 0.2
RAM 255540 255692 152 0.1
window-app BRD4187C FLASH 1057460 1059332 1872 0.2
RAM 122464 122592 128 0.1
esp32 all-clusters-app c3devkit DRAM 103192 103192 0 0.0
FLASH 1796424 1796424 0 0.0
IRAM 83862 83862 0 0.0
nrfconnect all-clusters-app nrf52840dk_nrf52840 FLASH 933264 933264 0 0.0
RAM 161069 161069 0 0.0
nxp contact mcxw71+release FLASH 691400 693240 1840 0.3
RAM 61424 61584 160 0.3
lighting mcxw71+release FLASH 722896 722896 0 0.0
RAM 68084 68084 0 0.0
lock mcxw71+release FLASH 773168 775016 1848 0.2
RAM 61868 62020 152 0.2
psoc6 all-clusters cy8ckit_062s2_43012 FLASH 1676484 1676484 0 0.0
RAM 213660 213660 0 0.0
all-clusters-minimal cy8ckit_062s2_43012 FLASH 1592556 1592556 0 0.0
RAM 210956 210956 0 0.0
light cy8ckit_062s2_43012 FLASH 1459172 1459172 0 0.0
RAM 197656 197656 0 0.0
lock cy8ckit_062s2_43012 FLASH 1491724 1493204 1480 0.1
RAM 225376 225528 152 0.1
qpg lighting-app qpg6200+debug FLASH 836552 837728 1176 0.1
RAM 127644 127804 160 0.1
lock-app qpg6200+debug FLASH 773252 774724 1472 0.2
RAM 118620 118780 160 0.1
realtek light-switch-app rtl8777g FLASH 706224 707696 1472 0.2
RAM 106800 106960 160 0.1
lighting-app rtl8777g FLASH 757320 757320 0 0.0
RAM 127164 127164 0 0.0
stm32 light STM32WB5MM-DK FLASH 469812 469812 0 0.0
RAM 141248 141248 0 0.0
telink bridge-app tl7218x FLASH 710462 710462 0 0.0
RAM 90436 90436 0 0.0
light-app-ota-compress-lzma-shell-factory-data tl3218x FLASH 796848 796848 0 0.0
RAM 40936 40936 0 0.0
light-app-ota-shell-factory-data tl7218x FLASH 788048 788048 0 0.0
RAM 93580 93580 0 0.0
light-switch-app-ota-compress-lzma-factory-data tl7218x_retention FLASH 714974 716154 1180 0.2
RAM 51736 51900 164 0.3
light-switch-app-ota-compress-lzma-shell-factory-data tlsr9528a FLASH 748278 749458 1180 0.2
RAM 70784 70940 156 0.2
light-switch-app-ota-factory-data tl3218x_retention FLASH 725126 726306 1180 0.2
RAM 34484 34648 164 0.5
lighting-app-ota-factory-data tlsr9118bdk40d FLASH 602366 602366 0 0.0
RAM 108628 108628 0 0.0
lighting-app-ota-rpc-factory-data-4mb tlsr9518adk80d FLASH 820668 820672 4 0.0
RAM 91976 91976 0 0.0

@codecov
Copy link

codecov bot commented Oct 17, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 58.06452% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.02%. Comparing base (a95f163) to head (b66acf8).
⚠️ Report is 54 commits behind head on master.

Files with missing lines Patch % Lines
...ers/icd-management-server/ICDManagementCluster.cpp 59.01% 25 Missing ⚠️
src/app/util/util.cpp 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##           master   #41499   +/-   ##
=======================================
  Coverage   51.01%   51.02%           
=======================================
  Files        1386     1387    +1     
  Lines      100988   101054   +66     
  Branches    13081    13085    +4     
=======================================
+ Hits        51519    51558   +39     
- Misses      49469    49496   +27

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

bzbarsky-apple
bzbarsky-apple requested changes Oct 17, 2025
View reviewed changes
andy31415
andy31415 reviewed Oct 17, 2025
View reviewed changes
Copilot AI review requested due to automatic review settings October 27, 2025 08:54
Copilot AI reviewed Oct 27, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 40 out of 40 changed files in this pull request and generated 3 comments.

andy31415
andy31415 reviewed Oct 27, 2025
View reviewed changes
andy31415
andy31415 reviewed Oct 27, 2025
View reviewed changes
andy31415
andy31415 reviewed Oct 27, 2025
View reviewed changes
andy31415
andy31415 reviewed Oct 27, 2025
View reviewed changes
andy31415
andy31415 reviewed Oct 27, 2025
View reviewed changes
andy31415
andy31415 reviewed Oct 27, 2025
View reviewed changes
andy31415
andy31415 reviewed Oct 27, 2025
View reviewed changes
andy31415
andy31415 reviewed Oct 27, 2025
View reviewed changes
andy31415
andy31415 reviewed Oct 27, 2025
View reviewed changes
andy31415
andy31415 reviewed Oct 27, 2025
View reviewed changes
andy31415
andy31415 reviewed Oct 27, 2025
View reviewed changes
andy31415
andy31415 reviewed Oct 27, 2025
View reviewed changes
andy31415
andy31415 reviewed Oct 27, 2025
View reviewed changes
src/app/clusters/icd-management-server/tests/TestICDManagementCluster.cpp
ASSERT_EQ(cluster.Attributes(ConcreteClusterPath(kRootEndpointId, IcdManagement::Id), attributesBuilder), CHIP_NO_ERROR);

// Calculate expected attributes based on feature map and configuration
BitFlags<IcdManagement::Feature> featureMap = icdConfig.GetFeatureMap();
Copy link
Contributor

Choose a reason for hiding this comment

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

we create he clusters here ... can't we control the feature map?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. Features are controlled by config options only. Different tests have different configs enabled so we have to read the feature map from icd configuration.

andy31415
andy31415 reviewed Oct 27, 2025
View reviewed changes
src/app/clusters/icd-management-server/tests/TestICDManagementCluster.cpp
BitMask<IcdManagement::OptionalCommands>(IcdManagement::OptionalCommands::kStayActive);
BitMask<IcdManagement::UserActiveModeTriggerBitmap> userActiveModeTriggerHint(0);

#if CHIP_CONFIG_ENABLE_ICD_CIP
Copy link
Contributor

Choose a reason for hiding this comment

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

Some explanation on why no separate step should be added here ... we could in theory test both (or just one) every time without ifdefs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, without ifdefs we can test both everytime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have added ICDManagementClusterWithCIP class under config, so we need ifdefs

bzbarsky-apple
bzbarsky-apple reviewed Oct 28, 2025
View reviewed changes
Copilot AI review requested due to automatic review settings October 29, 2025 03:21
Copilot AI reviewed Oct 29, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 40 out of 40 changed files in this pull request and generated 4 comments.

@github-actions
Copy link

github-actions bot commented Oct 29, 2025 •
edited
Loading

PR #41499: Size comparison from 480a0dd to 6682fc8

Increases above 0.2%:

platform target config section 480a0dd 6682fc8 change % change
nxp contact mcxw71+release FLASH 691936 693528 1592 0.2
RAM 61496 61648 152 0.2
lock mcxw71+release FLASH 773704 775296 1592 0.2
RAM 61932 62084 152 0.2
telink light-switch-app-ota-compress-lzma-factory-data tl7218x_retention RAM 51852 52004 152 0.3
light-switch-app-ota-factory-data tl3218x_retention RAM 34600 34752 152 0.4
Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
platform target config section 480a0dd 6682fc8 change % change
bl602 lighting-app bl602+mfd+littlefs+rpc FLASH 1106076 1106076 0 0.0
RAM 178882 178882 0 0.0
bl702 lighting-app bl702+eth FLASH 661228 661228 0 0.0
RAM 134969 134969 0 0.0
bl702+wifi FLASH 836764 836764 0 0.0
RAM 124405 124405 0 0.0
bl706+mfd+rpc+littlefs FLASH 1070308 1070308 0 0.0
RAM 117277 117277 0 0.0
bl702l contact-sensor-app bl702l+mfd+littlefs FLASH 899760 900778 1018 0.1
RAM 105540 105732 192 0.2
lighting-app bl702l+mfd+littlefs FLASH 983070 983070 0 0.0
RAM 109756 109756 0 0.0
cc13x4_26x4 lighting-app LP_EM_CC1354P10_6 FLASH 770436 770436 0 0.0
RAM 103304 103304 0 0.0
lock-ftd LP_EM_CC1354P10_6 FLASH 782184 783152 968 0.1
RAM 108472 108616 144 0.1
pump-app LP_EM_CC1354P10_6 FLASH 728244 728244 0 0.0
RAM 97364 97364 0 0.0
pump-controller-app LP_EM_CC1354P10_6 FLASH 712704 712704 0 0.0
RAM 97580 97580 0 0.0
cc32xx air-purifier CC3235SF_LAUNCHXL FLASH 554578 554578 0 0.0
RAM 205744 205744 0 0.0
lock CC3235SF_LAUNCHXL FLASH 587378 588362 984 0.2
RAM 205840 205992 152 0.1
efr32 lock-app BRD4187C FLASH 962976 964248 1272 0.1
RAM 123512 123672 160 0.1
BRD4338a FLASH 757128 758744 1616 0.2
RAM 254144 254304 160 0.1
window-app BRD4187C FLASH 1058172 1059772 1600 0.2
RAM 119740 119900 160 0.1
esp32 all-clusters-app c3devkit DRAM 102524 102524 0 0.0
FLASH 1836568 1836568 0 0.0
IRAM 93540 93540 0 0.0
nrfconnect all-clusters-app nrf52840dk_nrf52840 FLASH 933180 933180 0 0.0
RAM 161317 161317 0 0.0
nxp contact mcxw71+release FLASH 691936 693528 1592 0.2
RAM 61496 61648 152 0.2
lighting mcxw71+release FLASH 723440 723440 0 0.0
RAM 68148 68148 0 0.0
lock mcxw71+release FLASH 773704 775296 1592 0.2
RAM 61932 62084 152 0.2
psoc6 all-clusters cy8ckit_062s2_43012 FLASH 1677556 1677556 0 0.0
RAM 213908 213908 0 0.0
all-clusters-minimal cy8ckit_062s2_43012 FLASH 1593980 1593980 0 0.0
RAM 211116 211116 0 0.0
light cy8ckit_062s2_43012 FLASH 1460108 1460108 0 0.0
RAM 197736 197736 0 0.0
lock cy8ckit_062s2_43012 FLASH 1492644 1493916 1272 0.1
RAM 225448 225600 152 0.1
qpg lighting-app qpg6200+debug FLASH 837216 838192 976 0.1
RAM 127716 127868 152 0.1
lock-app qpg6200+debug FLASH 773996 775268 1272 0.2
RAM 118692 118836 144 0.1
realtek light-switch-app rtl8777g FLASH 706416 707704 1288 0.2
RAM 106904 107056 152 0.1
lighting-app rtl8777g FLASH 757512 757512 0 0.0
RAM 127236 127236 0 0.0
stm32 light STM32WB5MM-DK FLASH 469892 469892 0 0.0
RAM 141312 141312 0 0.0
telink bridge-app tl7218x FLASH 710554 710554 0 0.0
RAM 90544 90544 0 0.0
light-app-ota-compress-lzma-shell-factory-data tl3218x FLASH 796908 796908 0 0.0
RAM 41008 41008 0 0.0
light-app-ota-shell-factory-data tl7218x FLASH 788108 788108 0 0.0
RAM 93644 93644 0 0.0
light-switch-app-ota-compress-lzma-factory-data tl7218x_retention FLASH 714986 716004 1018 0.1
RAM 51852 52004 152 0.3
light-switch-app-ota-compress-lzma-shell-factory-data tlsr9528a FLASH 748282 749300 1018 0.1
RAM 70892 71052 160 0.2
light-switch-app-ota-factory-data tl3218x_retention FLASH 725138 726156 1018 0.1
RAM 34600 34752 152 0.4
lighting-app-ota-factory-data tlsr9118bdk40d FLASH 602540 602540 0 0.0
RAM 108912 108912 0 0.0
lighting-app-ota-rpc-factory-data-4mb tlsr9518adk80d FLASH 820712 820716 4 0.0
RAM 92040 92040 0 0.0

@bzbarsky-apple bzbarsky-apple mentioned this pull request Oct 31, 2025
Open
@yunhanw-google
Copy link
Contributor

please hold on merge for this one, waiting for #41725, I would cherry-pick 41725 into 1.4 and 1.4.2, then please update the MatterReportingAttributeChangeCallback with new API in this PR. thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andy31415 andy31415 andy31415 left review comments

Copilot code review Copilot Copilot left review comments

@bzbarsky-apple bzbarsky-apple bzbarsky-apple approved these changes

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

app examples chef Changes in examples/chef examples review - pending

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ServerClusterInterface: decouple ICD cluster.

4 participants

@jadhavrohit924 @yunhanw-google @andy31415 @bzbarsky-apple