Refactor Mqtt Manager and Roborock Device #364

Lash-L · 2025-04-09T14:04:29Z

No description provided.

roborock/mqtt_manager.py

…efactor

allenporter · 2025-04-12T14:46:54Z

roborock/device_trait.py

+from .containers import Consumable, DeviceFeatures, DnDTimer, RoborockBase
+
+
+@dataclass


The general shape looks good with the base class that supports listeners and handling updates.

I'm wondering if we can merge the trait and the dataclass objects. The API right now is like this:

if device.dnd: print(device.dnd.status.enabled)

But perhaps that extra level of status is worth trying to avoid? Then it could look like this:

if device.dnd: print(device.dnd.enabled)

The down side of this is that the dataclass has to become mutable, and we will likely want more subclassing (e.g. There is already RoborockBase for handling parsing of objects, but should it also have listeners? or make another class hierarchy? just want to avoid multiple paths to inherit RoborockBase).

So I just pushed a change that attempts to do this. It's a bit weird with the to and from dict logic. I started wondering though if the logic we use right now is overkill. We do all sorts of camelization and decamelization, but maybe it is better to just be explicit. It is less 'engineered'/ less fancy. But it is probably less prone to errors? I didn't touch the support features stuff you talked about. But want to take a look at that in the diff and see what you think about it/ if you have other feedback? We'd basically remove containers for anything that is a device trait.

I think we still can find ways to simplify this, but not yet seeing it. Maybe it's but cutting some of the requirements for now that are complex, then adding them in a follow up? The challenge is to have a nice API you like, then figure out how to add workarounds to add in extra constraints w/o uglifying the API.

Can we cut requirements for now that every trait has its own subscriber? I feel like that adds a little complexity i think we can work up to.

How about reducing the number of states a trait can be in? Right now we have these states to consider:

Can be None (not supported)

Can be present, but all fields are None (supported but not populated yet yet)

Can be present with a status (support but failed population on first message?)

Can be present with some fields None and some not None (does it happen? maybe depends on device? unclear to caller)

I think we need to remove 2 of these if we can... e.g. present with all None fields seems like it could be nice to avoid if its not meaningful.

Parsing wise, it feels like Dnd should be a @dataclass given all that initialization. Thee are also parsing libraries we can use e.g. mashumaro

Will responding to the rest when I'm at my machine. As far as serialization goes. We used dacite in the past and did camelization of atttibutes as sometimes they would randomly throw in a snake case attribute. This added some complexity, and when I removed the dacite dependency, I just wrote my own serializer as there were a few things that were happening that didn't work well with the libraries I tried(including madhumaru). Perhaps while we are already making a breaking change, we can switch to a library that may handle things slightly differently.

Maybe:

The "update" method can actually be a class method and return a instance of the trait.

Before we get any data it is None.

So long as it is supported, when we get data we update it.

self.dnd = DndTrait.update(data)

Not sure if that potentially opens the door to async problems.

Thoughts @allenporter

Although it gets a bit trickier with something like Consumable. Where different devices have different attributes on them. and therefore have to have some None attributes

allenporter · 2025-04-12T15:24:31Z

roborock/device_trait.py

+        super().__init__(send_command)
+
+    @classmethod
+    def supported(cls, features: DeviceFeatures) -> bool:


Right now the logic for this mapping is spread across a few places, the device features object and the trait itself, but wondering if it can live in a single place.

I think it either needs to be:

in the device features code, directly point to the trait, then look up the reference when building the device. i'm feeling like the first option may be simplest, though it flips around the dependencies it may have to move if going that way.

Define an enums the trait can reference then the parsing code can look it up in the right place. This may be a challenge since there are multiple ways to determine if something is supported.

push the "is supported" logic only in the trait (and figure out the right apis to pass the needed data through when building the device). This may be a challenge since there are multiple ways to determine if something is supported.

roborock/roborock_message.py

roborock/roborock_device.py

allenporter · 2025-06-04T03:59:27Z

