[HVAC] Thermostat deadband handling #35673
Open
+303
−41
Conversation
This file contains 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
|
Review changes with SemanticDiff. |
github-actions
bot
added
app
tests
matter-1.4-te2-script-change
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
axelnxp,
bauerschwan,
bzbarsky-apple,
carol-apple,
cecille,
chapongatien,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
doru91,
fessehaeve,
harimau-qirex and
harsha-rajendran
tcarmelveilleux
approved these changes
Sep 19, 2024
|
PR #35673: Size comparison from afb1a33 to 8b0f0cf Increases above 0.2%:
Full report (80 builds for bl602, bl702, bl702l, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
|
PR #35673: Size comparison from afb1a33 to b3bbfa2 Increases above 0.2%:
Full report (82 builds for bl602, bl702, bl702l, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
cjandhyala
reviewed
Sep 23, 2024
tcarmelveilleux
approved these changes
Sep 26, 2024
|
PR #35673: Size comparison from db11057 to 6a4a0ed Increases above 0.2%:
Full report (11 builds for cc32xx, nrfconnect, nxp, stm32, tizen)
|
|
PR #35673: Size comparison from db11057 to 3d70846 Increases above 0.2%:
Full report (88 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
… granbery/thermostat_deadband_fix
|
PR #35673: Size comparison from 55e2a3f to 6da9f85 Increases above 0.2%:
Full report (88 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
| if (OccupiedCoolingSetpoint::Get(endpoint, &setpointLimits.occupiedCoolingSetpoint) != Status::Success) | ||
| { | ||
| ChipLogError(Zcl, "Error: Can not read Occupied Cooling Setpoint"); | ||
| return Status::Failure; |
There was a problem hiding this comment.
Is there a reason we're not returning the actual status from the Get() here? Similar in various places below.
If there is a reason (e.g. we don't want to claim UnsupportedAttribute for an attribute write to some attribute that's different from the one being actually gotten here), it would be good to document that reason.
There was a problem hiding this comment.
In an attempt to keep the PR small, I put in a TODO down on line 829 to change the existing code to call this function, so that we wouldn't have two bits of code that fetch all the setpoints. For that to work and be consistent with the existing SDK, this function has to replicate what the existing code does.
There was a problem hiding this comment.
@hasty my point is that if it's important to not propagate the return value, that should be documented in the code, so someone does not "fix" it later.
| if (maxValidHeatingSetpoint < minHeatingSetpoint) | ||
| { | ||
| // If we need to adjust the heating setpoint to preserve the deadband, it will go below the min heat setpoint | ||
| return Status::InvalidValue; |
There was a problem hiding this comment.
So where does the spec say to return InvalidValue in this case? Here's what I see in the spec, for example for the OccupiedCoolingSetpoint attribute:
If an attempt is made to set this attribute to a value greater than MaxCoolSetpointLimit or less than MinCoolSetpointLimit, a response with the status code CONSTRAINT_ERROR SHALL be returned.
If this attribute is set to a value that is less than (OccupiedHeatingSetpoint + MinSetpointDeadBand), the value of OccupiedHeatingSetpoint SHALL be adjusted to (OccupiedCoolingSetpoint - MinSetpointDeadBand).
That second sentence does not seem to envision what happens when the value of OccupiedHeatingSetpoint cannot thus be adjusted. But that first items seems to suggest that trying to do the adjustment should result in ConstraintError, no?
There was a problem hiding this comment.
We can certainly improve the spec to explain that the server won't allow you to violate the deadband.
InvalidValue is ConstraintError; I'm only using it here because all the setpoint code uses InvalidValue, and again, trying to keep the PR consistent and small.
There was a problem hiding this comment.
InvalidValue is the old, deprecated name... But OK, maybe a followup to switch it globally here?
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
|
PR #35673: Size comparison from a854245 to e9b72fe Increases above 0.2%:
Full report (3 builds for cc32xx, stm32)
|
|
PR #35673: Size comparison from a854245 to 5d1c13b Increases above 0.2%:
Full report (88 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
bzbarsky-apple
approved these changes
Oct 9, 2024
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.
This syncs the deadband handling behavior in thermostat-server with the spec; instead of rejecting any setpoint writes which violate the deadband, it attempts to shift the complementary setpoint to preserve the deadband, only erroring if this violates the min/max for the setpoint.
Fixes #36191