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

Add ecovacs component #15520

Merged
merged 19 commits into from Aug 20, 2018

Conversation

@OverloadUT
Contributor

OverloadUT commented Jul 17, 2018

Description:

Adds support for Ecovacs vacuums (DEEBOT vacuums) via an ecovacs component that creates vacuum entities for each vacuum in the user's account.

Supports pretty much the full vacuum feature-set, and the vacuums respond instantly to Hass service calls due to the always-open XMPP connection, which is pretty cool! It also supports showing the lifespan of the various replaceable components as attributes on the vacuum state.

Support for the reverse-engineered API is very early, and as such it's likely that not all vacuums and not all regions will work correctly on this first release. The Hass documentation will link to the sucks project for users to find more information about what vacuums and regions have been tested.

This PR has been tested with two DEEBOT M80 Pros on the same account.

Related issue (if applicable): n/a

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#5922

Example entry for configuration.yaml (if applicable):

ecovacs:
  username: !secret ecovacs_username
  password: !secret ecovacs_password
  country: us
  continent: na

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

OverloadUT added some commits Dec 9, 2017

All core features implemented
Getting fan speed and locating the vac are still unsupported until sucks adds support
Adding support for subscribing to events from the sucks library
This support does not exist in sucks yet; this commit serves as a sort of TDD approach of what such support COULD look like.
Fix unique ID
Never worked before, as it should have been looking for a key, not an attribute
from homeassistant.const import CONF_USERNAME, CONF_PASSWORD, \
EVENT_HOMEASSISTANT_STOP

REQUIREMENTS = ['sucks==0.9.0']

This comment has been minimized.

@balloob

def setup(hass, config):
"""Set up the Ecovacs component."""
_LOGGER.info("Creating new Ecovacs component")

This comment has been minimized.

@balloob

balloob Jul 18, 2018

Member

debug

"""Set up the Ecovacs component."""
_LOGGER.info("Creating new Ecovacs component")

if ECOVACS_DEVICES not in hass.data:

This comment has been minimized.

@balloob

balloob Jul 18, 2018

Member

Components are only setup once, no need for this if-statement.


if hass.data[ECOVACS_DEVICES]:
_LOGGER.debug("Starting vacuum components")
# discovery.load_platform(hass, "sensor", DOMAIN, {}, config)

This comment has been minimized.

@balloob

balloob Jul 18, 2018

Member

Comment?

CONF_COUNTRY = "country"
CONF_CONTINENT = "continent"

