Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 55 additions & 7 deletions homeassistant/components/xiaomi_miio/fan.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from miio import ( # pylint: disable=import-error
AirFresh,
AirHumidifier,
AirHumidifierJsq,
AirPurifier,
AirPurifierMiot,
Device,
Expand All @@ -20,6 +21,10 @@
LedBrightness as AirhumidifierLedBrightness,
OperationMode as AirhumidifierOperationMode,
)
from miio.airhumidifier_jsq import (
LedBrightness as AirhumidifierJsqLedBrightness,
OperationMode as AirhumidifierJsqOperationMode,
)
from miio.airpurifier import ( # pylint: disable=import-error, import-error
LedBrightness as AirpurifierLedBrightness,
OperationMode as AirpurifierOperationMode,
Expand Down Expand Up @@ -89,6 +94,7 @@
MODEL_AIRHUMIDIFIER_V1 = "zhimi.humidifier.v1"
MODEL_AIRHUMIDIFIER_CA1 = "zhimi.humidifier.ca1"
MODEL_AIRHUMIDIFIER_CB1 = "zhimi.humidifier.cb1"
MODEL_AIRHUMIDIFIER_JSQ001 = "shuii.humidifier.jsq001"

MODEL_AIRFRESH_VA2 = "zhimi.airfresh.va2"

Expand Down Expand Up @@ -117,6 +123,7 @@
MODEL_AIRHUMIDIFIER_V1,
MODEL_AIRHUMIDIFIER_CA1,
MODEL_AIRHUMIDIFIER_CB1,
MODEL_AIRHUMIDIFIER_JSQ001,
MODEL_AIRFRESH_VA2,
]
),
Expand Down Expand Up @@ -169,6 +176,10 @@
ATTR_DEPTH = "depth"
ATTR_DRY = "dry"

# Air Humidifier JSQ001
ATTR_NO_WATER = "no_water"
ATTR_LID_OPENED = "lid_opened"

# Air Fresh
ATTR_CO2 = "co2"

Expand Down Expand Up @@ -296,25 +307,36 @@
ATTR_MODE: "mode",
ATTR_BUZZER: "buzzer",
ATTR_CHILD_LOCK: "child_lock",
ATTR_TARGET_HUMIDITY: "target_humidity",
ATTR_LED_BRIGHTNESS: "led_brightness",
ATTR_USE_TIME: "use_time",
ATTR_HARDWARE_VERSION: "hardware_version",
}

