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

Add multi map handling to roborock part 2 #1614

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open
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
4 changes: 4 additions & 0 deletions miio/integrations/roborock/vacuum/tests/test_vacuum.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ def __init__(self, *args, **kwargs):
}
self._maps = None
self._map_enum_cache = None
self._floor_clean_details = {}
self._last_clean_details = None
self._searched_clean_id = None

self._status_helper = UpdateHelper(self.vacuum_status)
self.dummies = {
"consumables": [
Expand Down
72 changes: 63 additions & 9 deletions miio/integrations/roborock/vacuum/vacuum.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import pathlib
import time
from enum import Enum
from typing import Any, List, Optional, Type
from typing import Any, Dict, List, Optional, Type

import click
import pytz
Expand Down Expand Up @@ -49,6 +49,7 @@
CleaningSummary,
ConsumableStatus,
DNDStatus,
FloorCleanDetails,
MapList,
MopDryerSettings,
SoundInstallStatus,
Expand Down Expand Up @@ -143,12 +144,18 @@ def __init__(
ip, token, start_id, debug, lazy_discover, timeout, model=model
)
self.manual_seqnum = -1
self._clean_history: Optional[CleaningSummary] = None
self._searched_clean_id: Optional[int] = None
self._floor_clean_details: Dict[int, Optional[CleaningDetails]] = {}
self._last_clean_details: Optional[CleaningDetails] = None
self._maps: Optional[MapList] = None
self._map_enum_cache = None
self._status_helper = UpdateHelper(self.vacuum_status)
self._status_helper.add_update_method("map_list", self.get_maps)
self._status_helper.add_update_method("consumables", self.consumable_status)
self._status_helper.add_update_method("dnd_status", self.dnd_status)
self._status_helper.add_update_method("clean_history", self.clean_history)
self._status_helper.add_update_method("floor_clean", self.last_clean_all_floor)
self._status_helper.add_update_method("last_clean", self.last_clean_details)
self._status_helper.add_update_method("mop_dryer", self.mop_dryer_settings)

Expand Down Expand Up @@ -515,20 +522,68 @@ def enable_lab_mode(self, enable):
@command()
def clean_history(self) -> CleaningSummary:
"""Return generic cleaning history."""
return CleaningSummary(self.send("get_clean_summary"))
self._clean_history = CleaningSummary(self.send("get_clean_summary"))
return self._clean_history

@command()
def last_clean_details(self) -> Optional[CleaningDetails]:
def last_clean_details(self, skip_cache=False) -> Optional[CleaningDetails]:
"""Return details from the last cleaning.

Returns None if there has been no cleanups.
"""
history = self.clean_history()
if not history.ids:
if self._clean_history is None or skip_cache:
self.clean_history()
Comment on lines +534 to +535
Copy link
Owner

Choose a reason for hiding this comment

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

So I am worried that introducing a state inside the class will cause some issues in the future. How would you feel if we'd skip caching here and leave it to downstreams to handle?

Alternatively, we should think about how to properly handle caching inside this lib (if it's eveń wanted). Having some methods that take skip_cache is just wrong from design perspective :-/

assert isinstance(self._clean_history, CleaningSummary) # nosec assert_used
if not self._clean_history.ids:
return None

last_clean_id = history.ids.pop(0)
return self.clean_details(last_clean_id)
last_clean_id = self._clean_history.ids[0]
if last_clean_id == self._searched_clean_id:
return self._last_clean_details

self._last_clean_details = self.clean_details(last_clean_id)
return self._last_clean_details

@command()
def last_clean_all_floor(self, skip_cache=False) -> FloorCleanDetails:
"""Return details from the last cleaning and for each floor.

Returns None if there has been no cleanups for that floor.
"""
if self._clean_history is None or skip_cache:
self.clean_history()
assert isinstance(self._clean_history, CleaningSummary) # nosec assert_used
Copy link
Owner

Choose a reason for hiding this comment

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

Let's remove the caching from this PR and concentrate on getting the multi map handling implemented.


map_ids = self.get_maps().map_id_list

# if cache empty, fill with None
if not self._floor_clean_details:
for id in map_ids:
self._floor_clean_details[id] = None

if not self._clean_history.ids:
return FloorCleanDetails(self._floor_clean_details)

last_clean_id = self._clean_history.ids[0]
for id in self._clean_history.ids:
# already searched this record
if id == self._searched_clean_id:
break

clean_detail = self.clean_details(id)
if clean_detail.map_id in map_ids:
self._floor_clean_details[clean_detail.map_id] = clean_detail
map_ids.remove(clean_detail.map_id)

if id == last_clean_id:
self._last_clean_details = clean_detail

# all floors found
if not map_ids:
break

self._searched_clean_id = last_clean_id
return FloorCleanDetails(self._floor_clean_details)

@command(
click.argument("id_", type=int, metavar="ID"),
Expand All @@ -541,8 +596,7 @@ def clean_details(self, id_: int) -> Optional[CleaningDetails]:
_LOGGER.warning("No cleaning record found for id %s", id_)
return None

res = CleaningDetails(details.pop())
return res
return CleaningDetails(details.pop())

@command()
@action(name="Find robot", type="vacuum")
Expand Down
73 changes: 73 additions & 0 deletions miio/integrations/roborock/vacuum/vacuumcontainers.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from croniter import croniter
from pytz import BaseTzInfo

from miio.descriptors import SensorDescriptor
from miio.device import DeviceStatus
from miio.devicestatus import sensor, setting
from miio.interfaces.vacuuminterface import VacuumDeviceStatus, VacuumState
Expand Down Expand Up @@ -337,6 +338,16 @@ def current_map_id(self) -> int:
"""
return int((self.data["map_status"] + 1) / 4 - 1)

@property
def current_map_name(self) -> str:
Comment on lines +341 to +342
Copy link
Owner

Choose a reason for hiding this comment

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

Should this be exported as a sensor/setting?

"""The name of the current map with regards to the multi map feature."""
try:
map_list = self.map_list.map_list
except AttributeError:
return str(self.current_map_id)

return map_list[self.current_map_id]["name"]

@property
def in_zone_cleaning(self) -> bool:
"""Return True if the vacuum is in zone cleaning mode."""
Expand Down Expand Up @@ -586,6 +597,17 @@ def map_id(self) -> int:
"""Map id used (multi map feature) during the cleaning run."""
return self.data.get("map_flag", 0)

@property
@sensor("Last clean map name", icon="mdi:floor-plan", entity_category="diagnostic")
def map_name(self) -> str:
"""The name of the map used (multi map feature) during the cleaning run."""
try:
map_list = self._parent.map_list.map_list
Copy link
Owner

Choose a reason for hiding this comment

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

The need to access map_list twice like this feels like a code smell...

except AttributeError:
return str(self.map_id)

return map_list[self.map_id]["name"]

@property
def error_code(self) -> int:
"""Error code."""
Expand All @@ -605,6 +627,57 @@ def complete(self) -> bool:
return self.data["complete"] == 1


class FloorCleanDetails(DeviceStatus):
"""Contains details about a last cleaning run per floor."""

def __init__(self, data: Dict[int, Any]) -> None:
self.data = data

for map_id in self.data:
if self.data[map_id] is None:
setattr(self, f"CleanDetails_{map_id}", None)
setattr(self, f"start_{map_id}", None)
continue
setattr(self, f"CleanDetails_{map_id}", self.data[map_id])
setattr(self, f"start_{map_id}", self.data[map_id].start)
Comment on lines +636 to +642
Copy link
Owner

Choose a reason for hiding this comment

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

setattr, getattr etc. should be avoided wherever possible as that breaks static code analysis. It'd be better to avoid this, store the data as it's already done, and expose the necessary information through regular methods using that data.


def __repr__(self):
s = f"<{self.__class__.__name__}"
for map_id in self.data:
name = f"CleanDetails_{map_id}"
value = getattr(self, name)
s += f" {name}={value}"

name = f"start_{map_id}"
value = getattr(self, name)
s += f" {name}={value}"

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

s += ">"
return s
Comment on lines +644 to +659
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
def __repr__(self):
s = f"<{self.__class__.__name__}"
for map_id in self.data:
name = f"CleanDetails_{map_id}"
value = getattr(self, name)
s += f" {name}={value}"
name = f"start_{map_id}"
value = getattr(self, name)
s += f" {name}={value}"
for name, embedded in self._embedded.items():
s += f" {name}={repr(embedded)}"
s += ">"
return s

Let's remove this to keep it cleaner. As the repr is for developers, we can omit the details here (or better yet, find a way to expose them for all containers).


def sensors(self) -> Dict[str, SensorDescriptor]:
"""Return the dict of sensors exposed by the status container."""
self._sensors = {} # type: ignore[attr-defined]

for map_id in self.data:
self._sensors[f"start_{map_id}"] = SensorDescriptor(
id=f"FloorCleanDetails.start_{map_id}",
property=f"start_{map_id}",
name=f"Floor {map_id} clean start",
type=datetime,
extras={
"icon": "mdi:clock-time-twelve",
"device_class": "timestamp",
"entity_category": "diagnostic",
},
)

return self._sensors


class ConsumableStatus(DeviceStatus):
"""Container for consumable status information, including information about brushes
and duration until they should be changed. The methods returning time left are based
Expand Down