[Bugfix] add GetInstance method instead of mInstance member #40412
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
…ected section instead of it (as same as other cluster's pattern)
|
|
There was a problem hiding this comment.
Code Review
This pull request refactors the Delegate class in the ModeBase cluster to improve encapsulation and align with patterns used in other clusters. The changes involve moving mInstance and SetInstance to be private, and adding a new protected GetInstance method. The logic is sound and this is a good improvement. I have one suggestion to add documentation for the new method to enhance maintainability.
|
PR #40412: Size comparison from f263ab7 to e9ee5a4 Full report (12 builds for cc13x4_26x4, cc32xx, nrfconnect, qpg, stm32, tizen)
|
|
PR #40412: Size comparison from f263ab7 to 0d2936d Full report (6 builds for cc32xx, nrfconnect, stm32, tizen)
|
|
PR #40412: Size comparison from f263ab7 to 2a56e1c Full report (57 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #40412: Size comparison from f263ab7 to 6d33bee Full report (2 builds for nrfconnect, stm32)
|
|
PR #40412: Size comparison from 8b40f17 to 6c5ef72 Full report (30 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, psoc6, qpg, stm32, telink, tizen)
|
…r' of github.com:ehsanziyaee/connectedhomeip into bugfix_add-GetInstance-method-instead-of-Instance-member
|
PR #40412: Size comparison from 9bd6b4b to c1c6275 Full report (30 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, psoc6, qpg, stm32, telink, tizen)
|
|
PR #40412: Size comparison from 9bd6b4b to 5b2ef26 Full report (22 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, psoc6, qpg, stm32, tizen)
|
|
PR #40412: Size comparison from a72f457 to 2290798 Full report (57 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the ModeBase::Delegate to improve encapsulation by making the mInstance member private and adding a GetInstance() accessor. The changes are correctly propagated to the example applications.
My review has two main points:
- In
mode-base-server.h, the newGetInstance()only returns aconstpointer, which breaks the documented contract for delegates that need to call non-const methods on the instance (likeReportSupportedModesChange()). I've suggested adding a non-const overload to fix this. - There's an inconsistency in the refactoring within the
chefexample. WhileOvenModewas nicely refactored to cache theendpointId,MicrowaveOvenModewas not. I've recommended applying the same pattern for consistency.
Overall, this is a good change for improving the API design. Addressing the feedback will make it more robust and consistent.
examples/chef/common/clusters/microwave-oven-mode/chef-microwave-oven-mode.cpp
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a GetInstance() method to the ModeBase::Delegate class, improving encapsulation by making the mInstance member private. This is a good architectural improvement that aligns ModeBase with other clusters. The changes in most example files correctly adapt to this new accessor.
However, I've found a critical issue in the changes for the chef-oven-mode example. A small refactoring to cache the endpointId was introduced, but it leads to a null pointer dereference during initialization, which will cause a crash. I've left detailed comments and suggestions in examples/chef/common/clusters/oven-mode/chef-oven-mode.h and examples/chef/common/clusters/oven-mode/chef-oven-mode.cpp to resolve this. The fix involves reverting this small refactoring and instead calling GetInstance()->GetEndpointId() where needed, which also makes the changes consistent with other files in this PR.
|
PR #40412: Size comparison from 7963c96 to eda771c Full report (12 builds for cc13x4_26x4, cc32xx, nrfconnect, qpg, realtek, stm32)
|
|
PR #40412: Size comparison from 7963c96 to 60b3753 Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the ModeBase::Delegate class to improve encapsulation by making the mInstance member private and introducing a protected GetInstance() accessor method. This change aligns the ModeBase cluster with existing patterns in other clusters, as described in the pull request. The updates in the example applications correctly adopt the new GetInstance() method. The change is well-implemented and improves code quality. I have one minor suggestion to improve documentation consistency.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the ModeBase::Delegate class to improve encapsulation by making the mInstance member private and introducing a protected GetInstance() accessor. This is a positive change that aligns with C++ best practices. The necessary updates in the example applications to use the new accessor are also included and appear correct. I have one minor suggestion to remove a duplicated comment to enhance code maintainability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly refactors the Delegate class in the ModeBase cluster to improve encapsulation by making the mInstance member private and providing a GetInstance() accessor. The necessary updates in the example applications are also included. This is a good improvement for code maintainability. I have one minor suggestion to fix a typo in a comment.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the ModeBase::Delegate class to improve encapsulation by making the mInstance member private and providing a protected GetInstance() accessor. This aligns the implementation with patterns used in other clusters, enhancing code consistency. The changes are correctly propagated to the example applications, which now use the new accessor. The modifications are a good improvement to the codebase, and I have no further feedback.
|
PR #40412: Size comparison from 7963c96 to 80462b8 Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #40412: Size comparison from db54d1a to 7cdbdd0 Full report (33 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, psoc6, qpg, realtek, stm32, telink)
|
andy31415
approved these changes
Sep 22, 2025
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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
1 - I saw that there is no GetInstance function in the protected section of the Delegate class of ModeBase cluster, like other clusters' structure pattern. So I opened an issue and its related PR.
2- Some examples, such as 'Chef', that used this cluster were also edited.
3- The 'SetInstace' function was moved to the public section of the delegate class.
4- The 'Instance class' member was moved to the private section of the delegate class.
Related issues
Testing
Readability checklist
The checklist below will help the reviewer finish PR review in time and keep the
code readable:
descriptive
“When in Rome…”
rule (coding style)
See: Pull Request Guidelines