AVAILABLE_ATTRIBUTES_AIRHUMIDIFIER = {
**AVAILABLE_ATTRIBUTES_AIRHUMIDIFIER_COMMON,
ATTR_TARGET_HUMIDITY: "target_humidity",
ATTR_HARDWARE_VERSION: "hardware_version",
ATTR_USE_TIME: "use_time",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a relative time? Relative times are not allowed in the state machine. We only allow absolute utc timestamps.

Copy link
Copy Markdown
Author

@iromeo iromeo Apr 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is a refactoring of existing attributes, I just moved some unsupported attributes from common to more specific attributes sets. use_time attribute was added in 25.03.2018 by @syssi (see e36f27d). So I assume that it is in UTC.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this platform isn't following our design guidelines so we can not trust that the existing code is ok.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You recently accepted PR #31729 with the same code.

I could revert this change to the state before my commit if you want. But I don't think that copy-paste is better than trivial refactoring of the code working for more than 2 years.

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare Apr 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We won't accept this PR until the integration and platform have been refactored. That should be done in a separate PR first.

This integration and platform needs to be refactored first to follow our design guidelines regarding fan modes, state attributes and entity access, before any more additions are made.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rytilahti, @syssi As code owners of the integration do you understand how it should be refactored to address this request?

Copy link
Copy Markdown
Member

@rytilahti rytilahti Apr 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@syssi is more knowledgeable on this specific case, but some things I feel should be refactored:

  • Moving supported features, available attributes towards the backend library.

    • This involves, e.g., moving the list of available fan speeds to the backend, like was done recently for the vacuum platform.
    • Homeassistant has only 3 different fan speeds (+ off), so rest of those need to be exposed over separate services (if wanted).
  • Moving supported models also towards the backend. This will simplify future developments when the backend lib is the only source of truth.

  • I think reading somewhere that sensors are preferred over custom state attributes, but cannot find the reference from the docs at the moment. So, these attributes are the ones that should be exposed through the main device: https://developers.home-assistant.io/docs/entity_fan/#properties

    • This would mean creating new entities for those custom attributes, that are linked to the main device.
    • We can adapt the backend to report what type of attribute something is, that can then be used inside homeassistant to create sensors, binary_sensors, switches, etc. depending on the type.
    • I'm not sure what Martin means with entity access, maybe that's related?
  • Firmware information etc. should only be exposed to the device registry (https://developers.home-assistant.io/docs/device_registry_index/)

  • Lot of common code could be shared with all the platforms, e.g., the initialization is almost always the same, but is still c&p in different platforms.

Copy link
Copy Markdown
Member

@rytilahti rytilahti Apr 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, found it (thanks @MartinHjelmare on that other PR: https://developers.home-assistant.io/docs/entity_sensor/#available-device-classes .

So in this case, to my understanding, we should have the following new entities (depending on the supported features of the device):

  • sensors:

    • temperature
    • humidity
  • binary_sensors:

    • child lock
    • led
    • no water
    • lid open
    • buzzer
  • unknown:

    • mode
    • led brightness

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more point, this architecture issue is relevant to this, too: home-assistant/architecture#310

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rytilahti I think that's a great plan and summary. It's not a small task though, so it will take some time and need to be taken step wise. 👍

With entity access I meant accessing the entity directly from outside the entity instead of via core or dispatcher. Direct entity access from outside is fragile since it only works after the entity has been added to home assistant. Now we also have a service helper that platforms can use to easily register services that need to call into an entity method.

await getattr(device, method["method"])(**params)
update_tasks.append(device.async_update_ha_state(True))

Here's the docs for the service helper:
https://developers.home-assistant.io/docs/dev_101_services#entity-services

And here's a real example how to use the service helper:

platform = entity_platform.current_platform.get()
platform.async_register_entity_service(
SERVICE_EFFECT,
{
vol.Optional(ATTR_EFFECT): vol.Any(cv.positive_int, cv.string),
vol.Optional(ATTR_INTENSITY): vol.All(
vol.Coerce(int), vol.Range(min=0, max=255)
),
vol.Optional(ATTR_REVERSE): cv.boolean,
vol.Optional(ATTR_SPEED): vol.All(
vol.Coerce(int), vol.Range(min=0, max=255)
),
},
"async_effect",
)

ATTR_TRANS_LEVEL: "trans_level",
ATTR_BUTTON_PRESSED: "button_pressed",
}

AVAILABLE_ATTRIBUTES_AIRHUMIDIFIER_CA_AND_CB = {
**AVAILABLE_ATTRIBUTES_AIRHUMIDIFIER_COMMON,
ATTR_TARGET_HUMIDITY: "target_humidity",
ATTR_HARDWARE_VERSION: "hardware_version",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static info is not meant for state attributes unless it's essential for automations.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 2 attributes were moved from AVAILABLE_ATTRIBUTES_AIRHUMIDIFIER_COMMON to more specific implementations because they are not common for all humidifiers and not supported by shuii.humidifier.jsq001.

ATTR_USE_TIME: "use_time",
ATTR_MOTOR_SPEED: "motor_speed",
ATTR_DEPTH: "depth",
ATTR_DRY: "dry",
}

AVAILABLE_ATTRIBUTES_AIRHUMIDIFIER_JSQ = {
**AVAILABLE_ATTRIBUTES_AIRHUMIDIFIER_COMMON,
ATTR_LED: "led",
ATTR_NO_WATER: "no_water",
ATTR_LID_OPENED: "lid_opened",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lid_opened seems like it should be a binary sensor. We want to minimize state attributes and instead break out to as many individual sensors as possible.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it could be a binary sensor. Could you guide me with some example of how I could replace with sate with a binary sensor?

}


AVAILABLE_ATTRIBUTES_AIRFRESH = {
ATTR_TEMPERATURE: "temperature",
ATTR_AIR_QUALITY_INDEX: "aqi",
Expand Down Expand Up @@ -421,6 +443,13 @@

FEATURE_FLAGS_AIRHUMIDIFIER_CA_AND_CB = FEATURE_FLAGS_AIRHUMIDIFIER | FEATURE_SET_DRY

FEATURE_FLAGS_AIRHUMIDIFIER_JSQ = (
FEATURE_SET_BUZZER
| FEATURE_SET_CHILD_LOCK
| FEATURE_SET_LED
| FEATURE_SET_LED_BRIGHTNESS
)

FEATURE_FLAGS_AIRFRESH = (
FEATURE_SET_BUZZER
| FEATURE_SET_CHILD_LOCK
Expand Down Expand Up @@ -535,6 +564,9 @@ async def async_setup_platform(hass, config, async_add_entities, discovery_info=
elif model.startswith("zhimi.humidifier."):
air_humidifier = AirHumidifier(host, token, model=model)
device = XiaomiAirHumidifier(name, air_humidifier, model, unique_id)
elif model.startswith("shuii.humidifier.jsq"):
air_humidifier = AirHumidifierJsq(host, token, model=model)
device = XiaomiAirHumidifier(name, air_humidifier, model, unique_id)
elif model.startswith("zhimi.airfresh."):
air_fresh = AirFresh(host, token)
device = XiaomiAirFresh(name, air_fresh, model, unique_id)
Expand Down Expand Up @@ -990,14 +1022,27 @@ def __init__(self, name, device, model, unique_id):
if self._model in [MODEL_AIRHUMIDIFIER_CA1, MODEL_AIRHUMIDIFIER_CB1]:
self._device_features = FEATURE_FLAGS_AIRHUMIDIFIER_CA_AND_CB
self._available_attributes = AVAILABLE_ATTRIBUTES_AIRHUMIDIFIER_CA_AND_CB
self._led_brightness_enum = AirhumidifierLedBrightness
self._led_brightness_default = 2
self._operation_mode_enum = AirhumidifierOperationMode
self._speed_list = [
mode.name
for mode in AirhumidifierOperationMode
if mode is not AirhumidifierOperationMode.Strong
]
elif self._model in [MODEL_AIRHUMIDIFIER_JSQ001]:
self._device_features = FEATURE_FLAGS_AIRHUMIDIFIER_JSQ
self._available_attributes = AVAILABLE_ATTRIBUTES_AIRHUMIDIFIER_JSQ
self._led_brightness_enum = AirhumidifierJsqLedBrightness
self._led_brightness_default = 0
self._operation_mode_enum = AirhumidifierJsqOperationMode
self._speed_list = [mode.name for mode in AirhumidifierJsqOperationMode]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only allow the modes in the fan base entity integration:

SPEED_OFF = "off"
SPEED_LOW = "low"
SPEED_MEDIUM = "medium"
SPEED_HIGH = "high"

Copy link
Copy Markdown
Author

@iromeo iromeo Apr 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The device does not fall into such a specification

Same issue as in #31729 (review)

else:
self._device_features = FEATURE_FLAGS_AIRHUMIDIFIER
self._available_attributes = AVAILABLE_ATTRIBUTES_AIRHUMIDIFIER
self._led_brightness_enum = AirhumidifierLedBrightness
self._led_brightness_default = 2
self._operation_mode_enum = AirhumidifierOperationMode
self._speed_list = [
mode.name
for mode in AirhumidifierOperationMode
Expand Down Expand Up @@ -1041,7 +1086,7 @@ def speed_list(self) -> list:
def speed(self):
"""Return the current speed."""
if self._state:
return AirhumidifierOperationMode(self._state_attrs[ATTR_MODE]).name
return self._operation_mode_enum(self._state_attrs[ATTR_MODE]).name

return None

Expand All @@ -1055,18 +1100,21 @@ async def async_set_speed(self, speed: str) -> None:
await self._try_command(
"Setting operation mode of the miio device failed.",
self._device.set_mode,
AirhumidifierOperationMode[speed.title()],
self._operation_mode_enum[speed.title()],
)

async def async_set_led_brightness(self, brightness: int = 2):
async def async_set_led_brightness(self, brightness: int = None):
"""Set the led brightness."""
if self._device_features & FEATURE_SET_LED_BRIGHTNESS == 0:
return

if brightness is None:
brightness = self._led_brightness_default

await self._try_command(
"Setting the led brightness of the miio device failed.",
self._device.set_led_brightness,
AirhumidifierLedBrightness(brightness),
self._led_brightness_enum(brightness),
)

async def async_set_target_humidity(self, humidity: int = 40):
Expand Down