Add MQTT climate temperature unit#34066
Add MQTT climate temperature unit#34066emontnemery merged 5 commits intohome-assistant:devfrom presslab-us:therm_unit
Conversation
probot-home-assistant
Bot
added
core
has-tests
integration: mqtt
small-pr
|
Hey there @home-assistant/core, @emontnemery, mind taking a look at this pull request as its been labeled with a integration ( |
emontnemery
left a comment
emontnemery
left a comment
There was a problem hiding this comment.
Looks good, just a couple of small comments.
| CONF_TEMP_MAX = "max_temp" | ||
| CONF_TEMP_MIN = "min_temp" | ||
| CONF_TEMP_STEP = "temp_step" | ||
| CONF_TEMP_UNIT = "unit_of_measurement" |
There was a problem hiding this comment.
Import CONF_UNIT_OF_MEASUREMENT from homeassistant.const instead.
| vol.Optional(CONF_TEMP_LOW_STATE_TOPIC): mqtt.valid_subscribe_topic, | ||
| vol.Optional(CONF_TEMP_STATE_TEMPLATE): cv.template, | ||
| vol.Optional(CONF_TEMP_STATE_TOPIC): mqtt.valid_subscribe_topic, | ||
| vol.Optional(CONF_TEMP_UNIT): cv.string, |
There was a problem hiding this comment.
Use cv.temperature_unit
There was a problem hiding this comment.
Thanks for reviewing! I changed it to cv.temperature_unit, but it doesn't work. The temperature_unit validator does not accept the degree symbol required when used as a unit of measurement.
https://github.com/home-assistant/core/blob/dev/homeassistant/helpers/config_validation.py#L468-L475
There was a problem hiding this comment.
Right, this is the purpose of cv.temperature_unit: in the MQTT discovery message or configuration.yaml, Cor F is specified, this is then transformed to °C or °F.
To make it more similar to other integrations which allows configuring temperature unit in the same way, change to use const.CONF_TEMPERATURE_UNIT instead of const.CONF_UNIT_OF_MEASUREMENT.
There was a problem hiding this comment.
As that change will basically rewrite this (small) pull request, I think I will just close this.
There was a problem hiding this comment.
I think the new functionality is good though. I did some adjustment, does it look OK?
There was a problem hiding this comment.
Thanks. Let me test it and get back to you. I find it curious that the temperature_unit climate property will work with either returning the degree symbol or without it. Which is technically correct?
There was a problem hiding this comment.
The degree symbol is added by cv.temperature_unit when the config schema is validated. I think the purpose of cv.temperature_unit is to not have to write the degree symbol in config files which doesn't exist on most keyboards.
There was a problem hiding this comment.
I tested your changes and it works fine. Thanks for your help.
emontnemery
changed the title
3 participants
Proposed change
Add unit_of_measurement to MQTT HVAC. This allows for a device to use a temperature unit other than the system unit.
Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.The integration reached or maintains the following Integration Quality Scale: