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

Conversation

starkillerOG
Copy link
Contributor

Part 2 of adding multi maps handeling for Roborock vacuums.

  • Clean details per floor
  • better caching of clean details and history to minimize IO calls.
  • current map name
  • last clean details map name

@starkillerOG
Copy link
Contributor Author

@rytilahti this is the last piece of the Roborock s7 MaxV+ support.
Could you make a first review and profide some feedback?

I know you were not that happy at the FloorCleanDetails class beeing very technicall and difficult to read (because it is dynammically constructed), but I don't know how I could improve that withouth just hard coding the 4 floors that are currently supported as a max, but you initially also did not like that.

Also passing in the MapList class to other classes did not have your prefrence, It is needed for mapping the map_id to the user frendly map name. How would you suggest to do that diffrently?

@starkillerOG
Copy link
Contributor Author

@rytilahti could you review so we can get this merged?

@starkillerOG
Copy link
Contributor Author

@rytilahti would you have some time this week or next week to make an appointment on discord to revieuw this.
I would really like to get this finished.

Maybe we could also discuss releasing a new python-miio version?
It has been 5 months since the last release, I think we schould at least aim to have a monthly release of python-miio since there are so many (merged) PRs for it.
It is really starting to hold up development on the HomeAssistant side.

@rytilahti
Copy link
Owner

Hi @starkillerOG and happy belated christmas! Sorry I haven't answered to you, the holiday season was (and remains) busy, but I'll try to find some time to look into this come next week. We can also have a chat on discord if you want, just give me a ping some time next week whenever it suits you.

@starkillerOG
Copy link
Contributor Author

@rytilahti that would be awesom!
Happy new year

@starkillerOG
Copy link
Contributor Author

@rytilahti I have updated this PR to use the new UpdateHelper, have simplified a few things and are not passing around the maplist anymore but use the _parent refrence.

Could you review this?

@starkillerOG
Copy link
Contributor Author

All tests (except for codecov) are now passing

@starkillerOG
Copy link
Contributor Author

@rytilahti could you please have a look?

Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

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

Ah, looks like commenting on this slipped through the cracks, sorry @starkillerOG and thanks for the reminder!

I added a couple of comments inline, but the general feeling that I have is that merging this as-is could introduce potentially hard to track down bugs, especially related to the use of setattr and the changes to caching mechanisms.

My proposal is following:

  1. Let's remove, caching set/getattr tricks etc. to keep this PR as simple as possible. A single I/O request or a few is not a big deal in the big picture, and we can always come back to that after we have a clean, working solution.
  2. We clean up the vacuumcontainers by moving the code into feature-specific files (i.e., by the command that is used to get the data). This will make it not only easier to maintain but easier to add new features.
  3. We find a way to introduce caching, if it still makes sense.

As I'm not seeing any tests or payloads for the new information here, I have no idea about what would be the best way forward on this. When/if you find time, ping me on discord and let's have a synchronous chat/call to figure this all out :-)

Comment on lines +341 to +342
@property
def current_map_name(self) -> str:
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?

Comment on lines +644 to +659
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
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).

Comment on lines +534 to +535
if self._clean_history is None or skip_cache:
self.clean_history()
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 :-/

Comment on lines +636 to +642
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)
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 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...

"""
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants