Move temperature conversions to sensor base class (1/8) by emontnemery · Pull Request #48261 · home-assistant/core

emontnemery · 2021-03-23T15:12:18Z

Breaking change

Although not a breaking change from a user's perspective, this is a significant change from a developer's perspective.
More details here: home-assistant/developers.home-assistant#1024

Proposed change

Move temperature conversions to sensor base class SensorEntity (1/8)

Temperature conversion will only be done for sensors with device class set to temperature.
Temperature conversion will still be done for other sensor entities during a transition period, which will end with Home Assistant Core 2022.3.0.
Temperature conversion for non sensor entities is unaffected by this PR.

This PR migrates sensors in integrations a-c to be compatible with unit conversion in SensorEntity.

Full list of PRs migrating sensor integrations:
#48261 (integrations a-c)
#54468 (integrations d-e)
#54469 (integrations f-h)
#54472 (integrations i-m)
#54475 (integrations n-q)
#54476 (integrations r-s)
#54482 (integrations s-u)
#54483 (integrations v-z)
#54623 (additional integrations which were added or updated)

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 is related to issue: Improve unit conversion for sensor entities architecture#534

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

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:

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

probot-home-assistant · 2021-03-23T15:16:05Z

Hey there @shred86, mind taking a look at this pull request as its been labeled with an integration (abode) you are listed as a codeowner for? Thanks!
_{^{(message by CodeOwnersMention)}}

balloob · 2021-03-23T22:39:46Z

homeassistant/helpers/entity.py

