Skip to content

Use push updates in Apple TV#6323

Merged
balloob merged 2 commits into
home-assistant:devfrom
postlund:atv_push
Mar 2, 2017
Merged

Use push updates in Apple TV#6323
balloob merged 2 commits into
home-assistant:devfrom
postlund:atv_push

Conversation

@postlund
Copy link
Copy Markdown
Contributor

@postlund postlund commented Mar 1, 2017

Description:

This basically removes the need to poll for device state changes.

Checklist:

If the code communicates with devices, web services, or third-party tools:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.

@mention-bot
Copy link
Copy Markdown

@postlund, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @fabaff and @rmkraus to be potential reviewers.

@balloob
Copy link
Copy Markdown
Member

balloob commented Mar 1, 2017

Please remove home-assistant-polymer from your changed files.

@postlund
Copy link
Copy Markdown
Contributor Author

postlund commented Mar 1, 2017

Ouch, missed that one. Removed now.

Copy link
Copy Markdown
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

I see a mix of async and sync methods being used. Please make sure you annotate methods correctly and don't mix them.

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.

Please add annotation @callback or @asyncio.coroutine (or, if this is not an async method, do not use hass.async_add_job but instead use hass.add_job.

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.

Should be a @callback, yes. It is called as a callback from an async method (think "future"), so everything it performs must be synced with event loop.

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.

Instead of starting this here, consider adding this to async_added_to_hass

@asyncio.coroutine
def async_added_to_hass(self):
    """Called when entity is about to be added to HASS."""
    self._atv.push_updater.start()

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.

Sure!

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.

Are you sure that you should not use asyncio.TimeoutError ?

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.

After some thinking I believe I can remove this altogether. It was some experimenting I worked with first but I don't think it's needed.

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.

Why do an update if you're not changing any of the internal state ?

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.

What, I must have missed something here. It's supposed to reset self._playing. I'll add that, good spotting!

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.

Is this supposed to be run inside a thread?

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.

I do not think so. Doing "stop" is basically cancel on a future and that is not thread-safe, so it must be run in the event loop. So it should be OK?

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.

Need to be add as @callback

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.

Are these methods async friendly ? (_set_power_off is marked as an async callback)

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.

Yes, they are from what I have gathered (with same reasoning as earlier regarding canceling a future - start effectively adds a future to the event loop).

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.

hass will be available as self.hass, no need to pass in reference yourself. If push_updater expects hass to be there, only make connection inside async_added_to_hass and you should be good 👍

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.

Ok, cool. Is this a new thing I have missed? I don't recognize async_added_to_hass 😄

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.

Yeah it's new, came in last release :)

@balloob balloob merged commit bae6333 into home-assistant:dev Mar 2, 2017
@balloob
Copy link
Copy Markdown
Member

balloob commented Mar 2, 2017

Niiice 🐬

@postlund
Copy link
Copy Markdown
Contributor Author

postlund commented Mar 2, 2017

Hmm, just pulled from dev and restarted. I get the following error in the log file now:

17-03-02 09:29:38 ERROR (MainThread) [homeassistant.components.media_player] Error while setting up platform apple_tv
Traceback (most recent call last):
  File "/home/pi/home-assistant/homeassistant/helpers/entity_component.py", line 142, in _async_setup_platform
    entity_platform.async_schedule_add_entities, discovery_info
  File "/home/pi/home-assistant/homeassistant/components/media_player/apple_tv.py", line 82, in async_setup_platform
    yield from async_add_entities([entity])
TypeError: 'NoneType' object is not iterable

I have not seen this before and how can that even happen?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants