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

Initialize and cache descriptors only once #1592

Closed
wants to merge 48 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
c99de9b
Add caching
starkillerOG Nov 9, 2022
14933c4
fix black
starkillerOG Nov 9, 2022
62cf7c7
improve caching
starkillerOG Nov 14, 2022
3166772
Move descriptor initialization after model is known
starkillerOG Nov 14, 2022
ba83361
Merge branch 'master' into caching
starkillerOG Nov 14, 2022
6904829
fix black
starkillerOG Nov 14, 2022
0fe1954
Merge branch 'caching' of https://github.com/starkillerOG/python-miio…
starkillerOG Nov 14, 2022
b2a7873
Update miio/device.py
starkillerOG Nov 14, 2022
1dee65a
Update device.py
starkillerOG Nov 14, 2022
a7fcbd8
Merge branch 'caching' of https://github.com/starkillerOG/python-miio…
starkillerOG Nov 14, 2022
67013e4
simplify
starkillerOG Nov 14, 2022
2bcb4ba
Merge branch 'master' into caching
starkillerOG Nov 14, 2022
54121f5
Update miio/device.py
starkillerOG Nov 15, 2022
d5ec615
initialize descriptors when needed
starkillerOG Nov 15, 2022
391e42f
Merge branch 'caching' of https://github.com/starkillerOG/python-miio…
starkillerOG Nov 15, 2022
86d6e69
Merge branch 'master' into caching
starkillerOG Nov 15, 2022
79b5e0e
Move actions to _initialize_descriptors
starkillerOG Nov 15, 2022
4494ad1
fix linting
starkillerOG Nov 15, 2022
cc010b6
fix typing
starkillerOG Nov 15, 2022
96ecb45
fix isort
starkillerOG Nov 15, 2022
7570e81
fix casting
starkillerOG Nov 15, 2022
a8a1ae5
Do not make descriptors optional
starkillerOG Nov 17, 2022
ba136e2
Split out action/setting/sensor descriptor retrieval
starkillerOG Nov 17, 2022
db24017
Update dummies.py
starkillerOG Nov 17, 2022
9c9493c
Move _initialize_descriptors out of info call
starkillerOG Nov 17, 2022
f31c701
fix black
starkillerOG Nov 18, 2022
a29c05d
update descriptions
starkillerOG Nov 18, 2022
c2691bc
fix docstring
starkillerOG Nov 18, 2022
15b67f9
docformatter
starkillerOG Nov 18, 2022
97d4784
Merge branch 'master' into caching
starkillerOG Nov 27, 2022
1ebb309
revert docstring changes
starkillerOG Nov 28, 2022
d03c6f2
Merge branch 'master' into caching
starkillerOG Nov 28, 2022
441de02
do not add initialize method
starkillerOG Nov 28, 2022
d5c8a6c
fix typing
starkillerOG Nov 28, 2022
1727060
fix black
starkillerOG Nov 28, 2022
7d17f08
fix mypy None possibilities
starkillerOG Nov 28, 2022
c92be40
fixes
starkillerOG Nov 28, 2022
58f72b3
fix mypy
starkillerOG Nov 28, 2022
ddb0e82
fix mypy complaining
starkillerOG Nov 28, 2022
587194f
fix tests
starkillerOG Nov 28, 2022
480f384
do not change model can be None
starkillerOG Nov 28, 2022
82d3782
cast model
starkillerOG Nov 28, 2022
a43614c
try fixing tests
starkillerOG Nov 28, 2022
0f01e36
try to fix tests
starkillerOG Nov 28, 2022
2c79fe9
try fixing tests
starkillerOG Nov 28, 2022
99a7f11
try fixing tests
starkillerOG Dec 5, 2022
051748c
fix tests device_id query
starkillerOG Dec 5, 2022
7f2e163
try fixing tests
starkillerOG Dec 5, 2022
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
2 changes: 1 addition & 1 deletion miio/airdehumidifier.py
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ def status(self) -> AirDehumidifierStatus:
values = self.get_properties(properties, max_properties=1)

