[Chef] Fix microwave oven chef app operational state server and delegate initialisations#71410
Open
sxb427 wants to merge 31 commits intoproject-chip:masterfrom
Open
[Chef] Fix microwave oven chef app operational state server and delegate initialisations#71410sxb427 wants to merge 31 commits intoproject-chip:masterfrom
sxb427 wants to merge 31 commits intoproject-chip:masterfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a new constructor for ChefMicrowaveOvenDevice to support custom operational state instances and delegates, specifically for endpoint 1, and adds a Python integration test for microwave oven functionality. Feedback highlights safety concerns in the new constructor where pointers are dereferenced in the initializer list before validation. It is recommended to use defensive null checks instead of VerifyOrDie and to ensure that the lifecycle management of these pointers follows repository standards for global objects.
examples/chef/common/clusters/microwave-oven-control/chef-microwave-oven-control.cpp
Outdated
Show resolved
Hide resolved
examples/chef/common/clusters/microwave-oven-control/chef-microwave-oven-control.cpp
Outdated
Show resolved
Hide resolved
examples/chef/common/clusters/microwave-oven-control/chef-microwave-oven-control.cpp
Outdated
Show resolved
Hide resolved
|
PR #71410: Size comparison from 2eff734 to 57dd215 Full report (21 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, psoc6, qpg, realtek, stm32)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #71410 +/- ##
=======================================
Coverage 54.32% 54.32%
=======================================
Files 1577 1577
Lines 108271 108271
Branches 13401 13401
=======================================
Hits 58820 58820
Misses 49451 49451 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
PR #71410: Size comparison from 2eff734 to 7e9aa5b Full report (10 builds for cc13x4_26x4, cc32xx, nrfconnect, realtek, stm32)
|
|
PR #71410: Size comparison from 2eff734 to 36168af Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
examples/chef/common/clusters/microwave-oven-control/chef-microwave-oven-control.cpp
Outdated
Show resolved
Hide resolved
examples/chef/common/clusters/microwave-oven-control/chef-microwave-oven-control.cpp
Outdated
Show resolved
Hide resolved
examples/chef/common/clusters/microwave-oven-control/chef-microwave-oven-control.cpp
Outdated
Show resolved
Hide resolved
|
PR #71410: Size comparison from da57c2e to 5f0ef5e Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #71410: Size comparison from da57c2e to 6791bbc Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #71410: Size comparison from da57c2e to 0de43e3 Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #71410: Size comparison from da57c2e to 22886a3 Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
examples/chef/common/clusters/microwave-oven-control/chef-microwave-oven-control.cpp
Outdated
Show resolved
Hide resolved
|
PR #71410: Size comparison from a914062 to 8e12c9c Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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
Steps to reproduce bug
Send
set-cooking-parameterswithStartAfterSettingset to True -Actual Behaviour: saw OperationalState was kStopped and CountdownTime was NULL -
Expect an operational cycle to start but no cycle was active.
Root cause
For endpoint 1 global operational state delegate and instance objects get registered in an ember callback - https://github.com/project-chip/connectedhomeip/blob/master/examples/chef/common/chef-operational-state-delegate-impl.cpp#L293-L306
The microwave oven initialiser creates new operational state delegate and instance objects - https://github.com/project-chip/connectedhomeip/blob/master/examples/chef/common/clusters/microwave-oven-control/chef-microwave-oven-control.cpp#L36-L38. But these registrations will be ignored as operational state things were already registered on endpoint 1. So the app does not use the right objects.
The PR fixes the issue.
Testing
Tested in microwaveoven_test.py