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 ADB server functionality to Fire TV #21221

Merged
merged 10 commits into from Feb 24, 2019

Conversation

Projects
None yet
4 participants
@JeffLIrion
Copy link
Contributor

JeffLIrion commented Feb 19, 2019

Description:

Allow for using an ADB server instance, such as the Hass.io ADB addon, with the Fire TV component.

This is a breaking change, as it removes the get_source configuration option.

This includes the changes in #21131.

Related issue (if applicable):

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

Example entry for configuration.yaml (if applicable):

media_player:
  - platform: firetv
    name: Fire TV
    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.

If the code does not interact with devices:

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

@wafflebot wafflebot bot added the in progress label Feb 19, 2019

@JeffLIrion JeffLIrion referenced this pull request Feb 19, 2019

Merged

Add ADB server functionality to the Fire TV component #8641

0 of 2 tasks complete

@JeffLIrion JeffLIrion closed this Feb 20, 2019

@JeffLIrion JeffLIrion reopened this Feb 20, 2019

@wafflebot wafflebot bot added in progress and removed in progress labels Feb 20, 2019

@JeffLIrion JeffLIrion referenced this pull request Feb 22, 2019

Closed

Bump firetv to 1.0.8 #21131

3 of 9 tasks complete
@JeffLIrion

This comment has been minimized.

Copy link
Contributor Author

JeffLIrion commented Feb 22, 2019

@rytilahti, @bachya, @MartinHjelmare could I please get a review for this pull request. I've tested the code, as have a number of users on the forum, and it works fine. The significance of this pull request is that by allowing the Fire TV component to use an ADB server, such as @frenck's addon, it enables the component to work for newer devices that the Python ADB implementation can't handle. It would be great to have this functionality in the built-in component instead of users needing to deal with a custom component and its dependencies.

@rytilahti
Copy link
Contributor

rytilahti left a comment

Nice to see device specific parts being moved out into the library!

I tested this briefly with my firetv stick and it seems to be working fine, I'm not sure if the active source ever worked, but I'm receiving this warning ([root] Couldn't get current app, reply was mCurrentFocus=null). As I don't have adbserver instance around, I'll leave testing that to someone else.

Show resolved Hide resolved homeassistant/components/media_player/firetv.py Outdated
Show resolved Hide resolved homeassistant/components/media_player/firetv.py Outdated
@JeffLIrion

This comment has been minimized.

Copy link
Contributor Author

JeffLIrion commented Feb 22, 2019

@rytilahti thanks for the review! I can change those comments later.

Here's the line in the firetv package that is responsible for the warning you got: https://github.com/happyleavesaoc/python-firetv/blob/cda9215dcb41c885ffd4075f55e274284b585d87/firetv/__init__.py#L587

The command executed successfully, but the output wasn't what it expected. I don't know enough about the Fire TV inner workings to say why it didn't work. I could:

  • Log the message at a lower level, such as debug.
  • Not log anything.
  • Leave it as is -- it seems like a reasonable thing to warn a user about, and if they don't like it they can always change their logging level.

Any preference?

@JeffLIrion

This comment has been minimized.

Copy link
Contributor Author

JeffLIrion commented Feb 23, 2019

This needs a breaking change label, since it removes the get_source configuration option.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Feb 23, 2019

Let me know when we're happy and I can merge.

@JeffLIrion JeffLIrion changed the title Add ADB server functionality to Fire TV component Add ADB server functionality to Fire TV component; remove 'get_source' config entry Feb 23, 2019

@JeffLIrion

This comment has been minimized.

Copy link
Contributor Author

JeffLIrion commented Feb 23, 2019

@rytilahti any preference on how to handle that warning message you got?

@rytilahti

This comment has been minimized.

Copy link
Contributor

rytilahti commented Feb 24, 2019

@JeffLIrion I don't know why the current focus was null in that case, I thought it could have been due to a popup from an app asking whether to continue where it was left the last time or to start the episode from the beginning. However, I just tested it out manually with dumpsys window windows | grep mCurrentFocus and it seems to deliver correct results now.

I suppose there is not much to be done in case the device does not report the active focus for a reason for another -- displaying a warning in the logs is fine for me.

@JeffLIrion

This comment has been minimized.

Copy link
Contributor Author

JeffLIrion commented Feb 24, 2019

@rytilahti I removed that line from the firetv package and bumped the version.

@JeffLIrion

This comment has been minimized.

Copy link
Contributor Author

JeffLIrion commented Feb 24, 2019

@MartinHjelmare, it's ready to be merged!

@MartinHjelmare MartinHjelmare changed the title Add ADB server functionality to Fire TV component; remove 'get_source' config entry Add ADB server functionality to Fire TV Feb 24, 2019

@MartinHjelmare MartinHjelmare merged commit ff93cdb into home-assistant:dev Feb 24, 2019

4 checks passed

Hound No violations found. Woof!
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-pr2 at 92.726%
Details

@wafflebot wafflebot bot removed the in progress label Feb 24, 2019

@JeffLIrion

This comment has been minimized.

Copy link
Contributor Author

JeffLIrion commented Feb 24, 2019

Thanks, @MartinHjelmare! I've got one more pull request: #21419

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.