return AirDehumidifierStatus(
defaultdict(lambda: None, zip(properties, values)), self.info()
defaultdict(lambda: None, zip(properties, values)), self._fetch_info()
Copy link
Owner

Choose a reason for hiding this comment

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

This is wrong, the _fetch_info() is device-class internals and should not be accessed by any other class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well how do you then suggest to prevent the loop: status --> info --> _initialize_descriptors--> status --> etc.
I can just remove the info call, but I am not sure what will then break downstream.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure what's the best approach right off the bat (especially as genericmiot constructs status containers dynamically), but for statically defined containers it would be enough to access the status container class without really initializing one, right?

In [5]: from miio.integrations.vacuum.roborock import VacuumStatus

In [6]: vars(VacuumStatus)
Out[6]: 
mappingproxy({'__module__': 'miio.integrations.vacuum.roborock.vacuumcontainers',
<snip>
              '_sensors': {'state_code': SensorDescriptor(id='VacuumStatus.state_code', type=<class 'int'>, name='State code', property='state_code', unit=None),
               'state': SensorDescriptor(id='VacuumStatus.state', type=<class 'str'>, name='State', property='state', unit=None),
<snip>
              '_settings': {'fanspeed': NumberSettingDescriptor(id='VacuumStatus.fanspeed', name='Fanspeed', property='fanspeed', unit='%', min_value=0, max_value=100, step=1, range_attribute=None, type=<SettingType.Number: 2>),
<snip>

Probably the simplest way to access the status container would be using the return type annotation like this:

_status_container = self_.status.__annotations__["return"]

Do you think that could work for the time being until we figure out a better solution for that?

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 just tried it, but that does not include the embeded sensors:

from miio import RoborockVacuum

r = RoborockVacuum(ip = "192.168.1.IP", token = "TokenTokenToken")
status = r.status.__annotations__["return"]
print(status.sensors(status))

That is missing the embeded sensors/settings....

Copy link
Owner

Choose a reason for hiding this comment

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

Ouch, that's indeed annoying... The real solution would be doing the "embedding" of status container classes outside of the status query (maybe inside __init__), and performing the update on the container prior to returning it in status(). This way:

  1. we can introspect the available information without performing any I/O.
  2. if some of the method calls required to embedded containers fail, we can remove those queries from future update cycles.

The question is how/where to do this sanely, I don't have an answer to that right now.

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 do see what you mean, and think it is a good path forward,
but I think at this point it gets a bit to complicated for me.

I think it would be more efficient if you would implement those changes.
Than we also do not need to have long wait times in between us working on it.

You could do one of the following:

  • make the changes and push them to this branch.
  • copy this code as a start, make your own branch with a new PR and close this one
  • First merge this as a starting point and then make the changes later to get rid of the _fetch_info calls in device classes (can be added as a TO DO)

)

@command(default_output=format_output("Powering on"))
Expand Down
6 changes: 5 additions & 1 deletion miio/airqualitymonitor.py
Original file line number Diff line number Diff line change
Expand Up @@ -170,9 +170,13 @@ def status(self) -> AirQualityMonitorStatus:
self.model, AVAILABLE_PROPERTIES[MODEL_AIRQUALITYMONITOR_V1]
)

firmware_version = self._fetch_info().firmware_version
if firmware_version is None:
firmware_version = "unknown"

is_s1_firmware_version_4 = (
self.model == MODEL_AIRQUALITYMONITOR_S1
and self.info().firmware_version.startswith("4")
and firmware_version.startswith("4")
)
if is_s1_firmware_version_4 and "battery" in properties:
properties.remove("battery")
Expand Down
113 changes: 77 additions & 36 deletions miio/device.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ def __init__(
self.token: Optional[str] = token
self._model: Optional[str] = model
self._info: Optional[DeviceInfo] = None
self._settings: Optional[Dict[str, SettingDescriptor]] = None
self._sensors: Optional[Dict[str, SensorDescriptor]] = None
self._actions: Optional[Dict[str, ActionDescriptor]] = None
timeout = timeout if timeout is not None else self.timeout
self._protocol = MiIOProtocol(
Expand Down Expand Up @@ -133,13 +135,18 @@ def info(self, *, skip_cache=False) -> DeviceInfo:

:param skip_cache bool: Skip the cache
"""
self._initialize_descriptors()

if self._info is not None and not skip_cache:
return self._info

return self._fetch_info()

def _fetch_info(self) -> DeviceInfo:
def _fetch_info(self, *, skip_cache=False) -> DeviceInfo:
"""Perform miIO.info query on the device and cache the result."""
if self._info is not None and not skip_cache:
return self._info

try:
devinfo = DeviceInfo(self.send("miIO.info"))
self._info = devinfo
Expand All @@ -159,6 +166,63 @@ def _fetch_info(self) -> DeviceInfo:
"Unable to request miIO.info from the device"
) from ex

def _setting_descriptors_from_status(
self, status: DeviceStatus
) -> Dict[str, SettingDescriptor]:
"""Get the setting descriptors from a DeviceStatus."""
settings = status.settings()
for setting in settings.values():
if setting.setter_name is not None:
setting.setter = getattr(self, setting.setter_name)
if setting.setter is None:
raise Exception(
f"Neither setter or setter_name was defined for {setting}"
)
setting = cast(EnumSettingDescriptor, setting)
if (
setting.type == SettingType.Enum
and setting.choices_attribute is not None
rytilahti marked this conversation as resolved.
Show resolved Hide resolved
):
retrieve_choices_function = getattr(self, setting.choices_attribute)
setting.choices = retrieve_choices_function()
if setting.type == SettingType.Number:
setting = cast(NumberSettingDescriptor, setting)
if setting.range_attribute is not None:
range_def = getattr(self, setting.range_attribute)
setting.min_value = range_def.min_value
setting.max_value = range_def.max_value
setting.step = range_def.step

return settings

def _sensor_descriptors_from_status(
self, status: DeviceStatus
) -> Dict[str, SensorDescriptor]:
"""Get the sensor descriptors from a DeviceStatus."""
return status.sensors()

def _action_descriptors(self) -> Dict[str, ActionDescriptor]:
"""Get the action descriptors from a DeviceStatus."""
actions = {}
for action_tuple in getmembers(self, lambda o: hasattr(o, "_action")):
method_name, method = action_tuple
action = method._action
action.method = method # bind the method
actions[method_name] = action

return actions

def _initialize_descriptors(self) -> None:
"""Cache all the descriptors once on the first call."""
if self._sensors is not None:
return

status = self.status()

self._sensors = self._sensor_descriptors_from_status(status)
self._settings = self._setting_descriptors_from_status(status)
self._actions = self._action_descriptors()

@property
def device_id(self) -> int:
"""Return device id (did), if available."""
Expand All @@ -182,7 +246,7 @@ def model(self) -> str:
if self._model is not None:
return self._model

return self.info().model
return cast(str, self._fetch_info().model)

def update(self, url: str, md5: str):
"""Start an OTA update."""
Expand Down Expand Up @@ -253,49 +317,26 @@ def status(self) -> DeviceStatus:
def actions(self) -> Dict[str, ActionDescriptor]:
"""Return device actions."""
if self._actions is None:
self._actions = {}
for action_tuple in getmembers(self, lambda o: hasattr(o, "_action")):
method_name, method = action_tuple
action = method._action
action.method = method # bind the method
self._actions[method_name] = action
self._initialize_descriptors()

self._actions = cast(Dict[str, ActionDescriptor], self._actions)
return self._actions

def settings(self) -> Dict[str, SettingDescriptor]:
"""Return device settings."""
settings = self.status().settings()
for setting in settings.values():
# TODO: Bind setter methods, this should probably done only once during init.
if setting.setter is None:
# TODO: this is ugly, how to fix the issue where setter_name is optional and thus not acceptable for getattr?
if setting.setter_name is None:
raise Exception(
f"Neither setter or setter_name was defined for {setting}"
)

setting.setter = getattr(self, setting.setter_name)
if (
isinstance(setting, EnumSettingDescriptor)
and setting.choices_attribute is not None
):
retrieve_choices_function = getattr(self, setting.choices_attribute)
setting.choices = retrieve_choices_function() # This can do IO
if setting.type == SettingType.Number:
setting = cast(NumberSettingDescriptor, setting)
if setting.range_attribute is not None:
range_def = getattr(self, setting.range_attribute)
setting.min_value = range_def.min_value
setting.max_value = range_def.max_value
setting.step = range_def.step
if self._settings is None:
self._initialize_descriptors()

return settings
self._settings = cast(Dict[str, SettingDescriptor], self._settings)
return self._settings

def sensors(self) -> Dict[str, SensorDescriptor]:
"""Return device sensors."""
# TODO: the latest status should be cached and re-used by all meta information getters
sensors = self.status().sensors()
return sensors
if self._sensors is None:
self._initialize_descriptors()

self._sensors = cast(Dict[str, SensorDescriptor], self._sensors)
return self._sensors

def __repr__(self):
return f"<{self.__class__.__name__ }: {self.ip} (token: {self.token})>"
2 changes: 1 addition & 1 deletion miio/gateway/gateway.py
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ def devices(self):
def mac(self):
"""Return the mac address of the gateway."""
if self._info is None:
self._info = self.info()
self._info = self._fetch_info()
return self._info.mac_address

@property
Expand Down
2 changes: 1 addition & 1 deletion miio/integrations/humidifier/zhimi/airhumidifier.py
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ def status(self) -> AirHumidifierStatus:
values = self.get_properties(properties, max_properties=_props_per_request)

return AirHumidifierStatus(
defaultdict(lambda: None, zip(properties, values)), self.info()
defaultdict(lambda: None, zip(properties, values)), self._fetch_info()
)

@command(default_output=format_output("Powering on"))
Expand Down
22 changes: 14 additions & 8 deletions miio/integrations/vacuum/roborock/vacuum.py
Original file line number Diff line number Diff line change
Expand Up @@ -175,12 +175,15 @@ def resume_or_start(self):

return self.start()

def _fetch_info(self) -> DeviceInfo:
def _fetch_info(self, *, skip_cache=False) -> DeviceInfo:
"""Return info about the device.

This is overrides the base class info to account for gen1 devices that do not
respond to info query properly when not connected to the cloud.
"""
if self._info is not None and not skip_cache:
return self._info

try:
info = super()._fetch_info()
return info
Expand Down Expand Up @@ -596,13 +599,16 @@ def _enum_as_dict(cls):

if self.model == ROCKROBO_V1:
_LOGGER.debug("Got robov1, checking for firmware version")
fw_version = self.info().firmware_version
version, build = fw_version.split("_")
version = tuple(map(int, version.split(".")))
if version >= (3, 5, 8):
fanspeeds = FanspeedV3
elif version == (3, 5, 7):
fanspeeds = FanspeedV2
fw_version = self._fetch_info().firmware_version
if fw_version is not None:
version, build = fw_version.split("_")
version_tuple = tuple(map(int, version.split(".")))
if version_tuple >= (3, 5, 8):
fanspeeds = FanspeedV3
elif version_tuple == (3, 5, 7):
fanspeeds = FanspeedV2
else:
fanspeeds = FanspeedV1
else:
fanspeeds = FanspeedV1
elif self.model == ROCKROBO_E2:
Expand Down
3 changes: 3 additions & 0 deletions miio/tests/dummies.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ def __init__(self, *args, **kwargs):
self.start_state = self.state.copy()
self._protocol = DummyMiIOProtocol(self)
self._info = None
self._settings = {}
self._sensors = {}
self._actions = {}
# TODO: ugly hack to check for pre-existing _model
if getattr(self, "_model", None) is None:
self._model = "dummy.model"
Expand Down
10 changes: 9 additions & 1 deletion miio/tests/test_airqualitymonitor.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
class DummyAirQualityMonitorV1(DummyDevice, AirQualityMonitor):
def __init__(self, *args, **kwargs):
self._model = MODEL_AIRQUALITYMONITOR_V1
self.version = "3.1.8_9999"
self.state = {
"power": "on",
"aqi": 34,
Expand All @@ -35,6 +36,9 @@ def __init__(self, *args, **kwargs):
}
super().__init__(args, kwargs)

def _fetch_info(self):
return DummyDeviceInfo(version=self.version)


@pytest.fixture(scope="class")
def airqualitymonitorv1(request):
Expand Down Expand Up @@ -100,7 +104,7 @@ def _get_state(self, props):
"""Return wanted properties."""
return self.state

def info(self):
def _fetch_info(self):
return DummyDeviceInfo(version=self.version)


Expand Down Expand Up @@ -185,6 +189,7 @@ def test_status(self):
class DummyAirQualityMonitorB1(DummyDevice, AirQualityMonitor):
def __init__(self, *args, **kwargs):
self._model = MODEL_AIRQUALITYMONITOR_B1
self.version = "3.1.8_9999"
self.state = {
"co2e": 1466,
"humidity": 59.79999923706055,
Expand All @@ -201,6 +206,9 @@ def _get_state(self, props):
"""Return wanted properties."""
return self.state

def _fetch_info(self):
return DummyDeviceInfo(version=self.version)


@pytest.fixture(scope="class")
def airqualitymonitorb1(request):
Expand Down
5 changes: 5 additions & 0 deletions miio/tests/test_devicestatus.py
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ def level(self) -> int:
return 1

mocker.patch("miio.Device.send")
mocker.patch("miio.Device.send_handshake")
d = Device("127.0.0.1", "68ffffffffffffffffffffffffffffff")

# Patch status to return our class
Expand Down Expand Up @@ -167,7 +168,9 @@ def level(self) -> int:
return 1

mocker.patch("miio.Device.send")
mocker.patch("miio.Device.send_handshake")
d = Device("127.0.0.1", "68ffffffffffffffffffffffffffffff")
d._protocol._device_id = b"12345678"

# Patch status to return our class
mocker.patch.object(d, "status", return_value=Settings())
Expand Down Expand Up @@ -208,7 +211,9 @@ def level(self) -> TestEnum:
return TestEnum.First

mocker.patch("miio.Device.send")
mocker.patch("miio.Device.send_handshake")
d = Device("127.0.0.1", "68ffffffffffffffffffffffffffffff")
d._protocol._device_id = b"12345678"

# Patch status to return our class
mocker.patch.object(d, "status", return_value=Settings())
Expand Down
4 changes: 2 additions & 2 deletions miio/wifirepeater.py
Original file line number Diff line number Diff line change
Expand Up @@ -132,9 +132,9 @@ def set_configuration(self, ssid: str, password: str, ssid_hidden: bool = False)
)
def wifi_roaming(self) -> bool:
"""Return the roaming setting."""
return self.info().raw["desc"]["wifi_explorer"] == 1
return self._fetch_info().raw["desc"]["wifi_explorer"] == 1

@command(default_output=format_output("RSSI of the accesspoint: {result}"))
def rssi_accesspoint(self) -> int:
"""Received signal strength indicator of the accesspoint."""
return self.info().accesspoint["rssi"]
return self._fetch_info().accesspoint["rssi"]