Add speed to units system

[rianadon]

Proposed change

This is a result of splitting #53846. This PR builds on the speed conversion functions to make speed a part of the units system.

There are a lot of commits here, but I will rebase once the former PR gets merged.

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

Additional information

• This PR fixes or closes issue: fixes #
• This PR is related to issue:
• Link to documentation pull request:

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:

• Documentation added/updated for www.home-assistant.io

To help with the load of incoming pull requests:

• I have reviewed two other open pull requests in this repository.

[probot-home-assistant]

Hey there @Danielhiversen, @thimic, mind taking a look at this pull request as it has been labeled with an integration (met) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

[probot-home-assistant]

Hey there @MatthewFlamm, mind taking a look at this pull request as it has been labeled with an integration (nws) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

[probot-home-assistant]

Hey there @raman325, mind taking a look at this pull request as it has been labeled with an integration (climacell) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

[probot-home-assistant]

Hey there @DylanGore, mind taking a look at this pull request as it has been labeled with an integration (met_eireann) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

[rianadon]

@emontnemery continuing our discussion here (I've tangentially lost comment access on #53846, so hopefully you see I've split these PRs), I chose km/h to match the units I saw in most of Home Assistant, since most speeds were used for wind speed.

I found that for preciptation speeds, there is a class of units prefixed by PRECIPITATION: PRECIPITATION_MILLIMETERS_PER_HOUR, PRECIPITATION_INCHES, and PRECIPITATION_INCHES_PER_HOUR. I don't think these are converted, but since you have been doing a lot of work on the units system you'd know better than I.

In that case, I wonder if we should add precipitation units to the unit system as well.

TL;DR: SPEED seems to only be used in the codebase for wind speed, so I think km/hr is a good choice, but we may want to worry about PRECIPITATION units as well.

[MatthewFlamm]

I'm responding to some of the comments in the linked PR about using SI units in UnitSystem. There are 5 unit types in UnitSystem, 4 of which have non-SI units. 3 have non-SI units if you want to count Celsius as being an SI unit. Is there some benefit to using SI rather than the most commonly used speed type?

[emontnemery]

The bot locked comments on #53846, fixed now.

SPEED seems to only be used in the codebase for wind speed, so I think km/hr is a good choice, but we may want to worry about PRECIPITATION units as well.

If that's the case, I think this change is good.
Adding precipitation speeds to the unit system can be done in another PR.

@MatthewFlamm agreed km/h is more widely used, I just didn't want to block merge of #53846 by discussing what unit is preferred

[rianadon]

I've rebased and marked this as ready for review. Happy to discuss more about unit choices & how we are handling other types of units like precipitation.

[emontnemery]

How do we handle wind speeds and rainfall?

In my own case (Sweden), we use "m/s" for wind speed, but we measure most other speeds in "km/h".
UK uses knots for wind speed.
In both cases, it means converting wind speed to km/h is not wanted.
Sources:
https://en.wikipedia.org/wiki/Wind_speed#Units
https://www.metoffice.gov.uk/weather/guides/observations/how-we-measure-wind

Do we include rainfall in the unit system?

[rianadon]

In reverse order:

Do we include rainfall in the unit system?

After working through the weather PR, I realize the forecast includes precipitation information (ATTR_FORECAST_PRECIPITATION), so this should likely be part of the unit system.

I was trying to figure out how the code currently deals with precipitation and got stuck. I found this one snippet of the darksky I started at for a while before thinking maybe PRECIPITATION_INCHES is better named PRECIPITATION_INCHES_PER_DAY, which would make precipitation a kind of speed?

core/homeassistant/components/darksky/sensor.py

Lines 147 to 157 in 363de37

	"precip_intensity": DarkskySensorEntityDescription(
	key="precip_intensity",
	name="Precip Intensity",
	si_unit=PRECIPITATION_MILLIMETERS_PER_HOUR,
	us_unit=PRECIPITATION_INCHES,
	ca_unit=PRECIPITATION_MILLIMETERS_PER_HOUR,
	uk_unit=PRECIPITATION_MILLIMETERS_PER_HOUR,
	uk2_unit=PRECIPITATION_MILLIMETERS_PER_HOUR,
	icon="mdi:weather-rainy",
	forecast_mode=["currently", "minutely", "hourly", "daily"],
	),

However, in the arwn component precipitation is being used here as a length/thickness/volume:

core/homeassistant/components/arwn/sensor.py

Lines 47 to 53 in ea2e94a

	return ArwnSensor(
	topic,
	"Rain Since Midnight",
	"since_midnight",
	PRECIPITATION_INCHES,
	"mdi:water",
	)

This makes me think we should have two units for both precipitation rate and accumulated precipitation .

In both cases, it means converting wind speed to km/h is not wanted.

Thank you for the correction! It sounds like the default should be m/s. Perhaps naming he unit wind_speed would be better than speed? I can't think of other speeds around the house one would need to measure in km/h.

[rianadon]

Another thing: all these units are currently being set in the frontend rather than the core. Which means we do already have defaults, even if they aren't the best:

See frontend/src/data/weather.ts

export const getWeatherUnit = (
  hass: HomeAssistant,
  measure: string
): string => {
  const lengthUnit = hass.config.unit_system.length || "";
  switch (measure) {
    case "pressure":
      return lengthUnit === "km" ? "hPa" : "inHg";
    case "wind_speed":
      return `${lengthUnit}/h`;
    case "visibility":
    case "length":
      return lengthUnit;
    case "precipitation":
      return lengthUnit === "km" ? "mm" : "in";
    case "humidity":
    case "precipitation_probability":
      return "%";
    default:
      return hass.config.unit_system[measure] || "";
  }
};

Ideally these units could be queried from Home Assistant, but that's a change for another PR.

[emontnemery]

Thank you for the correction! It sounds like the default should be m/s. Perhaps naming he unit wind_speed would be better than speed? I can't think of other speeds around the house one would need to measure in km/h.

I'm not sure what default unit is preferred, but no matter what we go for we should make it possible to override the wind speed unit in the unit system both via yaml and frontend IMHO.

[rianadon]

I changed to m/s, since that seems to be the standard unit used.
I was looking through the dark sky api for reference and liked how they handled unit systems:

[emontnemery]

What is the user impact if we merge this?

You mention the frontend already supports weather units, does it convert them too, so a wind speed in m/s or mi/h will be converted to km/h or mi/h depending on the length unit?

[rianadon]

I don't forsee any user impact if we merge this, since the wind speed units is hardcoded on the frontend and this PR doesn't do any conversions.
Going forward, the next steps would be:

• Add precipitation units (Add accumulated precipitation to unit system #59657) - no user impact here either for the same reasons
• Modify frontend so it uses these units from the user system (will break reported wind speeds in metric system, since some integrations will convert them to km/h but they will be in m/s)
• Add unit conversion to the WeatherEntity class (Add native unit types for weather entities #59533) - no user impact for core integrations, though very small chance custom integrations will break if they have a precipitation_unit method or similar and do not return precipitation as a number
• Add units to integrations and remove any conversion happening in the integration, if any (will convert wind speeds to m/s so they show correctly again, as well as adding unit conversion to any integrations that did not support them before)

[emontnemery]

Thanks, @rianadon 👍