[Code Driven Cluster] Decouple Fan Cluster part 2#43408
Open
LyudmilaKostanyan wants to merge 70 commits intoproject-chip:masterfrom
Open
[Code Driven Cluster] Decouple Fan Cluster part 2#43408LyudmilaKostanyan wants to merge 70 commits intoproject-chip:masterfrom
Fan Cluster part 2#43408LyudmilaKostanyan wants to merge 70 commits intoproject-chip:masterfrom
Conversation
LyudmilaKostanyan
moved this to In Progress
in Cluster and example long term maintainability
Contributor
Author
|
/gemini review |
![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 is a solid step forward in migrating the Fan Control cluster to a modern, code-driven architecture. However, a critical use-after-free vulnerability has been identified in the integration layer where stack-allocated delegates are passed to registration functions, which violates the rule regarding the lifetime of stack-allocated objects passed by reference. Beyond this, the FeatureMap attribute is hardcoded to zero, and key configuration attributes like SpeedMax, RockSupport, and WindSupport are hardcoded in CodegenIntegration.cpp, limiting customization. Logic errors were also found in the attribute write validation for FanMode, RockSetting, and WindSetting, potentially causing incorrect device behavior or denial of service. Addressing these points, including expanding unit tests for FeatureMap once corrected, is essential for a secure, correct, and flexible implementation.
Contributor
There was a problem hiding this comment.
Code Review
This pull request successfully migrates the Fan Control cluster to the code-driven architecture, which is a great improvement. The changes involve removing the old Ember-style implementation and replacing it with a new FanControlCluster class, along with the necessary integration code and unit tests.
My review found a couple of high-severity issues that should be addressed:
- In
CodegenIntegration.cpp, several cluster configuration values are hardcoded, which limits the functionality and flexibility of the cluster. - The
FanControlClusterimplementation incorrectly reports theFeatureMapattribute as 0, which will prevent clients from discovering supported features.
Addressing these issues will make this migration complete and robust.
|
PR #43408: Size comparison from a6e7031 to 0794253 Full report (14 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, qpg, realtek, stm32)
|
|
PR #43408: Size comparison from a6e7031 to 8a8c60a Full report (26 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, efr32, nxp, qpg, realtek, stm32, telink)
|
|
PR #43408: Size comparison from a6e7031 to 9bd7eea Full report (3 builds for realtek, stm32)
|
LyudmilaKostanyan
force-pushed
the
FanControl
branch
from
4ade9dd to
1d2acd4
Compare
|
PR #43408: Size comparison from 832cfef to 1d2acd4 Full report (32 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #43408: Size comparison from 832cfef to fbe8afe Full report (11 builds for cc13x4_26x4, cc32xx, qpg, realtek, stm32)
|
|
PR #43408: Size comparison from 832cfef to c5c98c9 Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #43408: Size comparison from 832cfef to 871a07f Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #43408 +/- ##
==========================================
+ Coverage 54.52% 54.71% +0.18%
==========================================
Files 1588 1590 +2
Lines 108569 108831 +262
Branches 13365 13325 -40
==========================================
+ Hits 59201 59546 +345
+ Misses 49368 49285 -83 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
PR #43408: Size comparison from 832cfef to 4e8369f Full report (3 builds for realtek, stm32)
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 59 out of 59 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
PR #43408: Size comparison from 1b8e309 to ee72891 Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 59 out of 59 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
examples/chef/common/chef-fan-control-manager.cpp:252
SetPercentCurrent()andSetSpeedCurrent()now callFanControlCluster::SetPercentSetting()/SetSpeedSetting(). This writes back to the *Setting attributes (not the *Current attributes), can retrigger FanControl attribute-change callbacks, and the error logs still refer to writing PercentCurrent/SpeedCurrent. If the intent is to update the *Current attributes, this should be done without writing the settings back (e.g. update only local tracking, or introduce/using an API that updates PercentCurrent/SpeedCurrent directly).
void ChefFanControlManager::SetPercentCurrent(uint8_t aNewPercentCurrent)
{
ChipLogDetail(NotSpecified, "ChefFanControlManager::SetPercentCurrent: %d", aNewPercentCurrent);
mPercentCurrent = aNewPercentCurrent;
Status status = Status::Success;
if (FanControlCluster * fc = FanControl::FindClusterOnEndpoint(mEndpoint); fc != nullptr)
{
status = fc->SetPercentSetting(DataModel::Nullable<Percent>(mPercentCurrent)).GetStatusCode().GetStatus();
}
else
{
status = Status::UnsupportedEndpoint;
}
if (status != Status::Success)
{
ChipLogError(NotSpecified, "ChefFanControlManager::SetPercentCurrent: failed to set PercentCurrent attribute: %d",
to_underlying(status));
}
}
void ChefFanControlManager::SetSpeedCurrent(uint8_t aNewSpeedCurrent)
{
mSpeedCurrent = aNewSpeedCurrent;
Status status = Status::Success;
if (FanControlCluster * fc = FanControl::FindClusterOnEndpoint(mEndpoint); fc != nullptr)
{
status = fc->SetSpeedSetting(MakeNullable(aNewSpeedCurrent)).GetStatusCode().GetStatus();
}
else
{
status = Status::UnsupportedEndpoint;
}
if (status != Status::Success)
{
ChipLogError(NotSpecified, "ChefFanControlManager::SetSpeedCurrent: failed to set SpeedCurrent attribute: %d",
to_underlying(status));
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
PR #43408: Size comparison from 1b8e309 to fa6889d Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
| } | ||
| else | ||
| bool wrapValue = commandData.wrap.ValueOr(false); | ||
| bool lowestOffValue = commandData.lowestOff.ValueOr(false); |
Contributor
There was a problem hiding this comment.
This is incorrect, according to the spec (https://github.com/CHIP-Specifications/connectedhomeip-spec/blob/master/src/app_clusters/FanControl.adoc#71-step-command).
It says the fallback here should be true:
Suggested change
| bool lowestOffValue = commandData.lowestOff.ValueOr(false); | |
| bool lowestOffValue = commandData.lowestOff.ValueOr(true); |
Could you double check? And also check if we have a python test that validates this? Otherwise we should create a separate issue to update the test to make sure this is not missed.
Contributor
There was a problem hiding this comment.
Here's how I reproduced this issue testing with chip tool:
# Repro: FanControl Step Fallback Issue
1. Set speed to 1:
```bash
./out/linux-x64-chip-tool-clang/chip-tool fancontrol step 0 1 1 --storage-directory ./
(Expect log: Speed changed from 0 to 1)
- Decrease speed without
LowestOff(should go to 0 per spec):
./out/linux-x64-chip-tool-clang/chip-tool fancontrol step 1 1 1 --storage-directory ./Result (Bug):
Log shows lowestOff=0 and speed stays at 1:
[DL] LoggingFanDevice::HandleStep() -> direction=1 wrap=0 lowestOff=0
[DL] LoggingFanDevice::HandleStep() -> Speed changed from 1 to 1
Expectation:
Spec says LowestOff falls back to true. It should have gone to 0.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 59 out of 59 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (13)
src/app/clusters/fan-control-server/FanControlCluster.cpp:1
- Optional FanControl attributes are being readable/writable unconditionally. This can produce incorrect behavior and status codes when features are not enabled (e.g., reading SpeedMax even when MultiSpeed is not supported, or writing RockSetting when Rocking is not supported). To fix: gate ReadAttribute/WriteAttribute cases for optional attributes using SupportsMultiSpeed()/SupportsRocking()/SupportsWind()/SupportsAirflowDirection() and return Status::UnsupportedAttribute when the feature/attribute is not present. This is also required for consistency with the AttributeList built in Attributes().
src/app/clusters/fan-control-server/FanControlCluster.cpp:1 - Optional FanControl attributes are being readable/writable unconditionally. This can produce incorrect behavior and status codes when features are not enabled (e.g., reading SpeedMax even when MultiSpeed is not supported, or writing RockSetting when Rocking is not supported). To fix: gate ReadAttribute/WriteAttribute cases for optional attributes using SupportsMultiSpeed()/SupportsRocking()/SupportsWind()/SupportsAirflowDirection() and return Status::UnsupportedAttribute when the feature/attribute is not present. This is also required for consistency with the AttributeList built in Attributes().
src/app/clusters/fan-control-server/FanControlCluster.cpp:1 - Optional FanControl attributes are being readable/writable unconditionally. This can produce incorrect behavior and status codes when features are not enabled (e.g., reading SpeedMax even when MultiSpeed is not supported, or writing RockSetting when Rocking is not supported). To fix: gate ReadAttribute/WriteAttribute cases for optional attributes using SupportsMultiSpeed()/SupportsRocking()/SupportsWind()/SupportsAirflowDirection() and return Status::UnsupportedAttribute when the feature/attribute is not present. This is also required for consistency with the AttributeList built in Attributes().
src/app/clusters/fan-control-server/FanControlCluster.cpp:1 - SetRockSetting/SetWindSetting/SetAirflowDirection do not check whether the corresponding optional attribute/feature is supported. When support bitmasks are default-initialized to 0, SetRockSetting/SetWindSetting will return ConstraintError for any nonzero value, but the expected behavior when the attribute is not present is UnsupportedAttribute (and tests in TestFanControlCluster.cpp assert UnsupportedAttribute). Add early guards like
if (!SupportsRocking()) return Status::UnsupportedAttribute;(and similarly for wind/airflow direction), before applying constraint checks.
src/app/clusters/fan-control-server/FanControlCluster.cpp:1 - SetRockSetting/SetWindSetting/SetAirflowDirection do not check whether the corresponding optional attribute/feature is supported. When support bitmasks are default-initialized to 0, SetRockSetting/SetWindSetting will return ConstraintError for any nonzero value, but the expected behavior when the attribute is not present is UnsupportedAttribute (and tests in TestFanControlCluster.cpp assert UnsupportedAttribute). Add early guards like
if (!SupportsRocking()) return Status::UnsupportedAttribute;(and similarly for wind/airflow direction), before applying constraint checks.
src/app/clusters/fan-control-server/FanControlCluster.cpp:1 - SetRockSetting/SetWindSetting/SetAirflowDirection do not check whether the corresponding optional attribute/feature is supported. When support bitmasks are default-initialized to 0, SetRockSetting/SetWindSetting will return ConstraintError for any nonzero value, but the expected behavior when the attribute is not present is UnsupportedAttribute (and tests in TestFanControlCluster.cpp assert UnsupportedAttribute). Add early guards like
if (!SupportsRocking()) return Status::UnsupportedAttribute;(and similarly for wind/airflow direction), before applying constraint checks.
src/app/clusters/fan-control-server/FanControlCluster.cpp:1 InvokeCommandhandlesStepwithout verifying the cluster actually supports the Step feature. Even though AcceptedCommands() omits Step when not supported, an incoming invocation can still reach this code path and should returnStatus::UnsupportedCommandwhen!SupportsStep(). Add an early check inside the Step case to enforce feature gating.
src/app/clusters/fan-control-server/fan-control-server.h:1- This header used to declare the legacy integration API (
SetDefaultDelegate/GetDelegate). Removing those declarations is an API-breaking change for code that still includesfan-control-server.h(which the PR states exists for backwards compatibility). If backwards compatibility is intended, consider re-exporting the legacy API by including CodegenIntegration.h here (or reintroducing declarations) so existing includes continue to compile.
src/app/clusters/fan-control-server/app_config_dependent_sources.cmake:1 - CMake builds no longer list
FanControlCluster.cpp(the actual cluster implementation) as a target source. Unless it is being built/linked via another CMake target elsewhere, this will lead to missing symbols or the FanControl server not being included at all. Recommended fix: ensure the FanControl cluster implementation is compiled in CMake builds too (either add FanControlCluster.cpp here, or link against a dedicated fan-control-server library/target that contains it).
src/app/clusters/fan-control-server/FanControlCluster.cpp:1 - When
mIsOnOffOnis false, ApplyPercentSettingChanged()/ApplySpeedSettingChanged() intentionally avoid updating the *Current attributes, but the code still callsNotifyAttributeChanged(PercentCurrent/SpeedCurrent). This can generate spurious reports/dirty state for attributes whose values did not change. Consider removing these notifications, or only notifying when the stored current value actually changes (e.g., when SetOnOffState toggles or when you explicitly update current).
src/app/clusters/fan-control-server/FanControlCluster.cpp:1 - When
mIsOnOffOnis false, ApplyPercentSettingChanged()/ApplySpeedSettingChanged() intentionally avoid updating the *Current attributes, but the code still callsNotifyAttributeChanged(PercentCurrent/SpeedCurrent). This can generate spurious reports/dirty state for attributes whose values did not change. Consider removing these notifications, or only notifying when the stored current value actually changes (e.g., when SetOnOffState toggles or when you explicitly update current).
examples/chef/common/chef-fan-control-manager.cpp:231 - In both SetPercentCurrent and SetSpeedCurrent, the error logs claim failures setting
PercentCurrent/SpeedCurrent, but the updated implementation callsFanControlCluster::SetPercentSetting/SetSpeedSetting(settings, not current). Either adjust the logs and function names to reflect that these are now writing *Setting, or rework the example so it updates the appropriate *Current attributes via the cluster’s intended mechanism (e.g., SetOnOffState + settings) without conflating measured/current vs requested/setting.
ChipLogError(NotSpecified, "ChefFanControlManager::SetPercentCurrent: failed to set PercentCurrent attribute: %d",
to_underlying(status));
examples/chef/common/chef-fan-control-manager.cpp:250
- In both SetPercentCurrent and SetSpeedCurrent, the error logs claim failures setting
PercentCurrent/SpeedCurrent, but the updated implementation callsFanControlCluster::SetPercentSetting/SetSpeedSetting(settings, not current). Either adjust the logs and function names to reflect that these are now writing *Setting, or rework the example so it updates the appropriate *Current attributes via the cluster’s intended mechanism (e.g., SetOnOffState + settings) without conflating measured/current vs requested/setting.
ChipLogError(NotSpecified, "ChefFanControlManager::SetSpeedCurrent: failed to set SpeedCurrent attribute: %d",
to_underlying(status));
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…zero Refactors SetFanModeToOff into CommitFanModeOffState to separate state commit from delegate notifications. Moves OnFanStateChanged and NotifyDelegateFanDriveState calls to SetPercentSetting and SetSpeedSetting to ensure they are only invoked once when transitioning to Off via zero values, preventing duplicate callbacks.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 59 out of 59 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
examples/chef/common/chef-fan-control-manager.cpp:232
SetPercentCurrent()now callsFanControlCluster::SetPercentSetting(...), but the error log still says it failed to set thePercentCurrentattribute. Update the log message (and possibly the function name) to reflect what is actually being written to avoid confusion during debugging.
void ChefFanControlManager::SetPercentCurrent(uint8_t aNewPercentCurrent)
{
ChipLogDetail(NotSpecified, "ChefFanControlManager::SetPercentCurrent: %d", aNewPercentCurrent);
mPercentCurrent = aNewPercentCurrent;
Status status = Status::Success;
if (FanControlCluster * fc = FanControl::FindClusterOnEndpoint(mEndpoint); fc != nullptr)
{
status = fc->SetPercentSetting(DataModel::Nullable<Percent>(mPercentCurrent)).GetStatusCode().GetStatus();
}
else
{
status = Status::UnsupportedEndpoint;
}
if (status != Status::Success)
{
ChipLogError(NotSpecified, "ChefFanControlManager::SetPercentCurrent: failed to set PercentCurrent attribute: %d",
to_underlying(status));
}
examples/chef/common/chef-fan-control-manager.cpp:251
SetSpeedCurrent()now callsFanControlCluster::SetSpeedSetting(...), but the error log still says it failed to set theSpeedCurrentattribute. Update the log message (and/or function naming) so the logs accurately reflect the operation being performed.
void ChefFanControlManager::SetSpeedCurrent(uint8_t aNewSpeedCurrent)
{
mSpeedCurrent = aNewSpeedCurrent;
Status status = Status::Success;
if (FanControlCluster * fc = FanControl::FindClusterOnEndpoint(mEndpoint); fc != nullptr)
{
status = fc->SetSpeedSetting(MakeNullable(aNewSpeedCurrent)).GetStatusCode().GetStatus();
}
else
{
status = Status::UnsupportedEndpoint;
}
if (status != Status::Success)
{
ChipLogError(NotSpecified, "ChefFanControlManager::SetSpeedCurrent: failed to set SpeedCurrent attribute: %d",
to_underlying(status));
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
PR #43408: Size comparison from d803429 to e28962b Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 59 out of 59 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
PR #43408: Size comparison from 59d22d2 to c99f183 Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #43408: Size comparison from 59d22d2 to d399071 Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 59 out of 59 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
examples/air-purifier-app/air-purifier-common/src/air-purifier-manager.cpp:241
PercentSettingWriteCallbackis invoked in response to a PercentSetting attribute change, but it now writes back viaFanControlCluster::SetPercentSetting(...). This is likely to re-trigger the same attribute-change path (feedback loop) and the log message indicates the intent was to update PercentCurrent, not PercentSetting. Consider removing this write-back and lettingFanControlClustermaintain PercentCurrent based onSetOnOffState, or update the callback to only update the current attribute through the proper mechanism.
void AirPurifierManager::PercentSettingWriteCallback(uint8_t aNewPercentSetting)
{
ChipLogDetail(NotSpecified, "AirPurifierManager::PercentSettingWriteCallback: %d", static_cast<int>(aNewPercentSetting));
if (mOnOffClusterOn)
{
FanControlCluster * c = FanControl::FindClusterOnEndpoint(mEndpointId);
Status status = c == nullptr
? Status::UnsupportedEndpoint
: c->SetPercentSetting(DataModel::Nullable<Percent>(aNewPercentSetting)).GetStatusCode().GetStatus();
if (status != Status::Success)
{
ChipLogError(NotSpecified,
"AirPurifierManager::PercentSettingWriteCallback: failed to set PercentCurrent attribute: %d",
to_underlying(status));
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
PR #43408: Size comparison from 59d22d2 to 09299a1 Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #43408: Size comparison from 59d22d2 to 2a15f03 Full report (10 builds for cc13x4_26x4, cc32xx, nrfconnect, realtek, stm32)
|
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
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
Migrates the Fan Control cluster from the legacy Ember implementation to the code-driven architecture.
Changes:
FanControlClusterclass withReadAttribute,WriteAttribute,InvokeCommandfor the Step command, and attribute cascades.CodegenIntegrationRelated issues
#43161
Testing
Tested using
chip-all-clusters-appandchip-repl