CONFIG_SCHEMA = vol.Schema({

This comment has been minimized.

@balloob

balloob Jul 18, 2018

Member

Any reason you went for configuration via configuration.yaml and not using our shiny new config entry?

This comment has been minimized.

@OverloadUT

OverloadUT Jul 18, 2018

Contributor

You know what, I created the bulk of this component 9 months ago and when I came back to it I didn't think to convert it. I'll read up on the config entry docs and see about converting it

This comment has been minimized.

@balloob

balloob Jul 18, 2018

Member

Hit me up on Discord, would love to help you with this. Hue is a good (although maybe a bit complicated) example.


ECOVACS_FAN_SPEED_LIST = ['normal', 'high']

# These consts represent bot statuses that can come from the `sucks` library

This comment has been minimized.

@balloob

balloob Jul 18, 2018

Member

Would we be able to get them from the sucks lib directly instead of defining them? sucks.STATUS_AUTO etc?

This comment has been minimized.

@OverloadUT

OverloadUT Jul 19, 2018

Contributor

In doing this, I actually decided to move the is_cleaning and is_charging logic in to the sucks lib which removed the need to define or import these at all!

STATUS_ERROR = 'error'

# Any status that represents active cleaning
STATUSES_CLEANING = [STATUS_AUTO, STATUS_EDGE, STATUS_SPOT, STATUS_SINGLE_ROOM]

This comment has been minimized.

@balloob

balloob Jul 18, 2018

Member

Make it a set for faster lookups 👍

_LOGGER.debug("Vacuum initialized: %s", self.name)

async def async_added_to_hass(self) -> None:
"""d."""

This comment has been minimized.

@balloob

balloob Jul 18, 2018

Member

?

async def async_added_to_hass(self) -> None:
"""d."""
# Fire off some queries to get initial state
self.device.statusEvents.subscribe(self.on_status)

This comment has been minimized.

@balloob

balloob Jul 18, 2018

Member

Since all you do is call schedule_update_ha_state, why not use lambdas?

self.device.statusEvents.subscribe(lambda _: self.schedule_update_ha_state())

This comment has been minimized.

@OverloadUT

OverloadUT Jul 18, 2018

Contributor

Yep, that's better. Each of those event handlers used to do more, but I moved the logic to the sucks library since their creation. I'll make this change.

self.device.run(Charge())

@property
def battery_icon(self):

This comment has been minimized.

@balloob

balloob Jul 18, 2018

Member

This seems a flaw in the vacuum ABC if you need to implement this yourself. Your implementation seems like how it should be?

This comment has been minimized.

@OverloadUT

OverloadUT Jul 18, 2018

Contributor

It definitely is an issue with the ABC having no formal support for determining if the bot is charging. The reason I chose not to fix the ABC here is because the proper fix is the rearchitecture discussed here: home-assistant/architecture#29

I plan on updating this component after/alongside @cnrd makes the foundational changes

# Convenient hack for debugging to pipe sucks logging to the Hass logger
if _LOGGER.getEffectiveLevel() <= logging.DEBUG:
import sucks
sucks.logging = _LOGGER

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jul 18, 2018

Member

It would be good if the sucks library could be updated to use a logger per module, as is recommended.

https://docs.python.org/3/library/logging.html#logger-objects

This comment has been minimized.

@OverloadUT

OverloadUT Jul 19, 2018

Contributor

Yep, you're right. Done.

ecovacs_api = EcoVacsAPI(ECOVACS_API_DEVICEID,
config[DOMAIN].get(CONF_USERNAME),
EcoVacsAPI.md5(config[DOMAIN].get(CONF_PASSWORD)),
config[DOMAIN].get(CONF_COUNTRY).lower(),

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jul 18, 2018

Member

Move the lowercase modification to the config schema and use vol.Lower().

@OverloadUT

This comment has been minimized.

Contributor

OverloadUT commented Jul 20, 2018

@andlo In the interest of not cluttering up the code review chatter here, let's take this to another thread. Could you either open a GHI where we can troubleshoot (include said logs, scrubbed of any tokens) or you can hit me up on Discord directly at OverloadUT#0001 so we can troubleshoot

DOMAIN: vol.Schema({
vol.Required(CONF_USERNAME): cv.string,
vol.Required(CONF_PASSWORD): cv.string,
vol.Required(CONF_COUNTRY): vol.All(vol.Lower, cv.string),

This comment was marked as resolved.

@MartinHjelmare

MartinHjelmare Jul 20, 2018

Member

Use vol.Lower(). Probably have to disable pylint warning.

This comment was marked as resolved.

@OverloadUT

OverloadUT Jul 20, 2018

Contributor

Changing vol.Lower to vol.Lower() doesn't work due to the missing parameter. The way I have it here works and seems to be the way it's used elsewhere. Can you clarify what change you're proposing here?

This comment was marked as resolved.

@MartinHjelmare

MartinHjelmare Jul 20, 2018

Member

Does it actually not work or is it just pylint complaining? Last time I read the voluptuous docs it's vol.Lower() that is correct.

This comment was marked as resolved.

@OverloadUT

OverloadUT Jul 20, 2018

Contributor

It throws an exception; not just a linting error
TypeError: Lower() missing 1 required positional argument: 'v'

This comment was marked as resolved.

@MartinHjelmare

MartinHjelmare Jul 20, 2018

Member

Ok, yeah, the docs support your experience. Either I remember incorrectly or that has changed. Thanks for sticking with me.

@frenck frenck added the docs-missing label Jul 21, 2018

@frenck

This comment has been minimized.

Member

frenck commented Jul 21, 2018

I was unable to spot a related documentation PR, hence, I've added the docs-missing label.


data[ATTR_ERROR] = self._error

for key, val in self.device.components.items():

This comment has been minimized.

@balloob

balloob Jul 23, 2018

Member

It's not clear to me what values are added to the state machine. Meaning it can't be documented nor can it be judged if they are appropriate to be stored as state attributes to begin with.

This comment has been minimized.

@OverloadUT

OverloadUT Jul 23, 2018

Contributor

Ah, these are attributes representing the lifespan of replaceable components on the vacuum. Today, they are hard coded in the lib to be main_brush, side_brush, and filter. (In Hass the attributes will be component_main_brush, etc.) The value is a 0-100 integer representing the percentage of lifespan remaining. I made it dynamic here because those three components may not be present on all vacuum models, so as we get feedback and make sucks more comprehensive for all vacuum models, this should appropriately populate based on what the vacuum actually has.

(I plan on documenting all of this in the docs once I implement the config entries)

@balloob

This comment has been minimized.

Member

balloob commented Jul 24, 2018

Looks good to me. Can be merged when the docs are done.

@OverloadUT

This comment has been minimized.

Contributor

OverloadUT commented Jul 30, 2018

Docs added, but now we're waiting on an upstream version bump due to a bug I found writing the docs :)

@cnrd

This comment has been minimized.

Contributor

cnrd commented Aug 1, 2018

@OverloadUT The STATE support has been merged here: 2ff5b4c but I'm hoping that I can also get the split of start / pause merged soon: #15751 not sure if you want to base your PR on those changes?

Revert to sucks 0.9.1 and include a fix for a bug in that release
Sucks is being slow to release currently, so doing this so we can get a version out the door.
@OverloadUT

This comment has been minimized.

Contributor

OverloadUT commented Aug 18, 2018

Okay, I just pushed up a very tiny change that fixes a bug in sucks 0.9.1 and bumps the requirement back down to that version.

I was originally waiting for 0.9.2 to be released, but the lib owner seems to currently be unavailable.

As soon as the documentation PR is reviewed, we should be good here: home-assistant/home-assistant.io#5922

( @cnrd in the interest of getting a version out so that we can start getting feedback on stability, I'm going to request this version be merged, but I'll work on a refactor to use your new system, if that's agreeable to everyone)

self.device.run(VacBotCommand(command, params))

@property
def state_attributes(self):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 18, 2018

Member

This should be device_state_attributes. These will be merged with state attributes.

This comment has been minimized.

@OverloadUT

OverloadUT Aug 18, 2018

Contributor

Thanks; I hadn't realized about that method!

@property
def state_attributes(self):
"""Return the state attributes of the vacuum cleaner."""
data = super().state_attributes

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 18, 2018

Member

This isn't needed.

@balloob balloob merged commit df6239e into home-assistant:dev Aug 20, 2018

4 of 5 checks passed

WIP work in progress
Details
Hound No violations found. Woof!
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.08%) to 93.747%
Details

@wafflebot wafflebot bot removed the in progress label Aug 20, 2018

@leofuscaldi

This comment has been minimized.

leofuscaldi commented Aug 23, 2018

After this, whats is the next step to have it working with HA?

@trisk

This comment has been minimized.

Contributor

trisk commented Aug 23, 2018

@leofuscaldi Install HA from the dev branch or wait for the next release (0.77).

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Aug 23, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.