Dynamic host ip tracking for cast groups #167

freol35241 · 2017-03-18T21:38:35Z

fixes #165

emlove

You might want to wait for feedback from balloob. I'm not sure if this is something he wants to support or not. In any case I added some feedback here on the implementation details.

emlove · 2017-03-18T22:35:58Z

pychromecast/__init__.py

+            else False
+        self.dynamic_host = kwargs.pop('dynamic_host', self.dynamic_host)
+        if self.dynamic_host:
+            self.start_dynamic_host_tracking()


We don't want to create a new thread for each cast group. I think what we'd have to do is start the thread for the first group, and add each listening object id to a list. The objects can remove themselves from the list in __del__, and when the list is empty the thread can be stopped.

Thanks for the feedback, the single thread for all groups is a good idea. Had a quick look at it but ran into some issues with the __del__ -method. It seems like it never gets called. Probably due to using strong (non-weak?) references for the listener registration, see for example here.

Yeah, as I understand, CPython doesn't provide any guarantee objects will necessarily be cleaned up in a timely manner. They should at least all be cleaned up with the interpreter is shutting down, though.

emlove · 2017-03-18T22:41:35Z

pychromecast/__init__.py

+        self.logger.info('Change in host ip detected, '
+                         'reinitializing object...')
+        stop_discovery(self.browser)
+        self.__init__(host, port=port, device=self.device, dynamic_host=True)


We definitely don't want to re-run __init__ when the host changes. Beyond being bad practice this is going to have a lot of unintended side-effects, such as dropping all the old kwargs passed in.

What we'll want to do is pull everything from init that needs to be re-run when the host changes into a separate method, and call that method in init, and here. (Probably everything from here down.)

You'll also need to clean-up the old socket client before creating the new one.

Fair enough. Out of interest, is calling the __init__-method not recommended due to some python under-the-hood magic that might go bad or is it just to avoid confusion?

What exactly is required for this?

You'll also need to clean-up the old socket client before creating the new one.

Same as in the __del__-method?

Yeah, that's pretty much it. AFAIK there isn't a technical reason it can't be called twice, it's just a confusing pattern.

For cleanup, you'll probably just need to call self.disconnect. You'll want to keep the default of blocking=True to ensure it closes successfully first.

balloob · 2017-03-19T18:37:03Z

I think that we should tackle it slightly different. When the group leader changes, it means that the old Chromecast object has been disconnected. We should listen for disconnect events and then try to reconnect, if that fails, try to see if we can find the same Cast Audio somewhere on the network and reconnect there. This should be handled transparently for the consumers of the Cast object.

New implementation should follow balloob's plan

Subscribing to connection ststaus messages from socket_client and allows for one reconnection attempt before starting to search forthe group on another ip on the network. If a new ip is found, re-initialize the chromecast object. If reconnection attemp succeds with old ip, stop discovery and do nothing.

freol35241 · 2017-03-24T07:04:21Z

pychromecast/__init__.py

+        """
+        Method for setting up, and resetting the chromecast object
+        """
+        tries = kwargs.pop('tries', None)


Another approach would be to default to a certain number (2?) of tries for a cast group and only listen to DISCONNECTED events. Might be a cleaner solution actually. What do you think?

…es for cast groups

freol35241 · 2017-03-25T19:14:43Z

Decided that listening for DISCONNECTED events was a cleaner approach. I am fairly happy with the solution now, and it seems to be stable.

balloob

This already looks a lot better. Really cool.

balloob · 2017-03-30T15:12:12Z

pychromecast/__init__.py

@@ -122,13 +129,14 @@ class Chromecast(object):
    :param retry_wait: A floating point number specifying how many seconds to
                       wait between each retry. None means to use the default
                       which is 5 seconds.
