Cast Integration Cleanup by OttoWinter · Pull Request #13275 · home-assistant/core

OttoWinter · 2018-03-16T21:43:03Z

Description:

Clean up the cast integration.

Most notably the pychromecast.Chromecast objects are now managed by the entities, not globally. This makes handling "moved" hosts, unavailable chromecasts and disconnecting much cleaner.

setup_platform and internal discovery now only create "ChromecastInfo" objects (with attrs 👌), not Chromecast objects. Missing information is automatically added via HTTP dial.
The CastDevice entities now take care of moved hosts themselves (using dispatcher callbacks).
Added available property.
Enabled polling mode if an app is active, because we sometimes don't receive media status callbacks from certain apps.
Added a ton of comments to hopefully make understanding the source code a bit easier.

Todo:

Some more local testing.
Fixing tests.
Add tests

Out of scope:

Move some of the logic into pychromecast (would be good to test the logic with this user base first; but pychromecast could benefit from some logic, like availability, info objects, or half-polling mode; the last one is a bit controversial though)

fixes #13359, fixes #13319, fixes #13090

Example entry for `configuration.yaml` (if applicable):

media_player:
  - platform: cast

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass

If the code does not interact with devices:

Tests have been added to verify that the new code works.

houndci-bot · 2018-03-16T21:43:49Z

