Tellstick Duo acync callback fix #10384
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,12 +4,14 @@ | |
| For more details about this component, please refer to the documentation at | ||
| https://home-assistant.io/components/tellstick/ | ||
| """ | ||
| import asyncio | ||
| import logging | ||
| import threading | ||
|
|
||
| import voluptuous as vol | ||
|
|
||
| from homeassistant.helpers import discovery | ||
| from homeassistant.core import callback | ||
| from homeassistant.const import ( | ||
| EVENT_HOMEASSISTANT_STOP, CONF_HOST, CONF_PORT) | ||
| from homeassistant.helpers.entity import Entity | ||
|
|
@@ -26,6 +28,8 @@ | |
| DEFAULT_SIGNAL_REPETITIONS = 1 | ||
| DOMAIN = 'tellstick' | ||
|
|
||
| DATA_TELLSTICK = 'tellcore_registry' | ||
|
|
||
| # Use a global tellstick domain lock to avoid getting Tellcore errors when | ||
| # calling concurrently. | ||
| TELLSTICK_LOCK = threading.RLock() | ||
|
|
@@ -62,7 +66,7 @@ def _discover(hass, config, component_name, found_tellcore_devices): | |
| def setup(hass, config): | ||
| """Set up the Tellstick component.""" | ||
| from tellcore.constants import TELLSTICK_DIM | ||
| from tellcore.telldus import AsyncioCallbackDispatcher | ||
| from tellcore.telldus import TelldusCore | ||
| from tellcorenet import TellCoreClient | ||
|
|
||
|
|
@@ -83,7 +87,7 @@ def stop_tellcore_net(event): | |
|
|
||
| try: | ||
| tellcore_lib = TelldusCore( | ||
| callback_dispatcher=AsyncioCallbackDispatcher(hass.loop)) | ||
|
There was a problem hiding this comment. The problem is, you need implement all in async to do that. There was a problem hiding this comment. Hmm, can you expand on that? Would that be cleaner still than flushing the queue of the QueuedCallbackDispatcher regularly (as I understood is needed to make callbacks happen for QueuedCallbackDispatcher)? Hoping we can find some fix to this, #10329 is a pretty serious bug for my HA-usecase. There was a problem hiding this comment. This platform/components run in our threaded environment. So it need run inside threads. If you want run asynchronous, you need port this into our async interfaces There was a problem hiding this comment. I don't know the Home Assistant codebase too well, so it's not clear to me what that would require. @stefan-jonasson, ideas? @pvizeli, since you know the code well, and landed 1c8f179 which regressed this, perhaps you could land a fix for it too? Would love to get my switches working again 😃 There was a problem hiding this comment. @pvizeli Can you describe the actual problem we trying to avoid (because the previous implementation worked just fine)? When you say all exactly what does that entail, just the callback handling or do we need to rewrite the whole platform? If so could you point me to some examples from other platforms that implemented in that way? There was a problem hiding this comment. @pvizeli Are you saying we can or can't revert this? If we can't, are you saying there will be a fix for 0.57.3? There was a problem hiding this comment. @torarnv Until this has been resolved, you can always just modify your local tellstick.py file and revert the breaking changes yourself. I did that and my HA runs as smoothly as ever. Sure, there might be problems with blocking the core loop and whatnot, but I haven't experienced any issues yet. There was a problem hiding this comment. @stefan-jonasson we should discuss a real solution here and no flameware. If you convert the callback they will be called from tellcore library to async (tag it with @callback from core.py too) it should work. Maybe that is the fast way and maybe also the best way. There was a problem hiding this comment. I give a try, but I can't test it There was a problem hiding this comment. @pvizeli Happy to test anything you come up with! 😄 |
||
| except OSError: | ||
| _LOGGER.exception("Could not initialize Tellstick") | ||
| return False | ||
|
|
@@ -94,7 +98,7 @@ def stop_tellcore_net(event): | |
| # Register devices | ||
| tellcore_registry = TellstickRegistry(hass, tellcore_lib) | ||
| tellcore_registry.register_tellcore_devices(all_tellcore_devices) | ||
| hass.data[DATA_TELLSTICK] = tellcore_registry | ||
|
|
||
| # Discover the switches | ||
| _discover(hass, config, 'switch', | ||
|
|
@@ -124,30 +128,35 @@ class TellstickRegistry(object): | |
| def __init__(self, hass, tellcore_lib): | ||
| """Initialize the Tellstick mappings and callbacks.""" | ||
| # used when map callback device id to ha entities. | ||
| self.hass = hass | ||
| self._id_to_ha_device_map = {} | ||
| self._id_to_tellcore_device_map = {} | ||
| self._setup_tellcore_callback(tellcore_lib) | ||
|
|
||
| @callback | ||
| def _async_tellcore_event_callback(self, tellcore_id, tellcore_command, | ||
| tellcore_data, cid): | ||
| """Handle the actual callback from Tellcore.""" | ||
| ha_device = self._id_to_ha_device_map.get(tellcore_id, None) | ||
| if ha_device is not None: | ||
| # Pass it on to the HA device object | ||
| self.hass.async_add_job( | ||
| ha_device.update_from_callback, tellcore_command, | ||
| tellcore_data) | ||
|
|
||
| def _setup_tellcore_callback(self, tellcore_lib): | ||
| """Register the callback handler.""" | ||
| callback_id = tellcore_lib.register_device_event( | ||
| self._async_tellcore_event_callback) | ||
|
|
||
| def clean_up_callback(event): | ||
| """Unregister the callback bindings.""" | ||
| if callback_id is not None: | ||
| tellcore_lib.unregister_callback(callback_id) | ||
| _LOGGER.debug("Tellstick callback unregistered") | ||
|
|
||
| self.hass.bus.listen_once( | ||
| EVENT_HOMEASSISTANT_STOP, clean_up_callback) | ||
|
|
||
| def register_ha_device(self, tellcore_id, ha_device): | ||
| """Register a new HA device to receive callback updates.""" | ||
|
|
@@ -182,10 +191,16 @@ def __init__(self, tellcore_id, tellcore_registry, signal_repetitions): | |
| self._tellcore_device = tellcore_registry.get_tellcore_device( | ||
| tellcore_id) | ||
| self._name = self._tellcore_device.name | ||
|
|
||
| @asyncio.coroutine | ||
| def async_added_to_hass(self): | ||
| """Register callbacks.""" | ||
| tellcore_registry = self.hass.data[DATA_TELLSTICK] | ||
|
|
||
| # Add ourselves to the mapping for callbacks | ||
| self.hass.async_add_job( | ||
| tellcore_registry.register_ha_device, | ||
| self._tellcore_device.id, self) | ||
|
|
||
| @property | ||
| def should_poll(self): | ||
|
|
@@ -283,7 +298,7 @@ def update_from_callback(self, tellcore_command, tellcore_data): | |
| # This is a benign race on _repeats_left -- it's checked with the lock | ||
| # in _send_repeated_command. | ||
| if self._repeats_left > 0: | ||
| self.hass.add_job(self._send_repeated_command) | ||
|
|
||
| def _update_from_tellcore(self): | ||
| """Read the current state of the device from the tellcore library.""" | ||
|
|
@@ -303,4 +318,3 @@ def _update_from_tellcore(self): | |
| def update(self): | ||
| """Poll the current state of the device.""" | ||
| self._update_from_tellcore() | ||
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 works, but the fix is conceptually in the wrong place. When we get into
_update_model_from_command, we know thetellcore_command, and we know that it'sTELLSTICK_DIM. That's the only use-case where parsing the data makes sense, per the docs:The fix should be in
_update_model_from_command, never calling_parse_tellcore_data, which by the time is called doesn't know what the command was, and can't reason about whether it's a failure that we have no data or if it's due to receiving one of the commands that have no data in the first place.