roborock/roborock_device.py

+            return trait_instance
+        return None
+
+    async def connect(self):


I know you have a TODO above, but i think it would be best to just now pass in a connected session and not add this method, or add a TODO remove here i guess.

allenporter · 2025-06-04T04:00:30Z

roborock/roborock_device.py

+
+    def determine_supported_traits(self, trait: type[DeviceTrait]):
+        def _send_command(
+            method: RoborockCommand | str, params: list | dict | int | None = None, use_cloud: bool = True


Can we omit use_cloud if we're not using it yet?

roborock/code_mappings.py

allenporter · 2025-06-04T04:24:51Z

roborock/containers.py

+    """Represents the features supported by a Roborock device."""
+
+    # Features derived from robot_new_features
+    is_map_carpet_add_support: bool


We can add the details about the bits into the metadata for the field itself like this:

from dataclasses import dataclass, field from typing import Any @dataclass class Example: is_map_carpet_add_support: bool = field(metadata={'feature': '1073741824'})

You can use fields() to iterate over all the fields and access the .metadata to get the dict. The field could be named different things for different feature types e.g. ("upper_32_bits": or "feature_bit_str":) and change the logic.

oh that's a cool idea. i like it

roborock/device_trait.py

allenporter · 2025-06-04T06:14:38Z

roborock/device_trait.py

+from .containers import Consumable, DeviceFeatures, DnDTimer, RoborockBase
+
+
+@dataclass


I think we still can find ways to simplify this, but not yet seeing it. Maybe it's but cutting some of the requirements for now that are complex, then adding them in a follow up? The challenge is to have a nice API you like, then figure out how to add workarounds to add in extra constraints w/o uglifying the API.

Can we cut requirements for now that every trait has its own subscriber? I feel like that adds a little complexity i think we can work up to.

How about reducing the number of states a trait can be in? Right now we have these states to consider:

Can be None (not supported)

Can be present, but all fields are None (supported but not populated yet yet)

Can be present with a status (support but failed population on first message?)

Can be present with some fields None and some not None (does it happen? maybe depends on device? unclear to caller)

I think we need to remove 2 of these if we can... e.g. present with all None fields seems like it could be nice to avoid if its not meaningful.

Parsing wise, it feels like Dnd should be a @dataclass given all that initialization. Thee are also parsing libraries we can use e.g. mashumaro

Lash-L added 2 commits April 9, 2025 10:03

chore: init commit

a53e73c

chore: init commit

10d6531

allenporter requested changes Apr 9, 2025

View reviewed changes

roborock/mqtt_manager.py Outdated Show resolved Hide resolved

roborock/mqtt_manager.py Outdated Show resolved Hide resolved

Lash-L added 3 commits April 10, 2025 09:46

feat: add device_features to automatically determine what is supported.

9b41365

Merge branch 'refs/heads/roborock_supported_features_auto' into big_r…

518ffca

…efactor

chore: some misc changes

f480b51

allenporter reviewed Apr 12, 2025

View reviewed changes

allenporter mentioned this pull request Apr 12, 2025

Add an aiomqtt based MQTT session module #366

Merged

Lash-L added 5 commits April 17, 2025 22:13

Merge remote-tracking branch 'origin/main' into big_refactor

e3a31cf

chore: random changes

51ad244

Merge branch 'refs/heads/main' into big_refactor

d830aa5

chore: make things functional again

b214d5b

chore: change a bit of how traits work

fbf859b

allenporter requested changes Jun 4, 2025

View reviewed changes

chore: some PR updates

d98b72e

		from .containers import Consumable, DeviceFeatures, DnDTimer, RoborockBase


		@dataclass

Refactor Mqtt Manager and Roborock Device #364

Are you sure you want to change the base?

Refactor Mqtt Manager and Roborock Device #364

Uh oh!

Conversation

Lash-L commented Apr 9, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lash-L Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Lash-L Jun 4, 2025 •

edited

Loading