Refactoring and Migrating Concentration Measurement Cluster #71608
Draft
marybadalyan wants to merge 13 commits intoproject-chip:masterfrom
Draft
Refactoring and Migrating Concentration Measurement Cluster #71608marybadalyan wants to merge 13 commits intoproject-chip:masterfrom
marybadalyan wants to merge 13 commits intoproject-chip:masterfrom
Conversation
pullapprove
bot
added
review - pending
and removed
examples
app
examples chef
![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 refactors the Concentration Measurement cluster to a code-driven implementation, replacing the previous template-based approach with a delegate-based architecture. It introduces a new ConcentrationMeasurementCluster class and a Delegate interface to handle attribute reads across all 10 aliased concentration clusters. However, the current implementation contains critical bugs: the Delegate base class is missing the SetCluster method required by the cluster's lifecycle, and the DefaultDelegate fails to notify the cluster when attributes change, which will break attribute reporting. Additionally, several unused helper methods and constants were left in the header files and should be removed.
I am having trouble creating individual review comments. Click here to see my feedback.
src/app/clusters/concentration-measurement-server/ConcentrationMeasurementDelegate.h (30-39)
The Delegate class is missing the SetCluster method, which is called by ConcentrationMeasurementCluster in its destructor and Startup method. This will result in a compilation error. A forward declaration of ConcentrationMeasurementCluster is also required to avoid circular dependencies. This addition is consistent with the existing API design for cluster delegates.
class ConcentrationMeasurementCluster;
// Delegate — abstract interface your application implements to supply attribute values.
class Delegate
{
public:
Delegate() = default;
virtual ~Delegate() = default;
virtual void SetCluster(ConcentrationMeasurementCluster * cluster) {}
// Called once by ConcentrationMeasurementCluster::Startup().
// Override to initialise your hardware or sensor driver. Default is a no-op.
virtual CHIP_ERROR Init() { return CHIP_NO_ERROR; }References
- When adding new methods to a class, ensure they are consistent with the existing API design, for example by using class members instead of passing redundant parameters.
src/app/clusters/concentration-measurement-server/ConcentrationMeasurementDelegate.h (143-189)
The DefaultDelegate implementation is missing the logic to notify the cluster of attribute changes. To maintain consistency with existing patterns, DefaultDelegate should store a pointer to the cluster and call NotifyAttributeChanged in its setters. This implementation should use defensive null checks for the cluster pointer and ensure that invalid enum values are not silently ignored.
// ── Handle*()
// Each method stores the value and notifies the cluster of the change to trigger reporting.
void SetCluster(ConcentrationMeasurementCluster * cluster) override { mCluster = cluster; }
void HandleNewMeasuredValue(DataModel::Nullable<float> v)
{
if (mMeasuredValue != v)
{
mMeasuredValue = v;
if (mCluster) mCluster->NotifyAttributeChanged(Attributes::MeasuredValue::Id);
}
}
void HandleNewMinMeasuredValue(DataModel::Nullable<float> v)
{
if (mMinMeasuredValue != v)
{
mMinMeasuredValue = v;
if (mCluster) mCluster->NotifyAttributeChanged(Attributes::MinMeasuredValue::Id);
}
}
void HandleNewMaxMeasuredValue(DataModel::Nullable<float> v)
{
if (mMaxMeasuredValue != v)
{
mMaxMeasuredValue = v;
if (mCluster) mCluster->NotifyAttributeChanged(Attributes::MaxMeasuredValue::Id);
}
}
void HandleNewUncertainty(float v)
{
if (mUncertainty != v)
{
mUncertainty = v;
if (mCluster) mCluster->NotifyAttributeChanged(Attributes::Uncertainty::Id);
}
}
void HandleNewPeakMeasuredValue(DataModel::Nullable<float> v)
{
if (mPeakMeasuredValue != v)
{
mPeakMeasuredValue = v;
if (mCluster) mCluster->NotifyAttributeChanged(Attributes::PeakMeasuredValue::Id);
}
}
void HandleNewPeakMeasuredValueWindow(uint32_t v)
{
if (mPeakMeasuredValueWindow != v)
{
mPeakMeasuredValueWindow = v;
if (mCluster) mCluster->NotifyAttributeChanged(Attributes::PeakMeasuredValueWindow::Id);
}
}
void HandleNewAverageMeasuredValue(DataModel::Nullable<float> v)
{
if (mAverageMeasuredValue != v)
{
mAverageMeasuredValue = v;
if (mCluster) mCluster->NotifyAttributeChanged(Attributes::AverageMeasuredValue::Id);
}
}
void HandleNewAverageMeasuredValueWindow(uint32_t v)
{
if (mAverageMeasuredValueWindow != v)
{
mAverageMeasuredValueWindow = v;
if (mCluster) mCluster->NotifyAttributeChanged(Attributes::AverageMeasuredValueWindow::Id);
}
}
void HandleNewLevelValue(LevelValueEnum v)
{
if (mLevelValue != v)
{
mLevelValue = v;
if (mCluster) mCluster->NotifyAttributeChanged(Attributes::LevelValue::Id);
}
}
private:
// All fields start null/zero/unknown — safe defaults before first reading.
DataModel::Nullable<float> mMeasuredValue;
DataModel::Nullable<float> mMinMeasuredValue;
DataModel::Nullable<float> mMaxMeasuredValue;
DataModel::Nullable<float> mPeakMeasuredValue;
uint32_t mPeakMeasuredValueWindow = 0;
DataModel::Nullable<float> mAverageMeasuredValue;
uint32_t mAverageMeasuredValueWindow = 0;
float mUncertainty = 0.0f;
MeasurementMediumEnum mMedium;
MeasurementUnitEnum mUnit;
LevelValueEnum mLevelValue = LevelValueEnum::kUnknown;
ConcentrationMeasurementCluster * mCluster = nullptr;
};References
- Prefer defensive null checks (e.g., if (mCluster)) over using VerifyOrDie for delegate or cluster pointers to ensure consistency across the repository.
- Use NotifyAttributeChanged for attribute change notifications to align with modern cluster implementation patterns.
- Silently ignoring writes of invalid enum values is considered an unexpected pattern and should be reviewed.
src/app/clusters/concentration-measurement-server/ConcentrationMeasurementCluster.h (135-155)
These static helper methods (CheckConstraintMinMax, CheckConstraintsLessThanOrEqualTo, CheckConstraintsGreaterThanOrEqualTo) are defined but not used anywhere in the new implementation. Since the cluster now only handles attribute reads and delegates value management to the application, these constraints should be enforced by the application/delegate before updating the values. These should be removed to clean up the code.
src/app/clusters/concentration-measurement-server/ConcentrationMeasurementCluster.h (126)
The constant kWindowMaxSeconds is defined but not used within the class. It should be removed if it is no longer needed.
|
PR #71608: Size comparison from a6e7ae0 to e39b415 Full report (23 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, nxp, qpg, realtek, stm32, telink)
|
|
PR #71608: Size comparison from a6e7ae0 to 214c3c3 Full report (9 builds for cc13x4_26x4, cc32xx, realtek, stm32)
|
marybadalyan
force-pushed
the
ConcentMeasureCluster
branch
from
214c3c3 to
4a61ea6
Compare
|
PR #71608: Size comparison from 3a0c11a to 678ab13 Full report (29 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, nxp, psoc6, qpg, realtek, stm32, telink)
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #71608 +/- ##
==========================================
- Coverage 54.40% 54.36% -0.05%
==========================================
Files 1582 1584 +2
Lines 108395 108364 -31
Branches 13384 13399 +15
==========================================
- Hits 58976 58912 -64
- Misses 49419 49452 +33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
PR #71608: Size comparison from 3a0c11a to 4ee42b0 Full report (29 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, nxp, psoc6, qpg, realtek, stm32, telink)
|
marybadalyan
force-pushed
the
ConcentMeasureCluster
branch
from
295f0c4 to
e258ff2
Compare
marybadalyan
force-pushed
the
ConcentMeasureCluster
branch
from
e258ff2 to
5b9fba3
Compare
|
PR #71608: Size comparison from 5f28762 to 5b9fba3 Full report (29 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, nxp, psoc6, qpg, realtek, stm32, telink)
|
Contributor
Author
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the Concentration Measurement cluster implementation to a delegate-based architecture, replacing the previous template-based approach. It introduces new cluster and delegate classes and updates multiple example applications to align with this new structure. Feedback includes a critical issue where ReadAttribute fails to delegate to the base class for global attributes, and a requirement to use NotifyAttributeChanged for attribute reporting in the delegate. Additionally, several instances of dead code (unused helper methods and constants) and a logging format typo were identified.
Comment on lines
+112
to
+114
| default: | ||
| return Status::UnsupportedAttribute; | ||
| } |
Contributor
There was a problem hiding this comment.
The ReadAttribute implementation does not delegate to the base class DefaultServerCluster::ReadAttribute for attributes it doesn't handle (the default case). This will prevent mandatory global attributes like AttributeList, AcceptedCommandList, GeneratedCommandList, and EventList from being readable, even though they are correctly advertised in the Attributes() method.
default:
return DefaultServerCluster::ReadAttribute(request, encoder);
}
Comment on lines
+148
to
+164
| void HandleNewMeasuredValue(DataModel::Nullable<float> v) { mMeasuredValue = v; } | ||
|
|
||
| void HandleNewMinMeasuredValue(DataModel::Nullable<float> v) { mMinMeasuredValue = v; } | ||
|
|
||
| void HandleNewMaxMeasuredValue(DataModel::Nullable<float> v) { mMaxMeasuredValue = v; } | ||
|
|
||
| void HandleNewUncertainty(float v) { mUncertainty = v; } | ||
|
|
||
| void HandleNewPeakMeasuredValue(DataModel::Nullable<float> v) { mPeakMeasuredValue = v; } | ||
|
|
||
| void HandleNewPeakMeasuredValueWindow(uint32_t v) { mPeakMeasuredValueWindow = v; } | ||
|
|
||
| void HandleNewAverageMeasuredValue(DataModel::Nullable<float> v) { mAverageMeasuredValue = v; } | ||
|
|
||
| void HandleNewAverageMeasuredValueWindow(uint32_t v) { mAverageMeasuredValueWindow = v; } | ||
|
|
||
| void HandleNewLevelValue(LevelValueEnum v) { mLevelValue = v; } |
Contributor
There was a problem hiding this comment.
The HandleNew* methods in DefaultDelegate update the internal state but do not trigger attribute reports. In accordance with repository standards, NotifyAttributeChanged should be used for notifications rather than MatterReportingAttributeChangeCallback. The delegate needs a mechanism to notify the cluster of changes so that NotifyAttributeChanged() can be called.
References
- In the Groupcast cluster, use NotifyAttributeChanged for attribute change notifications, not MatterReportingAttributeChangeCallback.
Comment on lines
+136
to
+153
| static bool CheckConstraintMinMax(DataModel::Nullable<float> value, DataModel::Nullable<float> minValue, | ||
| DataModel::Nullable<float> maxValue) | ||
| { | ||
| return (minValue.IsNull() || value.IsNull() || (value.Value() >= minValue.Value())) && | ||
| (maxValue.IsNull() || value.IsNull() || (value.Value() <= maxValue.Value())); | ||
| } | ||
|
|
||
| static bool CheckConstraintsLessThanOrEqualTo(DataModel::Nullable<float> value, | ||
| DataModel::Nullable<float> valueToBeLessThanOrEqualTo) | ||
| { | ||
| return valueToBeLessThanOrEqualTo.IsNull() || value.IsNull() || (value.Value() <= valueToBeLessThanOrEqualTo.Value()); | ||
| } | ||
|
|
||
| static bool CheckConstraintsGreaterThanOrEqualTo(DataModel::Nullable<float> value, | ||
| DataModel::Nullable<float> valueToBeGreaterThanOrEqualTo) | ||
| { | ||
| return valueToBeGreaterThanOrEqualTo.IsNull() || value.IsNull() || (value.Value() >= valueToBeGreaterThanOrEqualTo.Value()); | ||
| } |
Contributor
There was a problem hiding this comment.
These private static helper methods (CheckConstraintMinMax, CheckConstraintsLessThanOrEqualTo, and CheckConstraintsGreaterThanOrEqualTo) are defined but not used anywhere in the cluster implementation. Since this cluster is read-only and delegates data retrieval to the Delegate, these validation helpers are dead code and should be removed.
| { | ||
| mAirQualityInstance.UpdateAirQuality(newValue); | ||
| airQualityInstance.UpdateAirQuality(newValue); | ||
| ChipLogDetail(NotSpecified, "Updated AirQuality value: %huu", chip::to_underlying(newValue)); |
Contributor
There was a problem hiding this comment.
Typo in the log format string: %huu is invalid and should be corrected to %u or %d.
ChipLogDetail(NotSpecified, "Updated AirQuality value: %u", chip::to_underlying(newValue));|
|
||
| private: | ||
| // Per-spec maximum window for PeakMeasuredValueWindow / AverageMeasuredValueWindow: 7 days | ||
| static constexpr uint32_t kWindowMaxSeconds = 604800; |
Contributor
There was a problem hiding this comment.
The constant kWindowMaxSeconds is defined but not used in the current implementation. It should be removed to avoid dead code.
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
2 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
This PR Migrates ConcnetrationMeasurementCluster and migrates it.
Related issues
#71607
Testing