Centralize codegen integration #40553
Merged
Conversation
This file contains hidden or 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
…12 bytes of overhead on efr32 and with much better logging
src/app/clusters/administrator-commissioning-server/CodegenIntegration.cpp
Outdated
Show resolved
Hide resolved
src/app/clusters/administrator-commissioning-server/CodegenIntegration.cpp
Outdated
Show resolved
Hide resolved
src/app/clusters/general-diagnostics-server/CodegenIntegration.cpp
Outdated
Show resolved
Hide resolved
src/app/clusters/general-diagnostics-server/CodegenIntegration.cpp
Outdated
Show resolved
Hide resolved
src/app/clusters/software-diagnostics-server/CodegenIntegration.cpp
Outdated
Show resolved
Hide resolved
src/app/clusters/wifi-network-diagnostics-server/CodegenIntegration.cpp
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request does a great job of centralizing the codegen integration logic, which significantly simplifies the cluster initialization and shutdown code in several places. The new CodegenClusterIntegration utility is well-designed and the changes to OptionalAttributeSet are a good addition to support it. I've found one critical copy-paste error in the WiFi Network Diagnostics cluster shutdown logic that needs to be addressed. Otherwise, the changes look solid.
src/app/clusters/wifi-network-diagnostics-server/CodegenIntegration.cpp
Outdated
Show resolved
Hide resolved
|
PR #40553: Size comparison from 5425fc0 to ac1b359 Full report (30 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, psoc6, qpg, stm32, telink, tizen)
|
|
PR #40553: Size comparison from 71d1100 to 2f86de2 Increases above 0.2%:
Full report (57 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
tcarmelveilleux
approved these changes
Aug 15, 2025
dsavitsky-dsr
pushed a commit
to dsavitsky-dsr/connectedhomeip
that referenced
this pull request
Aug 19, 2025
* Start some prototypes * More integration * More work * Restyle * Update attributes sets a bit to help for more generic code * Comment update * General diangostics integration: this seems to cost too much right now * Start seeing some savings afte wifi cluste migration. We are now at 212 bytes of overhead on efr32 and with much better logging * Another update, we are at 124 bytes of overhead * Code size diff is now 100% a wash * All attributes logic... * Slight API cleanup * Add some unit tests * Slightly shorter code * Restyled by clang-format * Use maxendpointcount be 1 * Fix conditional * Fix typo * Fix copy and paste * Update data type * Fix typo * Remove %u check ... althouhg this is silly... * Add file names to gni file * Fix ota provider * Also convert basic information * slight arrange * Undo submodule update * Add one more ember override in dynamic dispatcher * Restyled by clang-format * Fix ifdef not being defined * Fix typo * Fix typo * Update src/app/server-cluster/tests/TestOptionalAttributeSet.cpp Co-authored-by: Copilot <[email protected]> * Slight clarity update * Ameba does not like typeof * Also convert the pushav stream transport server * fix compiles * fix compiles * Logic fix in pushav stream server * Slight test adjustment: better comments and looks like a better test * Update src/data-model-providers/codegen/ClusterIntegration.h Co-authored-by: Sergio Soares <[email protected]> * Update src/data-model-providers/codegen/ClusterIntegration.cpp Co-authored-by: Sergio Soares <[email protected]> * Update comments * Restyled by clang-format * Update some comments to have better capitalization * More comment updates * Some review feedback: updated variable name, added more comment on destroy interface * More renames based on review feedback * A few comment updates * place logging on a configuration flag * Convert group key management cluster as well * make feature map return a value * Restyled by clang-format * Restyled by gn * Fix comment --------- Co-authored-by: Andrei Litvin <[email protected]> Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: Sergio Soares <[email protected]>
jamesharrow
added a commit
that referenced
this pull request
Sep 10, 2025
* Sixed some build errors under gcc/Linux * Revert accidentally changed line * Improvemets of the timeshift test feature in server sources * Improvemets of the timeshift test feature in example sources * Removed floor * changed default tariff sample file * Restyled * Added include file for size_t * Restyled * Update src/app/clusters/commodity-tariff-server/commodity-tariff-server.cpp Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update src/app/clusters/commodity-tariff-server/commodity-tariff-server.cpp Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update src/app/clusters/commodity-tariff-server/CommodityTariffAttrsDataMgmt.cpp Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update src/app/clusters/commodity-tariff-server/commodity-tariff-server.cpp Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update src/app/clusters/commodity-tariff-server/commodity-tariff-server.cpp Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Restyled by clang-format * Restyled fixes * Fixes by Gemini's CR notes * Restyle fix * Fixes by Gemini's CR motes * Added thr call - clearEndpointInEventTrigger * Renamed optional structs comparison helpers * Restyled fix * Restyled * Some fixes by failed CI checks * Applied some fixes by CR requests * Fixes by CR requests * Applied code style fix * The Json samples has been moved into C-header as string constans * Applied code style fix * Update CommodityTariffAttrsDataMgmt.cpp Removed commented line * Removed static_assert * Fix for #40085 & #40104 * Fix for #40098,#40100,#40103 * Fix for #40112 * Fix for #40089 * Fix for #40110 (Updated description) * Fix for #40117 * Removed some Json related sources. Added new data samples as C++ structs * server srcs. Update of DataMgmt API * example app srcs. Update of DataMgmt API * Fixes/Iproves for AttrDataMgmt (Change reporting behavior) * Fixes and improvements for time depends attrs update logic * Fixes and improvements in example app for time depends attrs updating * Example app. CR notes fix * Server implementation. CR notes fix * Tariff data samples upd: C++ Array Conversion with Designated Initializers * Improvements and fixes for server sources * Improvements and fixes for example app * Fixed build issues for linux * server srcs. Small code improvements * Server srcs. Fixes/Improvements for data mgmt template * Fix for: #40120, #40121, #40122 - used AttrId type, removed some redundant args In Attrs update flow methods Fix for: #40087 - fixed IsStruct trait Fix for: #40109, #40113, #40114, #40116, #40118, #40123 - Fixed issues with CTC_BaseDataClass removed some redundant API calls Fix for: #40124,#40125,#40126 - the values comparison performs only with to operators == / != * Fix for: #40091,#40105,#40111- the types has renamed * Server srcs. Fix for: #40111 - types has renaming * Fixes for Gemini's CR notes * Update examples/energy-gateway-app/commodity-tariff/include/CommodityTariffSamples.h Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Server srcs. code style fix * Example srcs. code style fix * Fixes for Gemini's CR notes. upd * Fixes to support some declared thread-safe features: #40107,#40108, Fix by comment from #40085 * Server srcs. code style fix * Server srcs. Upd by CR nootes * example srcs. The issue ##40128 probably fixed here * Example app. Restyled * Sync with upstream. Conflict fixes * Fixed the field type in tariff samples * Server srcs. Fixed linux build issue * small style fix * Server srcs. Restyled * Server srcs. Some changes related with fix for the isse #39961 * Example app srcs. Some changes related with fix for the isse #39961 * Server srcs. Fixed errs at building for ESP32 * Added CTCs sources paths into ESP32 project cmake * Srv srcs. Code fixes and improvements * The map and unordered_set types has been allowed for CTC in linter checks * Enable Energy-gateway and Energy Management apps run ESP32 CI (#40327) * Made Energy-gateway and Energy Management apps run ESP32 CI * Update .github/workflows/examples-esp32.yaml Co-authored-by: Copilot <[email protected]> * Updated examples-esp32.yaml so it only runs the build if there have been changes in particular folders. * Test commit to check if code changes trigger build of EGA / EMA. Will revert this commit later. * Restyled by clang-format * Revert "Test commit to check if code changes trigger build of EGA / EMA. Will revert this commit later." This reverts commit 797e2d8. --------- Co-authored-by: Copilot <[email protected]> Co-authored-by: Restyled.io <[email protected]> * [OpenSSL] Removing deprecated functions + Adding missing nullptr check (#40590) * changing deprecated functions to new functions * Adding nullptr check and stop using strlen * integrate gemini comments * cast int to size_t since triggering macOs build error * Automating TC-LWM-2.1 (step 7) and updating pics (#39972) * Automating step 7 and updating pics * Restyle * Restyle * Move defaults vars to its own file. (#39696) * Moved _DEFAULTS_ to single file * Update file name * Trying new import type * Fix lint * Removed _ prefix from DEFAULT variables * Updated name from matter_global_defaults to matter_testing_defaults * isort fix * Update matter_test_config to use variables from matter_testing_defaults * Restyled by isort * Restyled by isort * Conflict typo * Fix from CI jo * Restyled by gn * Restyled by isort * Fix import path * Restyled by gn * Restyled by isort * Update file name to match new path matter/testing instead of chip * Restyled by gn --------- Co-authored-by: Restyled.io <[email protected]> * esp32: remove unused Wi-Fi AP settings (#39734) * Enable HardPrivacy mode in the CameraDevice to enable optional attribute HardPrivacyModeOn. (#40598) This is to allow a successful wildcard read of all attributes on the AVSM cluster. * Increase unit test code coverage of `credentials/attestation_verifier/` by 4.3% (#40540) * Add tests for PartialDACVerifier - Added tests to check PartialDACVerifier with invalid parameters (empty attestation info) and with. - Added tests to check PartialDACVerifier with an overly large attestationElements buffer. - Updated build.gn to include the new test file in the build. * Refactor callback function formatting and add comments * Add tests for the DacOnlyPartialAttestationVerifier - TestWithValidAttestation: Tests verifier with valid certificate assets - TestWithVIDPIDMismatch: Tests handling of mismatched Vendor/Product IDs - TestWithNonceMismatch: Tests nonce validation scenarios - TestWithValidDACButInvalidPAI: Tests invalid PAI certificate handling - TestWithMismatchedVendorIDs: Tests DAC/PAI vendor ID mismatch detection - TestPassingVIDPIDChecks: Tests certificates that pass VID/PID validation - TestWithMatchingPAIAndDACProductIDs: Tests matching product ID scenarios - TestWithMismatchedPAIAndDACProductIDs: Tests product ID mismatch detection - TestWithInvalidAttestationSignatureFormat: Tests oversized signature handling * Add comments for readability - Added comments for improve code readbility - Removed a useless test case - Refactored some part of the code * Refactor TestDacOnlyPartialAttestationVerifier to reduce boilerplate - Moved common objects (PartialDACVerifier, attestationResult, attestationCallback) into the test fixture as member variables. - Updated all test cases to use these members directly. - This makes the tests cleaner, easier to maintain, and follows the style guide's advice to avoid repetition. * Cleanup and fix unit tests - Deleted all useless and incorrect tests that did not exercise the intended code paths. - Added missing tests to cover important error conditions and certificate validation scenarios. - Replaced hardcoded vendor/product IDs with descriptive constants for clarity. - Improved naming of alternate vendor/product ID constants for mismatch scenarios. - Changed some comments * Migrate Group Key Management Cluster to be Code Driven (#40504) * Initial seperation of cluster * Update config * Codegen * Complete initial cluster impl, update build files * Add codegen integration * Fix build issues * Use codegen mandatory attributes * Use anon namespace, remove old cluster code * Add unit tests * Restyled by whitespace * Restyled by clang-format * Restyled by gn * Remove old header includes * Remove unused variable * Update BUILD.gn for test * Update config and CodegenIntegration * Codegen * Remove unneeded chip::app references * Restyled by clang-format * Codegen * Add NotifyAttributeChangedIfSuccess on WriteAttribute * Fix CodegenIntegration callback * Update src/app/clusters/group-key-mgmt-server/group-key-mgmt-cluster.cpp Co-authored-by: Andrei Litvin <[email protected]> * Address review comments * Restyled by whitespace * Restyled by clang-format --------- Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Andrei Litvin <[email protected]> * [Test Update] ACL_2_8 test module changes for testing ACL override List method (#40238) * Rebasing changes from master into TC_ACL_2_8_Update and squashing commits Adding support for flag forceLegacyListEncoding, useful when writing Attributes with Legacy Write Request Combining prior commits into one im order to merge latest changes WIP: Example of forcing old Write Encoding fixing typehint Update TC_ACL_2_3.py - Added force_legacy_encoding parameter to TC_ACL_2_3 test - Added support for controlling the forceLegacyListEncoding parameter in the TC_ACL_2_3 test through the existing --bool-arg CLI infrastructure. The parameter defaults to True if not specified and can be set via: --bool-arg force_legacy_encoding:false - This allows testing both legacy and non-legacy list encoding behaviors through the test runner. Update TC_ACL_2_3.py Changed expected result for test step 18, as the result should be D_OK_EMPTY if forced legacy encoding, otherwise it should be D_OK_FULL if using the new encoding restyle Updated TC_ACL_2_3 test module: - Changing to establishing a new local function to contain test code - Running the new local function from the main test function, changing the bool value for the force_legacy_encoding var between iterations Restyled by autopep8 clang-tidy fixes: use make_unique to create unique_ptrs making forceLegacyListEncoding default to False Updating TC_ACL_2_8 python3 test module: - Added legacy mode rerun loop to verify that the test will work for both the new and legacy list write methods. - Added test step 11 to show that the test will be rerun after the new list method has completed. - Added resetting the fabrics inbetween the loops of running the new and legacy write list test runs. - This is for easier review of modified ACL_2_6 test module for override PR here: [38693](#38693) - Test Plan PR Link: *To be added* Restoring minor changes noticed during rebase * Update TC_ACL_2_3.py * Update TC_ACL_2_3.py * Stylizer fixes applied * Resolving linting error * Combining teardown th logic between tests into an async function as suggested by Gemini AI * Refactor test steps 9 and 10 in TC_ACL_2_8 to remove code duplication and improve maintainability: - Unified event extraction and verification logic for both legacy and new encoding modes - Always verify the initial 'added' event outside the encoding-specific logic - Consolidated fabricIndex checks into a single loop for all relevant events - Improved readability and maintainability by reducing duplicated code in steps 9 (TH1) and 10 (TH2) * Re-adding test step 11 * Restyled by autopep8 * Updating verbiage for expected results for test steps 9 and 10 --------- Co-authored-by: Alami-Amine <[email protected]> Co-authored-by: Restyled.io <[email protected]> * [Test Update] Updating TC_ACL_2_6 test module to include validation for legacy override list write method (#40267) * Updating TC_ACL_2_6 test module to include validation for legacy override list write method: - Update ACL_2_6 python3 test module to add override for list write methods - Updated test steps to match override for legacy and new write lists methods - Created test loop to run test with override then without override - Updated test step 5 to validate override works as expected for this test - Test Plan PR Link: 5183 (Already Merged) * autopep8 and isort fixes implemented * Apply suggestions from code review by Gemini Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Updating test step 5 to remove redundant check no longer needed * Refactor: add _validate_event_fields helper for TC_ACL_2_6 test step 5 event validation * Apply suggestions from code review from Amine Updating test to rerun with legacy write list encoding mechanism, instead of with the new write list encoding mechanism Co-authored-by: Amine Alami <[email protected]> * Resetting ACL events to only admin/case before re-running test with legacy list encoding mechanism * Swapping if statement order in test step 8 * Restyled by autopep8 * Added comment in code for urgency delivery flag for read event and set a new var to hold the magic value --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Amine Alami <[email protected]> Co-authored-by: Restyled.io <[email protected]> * Adding WiFi Validation (#39573) * Adding WiFi Validation * Update PICS from CNET.S to CNET.S.F00 and remove internal check * Centralize codegen integration (#40553) * Start some prototypes * More integration * More work * Restyle * Update attributes sets a bit to help for more generic code * Comment update * General diangostics integration: this seems to cost too much right now * Start seeing some savings afte wifi cluste migration. We are now at 212 bytes of overhead on efr32 and with much better logging * Another update, we are at 124 bytes of overhead * Code size diff is now 100% a wash * All attributes logic... * Slight API cleanup * Add some unit tests * Slightly shorter code * Restyled by clang-format * Use maxendpointcount be 1 * Fix conditional * Fix typo * Fix copy and paste * Update data type * Fix typo * Remove %u check ... althouhg this is silly... * Add file names to gni file * Fix ota provider * Also convert basic information * slight arrange * Undo submodule update * Add one more ember override in dynamic dispatcher * Restyled by clang-format * Fix ifdef not being defined * Fix typo * Fix typo * Update src/app/server-cluster/tests/TestOptionalAttributeSet.cpp Co-authored-by: Copilot <[email protected]> * Slight clarity update * Ameba does not like typeof * Also convert the pushav stream transport server * fix compiles * fix compiles * Logic fix in pushav stream server * Slight test adjustment: better comments and looks like a better test * Update src/data-model-providers/codegen/ClusterIntegration.h Co-authored-by: Sergio Soares <[email protected]> * Update src/data-model-providers/codegen/ClusterIntegration.cpp Co-authored-by: Sergio Soares <[email protected]> * Update comments * Restyled by clang-format * Update some comments to have better capitalization * More comment updates * Some review feedback: updated variable name, added more comment on destroy interface * More renames based on review feedback * A few comment updates * place logging on a configuration flag * Convert group key management cluster as well * make feature map return a value * Restyled by clang-format * Restyled by gn * Fix comment --------- Co-authored-by: Andrei Litvin <[email protected]> Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: Sergio Soares <[email protected]> * Update Video/Snapshot streams to adjust range parameters when reusing streams during allocations (#40572) * Adjust IsCompatible for Video and Snapshot streams. * Update the Video and Snapshot streams to adjust the range parameters when an already allocated stream is being re-used. -Minor update to bitRates in scripts and device for AudioStream allocation. * Add test script to validate a narrower range setting of the SnapshotStream min/max resolution during Stream allocation reuse. * Rename variables * Fix BUILD.gn detecting host platform as linux-x86_64 on macOS (#40613) * [Camera] Refactor the AllocateVideoStream method to config camera parameters (#40606) * Refactor the AllocateVideoStream method to accept optional parameters for minimum width, height, frame rate, and bit rate. * Address review comments * Address review comments * Address review lef comment * Implementation of PushAVTransport and PushAVStreamTransportDelegate (#38331) * Implement PushAVTransport and PushAVStreamTransportDelegate. Co-Author: Tushant Yadav <[email protected]> Co-Author: Chayan Ailawalia <[email protected]> Signed-off-by: Raveendra Karu <[email protected]> * Restyled by clang-format * Restyled by gn * addressed build issue Co-Author: Tushant Yadav <[email protected]> * Implemented delegate API Co-Author: Tushant Yadav <[email protected]> * restyled and commented cluster dependent code Co-Author: Tushant Yadav <[email protected]> * Restyled by clang-format * Restyled by gn * code cleanup Co-Author: Tushant Yadav <[email protected]> * Implement PushAVTransport integration and code cleanup - Add PushAVTransport cluster support to camera-app - Remove TODO comments as dependencies are now resolved - Clean up BUILD.gn files and remove outdated comments Author: Tushant Yadav <[email protected]> * Updated namespace Author: Tushant Yadav <[email protected]> * Rebase to latest code. Signed-off-by: Raveendra Karu <[email protected]> * Restyled by clang-format * Restyled by gn * Address LINT errors. Signed-off-by: Raveendra Karu <[email protected]> * Fix compile erros to unblock CI * Fix compile error --------- Signed-off-by: Raveendra Karu <[email protected]> Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Yufeng Wang <[email protected]> * ESP32: Add APIs to get random values from secure cert partition (#40563) * ESP32: Add APIs to get random values from secure cert partition * address review from gemini * add some comments and fix names * ESP32: remove redundant c++17 flag setting from example's main component (#40586) All examples already set the C++ standard in the project-level CMakeLists.txt. The main component redundantly sets it again. As per the ESP-IDF C++ support guide, a component should only override the C++ version if it intends to use a different one. We want to keep a consistent version across the project, so the redundant setting is removed. * [Realtek] Support ci test (#40561) * [Realtek] Support ci test * [Realtek] add subset ci build * [Realtek] Add bloat report * [Realtek] Fix build error * [Realtek] Fix build error * [Realtek] Add upload reports * [Tizen] Disable CI size report since it is not necessary (#40627) * Fix Matter Refrigerator and Temperature Controlled Example (#40625) * Remove white space * Fix anchor link's Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update README.md --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * [TC_EEVSE_2.7] - Fix test script - Fails if run after another test without rebooting DUT (#40498) * Added step 4 (shuffling step numbering) - which sends Disable to EVSE to ensure it is in known state at beginning of test. * Update src/python_testing/TC_EEVSE_2_7.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update src/python_testing/TC_EEVSE_2_7.py Co-authored-by: Copilot <[email protected]> --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Copilot <[email protected]> Co-authored-by: Hasty Granbery <[email protected]> * Additional AGP 8.5.1+ NDK r28 changes (#40462) * [Linux] Disable CI size report since it is not necessary (#40633) * [Create Test] Create ACL 2 9 python3 test module (#39792) * [Create test] Create python3 test module for TC_ACL_2_9 - Create python3 test module for TC_ACL_2_9 for matter-test-scripts task #[620](project-chip/matter-test-scripts#620) - Test steps 1-11 fully implemented and functional - Currently WIP as we need to find a way to cleanup after test steps are run as this test leaves the TH with manage access only and not admin access once this test completes * Added TODO for resetting ACL back to admin rights at end of test steps * Apply suggestions from code review from Gemini-AI Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Updating TC_ACL_2_9 python3 test module: - Resolving issue noticed with latest changes suggested by Gemini-AI bot - isort and autopep8 run to resolve sytle issues noticed * Resolving linting errors * Restyled by autopep8 * Updating TC_ACL_2_9 python3 test module: - Changed to establishing TH2 and ran TH2 through the test steps in place of TH1 - Had TH1 remove TH2 fabric index at the end of the test as a final clean up step * Resolving style issues with autopep8 and isort * Removed Test_TC_ACL_2_9 yaml module and references in test runner docs * Update TC_ACL_2_9.py updating chip namespace imports to matter * Restyled by isort * Update src/python_testing/TC_ACL_2_9.py Co-authored-by: C Freeman <[email protected]> --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Restyled.io <[email protected]> Co-authored-by: C Freeman <[email protected]> * Fix crash if JSON Revocation Set is malformed (#40630) * Fix Crash if JSON Revocation Set is malformed * Adding a testcase that covers the failure * integrate AI * Integrating Comment * Removing timeout ceiling from GetConnectedDevice (#40632) * [Camera] Revert Create Video Pipeline change (#40619) * Revert Create Video Pipeline change * Update examples/camera-app/linux/src/camera-device.cpp Co-authored-by: Copilot <[email protected]> * Address gemini comments --------- Co-authored-by: Copilot <[email protected]> * Integrate PushAvStreamTransport into camera app (#40573) * Fix StreamUsage constraint checks for Audio and Video stream allocation (#40617) * Fix StreamUsage constraint checks for Audio and Video stream allocation * Adjust test scripts for the new constraints around StreamUsage enum. * Return DynamicConstraintError if the StreamUSage for the de-allocated streamID is Internal. * [Android][Docker] Upgrade Android API level 30 to 34 (#40615) Fix SDK Platform 34 download link * [Camera-Controller] Add instructions for uploading video clip via Push AV Transport (#40624) * Add instructions for uploading video clip via Push AV Transport * Update examples/camera-controller/README.md Co-authored-by: Copilot <[email protected]> * Update examples/camera-controller/README.md Co-authored-by: Copilot <[email protected]> * Update examples/camera-controller/README.md Co-authored-by: Pradip De <[email protected]> * Update examples/camera-controller/README.md Co-authored-by: Pradip De <[email protected]> * Address review comments --------- Co-authored-by: Copilot <[email protected]> Co-authored-by: Pradip De <[email protected]> * Add enum handling in AttributePersistence. (#40568) * Add enum check and unit tests * Change AttributePersistence UT * Restyled by whitespace * Restyled by clang-format * Make explicit change in handling for enums * Add missing file to BUILD.gn * Add explicit initialization to avoid static analysis error * Remove TestEnum logic * Change macros for tests * Separate template for enums * Fix header order and add additional Assert for enums --------- Co-authored-by: Restyled.io <[email protected]> * [Closures] Add secure state field validation check. (#40276) * add secure state feild validation * Add test cases * Restyled by whitespace * Restyled by clang-format --------- Co-authored-by: Restyled.io <[email protected]> * [Chef] [Minor fix] [Roomairconditioner] Fix few ZAP initialisation values for setpoint ranges (#40597) * String type has been allowed for CTC in linter checks * Example app. Restyled * Server srcs. Restyled * Updated AtrrMgmt template * Improve by CR request * Server srcs. Restyled * Just to keep changes * Added new tests path * Added actual test cases. * Added few test cases * Upd by Gemeni CR notes * Fixed a Linux build issue * Code style fix * Fixed some build erros in Linux/gcc * Another one fix for CI passing * Code style fix * Removed unused namespace * To save current changes * Fixes to passing some CI checks * Ussed fet test casses in UT set * Bixes build issue in linux env * Applied restyler fix * Few minoe fixes by Gemini CR * Added blockMode fieald in ctx * Just to same progress * Updated some lists len constraints * Added new UT case * Code style upd * Reverted an unwanted edit * Just to save last changes * Just to save last changes in example * Add docs explaining problems in ScanNetworks handling during commissioning. (#40761) * [NXP] Enable Zigbee (#40513) Signed-off-by: Mihnea Pascu <[email protected]> * [ESP32] Fix the BLE advertisement data parsing for BLE scanner (#40733) * ESP32: Fix the BLE advertisement data parsing for BLE scanner * Restyled by clang-format * AI review changes --------- Co-authored-by: Restyled.io <[email protected]> * \ No newline at end of file * \ No newline at end of file * Changed the way to finding of values for Current/Next DayEntryDate attrs * Updated the tariff data sample for NoBlock case * Small improvements in definition of attr validators helpers * Example app. Updates in attr validation context * Upd validator for Tariff Period values * Just to save last changes * Update the GetTariffComponet response * Small updates in validation * Fix to avoid build issues in Linux * Server implementation. Restyled * Example app. Restyled * Fixed comment * Added another variant for ListToMap template * Small fixes by Gemini's CR notes * Fixed log message * Fixed msg format * Restyler update * Server implementation. Removig if X-pattern macro * Just to save last changes * Fixed few critical issues * Update examples/energy-gateway-app/commodity-tariff/src/CommodityTariffEventTriggers.cpp Co-authored-by: Hasty Granbery <[email protected]> * Server sources. Fixes and improvements by last CR * Server sources. Fixed log messages to AppServer * Server sources. Fixed log messages to AppServer * Reverted path's changhing * Samll code improvements * Upd the xml * Restyled * Removed X-pattern macro * Fixed log message format * Code style fix * Code style fix * Removed asserts and fixed some comments * Fixed some build issues in ESP32 build env * Restyled by whitespace * Fixed XMLs * Update examples/energy-gateway-app/commodity-tariff/include/CommodityTariffSamples.h Co-authored-by: jamesharrow <[email protected]> * Removed unnecessary lines * ESP32 build fix * Upd log messages type * Small arg name fix * Restyled by clang-format * Patch some compile errors in commodity-tariff-server * unwrapped the X-pattern macro * Template dummies mark as not implemented * Fixed naming in function args * Added ret code for the current attr updating methods * Upd ret value for SpanCopiers<> * Removed unused func declaration * big function from a .h file moved to cpp * Jost to store last changhes * Added common GetMgmtObj method to access for all Atts mgmt objs by enum key * Restyler fix * Fixed rotating day checks * Upd xml's for MereId and CMC * Upd xml's for CTC --------- Signed-off-by: Raveendra Karu <[email protected]> Signed-off-by: Mihnea Pascu <[email protected]> Co-authored-by: Hasty Granbery <[email protected]> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Hasty Granbery <[email protected]> Co-authored-by: jamesharrow <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: Amine Alami <[email protected]> Co-authored-by: Ramiro Recchia <[email protected]> Co-authored-by: jtrejoespinoza-grid <[email protected]> Co-authored-by: Dejin <[email protected]> Co-authored-by: Pradip De <[email protected]> Co-authored-by: Lyudmila Kostanyan <[email protected]> Co-authored-by: Zaid Omer <[email protected]> Co-authored-by: Andrei Litvin <[email protected]> Co-authored-by: Jake Ororke <[email protected]> Co-authored-by: Alami-Amine <[email protected]> Co-authored-by: Andrei Litvin <[email protected]> Co-authored-by: Sergio Soares <[email protected]> Co-authored-by: Philip Gregor <[email protected]> Co-authored-by: Yufeng Wang <[email protected]> Co-authored-by: [Samsung] Raveendra Karu <[email protected]> Co-authored-by: Shubham Patil <[email protected]> Co-authored-by: Realtek-pankore <[email protected]> Co-authored-by: Arkadiusz Bokowy <[email protected]> Co-authored-by: Ludovic BOUÉ <[email protected]> Co-authored-by: C Freeman <[email protected]> Co-authored-by: Pradip De <[email protected]> Co-authored-by: Moises Terrones <[email protected]> Co-authored-by: sabollim-silabs <[email protected]> Co-authored-by: Shreyas Balakrishna Bhandare <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]> Co-authored-by: MihneaP-nxp <[email protected]> Co-authored-by: Wang Qixiang <[email protected]>
| // - Thermostat and DoorLock have more than 32 attributes in general | ||
| // - ColorControl has a lot of high-ID attributes | ||
| // | ||
| // The above examples however are few compared to the large number of clusters that matter supports, |
There was a problem hiding this comment.
"Matter", not "matter".
| { | ||
| .endpointId = endpointId, | ||
| .clusterId = AdministratorCommissioning::Id, | ||
| .fixedClusterServerEndpointCount = 1, |
There was a problem hiding this comment.
How do you know it's 1? What if endpoint 0 is a dynamic endpoint? This could be 0. Or potentially more than 1 in the presence of fabric sync, but see below.
This needs to use the right kFixedClusterConfig.size(), no?
| .endpointId = endpointId, | ||
| .clusterId = AdministratorCommissioning::Id, | ||
| .fixedClusterServerEndpointCount = 1, | ||
| .maxEndpointCount = 1, |
There was a problem hiding this comment.
This looks wrong a priori; fabric sync allows multiple Administrator Commissioning clusters on multiple endpoints, right?
Now I know the existing code does not work with that, so you can't use codegen for fabric sync, but this is at least worth documentation explaining why this value is 1.
There was a problem hiding this comment.
In this case we claim that "the implementation here only handles the root node global instance administrator commissioning" hence the value of 1: we only ever support root endpoint.
For the rest, we would error out if codegen requests it. AdministratorCommissioning logic in fabric sync installs its own versions of this cluster and it is not this cluster in particular.
There was a problem hiding this comment.
My point is we should document that here.
Comment on lines
+95
to
+96
| .fixedClusterServerEndpointCount = 1, | ||
| .maxEndpointCount = 1, |
There was a problem hiding this comment.
Again, these counts look off.
Comment on lines
+80
to
+81
| .fixedClusterServerEndpointCount = 1, | ||
| .maxEndpointCount = 1, |
There was a problem hiding this comment.
As above: the first 1 looks wrong, the second 1 needs documentation (i.e: there is only one Basic Information instance on a node; this one is simpler than Administrator Commissioning).
| /// has been validated. Validation includes: | ||
| /// - cluster exists on the given endpoint and a valid index could be found for it | ||
| /// - feature map could be loaded (if applicable, otherwise it will be set to 0) | ||
| /// - optional attributes were loaded |
| /// - optional attributes were loaded | ||
| /// | ||
| /// The returned `CreateRegistration` value will be used to register to the codegen | ||
| /// data model provider registry |
There was a problem hiding this comment.
Period at end of sentence, please. That applies to a bunch of these comments.
| EndpointId endpointId; | ||
| ClusterId clusterId; | ||
| uint16_t fixedClusterServerEndpointCount; // Number of fixed endpoints in ember configuration. | ||
| uint16_t maxEndpointCount; // This is how many endpoints are supported by the delegate (0-based index). |
| { | ||
| EndpointId endpointId; | ||
| ClusterId clusterId; | ||
| uint16_t fixedClusterServerEndpointCount; // Number of fixed endpoints in ember configuration. |
There was a problem hiding this comment.
No, this is the number of fixed endpoints that have the relevant cluster on them, not just the number of fixed endpoints.
| { | ||
| EndpointId endpointId; | ||
| ClusterId clusterId; | ||
| uint16_t fixedClusterServerEndpointCount; // Number of fixed endpoints in ember configuration. |
andy31415
added a commit
to andy31415/connectedhomeip
that referenced
this pull request
Sep 11, 2025
andy31415
added a commit
to andy31415/connectedhomeip
that referenced
this pull request
Sep 11, 2025
andy31415
added a commit
to andy31415/connectedhomeip
that referenced
this pull request
Sep 11, 2025
mergify bot
pushed a commit
that referenced
this pull request
Sep 15, 2025
* Addressing post-merge review comments from #40553 * Update src/data-model-providers/codegen/ClusterIntegration.h Co-authored-by: Copilot <[email protected]> * Update src/app/clusters/administrator-commissioning-server/CodegenIntegration.cpp Co-authored-by: Copilot <[email protected]> * Update src/app/clusters/administrator-commissioning-server/CodegenIntegration.cpp Co-authored-by: Boris Zbarsky <[email protected]> * Update src/app/clusters/administrator-commissioning-server/CodegenIntegration.cpp Co-authored-by: Boris Zbarsky <[email protected]> * Update src/data-model-providers/codegen/ClusterIntegration.h Co-authored-by: Boris Zbarsky <[email protected]> * Update src/data-model-providers/codegen/ClusterIntegration.h Co-authored-by: Boris Zbarsky <[email protected]> * Update src/data-model-providers/codegen/ClusterIntegration.h Co-authored-by: Boris Zbarsky <[email protected]> * Update emberEndpointIndex to clusterInstanceIndex * Update src/data-model-providers/codegen/ClusterIntegration.h Co-authored-by: Boris Zbarsky <[email protected]> * More renames and restyles. * Add comments about individual max instance counts. * Restyle * Fix compile. * Restyled by clang-format * Remove wrong file * Update src/data-model-providers/codegen/ClusterIntegration.h Co-authored-by: Boris Zbarsky <[email protected]> * Update src/data-model-providers/codegen/ClusterIntegration.h Co-authored-by: Boris Zbarsky <[email protected]> * Restyled by clang-format --------- Co-authored-by: Copilot <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]> Co-authored-by: Restyled.io <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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.
Summary
CodegenIntegration.cpp has several common operations:
Extracted these common operations in a central integration method. This simplifies the cluster integration code in most cases. The intent is that the more clusters we convert, the more code savings we get by sharing the common code bits.
Testing