Add sensor platform for Xiaomi Miio fans #54564

bieniu · 2021-08-12T21:06:44Z

Breaking change

The following fan entity attributes temperature, humidity, aqi, purify volume, filter life remaining, filter hours used, co2, illuminance, motor speed and motor2 speed has been migrated to the senor entity.
Users need to update their automations.

The filter_rfid_product_id and filter_rfid_tag fan entity attributes has been removed as they are of little use to users.

Proposed change

This PR migrates the existing fan attributes to the sensor entities.

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: Add information about sensor platform for fan home-assistant.io#18912

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-08-12T21:06:47Z

This pull request needs to be manually signed off by @home-assistant/core before it can get merged.
_{^{(message by ReviewEnforcer)}}

probot-home-assistant · 2021-08-12T21:06:51Z

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

bieniu · 2021-08-12T21:07:29Z

@jbouwh Please have a look.

jbouwh

Look good overall a few suggestions.

jbouwh · 2021-08-13T10:03:57Z

homeassistant/components/xiaomi_miio/sensor.py

    MODELS_HUMIDIFIER_MIIO,
    MODELS_HUMIDIFIER_MIOT,
    MODELS_HUMIDIFIER_MJJSQ,
+    MODELS_PURIFIER_MIIO,
+    MODELS_PURIFIER_MIOT,
 )
 from .device import XiaomiCoordinatedMiioEntity, XiaomiMiioEntity
 from .gateway import XiaomiGatewayDevice


The constant in line 64->78 does no seem to be reference anymore.

UNIT_LUMEN = "lm"

UNIT_LUMEN is used in the line 180.

jbouwh · 2021-08-13T10:17:40Z

homeassistant/components/xiaomi_miio/sensor.py

@@ -238,13 +362,34 @@ async def async_setup_entry(hass, config_entry, async_add_entities):
        elif model in MODELS_HUMIDIFIER_MIIO:
            device = hass.data[DOMAIN][config_entry.entry_id][KEY_DEVICE]


Assignment of device is all the same perhaps we can do the check for the condition in the else statement first.
Just get the value for KEY_DEVICE if key if it is available (perhaps use get).

Background:
AIR_MONITOR_PLATFORMS (MODELS_AIR_MONITOR) (flow_type == CONF_DEVICE) and GATEWAY_PLATFORMS (flow_type == CONF_GATEWAY) are initializing then sensor platform too.

jbouwh · 2021-08-13T10:18:35Z

homeassistant/components/xiaomi_miio/sensor.py

+            sensors = PURIFIER_MIIO_SENSORS
+        elif model in MODELS_PURIFIER_MIOT:
+            device = hass.data[DOMAIN][config_entry.entry_id][KEY_DEVICE]
+            sensors = PURIFIER_MIOT_SENSORS
        else:


In a next PR we should also move there sensors to a coordinated context

jbouwh · 2021-08-13T10:52:53Z

homeassistant/components/xiaomi_miio/sensor.py

            sensors = HUMIDIFIER_MIIO_SENSORS
+        elif model == MODEL_AIRPURIFIER_V2:


Perhaps we could do the model to sensors translation using a constant dict

bieniu · 2021-08-13T10:55:48Z

homeassistant/components/xiaomi_miio/sensor.py

