-
Notifications
You must be signed in to change notification settings - Fork 31
[SL-ONLY] Oven app commands implementation #693
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.
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
base: main
Are you sure you want to change the base?
Conversation
af4ef43 to
83372e0
Compare
| /** | ||
| * @brief Set On/Off state for the CookSurface. | ||
| */ | ||
| void SetOnOffState(bool state); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set should return the status
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified as per suggestion
| */ | ||
| void HandleOffCommand(); | ||
|
|
||
| bool GetOnOffState(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should return status and get should fil the data in variable passed as arg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| * @brief Handle the "off" command for the cooktop. | ||
| */ | ||
| void HandleOffCommand(); | ||
| void SetOnOffState(bool state); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set should return the status
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| CHIP_ERROR GetModeValueByIndex(uint8_t modeIndex, uint8_t & value) override; | ||
| CHIP_ERROR GetModeTagsByIndex(uint8_t modeIndex, DataModel::List<detail::Structs::ModeTagStruct::Type> & tags) override; | ||
|
|
||
| // Public helper to query support status without exposing internal tables |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment irrelevant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed the comment
| CHIP_ERROR GetModeTagsByIndex(uint8_t modeIndex, DataModel::List<detail::Structs::ModeTagStruct::Type> & tags) override; | ||
|
|
||
| // Public helper to query support status without exposing internal tables | ||
| static bool IsSupportedMode(uint8_t mode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any specific reason for making this static
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial skeleton code has the oven-modes as static variables, so made this function static to access those variables.
Now, since the oven-modes are modified to enum, we can make this function non-static. Made this change
|
|
||
| CHIP_ERROR CookSurfaceEndpoint::Init() | ||
| { | ||
| bool state = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be removed, as this doesnt add functionaity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
| } | ||
|
|
||
| void CookSurfaceEndpoint::HandleOffCommand() | ||
| bool CookSurfaceEndpoint::GetOnOffState() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as comments in .h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified as per the suggestions
| return state; | ||
| } | ||
|
|
||
| void CookSurfaceEndpoint::SetOnOffState(bool state) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as comments in .h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified
| } | ||
|
|
||
| void CookTopEndpoint::HandleOffCommand() | ||
| void CookTopEndpoint::SetOnOffState(bool state) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as comments in .h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified
| } | ||
| } // namespace | ||
|
|
||
| void OvenManager::ProcessOvenModeChange(chip::EndpointId endpointId, uint8_t newMode, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some part of code can be leveraged into common code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, moved most of the code to common layer and kept only the mode-transition logic in OvenManager.
2014ada to
43caebd
Compare
| // Get CookTop On/Off value | ||
| // OnOffServer::Instance().getOnOffValue(1, ¤tLedState); | ||
| // mCookTopState = currentLedState ? kState_OnCompleted : kState_OffCompleted; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not needed, let's remove it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed.
| { | ||
| if (aActor == AppEvent::kEventType_CookTop) | ||
| { | ||
| bool lightOn = aAction == OvenManager::ON_ACTION; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lightOn should be renamed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed this as the action_initiated and completed callbacks are suggested to be removed.
| AppTask::GetAppTask().PostEvent(&event); | ||
| } | ||
|
|
||
| if (action_initiated && mActionInitiated_CB) | ||
| { | ||
| mActionInitiated_CB(aAction, aActor, aValue); | ||
| } | ||
|
|
||
| return action_initiated; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole PostEvent + ActionInitiated callback and then PostEvent calling ActionCompleted as no value for this app.
Nothing is really done in your ActuatorMovementHandler that necessitate a Queue event and then nothing is done either in the ActionCompleted except a print.
mSyncClusterToButtonAction is set to true/false over ActionInitiated and ActionCompleted but nothing is done with this variable.
I get that you copied this pattern from other sample apps but in this instance there is a whole lot of function and events to essentially do nothin except setting an object variable state and telling the LCD to update its screen.
This can all be simplified to 1 function or one posted Event
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the PostEvent + ActionInitiated callback flow as suggested. I added logic just to post an event to AppTask which takes care of the required hardware action (LED and LCD update in our sample app).
| bool cookSurfaceEndpoint2State; | ||
| mCookSurfaceEndpoint1.GetOnOffState(cookSurfaceEndpoint1State); | ||
| mCookSurfaceEndpoint2.GetOnOffState(cookSurfaceEndpoint2State); | ||
| // Check if both cooksurfaces are off. If yes, turn off the cooktop (call cooktop.TurnOffCookTop) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this defined by spec or a choice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not defined in spec, we have added this in our application as a choice.
| chip::Protocols::InteractionModel::Status CookSurfaceEndpoint::SetOnOffState(bool state) | ||
| { | ||
| CommandId commandId = state ? OnOff::Commands::On::Id : OnOff::Commands::Off::Id; | ||
| auto status = OnOffServer::Instance().setOnOffValue(mEndpointId, commandId, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling 'OnOffServer::Instance().setOnOffValue` must be called from the ChipTask context or with the ChipTask lock else it will cause and assert/chip die.
When called in OnOffAttributeChangeHandler which is called from MatterPostAttributeCallback, we are indeed in the ChipTaskl context. However, if this is called somewhere else then this might not be the case and it can lead to crashes.
If you keep it this way this function usage (an all the copied functionson the other endpoints) needs to be documented to explain this risk/limitation in the usage.
Same goes for the GetOnOffState
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like @jmartinez-silabs said please add a lock on the chip task before doing so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added comments in the header file regarding the usage of this function.
I guess we do not need to add lock on chip-task in this place as the SetOnOffValue is being executed from chiptask only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements command handling logic for an oven application across multiple endpoints as part of the [SL-ONLY] Oven app commands implementation. It adds support for set-temperature and change-to-mode commands on the TemperatureControlledCabinet endpoint (endpoint 2), and off commands for CookTop (endpoint 3) and CookSurface endpoints (endpoints 4 & 5).
- Adds complete oven application structure with endpoint management and command handling
- Implements temperature control with supported levels for cook surfaces and setpoint control for temperature controlled cabinet
- Creates data model callbacks and attribute change handlers for coordinated state management across endpoints
Reviewed Changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/oven-app/silabs/src/OvenManager.cpp | Core manager implementing endpoint initialization and command handling coordination |
| examples/oven-app/silabs/src/DataModelCallbacks.cpp | Attribute change callback handlers for OnOff, TemperatureControl, and OvenMode clusters |
| examples/oven-app/silabs/src/AppTask.cpp | Main application task with oven manager initialization |
| examples/oven-app/silabs/src/AppSupportedTemperatureLevelsDelegate.cpp | Temperature levels delegate for cook surface endpoints |
| examples/oven-app/oven-app-common/src/OvenEndpoint.cpp | Oven mode delegate implementation with transition blocking logic |
| examples/oven-app/oven-app-common/src/CookTopEndpoint.cpp | CookTop endpoint OnOff state management |
| examples/oven-app/oven-app-common/src/CookSurfaceEndpoint.cpp | CookSurface endpoint OnOff state management |
| examples/oven-app/oven-app-common/oven-app.matter | Matter data model definition for oven application |
| examples/oven-app/silabs/include/*.h | Header files defining class interfaces and configuration |
| examples/oven-app/oven-app-common/include/*.h | Common header files for endpoint implementations |
| examples/oven-app/silabs/build_.gn | Build configuration files for WiFi and general builds |
| examples/oven-app/oven-app-common/BUILD.gn | Build configuration for common oven app components |
examples/oven-app/silabs/src/AppSupportedTemperatureLevelsDelegate.cpp
Outdated
Show resolved
Hide resolved
daebfbd to
2434980
Compare
|
Regarding the folder structure we should really have the following : examples |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stopping review for now as this seems to be a work in progress.
Please apply the comments, cleanup the logs, remove unused functions/methods when necessary.
| chip::Protocols::InteractionModel::Status CookSurfaceEndpoint::GetOnOffState(bool & state) | ||
| { | ||
| auto status = OnOffServer::Instance().getOnOffValue(mEndpointId, &state); | ||
| VerifyOrReturnValue(status == Protocols::InteractionModel::Status::Success, status, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could do without this verify or return and let the higher level handle the logging if necessary. Please remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
| chip::Protocols::InteractionModel::Status CookSurfaceEndpoint::SetOnOffState(bool state) | ||
| { | ||
| CommandId commandId = state ? OnOff::Commands::On::Id : OnOff::Commands::Off::Id; | ||
| auto status = OnOffServer::Instance().setOnOffValue(mEndpointId, commandId, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like @jmartinez-silabs said please add a lock on the chip task before doing so.
| { | ||
| CommandId commandId = state ? OnOff::Commands::On::Id : OnOff::Commands::Off::Id; | ||
| auto status = OnOffServer::Instance().setOnOffValue(mEndpointId, commandId, false); | ||
| VerifyOrReturnValue(status == Protocols::InteractionModel::Status::Success, status, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again since we are returning a status, please remove this check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
|
|
||
| CHIP_ERROR CookTopEndpoint::Init() | ||
| { | ||
| return CHIP_NO_ERROR; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again please add a comment as why this function is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a placeholder function similar to one in CookSurface, added a comment.
| { | ||
| CommandId commandId = state ? OnOff::Commands::On::Id : OnOff::Commands::Off::Id; | ||
| auto status = OnOffServer::Instance().setOnOffValue(mEndpointId, commandId, false); | ||
| VerifyOrReturnValue(status == Protocols::InteractionModel::Status::Success, status, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this line as we are returning a status
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
| using chip::Protocols::InteractionModel::Status; | ||
|
|
||
| // Static member definitions | ||
| const detail::Structs::ModeTagStruct::Type OvenModeDelegate::sModeTagsBake[1] = { { .value = to_underlying(ModeTag::kBake) } }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please find a cleaner way to handle this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, simplified the tag-definitions.
| to_underlying(ModeTag::kProofing) } }; | ||
|
|
||
| const detail::Structs::ModeOptionStruct::Type OvenModeDelegate::skModeOptions[to_underlying(OvenModes::kModeCount)] = { | ||
| { .label = CharSpan::fromCharString("Bake"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again this is unreadable 😢 , there must be a cleaner and more concise way to achieve the desired result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simplified the declarations by adding this : using detail::Structs::ModeOptionStruct::Type
|
|
||
| CHIP_ERROR OvenModeDelegate::Init() | ||
| { | ||
| // No initialization required for OvenModeDelegate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then why do we need this method ? please remove it since it's adding dead code which can cause code size bloats for no reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ModeBase::Instance is the base interface from where we extend the OvenModeDelegate class. Init function is declared in the ModeBase::Instance interface, so it should be defined in our OvenModeDelegate class.
| return CHIP_NO_ERROR; | ||
| } | ||
|
|
||
| void OvenModeDelegate::HandleChangeToMode(uint8_t NewMode, ModeBase::Commands::ChangeToModeResponse::Type & response) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't we have a mode enum somewhere in another header file ? is so we should use it instead of the generic uint8_t
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HandleChangeToMode is a function given by the delegate. In the delegate, it has uint8_t as the parameter type.
For oven modes, we have added a mode enum instead of having the modes as some static constants.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
2434980 to
232fe4f
Compare
There was a problem hiding this 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 15 out of 15 changed files in this pull request and generated 3 comments.
Co-authored-by: Jean-Francois Penven <[email protected]> Co-authored-by: Copilot <[email protected]>
There was a problem hiding this 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 15 out of 15 changed files in this pull request and generated 3 comments.
| static constexpr BlockedTransition kBlockedTransitions[3] = { | ||
| { chip::to_underlying(chip::app::Clusters::TemperatureControlledCabinet::OvenModeDelegate::OvenModes::kModeGrill), | ||
| chip::to_underlying(chip::app::Clusters::TemperatureControlledCabinet::OvenModeDelegate::OvenModes::kModeProofing) }, | ||
| { chip::to_underlying(chip::app::Clusters::TemperatureControlledCabinet::OvenModeDelegate::OvenModes::kModeProofing), | ||
| chip::to_underlying(chip::app::Clusters::TemperatureControlledCabinet::OvenModeDelegate::OvenModes::kModeClean) }, | ||
| { chip::to_underlying(chip::app::Clusters::TemperatureControlledCabinet::OvenModeDelegate::OvenModes::kModeClean), | ||
| chip::to_underlying(chip::app::Clusters::TemperatureControlledCabinet::OvenModeDelegate::OvenModes::kModeBake) }, | ||
| }; |
Copilot
AI
Nov 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The blocked transitions array is defined but lacks explanation of why these specific transitions are blocked. Consider adding a comment describing the rationale (e.g., safety concerns, mode incompatibilities) for blocking Grill→Proofing, Proofing→Clean, and Clean→Bake transitions.
There was a problem hiding this 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 14 out of 14 changed files in this pull request and generated 1 comment.
| chip::Protocols::InteractionModel::Status SetOnOffState(bool state); | ||
|
|
||
| private: | ||
| bool currentOnOffState = false; |
Copilot
AI
Nov 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The private member variable currentOnOffState appears to be unused since the implementation now relies on OnOffServer::Instance() to manage the on/off state. This field should be removed to avoid confusion and potential state synchronization issues.
| bool currentOnOffState = false; |
There was a problem hiding this 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 14 out of 14 changed files in this pull request and generated 1 comment.
| const Type OvenModeDelegate::sModeTagsConvection[1] = { { .value = to_underlying(ModeTag::kConvection) } }; | ||
|
|
||
| const detail::Structs::ModeTagStruct::Type OvenModeDelegate::sModeTagsGrill[1] = { { .value = to_underlying(ModeTag::kGrill) } }; | ||
| const Type OvenModeDelegate::sModeTagsGrill[1] = { { .value = to_underlying(ModeTag::kGrill) } }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need an array of 1 ? What is this logic and why is this needed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic is for adding a unique tag to all the oven-modes.
those length‑1 arrays hold the single ModeTag value for each oven mode so we can give a DataModel::List view (which expects contiguous storage + a size) to the ModeOptionStruct entries.
| chip::Protocols::InteractionModel::Status CookSurfaceEndpoint::SetOnOffState(bool state) | ||
| { | ||
| CommandId commandId = state ? OnOff::Commands::On::Id : OnOff::Commands::Off::Id; | ||
| return OnOffServer::Instance().setOnOffValue(mEndpointId, commandId, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be encased in a chipTaskLock ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily if it is called from ChipTask. I have added the function usage in header file (
| * Note: This helper writes the OnOff attribute to the CHIP attribute storage and |
ScheduleWork or ChipTaskLock should only be used if this function is called from AppTask
|
|
||
| #include "AppEvent.h" | ||
| #include "BaseApplication.h" | ||
| #include "OvenManager.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this needed here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not needed, removed it.
| ChipLogValueMEI(attributeId), type, *value, size); | ||
| break; | ||
| case app::Clusters::OnOff::Id: | ||
| ChipLogProgress(Zcl, "OnOff cluster ID: " ChipLogFormatMEI " Type: %u Value: %u, length %u", ChipLogValueMEI(attributeId), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these should be ChipLogDetails and not ChipLogProgress
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified as suggested
| chip::app::Clusters::AppSupportedTemperatureLevelsDelegate mTemperatureControlDelegate; | ||
|
|
||
| State_t mCookTopState; | ||
| State_t mCookSurfaceState1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need 2 SurfaceState ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
customer requirements mentioned 2 surfaces . so we added 2 surfaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A cook-top can contain multiple cook-surfaces
In our app, we have 2 cook surfaces (as per the requirements doc that we received). . To store the state of these 2 cook-surfaces separately, i have used 2 state variables. There is a possibility that one cook-surface is "On" and another cook-surface is "Off"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a couple of things left to polish out
There was a problem hiding this 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 14 out of 14 changed files in this pull request and generated 3 comments.
| const Type OvenModeDelegate::sModeTagsBake[1] = { { .value = to_underlying(ModeTag::kBake) } }; | ||
|
|
||
| const detail::Structs::ModeTagStruct::Type OvenModeDelegate::sModeTagsConvection[1] = { { .value = to_underlying( | ||
| ModeTag::kConvection) } }; | ||
| const Type OvenModeDelegate::sModeTagsConvection[1] = { { .value = to_underlying(ModeTag::kConvection) } }; | ||
|
|
||
| const detail::Structs::ModeTagStruct::Type OvenModeDelegate::sModeTagsGrill[1] = { { .value = to_underlying(ModeTag::kGrill) } }; | ||
| const Type OvenModeDelegate::sModeTagsGrill[1] = { { .value = to_underlying(ModeTag::kGrill) } }; | ||
|
|
||
| const detail::Structs::ModeTagStruct::Type OvenModeDelegate::sModeTagsRoast[1] = { { .value = to_underlying(ModeTag::kRoast) } }; | ||
| const Type OvenModeDelegate::sModeTagsRoast[1] = { { .value = to_underlying(ModeTag::kRoast) } }; | ||
|
|
||
| const detail::Structs::ModeTagStruct::Type OvenModeDelegate::sModeTagsClean[1] = { { .value = to_underlying(ModeTag::kClean) } }; | ||
| const Type OvenModeDelegate::sModeTagsClean[1] = { { .value = to_underlying(ModeTag::kClean) } }; | ||
|
|
||
| const detail::Structs::ModeTagStruct::Type OvenModeDelegate::sModeTagsConvectionBake[1] = { { .value = to_underlying( | ||
| ModeTag::kConvectionBake) } }; | ||
| const Type OvenModeDelegate::sModeTagsConvectionBake[1] = { { .value = to_underlying(ModeTag::kConvectionBake) } }; | ||
|
|
||
| const detail::Structs::ModeTagStruct::Type OvenModeDelegate::sModeTagsConvectionRoast[1] = { { .value = to_underlying( | ||
| ModeTag::kConvectionRoast) } }; | ||
| const Type OvenModeDelegate::sModeTagsConvectionRoast[1] = { { .value = to_underlying(ModeTag::kConvectionRoast) } }; | ||
|
|
||
| const detail::Structs::ModeTagStruct::Type OvenModeDelegate::sModeTagsWarming[1] = { { .value = | ||
| to_underlying(ModeTag::kWarming) } }; | ||
| const Type OvenModeDelegate::sModeTagsWarming[1] = { { .value = to_underlying(ModeTag::kWarming) } }; | ||
|
|
||
| const detail::Structs::ModeTagStruct::Type OvenModeDelegate::sModeTagsProofing[1] = { { .value = | ||
| to_underlying(ModeTag::kProofing) } }; | ||
| const Type OvenModeDelegate::sModeTagsProofing[1] = { { .value = to_underlying(ModeTag::kProofing) } }; |
Copilot
AI
Nov 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The typedef alias 'Type' for 'detail::Structs::ModeTagStruct::Type' obscures the actual type being used in these static member definitions. While it reduces line length, it makes the code less self-documenting. Consider using the full type name or a more descriptive alias that indicates it's a ModeTag struct type.
| mCookSurfaceEndpoint1.GetOnOffState(cookSurfaceEndpoint1State); | ||
| mCookSurfaceEndpoint2.GetOnOffState(cookSurfaceEndpoint2State); | ||
| // Turn off CookTop if both the CookSurfaces are off. | ||
| if (!cookSurfaceEndpoint1State && !cookSurfaceEndpoint2State) | ||
| { | ||
| mCookTopEndpoint.SetOnOffState(false); | ||
| mCookTopState = kCookTopState_Off; | ||
| } |
Copilot
AI
Nov 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return value of GetOnOffState() is not checked. If GetOnOffState() fails (returns error Status), the variables cookSurfaceEndpoint1State and cookSurfaceEndpoint2State will contain uninitialized values, potentially causing incorrect state decisions on line 168. Check the Status return value before using these variables.
| mCookSurfaceEndpoint1.GetOnOffState(cookSurfaceEndpoint1State); | |
| mCookSurfaceEndpoint2.GetOnOffState(cookSurfaceEndpoint2State); | |
| // Turn off CookTop if both the CookSurfaces are off. | |
| if (!cookSurfaceEndpoint1State && !cookSurfaceEndpoint2State) | |
| { | |
| mCookTopEndpoint.SetOnOffState(false); | |
| mCookTopState = kCookTopState_Off; | |
| } | |
| Status status1 = mCookSurfaceEndpoint1.GetOnOffState(cookSurfaceEndpoint1State); | |
| Status status2 = mCookSurfaceEndpoint2.GetOnOffState(cookSurfaceEndpoint2State); | |
| // Turn off CookTop if both the CookSurfaces are off. | |
| if (status1 == Status::Success && status2 == Status::Success) | |
| { | |
| if (!cookSurfaceEndpoint1State && !cookSurfaceEndpoint2State) | |
| { | |
| mCookTopEndpoint.SetOnOffState(false); | |
| mCookTopState = kCookTopState_Off; | |
| } | |
| } | |
| else | |
| { | |
| ChipLogError(AppServer, "Failed to get CookSurface OnOff state (status1=%d, status2=%d)", static_cast<int>(status1), static_cast<int>(status2)); | |
| } |
| * | ||
| * @return Returns Status::Success on success, or an error code on failure. | ||
| */ | ||
|
|
Copilot
AI
Nov 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Extra blank line between documentation and function declaration. Remove the blank line on line 55 to maintain consistency with standard documentation formatting practices.
Summary
This PR adds logic to handle the commands in the oven-app over all the oven endpoints.
Below are the list of commands supported over each endpoint:
Endpoint 2 (TemperatureControlledCabinet) : set-temperature, change-to-mode
Endpoint 3 (CookTop) : off
Endpoint 4, Endpoint 5 (CookSurface) : off
Related issues
MATTER-5573
Testing
Tested all the commands with 917SoC and verified that the functionality is working as mentioned in the design (https://confluence.silabs.com/spaces/MATTER/pages/696870856/MDD-0022+-+Oven+and+RangeHood+app+design)
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