Remove the SoftwareDiagnostics "LOGIC" template from clusters #39977
+50
−71
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
src/app/clusters/software-diagnostics-server/CodegenIntegration.cpp
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Pull Request Overview
This PR removes the templated logic injection in the Software Diagnostics cluster and consolidates all diagnostic calls to the DeviceLayer singleton, simplifying both production and test code.
- Forward diagnostic provider calls directly to
DeviceLayer::GetDiagnosticDataProvider() - Eliminate
DeviceLayerSoftwareDiagnosticsLogicandInjectedDiagnosticsSoftwareDiagnosticsLogicclasses and the cluster’s template parameter - Introduce
ScopedDiagnosticsProviderin tests to temporarily override the global provider
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/app/clusters/software-diagnostics-server/software-diagnostics-logic.h | Switched logic methods to call the DeviceLayer singleton and removed provider-injection classes |
| src/app/clusters/software-diagnostics-server/software-diagnostics-logic.cpp | Updated thread‐metrics and accepted‐commands calls to use DeviceLayer::GetDiagnosticDataProvider() |
| src/app/clusters/software-diagnostics-server/software-diagnostics-cluster.h | Dropped the LOGIC template parameter and updated all calls to use direct methods on SoftwareDiagnosticsLogic |
| src/app/clusters/software-diagnostics-server/tests/TestSoftwareDiagnosticsCluster.cpp | Added ScopedDiagnosticsProvider helper and simplified test logic instantiation |
| src/app/clusters/software-diagnostics-server/CodegenIntegration.cpp | Adjusted LazyRegisteredServerCluster instantiation to the non‐templated cluster |
Comments suppressed due to low confidence (3)
src/app/clusters/software-diagnostics-server/tests/TestSoftwareDiagnosticsCluster.cpp:90
- [nitpick] The variable
nullProvideris now aScopedDiagnosticsProvider<NullProvider>, which can be confusing. Consider renaming it toscopedNullProviderornullProviderOverridefor clarity.
ScopedDiagnosticsProvider<NullProvider> nullProvider;
src/app/clusters/software-diagnostics-server/tests/TestSoftwareDiagnosticsCluster.cpp:122
- [nitpick] This variable wraps the provider in an RAII scope but is named
watermarksProvider. Consider renaming it toscopedWatermarksProviderto reflect its behavior.
ScopedDiagnosticsProvider<WatermarksProvider> watermarksProvider;
src/app/clusters/software-diagnostics-server/tests/TestSoftwareDiagnosticsCluster.cpp:179
- [nitpick] The name
allProviderdoes not convey that this is a scoped override. Consider renaming toscopedAllProviderorallProviderOverridefor consistency.
ScopedDiagnosticsProvider<AllProvider> allProvider;
src/app/clusters/software-diagnostics-server/software-diagnostics-logic.h
Show resolved
Hide resolved
src/app/clusters/software-diagnostics-server/software-diagnostics-cluster.h
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Code Review
This pull request refactors the Software Diagnostics cluster to remove the template-based logic provider, simplifying the design and improving readability by directly using the injectable DeviceLayer singleton. The changes are well-contained and the test adjustments using the new ScopedDiagnosticsProvider are appropriate. I have one suggestion to improve the robustness of the new test helper class.
src/app/clusters/software-diagnostics-server/tests/TestSoftwareDiagnosticsCluster.cpp
Show resolved
Hide resolved
…eDiagnosticsCluster.cpp Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…dhomeip into remove_template_logic
zaid-google
approved these changes
Jul 11, 2025
|
PR #39977: Size comparison from 48704fe to 1d37a89 Full report (59 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
tersal
reviewed
Jul 11, 2025
src/app/clusters/software-diagnostics-server/software-diagnostics-cluster.h
Outdated
Show resolved
Hide resolved
andy31415
commented
Jul 12, 2025
src/app/clusters/software-diagnostics-server/software-diagnostics-cluster.h
Outdated
Show resolved
Hide resolved
|
PR #39977: Size comparison from 48704fe to 140301e Increases above 0.2%:
Full report (56 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, nxp, psoc6, qpg, stm32, telink, tizen)
|
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.
Summary
This usage was built to faciliate unit testing, however the underlying devicelayer classes are already injectable, so for now it seems more natural to use the singletons directly (since we have to do that to save flash anyway).
This should make the code easier to read.
Testing
unit tests updated, expected to still pass. There is no functional change here really. Only interested in readability and no flash/ram impact.