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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor switch for vesync #134409

Merged
merged 33 commits into from
Feb 5, 2025
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
0d67c7b
Refactor switch sensor so we can use it for more.
cdnninja Dec 30, 2024
033cac8
Merge branch 'dev' into sensorRefactor
cdnninja Jan 1, 2025
3a663de
Adjust order so all platforms match between PRs
cdnninja Jan 2, 2025
6604d26
Correct type
cdnninja Jan 2, 2025
f4352fd
Attempt to fix mypy error
cdnninja Jan 2, 2025
f143594
Correct mypy erros.
cdnninja Jan 2, 2025
1337372
Correct Errors
cdnninja Jan 2, 2025
7b0510d
Add callbacks
cdnninja Jan 2, 2025
8db10fb
Merge branch 'dev' into sensorRefactor
cdnninja Jan 3, 2025
0ce2086
Add coordinator as per other PR
cdnninja Jan 3, 2025
0fc9e81
Merge branch 'dev' into sensorRefactor
cdnninja Jan 5, 2025
4a2fa3f
Merge issue
cdnninja Jan 5, 2025
0288768
ruff
cdnninja Jan 6, 2025
c12b55d
Fedback
cdnninja Jan 6, 2025
9043ba7
Merge branch 'dev' into sensorRefactor
cdnninja Jan 15, 2025
ff5f13c
Ruff correction
cdnninja Jan 15, 2025
8f13594
Add exists FN
cdnninja Jan 15, 2025
5b08b11
Corrections to pass tests
cdnninja Jan 15, 2025
e32f14f
Remove dimmable switch from scope to pass tests.
cdnninja Jan 15, 2025
139d4e1
Remove unused code to increase test coverage
cdnninja Jan 15, 2025
10e9925
switch to async updates and refresh
cdnninja Jan 18, 2025
8b38c47
Revert last change
cdnninja Jan 19, 2025
85fdfdd
Update homeassistant/components/vesync/common.py
cdnninja Jan 19, 2025
70e2cdf
Update homeassistant/components/vesync/switch.py
cdnninja Jan 19, 2025
c6d2088
Feedback Items
cdnninja Jan 19, 2025
27c255e
Improved comments.
cdnninja Jan 19, 2025
1013880
feedback elif
cdnninja Jan 25, 2025
51d1ead
Feedback
cdnninja Jan 25, 2025
d963ec7
Add unique ID
cdnninja Jan 29, 2025
b39778e
Merge branch 'dev' into sensorRefactor
cdnninja Feb 3, 2025
bde6507
If for on / off
cdnninja Feb 3, 2025
765b86b
remove mistake
cdnninja Feb 4, 2025
918ff0e
Ruff
cdnninja Feb 4, 2025
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
14 changes: 14 additions & 0 deletions homeassistant/components/vesync/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

from pyvesync import VeSync
from pyvesync.vesyncbasedevice import VeSyncBaseDevice
from pyvesync.vesyncoutlet import VeSyncOutlet
from pyvesync.vesyncswitch import VeSyncWallSwitch

from homeassistant.core import HomeAssistant

Expand Down Expand Up @@ -32,3 +34,15 @@ def is_humidifier(device: VeSyncBaseDevice) -> bool:
"""Check if the device represents a humidifier."""

return isinstance(device, VeSyncHumidifierDevice)


def is_outlet(device: VeSyncBaseDevice) -> bool:
"""Check if the device represents an outlet."""

return isinstance(device, VeSyncOutlet)


def is_wall_switch(device: VeSyncBaseDevice) -> bool:
"""Check if the device represents a wall switch, note this doessn't include dimming switches."""

return isinstance(device, VeSyncWallSwitch)
10 changes: 9 additions & 1 deletion homeassistant/components/vesync/const.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
"""Constants for VeSync Component."""

from pyvesync.vesyncfan import VeSyncHumid200300S, VeSyncSuperior6000S
from pyvesync.vesyncfan import (
VeSyncAir131,
VeSyncAirBypass,
VeSyncHumid200300S,
VeSyncSuperior6000S,
)

DOMAIN = "vesync"
VS_DISCOVERY = "vesync_discovery_{}"
Expand Down Expand Up @@ -32,6 +37,9 @@
VeSyncHumidifierDevice = VeSyncHumid200300S | VeSyncSuperior6000S
"""Humidifier device types"""

VeSyncFanDevice = VeSyncAirBypass | VeSyncAir131
"""Humidifier device types"""
cdnninja marked this conversation as resolved.
Show resolved Hide resolved

