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

vesync new platform of binary sensor #134221

Open
wants to merge 29 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
fab323f
Added the basics for binary sensor support.
cdnninja Dec 29, 2024
c4d2533
Binary sensor first draft
cdnninja Dec 29, 2024
de6f8da
Update tests
cdnninja Dec 29, 2024
9c44916
Remove unneeded code to improve test coverage.
cdnninja Dec 29, 2024
02dc884
Adjust what sensors are displayed.
cdnninja Dec 29, 2024
a8cab84
Final corrections to handle dict. Clean up too.
cdnninja Dec 29, 2024
cefac79
Align order to sensor for consistancy
cdnninja Jan 2, 2025
de6f5b7
Merge branch 'dev' into binary-sensor
cdnninja Jan 2, 2025
25c5958
unload binary sensor
cdnninja Jan 2, 2025
7e7c0d5
Update homeassistant/components/vesync/binary_sensor.py
cdnninja Jan 2, 2025
faccf26
Update strings.json
cdnninja Jan 2, 2025
66fb73c
Wrong type
cdnninja Jan 2, 2025
33ab7be
Add call backs.
cdnninja Jan 2, 2025
0b50f48
Merge branch 'home-assistant:dev' into binary-sensor
cdnninja Jan 3, 2025
c010230
Align to coordinator
cdnninja Jan 3, 2025
8eaa370
Update homeassistant/components/vesync/binary_sensor.py
cdnninja Jan 3, 2025
412e1ad
Update common.py
cdnninja Jan 3, 2025
0a9331a
Update common.py
cdnninja Jan 3, 2025
3a01836
Remove warning as this mostly hits humidifers
cdnninja Jan 4, 2025
9410726
Merge branch 'dev' into binary-sensor
cdnninja Jan 5, 2025
a7e50e7
Correct ruff.
cdnninja Jan 5, 2025
330457c
format
cdnninja Jan 6, 2025
0699a22
Merge branch 'dev' into binary-sensor
cdnninja Jan 10, 2025
8d9b4ef
Refactor based on parent branch
cdnninja Jan 10, 2025
b4695c9
Merge branch 'dev' into binary-sensor
cdnninja Jan 15, 2025
27d775e
Correct merge issues and initial test corrections
cdnninja Jan 15, 2025
163d18b
Feedback corrections.
cdnninja Jan 15, 2025
b3211ff
Remove is_on value
cdnninja Jan 17, 2025
8eeec71
Move typing as per other feedback
cdnninja Jan 19, 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
1 change: 1 addition & 0 deletions homeassistant/components/vesync/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
from .coordinator import VeSyncDataCoordinator

