-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Bug fix for thermostat application for wifi #28427
base: master
Are you sure you want to change the base?
Bug fix for thermostat application for wifi #28427
Conversation
KiranKumar seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
@@ -349,11 +349,11 @@ MatterThermostatClusterServerPreAttributeChangedCallback(const app::ConcreteAttr | |||
requested = static_cast<int16_t>(chip::Encoding::LittleEndian::Get16(value)); | |||
if (!HeatSupported) | |||
return imcode::UnsupportedAttribute; | |||
if (requested < AbsMinHeatSetpointLimit || requested < MinHeatSetpointLimit || requested > AbsMaxHeatSetpointLimit) | |||
if (requested < AbsMinHeatSetpointLimit || requested < MinHeatSetpointLimit || requested < AbsMaxHeatSetpointLimit) |
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 change doesn't seem correct. Shouldn't we return InvalidValue
if the requested value is superior to the AbsMaxHeatSetpointLimit
?
@kiran0284 Could you change the name of the PR by removing the |
PR #28427: Size comparison from e059202 to bee2ab1 Increases (18 builds for bl602, cc32xx, esp32, nrfconnect, psoc6, telink)
Decreases (9 builds for bl702, bl702l, cyw30739, esp32, telink)
Full report (60 builds for bl602, bl702, bl702l, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
@@ -297,7 +297,7 @@ MatterThermostatClusterServerPreAttributeChangedCallback(const app::ConcreteAttr | |||
return imcode::InvalidValue; | |||
if (AutoSupported) | |||
{ | |||
if (requested < OccupiedHeatingSetpoint + DeadBandTemp) | |||
if (requested > OccupiedHeatingSetpoint + DeadBandTemp) |
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 change is wrong, as far as I can tell. What the spec says is:
if, and only if, the AUTO feature is supported, a deadband must be maintained between Heating and Cooling setpoints and limits:
...
OccupiedHeatingSetpoint ≤ (OccupiedCoolingSetpoint - MinSetpointDeadBand)
So if that condition is violated, i.e. "OccupiedHeatingSetpoint + MinSetpointDeadBand > OccupiedCoolingSetpoint", then that's not a valid setting. And that's what this code is checking for.
Please clearly explain why you are introducing these changes that violate the spec (and incidentally fail the conformance tests like Test_TC_TSTAT_2_2).
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Added :
- added fix for OccupiedCoolingSetpoint and MaxHeatSetpointLimit write command in thermostat cluster
Issue :
- while trying set OccupiedCoolingSetpoint and MaxHeatSetpointLimit got CONSTRAINT_ERROR instead of SUCCESS
Tested :
- manually tested on MG24 + 917 Exp