DEV_TYPE_TO_HA = {
"wifi-switch-1.3": "outlet",
"ESW03-USA": "outlet",
Expand Down
112 changes: 67 additions & 45 deletions homeassistant/components/vesync/switch.py
Original file line number Diff line number Diff line change
@@ -1,29 +1,59 @@
"""Support for VeSync switches."""

from collections.abc import Callable
from dataclasses import dataclass
import logging
from typing import Any
from typing import Any, Final

from pyvesync.vesyncbasedevice import VeSyncBaseDevice

from homeassistant.components.switch import SwitchEntity
from homeassistant.components.switch import (
SwitchDeviceClass,
SwitchEntity,
SwitchEntityDescription,
)
from homeassistant.config_entries import ConfigEntry
from homeassistant.core import HomeAssistant, callback
from homeassistant.helpers.dispatcher import async_dispatcher_connect
from homeassistant.helpers.entity_platform import AddEntitiesCallback

from .const import DEV_TYPE_TO_HA, DOMAIN, VS_COORDINATOR, VS_DEVICES, VS_DISCOVERY
from .common import is_outlet, is_wall_switch
from .const import DOMAIN, VS_COORDINATOR, VS_DEVICES, VS_DISCOVERY
from .coordinator import VeSyncDataCoordinator
from .entity import VeSyncBaseEntity

_LOGGER = logging.getLogger(__name__)


@dataclass(frozen=True, kw_only=True)
class VeSyncSwitchEntityDescription(SwitchEntityDescription):
"""A class that describes custom switch entities."""

is_on: Callable[[VeSyncBaseDevice], bool]
exists_fn: Callable[[VeSyncBaseDevice], bool]
on_fn: Callable[[VeSyncBaseDevice], bool]
off_fn: Callable[[VeSyncBaseDevice], bool]


SENSOR_DESCRIPTIONS: Final[tuple[VeSyncSwitchEntityDescription, ...]] = (
VeSyncSwitchEntityDescription(
key="device_status",
is_on=lambda device: device.device_status == "on",
# Other types of wall switches support dimming. Those use light.py platform.
exists_fn=lambda device: is_wall_switch(device) or is_outlet(device),
Comment on lines +42 to +43
Copy link
Member

Choose a reason for hiding this comment

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

Okay, but how come only these 2 are switches? As in, is_wall_switch doesnt sound to incorporate dimming

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought I had handled this different maybe lost in a merge - so here is the challenge. The old lookup table takes two model of switches and calls them a dimmer switch. Goal is to not need that lookup table for models in the future.

https://github.com/cdnninja/core/blob/09ae388f4e947f508cedda70b806a839326ea4af/homeassistant/components/vesync/const.py#L51-L52

These devices in the old method didn't create a switch entity just a light entity. Currently with my code it would also get a switch. Should we leave it like that so if dimmable switch or outlet you still get a switch toggle? This is the easier way, other option is adjust the exists to exclude the type of VeSyncDimmerSwitch which is a type of switch within the library.

@iprak Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to provided a separate switch for dimmer. LightEntity extends from ToggleEntity and so it provided on/off capability natively.

This is what I see for a dimmer switch
image

Copy link
Contributor Author

@cdnninja cdnninja Jan 19, 2025

Choose a reason for hiding this comment

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

Went back to fix this and realized I was correct up front. In pyvesync vesyncswitch has two child classes, VeSyncWallSwitch and VeSyncDimmerSwitch. The above filters to just wall switch so dimmer isn't used. As such this won't show for dimmers. No changes required. I updated the helper comment some to try make this more clear.

name=None,
on_fn=lambda device: device.turn_on(),
off_fn=lambda device: device.turn_off(),
),
)


async def async_setup_entry(
hass: HomeAssistant,
config_entry: ConfigEntry,
async_add_entities: AddEntitiesCallback,
) -> None:
"""Set up switches."""
"""Set up switch platform."""

coordinator = hass.data[DOMAIN][VS_COORDINATOR]

Expand All @@ -45,53 +75,45 @@
async_add_entities,
coordinator: VeSyncDataCoordinator,
):
"""Check if device is a switch and add entity."""
entities: list[VeSyncBaseSwitch] = []
for dev in devices:
if DEV_TYPE_TO_HA.get(dev.device_type) == "outlet":
entities.append(VeSyncSwitchHA(dev, coordinator))
elif DEV_TYPE_TO_HA.get(dev.device_type) == "switch":
entities.append(VeSyncLightSwitch(dev, coordinator))

async_add_entities(entities, update_before_add=True)


class VeSyncBaseSwitch(VeSyncBaseEntity, SwitchEntity):
"""Base class for VeSync switch Device Representations."""

_attr_name = None

def turn_on(self, **kwargs: Any) -> None:
"""Turn the device on."""
self.device.turn_on()

@property
def is_on(self) -> bool:
"""Return True if device is on."""
return self.device.device_status == "on"
"""Check if device is online and add entity."""
async_add_entities(
VeSyncSwitchEntity(dev, description, coordinator)
for dev in devices
for description in SENSOR_DESCRIPTIONS
if description.exists_fn(dev)
)

def turn_off(self, **kwargs: Any) -> None:
"""Turn the device off."""
self.device.turn_off()

class VeSyncSwitchEntity(SwitchEntity, VeSyncBaseEntity):
"""VeSync switch entity class."""

class VeSyncSwitchHA(VeSyncBaseSwitch, SwitchEntity):
"""Representation of a VeSync switch."""
entity_description: VeSyncSwitchEntityDescription

