Basic MQTT vacuum support#9386
Conversation
|
Hi @johnboiles, It seems you haven't yet signed a CLA. Please do so here. Once you do that we will be able to review and accept this pull request. Thanks! |
There was a problem hiding this comment.
line too long (83 > 79 characters)
There was a problem hiding this comment.
line too long (92 > 79 characters)
There was a problem hiding this comment.
line too long (114 > 79 characters)
There was a problem hiding this comment.
line too long (83 > 79 characters)
There was a problem hiding this comment.
line too long (117 > 79 characters)
There was a problem hiding this comment.
line too long (103 > 79 characters)
There was a problem hiding this comment.
line too long (102 > 79 characters)
There was a problem hiding this comment.
line too long (83 > 79 characters)
There was a problem hiding this comment.
line too long (111 > 79 characters)
There was a problem hiding this comment.
line too long (83 > 79 characters)
|
That is cool 👍 link your firmware to documentation |
|
Thanks @pvizeli. Planning to write some docs on that tonight. Starting with docs here to get the home assistant process rolling |
There was a problem hiding this comment.
Please use the new style self.hass.components.mqtt.async_....
There was a problem hiding this comment.
Please use last change: self.async_schedule_...
There was a problem hiding this comment.
I think that need to be more generic like other mqtt component. Maybe wuth templates (that support also json) and variable topics for that points. In yiur case you can set all the sane topic and use the tempkate to parse the correct value on it.
There was a problem hiding this comment.
Ok sure, will do
There was a problem hiding this comment.
Only allow the methods in SERVICE_TO_STRING:
vol.All(cv.ensure_list, vol.In(SERVICE_TO_STRING.keys())),There was a problem hiding this comment.
Please sort the imported names alphabetically 🔡 and break the line with parenthesis.
There was a problem hiding this comment.
Is there a doc somewhere about code-style things that aren't otherwise enforced by flake8/pylint/pydocstyle?
There was a problem hiding this comment.
Here are our recommendations: https://home-assistant.io/developers/development_guidelines/. Neither of my comments here are mentioned there though.
There was a problem hiding this comment.
Use the async version async_setup_platform. See eg https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/switch/mqtt.py.
There was a problem hiding this comment.
Will do. Thanks for the link!
There was a problem hiding this comment.
https://home-assistant.io/components/vacuum.mqtt/
There was a problem hiding this comment.
No need to overwrite this property if you don't return a dict with contents.
There was a problem hiding this comment.
Ah cool. I'll have to do more reading about what sort of things typically go in device_state_attributes to see if there's anything meaningful I could be returning.
There was a problem hiding this comment.
Change to the async version, and annotate the method with @asyncio.coroutine. See eg https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/switch/mqtt.py#L165 and https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/vacuum/xiaomi.py#L231.
This applies to all methods below.
There was a problem hiding this comment.
Will do, thanks for the links!
There was a problem hiding this comment.
I noticed the superclass VacuumDevice, has code like this
def stop(self, **kwargs):
"""Stop the vacuum cleaner."""
raise NotImplementedError()
def async_stop(self, **kwargs):
"""Stop the vacuum cleaner.
This method must be run in the event loop and returns a coroutine.
"""
return self.hass.async_add_job(partial(self.stop, **kwargs))Does it make sense to update the superclass code to remove the non-async version?
There was a problem hiding this comment.
No, each platform will decide to use either the sync or the async version of the method, so one of them will be overwritten. The service handler in the base component will always use the async version.
There was a problem hiding this comment.
Use the new async version that @pvizeli mentioned: self.async_schedule_update_ha_state()
This applies to all methods below.
There was a problem hiding this comment.
Other mqtt platforms have configurable payloads. Maybe we want that here too, continuing @pvizeli's suggestion on generalization.
There was a problem hiding this comment.
Totally open to that. It will add a ton of configuration options which might look intimidating to newer users. But I think that's probably an ok tradeoff
johnboiles
force-pushed
the
mqtt-vacuum
branch
from
dc7e423 to
43dc1a6
Compare
| payload, | ||
| error_value=None) | ||
| if state: | ||
| if state == 'cleaning': |
There was a problem hiding this comment.
I'm open to ideas on how to handle the state enum in a generic way as well.
There was a problem hiding this comment.
I wonder if it'd be better to just handle state as a set of bools potentially with their own topics/templates, then determine a state on the home assistant side.
| if state: | ||
| if state == 'cleaning': | ||
| self._is_cleaning = True | ||
| self._status = "Cleaning" |
There was a problem hiding this comment.
Is there already a strategy for localization that's used in Home Assistant?
pvizeli
left a comment
pvizeli
left a comment
There was a problem hiding this comment.
Looks good. Some small things:
- Please revert change on demo. We run that thread for a better coverage of sync function
- Don't add a lot of defaults to the schema. Look to light/mqtt.py - It should work in same way
- Move the change from vacuum/init into your platform if you realy need this
Address this 3 Points and we are fine to merge it 👍 good work
Done. I don't feel strongly but I think it'd be nice if the
Done; moved into
For this component, I think opinionated defaults make sense for a two reasons:
|
MartinHjelmare
left a comment
MartinHjelmare
left a comment
There was a problem hiding this comment.
Noting possible improvements (for the future).
| payload, | ||
| error_value=None) | ||
| if battery_level is not None: | ||
| self._battery_level = int(battery_level) |
There was a problem hiding this comment.
A possible improvement (for the future) would be to use a nested dict to lookup the template, function for modifying the value, and value attribute name, from the topic. Then all the topic checks could be replaced with one code block.
Also I think you could just have the whole config as an instance attribute and use the string constants to lookup what you need in the config.
There was a problem hiding this comment.
That's a good idea
| mqtt.async_publish(self.hass, self._command_topic, | ||
| self._payload_turn_on, self._qos, self._retain) | ||
| self._status = 'Cleaning' | ||
| self.async_schedule_update_ha_state() |
There was a problem hiding this comment.
Since all of the methods have a very similar structure, an improvement (for the future) would be to add a helper method, that takes the support, payload and status as arguments. Then use the helper method in all these methods.
|
We can allow to discovery this platform. So you can make a new platform for your firmware that discovery the mqtt platform with correct settings... Like a lot camera platform does with mjepeg platform. Or we provide a ready to use copy past config inside documentation for your firmware 👍 |
|
Good work 👍 I will try out with my roomba |
| DEFAULT_PAYLOAD_CLEAN_SPOT = 'clean_spot' | ||
| DEFAULT_PAYLOAD_LOCATE = 'locate' | ||
| DEFAULT_PAYLOAD_START_PAUSE = 'start_pause' | ||
| DEFAULT_BATTERY_LEVEL_TOPIC = 'vacuum/state' |
There was a problem hiding this comment.
Removing sane defaults is a bummer to me. Every home assistant user that wants to use this component will now need to add an at least ~10 line config block to configuration.yaml. I bet this configuration will be exactly the same (copied and pasted from the github.io docs example) for 80% of people.
There was a problem hiding this comment.
@MartinHjelmare what do you think? If you both agree then I'll drop the issue.
Seems to me that managing configuration.yaml config is already really hard for Home Assistant users. I'm in favor of whatever makes it easier/simpler.
| CONF_NAME: 'mqtttest', | ||
| ATTR_SUPPORTED_FEATURES: services_to_strings(ALL_SERVICES), | ||
| } | ||
| vacuum.DOMAIN: self.default_config.update({ |
There was a problem hiding this comment.
Neat, great to have these shared defaults from self.default_config. Thanks for the cleanup
|
@pvizeli @MartinHjelmare see my comment on https://github.com/home-assistant/home-assistant/pull/9386/files/fb24fdf19af0b49328e8daa656a63f3fcd398cda..60d9f71386aa597e4ed39b5a86022f7e66c6d412#r139182488 and here about removing the defaults. It's a bummer to me that users will now need a 10+ line config block in configuration.yaml for settings that will probably be exactly the same for most users (likely copied and pasted from the example in the home assistant docs). Removing defaults from the code makes it harder for users to get started with this platform, and potentially increases the likelihood of causing breaking changes for users (e.g. if we need to change the config schema for some reason and users have the default config copied and pasted into their configuration.yaml). That's the only thing I disagree with from @pvizeli's changes. Otherwise thanks for the cleanup @pvizeli! I'm excited to get this out there. I'll update the docs PR tonight |
|
Less defaults is in line with the other mqtt platforms. Hopefully it's just a one time setup of that part of the config, so I think it's OK. |
|
Ok cool thanks for chiming in @MartinHjelmare! I'll update the docs PR to reflect @pvizeli's changes tonight! |
ghost
added
6 participants
Basic MQTT vacuum support
Useful for retrofitting older Roombas and other non-wifi vacuums to be controlled via Home Assistant.
See also my WIP ESP8266 firmware that works with this branch (README.md coming soon on that repo).
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#3354
Basic example entry for
configuration.yaml:More advanced example entry for
configuration.yaml:Forum Discussion(s):
Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
toxrun successfully. Your PR cannot be merged unless tests passREQUIREMENTSvariable (example).requirements_all.txtby runningscript/gen_requirements_all.py..coveragerc.If the code does not interact with devices:
toxrun successfully. Your PR cannot be merged unless tests pass