-
Notifications
You must be signed in to change notification settings - Fork 51
feat: add dynamic changing status #511
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
from typing import Self | ||
|
||
from roborock.containers import HomeDataProduct, ModelStatus, S7MaxVStatus, Status | ||
from roborock.containers import HomeDataProduct, Status, get_custom_status | ||
from roborock.devices.traits.v1 import common | ||
from roborock.roborock_typing import RoborockCommand | ||
|
||
|
@@ -13,12 +13,12 @@ class StatusTrait(Status, common.V1TraitMixin): | |
def __init__(self, product_info: HomeDataProduct) -> None: | ||
"""Initialize the StatusTrait.""" | ||
self._product_info = product_info | ||
self._status_type = get_custom_status(self.device_info.device_features, self.device_info.region) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is where it gets weird - To determine the status type, we need the device features, which we get from init status, which we need to be connected for. Hoping you may have some ideas here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can device features be provided similar to how HomeDataProduct or network info are supplied? Do it up front, and we provide an interface for caching it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's what i thought about doing but I don't know if it is possible. The Trait objects are created synchronously before we are subscribed and we could send a message to get the init status. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe making a separate trait for now can work, then once its loaded it can be used? This could override Separately: Is making it part of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not explicitly, no, but i figured it would make sense. but open to doing it differently. I could make a device features trait that gets the init status/whatever else it needs and other things rely on it? As we will need it to determine what is supported when we start doing more complicated traits? Thoughts on that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems worth a try? That way we can get it working then next figure out how to best use it? this thing of combining traits together with dependencies is kinda weird but kind of simple too but not sure if it will bite us later, but may be an easy way to string things together. |
||
|
||
def _parse_response(self, response: common.V1ResponseData) -> Self: | ||
"""Parse the response from the device into a CleanSummary.""" | ||
status_type: type[Status] = ModelStatus.get(self._product_info.model, S7MaxVStatus) | ||
if isinstance(response, list): | ||
response = response[0] | ||
if isinstance(response, dict): | ||
return status_type.from_dict(response) | ||
return self._status_type.from_dict(response) | ||
raise ValueError(f"Unexpected status format: {response!r}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to be a protocol or something? Was hoping you may have some ideas on how to improve it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see why this is set up this way.
Does it make sense for DeviceStatus to be a top level object with a DeviceFeatures + region as input to constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the issue is right now, every instance of status that we do we use from_dict()
So we'd need to add it to from_dict() and constructor. Theoretically fine, but potentially bloats from_dict() calls for status so i wasn't sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems similar to the traits that need to parse room names. We basically have a hacky dance to make this work:
Perhaps this could just be added to
StatusTrait
?