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

Rename 'firetv' to 'androidtv' and add Android TV functionality #21944

Merged
merged 13 commits into from Mar 13, 2019

Conversation

Projects
None yet
7 participants
@JeffLIrion
Copy link
Contributor

JeffLIrion commented Mar 11, 2019

Description:

This is a continuation of #21872.

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

Breaking Change

The firetv integration has been renamed to androidtv. Users will need to change - platform: firetv to - platform: androidtv in their configuration.

This is necessary to avoid having near-duplicate integrations for androidtv and firetv. It makes more sense to call the combined integration androidtv, hence the renaming.

Example entry for configuration.yaml (if applicable):

media_player:
  # Use the Python ADB implementation without authentication
  - platform: androidtv
    name: Android TV 1
    host: 192.168.0.111

  # Use the Python ADB implementation with authentication
  - platform: androidtv
    name: Android TV 2
    host: 192.168.0.222
    adbkey: "/config/android/adbkey"

  # Use an ADB server for sending ADB commands
  - platform: androidtv
    name: Android TV 3
    host: 192.168.0.123
    adb_server_ip: 127.0.0.1

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

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

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

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

JeffLIrion added some commits Mar 9, 2019

@JeffLIrion

This comment has been minimized.

Copy link
Contributor Author

JeffLIrion commented Mar 12, 2019

This is ready for review! Hopefully it can get merged in before the deadline for the next release!

@MartinHjelmare, @frenck, @rytilahti, @rohankapoorcom, could I please get a review.

@JeffLIrion JeffLIrion referenced this pull request Mar 12, 2019

Closed

New Android TV media player integration #19157

8 of 8 tasks complete
@pvizeli
Copy link
Member

pvizeli left a comment

Looks good. Some small comments

Show resolved Hide resolved homeassistant/components/androidtv/media_player.py Outdated
Show resolved Hide resolved homeassistant/components/androidtv/media_player.py Outdated

@pvizeli pvizeli self-assigned this Mar 12, 2019

@pvizeli
Copy link
Member

pvizeli left a comment

Small comment. Move services into __init__ and run it under that namespace. Remove this ANDROIDTV_DOMAIN thing on media_player

@JeffLIrion

This comment has been minimized.

Copy link
Contributor Author

JeffLIrion commented Mar 12, 2019

Small comment. Move services into __init__ and run it under that namespace. Remove this ANDROIDTV_DOMAIN thing on media_player

I'm a bit unclear on what you're saying here. Is the xiaomi_aqara platform an example of what you're talking about?

@pvizeli

This comment has been minimized.

Copy link
Member

pvizeli commented Mar 12, 2019

https://github.com/home-assistant/home-assistant/tree/dev/homeassistant/components/fastdotcom

With new data structure, we run service under that namespace

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Mar 12, 2019

@pvizeli the service is already registered under the component. It doesn't really matter what module in the component does the actual registration.

@bogdanalexe90

This comment has been minimized.

Copy link

bogdanalexe90 commented Mar 12, 2019

For me the volume_level is wrongly reported. The media player component expects a value between 0 and 1 but i received values up until 4 (max volume on my sound bar is 44).

@JeffLIrion

This comment has been minimized.

Copy link
Contributor Author

JeffLIrion commented Mar 12, 2019

For me the volume_level is wrongly reported. The media player component expects a value between 0 and 1 but i received values up until 4 (max volume on my sound bar is 44).

You should submit an issue in the androidtv repo.

Here's the line causing the issue: https://github.com/JeffLIrion/python-androidtv/blob/38a01f2ddba7aefb835391b1b3a756a84ed6e044/androidtv/androidtv.py#L330

It would be ideal if the max volume could be determined programmatically. Otherwise, it could be provided as an input (and as a config parameter for the HA component).

@awarecan

This comment has been minimized.

Copy link
Contributor

awarecan commented Mar 13, 2019

Please add a break change sections in top of PR description to explain what is break and why we have to break it.

@JeffLIrion

This comment has been minimized.

Copy link
Contributor Author

JeffLIrion commented Mar 13, 2019

Please add a break change sections in top of PR description to explain what is break and why we have to break it.

Done.

@JeffLIrion

This comment has been minimized.

Copy link
Contributor Author

JeffLIrion commented Mar 13, 2019

@pvizeli is it OK if I leave it as is and register the androidtv.adb_command service in media_player.py?

@JeffLIrion

This comment has been minimized.

Copy link
Contributor Author

JeffLIrion commented Mar 13, 2019

I don't know why the Hound check is taking forever. It passed earlier and it should pass now. Could someone please restart that test.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Mar 13, 2019

We should not change the registration of the ADB command service. It's good as is. Custom services should be registered under the embedding component as it's done here already.

@pvizeli pvizeli merged commit 007bf2b into home-assistant:dev Mar 13, 2019

3 of 4 checks passed

Hound Hound is busy reviewing changes...
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on firetv-androidtv2 at 92.829%
Details

@wafflebot wafflebot bot removed the in progress label Mar 13, 2019

@earth08

This comment has been minimized.

Copy link

earth08 commented Mar 13, 2019

Great, now we will see androidtv,
Working for all device.

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