+    """Helper class to handle pychromecast status callbacks.
+
+    Necessary because a CastDevice entity can create a new socket client
+    and therefore callbacks from multiple chromecast connections can potentially


line too long (80 > 79 characters)

OttoWinter · 2018-03-17T10:24:17Z

Tested extensively with chromecast, google home, cast audio and audio groups now. On to the tests!

MartinHjelmare · 2018-03-17T14:36:55Z

+        """Turn on the cast device."""
+        import pychromecast
+
+        if self._chromecast is None:


When can this happen? We set the cast info in async_added_to_hass.

When the chromecast "switched host". This happens for example when a cast audio group changes its "elected leader"/master. Then we first disconnect the old chromecast object and then connect the new one.

In that process, self._chromecast is briefly changed to None to indicate that the old chromecast object is invalid (any commands on the old object would result in exceptions, as the socket is no longer connected)

Additionally, when starting up, there might be a phase when the chromecast object is not connected yet but the entity is already visible in the frontend. This can be reproduced by manually specifying the cast device by host: in configuration.yaml and disabling any internet connection of your computer.

Shouldn't we set the entity to unavailable if it looses the connection to the chromecast? Then you wouldn't need to guard in all the methods.

It sounds weird that we allow entities to be created without a working connection during setup.

Shouldn't we set the entity to unavailable if it looses the connection to the chromecast?

Yes, that's also what this PR does. When self._chromecast is None available will also be False.

Then you wouldn't need to guard in all the methods.

Ok, that's good. I put those checks everywhere to protect for potential race conditions, where the service calls are executed even though we're not connected. If the core can guarantee that these methods will never be called with an unavailable state I can also remove those checks.

It sounds weird that we allow entities to be created without a working connection during setup.

That's true, and also was the previous behavior. The problem with that was that we would block the entire Home Assistant startup waiting for the socket client to connect. If no connection was available, we would even stop startup indefinitely - not good. I tried looking for a solution to that with asyncio for some time, but ultimately I think unavailable is better (and quicker).

I mean if we want to we can also try checking the HTTP API of the chromecast before adding the device, that would fix part of the problem (for standard chromecasts).

MartinHjelmare · 2018-03-17T14:37:50Z

        """Return the state of the player."""
-        if self.media_status is None:
+        if self._media_status is None:
            return STATE_UNKNOWN


Return None for unknown state.

MartinHjelmare · 2018-03-17T14:38:15Z

-        elif self.cast.is_idle:
+        elif self._chromecast is not None and self._chromecast.is_idle:
            return STATE_OFF
        return STATE_UNKNOWN


Gets rid of those pesky "Setup of platform cast is taking over 10 seconds." messages.

edif30 · 2018-03-17T15:59:41Z

Once ready let me know and I can test if you'd like.

MartinHjelmare · 2018-03-17T18:05:17Z

FYI, regarding core guarding from calling services for unavailable entities: Components that use async_extract_from_service handle this. If a platform registers its own services it has to handle that on its own, in the service handler.

It's not using async anyway

OttoWinter

Added some comments here to point out some aspects of the changes that didn't really fit in code comments.

Since yesterday I also went through all sorts of scenarios of chromecasts disconnecting at certain times, no network connection, manually moving hosts around and so on. All of those "physical tests" appear to be working correctly with my set up + the available property now gives immediate feedback as to what is happening.

@edif30 If you want you could test these changes. It should essentially be as simple as copying this file into [config_dir]/custom_components/media_player/cast.py.

OttoWinter · 2018-03-18T10:39:04Z

    vol.Optional(CONF_HOST): cv.string,
-    vol.Optional(CONF_IGNORE_CEC): [cv.string],
+    vol.Optional(CONF_IGNORE_CEC, default=[]): vol.All(cv.ensure_list,
+                                                       [cv.string])


It would be great if someone with more knowledge of voluptuous could take a look at this changed line. As far as I know, we should use cv.ensure_list wherever possible. Another question is whether to put it in this PR, but it's title "cleanup" after all...

OttoWinter · 2018-03-18T10:41:54Z

+        # Remove previous cast infos with same uuid from known chromecasts.
+        same_uuid = set(x for x in hass.data[KNOWN_CHROMECAST_INFO_KEY]
+                        if info.uuid == x.uuid)
+        hass.data[KNOWN_CHROMECAST_INFO_KEY] -= same_uuid


I mean there can only be a single uuid removed from this set anyway as non-None UUIDs are unique, but the alternative solutions of using set#remove and a dictionary for hass.data[KNOWN_CHROMECAST_INFO_KEY] were just way messier (the latter one getting particularly complicated because of None UUID handling).

OttoWinter · 2018-03-18T10:44:05Z

    """Set up the pychromecast internal discovery."""
-    hass.data.setdefault(INTERNAL_DISCOVERY_RUNNING_KEY, threading.Lock())
+    if INTERNAL_DISCOVERY_RUNNING_KEY not in hass.data:
+        hass.data[INTERNAL_DISCOVERY_RUNNING_KEY] = threading.Lock()


Changed this to be the non-setdefault syntax because creating a threading.Lock instance is quite expensive.

OttoWinter · 2018-03-18T10:45:27Z

@@ -139,191 +191,402 @@ async def async_setup_platform(hass: HomeAssistantType, config: ConfigType,

    # Import CEC IGNORE attributes
    pychromecast.IGNORE_CEC += config.get(CONF_IGNORE_CEC, [])


If you're wondering why I'm still supplying [] here even though a default is set in the platform schema, it's because discovery sends an empty dict as config.

OttoWinter · 2018-03-18T10:46:54Z

+                                        info)
+        if info.friendly_name is None:
+            # HTTP dial failed, so we won't be able to connect.
            raise PlatformNotReady


When manually specifying a host in configuration.yaml like this:

media_player: - platform: cast host: 192.168.178.42

this will block startup for 10 seconds; though I suppose that is okay when users manually specify the host.

I like "protected" access, but I like reviewing more :)

balloob · 2018-03-23T21:02:49Z

Awesome work 🐬

MaluNoPeleke · 2018-03-27T06:24:22Z

@OttoWinter Is it already merged?
I just got a

Platform cast not ready yet. Retrying in 30 seconds.

with beta 66.

See #13359

OttoWinter · 2018-03-27T06:26:45Z

Yes it is in the beta. And that error message is expected now, it signals that one of Chromecast devices defined in your configuration.yaml can't be reached and therefore can't be setup.

MaluNoPeleke · 2018-03-27T06:39:07Z

You mean the automatically created entity_registry.yaml?
Yesterday the same setup worked as expected but today not. All devices for the group are connected to power and the network (while not all devices of the list in the above mentioned file are connected to power).
Could the TTS and delay command be a problem?

    action:
      - service: media_player.volume_set
        data_template:
          entity_id: media_player.schlafzimmer
          volume_level: 0.1
      - service: tts.google_say
        data_template:
          entity_id: media_player.schlafzimmer
          message: Guten Morgen. Es ist {{now().hour}} Uhr {{now().minute}}
          cache: false
      - delay: 00:00:15
      - service: media_player.play_media
        data_template:
          entity_id: media_player.wecker
          media_content_id: 'http://fluxfm.hoerradar.de/fluxfmberlin-live-aac-mq?sABC=59s45n23%230%23no8617362n29q2o435p17n54928n16s5%23gharva&amsparams=playerid:tunein;skey:1509186083'
          media_content_type: 'audio/mp3'

OttoWinter · 2018-03-27T06:46:34Z

What are you using to discover your cast devices? discovery: in your configuration.yaml, or media_player: platform: cast or manually by host:?
In what way are they not working? Is it just the error message or do they not turn on/execute the command?
If you have some Chromecast devices in your configuration.yaml that are not connected to power that error message is expected; the other devices should work correctly though. (I hope, at least they did during my testing)
The TTS and delay shouldn't really affect anything here. The entity_regitry.yaml neither.
I don't think this discussion should be done in this PR, as it's not a comment on the code. Please open a separate issue and I will help you out there.

MaluNoPeleke · 2018-03-27T07:21:46Z

Sorry, I have tried to summarize everything in #13483

homeassistant added cla-signed platform: media_player.cast labels Mar 16, 2018

Cast Integration Cleanup

827d524

OttoWinter force-pushed the cast-cleanup branch from 114167f to 827d524 Compare March 16, 2018 21:43

homeassistant added the cla-signed label Mar 16, 2018

houndci-bot reviewed Mar 16, 2018

View reviewed changes

OttoWinter added 3 commits March 16, 2018 22:44

Fix long line

d1facf4

Fixes and logging

3243ec0

Fix tests

fd9e0d9

Lint

28200e9

OttoWinter mentioned this pull request Mar 17, 2018

Google Cast device not recognised #13090

Closed

MartinHjelmare reviewed Mar 17, 2018

View reviewed changes

OttoWinter added 3 commits March 17, 2018 15:47

Report unknown state with None

5d65452

Lint

1b864e7

Switch to async_add_job

be9276b

Gets rid of those pesky "Setup of platform cast is taking over 10 seconds." messages.

OttoWinter added 3 commits March 17, 2018 18:44

Re-introduce PlatformNotReady

81fbd66

Add tests

21c5a18

Remove unnecessary checks

4d35d63

OttoWinter added 3 commits March 17, 2018 19:27

Test PlatformNotReady

04045c3

Fix async in sync context

601c4c7

Blocking update

65deab1

It's not using async anyway

OttoWinter commented Mar 18, 2018

View reviewed changes

OttoWinter changed the title ~~[WIP] Cast Integration Cleanup~~ Cast Integration Cleanup Mar 18, 2018

OttoWinter requested a review from andrey-git as a code owner March 19, 2018 15:16

homeassistant added the cla-signed label Mar 19, 2018

Upgrade pychromecast to 2.1.0

726f2e8

OttoWinter mentioned this pull request Mar 19, 2018

Store Hue auth as config entries #13034

Merged

7 tasks

OttoWinter mentioned this pull request Mar 21, 2018

Cannot connect to Google Home (Casting) #13359

Closed

OttoWinter added 3 commits March 21, 2018 18:40

Make reviewing easier

67ee291

I like "protected" access, but I like reviewing more :)

Make reviewing even easier :)

0fe3728

Comment tests

98eb91b

balloob approved these changes Mar 23, 2018

View reviewed changes

balloob merged commit 6a625bd into home-assistant:dev Mar 23, 2018

OttoWinter mentioned this pull request Mar 27, 2018

Cast feature doesn't work reliable #13483

Closed

balloob mentioned this pull request Mar 30, 2018

0.66.0 #13554

Merged

OttoWinter mentioned this pull request Apr 9, 2018

Avoid unnecessary cast state updates #13770

Merged

3 tasks

OttoWinter mentioned this pull request Apr 21, 2018

Revert cast platform polling mode #14027

Merged

3 tasks

home-assistant locked and limited conversation to collaborators Jul 26, 2018

OttoWinter deleted the cast-cleanup branch September 29, 2018 10:32

ghost added integration: cast and removed platform: media_player.cast labels Mar 21, 2019

		@@ -139,191 +191,402 @@ async def async_setup_platform(hass: HomeAssistantType, config: ConfigType,

		# Import CEC IGNORE attributes
		pychromecast.IGNORE_CEC += config.get(CONF_IGNORE_CEC, [])

Uh oh!

Conversation

OttoWinter commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OttoWinter commented Mar 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edif30 commented Mar 17, 2018

Uh oh!

MartinHjelmare commented Mar 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OttoWinter left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob commented Mar 23, 2018

Uh oh!

MaluNoPeleke commented Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OttoWinter commented Mar 27, 2018

Uh oh!

MaluNoPeleke commented Mar 27, 2018

Uh oh!

OttoWinter commented Mar 27, 2018

Uh oh!

MaluNoPeleke commented Mar 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

OttoWinter commented Mar 16, 2018 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

MartinHjelmare commented Mar 17, 2018 •

edited

Loading

OttoWinter left a comment •

edited

Loading

MaluNoPeleke commented Mar 27, 2018 •

edited

Loading