+    :param dynamic_host: Dynamic or non_dynamic host, if True dynamic host
+                         tracking is enabled. Will be True by default for
+                         cast_type == CAST_TYPE_GROUP.
    """

    def __init__(self, host, port=None, device=None, **kwargs):


Instead of storing kwargs, please write out all the attributes that we can have and their default values and store as instance variables.

def __init__(self, host, port=None, device=None, *, tries=None, timeout=None, retry_wait=None, blocking=True): self._tries = tries self._timeout = timeout

balloob · 2017-03-30T15:13:24Z

pychromecast/__init__.py

+
+        self.setup(host, port, self.device, self.kwargs)
+
+    def setup(self, host, port, device, kwargs):


Why take in port if that will always be equal to self.port ?

Same with kwargs and device

balloob · 2017-03-30T15:15:28Z

pychromecast/__init__.py

+        """
+        Method for setting up, and resetting the chromecast object
+        """
+        tries = 2 if self.cast_type == CAST_TYPE_GROUP \


Why is tries bigger for a group? We should store these values in the constructor.

Also, please rewrite as normal if statement instead of inline if statement. It is confusing to read.

Tries is None by default for all other CAST_TYPES meaning that the socket client will try to reconnect forever. By defining a finite number of tries we make sure that the socket client will eventually fail (when ip is switched) and then sends a DISCONNECTED event which we listen for.

balloob · 2017-03-30T15:16:05Z

pychromecast/__init__.py

@@ -194,6 +217,13 @@ def __init__(self, host, port=None, device=None, **kwargs):
        if blocking:
            self.socket_client.start()

+        # Set to True by default if cast_type == CAST_TYPE_GROUP
+        self.dynamic_host = kwargs.pop('dynamic_host', True if


Can be set in the constructor.

balloob · 2017-03-30T15:16:27Z

pychromecast/__init__.py

@@ -122,13 +129,14 @@ class Chromecast(object):
    :param retry_wait: A floating point number specifying how many seconds to
                       wait between each retry. None means to use the default
                       which is 5 seconds.
+    :param dynamic_host: Dynamic or non_dynamic host, if True dynamic host


Can you explain what dynamic host tracking is in the comment.

balloob · 2017-03-30T15:18:35Z

pychromecast/__init__.py

+                self.disconnect()
+                self.setup(self.host, self.port, self.device, self.kwargs)
+
+        self.logger.info('Dynamic host discovery started')


Can you include the name of the cast device?

balloob · 2017-03-30T15:19:56Z

pychromecast/__init__.py

+        timeout = kwargs.pop('timeout', None)
+        retry_wait = kwargs.pop('retry_wait', None)
+        blocking = kwargs.pop('blocking', True)
+
        self.socket_client = socket_client.SocketClient(


Socket client will automatically try to reconnect. After re-discovery, we should make sure we stop the old socket client or else it will keep reconnecting in the background.

As the number of tries is a finite number the old socket client will not try to reconnect forever, also it is stopped here

balloob · 2017-03-30T15:20:51Z

pychromecast/__init__.py

+        self.host = host
+        try:
+            self.setup(host, port, self.device, self.kwargs)
+        except ChromecastConnectionError:  # noqa


Why noqa ?

To be fair, not sure. But flake8 throws an error otherwise.

I guess it has something to do with this noqa?

noqa disabled the linter. What is the error it raises? Why can't it be fixed?

I can be fixed by removing the
from .error import *
in the beginning of the file. Not sure way that import is there though. Do you have any guess?

balloob · 2017-03-30T15:22:19Z

pychromecast/__init__.py

+            """
+            Internal scheduled timeout for discovery, 5 seconds
+            """
+            time.sleep(10)


Any reason why we would wait and not just start discovery right away? The function doc also seems to state 5 seconds?

Instead of starting a thread that sleeps for 10 seconds, consider using a Timer

Its a timeout to not let the discovery session run forever if no new ip is found.

A timer is a good idea, did not know about that one.

balloob · 2017-03-30T15:22:42Z

pychromecast/__init__.py

+            if not self.browser:
+                return  # Change in ip detected, discovery did not time out
+
+            self.logger.info('Discovery timed out, stopping')


You use logger.info a lot. I think that for example this one should be reduced to debug level.

balloob · 2017-04-12T03:06:31Z

Sorry, I have been very busy lately. This PR is still on my radar. Will look at it soon.

In the meanwhile, I just came by a dump of the output of the cast discovery. I wonder that instead of looking at port numbers, we can look at the 'rm' property?

To show discovery info on your network, run:

pip3 install netdisco==1.0.0rc2
pip3 -m netdisco

freol35241 · 2017-05-02T16:39:11Z

Sorry, have been busy with other commitments. Will try to find some time soon to finish this PR.

Had a look at the netdisco output on my network and it seems like the rm property is false for all the chromecasts?

balloob · 2017-05-15T07:14:18Z

Not sure if that is correct. My Google Home has an rm

google_cast:
[{'host': '192.168.1.109',
  'hostname': '92f538f5-433e-3500-075f-1f8ae1c92467.local.',
  'port': 8009,
  'properties': {'bs': 'FA8FCA87650A',
                 'ca': '4100',
                 'fn': 'Living room Home',
                 'ic': '/setup/icon.png',
                 'id': '92f538f5433e3500075f1f8ae1c92467',
                 'md': 'Google Home',
                 'rm': '4E05B1EDB9474484',
                 'rs': False,
                 'st': '0',
                 've': '05'}}]

foxel · 2017-08-31T07:07:25Z

Hi. Is there any chance this PR will be incorporated some way?

balloob · 2017-09-04T14:53:02Z

Doesn't look like it. Will close it for now as it has gone stale.

foxel · 2017-09-05T03:10:39Z

@balloob is there any plan for implementing this feature?

I see this lack of cromecast leader tracking as a blocker for home-assistant cast integration to be usable for groups.

balloob · 2017-09-05T15:31:41Z

No plans here, I'm busy as it is. You're welcome to implement it and open a PR to contribute it to pychromecast.

freol35241 added 3 commits March 18, 2017 22:27

Using zeroconf discovery for tracking host ip changes of cast groups.

5346a67

Fixing pylint errors

d1fc668

Fixing flake8 errors

ea14b4f

emlove previously requested changes Mar 18, 2017

View reviewed changes

freol35241 added 3 commits March 23, 2017 21:23

Fixing pylint and flake8 errors

df85897

Cleaning up a bit

273e5ee

freol35241 commented Mar 24, 2017

View reviewed changes

change to listen to DISCONNECTED events, defaults to 2 connection tri…

d57f106

…es for cast groups

balloob requested changes Mar 30, 2017

View reviewed changes

balloob closed this Sep 4, 2017

This was referenced Oct 12, 2017

pychromecast.error.NotConnected: Chromecast is connecting... home-assistant/core#9800

Closed

pychromecast.error.NotConnected: Chromecast is connecting... home-assistant/core#7782

Closed

michaelarnauts mentioned this pull request Mar 15, 2018

cast groups do not show p in docker without net=host home-assistant/core#13239

Closed

GeekVisit mentioned this pull request Sep 3, 2018

Error if google home is offline harperreed/google-home-notifier-python#5

Open

jimtng mentioned this pull request May 6, 2019

[chromecast] Chromecast audiogroup ever changing elected leader openhab/openhab-addons#5564

Closed


		self.setup(host, port, self.device, self.kwargs)

		def setup(self, host, port, device, kwargs):

Dynamic host ip tracking for cast groups #167

Dynamic host ip tracking for cast groups #167

Conversation

freol35241 commented Mar 18, 2017

emlove left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balloob commented Mar 19, 2017

Choose a reason for hiding this comment

freol35241 commented Mar 25, 2017

balloob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balloob commented Apr 12, 2017

freol35241 commented May 2, 2017

balloob commented May 15, 2017

foxel commented Aug 31, 2017

balloob commented Sep 4, 2017

foxel commented Sep 5, 2017

balloob commented Sep 5, 2017