-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Align thermostat cluster better with the spec. #18594
Align thermostat cluster better with the spec. #18594
Conversation
PR #18594: Size comparison from 70e259c to f959293 Increases (4 builds for linux, nrfconnect, p6)
Full report (14 builds for cyw30739, k32w, linux, mbed, nrfconnect, p6, telink)
|
f959293
to
fb27c1e
Compare
PR #18594: Size comparison from 70e259c to fb27c1e Increases (6 builds for esp32, linux, nrfconnect, p6)
Full report (21 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
fb27c1e
to
ba9d5b8
Compare
src/app/zap-templates/zcl/data-model/chip/thermostat-cluster.xml
Outdated
Show resolved
Hide resolved
Specific changes: * Adjust names of attributes to match the spec. * Mark the attributes that are supposed to be nullable as nullable. * Make the min values be decimal for various attributes, for readability. * Add the various setback attributes and EmergencyHeatDelta. * Fix incorrect fourth field for SetWeeklySchedule and GetWeeklyScheduleResponse. * Fix the types for the NumberOfTransitionsForSequence fields in SetWeeklySchedule and GetWeeklyScheduleResponse. * Fix incorrect type of RelayStatus in GetRelayStatusLogResponse. CHIP-Specifications/connectedhomeip-spec#5238 tracks the outstanding spec issue about the fields of the ThermostatScheduleTransition struct. Fixes project-chip#11670
ba9d5b8
to
c3a74f2
Compare
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.
If my understanding is correct the only files to review are:
- src/app/zap-templates/zcl/data-model/chip/thermostat-cluster.xml
- src/controller/python/chip/clusters/CHIPClusters.py
- src/controller/python/chip/clusters/Objects.py
Reading it along side the spec everything LGTM. But I am still pretty green
PR #18594: Size comparison from 329389d to c3a74f2 Increases above 0.2%:
Increases (12 builds for cc13x2_26x2, esp32, linux, nrfconnect, p6)
Decreases (2 builds for cc13x2_26x2)
Full report (37 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
The file to review is the .xml along with the spec, but the .matter files can be helpful in reviewing. The .py files you list are auto-generated, so don't really need to be reviewed. |
Oh, and the other files with manual changes here were the .yaml test files. |
Specific changes:
GetWeeklyScheduleResponse.
SetWeeklySchedule and GetWeeklyScheduleResponse.
https://github.com/CHIP-Specifications/connectedhomeip-spec/issues/5238
tracks the outstanding spec issue about the fields of the
ThermostatScheduleTransition struct.
Fixes #11670
Problem
XML does not match spec.
Change overview
Align the two, as much as possible given the state of the spec.
Testing
Examined some of the generated code. The existing CI for thermostat cluster should keep passing, but we should also be able to enable more YAML tests now.