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 integration for Vallox Ventilation Units #24660
Conversation
@yozik04 FYI. |
Ohh. Nice! |
SPEED_BOOST = 'Boost' | ||
SPEED_FIREPLACE = 'Fireplace' | ||
|
||
SPEED_LIST = [SPEED_HOME, SPEED_AWAY, SPEED_BOOST, SPEED_FIREPLACE] |
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.
Why you mess names? It makes hard to understand. SPEED != PROFILE. I would rename all SPEED_* -> PROFILE_*
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.
I did it because the HA functions consuming it later have "speed" in their name. So one way or the other we'll have this indirection.
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.
Only speeds in the base fan component are allowed for the set speed service/method and speed property.
See this architecture issue for a future possible change:
home-assistant/architecture#27
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.
If it is a hard requirement to only expose these speed options, then I fear this is a blocker for the Vallox ventilation unit integration as I picture it. The units are always-on units that constantly supply a whole building with fresh air, and the control interfaces expose four inherent, manufacturer-provided profiles between which the user can switch. Here's a screenshot of OEM web client:
Controlling your Vallox units from within HA only makes sense if the profile semantics can be kept. Can we make an exception until your linked PR lands?
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 a hard requirement. We are free to map the device modes to the home assistant speed modes, but that might not make sense in all cases.
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.
While I find it a bit stubborn to not allow this temporarily, I will transform profile selection to a service then.
Give me some days to complete this.
I need to make my lib async as well... |
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.
I've not done a full review yet. I've only commented on the main architectural dependent changes needed.
@@ -211,3 +211,34 @@ wemo_reset_filter_life: | |||
entity_id: | |||
description: Names of the WeMo humidifier entities (1 or more entity_ids are required). | |||
example: 'fan.wemo_humidifier' | |||
|
|||
vallox_set_profile_fan_speed_home: |
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.
Integration specific services should be added to the integration package and registered under the integration domain.
Move these service descriptions to homeassistant/components/vallox/services.yaml
and change the registration to the mentioned domain.
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.
Will change.
Just for my own understanding: I tried to start from an existing integration, in this case xiaomi miio and wemo, where they did it that way. Was I wrong to assume this is the same case, or did these rules not apply back when they integrated 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.
Those integrations are not the best examples, since they don't always follow our current standards and architecture.
Our architecture decisions have evolved and there are still some old integrations that don't follow them.
"Check for \"vallox:\" in the configuration.") | ||
return | ||
|
||
global SENSOR_INSTANCES |
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.
Don't use global. We can use hass.data to store instances specific to the integration platform.
hass.data[DATA_VALLOX_SENSOR_LIST] = sensor_list | ||
await discovery.async_load_platform(hass, 'sensor', DOMAIN, {}, config) | ||
|
||
await discovery.async_load_platform(hass, 'fan', 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.
Use hass.async_create_task
to schedule this as a task to avoid possible deadlock.
DOMAIN: vol.Schema({ | ||
vol.Required(CONF_HOST): vol.All(ipaddress.ip_address, cv.string), | ||
vol.Optional(CONF_NAME, default=DEFAULT_NAME): cv.string, | ||
vol.Optional(CONF_SENSORS): vol.All(cv.ensure_list, [vol.In(SENSORS)]), |
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.
We no longer want to include config options for selecting sensor types. All available types should be included by default.
"requirements": [ | ||
"vallox-websocket-api==1.5.1", | ||
"numpy==1.16.3", | ||
"websocket-client==0.54.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.
Why do we need to include the websocket client as a requirement? I would think that the vallox websocket api library would depend on the websocket client 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.
Why include numpy? We don't seem to use that.
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.
vallox-websockets-api lib uses that. But it should be pulled as lib dependency then.
I am in progress or changing vallox-websocket-api to be async. Maybe will finish next week. |
I want to refactor a bit and introduce an intermediary class that fetches and caches all read values with a single call periodically. Fan and sensor classes would then fetch those cached values. @MartinHjelmare, which is an integration I can use as reference? Is smarty okay? |
Yes, that's a good example. 👍 |
Addressing the review comments resulted in a bigger refactoring. Please have a look. Might still need some tweaks here and there, but I hope I addressed all the bigger architectural concerns. Removing I will update the corresponding documentation PR once we agreed on the architecture. |
self._profile = None | ||
self._valid = False | ||
|
||
async_track_time_interval(self._hass, self.async_update, SCAN_INTERVAL) |
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.
We don't want side effects in init method. Please move this outside the class or into a dedicated setup method.
|
||
def get_profile(self): | ||
"""Return cached profile value.""" | ||
_LOGGER.debug("Returning profile.") |
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.
We don't end logging messages with period.
_LOGGER.debug("Updating Vallox state cache.") | ||
|
||
try: | ||
self._metric_cache = self._client.fetch_metrics() |
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.
If I'm reading the vallox library correctly, this does non asyncio I/O and we're in a coroutine here. That's not allowed. Coroutines will be executed in the event loop which should never block.
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.
Consider switching affected methods to regular sync functions. This might mean we have to use the home assistant sync api.
The alternative is to schedule I/O on the executor thread pool with hass.async_add_executor_job
. But if we need to do that a lot, the sync api is cleaner.
device = ValloxFan(hass.data[DOMAIN]['name'], | ||
hass.data[DOMAIN]['client'], | ||
hass.data[DOMAIN]['state_proxy']) | ||
except KeyError: |
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.
Remove this. Instead add a guard clause at the top of the function body that checks if discovery_info is None and return if so.
|
||
async def async_setup(hass, config): | ||
"""Set up the client and boot the platforms.""" | ||
if DOMAIN not in 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.
This can't happen. There is no network discovery and we're not using config entries here yet.
self._fan_speed_boost = None | ||
|
||
try: | ||
self._client.set_settable_address(METRIC_KEY_MODE, int) |
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.
Please avoid side effects in init method.
self._available = False | ||
_LOGGER.error("Error turning on: %s", io_err) | ||
else: | ||
_LOGGER.error("Already on.") |
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.
If we have the wrong state info we won't be able to change state. If that's a possibility, we try to avoid guards like this.
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.
I put it there to catch a call to the service fan/turn_on
, where tests showed it executes even if the fan is already on.
This could for example happen if a user-automation script is erroneous.
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 it be bad for the device to call turn on if it's already on?
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.
I don't know. There's no documentation that would give it away, so I did not want to take chances and cause side effects in the device when we can prevent it on the higher level.
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.
I think it's better to put protection close to the device if the device requires protection.
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.
The problem is we cannot find out if the device firmware is fine with mutliple calls or if there would be unforeseen consequences if it would be spammed with those commands due to a user script gone rogue (making up a "dramatic" case here).
In the end it is just an if/else on a boolean to be on the safe side here. IMO, that's an acceptable price to pay for a little more safety?
_LOGGER.debug("Turn on: %s", speed) | ||
|
||
# Case speed == None equals the GUI toggle switch being activated. | ||
if speed is None: |
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.
Invert this and make it a guard clause instead. Ie return if true. Then we can outdent below.
if mode == 0: | ||
self._state = True | ||
else: | ||
self._state = None |
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.
We don't want to set it to False
which means off?
I'll look into the requirements. |
I think the design is ok. Consider using the home assistant sync api until the library supports asyncio. |
Due to lack of personal bandwidth, I went for |
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.
I think this looks good! I'll have a look at the requirements now.
self._client.get_profile) | ||
self._valid = True | ||
|
||
except IOError as io_err: |
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.
We can except OSError instead. IOError is an alias of OSError.
https://docs.python.org/3/library/exceptions.html#IOError
The following exceptions are kept for compatibility with previous versions; starting from Python 3.3, they are aliases of OSError.
Python 3.6.7 (default, Oct 22 2018, 11:32:17)
Type 'copyright', 'credits' or 'license' for more information
IPython 7.3.0 -- An enhanced Interactive Python. Type '?' for help.
In [1]: try:
...: raise IOError
...: except OSError:
...: print("Caught the IOError")
...:
Caught the IOError
In [2]:
I think I've found the problem with the requirements not installing: |
PR Merged and new version created: https://pypi.org/project/vallox-websocket-api/ About async version of the API. For some reason aiohttp.ClientRequest does not connect to Vallox websocket... Wireshark shows exactly the same request. Response from the unit is different... Investigating. |
Tested and working 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.
Great!
I have released async version of the API Interface is exactly the same. Just add |
Hi @yozik04 , Please see the error message below from https://community.home-assistant.io/t/vallox-valloplus-350mv-partial-success-work-in-progress-on-vallox-component/52588/30 Is the response from the Vallox unit bad? Anything we can do about it? I hit this bug as well already, sometimes it helps to try multiple times.
|
Please don't discuss in merged PRs. Open an issue instead. Thanks! |
Description:
This PR adds initial support for Vallox ventilation units that are compatible to the vallox_websocket_api PyPi library.
The fan component code reuses parts from the Xiaomi miio fan code, which has been used as a template for this integration.
Related issue (if applicable): N/A
Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#9668
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:
python3 -m script.hassfest
.requirements_all.txt
by runningpython3 -m script.gen_requirements_all
..coveragerc
.If the code does not interact with devices: