wpietri / sucks

Simple command-line script for the Ecovacs series of robot vacuums
GNU General Public License v3.0
281 stars 104 forks source link

License needed, and any chance for a PyPi library? #3

Closed OverloadUT closed 6 years ago

OverloadUT commented 6 years ago

Hey there!

I am planning on adding support for Deebot to Home Assistant. Your code here will get me most of the way!

However, you currently don't have a LICENSE file in the project that I could find. Could you add a license so I know if I could use this?

Second, do you plan on turning this in to a library and publishing to PyPi? I will do it if you aren't planning on it, which brings up the subquestion: would you want that as PRs to this repo, or should a library version be an entirely different project that's just based on the code discoveries here?

wpietri commented 6 years ago

Hi! My philosophy is not to code in advance of actual need, so at this point the code is just what I needed. But I'd be happy to both license it and make it a library. What license did you have in mind? I default to GPL, but am open to others.

Let's talk a bit more about what kind of API you'd need for your work. I got where I did just based on my needs, but I don't like to lock in an API until a few different use cases have been covered.

What are the actual user-facing use cases for you? What about the current API suits you? What would you change?

OverloadUT commented 6 years ago

I wouldn't be picky on license - for Home Assistant's purposes, the library would be loaded like any other PyPi library so something like the GPL would be fine. (It doesn't bundle/modify the code or anything like that.)

The user-facing use-cases are:

  1. Being able to see (both live and historically) the current state of the vacuum and its battery and accessory lifespans
  2. Being able to trigger commands like clean, return to home, pause, etc. The "turn left" type commands would not be so useful and would probably not be implemented.
  3. Allowing the user to set up automatons that combine 1 and 2 with all other HA devices in the Home Assistant ecosystem ("when the TV is on, pause vacuuming". "when max-clean mode is enabled, start an auto clean as soon as Deebot battery is full". "When all residents leave the house, begin vacuuming"). This we get for free with 1 and 2, but it's worth mentioning the whole point of the Home Assistant component I'd be writing! :)

One thing I would change in the API from the way it is currently is to have it be a persistent connection to the vacuum, so the state changes can be reported instantaneously rather than relying on polling.

(Edit: Oh, and of course the fact that it would be a library for accessing from code rather than a CLI. That's the fundamental difference we'd need to make it work!)

This docs page might be interesting to understand what the vacuum parent component is currently capable of (specific deebot features would be implemented in the Deebot-specific component): https://home-assistant.io/components/vacuum/

Zenconomy commented 6 years ago

Awesome! I was also looking into integration with Home Assistant. Looks like making it a platform would be the way to go.

wpietri commented 6 years ago

Very interesting. Thanks!

One thing that writing this code for a CLI let me dodge was issues of sync vs async. The XMPP stuff itself is async, but I mostly ignored that. For the live part of the first use case, some rework will be required. I've done a fair bit of async work in other languages, but very little in Python, so I'm open to suggestion there. And it may be worth looking at other XMPP libraries; I just picked the first one that happened to work.

I also have questions about what will happen with long-lived connections. The XMPP stuff itself, of course. But the auth tokens from the HTTPS API appear to expire, and it's possible that their expiration is triggered by unobservable events. (E.g., opening up the official app on one's phone may expire the token in use by the HA server.) So there's probably some work there.

But it sounds like the first step is for me to split this out into a library and release it as something like v0.8.2. Then you can try it out properly in your context and we can rev the API as needed, shooting for an eventual 1.0 release that's suitable for proper integration. Sound good?

Also, I've released exactly one Python library before, so if you could take a look at that setup.py and point out any issues, I'd appreciate it.

OverloadUT commented 6 years ago

That sounds great to me!

I should say: the persistent connection thought is a wishlist item for sure. It's definitely preferred, but tons of components in Home Assistant do polling instead, either because a live connection is not supported, or because it hasn't been worth dealing with the complexity of all the things you mentioned.

So, it would be totally fine to start with a polling implementation, and perhaps work towards a persistent connection option as questions about auth/etc are figured out.

wpietri commented 6 years ago

Fab. I have pushed something that might possibly be useful. I believe you can try this out with pip install -e .; any feedback is welcome.

OverloadUT commented 6 years ago

Getting set up to test an implementation now!

First feedback: looks like pipenv is missing from the dependencies: ImportError: No module named 'pipenv'

It installed fine after I manually installed pipenv

wpietri commented 6 years ago

Yeah, there's a question for me about where the authoritative list of requirements should go. Pipenv believes it belongs in the Pipfile. But pip would like it in setup.py, or at least loaded from there. Maybe I should just put it in all in setup.py and get rid of the Pipfile now that we're moving to a library.

OverloadUT commented 6 years ago

Good news!

I've got Home Assistant working with sucks!

2017-12-07 16:46:34 INFO (Thread-10) [homeassistant.components.ecovacs] Creating new Ecovacs component
2017-12-07 16:46:37 DEBUG (Thread-10) [homeassistant.components.ecovacs] Setting up Ecovacs device: Bulbasaur
2017-12-07 16:46:40 DEBUG (Thread-10) [homeassistant.components.ecovacs] Vacuum initialized. Clean Status: None, Charging: None
2017-12-07 16:46:40 DEBUG (Thread-10) [homeassistant.components.ecovacs] Setting up Ecovacs device: Squirtle
2017-12-07 16:46:43 DEBUG (Thread-10) [homeassistant.components.ecovacs] Vacuum initialized. Clean Status: None, Charging: None

It's just the core initialization code right now. Without event handling or observer support for the VacBot state properties there's not much I can do to implement the actual devices quite yet. But the point is it is working!

wpietri commented 6 years ago

Fab. In that case it sounds like we can close this issue, in that we have both a license and a library. Perhaps we can move discussion of the broader HomeAssistant effort to the [mailing list])(https://groups.google.com/forum/#!forum/sucks-users)?