+    ),
+    ATTR_AQI: XiaomiMiioSensorDescription(
+        key=ATTR_AQI,
+        name="Air Quality Index",


I just noticed that in the Xiaomi Home app this value is specified as PM2.5 and not AQI. I wonder if we should not treat it as a PM2.5 sensor.

jbouwh

LGTM

jbouwh

Looks good

homeassistant/components/xiaomi_miio/sensor.py

MartinHjelmare · 2021-08-14T05:43:00Z

homeassistant/components/xiaomi_miio/sensor.py

+        state_class=STATE_CLASS_MEASUREMENT,
+        attributes=(
+            "filter_hours_used",
+            "filter_rfid_product_id",


This sounds like it's static info. We only want to have dynamic info related to the entity state in the state attributes. There are only some exceptions like attribution or if the info is essential for automations.

This filter_rfid_product_id looks like this : 0:0:30:33. It could change if you use a different filter product. The filter_type is extracted/is part of the product id.

homeassistant/components/xiaomi_miio/sensor.py

MartinHjelmare

Looks good!

MartinHjelmare · 2021-08-14T12:24:20Z

We should mention that filter product id and rf id tag were removed in the breaking changes paragraph.

jbouwh · 2021-08-14T13:37:45Z

Please have a look at the error at #54625. The water_level sensor seems to create a an error when the cover is removed.

jbouwh · 2021-08-14T13:38:15Z

2021-08-14 13:35:24 ERROR (MainThread) [homeassistant] Error doing job: Task exception was never retrieved
Traceback (most recent call last):
  File "/workspaces/core/homeassistant/helpers/update_coordinator.py", line 134, in _handle_refresh_interval
    await self._async_refresh(log_failures=True, scheduled=True)
  File "/workspaces/core/homeassistant/helpers/update_coordinator.py", line 265, in _async_refresh
    update_callback()
  File "/workspaces/core/homeassistant/helpers/update_coordinator.py", line 325, in _handle_coordinator_update
    self.async_write_ha_state()
  File "/workspaces/core/homeassistant/helpers/entity.py", line 464, in async_write_ha_state
    self._async_write_ha_state()
  File "/workspaces/core/homeassistant/helpers/entity.py", line 498, in _async_write_ha_state
    state = self._stringify_state()
  File "/workspaces/core/homeassistant/helpers/entity.py", line 470, in _stringify_state
    state = self.state
  File "/workspaces/core/homeassistant/components/xiaomi_miio/sensor.py", line 450, in state
    and self._state > self.entity_description.valid_max_value
TypeError: '>' not supported between instances of 'NoneType' and 'float'

I merged this pull to sync the code base.

jbouwh · 2021-08-14T13:49:00Z

This issue is caused here:

core/homeassistant/components/xiaomi_miio/sensor.py

Lines 437 to 446 in 3d5d740

    
           self._state = self._extract_value_from_attribute( 
        
               self.coordinator.data, self.entity_description.key 
        
           ) 
        
           if ( 
        
               self.entity_description.valid_min_value 
        
               and self._state < self.entity_description.valid_min_value 
        
           ) or ( 
        
               self.entity_description.valid_max_value 
        
               and self._state > self.entity_description.valid_max_value 
        
           ):

When state is None. No further checks should be performed. When the cover is removed the backend returns None.

Suggestion is to insert one line of code...

core/homeassistant/components/xiaomi_miio/sensor.py

Lines 434 to 439 in 3d5d740

    
           @property 
        
           def state(self): 
        
               """Return the state of the device.""" 
        
               self._state = self._extract_value_from_attribute( 
        
                   self.coordinator.data, self.entity_description.key 
        
               )

        if self._state is None:
          return None

core/homeassistant/components/xiaomi_miio/sensor.py

Lines 440 to 446 in 3d5d740

    
           if ( 
        
               self.entity_description.valid_min_value 
        
               and self._state < self.entity_description.valid_min_value 
        
           ) or ( 
        
               self.entity_description.valid_max_value 
        
               and self._state > self.entity_description.valid_max_value 
        
           ):

bieniu · 2021-08-15T06:56:50Z

We should mention that filter product id and rf id tag were removed in the breaking changes paragraph.

Done

bieniu · 2021-08-15T06:57:17Z

This issue is caused here

Great catch. Fixed.

MartinHjelmare · 2021-08-15T07:30:10Z

Why did we have the valid max and min value checks? Don't we need them anymore?

bieniu · 2021-08-15T09:04:49Z

Why did we have the valid max and min value checks? Don't we need them anymore?

With earlier versions of python-miio than 0.5.7 the integration had to check that the value of water_level was not greater than 100% (with water tank detached the value of water_level was 127/1.2), now python-miio does this, please look here.

MartinHjelmare

Good!

MartinHjelmare · 2021-08-15T09:15:14Z

@jbouwh please confirm we can merge here.

jbouwh · 2021-08-15T11:01:18Z

@jbouwh please confirm we can merge here.

I'll check within a few hours

jbouwh · 2021-08-15T11:44:33Z

Tested. All seems ok. The water_level sensor turn to unknown when the cover is removed. This is expected.
Alto tested together with PR #54625. All seems right now. Now exceptions.

MartinHjelmare · 2021-08-15T11:49:21Z

Thanks!

bieniu requested review from rytilahti and syssi as code owners August 12, 2021 21:06

homeassistant added the cla-signed label Aug 12, 2021

probot-home-assistant bot added integration: xiaomi_miio new-feature labels Aug 12, 2021

probot-home-assistant bot assigned rytilahti and syssi Aug 12, 2021

bieniu added the breaking-change label Aug 12, 2021

bieniu mentioned this pull request Aug 12, 2021

Add information about sensor platform for fan home-assistant/home-assistant.io#18912

Merged

8 tasks

jbouwh requested changes Aug 13, 2021

View reviewed changes

jbouwh reviewed Aug 13, 2021

View reviewed changes

bieniu commented Aug 13, 2021

View reviewed changes

jbouwh approved these changes Aug 13, 2021

View reviewed changes

MartinHjelmare reviewed Aug 14, 2021

View reviewed changes

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

MartinHjelmare approved these changes Aug 14, 2021

View reviewed changes

bieniu added 3 commits August 15, 2021 08:45

Add sensor platform for purifiers

6731450

Next step

6e31c38

Remove unused attributes

60d4340

bieniu added 10 commits August 15, 2021 08:45

Clean up sensors

9cfceaf

Simplify device assignment

28dbf81

Remove unnecessary condition

c8fd7ab

Add the model to sensor list map

881ff3b

Rename AQI sensor to PM2.5

c8a6219

Fix AirQualityMonitor sensor

fa23d9e

Use STATE_CLASS_TOTAL_INCREASING for purify volume sensor

b8d0f9b

Add filter use sensor and clean up attributes

898bd43

Remove filter_rfid_tag attribute

2318dee

Remove valid_max_value/valid_min_value conditions

ba7eac7

MartinHjelmare approved these changes Aug 15, 2021

View reviewed changes

Rename state property to native_value

604a234

MartinHjelmare merged commit ebaae8d into home-assistant:dev Aug 15, 2021

bieniu deleted the xiaomi-miio-fan-sensor branch August 16, 2021 07:23

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sensor platform for Xiaomi Miio fans #54564

Add sensor platform for Xiaomi Miio fans #54564

bieniu commented Aug 12, 2021 •

edited by MartinHjelmare

Loading

probot-home-assistant bot commented Aug 12, 2021

probot-home-assistant bot commented Aug 12, 2021

bieniu commented Aug 12, 2021

jbouwh left a comment

jbouwh Aug 13, 2021

bieniu Aug 13, 2021

jbouwh Aug 13, 2021

jbouwh Aug 13, 2021

jbouwh Aug 13, 2021

bieniu Aug 13, 2021

bieniu Aug 13, 2021

jbouwh left a comment

jbouwh left a comment

MartinHjelmare Aug 14, 2021

syssi Aug 14, 2021 •

edited

Loading

MartinHjelmare left a comment

MartinHjelmare commented Aug 14, 2021

jbouwh commented Aug 14, 2021

jbouwh commented Aug 14, 2021 •

edited

Loading

jbouwh commented Aug 14, 2021 •

edited

Loading

bieniu commented Aug 15, 2021

bieniu commented Aug 15, 2021

MartinHjelmare commented Aug 15, 2021

bieniu commented Aug 15, 2021

MartinHjelmare left a comment

MartinHjelmare commented Aug 15, 2021 •

edited

Loading

jbouwh commented Aug 15, 2021

jbouwh commented Aug 15, 2021

MartinHjelmare commented Aug 15, 2021

		@@ -238,13 +362,34 @@ async def async_setup_entry(hass, config_entry, async_add_entities):
		elif model in MODELS_HUMIDIFIER_MIIO:
		device = hass.data[DOMAIN][config_entry.entry_id][KEY_DEVICE]

		sensors = HUMIDIFIER_MIIO_SENSORS
		elif model == MODEL_AIRPURIFIER_V2:

Add sensor platform for Xiaomi Miio fans #54564

Add sensor platform for Xiaomi Miio fans #54564

Conversation

bieniu commented Aug 12, 2021 • edited by MartinHjelmare Loading

Breaking change

Proposed change

Type of change

Additional information

Checklist

probot-home-assistant bot commented Aug 12, 2021

probot-home-assistant bot commented Aug 12, 2021

bieniu commented Aug 12, 2021

jbouwh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbouwh left a comment

Choose a reason for hiding this comment

jbouwh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syssi Aug 14, 2021 • edited Loading

Choose a reason for hiding this comment

MartinHjelmare left a comment

Choose a reason for hiding this comment

MartinHjelmare commented Aug 14, 2021

jbouwh commented Aug 14, 2021

jbouwh commented Aug 14, 2021 • edited Loading

jbouwh commented Aug 14, 2021 • edited Loading

bieniu commented Aug 15, 2021

bieniu commented Aug 15, 2021

MartinHjelmare commented Aug 15, 2021

bieniu commented Aug 15, 2021

MartinHjelmare left a comment

Choose a reason for hiding this comment

MartinHjelmare commented Aug 15, 2021 • edited Loading

jbouwh commented Aug 15, 2021

jbouwh commented Aug 15, 2021

MartinHjelmare commented Aug 15, 2021

bieniu commented Aug 12, 2021 •

edited by MartinHjelmare

Loading

syssi Aug 14, 2021 •

edited

Loading

jbouwh commented Aug 14, 2021 •

edited

Loading

jbouwh commented Aug 14, 2021 •

edited

Loading

MartinHjelmare commented Aug 15, 2021 •

edited

Loading