Skip to content

Rework chromecast fix #16804

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.

Sign up for GitHub

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

Merged
balloob merged 7 commits into from
Sep 23, 2018
Merged

Rework chromecast fix #16804

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
00a3438
Revert changes of #16471, and fix the platform setup issue
awarecan Sep 23, 2018
897934f
Fix unit test
awarecan Sep 23, 2018
73b4e2e
Fix
awarecan Sep 23, 2018
abbe32c
Fix comment
awarecan Sep 23, 2018
c480850
Fix test
awarecan Sep 23, 2018
0fc48ad
Address review comment
awarecan Sep 23, 2018
b0ef45e
Address review comment
awarecan Sep 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 17 additions & 13 deletions homeassistant/components/media_player/cast.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,6 @@
vol.All(cv.ensure_list, [cv.string]),
})

CONNECTION_RETRY = 3
CONNECTION_RETRY_WAIT = 2
CONNECTION_TIMEOUT = 10


@attr.s(slots=True, frozen=True)
class ChromecastInfo:
Expand Down Expand Up @@ -206,8 +202,9 @@ async def async_setup_platform(hass: HomeAssistantType, config: ConfigType,
_LOGGER.warning(
'Setting configuration for Cast via platform is deprecated. '
'Configure via Cast component instead.')
await _async_setup_platform(
hass, config, async_add_entities, discovery_info)
if not await _async_setup_platform(
hass, config, async_add_entities, discovery_info):
raise PlatformNotReady


async def async_setup_entry(hass, config_entry, async_add_entities):
Expand All @@ -216,13 +213,15 @@ async def async_setup_entry(hass, config_entry, async_add_entities):
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])

@OttoWinter OttoWinter Sep 23, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@awarecan awarecan Sep 23, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed pending

if pending or any([not task.result() for task in done]):
raise PlatformNotReady

@OttoWinter OttoWinter Sep 23, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@awarecan awarecan Sep 23, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will get the message such as
Discovered previous chromecast ...



async def _async_setup_platform(hass: HomeAssistantType, config: ConfigType,
async_add_entities, discovery_info):
async_add_entities, discovery_info) -> bool:
"""Set up the cast platform."""
import pychromecast

Expand Down Expand Up @@ -250,8 +249,8 @@ def async_cast_discovered(discover: ChromecastInfo) -> None:
if cast_device is not None:
async_add_entities([cast_device])

async_dispatcher_connect(hass, SIGNAL_CAST_DISCOVERED,
async_cast_discovered)
remove_handler = async_dispatcher_connect(
hass, SIGNAL_CAST_DISCOVERED, async_cast_discovered)
# Re-play the callback for all past chromecasts, store the objects in
# a list to avoid concurrent modification resulting in exception.
for chromecast in list(hass.data[KNOWN_CHROMECAST_INFO_KEY]):
Expand All @@ -265,10 +264,15 @@ def async_cast_discovered(discover: ChromecastInfo) -> None:
info = await hass.async_add_job(_fill_out_missing_chromecast_info,
info)
if info.friendly_name is None:
# 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)

@OttoWinter OttoWinter Sep 23, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the device may not be online ;-)

remove_handler()

@OttoWinter OttoWinter Sep 23, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, removing the handler is a good idea here 👍

return False

@OttoWinter OttoWinter Sep 23, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
    # ...


hass.async_add_job(_discover_chromecast, hass, info)

return True


class CastStatusListener:
"""Helper class to handle pychromecast status callbacks.
Expand Down Expand Up @@ -379,7 +383,7 @@ async def async_set_cast_info(self, cast_info):
pychromecast._get_chromecast_from_host, (
cast_info.host, cast_info.port, cast_info.uuid,
cast_info.model_name, cast_info.friendly_name
), CONNECTION_RETRY, CONNECTION_RETRY_WAIT, CONNECTION_TIMEOUT)
))
self._chromecast = chromecast
self._status_listener = CastStatusListener(self, chromecast)
# Initialise connection status as connected because we can only
Expand Down
6 changes: 3 additions & 3 deletions tests/components/media_player/test_cast.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ async def test_entry_setup_no_config(hass: HomeAssistantType):

with patch(
'homeassistant.components.media_player.cast._async_setup_platform',
return_value=mock_coro()) as mock_setup:
return_value=mock_coro(True)) as mock_setup:
await cast.async_setup_entry(hass, MockConfigEntry(), None)

assert len(mock_setup.mock_calls) == 1
Expand All @@ -389,7 +389,7 @@ async def test_entry_setup_single_config(hass: HomeAssistantType):

with patch(
'homeassistant.components.media_player.cast._async_setup_platform',
return_value=mock_coro()) as mock_setup:
return_value=mock_coro(True)) as mock_setup:
await cast.async_setup_entry(hass, MockConfigEntry(), None)

assert len(mock_setup.mock_calls) == 1
Expand All @@ -409,7 +409,7 @@ async def test_entry_setup_list_config(hass: HomeAssistantType):

with patch(
'homeassistant.components.media_player.cast._async_setup_platform',
return_value=mock_coro()) as mock_setup:
return_value=mock_coro(True)) as mock_setup:
await cast.async_setup_entry(hass, MockConfigEntry(), None)

assert len(mock_setup.mock_calls) == 2
Expand Down