[Draft] Thermostat - Relocate setpoint logic to separate file#42326
Draft
hasty wants to merge 9 commits intoproject-chip:masterfrom
Draft
[Draft] Thermostat - Relocate setpoint logic to separate file#42326hasty wants to merge 9 commits intoproject-chip:masterfrom
hasty wants to merge 9 commits intoproject-chip:masterfrom
Conversation
hasty
changed the title
hasty
changed the title
![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 successfully refactors the Thermostat cluster by moving setpoint-related logic into new dedicated files, ThermostatClusterSetpoints.cpp and ThermostatClusterSetpoints.h. This improves modularity and prepares for a code-driven cluster implementation. The changes also include updating the default value for minSetpointDeadBand across various examples and tests, which is a good consistency improvement. My review includes suggestions to further improve the refactoring by using the new GetSetpointLimits helper function in a couple of places where limit attributes are still being fetched manually, aligning with the rule to extract common logic into helper functions. This will make the code cleaner and more consistent.
src/app/clusters/thermostat-server/ThermostatClusterSetpoints.cpp
Outdated
Show resolved
Hide resolved
|
PR #42326: Size comparison from efdf99d to 99f09f4 Full report (32 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, psoc6, qpg, realtek, stm32, telink)
|
|
PR #42326: Size comparison from efdf99d to 18fd2e4 Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
PR #42326: Size comparison from 9a96230 to 8a7bc0a Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Contributor
|
Any news? |
# Conflicts: # examples/all-clusters-app/linux/main-common.cpp
…ter tests to change it
hasty
force-pushed
the
granbery/cluster_driven_thermostat_setpoints
branch
from
bc1c1b7 to
e9b7549
Compare
hasty
force-pushed
the
granbery/cluster_driven_thermostat_setpoints
branch
from
e9b7549 to
f5cbd69
Compare
|
PR #42326: Size comparison from a914062 to f5cbd69 Full report (3 builds for realtek, stm32)
|
hasty
force-pushed
the
granbery/cluster_driven_thermostat_setpoints
branch
3 times, most recently
from
41a0d50 to
18932e9
Compare
|
PR #42326: Size comparison from a914062 to 18932e9 Full report (28 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, psoc6, qpg, realtek, stm32, telink)
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #42326 +/- ##
==========================================
+ Coverage 54.32% 54.35% +0.02%
==========================================
Files 1577 1583 +6
Lines 108271 108218 -53
Branches 13401 13342 -59
==========================================
Hits 58820 58820
+ Misses 49451 49398 -53 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
hasty
force-pushed
the
granbery/cluster_driven_thermostat_setpoints
branch
2 times, most recently
from
0d736be to
4d0bf50
Compare
|
PR #42326: Size comparison from a914062 to 4d0bf50 Full report (5 builds for cc32xx, realtek, stm32)
|
hasty
force-pushed
the
granbery/cluster_driven_thermostat_setpoints
branch
from
4d0bf50 to
2ff1353
Compare
|
PR #42326: Size comparison from a914062 to 2ff1353 Full report (21 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, psoc6, qpg, realtek, stm32)
|
hasty
force-pushed
the
granbery/cluster_driven_thermostat_setpoints
branch
from
2ff1353 to
9d5db83
Compare
|
PR #42326: Size comparison from a914062 to 9d5db83 Full report (12 builds for cc13x4_26x4, cc32xx, nrfconnect, qpg, realtek, stm32)
|
hasty
force-pushed
the
granbery/cluster_driven_thermostat_setpoints
branch
from
9d5db83 to
a6edcdb
Compare
|
PR #42326: Size comparison from a914062 to a6edcdb Full report (10 builds for cc13x4_26x4, cc32xx, nrfconnect, realtek, stm32)
|
hasty
force-pushed
the
granbery/cluster_driven_thermostat_setpoints
branch
from
a6edcdb to
5fa7767
Compare
|
PR #42326: Size comparison from a914062 to 5fa7767 Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, 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 Thermostat cluster server implementation, transitioning from a monolithic structure to a modular design with dedicated classes for setpoint and limit management. Key additions include the Setpoints and SetpointLimits components to handle complex validation and deadband logic. The review identifies critical architectural issues, specifically the performance of attribute writes within the PreAttributeChangedCallback, which violates the SDK's concurrency model and can lead to infinite recursion. Additionally, the AttributeAccessInterface implementation incorrectly returns success for unhandled attributes, which would prevent the system from falling back to the default attribute store. Other feedback highlights missing error propagation in the SetpointRaiseLower command, a potential specification violation regarding the default deadband value, and missing range validation for deadband updates.
| } | ||
| if (status == Status::Success && affectedAttributes.HasAny()) | ||
| { | ||
| status = SaveSetpoints(attributePath.mEndpointId, setpoints, affectedAttributes); |
Contributor
There was a problem hiding this comment.
The HandleSetpointChange function is called from MatterThermostatClusterServerPreAttributeChangedCallback. Invoking SaveSetpoints here performs attribute writes, which are side effects. As noted in the comment at line 44 of ThermostatCluster.cpp, side effects are not permitted in the Pre callback. Performing writes here can lead to inconsistent state if the current change is later rejected, and it can cause infinite recursion as SaveSetpoints triggers further Pre callbacks. This logic should be moved to the Post callback (MatterThermostatClusterServerAttributeChangedCallback).
| break; | ||
| } | ||
|
|
||
| return CHIP_NO_ERROR; |
Contributor
There was a problem hiding this comment.
In an AttributeAccessInterface, returning CHIP_NO_ERROR indicates that the attribute was successfully handled and encoded. For attributes not handled by this interface (which should fall back to the default attribute store), it must return CHIP_ERROR_NOT_FOUND. Returning CHIP_NO_ERROR without encoding anything will lead to malformed responses or internal errors in the Interaction Model engine. This applies to the Write method as well (line 365).
return CHIP_ERROR_NOT_FOUND;References
- In ReadAttribute handlers, do not add redundant checks for attribute existence, as API contracts guarantee they are only called for existent paths. The only requirement is to not crash on an invalid read and return the appropriate status for handled vs unhandled paths.
Comment on lines
+210
to
+218
| chip::BitFlags<SetpointAttributes> affectedAttributes; | ||
| status = setpoints.ChangeRange(range, heat, cool, affectedAttributes); | ||
| if (status == Status::Success) | ||
| { | ||
| return SaveSetpoints(endpointId, setpoints, affectedAttributes); | ||
| } | ||
| } | ||
|
|
||
| return Status::Success; |
Contributor
There was a problem hiding this comment.
The function should return the actual status of the operation. If ChangeRange fails (e.g., due to deadband constraints), the error should be propagated to the caller rather than returning Status::Success.
chip::BitFlags<SetpointAttributes> affectedAttributes;
status = setpoints.ChangeRange(range, heat, cool, affectedAttributes);
if (status == Status::Success)
{
return SaveSetpoints(endpointId, setpoints, affectedAttributes);
}
}
return status;References
- Always check the return value of initialization or logic functions that can fail to ensure robust operation and prevent unstable states.
| constexpr int16_t kDefaultAbsMaxHeatSetpointLimit = 3000; // 30C (86 F) is the default | ||
| constexpr int16_t kDefaultAbsMinCoolSetpointLimit = 1600; // 16C (61 F) is the default | ||
| constexpr int16_t kDefaultAbsMaxCoolSetpointLimit = 3200; // 32C (90 F) is the default | ||
| constexpr int16_t kDefaultDeadBand = 200; // 2.0C is the default |
Contributor
There was a problem hiding this comment.
The default value for MinSetpointDeadBand is changed from 25 (2.5°C) to 20 (2.0°C). According to the Matter specification (e.g., v1.3, Section 4.3.8.14), the default value for this attribute is 25. Changing this default may cause compliance issues or unexpected behavior for clients expecting the standard default value.
| #define FEATURE_MAP_SB 0x10 | ||
| #define FEATURE_MAP_AUTO 0x20 | ||
|
|
||
| #define FEATURE_MAP_DEFAULT FEATURE_MAP_HEAT | FEATURE_MAP_COOL | FEATURE_MAP_AUTO |
Contributor
There was a problem hiding this comment.
It is safer to wrap the macro definition in parentheses to avoid potential operator precedence issues when used in expressions.
#define FEATURE_MAP_DEFAULT (FEATURE_MAP_HEAT | FEATURE_MAP_COOL | FEATURE_MAP_AUTO)
Comment on lines
+131
to
+133
| case MinSetpointDeadBand::Id: { | ||
| return Status::Success; | ||
| } |
Contributor
There was a problem hiding this comment.
The HandleSetpointChange for MinSetpointDeadBand currently performs no validation. The previous implementation checked if the value was within the valid range (0-127) and if AutoMode was supported. Additionally, changing the deadband might require re-validating or adjusting existing setpoints to maintain the new deadband constraint.
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
This PR moves the setpoint functionality of the Thermostat cluster into a separate file, in preparation for migrating to a code-driven-cluster implementation of Thermostat.
Related issues
This is meant to be merged after #41739 and #41772.
Testing
Ran test suite.