[HVAC] fix TRV attr MinSetpointDeadBand shall allow 0-127 but ignore any attempted change #38834
Conversation
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
kliffwong
force-pushed
the
mr/fix-trv-minsetpointdeadband
branch
from
911ac7f to
2a16290
Compare
|
PR #38834: Size comparison from b8d1181 to 2a16290 Full report (3 builds for cc32xx, stm32)
|
kliffwong
force-pushed
the
mr/fix-trv-minsetpointdeadband
branch
from
2a16290 to
9271e7b
Compare
|
PR #38834: Size comparison from 1f6aada to 9271e7b Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
kliffwong
force-pushed
the
mr/fix-trv-minsetpointdeadband
branch
from
9271e7b to
08c0596
Compare
|
PR #38834: Size comparison from 8167c5f to 08c0596 Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
previously requested changes
May 12, 2025
There was a problem hiding this comment.
@kliffwong please provide details for testing instead of manual:
What commands did you run, what were the observed results, how do they prove that this shows code working correclty, why is this test not auomatisable?
I believe cluster code test should be automatable. Please prefer to automate it and if not possible ,describe in the summary why that is so.
The bar for manual tests is set high on purpose: we try to strongly nudge people to write automated tests instead of marking things as 'tested manually' for expedience.
|
PR #38834: Size comparison from d0ac18d to 9ff7cda Full report (14 builds for cc13x4_26x4, cc32xx, nrfconnect, qpg, stm32, tizen)
|
Indeed Test Harness can be used as automated test for thess changes. |
andy31415
reviewed
May 12, 2025
You have to implement the test plan or explain when it will be later (i.e. very soon and explain why you cannot do it right now). Code changes should come with tests. |
|
PR #38834: Size comparison from e4590b6 to 9b05731 Full report (14 builds for cc13x4_26x4, cc32xx, nrfconnect, qpg, stm32, tizen)
|
kliffwong
force-pushed
the
mr/fix-trv-minsetpointdeadband
branch
from
9b05731 to
6472de8
Compare
|
PR #38834: Size comparison from eda485e to 6472de8 Full report (46 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, nrfconnect, psoc6, qpg, stm32, telink, tizen)
|
kliffwong
force-pushed
the
mr/fix-trv-minsetpointdeadband
branch
from
6472de8 to
289a2f2
Compare
Test plan TC_TSTAT_2_2.py updated |
kliffwong
force-pushed
the
mr/fix-trv-minsetpointdeadband
branch
3 times, most recently
from
759b7ab to
bf7267d
Compare
kliffwong
force-pushed
the
mr/fix-trv-minsetpointdeadband
branch
from
fca7219 to
94e3f1f
Compare
|
PR #38834: Size comparison from f23bb25 to 7656f63 Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
reviewed
May 12, 2025
andy31415
reviewed
May 12, 2025
kliffwong
force-pushed
the
mr/fix-trv-minsetpointdeadband
branch
from
7656f63 to
6fcc7c1
Compare
kliffwong
force-pushed
the
mr/fix-trv-minsetpointdeadband
branch
from
6fcc7c1 to
f43166f
Compare
kliffwong
changed the title
|
PR #38834: Size comparison from 70e5039 to cd95d89 Full report (27 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, qpg, stm32, telink, tizen)
|
kliffwong
force-pushed
the
mr/fix-trv-minsetpointdeadband
branch
3 times, most recently
from
609d591 to
f021607
Compare
|
PR #38834: Size comparison from 16ba524 to f021607 Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #38834: Size comparison from 53ed908 to d7e3949 Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #38834: Size comparison from d6b2cee to 514e071 Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #38834: Size comparison from 6915d2e to e7dc5c6 Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #38834: Size comparison from 1125b84 to 1c5711e Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #38834: Size comparison from 2ed64bc to e36db39 Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
kliffwong
force-pushed
the
mr/fix-trv-minsetpointdeadband
branch
from
75636d1 to
d252a43
Compare
|
PR #38834: Size comparison from d3df54e to d252a43 Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
reviewed
May 20, 2025
src/python_testing/TC_TSTAT_2_2.py
Outdated
| newval = min(newval, (TempOccupiedCoolingSetpointValue - TempOccupiedHeatingSetpointValue)) # note 5 | ||
| newval = min(newval, (TempUnoccupiedCoolingSetpointValue - TempUnoccupiedHeatingSetpointValue)) # note 6 | ||
| newval = max(0, newval) # cannot be negative | ||
| asserts.assert_not_equal(newval, val) # valid value different from original value cannot be found |
There was a problem hiding this comment.
newval was *10 above on line 610 ... so this check seems to test different ranges (0.01C vs 0.1C) ...
Do we need all this complexity? Could we have a IsValidTemperature and maybe loop through some options or try val-1 and val+1 only? the +-5 and multiplication and division seems very complex...
There was a problem hiding this comment.
All other values are in scale of 0.01C, except the deadband value is in scale of 0.1C, so the conversion is needed before the checkings.
The 6 checks are necessarily required in the test spec to ensure the value is valid before sending to the node and I have no confidence if +-1 or +-5 can generate a valid value for all cases
There was a problem hiding this comment.
So you try to find a valid value for MinSetpointDeadBand:
val = await self.read_single_attribute_check_success(endpoint=endpoint, cluster=cluster, attribute=cluster.Attributes.MinSetpointDeadBand)
...
newval *= 10
...
asserts.assert_not_equal(newval, val) # valid value different from original value cannot be found
validval = (newval / 10)
...
self.write_single_attribute(attribute_value=cluster.Attributes.MinSetpointDeadBand(validval), endpoint_id=endpoint)
So why if newval != val, do we write "newval / 10"? You modify newval after comparison. You should write it before. Also is this computation integer-safe? Why don't you keep computations in the 0.01 scale?
There was a problem hiding this comment.
As discussed offline, only smaller values are guaranteed valid. In case of 0 read from server, we randomly pick a value to test
kliffwong
force-pushed
the
mr/fix-trv-minsetpointdeadband
branch
2 times, most recently
from
41e6c9f to
ebfbb1c
Compare
…e any attempted change
kliffwong
force-pushed
the
mr/fix-trv-minsetpointdeadband
branch
from
ebfbb1c to
ea0dc6f
Compare
|
PR #38834: Size comparison from feba8fd to ea0dc6f Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
approved these changes
May 21, 2025
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.
Testing