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
Add ecovacs component #15520
Conversation
Getting fan speed and locating the vac are still unsupported until sucks adds support
This support does not exist in sucks yet; this commit serves as a sort of TDD approach of what such support COULD look like.
Never worked before, as it should have been looking for a key, not an attribute
homeassistant/components/ecovacs.py
Outdated
from homeassistant.const import CONF_USERNAME, CONF_PASSWORD, \ | ||
EVENT_HOMEASSISTANT_STOP | ||
|
||
REQUIREMENTS = ['sucks==0.9.0'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤣
homeassistant/components/ecovacs.py
Outdated
|
||
def setup(hass, config): | ||
"""Set up the Ecovacs component.""" | ||
_LOGGER.info("Creating new Ecovacs component") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debug
homeassistant/components/ecovacs.py
Outdated
"""Set up the Ecovacs component.""" | ||
_LOGGER.info("Creating new Ecovacs component") | ||
|
||
if ECOVACS_DEVICES not in hass.data: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Components are only setup once, no need for this if-statement.
homeassistant/components/ecovacs.py
Outdated
|
||
if hass.data[ECOVACS_DEVICES]: | ||
_LOGGER.debug("Starting vacuum components") | ||
# discovery.load_platform(hass, "sensor", DOMAIN, {}, config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment?
CONF_COUNTRY = "country" | ||
CONF_CONTINENT = "continent" | ||
|
||
CONFIG_SCHEMA = vol.Schema({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason you went for configuration via configuration.yaml and not using our shiny new config entry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we be able to get them from the sucks lib directly instead of defining them? sucks.STATUS_AUTO
etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make it a set for faster lookups 👍
_LOGGER.debug("Vacuum initialized: %s", self.name) | ||
|
||
async def async_added_to_hass(self) -> None: | ||
"""d.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
async def async_added_to_hass(self) -> None: | ||
"""d.""" | ||
# Fire off some queries to get initial state | ||
self.device.statusEvents.subscribe(self.on_status) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since all you do is call schedule_update_ha_state
, why not use lambdas?
self.device.statusEvents.subscribe(lambda _: self.schedule_update_ha_state())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a flaw in the vacuum ABC if you need to implement this yourself. Your implementation seems like how it should be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
homeassistant/components/ecovacs.py
Outdated
# Convenient hack for debugging to pipe sucks logging to the Hass logger | ||
if _LOGGER.getEffectiveLevel() <= logging.DEBUG: | ||
import sucks | ||
sucks.logging = _LOGGER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, you're right. Done.
homeassistant/components/ecovacs.py
Outdated
ecovacs_api = EcoVacsAPI(ECOVACS_API_DEVICEID, | ||
config[DOMAIN].get(CONF_USERNAME), | ||
EcoVacsAPI.md5(config[DOMAIN].get(CONF_PASSWORD)), | ||
config[DOMAIN].get(CONF_COUNTRY).lower(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move the lowercase modification to the config schema and use vol.Lower()
.
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.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
I was unable to spot a related documentation PR, hence, I've added the |
|
||
data[ATTR_ERROR] = self._error | ||
|
||
for key, val in self.device.components.items(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Looks good to me. Can be merged when the docs are done. |
Docs added, but now we're waiting on an upstream version bump due to a bug I found writing the docs :) |
@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? |
Sucks is being slow to release currently, so doing this so we can get a version out the door.
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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be device_state_attributes
. These will be merged with state attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't needed.
After this, whats is the next step to have it working with HA? |
@leofuscaldi Install HA from the dev branch or wait for the next release (0.77). |
Description:
Adds support for Ecovacs vacuums (DEEBOT vacuums) via an
ecovacs
component that createsvacuum
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):Checklist:
tox
. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
REQUIREMENTS
variable (example).requirements_all.txt
by runningscript/gen_requirements_all.py
..coveragerc
.