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

Implement embedding DeviceStatus containers #1526

Merged
merged 4 commits into from
Sep 20, 2022
Merged
Show file tree
Hide file tree
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
12 changes: 12 additions & 0 deletions docs/contributing.rst
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,18 @@ Furthermore, it allows defining meta information about properties that are espec
In practice this means that neither the input nor the output values of functions decorated with
the descriptors are enforced automatically by this library.

Embedding Containers
""""""""""""""""""""

Sometimes your device requires multiple I/O requests to gather information you want to expose
to downstream users. One example of such is Roborock vacuum integration, where the status request
does not report on information about consumables.

To make it easy for downstream users, you can *embed* other status container classes into a single
one using :meth:`miio.devicestatus.DeviceStatus.embed`.
This will create a copy of the exposed descriptors to the main container and act as a proxy to give
access to the properties of embedded containers.


Sensors
"""""""
Expand Down
50 changes: 46 additions & 4 deletions miio/devicestatus.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ def __new__(metacls, name, bases, namespace, **kwargs):
cls._switches: Dict[str, SwitchDescriptor] = {}
cls._settings: Dict[str, SettingDescriptor] = {}

cls._embedded: Dict[str, "DeviceStatus"] = {}

descriptor_map = {
"sensor": cls._sensors,
"switch": cls._switches,
Expand All @@ -58,7 +60,8 @@ class DeviceStatus(metaclass=_StatusMeta):
All status container classes should inherit from this class:
* This class allows downstream users to access the available information in an
introspectable way.
introspectable way. See :func:`@property`, :func:`switch`, and :func:`@setting`.
* :func:`embed` allows embedding other status containers.
* The __repr__ implementation returns all defined properties and their values.
"""

Expand All @@ -76,6 +79,10 @@ def __repr__(self):
prop_value = ex.__class__.__name__

s += f" {name}={prop_value}"

for name, embedded in self._embedded.items():
s += f" {name}={repr(embedded)}"

s += ">"
return s

Expand All @@ -100,11 +107,46 @@ def settings(self) -> Dict[str, SettingDescriptor]:
"""
return self._settings # type: ignore[attr-defined]

def embed(self, other: "DeviceStatus"):
"""Embed another status container to current one.
This makes it easy to provide a single status response for cases where responses
from multiple I/O calls is wanted to provide a simple interface for downstreams.
Internally, this will prepend the name of the other class to the property names,
and override the __getattribute__ to lookup attributes in the embedded containers.
"""
other_name = str(other.__class__.__name__)

self._embedded[other_name] = other

for name, sensor in other.sensors().items():
final_name = f"{other_name}:{name}"
import attr

self._sensors[final_name] = attr.evolve(sensor, property=final_name)

for name, switch in other.switches().items():
final_name = f"{other_name}:{name}"
self._switches[final_name] = attr.evolve(switch, property=final_name)

for name, setting in other.settings().items():
final_name = f"{other_name}:{name}"
self._settings[final_name] = attr.evolve(setting, property=final_name)

def __getattribute__(self, item):
"""Overridden to lookup properties from embedded containers."""
if ":" not in item:
return super().__getattribute__(item)

embed, prop = item.split(":")
return getattr(self._embedded[embed], prop)


def sensor(name: str, *, unit: str = "", **kwargs):
"""Syntactic sugar to create SensorDescriptor objects.
The information can be used by users of the library to programatically find out what
The information can be used by users of the library to programmatically find out what
types of sensors are available for the device.
The interface is kept minimal, but you can pass any extra keyword arguments.
Expand Down Expand Up @@ -144,7 +186,7 @@ def _sensor_type_for_return_type(func):
def switch(name: str, *, setter_name: str, **kwargs):
"""Syntactic sugar to create SwitchDescriptor objects.
The information can be used by users of the library to programatically find out what
The information can be used by users of the library to programmatically find out what
types of sensors are available for the device.
The interface is kept minimal, but you can pass any extra keyword arguments.
Expand Down Expand Up @@ -184,7 +226,7 @@ def setting(
):
"""Syntactic sugar to create SettingDescriptor objects.
The information can be used by users of the library to programatically find out what
The information can be used by users of the library to programmatically find out what
types of sensors are available for the device.
The interface is kept minimal, but you can pass any extra keyword arguments.
Expand Down
31 changes: 27 additions & 4 deletions miio/integrations/vacuum/roborock/tests/test_vacuum.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,34 @@ def __init__(self, *args, **kwargs):
"water_box_status": 1,
}

self.dummies = {}
self.dummies["consumables"] = [
{
"filter_work_time": 32454,
"sensor_dirty_time": 3798,
"side_brush_work_time": 32454,
"main_brush_work_time": 32454,
}
]
self.dummies["clean_summary"] = [
174145,
2410150000,
82,
[
1488240000,
1488153600,
1488067200,
1487980800,
1487894400,
1487808000,
1487548800,
],
]

self.return_values = {
"get_status": self.vacuum_state,
"get_status": lambda x: [self.state],
"get_consumable": lambda x: self.dummies["consumables"],
"get_clean_summary": lambda x: self.dummies["clean_summary"],
"app_start": lambda x: self.change_mode("start"),
"app_stop": lambda x: self.change_mode("stop"),
"app_pause": lambda x: self.change_mode("pause"),
Expand Down Expand Up @@ -77,9 +103,6 @@ def change_mode(self, new_mode):
elif new_mode == "charge":
self.state["state"] = DummyVacuum.STATE_CHARGING

def vacuum_state(self, _):
return [self.state]


@pytest.fixture(scope="class")
def dummyvacuum(request):
Expand Down
5 changes: 4 additions & 1 deletion miio/integrations/vacuum/roborock/vacuum.py
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,10 @@ def manual_control(
@command()
def status(self) -> VacuumStatus:
"""Return status of the vacuum."""
return VacuumStatus(self.send("get_status")[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

@rytilahti very nice, this simplifies it quite a bit.
However now there is no way to only get the VucuumStatus withouth calling the consumable_status and clean_history.
For HomeAssistant it is optimal to have every endpoint a diffrent UpdateCoordinator such that if some entities are disabled, no needless calls are made to the roborock.

However this would complicate the code quite a bit, so I am okay with just having a single coordinator that always calls all endpoints and just accept the possible unneeded calls.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will try to test this tonight

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm slowly working the backlog after returning back home. You have a good point about having separate functions to make it possible to avoid unnecessary I/O that I was not considering while working on the initial implementation. This is not only relevant to save some I/O requests, but the current way has another issue, namely that handling of error cases (e.g., a device not supporting some feature) needs to be done case-by-case...

Instead of embedding, another approach could be to extend the API to add a way to expose references to methods that can be used to obtain the status information, and leave it to the downstream like homeassistant to choose if they want to use it. The problem I'm seeing though is that when the sensors & co are dynamic, homeassistant would need to hardcode what it wants as long as there is no way to check which entities are disabled, so I don't think that's a good idea for now. Your solution to add a separate "raw update" for cli and other users to allow obtaining just the get_status results sounds good to me.

Back to the issue of error handling / avoiding unsupported calls: maybe the embed should rather acts as a builder and take a callable instead of a status container as an input. This would make it easier to implement generic handling to avoid crashing on unsupported features, as well as to avoid doing unnecessary I/O on subsequent update cycles. I think this needs some thinking and experimenting to see what works the best.

status = VacuumStatus(self.send("get_status")[0])
status.embed(self.consumable_status())
status.embed(self.clean_history())
return status

def enable_log_upload(self):
raise NotImplementedError("unknown parameters")
Expand Down
35 changes: 35 additions & 0 deletions miio/tests/test_devicestatus.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
from enum import Enum

import pytest

from miio import Device, DeviceStatus
from miio.descriptors import EnumSettingDescriptor, NumberSettingDescriptor
from miio.devicestatus import sensor, setting, switch
Expand Down Expand Up @@ -198,3 +200,36 @@ def level(self) -> TestEnum:

settings["level"].setter(TestEnum.Second)
setter.assert_called_with(TestEnum.Second)


def test_embed():
class MainStatus(DeviceStatus):
@property
@sensor("main_sensor")
def main_sensor(self):
return "main"

class SubStatus(DeviceStatus):
@property
@sensor("sub_sensor")
def sub_sensor(self):
return "sub"

main = MainStatus()
assert len(main.sensors()) == 1

sub = SubStatus()
main.embed(sub)
sensors = main.sensors()
assert len(sensors) == 2

assert getattr(main, sensors["main_sensor"].property) == "main"
assert getattr(main, sensors["SubStatus:sub_sensor"].property) == "sub"

with pytest.raises(KeyError):
main.sensors()["nonexisting_sensor"]

assert (
repr(main)
== "<MainStatus main_sensor=main SubStatus=<SubStatus sub_sensor=sub>>"
)