def __init__(
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to define _attr_unique_id otherwise adding another switch entity fails.

Something like this will work - self._attr_unique_id = f"{super().unique_id}-{description.key}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the two devices exposed today if we change this it will be a breaking change. However I could add a filter to only do the above unique ID on ones other than the outlet and switch on we offer today. It would set us up for future.

Thoughts on that approach?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose that will work. @joostlek might have reviewed code which ran into similar shortcoming and if the _attr_unique_id was defined as a breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

Oh that is a good catch. If that is the case we should migrate first. I don't feel like merging this as it will create entities with not unique unique ids. So we should have a preliminary PR where we migrate all the unique ids of the switches to something else and then we can set up new ones

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, today this code doesn't conflict or impact unique ID, however since trying to setup for growth makes sense we fix this now. I will update this code to have the unique ID as mentioned above and chat with iprak around getting a PR for that migration that will merge first.

self, plug: VeSyncBaseDevice, coordinator: VeSyncDataCoordinator
self,
device: VeSyncBaseDevice,
description: VeSyncSwitchEntityDescription,
coordinator: VeSyncDataCoordinator,
) -> None:
"""Initialize the VeSync switch device."""
super().__init__(plug, coordinator)
self.smartplug = plug
"""Initialize the sensor."""
super().__init__(device, coordinator)
self.entity_description = description
if is_outlet(self.device):
self._attr_device_class = SwitchDeviceClass.OUTLET
elif is_wall_switch(self.device):
self._attr_device_class = SwitchDeviceClass.SWITCH

@property
def is_on(self) -> bool | None:
"""Return the entity value to represent the entity state."""
return self.entity_description.is_on(self.device)

class VeSyncLightSwitch(VeSyncBaseSwitch, SwitchEntity):
"""Handle representation of VeSync Light Switch."""
def turn_off(self, **kwargs: Any) -> None:
"""Turn the entity off."""
self.entity_description.off_fn(self.device)
self.schedule_update_ha_state()

Check warning on line 114 in homeassistant/components/vesync/switch.py

View check run for this annotation

Codecov / codecov/patch

homeassistant/components/vesync/switch.py#L113-L114

Added lines #L113 - L114 were not covered by tests
Copy link
Member

Choose a reason for hiding this comment

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

Why do we do that with the state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without it HA toggles it back the other way until next scan interval which gives it a poor user experience as it appears to have not worked.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't schedule_update_ha_state be only called if turn_off succeeds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some other spots have it this way - my theory was after touching devices we should check the source data regardless to ensure we are updated. Both approaches though in most cases would show correctly I would think. Which do you prefer?

Copy link
Contributor

@iprak iprak Jan 25, 2025

Choose a reason for hiding this comment

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

The turn_on/off return true if operation succeeds and I think that should be respected and used as the basis of state refresh. Yes I do see some integration call this all the time and I would like @joostlek 's input on this.

I think without this, the entity would show the new data when it is refreshed but I am not sure if the underlying framework does the same. Some other integrations invoke a refresh and I think that takes care of cases where multiple entities get updated in response to one action.

Copy link
Contributor

Choose a reason for hiding this comment

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

Switch returns true if the web call succeeded, we should use that return value to schedule_update_ha_state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adjusted.


def __init__(
self, switch: VeSyncBaseDevice, coordinator: VeSyncDataCoordinator
) -> None:
"""Initialize Light Switch device class."""
super().__init__(switch, coordinator)
self.switch = switch
def turn_on(self, **kwargs: Any) -> None:
"""Turn the entity on."""
self.entity_description.on_fn(self.device)
self.schedule_update_ha_state()

Check warning on line 119 in homeassistant/components/vesync/switch.py

View check run for this annotation

Codecov / codecov/patch

homeassistant/components/vesync/switch.py#L118-L119

Added lines #L118 - L119 were not covered by tests
6 changes: 4 additions & 2 deletions tests/components/vesync/snapshots/test_switch.ambr
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@
'name': None,
'options': dict({
}),
'original_device_class': None,
'original_device_class': <SwitchDeviceClass.OUTLET: 'outlet'>,
'original_icon': None,
'original_name': None,
'platform': 'vesync',
Expand All @@ -375,6 +375,7 @@
# name: test_switch_state[Outlet][switch.outlet]
StateSnapshot({
'attributes': ReadOnlyDict({
'device_class': 'outlet',
'friendly_name': 'Outlet',
}),
'context': <ANY>,
Expand Down Expand Up @@ -480,7 +481,7 @@
'name': None,
'options': dict({
}),
'original_device_class': None,
'original_device_class': <SwitchDeviceClass.SWITCH: 'switch'>,
'original_icon': None,
'original_name': None,
'platform': 'vesync',
Expand All @@ -495,6 +496,7 @@
# name: test_switch_state[Wall Switch][switch.wall_switch]
StateSnapshot({
'attributes': ReadOnlyDict({
'device_class': 'switch',
'friendly_name': 'Wall Switch',
}),
'context': <ANY>,
Expand Down