Add temperature unit for climate and water heater#1586
Add temperature unit for climate and water heater#1586jhenkens wants to merge 5 commits intoesphome:mainfrom
Conversation
WalkthroughThis PR adds support for temperature unit tracking to climate and water heater entities by introducing a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
aioesphomeapi/api.proto (1)
1096-1096: Consider documenting unit semantics on info fields.Add a brief comment that climate/water-heater temperature values in state/command messages are expressed in this
temperature_unit, to prevent consumer ambiguity.Also applies to: 1196-1196
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aioesphomeapi/api.proto` at line 1096, The proto field temperature_unit (type ClimateTemperatureUnit) lacks unit semantics; add a brief comment above the field(s) (e.g., the ClimateTemperatureUnit temperature_unit = 28; occurrences and the similar occurrence around 1196) stating that all climate/water-heater temperature values in related state and command messages are expressed in this unit so consumers know how to interpret numeric temperature fields; reference the symbol temperature_unit and ClimateTemperatureUnit in the comment for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@aioesphomeapi/model.py`:
- Around line 743-745: The temperature enum fields are annotated as non-optional
but their converters (ClimateTemperatureUnit.convert and
WaterHeaterTemperatureUnit.convert) can return None; update the field
annotations temperature_unit (and the analogous water heater field) to be
Optional[ClimateTemperatureUnit] and Optional[WaterHeaterTemperatureUnit]
respectively, add/import typing.Optional if missing, and ensure the
converter_field calls remain unchanged so the post-init assignment of None
satisfies the type contract.
---
Nitpick comments:
In `@aioesphomeapi/api.proto`:
- Line 1096: The proto field temperature_unit (type ClimateTemperatureUnit)
lacks unit semantics; add a brief comment above the field(s) (e.g., the
ClimateTemperatureUnit temperature_unit = 28; occurrences and the similar
occurrence around 1196) stating that all climate/water-heater temperature values
in related state and command messages are expressed in this unit so consumers
know how to interpret numeric temperature fields; reference the symbol
temperature_unit and ClimateTemperatureUnit in the comment for clarity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 93a5c055-bf97-4f7b-889a-4f3236deca20
📒 Files selected for processing (3)
aioesphomeapi/api.protoaioesphomeapi/api_pb2.pyaioesphomeapi/model.py
| temperature_unit: ClimateTemperatureUnit = converter_field( | ||
| default=ClimateTemperatureUnit.UNSET, converter=ClimateTemperatureUnit.convert | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify converter contract vs annotated field types in aioesphomeapi/model.py
rg -n -C2 "def convert\(cls: type\[_T\], value: int\) -> _T \| None" aioesphomeapi/model.py
rg -n -C2 "temperature_unit: ClimateTemperatureUnit|temperature_unit: WaterHeaterTemperatureUnit" aioesphomeapi/model.py
rg -n -C2 "converter=ClimateTemperatureUnit.convert|converter=WaterHeaterTemperatureUnit.convert" aioesphomeapi/model.pyRepository: esphome/aioesphomeapi
Length of output: 1122
🏁 Script executed:
# Find the converter_field definition and its implementation
rg -n "def converter_field" aioesphomeapi/model.py
# Check if converter_field is imported from elsewhere
rg -n "converter_field" aioesphomeapi/model.py | head -5Repository: esphome/aioesphomeapi
Length of output: 436
🏁 Script executed:
# Search for imports at the top of the file
head -50 aioesphomeapi/model.py | grep -n "converter_field\|from\|import"Repository: esphome/aioesphomeapi
Length of output: 571
🏁 Script executed:
# Look at the converter_field usage to understand its signature and behavior
rg -n -B5 "converter_field" aioesphomeapi/model.py | head -100Repository: esphome/aioesphomeapi
Length of output: 3509
🏁 Script executed:
# Get the full converter_field function implementation
sed -n '90,110p' aioesphomeapi/model.pyRepository: esphome/aioesphomeapi
Length of output: 626
🏁 Script executed:
# Check if converter_field has any special handling for None values
rg -n -A20 "^def converter_field" aioesphomeapi/model.pyRepository: esphome/aioesphomeapi
Length of output: 700
🏁 Script executed:
# Search for where converters from metadata are applied
rg -n "metadata.*converter|converter.*metadata" aioesphomeapi/model.py
# Check APIModelBase methods
sed -n '60,90p' aioesphomeapi/model.pyRepository: esphome/aioesphomeapi
Length of output: 1299
🏁 Script executed:
# Search for usage of converter in metadata across the codebase
rg -n "\[\"converter\"\]|\[.converter.\]" aioesphomeapi/Repository: esphome/aioesphomeapi
Length of output: 128
Fix type annotation for enum fields with nullable converters.
The convert() method on ClimateTemperatureUnit and WaterHeaterTemperatureUnit returns _T | None, but the temperature_unit fields at lines 743 and 1203 are annotated as non-optional. When __post_init__ applies the converter, a None value can be assigned to these fields, violating the type contract.
Update both fields to allow None:
Proposed fix
- temperature_unit: ClimateTemperatureUnit = converter_field(
+ temperature_unit: ClimateTemperatureUnit | None = converter_field(
default=ClimateTemperatureUnit.UNSET, converter=ClimateTemperatureUnit.convert
)- temperature_unit: WaterHeaterTemperatureUnit = converter_field(
+ temperature_unit: WaterHeaterTemperatureUnit | None = converter_field(
default=WaterHeaterTemperatureUnit.UNSET, converter=WaterHeaterTemperatureUnit.convert
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| temperature_unit: ClimateTemperatureUnit = converter_field( | |
| default=ClimateTemperatureUnit.UNSET, converter=ClimateTemperatureUnit.convert | |
| ) | |
| temperature_unit: ClimateTemperatureUnit | None = converter_field( | |
| default=ClimateTemperatureUnit.UNSET, converter=ClimateTemperatureUnit.convert | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@aioesphomeapi/model.py` around lines 743 - 745, The temperature enum fields
are annotated as non-optional but their converters
(ClimateTemperatureUnit.convert and WaterHeaterTemperatureUnit.convert) can
return None; update the field annotations temperature_unit (and the analogous
water heater field) to be Optional[ClimateTemperatureUnit] and
Optional[WaterHeaterTemperatureUnit] respectively, add/import typing.Optional if
missing, and ensure the converter_field calls remain unchanged so the post-init
assignment of None satisfies the type contract.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1586 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 25 25
Lines 4002 4008 +6
=========================================
+ Hits 4002 4008 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jhenkens
force-pushed
the
main
branch
4 times, most recently
from
15b03e7 to
c66f09e
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
setup.py (1)
44-60:⚠️ Potential issue | 🟠 MajorKeep the package version on the existing
44.xrelease line.Changing
VERSIONfrom44.17.0to0.1.0makes this feature release look older than the current published series, so normal dependency upgrades may never select it; it also changesDOWNLOAD_URLto the0.1.0archive path. Please restore the existing version stream and bump to the intended next release version.Proposed fix
-VERSION = "0.1.0" +VERSION = "44.18.0"If the project uses a different release cadence, use the next valid
44.xversion instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup.py` around lines 44 - 60, The VERSION constant was mistakenly set to "0.1.0"; restore it to the project's 44.x release line (e.g., set VERSION back to the previous 44.17.0 or bump to the intended next 44.x release) so dependency resolution and DOWNLOAD_URL remain correct; update the VERSION value used by DOWNLOAD_URL (and verify DOWNLOAD_URL still points to f"{GITHUB_URL}/archive/{VERSION}.zip") to reflect the corrected 44.x version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@setup.py`:
- Around line 44-60: The VERSION constant was mistakenly set to "0.1.0"; restore
it to the project's 44.x release line (e.g., set VERSION back to the previous
44.17.0 or bump to the intended next 44.x release) so dependency resolution and
DOWNLOAD_URL remain correct; update the VERSION value used by DOWNLOAD_URL (and
verify DOWNLOAD_URL still points to f"{GITHUB_URL}/archive/{VERSION}.zip") to
reflect the corrected 44.x version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad8f6789-d3eb-4f05-bd4b-956e8f81eafa
📒 Files selected for processing (4)
aioesphomeapi/api.protoaioesphomeapi/api_pb2.pyaioesphomeapi/model.pysetup.py
🚧 Files skipped from review as they are similar to previous changes (2)
- aioesphomeapi/model.py
- aioesphomeapi/api.proto
|
|
||
|
|
||
| VERSION = "44.17.0" | ||
| VERSION = "0.1.0" |
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
There was a problem hiding this comment.
Pull request overview
This PR adds support for representing the temperature unit (C/F/K) on Climate and Water Heater entities in the aioesphomeapi client models and protocol, enabling native Fahrenheit flows end-to-end.
Changes:
- Add
TemperatureUnitenum to the API protocol (api.proto) and regenerate protobuf bindings (api_pb2.py). - Expose
temperature_unitonClimateInfoandWaterHeaterInfoinaioesphomeapi/model.py. - Modify package version in
setup.py.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| setup.py | Updates package version used for releases/build metadata. |
| aioesphomeapi/model.py | Adds TemperatureUnit model enum and new temperature_unit fields on entity info models. |
| aioesphomeapi/api.proto | Extends the protobuf API with a new enum and new fields on climate/water_heater entity info messages. |
| aioesphomeapi/api_pb2.py | Regenerated protobuf Python output reflecting the updated api.proto. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
VERSION was changed from the project’s current release series to "0.1.0", which will break packaging/version ordering (pip will treat this as a major downgrade) and produce an incorrect download URL. Please restore the correct versioning scheme and bump appropriately for this feature release (e.g., next patch/minor in the existing 44.x line).
| VERSION = "44.1.0" |
| temperature_unit: TemperatureUnit = ( | ||
| converter_field( | ||
| default=TemperatureUnit.CELSIUS, | ||
| converter=TemperatureUnit.convert, | ||
| ) | ||
| or TemperatureUnit.CELSIUS |
There was a problem hiding this comment.
The or TemperatureUnit.CELSIUS fallback here is ineffective because it runs at class definition time and converter_field(...) is always truthy. Also, TemperatureUnit.convert can return None for unknown enum values, but this field is not typed as optional, so future/invalid values could result in temperature_unit=None unexpectedly. Consider either making the type TemperatureUnit | None (consistent with other converted enums) or using a converter that falls back to TemperatureUnit.CELSIUS when conversion fails, and drop the redundant or ....
| temperature_unit: TemperatureUnit = ( | |
| converter_field( | |
| default=TemperatureUnit.CELSIUS, | |
| converter=TemperatureUnit.convert, | |
| ) | |
| or TemperatureUnit.CELSIUS | |
| temperature_unit: TemperatureUnit = converter_field( | |
| default=TemperatureUnit.CELSIUS, | |
| converter=lambda value: TemperatureUnit.convert(value) | |
| or TemperatureUnit.CELSIUS, |
| temperature_unit: TemperatureUnit = ( | ||
| converter_field( | ||
| default=TemperatureUnit.CELSIUS, | ||
| converter=TemperatureUnit.convert, | ||
| ) | ||
| or TemperatureUnit.CELSIUS |
There was a problem hiding this comment.
Same issue as ClimateInfo: converter_field(...) or TemperatureUnit.CELSIUS is redundant, and TemperatureUnit.convert may yield None for unknown values even though this field is not optional. Please align with the project’s usual enum-conversion pattern (optional type or safe fallback converter) and remove the ineffective or expression.
| temperature_unit: TemperatureUnit = ( | |
| converter_field( | |
| default=TemperatureUnit.CELSIUS, | |
| converter=TemperatureUnit.convert, | |
| ) | |
| or TemperatureUnit.CELSIUS | |
| temperature_unit: TemperatureUnit = converter_field( | |
| default=TemperatureUnit.CELSIUS, | |
| converter=lambda value: TemperatureUnit.convert(value) | |
| or TemperatureUnit.CELSIUS, |
| temperature_unit: TemperatureUnit = ( | ||
| converter_field( | ||
| default=TemperatureUnit.CELSIUS, | ||
| converter=TemperatureUnit.convert, | ||
| ) | ||
| or TemperatureUnit.CELSIUS |
There was a problem hiding this comment.
New behavior is added (temperature_unit for climate/water_heater), but there are no targeted tests asserting correct conversion/round-tripping (e.g., from_pb / from_dict with Fahrenheit, and handling of unknown enum values). Since this repo has extensive model conversion tests, please add coverage for these new fields to prevent regressions.
| temperature_unit: TemperatureUnit = ( | |
| converter_field( | |
| default=TemperatureUnit.CELSIUS, | |
| converter=TemperatureUnit.convert, | |
| ) | |
| or TemperatureUnit.CELSIUS | |
| temperature_unit: TemperatureUnit = converter_field( | |
| default=TemperatureUnit.CELSIUS, | |
| converter=TemperatureUnit.convert, |
What does this implement/fix?
Exposes temperature unit for climate and water_heater, with corresponding changes in home-assistant/core, aioesphome, esphome, and esphome-docs, to enable native Fahrenheit measurements on those entities. Usecase is to enable accurate temperature settings on a Fahrenheit hot tub, controlled via RS485-UART. Rather than round tripped from F -> C -> C, with various floating point errors and weirdnesses, this just makes it stay F the entire way and work much more reliably.
Tests still need to be added where appropriate - that's on the todo - but the code has been validated.
When esphome is updated, but core is not, as long as you do not set the temperature unit on a climate/water heater, everything works the same as it currently is. If you do set it, you will convert from F -> "F", as home assistant will think the incoming value from ESPHome is C, without reading the UOM. If ESPHome does not have F specified, it continues to default to C.
When HA is updated, but ESPHome is on an old version, it maintains legacy behavior of treating all climate/water_heaters as using C.
Types of changes
Related issue or feature (if applicable):
Pull request in esphome (if applicable):
home-assistant/core#168261
#1586
esphome/esphome#15815
esphome/esphome-docs#6463
Checklist:
tests/folder).