[climate] Implement feature flags field to replace multiple bools #1376
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1376 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 3287 3310 +23
=========================================
+ Hits 3287 3310 +23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #1376 will not alter performanceComparing Summary
Footnotes |
WalkthroughAdded a uint32 Changes
Sequence Diagram(s)(omitted — changes do not introduce new control-flow requiring a sequence diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kbx81
changed the title
requires_two_point_target_temperature flagclimate
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
aioesphomeapi/api.proto (1)
974-999: Mark the deprecated climate booleans with protobuf deprecation markersYou flagged these booleans as deprecated in comments, but unlike the rest of the schema we aren’t setting
[deprecated=true]. Adding the annotation keeps codegen warnings consistent and nudges consumers to adoptfeature_flags.- bool supports_current_temperature = 5; // Deprecated: use feature_flags - bool supports_two_point_target_temperature = 6; // Deprecated: use feature_flags + bool supports_current_temperature = 5 [deprecated=true]; // Use feature_flags + bool supports_two_point_target_temperature = 6 [deprecated=true]; // Use feature_flags @@ - bool supports_action = 12; // Deprecated: use feature_flags + bool supports_action = 12 [deprecated=true]; // Use feature_flags @@ - bool supports_current_humidity = 22; // Deprecated: use feature_flags - bool supports_target_humidity = 23; // Deprecated: use feature_flags + bool supports_current_humidity = 22 [deprecated=true]; // Use feature_flags + bool supports_target_humidity = 23 [deprecated=true]; // Use feature_flags
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
aioesphomeapi/api.proto(3 hunks)aioesphomeapi/model.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- aioesphomeapi/model.py
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
- Do not generate or add any sequence diagrams
Files:
aioesphomeapi/api.proto
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run benchmarks
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_model.py (1)
339-376: Well-structured compatibility test.The test correctly validates the backward compatibility logic for ClimateInfo feature flags across API versions. The parametrization covers individual flags for API < 1.13 and demonstrates the new feature_flags field for API >= 1.13.
Consider adding optional test cases to improve coverage:
- Combinations of multiple boolean flags for API < 1.13 (e.g.,
supports_current_temperature=True, supports_action=True)- Distinct testing of
SUPPORTS_TWO_POINT_TARGET_TEMPERATUREvsREQUIRES_TWO_POINT_TARGET_TEMPERATUREflags for API >= 1.13- Edge case verification at exactly API version (1, 13)
Example test case for multiple flags:
( ClimateInfo( supports_current_temperature=True, supports_action=True, ), (1, 12), ClimateFeature.SUPPORTS_CURRENT_TEMPERATURE | ClimateFeature.SUPPORTS_ACTION, ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_model.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
- Do not generate or add any sequence diagrams
Files:
tests/test_model.py
🧬 Code graph analysis (1)
tests/test_model.py (1)
aioesphomeapi/model.py (4)
ClimateFeature(115-121)ClimateInfo(654-719)supported_feature_flags_compat(695-710)APIVersion(100-102)
🔇 Additional comments (1)
tests/test_model.py (1)
86-86: LGTM!The import of
ClimateFeatureis correctly added to support the new compatibility test.
kbx81
changed the title
climate…etpoint-required-flag
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
aioesphomeapi/model.py (1)
115-122: LGTM! ClimateFeature enum follows established pattern.The
ClimateFeatureIntFlag enum is well-defined and follows the same pattern asBluetoothProxyFeatureandVoiceAssistantFeaturein the codebase. The bit-shift values are correct and non-overlapping.Consider adding docstring comments to clarify the semantic relationship between
SUPPORTS_TWO_POINT_TARGET_TEMPERATUREandREQUIRES_TWO_POINT_TARGET_TEMPERATUREflags (e.g., whether requiring implies supporting).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
aioesphomeapi/api.proto(3 hunks)aioesphomeapi/model.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- aioesphomeapi/api.proto
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
- Do not generate or add any sequence diagrams
Files:
aioesphomeapi/model.py
🧬 Code graph analysis (1)
aioesphomeapi/model.py (1)
aioesphomeapi/client_base.py (1)
api_version(329-332)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: py 3.12 on windows-latest (skip_cython)
- GitHub Check: Run benchmarks
🔇 Additional comments (2)
aioesphomeapi/model.py (2)
655-655: LGTM! Field addition follows dataclass conventions.The
feature_flags: int = 0field is correctly added with an appropriate default value for a flags field.
695-710: Verify flag mapping for two-point target temperature compatibility.The compatibility method correctly follows the established pattern (similar to
bluetooth_proxy_feature_flags_compatandvoice_assistant_feature_flags_compat). The API version check at 1.13 aligns with the version bump in connection.py.However, the mapping on line 702 sets only
REQUIRES_TWO_POINT_TARGET_TEMPERATUREwithout also settingSUPPORTS_TWO_POINT_TARGET_TEMPERATURE. While the comment indicates this is intentional to "mimic previous behavior," verify that:
- The test coverage in
tests/test_model.pyvalidates this specific flag combination for old API versions- Consumer code correctly handles cases where
REQUIRESis set withoutSUPPORTSfor backward compatibility- For new devices (API >= 1.13), devices that require two-point target temperature set both flags in
feature_flagsBased on past review comments, this implementation aligns with the reviewer's suggestions.
…etpoint-required-flag
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
aioesphomeapi/api.proto (2)
990-992: Make deprecations explicit and consistent ([deprecated=true] + version note).Elsewhere in this proto, deprecated fields use
[deprecated=true]and “Deprecated in API version …”. Do the same here to guide consumers and keep consistency.- bool supports_current_temperature = 5; // Deprecated: use feature_flags + // Deprecated in API version 1.13: use feature_flags + bool supports_current_temperature = 5 [deprecated=true]; - bool supports_two_point_target_temperature = 6; // Deprecated: use feature_flags + // Deprecated in API version 1.13: use feature_flags + bool supports_two_point_target_temperature = 6 [deprecated=true]; - bool supports_action = 12; // Deprecated: use feature_flags + // Deprecated in API version 1.13: use feature_flags + bool supports_action = 12 [deprecated=true]; - bool supports_current_humidity = 22; // Deprecated: use feature_flags + // Deprecated in API version 1.13: use feature_flags + bool supports_current_humidity = 22 [deprecated=true]; - bool supports_target_humidity = 23; // Deprecated: use feature_flags + // Deprecated in API version 1.13: use feature_flags + bool supports_target_humidity = 23 [deprecated=true];Also applies to: 1000-1001, 1010-1011
1015-1015: Add comment on feature_flags semantics
feature_flags(tag 27) is aligned with server PR 10987. Please add a one-line comment in aioesphomeapi/api.proto:uint32 feature_flags = 27; // Bitmask of climate capabilities (see ClimateFeature enum for bit values)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
aioesphomeapi/api.proto(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
- Do not generate or add any sequence diagrams
Files:
aioesphomeapi/api.proto
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: py 3.12 on windows-latest (skip_cython)
…etpoint-required-flag
2 participants
What does this implement/fix?
Implements a feature flags system for climate entities to replace multiple individual boolean capability flags. This addresses home-assistant/core#153331 by introducing a new
REQUIRES_TWO_POINT_TARGET_TEMPERATUREflag to distinguish between devices that optionally support dual setpoint vs. those that require it.Changes
feature_flagsfield (uint32) toListEntitiesClimateResponsein api.protoClimateFeatureIntFlag enum with flags for all climate capabilitiessupports_current_temperaturesupports_two_point_target_temperaturesupports_actionsupports_current_humiditysupports_target_humidityREQUIRES_TWO_POINT_TARGET_TEMPERATUREflag to handle mandatory dual setpointBreaking Changes
IMPORTANT: This PR is marked as breaking to alert API consumers that migration is required, even though nothing breaks immediately.
The boolean fields (
supports_current_temperature,supports_two_point_target_temperature,supports_action,supports_current_humidity,supports_target_humidity) are deprecated and will be removed in a future release (likely with another major version bump).Why this is marked breaking:
REQUIRES_TWO_POINT_TARGET_TEMPERATUREflag, which is only available via the feature flags systemWhat will break later when fields are removed:
All code must migrate to using the feature flags system now to access the new functionality and avoid breakage in the future.
Migration required:
The
supported_feature_flags_compat(api_version)method handles both old and new API versions:feature_flagsfield directly (includes all flags, old and new)See also:
climateesphome#10987climatehome-assistant/core#153507Types of changes
Related issue or feature (if applicable):
Pull request in esphome (if applicable):
Checklist:
tests/folder).