Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add template support for remaining attributes of weather entity #47736

Merged
merged 2 commits into from Mar 27, 2021

Conversation

csoltenborn
Copy link
Contributor

@csoltenborn csoltenborn commented Mar 10, 2021

Proposed change

The new weather template is great, but does not support all attributes of the weather entity. This PR adds support for these attributes.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml
weather:
  - platform: template
    name: "My own weather station"
    attribution_template: "{{ 'My attribution' }}" # new
    condition_template: "sunny"
    temperature_template: "{{ states('sensor.temperature') | float}}"
    humidity_template: "{{ states('sensor.humidity')| float }}"
    pressure_template: "{{ states('sensor.pressure')| float }}"
    wind_speed_template: "{{ states('sensor.wind_speed')| float }}"
    wind_bearing_template: "{{ states('sensor.wind_bearing')| float }}" # new
    ozone_template: "{{ states('sensor.ozone')| float }}" # new
    visibility_template: "{{ states('sensor.visibility')| float }}" # new
    forecast_template: "{{ states.weather.my_region.attributes.forecast }}"

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 馃 Silver
  • 馃 Gold
  • 馃弳 Platinum

To help with the load of incoming pull requests:

@homeassistant
Copy link
Contributor

Hi @csoltenborn,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

Dev automation moved this from Needs review to Reviewer approved Mar 10, 2021
@MartinHjelmare MartinHjelmare changed the title Added template support for remaining attributes of weather entity Add template support for remaining attributes of weather entity Mar 11, 2021
@csoltenborn
Copy link
Contributor Author

Thanks for the fast review! Now that I have your attention, two questions which came into my mind:

  • Would it make sense to also expose the temperature_unit (and let it fall back to self.hass.config.units.temperature_unit only in case it is not set)?
  • As far as I can see, the units of the other elements are implicitly fixed (e.g. mBa for air pressure). Should the documentation contain information on the expected units? If yes, what are they? :-)
    • 掳 for wind bearing?
    • m/s for wind speed?
    • ppm for ozone level?
    • % for humidity?
    • km for visibility?

@dgomes
Copy link
Contributor

dgomes commented Mar 11, 2021

@csoltenborn
Copy link
Contributor Author

  • Yes it makes sense to extract temperature from the entity in the template :)

I was talking about temperature_unit - temperature is already exposed ;-)

Shall I than expose that attribute, too?

Not to nitpick, but the documentation you referred to suggests different kinds of units (e.g. The current air pressure in hPa or inHg). I'm not even sure how HA handles this - how does it know whether the number is in hPa or inHg? And if it doesn't know, but relies on fixed units in this context, than these units should probably be documented...

@dgomes
Copy link
Contributor

dgomes commented Mar 11, 2021

  • I meant temperature_unit 馃槃 , but my point is that you can extract the unit, you don't need to define a configuration parameter.
  • Well this is the current model, any changes must go through architectural review. There are of course legacy components that do adhere to this model, but they should be fixed and not become a reason to detour from the architectural guidelines.

@csoltenborn
Copy link
Contributor Author

Sorry that I'm not getting it - I'm rather new to the HA world...

  • I meant temperature_unit 馃槃 , but my point is that you can extract the unit, you don't need to define a configuration parameter.

What do you mean with "extract the unit"? From self.hass.config.units.temperature_unit as it is now? But what if that unit diverges from the one from the sensor - wouldn't I be out of luck then (or have to do the calculation in the temperature_template)?

  • Well this is the current model, any changes must go through architectural review. There are of course legacy components that do adhere to this model, but they should be fixed and not become a reason to detour from the architectural guidelines.

I didn't want to change anything - it's just that when I implemented a weather template, I had to figure out myself what the parameters' units are because the docs don't say so. In particular, my sensor would provide wind speed in m/s, and I only noticed on the GUI that I have to compute km/h in wind_speed_template. Thus, I think that this information should be contained in the docs of wind_speed_template (which currently only says The current wind speed and could be e.g. be changed to The current wind speed in km/h).

@dgomes
Copy link
Contributor

dgomes commented Mar 11, 2021

What do you mean with "extract the unit"? From self.hass.config.units.temperature_unit as it is now? But what if that unit diverges from the one from the sensor - wouldn't I be out of luck then (or have to do the calculation in the temperature_template)?

I was thinking that if you have the temperature entity (from the template) you could check it's attributes to know the unit. But I came to realise this might not be available in all cases. So lets go with template_temperature_unit

  • Well this is the current model, any changes must go through architectural review. There are of course legacy components that do adhere to this model, but they should be fixed and not become a reason to detour from the architectural guidelines.

I didn't want to change anything - it's just that when I implemented a weather template, I had to figure out myself what the parameters' units are because the docs don't say so. In particular, my sensor would provide wind speed in m/s, and I only noticed on the GUI that I have to compute km/h in wind_speed_template. Thus, I think that this information should be contained in the docs of wind_speed_template (which currently only says The current wind speed and could be e.g. be changed to The current wind speed in km/h).

OK, that can be updated in documentation repository through a new PR

@csoltenborn
Copy link
Contributor Author

