-
Notifications
You must be signed in to change notification settings - Fork 32
[SL-ONLY] Feature/rangehood app #698
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
examples/rangehood-app/rangehood-app-common/include/ExtractorHoodEndpoint.h
Outdated
Show resolved
Hide resolved
examples/rangehood-app/rangehood-app-common/include/ExtractorHoodEndpoint.h
Outdated
Show resolved
Hide resolved
examples/rangehood-app/rangehood-app-common/src/LightEndpoint.cpp
Outdated
Show resolved
Hide resolved
examples/rangehood-app/rangehood-app-common/src/LightEndpoint.cpp
Outdated
Show resolved
Hide resolved
examples/rangehood-app/rangehood-app-common/src/LightEndpoint.cpp
Outdated
Show resolved
Hide resolved
bb74b3b to
4e4e746
Compare
4e4e746 to
993bfda
Compare
examples/rangehood-app/rangehood-app-common/include/ExtractorHoodEndpoint.h
Outdated
Show resolved
Hide resolved
examples/rangehood-app/rangehood-app-common/src/ExtractorHoodEndpoint.cpp
Outdated
Show resolved
Hide resolved
| { | ||
| chip::app::DataModel::Nullable<chip::Percent> percentSetting; | ||
|
|
||
| chip::DeviceLayer::PlatformMgr().LockChipStack(); |
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 will not cause any hang issues?
Should ScheduleWork be used instead?
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.
they are already call from chip stack only right?
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.
Yes, but if they are called from AppTask, it may lead to hangs due to deadlock. Function comments can be added to explain the usage of this API and potential risks
| Status ExtractorHoodEndpoint::GetFanMode(chip::app::Clusters::FanControl::FanModeEnum & fanMode) const | ||
| { | ||
| chip::DeviceLayer::PlatformMgr().LockChipStack(); | ||
| Status status = chip::app::Clusters::FanControl::Attributes::FanMode::Get(mEndpointId, &fanMode); |
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 comment as above. Should ScheduleWork be used?
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.
they are already call from chip stack only right?
examples/rangehood-app/rangehood-app-common/src/ExtractorHoodEndpoint.cpp
Outdated
Show resolved
Hide resolved
examples/rangehood-app/rangehood-app-common/src/ExtractorHoodEndpoint.cpp
Outdated
Show resolved
Hide resolved
examples/rangehood-app/rangehood-app-common/include/LightEndpoint.h
Outdated
Show resolved
Hide resolved
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 20 out of 21 changed files in this pull request and generated 3 comments.
examples/rangehood-app/rangehood-app-common/src/ExtractorHoodEndpoint.cpp
Show resolved
Hide resolved
| case FanControl::FanModeEnum::kAuto: { | ||
| // For Auto/Smart modes, update the FanMode attribute to reflect the current mode | ||
| return UpdateFanModeAttribute(aNewFanMode); | ||
| } |
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.
Bug: Improve Fan Mode Attribute Update Efficiency
For Auto and Smart fan modes, UpdateFanModeAttribute is called to set the FanMode attribute to the value it was just changed to (since this code runs in the attribute change callback). This redundantly writes the same value back to the attribute, which is wasteful and illogical. The air-purifier-app implementation simply breaks without re-setting the attribute for these modes.
examples/rangehood-app/rangehood-app-common/src/LightEndpoint.cpp
Outdated
Show resolved
Hide resolved
|
Restyle is failing. Please provide description on it was tested. Ideally, all description is added in the proposed template section and not added by the bot post PR creation. |
| * @param mediumPercent Percent value for Medium mode (typically 60) | ||
| * @param highPercent Percent value for High/On mode (typically 100) | ||
| */ | ||
| CHIP_ERROR Init(chip::Percent offPercent, chip::Percent lowPercent, chip::Percent mediumPercent, chip::Percent highPercent); |
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 have a OffPercent here? It is always 0 based on what I read on the spec
The value 0 (zero) in this attribute SHALL be in a range by itself. This range SHALL map to 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.
As per your comment, offPercent is not needed, so removed offpercent here in Init.
| * @brief Initialize the ExtractorHood endpoint. | ||
| * @param offPercent Percent value for Off mode (typically 0) | ||
| * @param lowPercent Percent value for Low mode (typically 30) | ||
| * @param mediumPercent Percent value for Medium mode (typically 60) | ||
| * @param highPercent Percent value for High/On mode (typically 100) |
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.
Where does the "Typically" value come from? I don't see any reference from the spec suggesting those values.
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.
Each FanMode is mapped to a value from 0-100, this can be application-specific if i understand correctly.
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.
Sure, but I don't think we should provide a "typically" statement/value when we don't actually have info on that
| * @param mediumPercent Percent value for Medium mode (typically 60) | ||
| * @param highPercent Percent value for High/On mode (typically 100) | ||
| */ | ||
| CHIP_ERROR Init(chip::Percent offPercent, chip::Percent lowPercent, chip::Percent mediumPercent, chip::Percent highPercent); |
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.
Doesn't the # of Percent "steps" depend on FanModeEnum and FanModeSequenceEnum settings?
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.
FanModeSequenceEnum value by default 0 which means it supports all the fanModes - off,low medium,high , so application supports all fanmodes.
examples/rangehood-app/rangehood-app-common/include/ExtractorHoodEndpoint.h
Outdated
Show resolved
Hide resolved
examples/rangehood-app/rangehood-app-common/include/ExtractorHoodEndpoint.h
Outdated
Show resolved
Hide resolved
examples/rangehood-app/rangehood-app-common/src/LightEndpoint.cpp
Outdated
Show resolved
Hide resolved
| static_cast<uint8_t>(::chip::DeviceLayer::Silabs::SilabsPlatform::ButtonAction::ButtonPressed)) | ||
| { | ||
| // Schedule fan mode toggle on CHIP stack thread to avoid direct access causing locking errors. | ||
| chip::DeviceLayer::PlatformMgr().ScheduleWork([](intptr_t) { RangeHoodMgr().GetExtractorHoodEndpoint().ToggleFanMode(); }, |
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 doesn't really make sense since you use ChipLock/Unlock in the ExtractorHoodEndpoint APIs.
I am not even sure the chip task mutex defined as Recursive. It is possible that you are creating a deadlock here. Was this tested?
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 tested like if button is pressed fan mode changes on/off.
Lock/unlock added as if this ToggleFanMode called from other than chip task. Please suggest the correct way of doing this kind of handling.
| to_underlying(fanModeStatus))); | ||
|
|
||
| // Don't update PercentCurrent if fan mode is Auto | ||
| VerifyOrReturnValue(currentFanMode != FanControl::FanModeEnum::kAuto, Status::Success); |
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.
Bug: Smart Mode: Automatic Control Broken
The HandlePercentSettingChange function only checks for kAuto mode before updating PercentCurrent, but HandleFanModeChange treats both kAuto and kSmart modes identically. This inconsistency means PercentCurrent will be updated when in kSmart mode even though it shouldn't be, since Smart mode should control the fan speed automatically like Auto 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.
Pull Request Overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated no new comments.
examples/rangehood-app/rangehood-app-common/src/ExtractorHoodEndpoint.cpp
Show resolved
Hide resolved
examples/rangehood-app/rangehood-app-common/src/ExtractorHoodEndpoint.cpp
Outdated
Show resolved
Hide resolved
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 20 out of 21 changed files in this pull request and generated no new comments.
examples/rangehood-app/rangehood-app-common/src/ExtractorHoodEndpoint.cpp
Show resolved
Hide resolved
| "ExtractorHoodEndpoint::SetPercentCurrent: failed to update PercentCurrent attribute: %d", | ||
| to_underlying(setStatus))); | ||
| return Status::Success; | ||
| } |
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.
Bug: Nested Lock Acquisition Causes Deadlock
SetPercentCurrent() acquires the chip stack lock multiple times, but it's called from HandleFanModeChange() which is invoked from MatterPostAttributeChangeCallback where the lock is already held. This causes a deadlock. The lock/unlock calls need to be removed as the calling context already holds the lock.
examples/rangehood-app/rangehood-app-common/src/ExtractorHoodEndpoint.cpp
Show resolved
Hide resolved
| "ExtractorHoodEndpoint::UpdateFanModeAttribute: failed to update FanMode attribute: %d", | ||
| to_underlying(setStatus))); | ||
| return Status::Success; | ||
| } |
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.
Bug: Double Lock Deadlock
UpdateFanModeAttribute() acquires the chip stack lock, but it's called from ToggleFanMode() which is invoked via ScheduleWork where the lock is already held, and from HandleFanModeChange() which is called from MatterPostAttributeChangeCallback where the lock is already held. This causes a deadlock. The lock/unlock calls need to be removed.
examples/rangehood-app/rangehood-app-common/src/ExtractorHoodEndpoint.cpp
Show resolved
Hide resolved
| #endif | ||
|
|
||
| // Initialization of RangeHoodManager and endpoints of range hood. | ||
| RangeHoodManager::GetInstance().Init(); |
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.
Bug: Initialization Failures: A Silent Path to Instability
RangeHoodManager::GetInstance().Init() return value is not checked. If initialization fails (e.g., if mExtractorHoodEndpoint.Init() returns an error), the application continues running with an uninitialized state, which could lead to undefined behavior when the endpoints are used. Other similar initialization code in the codebase checks the return value and calls appError() on failure.
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 20 out of 21 changed files in this pull request and generated 3 comments.
examples/rangehood-app/rangehood-app-common/src/ExtractorHoodEndpoint.cpp
Show resolved
Hide resolved
examples/rangehood-app/rangehood-app-common/src/ExtractorHoodEndpoint.cpp
Show resolved
Hide resolved
examples/rangehood-app/rangehood-app-common/src/ExtractorHoodEndpoint.cpp
Show resolved
Hide resolved
| (currentFanMode == FanControl::FanModeEnum::kOff) ? FanControl::FanModeEnum::kHigh : FanControl::FanModeEnum::kOff; | ||
|
|
||
| return UpdateFanModeAttribute(target); | ||
| } |
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.
Bug
ToggleFanMode() calls GetFanMode() and UpdateFanModeAttribute() without holding the CHIP stack lock. The GetFanMode() function explicitly requires the caller to hold the lock (per its documentation comment), and UpdateFanModeAttribute() accesses Matter attributes which also require lock protection. This causes a race condition when the function is invoked via ScheduleWork from the button handler.
Summary
This PR introduces new Matter Rangehood example implementing composite device type of Fan and Light.
Summary of Updates:
Implemented multi-endpoint architecture:
Root Node Endpoint (ID: 0)
Fan Endpoint (ID: 1)
Light Endpoint(ID: 2)
Added Fan control and Light On/Off control. Added button implementation to control fan on/off.
Also added LED mapping to display current state of the light.
Related issues
MATTER-5148
Testing
Verified commissioning and basic functionality on SiWx917 evaluation board.
Validated fan control via cluster commands as well as on BTN1 Press.
Validated lighting on/off via cluster commands
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
Note
Introduces a Silabs RangeHood example with extractor hood fan and light endpoints, button-driven fan toggle, LED light feedback, and full ZAP data model + GN build integration.
0(Root),1(Extractor Hood/FanControl),2(Light/OnOff).RangeHoodManager,ExtractorHoodEndpoint,LightEndpoint,AppTask,DataModelCallbacks.FanMode(Off/Low/Medium/High/On/Auto/Smart) toPercentCurrent; syncs on init.PercentSetting/FanModeattribute changes; button (BTN1) togglesFanModeOff⇄High.BUILD.gn,.gn, Wi‑Fi args) and vendor config (CHIPProjectConfig.h).rangehood-wifi-app.zap/.matter) defining clusters and endpoints; enables OTA Requestor.Written by Cursor Bugbot for commit 59ca047. This will update automatically on new commits. Configure here.