+                if not self._temperature_conversion_reported:
+                    self._temperature_conversion_reported = True
+                    _LOGGER.warning(
+                        "Entity %s reports a temperature, but it doesn't extend "


Make sure you document this on the dev blog.

balloob · 2021-03-23T22:43:11Z

homeassistant/helpers/entity.py

            await self.async_turn_on(**kwargs)
+
+
+class MeasurableUnitEntity(Entity):


Is the plan to create a measurable entity that can handle any unit conversion or should we consider mixins per unit conversion (ie TemperatureEntity)

I think the former, it's fairly common pattern for integrations to have a shared class for all sensors, then just set the attributes, including unit_of_measurement, based on some input from the lib.
If we want to specialize like this, those integrations would have to be refactored.

I agree. Besides temperature, I think we should only convert units based on device class. That will allow us to have a look up table for the conversion to apply and a clean implementation in a single class.

we should only convert units based on device class

I actually implemented it like that first, but there are two issues:

We will need to be backwards compatible for some time to handle integrations which don't set device_ class on their sensors.

How to handle integrations such as input_number which allow configuring a unit_of_measurement but not a device_class?

The first issue is not a problem, but I'm not sure about the second one.

If the unit of measurement is configured by the user we probably shouldn't convert it.

OK, that makes sense for input_number.
Not doing the conversion could be considered a breaking change since we currently do convert all temperatures, but maybe it would be for the better?

unit_of_measurement is also used by base components geo_location (for distance) and air_quality (concentrations, I think).

I really don't think device class and unit of measurement should be coupled. I think in the long run it would make more sense to device a measurement class/type.

We are more and more mixing up the object and the measurement it provides. For example, temperature vs garage door.

In the long run, this is going to bite.

device class and unit of measurement need to be coupled for blueprints and to export it to Homekit/Google/Alexa. It's not possible to make code that can work with everything. If it's a device class, we can categorize the entity and know how to deal with it.

balloob · 2021-03-23T22:43:38Z

homeassistant/helpers/entity.py

+    """An abstract class for entities which state is a measurable unit."""
+
+    @property
+    def native_value(self) -> str | None:


Native or raw?

raw is not so good, I would expect that to be the raw binary value read out from the sensor, before conversion to a standard unit.

balloob · 2021-03-23T22:44:10Z

homeassistant/components/sensor/__init__.py



-class SensorEntity(Entity):
+class SensorEntity(MeasurableUnitEntity):


Should all sensors get this logic or should be applied to the right ones? I guess that is a migration we can't do anymore at this point.

Right, because it's a migration it's not feasible to do anything else.
We could do something like this though:

class SensorEntity(Entity): """Base class for sensor entities""" class MeasurableSensorEntity(MeasurableUnitEntity, SensorEntity): """Base class for sensor entities which represent a measurable unit.""" """

The migration would then be to MeasurableSensorEntity, but integrations can move to SensorEntity for sensors which don't represent a measurable unit?

homeassistant/components/sensor/__init__.py

MartinHjelmare · 2021-03-26T17:13:10Z

Looks good! Before we merge this I suggest we search for sensor platforms that use a temperature unit and try to have them updated to use device class temperature if it's missing. We do that separately.

emontnemery · 2021-03-27T09:16:57Z

search for sensor platforms that use a temperature unit and try to have them updated to use device class temperature if it's missing

After a quick check it's clear that many, possibly even most, integrations fail to set device_class.
Now that we have a SensorEntity, should we allow this behavior by setting device_class according to the unit_of_measurement if it's not set by the integration's sensor platform?

MartinHjelmare · 2021-03-27T16:05:47Z

Maybe, but it would be even better to add the device class in the platforms. Not having it there will be less explicit and explicit is good.

emontnemery · 2021-03-29T15:34:40Z

OK, I'm marking this as draft for now, we need to fix integrations not setting device_class in their sensor platforms first.

github-actions · 2021-04-28T16:08:47Z

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

github-actions · 2021-05-28T19:29:20Z

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

github-actions · 2021-06-27T22:04:50Z

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

emontnemery · 2021-07-05T06:32:07Z

This should still be done

homeassistant added the cla-signed label Mar 23, 2021

probot-home-assistant bot added core integration: abode integration: sensor new-feature labels Mar 23, 2021

balloob reviewed Mar 23, 2021

View reviewed changes

MartinHjelmare reviewed Mar 26, 2021

View reviewed changes

homeassistant/components/sensor/__init__.py Outdated Show resolved Hide resolved

emontnemery changed the title ~~Move temperature conversions to entity base class (1/8)~~ Move temperature conversions to sensor base class (1/8) Mar 26, 2021

emontnemery marked this pull request as draft March 29, 2021 15:33

github-actions bot added the stale label Apr 28, 2021

MartinHjelmare removed the stale label Apr 28, 2021

github-actions bot added the stale label May 28, 2021

MartinHjelmare removed the stale label May 28, 2021

github-actions bot added the stale label Jun 27, 2021

github-actions bot closed this Jul 4, 2021

emontnemery reopened this Jul 5, 2021

github-actions bot removed the stale label Jul 5, 2021

emontnemery mentioned this pull request Jul 6, 2021

Make sure weather templates work with both Metric and Imperial unit systems #48641

Closed

21 tasks

emontnemery added 22 commits August 11, 2021 08:32

Fix additional rebase mistakes

3266410

Only report temperature conversion once

f7373d0

Fix additional rebase mistakes

365f348

Format homeassistant/components/bbox/sensor.py

1b5921c

Fix check for overidden _attr_state

9348511

Remove test workarounds from implementation

7000b3f

Remove useless None-check

22df062

Tweaks

3777037

Migrate new sensors a-c

7e578b0

Update climacell

7b40957

Push deprecation of temperature conversion forward

f87b3c1

Override __repr__ in SensorEntity

6216b66

Include native_value in SensorEntity attributes

1fd7c22

Pylint

a48a18e

Black

b82d4a4

Black

6802df5

Fix rebase mistakes

61eb1c7

black

7419646

Fix rebase mistakes

73de387

Revert changes in august/sensor.py

f7d36a5

Revert handling of unit converted restored state

0890af5

Apply code review suggestion

904adf4

emontnemery force-pushed the sensor_temperature_conversion_1 branch from 2ce8880 to 904adf4 Compare August 11, 2021 06:48

Fix arlo test

9946acb

MartinHjelmare approved these changes Aug 11, 2021

View reviewed changes

emontnemery merged commit 4e07ab1 into home-assistant:dev Aug 11, 2021

michaelarnauts mentioned this pull request Aug 11, 2021

Add state_class and use SensorEntityDescription for comfoconnect #54066

Merged

21 tasks

ludeeus mentioned this pull request Aug 12, 2021

Add sensor platform for Tractive integration #54143

Merged

21 tasks

github-actions bot locked and limited conversation to collaborators Aug 12, 2021

emontnemery added the breaking-change label Aug 24, 2021

		await self.async_turn_on(**kwargs)


		class MeasurableUnitEntity(Entity):



		class SensorEntity(Entity):
		class SensorEntity(MeasurableUnitEntity):

Uh oh!

Conversation

emontnemery commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking change

Proposed change

Type of change

Additional information

Checklist

Uh oh!

probot-home-assistant bot commented Mar 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emontnemery Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emontnemery Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MartinHjelmare commented Mar 26, 2021

Uh oh!

emontnemery commented Mar 27, 2021

Uh oh!

MartinHjelmare commented Mar 27, 2021

Uh oh!

emontnemery commented Mar 29, 2021

Uh oh!

github-actions bot commented Apr 28, 2021

Uh oh!

github-actions bot commented May 28, 2021

Uh oh!

github-actions bot commented Jun 27, 2021

Uh oh!

emontnemery commented Jul 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

emontnemery commented Mar 23, 2021 •

edited

Loading

emontnemery Mar 24, 2021 •

edited

Loading

emontnemery Mar 24, 2021 •

edited

Loading