PLATFORMS = [
Platform.BINARY_SENSOR,
Platform.FAN,
Platform.HUMIDIFIER,
Platform.LIGHT,
Expand Down
96 changes: 96 additions & 0 deletions homeassistant/components/vesync/binary_sensor.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
"""Binary Sensor for VeSync."""

from __future__ import annotations

import logging

from pyvesync.vesyncbasedevice import VeSyncBaseDevice

from homeassistant.components.binary_sensor import (
BinarySensorDeviceClass,
BinarySensorEntity,
BinarySensorEntityDescription,
)
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 .common import rgetattr
from .const import DOMAIN, VS_COORDINATOR, VS_DEVICES, VS_DISCOVERY
from .coordinator import VeSyncDataCoordinator
from .entity import VeSyncBaseEntity

_LOGGER = logging.getLogger(__name__)


SENSOR_DESCRIPTIONS: tuple[BinarySensorEntityDescription, ...] = (
BinarySensorEntityDescription(
key="water_lacks",
translation_key="water_lacks",
device_class=BinarySensorDeviceClass.PROBLEM,
),
BinarySensorEntityDescription(
key="details.water_tank_lifted",
translation_key="water_tank_lifted",
device_class=BinarySensorDeviceClass.PROBLEM,
),
)


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

coordinator = hass.data[DOMAIN][VS_COORDINATOR]

@callback
def discover(devices):
"""Add new devices to platform."""
_setup_entities(devices, async_add_entities)

Check warning on line 53 in homeassistant/components/vesync/binary_sensor.py

View check run for this annotation

Codecov / codecov/patch

homeassistant/components/vesync/binary_sensor.py#L53

Added line #L53 was not covered by tests

config_entry.async_on_unload(
async_dispatcher_connect(hass, VS_DISCOVERY.format(VS_DEVICES), discover)
)

_setup_entities(hass.data[DOMAIN][VS_DEVICES], async_add_entities, coordinator)


@callback
def _setup_entities(devices, async_add_entities, coordinator):
"""Add entity."""
async_add_entities(
(
VeSyncBinarySensor(dev, description, coordinator)
for dev in devices
for description in SENSOR_DESCRIPTIONS
if rgetattr(dev, description.key) is not None
Copy link
Contributor

@iprak iprak Jan 15, 2025

Choose a reason for hiding this comment

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

I find the use of key as means to check attribute presence cryptic. Why not use device type based check so that we generate sensors for known cases?

It looks like a lot more attributes are populated in build_humid_dict with default value but I definitely know that my humidifier Classic 200 does not support any of them.

However it could be tat some attributes are common but someone would need to verify that fact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My theory on this types of data and sensor.py is that datapoints vary by device. Different humidifiers have different data points for example. By checking for the datapoint we don't need to check against every potential child class defined. We are agnostic and data focused so if a new humidifier type is added to the child library no update needed beyond bumping the library. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking more on this - maybe I move that same logic to a exists_fn? Still with the reget but within the exists lambda. That way it is flexible depending on what is needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

The datapoint approach is what we would want but I don't have other devices to confirm or deny my theory. Let us go with it. The custom vesync integration also seems to do something similar so there must some prior validation.

As for the key, isn't that what is being read in the value lambda. What if we called it data_path or something and got rid of is_on lambda?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is_on is binary sensor native name: https://developers.home-assistant.io/docs/core/entity/binary-sensor

Now that I look at it again I think I leave as is. Not sure an easier way exists to use the is on value to safely check it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To confirm do you have changes requested or are you okay with approving as is?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would loved to see key and value lambda be combined since they seem to be using the same data path but I am okay with what we have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. So combine in description and use the same value programmatically for "is_on". I will give it a go.

Copy link
Contributor

Choose a reason for hiding this comment

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

It was just an idea. If we can have cases where key is one thing but the data returned is something, then we would want to keep what we have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is possible that occurs but quick to make that change when/if it occurs. VScode is uploading the changes now. Should be up shortly - it takes forever.

),
)


class VeSyncBinarySensor(BinarySensorEntity, VeSyncBaseEntity):
"""Vesync binary sensor class."""

entity_description: BinarySensorEntityDescription

def __init__(
self,
device: VeSyncBaseDevice,
description: BinarySensorEntityDescription,
coordinator: VeSyncDataCoordinator,
) -> None:
"""Initialize the sensor."""
super().__init__(device, coordinator)
self.entity_description = description
self._attr_unique_id = f"{super().unique_id}-{description.key}"

@property
def is_on(self) -> bool:
"""Return true if the binary sensor is on."""
_LOGGER.debug(rgetattr(self.device, self.entity_description.key))
return rgetattr(self.device, self.entity_description.key)
# return self.entity_description.is_on(self.device)
22 changes: 22 additions & 0 deletions homeassistant/components/vesync/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,28 @@
_LOGGER = logging.getLogger(__name__)


def rgetattr(obj: object, attr: str):
"""Return a string in the form word.1.2.3 and return the item as 3. Note that this last value could be in a dict as well."""
_this_func = rgetattr
sp = attr.split(".", 1)
if len(sp) == 1:
left, right = sp[0], ""
else:
left, right = sp

if isinstance(obj, dict):
obj = obj.get(left)
elif hasattr(obj, left):
obj = getattr(obj, left)
else:
return None

if right:
obj = _this_func(obj, right)

return obj


async def async_generate_device_list(
hass: HomeAssistant, manager: VeSync
) -> list[VeSyncBaseDevice]:
Expand Down
8 changes: 8 additions & 0 deletions homeassistant/components/vesync/strings.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,14 @@
"name": "Current voltage"
}
},
"binary_sensor": {
"water_lacks": {
"name": "Low water"
},
"water_tank_lifted": {
"name": "Water tank lifted"
}
},
"number": {
"mist_level": {
"name": "Mist level"
Expand Down
3 changes: 3 additions & 0 deletions tests/components/vesync/test_diagnostics.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,9 @@ async def test_async_get_device_diagnostics__single_fan(
"home_assistant.entities.2.state.last_changed": (str,),
"home_assistant.entities.2.state.last_reported": (str,),
"home_assistant.entities.2.state.last_updated": (str,),
"home_assistant.entities.3.state.last_changed": (str,),
"home_assistant.entities.3.state.last_reported": (str,),
"home_assistant.entities.3.state.last_updated": (str,),
}
)
)
2 changes: 2 additions & 0 deletions tests/components/vesync/test_init.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ async def test_async_setup_entry__no_devices(
assert setups_mock.call_count == 1
assert setups_mock.call_args.args[0] == config_entry
assert setups_mock.call_args.args[1] == [
Platform.BINARY_SENSOR,
Platform.FAN,
Platform.HUMIDIFIER,
Platform.LIGHT,
Expand Down Expand Up @@ -78,6 +79,7 @@ async def test_async_setup_entry__loads_fans(
assert setups_mock.call_count == 1
assert setups_mock.call_args.args[0] == config_entry
assert setups_mock.call_args.args[1] == [
Platform.BINARY_SENSOR,
Platform.FAN,
Platform.HUMIDIFIER,
Platform.LIGHT,
Expand Down