[HVAC] Thermostat events implementation #39923
Draft
+449
−2
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
There was a problem hiding this comment.
Code Review
This pull request effectively introduces the thermostat events implementation. The changes are well-structured, with new files for event handling and updates to the build system. The modifications to the test scripts to verify event emission are a great addition.
I've identified a couple of areas for improvement. There's a minor correctness issue in how event data is populated for unoccupied setpoint changes, and I've noticed a potential for race conditions in the tests when multiple events are expected. Addressing these points will enhance the robustness of the implementation.
Comment on lines
+361
to
+365
| if self.pics_guard(hasAutoModeFeature): | ||
| if hasEventsFeature: | ||
| # If we have auto mode, this will have also adjusted the cooling setpoint to preserve the deadband | ||
| await self.check_setpoint_event(events_callback=events_callback, attribute=cluster.Attributes.OccupiedCoolingSetpoint, system_mode=cluster.Enums.SystemModeEnum.kCool, occupancy=cluster.Bitmaps.OccupancyBitmap.kOccupied, endpoint=endpoint) | ||
| await self.check_setpoint_event(events_callback=events_callback, attribute=cluster.Attributes.OccupiedHeatingSetpoint, system_mode=cluster.Enums.SystemModeEnum.kHeat, occupancy=cluster.Bitmaps.OccupancyBitmap.kOccupied, endpoint=endpoint) |
There was a problem hiding this comment.
This test logic assumes a specific order for receiving events, which can lead to flaky tests. When a setpoint is changed in auto mode, the other setpoint might be adjusted to maintain the deadband, resulting in two SetpointChange events. The order in which these events are received is not guaranteed.
A more robust approach would be to collect all SetpointChange events received within a short timeframe and then verify that both expected events (one for heating, one for cooling) are present, without assuming their order.1
Style Guide References
Footnotes
|
PR #39923: Size comparison from 5dc7824 to a24ea6b Increases above 0.2%:
Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #39923: Size comparison from 5dc7824 to ee0e4b8 Increases above 0.2%:
Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nxp, psoc6, qpg, stm32, telink, tizen)
|
…at_events # Conflicts: # src/app/zap-templates/zcl/data-model/chip/thermostat-cluster.xml
This reverts commit 56bf19d.
|
PR #39923: Size comparison from d033b40 to 5763827 Increases above 0.2%:
Full report (59 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
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.
Summary
This adds the thermostat events implementation to the thermostat server and example app, along with modifications to the test scripts to check for event emission.
Note: this contains all the XML changes in #39918, so should not be merged before it.
Testing
Added unit tests and modified existing tests to match the changes in PR 5307