Skip to content

WIP: General Vacuum Interface (#1059)#1060

Closed
stek29 wants to merge 1 commit intorytilahti:masterfrom
stek29:vacuum-interface
Closed

WIP: General Vacuum Interface (#1059)#1060
stek29 wants to merge 1 commit intorytilahti:masterfrom
stek29:vacuum-interface

Conversation

@stek29
Copy link
Copy Markdown

@stek29 stek29 commented May 31, 2021

WIP: suggestions and corrections are welcome :)

todo:

  • Roborock implementation
  • Consumables (+ resetting)
  • Mopping/Water
  • Feature flags
  • Clean History
  • Execute Custom Action (device specific)

Would be nice to add:

  • Stats
  • RC/Manual control
  • DND/Timers
  • Audio Control (Volume, Voice Packs)
  • Viomi Implementation

@stek29
Copy link
Copy Markdown
Author

stek29 commented May 31, 2021

@rytilahti there's also another option – using universal MixIns, kinda like traits in Google Assistant/Home

That'd be more universal: there would be "MixIn"s for battery status, start/stop, pause/resume, setting fan speed, which can be reused for other kinds of devices
If designed corretly, mixins might also cover feature detection/flags

but I'm not quite sure if it's worth the effort for now

@stek29
Copy link
Copy Markdown
Author

stek29 commented May 31, 2021

Valetudo also has capabilities based design, see https://valetudo.cloud/pages/general/capabilities-overview.html

@stek29
Copy link
Copy Markdown
Author

stek29 commented Jun 2, 2021

@rytilahti any thoughts?

@rytilahti
Copy link
Copy Markdown
Owner

Hey @stek29 and thanks for the PR! I'm currently very busy with other things and cannot currently do a full review, but did you take a look at my comment related to the directory structure? IMO the base classes should be inside the main package, and the implementations moved out to their own directories.

On the mixins, I feel that they are not very pythonic, and will likely just cause more confusion than they are worth. For example: if you are implementing support for new device, you either must search for the wanted mixins (or sadly, simply copy&paste them over from something else). So I think it's better to have explicit interfaces (e.g., using abstract classes) that are directly useful if you are using a proper IDE for development.

When it will get quieter at some point late next week, I'll write some more comments!

@starkillerOG
Copy link
Copy Markdown
Contributor

@stek29 I have also been thinking about how to generalize the interface of python-miio, see #929 (comment).

In my opinion there schould be one common .update() function that all integrations use to update all available data.
The .update() function can be defined in device.py and overwritten by each device class to implement there own specific update calls.
The resulting data also needs to be stored in a common fasion, preferably there would be a .status() function (again in the device.py) that is overwritten by each implementation that returns all available data.

I think most of the current implementations use info classes to represent the data, so the .status() could return that info class. preferably there would also be a .status_dict() function that would return the status as a dictionarry.

In that way implementations as HomeAssistant can use common functions to extract common device status attributes like "temperature".

I already did this for the subdevices of the xiaomi gateway where I define the available properties in a .yaml file:
https://github.com/rytilahti/python-miio/blob/master/miio/gateway/devices/subdevices.yaml

HomeAssistant uses the common attributes to easily setup sensor entities:
https://github.com/home-assistant/core/blob/12f2482c9bb062f347cd1e80a68e9d0d469f897a/homeassistant/components/xiaomi_miio/sensor.py#L76-L98
https://github.com/home-assistant/core/blob/12f2482c9bb062f347cd1e80a68e9d0d469f897a/homeassistant/components/xiaomi_miio/sensor.py#L137-L147

@rytilahti
Copy link
Copy Markdown
Owner

Hey @starkillerOG, in most of the cases there exists a status() that is used to report the device state, so in my mind that is sort of already a part of the common device API even when it is not defined in the device class. I think this should change though, and the base device API could at least define that (and maybe even on() and off() for basic controls?).

Now, on how to expose the separate sensors is an open question, but we should find a way to do that. Introducing a separate decorator (instead of using pure @property) to allow defining the sensory properties would make sense. The base class can implement a way to get a list of these (as well as implement as_dict() for accessing all properties), and downstreams like homeassistant can simply iterate over them when creating and updating the sensor states. This will probably need some experimentation to get a feeling what makes most sense and feels natural...

See also my (rather preliminary) comment about the general (re)structure here #1059 (comment) . It's a quite a bit of work to do such restructuring without breaking existing use cases, but it will hopefully be worth it at the end.

This should also make it simpler to add support for generic device-type specific miot devices (@ha0y has done great work on creating a custom HA component, but not everyone uses HA, this comment from him/her is relevant on the topic #1029 (comment)). While I'm reluctant to add any sort of cloud requirement to this library, we could still provide a way to use already downloaded miot spec files for generic device support.

@stek29
Copy link
Copy Markdown
Author

stek29 commented Aug 30, 2021

sadly I'm no longer interested in this since valetudo is now available for Dreame robots

@stek29 stek29 closed this Aug 30, 2021
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.

3 participants