Skip to content
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.

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

Add new feature to Apple TV platform #8122

Merged
merged 7 commits into from Jul 5, 2017

Conversation

Projects
None yet
6 participants
@postlund
Copy link
Contributor

commented Jun 20, 2017

Description:

This PR updates pyatv to 0.3.2 and adds new features, mainly:

  • Support for "stop"
  • Remote control buttons (e.g. menu, arrow keys)
  • Device authentication needed for play_media on tvOS 10.2+
  • Scanning for devices

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#2858

Example entry for configuration.yaml (if applicable):

apple_tv:
  - name: Apple TV
    host: 10.0.10.20
    login_id: 00000000-1234-5678-9012-345678901234
    start_off: true
    credentials: 8660DEA5154FB46B:20B94847926112B3F46F85DB3A7311830463BF65570C22C3786E27F38C3326CF

credentials is new and user by the device authentication feature.

Checklist:

If user exposed functionality or configuration variables are added/changed:

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.
  • New files were added to .coveragerc.
@pvizeli

This comment has been minimized.

Copy link
Member

commented Jun 21, 2017

It is only a question. Could be make sense to add this as remote platform?

@postlund

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2017

Been thinking a bit about what you said and it's starting to make sense. It would probably be a good idea. I assume you mean extracting apple_tv to its own component and then load a remote and a media_player platform when a new device is added, no?

@balloob

This comment has been minimized.

Copy link
Member

commented Jun 24, 2017

@postlund yep.

@postlund postlund force-pushed the postlund:atv_lift branch to 1d4056f Jun 26, 2017

@postlund

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2017

@pvizeli @balloob I have finished the code changes by converting it to a component with two platforms now now. Just have to update documentation which will require some rearrangement.

@balloob

This comment has been minimized.

Copy link
Member

commented Jun 27, 2017

Can you update .coveragerc and re-run script/gen_requirements_all.py ?

@balloob

This comment has been minimized.

Copy link
Member

commented Jun 27, 2017

Also, please include in the description what the breaking change entails so I can copy paste that to the release notes.

vol.Optional(CONF_NAME, default=DEFAULT_NAME): cv.string,
vol.Optional(CONF_START_OFF, default=False): cv.boolean
})
def request_configuration(hass, config, atv, credentials):

This comment has been minimized.

Copy link
@balloob

balloob Jun 27, 2017

Member

Is authentication something that we should move to the component too?

This comment has been minimized.

Copy link
@postlund

postlund Jun 27, 2017

Author Contributor

It probably can, but it depends on how we want to handle entity lookups in the service handler. Since entity ids passed are passed to apple_tv_authenticate (like with all services), the entity objects must be saved in the media_player platform (which is done now) but retrieved from the component. Not an implementation problem in any way, more a technical thing if it's "OK" to have the component-platform-coupling this way?

This comment has been minimized.

Copy link
@balloob

balloob Jun 30, 2017

Member

Yes, coupling between platforms and components that share auth is fine as long as they share the same name 👍

postlund added some commits Jun 20, 2017

Lift Apple TV to pyatv 0.3.2
Update code to use basic new features.
Convert Apple TV to a component
A media_player platform and a remote platform will be loaded for each
manually configured or discovered device.

@postlund postlund force-pushed the postlund:atv_lift branch from 1d4056f to 2204803 Jul 3, 2017

@postlund

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2017

I have moved the device authentication service to the component now and fixed the other remarks. Hopefully everything will pass now. As soon as I get a few minutes over I will try to fix the documentation and write description about the breaking change. Feel free to leave comments on the code until I have time to finish the rest!

@postlund

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2017

I added an additional service for scanning for devices. It makes it easier for user to find them and the login_id as they don't have to use atvremote from CLI.

@balloobbot balloobbot added the component label Jul 3, 2017

@postlund postlund force-pushed the postlund:atv_lift branch Jul 3, 2017

homeassistant/components/apple_tv.py Outdated

devices = []
for atv in atvs:
login_id = '00000000-1234-5678-9012-345678901234' #atv.login_id

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jul 3, 2017

at least two spaces before inline comment
inline comment should start with '# '

@postlund postlund force-pushed the postlund:atv_lift branch Jul 3, 2017

@postlund postlund force-pushed the postlund:atv_lift branch to a4d2952 Jul 3, 2017

@postlund

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2017

Everything should be in place now (including documentation). A suitable text for breaking changes might be along the lines:

Previously, Apple TV support was implemented as a media_player. It has now been converted into a component and must be configured as such:

apple_tv:
  - host: 10.0.10.201
    name: Apple TV 1
    login_id: 00000000-1234-5678-9012-345678901234)
  - host: 10.0.10.202
    name: Apple TV 2
    login_id: 00000000-1234-5678-9012-345678901432)
@pvizeli

pvizeli approved these changes Jul 4, 2017

Copy link
Member

left a comment

Looks good to me. @balloob can you also verify that?

@balloob

balloob approved these changes Jul 5, 2017

@balloob balloob merged commit ea5bec3 into home-assistant:dev Jul 5, 2017

4 checks passed

cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.0003%) to 93.609%
Details
hound No violations found. Woof!

@balloob balloob referenced this pull request Jul 13, 2017

Merged

0.49 #8468

@postlund postlund deleted the postlund:atv_lift branch Jul 14, 2017

@Fogh Fogh referenced this pull request Jul 17, 2017

Closed

Fix Apple TV breaking changes #3

dethpickle added a commit to dethpickle/home-assistant that referenced this pull request Aug 18, 2017

Add new feature to Apple TV platform (home-assistant#8122)
* Lift Apple TV to pyatv 0.3.2

Update code to use basic new features.

* Support button presses in Apple TV

* Support device authentication

* Convert Apple TV to a component

A media_player platform and a remote platform will be loaded for each
manually configured or discovered device.

* Move device auth to apple_tv component

* Update requirements and coverage config

* Add scan support to apple_tv

@home-assistant home-assistant locked and limited conversation to collaborators Oct 20, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.