Rework chromecast fix #16804
Rework chromecast fix #16804
Conversation
homeassistant
added
platform: media_player.cast
small-pr
ghost
assigned 
| @@ -216,13 +213,15 @@ def _async_create_cast_device(hass: HomeAssistantType, | |||
| if not isinstance(config, list): | |||
| config = [config] | |||
|
|
|||
| await asyncio.wait([ | |||
| done, pending = await asyncio.wait([ | |||
| _async_setup_platform(hass, cfg, async_add_entities, None) | |||
| for cfg in config]) | |||
There was a problem hiding this comment.
Isn't pending always empty here? From what I understand from the docs, pending contains a set of tasks that are not done when the timeout occurs. But there is no timeout configured here...
Also, it would be good to cancel all tasks in pending.
| raise PlatformNotReady | ||
| _LOGGER.debug("Cannot retrieve detail information for chromecast" | ||
| " %s, the device may not online", info) | ||
| remove_handler() |
There was a problem hiding this comment.
Ah yes, removing the handler is a good idea here 👍
| # HTTP dial failed, so we won't be able to connect. | ||
| raise PlatformNotReady | ||
| _LOGGER.debug("Cannot retrieve detail information for chromecast" | ||
| " %s, the device may not online", info) |
There was a problem hiding this comment.
the device may not be online ;-)
| _async_setup_platform(hass, cfg, async_add_entities, None) | ||
| for cfg in config]) | ||
| if pending or any([not task.result() for task in done]): | ||
| raise PlatformNotReady |
There was a problem hiding this comment.
What happens if the setup for two chromecast succeeded but failed for a third chromecast? Then I think this code would raise a PlatformNotReady and re-schedule setup. But each time the setup occurs, this would end up re-connecting all three chromecasts, not just the one that failed.
If the devices have no UUID (don't know if this can be the case with config entries), this could end up creating new entities for the two connectable chromecasts every 10 seconds.
There was a problem hiding this comment.
We will get the message such as
Discovered previous chromecast ...
| _LOGGER.debug("Cannot retrieve detail information for chromecast" | ||
| " %s, the device may not online", info) | ||
| remove_handler() | ||
| return False |
There was a problem hiding this comment.
Style-wise I find returning a boolean a bit bad. async_setup_platforms are not supposed to return booleans. Of course _async_setup_platform is only a proxy for async_setup_platform but it might be good to keep the syntax the same for style. Besides, the old way could actually be a bit nicer too:
async def _async_setup_platform(hass: HomeAssistantType, config: ConfigType,
async_add_entities, discovery_info):
# ...
if info.friendly_name is None:
# HTTP dial failed, so we won't be able to connect.
remove_handler()
raise PlatformNotReady
# ...
async def async_setup_entry(hass, config_entry, async_add_entities):
# ...
done, pending = await asyncio.wait([
_async_setup_platform(hass, cfg, async_add_entities, None)
for cfg in config], timeout=10)
# ...
| with patch( | ||
| 'homeassistant.components.media_player.cast._async_setup_platform', | ||
| return_value=mock_coro(exception=PlatformNotReady)) as mock_setup: | ||
| with pytest.raises(PlatformNotReady): |
There was a problem hiding this comment.
Based on the code, this could be just Exception right?
There was a problem hiding this comment.
Edit: I see it now.
There was a problem hiding this comment.
Changed test to throw Exception
|
I'm using 0.79.3 but google home still not recognize, although google home is already alive |
|
@dony71 please open an issue if you still met problem. We don't discuss it in merged pull request. |
Description:
Revert the change of #16471.
Fix the usage of PlatformNotReady exception to resolve startup hang up issue.
If user configured cast by host IP address and one of chromedevice is not online during the startup, a PlatformNotReady exception will allow system start up continue and a platform setup task will be retried later.
This PR will revert chromecast connection/re-connection and discovery logic back to 0.77, but fix the issue in previous release that if device is not online during the system startup, it won't be connected even it came back later.
Related issue (if applicable): fixes #16686
Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>
Example entry for
configuration.yaml(if applicable):Checklist:
tox. Your PR cannot be merged unless tests pass