Skip to content

feat: Support both a01 and v1 device types with traits #425

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

Merged
merged 7 commits into from
Aug 9, 2025

Conversation

allenporter
Copy link
Contributor

No description provided.

@allenporter allenporter marked this pull request as draft August 7, 2025 14:53
Comment on lines +86 to +88
def traits(self) -> Mapping[str, Trait]:
"""Return the traits of the device."""
return MappingProxyType(self._trait_map)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why can't this just stay as a dict as is? I don't really know what MappingProxyType is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When this returns a dict its actually mutable so a caller could theoretically change it. It may be overkill here...

@allenporter allenporter marked this pull request as ready for review August 8, 2025 03:41
self._product_info = product_info
self._rpc_channel = rpc_channel

async def get_status(self) -> Status:
Copy link
Collaborator

Choose a reason for hiding this comment

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

So does each trait have its own unique functions? This may negate the need for the traits property, as iterating through them wouldn't be helpful as you need to do different things Per trait

Copy link
Collaborator

Choose a reason for hiding this comment

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

although I guess the traits() property just gives a mapping, so you can still do everything specifically there as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My assumption is traits will have multiple functions of related properties. However, maybe that is not the case, so i take your point that if every trait really is one thing then its just a grab bag of functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe i take your point as traits can be flattened properties on the device, which i agree with. I was assuming we'd go to something like this:

if device.status:
    print(await device.status.get_status())

Copy link
Collaborator

@Lash-L Lash-L Aug 9, 2025

Choose a reason for hiding this comment

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

Most of the time they come down to two things:

Get the latest version of myself

change myself or perform an action

sometimes there are multiple different ways you can make changes, but typically, there is just one way to update data and every single trait would need to have that.

I don't think there is a problem with:

tasks = [] if self.status: tasks.append(self.status.get_status())

but we will just have a long set of code opposed to:

await asyncio.gather(*trait.update() for trait in self.traits)

@allenporter allenporter merged commit f7d1a55 into Python-roborock:main Aug 9, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants