-
-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
New Android TV media player integration #19157
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some general comments about the code, no testing on a real device.
|
||
|
||
ACTIONS = { | ||
"back": "4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This listing should definitely go into the backend library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to let it here because as it used for a service, there is a check to see if the requested action exists in the component. Also contributors can more easily add their actions in the component rather than in the backend lib.
} | ||
|
||
KNOWN_APPS = { | ||
"amazon": "Amazon Prime Video", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this too, or is there some reason for not doing so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same reasons, and also this is very specific to my component (the backend lib is made to work for the FireTV component too).
|
||
self._name = name | ||
self._apps = KNOWN_APPS | ||
self._apps.update(dict(apps)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understood correctly, this means that the KNOWN_APPS
cannot be overridden nor hidden anyhow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this just means that user can add their custom know apps in the configuration to be added to the KNOWN_APPS dict.
def media_play(self): | ||
"""Send play command.""" | ||
self.androidtv.media_play() | ||
self._state = STATE_PLAYING |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't recall right now if manual (assumed) state changes are preferred instead of waiting for the update()
to update the state, so you may need to look it up.
…roidTV.connect()' and 'AndroidTV.update()' methods This commit requires an updated `androidtv` package, which is on GitHub but not yet on pypi. If the code in this component and in the package look good, then I'll push the new version to pypi.
Don't access 'self.androidtv._available', get this info from the 'And…
Make 'self.exceptions' a tuple
Catch exceptions in 'adb_wrapper', not 'update'
Use 'atv.available' to avoid connecting twice
Tested on my Shield and got the following error: Thu Jan 10 2019 15:03:48 GMT-0500 (Eastern Standard Time)
Error while setting up platform androidtv
Traceback (most recent call last):
File "/srv/homeassistant/lib/python3.6/site-packages/homeassistant/helpers/entity_platform.py", line 128, in _async_setup_platform
SLOW_SETUP_MAX_WAIT, loop=hass.loop)
File "/usr/local/lib/python3.6/asyncio/tasks.py", line 358, in wait_for
return fut.result()
File "/usr/local/lib/python3.6/concurrent/futures/thread.py", line 56, in run
result = self.fn(*self.args, **self.kwargs)
File "/home/homeassistant/.homeassistant/custom_components/media_player/androidtv.py", line 181, in setup_platform
atv = AndroidTV(host, adbkey)
File "/srv/homeassistant/lib/python3.6/site-packages/androidtv/__init__.py", line 230, in __init__
self.properties = self.device_info()
File "/srv/homeassistant/lib/python3.6/site-packages/androidtv/__init__.py", line 327, in device_info
btmac = re.findall(BTMAC_REGEX_PATTERN, properties)[0]
IndexError: list index out of range |
I just bumped the version of |
Also testing on my Shield after changing the requirements of androidtv==0.05 I'm getting an error:
|
@bobokun using the Python
More info can be found in the forum thread: |
Reviewers, what's needed to get this merged? I submitted a couple PRs to @a1ex4 that bump the
|
Thanks! Using the adb server addon made the component work on my ShieldTV |
Yes, tried it with @frenck's addon and it worked with my Shield. |
# "pure-python-adb" | ||
atv = AndroidTV( | ||
host, | ||
adb_server_ip=config[CONF_ADB_SERVER_IP], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Question] Shouldn't you accept the adbkey for remote servers in the case someone is hosting ADB behind auth?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The adbkey
configuration entry is only needed when using the Python implementation of the ADB protocol (aka, the adb
package). When using an ADB server for ADB commands (such as the Hass.io addon), the adb instance is responsible for authenticating. Since that responsibility is outside the scope of this component, the adbkey
configuration entry is not relevant.
Component doeasn't work with HA 0.88b0.
|
This pull request should fix the issue: https://github.com/a1ex4/home-assistant/pull/8 In the meantime, this component should work (and be backwards compatible): https://github.com/JeffLIrion/home-assistant/blob/androidtv-backwards-compatible/homeassistant/components/media_player/androidtv.py |
@JeffLIrion Does it have to be placed in |
@arsaboo |
@JeffLIrion Yes, we are moving to a different folder structure. From the release notes for 0.88
If we use it under Thu Feb 14 2019 11:57:46 GMT-0500 (Eastern Standard Time)
Error loading custom_components.androidtv.media_player. Make sure all dependencies are installed
Traceback (most recent call last):
File "/srv/homeassistant/lib/python3.6/site-packages/homeassistant/loader.py", line 147, in _load_file
module = importlib.import_module(path)
File "/usr/local/lib/python3.6/importlib/__init__.py", line 126, in import_module
return _bootstrap._gcd_import(name[level:], package, level)
File "<frozen importlib._bootstrap>", line 994, in _gcd_import
File "<frozen importlib._bootstrap>", line 971, in _find_and_load
File "<frozen importlib._bootstrap>", line 955, in _find_and_load_unlocked
File "<frozen importlib._bootstrap>", line 665, in _load_unlocked
File "<frozen importlib._bootstrap_external>", line 678, in exec_module
File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
File "/home/homeassistant/.homeassistant/custom_components/androidtv/media_player.py", line 33, in <module>
from homeassistant.components.media_player import (
ImportError: cannot import name 'SUPPORT_TURN_OFF' and a warning about the folder change
|
@arsaboo I think it will work if you use the backwards compatible version (and possibly rename it as you mentioned): https://github.com/JeffLIrion/home-assistant/blob/androidtv-backwards-compatible/homeassistant/components/media_player/androidtv.py |
Any chance of including hassio-addons/addon-adb#6 in this? |
EDIT: It looks like it's just showing in the console, not actually hitting the logger. EDIT 2: All the debug messages are from the Looks like the
Every few seconds messages like this are printed in my HA log:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small comments, after that we can merge it
name: MIBOX3 | ||
adbkey: /config/adbkey | ||
apps: | ||
"amazon": "Amazon Premium Video" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleanup this part
CONF_ADB_SERVER_IP = 'adb_server_ip' | ||
CONF_ADB_SERVER_PORT = 'adb_server_port' | ||
|
||
DEFAULT_APPS = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove that
vol.Optional(CONF_NAME, default=DEFAULT_NAME): cv.string, | ||
vol.Optional(CONF_PORT, default=DEFAULT_PORT): vol.All(cv.port, cv.string), | ||
vol.Optional(CONF_ADBKEY): has_adb_files, | ||
vol.Optional(CONF_APPS, default=DEFAULT_APPS): dict, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default=dict()
Since #21944 has been merged we can close this, no? |
Description:
This component allows to use any Android TV / Android device as a media player in Home-Assistant.
Right now there are two ways to communicate with an Android device, through the ADB protocol: use the Python implementation of the protocol, or connect to an external ADB server that is connected to the Android device. Given the many different combinations of Android devices and machines hosting HA, both methods are implemented in this component, trying to make the experience as streamlined as possible.
This PR is no longer WIP 🙂
Thanks in advance for any input!
Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#7788
Example entry for
configuration.yaml
(if applicable):Checklist:
tox
. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
REQUIREMENTS
variable ([example][ex-requir]).requirements_all.txt
by runningscript/gen_requirements_all.py
..coveragerc
.