My bad - I'm facing issues adding the temperature_unit_template that I don't understand, and I'm lacking knowledge concerning basic stuff like debugging tests. Therefore, I suggest that you merge what we have here, and I might come up with another PR tackling the temperature_unit_template at a later time.

@MatthewFlamm
Copy link
Contributor

I believe all sensors with a unit of measurement set to a valid temperature unit will be automatically converted to self.hass.config.units.temperature_unit. See below:

try:
unit_of_measure = attr.get(ATTR_UNIT_OF_MEASUREMENT)
units = self.hass.config.units
if (
unit_of_measure in (TEMP_CELSIUS, TEMP_FAHRENHEIT)
and unit_of_measure != units.temperature_unit
):
prec = len(state) - state.index(".") - 1 if "." in state else 0
temp = units.temperature(float(state), unit_of_measure)
state = str(round(temp) if prec == 0 else round(temp, prec))
attr[ATTR_UNIT_OF_MEASUREMENT] = units.temperature_unit

This is also true for weather entitie's temperature attribute. In my understanding, the only case that can fail is a sensor that does not have a unit of measurement. But this can be fixed by providing a unit of measurement for the sensor in question, or by making a template sensor with a unit of measurement, or by making the conversion in the template for temperature itself.

There is also the complexity that I believe that the temperature_unit will apply to both the current temperature and the temperatures in the forecast. These could come from different entities here.

I would vote to keep it simple by not including the template for the temperature unit.

@dgomes
Copy link
Contributor

dgomes commented Mar 11, 2021

Let me start by saying that I agree with the current PR as is (no need to add anymore features)

I also agree with @MatthewFlamm, but that is only true if the template has an entity. But you can write in any number in the template...

@MatthewFlamm
Copy link
Contributor

Good point @dgomes. I forgot about that possibility. In this case, the user should also be able to write in the number in the correct units. This is probably better in the documentation (in a different PR, which I also agree).

@csoltenborn
Copy link
Contributor Author

csoltenborn commented Mar 14, 2021

I believe all sensors with a unit of measurement set to a valid temperature unit will be automatically converted to self.hass.config.units.temperature_unit. See below:

Thanks - I've learned something here. However, there seems to be a problem with some of the other properties. For instance, I have a sensor as follows:

- platform: command_line
  command: python3 -c "import re; import requests; print(re.search(r'Wind speed:<br><b> .*<br> ([\d\.]*) km\/h', requests.get('http://foo/bar.html').text).group(1))"
  name: Foo Weather Station - Wind speed
  unit_of_measurement: 'km/h'
  scan_interval: 900

I use that sensor in my weather template:

weather:
  - platform: template
    name: Foo
    wind_speed_template: "{{ (states('sensor.foo_weather_station_wind_speed') | float) | round(1) }}"

The problem is that the weather's wind speed will always have unit km/h, but its value is displayed as mph in Imperial mode. However, there's no way I'm aware of (which doesn't mean much, though :-) ) to check within wind_speed_template whether km/h or mph shall be returned (i.e., to figure out whether Metric or Imperial unit system is configured). If this is indeed not possible, I think that this is in contrast to WeatherEntity (superclass of WeatherTemplate), the doc of which require that Properties have to follow the units defined in the unit_system.

I would then propose the following changes (in a different PR):

  • Clearly state in the docs which units are expected for each template (e.g. km/h for wind speed)
  • Let the WeatherTemplate class deal with unit conversion. I think this mainly is about wind speed and visibility (e.g. in km), but I'm not sure about ozone level and air pressure (for the latter, the weather UI does not show a unit).

This would be in the line of

    @property
    def wind_speed(self):
        """Return the wind speed."""
        return self.hass.config.units.length(self._wind_speed, LENGTH_KILOMETERS)

    @property
    def visibility(self):
        """Return the visibility."""
        return self.hass.config.units.length(self._visibility, LENGTH_KILOMETERS)

I think that this change will make it possible to write weather templates which work both in Metric and Imperial mode. Shall I open that PR?

@csoltenborn
Copy link
Contributor Author

Ok, I will do so as soon as this PR has been merged!

@csoltenborn
Copy link
Contributor Author

One final note: Let's try to get this and the upcoming PR into the next version! The reason is that the "fix units" change is a breaking one - it will break weather templates of people using Imperial mode (the weather templates of which would not work correctly in Metric mode). Since the weather template has only been released lately, the sooner that fix is in, the less people will be affected, I guess...

@jdwhite
Copy link

jdwhite commented Mar 26, 2021

These new attributes are a great addition to this template. My weather station (WeatherRack2) also reports the following data, so I'd also like to suggest additional attributes for:

  • UV Index
  • Wind Speed (Gusting)
  • Lux

Thank you.

@csoltenborn
Copy link
Contributor Author

I'm rather new to Home Assistant, but afaik, there is no HA support for these attributes yet, so even if there would be templates for them, there's nothing to map the computed values to... So my guess is that you should open a feature request against HA Core.

@MartinHjelmare
Copy link
Member

Is this ready to be merged?

@dgomes dgomes merged commit 955804b into home-assistant:dev Mar 27, 2021
Dev automation moved this from Reviewer approved to Done Mar 27, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Mar 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

9 participants