Android TV Remote integration#89935
Conversation
Lash-L
left a comment
There was a problem hiding this comment.
Just a few pieces of feedback
That being said - You may want to decrease the size of this PR if you want maintainers to review it.
https://developers.home-assistant.io/docs/review-process#creating-the-perfect-pr
Potentially remove zeroconf and anything else you can think of that isn't necessary for a initial release.
Secondly, I'm not sure what the maintainers will think about adding another integration for android tv. Is there no chance of you just taking the improvements you have and applying it to the already existing android tv - I'm sure that is what maintainers are going to prefer.
Yes I'm aware of the preference on small PRs. I prefer them myself at work too. I considered extracting the diagnostics in a separate PR but that is essentially just 5 lines. I didn't consider separating zeroconf since it's an important feature of this integration. If I do it I could maybe reduce it by 20 lines or so of non testing code. I prefer keeping this as a separate integration. Code would be too complicated having the existing integration support both for no added benefit that I can see. In addition in the past I was told to create a new integration in #82188 I believe the same applies here since it's using a different api. |
|
Great work 👍. Tested, everything works. This could potentially be a good alternative to the current Android TV integration. So I would like to suggest a few things:
|
Thanks for testing and confirming it works for you. What devices did you test it with? I have tested it with Nvidia Shield TV, Chromecast with Google TV and a TV with integrated Google TV.
|
Works well with my Sony Bravia 55XH9096 on Android TV 10 and XGIMI Elfin Projector on Android TV 11.
The media players may not have a playback state, and can be labeled as I think that if this integration will offer a media player, then they will be perfectly combined with the Cast integration through the universal integration. Not all applications return |
Of course ideally if this integration is called |
So why not implement that in the existing android tv integration? Adding another integration that does the same sounds super confusing to an end-user? To me that sounds like a great optimization of the existing integration?
I'm not sure if I agree. It is making code simpler at the cost of the user experience. |
|
I don't see a clean way adding this functionally in the existing integration that will be easy to maintain. They are using different APIs. The existing one with ADB is more flexible and powerful but harder to setup for average users. This is using an API limited to remote support, thus the name. Having both implementations in the same integration might actually cause user confusion since they will get different functionality based on whether they have done the extra step to enable developer tools or the extra step of pairing with PIN. Thinking about it the config flow will be a mess to support both. @balloob in the past, see #82188 when I wanted to add the ability to send commands to Google Assistant in the existing integration to avoid user confusion with similarly named integrations, said to create a new integration since it's a different API and not let frontend concerns drive backend decisions. I believe the same applies here. |
Yes, that is to make things easier when different type of devices are used, however, that is not the case here. This is adding the exact same functionality again. Brands help a lot. However, a user still has to be able to make a choice. I think this will worsen it a lot. I don't see how this is helping with any confusion. I don't care if it is backend or frontend driven. The UX counts, at any point in time (no matter the origin). |
|
@Drafteed suggested earlier to rename the existing one to Android ADB. Would that help? Can you clarify how having two integrations would be confusing to the user? Most users will likely just install this since it supports discovery and no advanced extra steps such as enabling developer tools. I also assume most users are interested in remote control, powering on/off, maybe launching apps and not media player support since the cast integration already adds such functionally for Android TV devices. So regulars users don't even need to know the existing integration exists. Advanced users that want more functionally (e.g. screen capture, sending any adb command) can install the existing integration. Having just one integration might be more confusing to regular users since they won't know how to enable developer tools to get the extra functionality and they might come across a post or video with the extra functionality and they will be wondering why they don't have it when they already have the integration installed. Having separate integrations I think makes the distinction more clear. The new one only gives TV remote functionally, the existing one gives the same TV remote functionality (using a different implementation) among much more. To your point about having two different integrations having similar functionally, the Google Cast and Android TV integrations both add a media player entity for Android TV devices. |
|
@frenck The current integration of Android TV is not really related directly to Android TV. The integration simply polls the debug interface and can work with any Android device such as phones, tablets or wall panels. Some users use this integration with TV Boxes on native Android (not Android TV). In turn, this integration works exclusively with Android TV devices, as it interacts with a special native service called Android TV Remote Service, which exists only in real Android TV devices. Moreover, this integration uses At the same time, as I wrote above, I think the old integration should have been called |
|
A very good point in your comment, that I forgot, is that the the two integrations are of different connection class: local push vs local polling. If we were to combine them, what connection class would it be? |
|
I'm not commenting here as a user who just started with integrating Google Cast / Android TV into my System AMD trying different "remote Control" solutions available in hacs at the Moment. Honestly speaking - I don't Care If this will be integrated into the existing Integration, or will be available as another one... as Long, as it would improve the ux at all. My current concern is, that my Android TV device seems to have issues with the Android TV Integration at all - ist dies Not Show Playback Information & it does Not Switch between Apps / sources. The Cast seems to provide those Information - at least, for most - but not for all Apps. In Addition to that: So... Finally: In Terms of different Communication Methods, APIs I so agree with the Idea to have it as another / separate Integration. |
|
Let's rename the existing |
|
Sounds good. Doing that in #90657 and home-assistant/home-assistant.io#26830 |
balloob
left a comment
There was a problem hiding this comment.
Looks great. 2 tiny comments, otherwise good to go.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Co-authored-by: Paulus Schoutsen <paulus@home-assistant.io>
|
Thanks for the review and apologies for the PR size. Given this is using discovery and a lot of users have Android TV devices I tried my best to follow best practices and aimed for platinum quality from the beginning. |
|
@tronikos I have found that when the Android TV device is physically turned off, the entity in the HA remains on. Moreover, it looks like the connection is not being closed/recreated, and if the device comes back online, the next command will be ignored (as the reconnect process starts). |
|
Do you have more than one Android TV device? Is this specific to a particular device? The three different devices I have all report off as soon as I physically turn them off. Maybe some devices don't really turn off or don't report they are off? |
|
This behavior is observed on all my three devices:
|
I physically unplugged them from power. |
|
Thanks Paulus! |
Proposed change
Implement Android TV Remote integration that allows sending commands and launching apps on an Android TV. This contrary to the existing Android TV integration doesn't require ADB and enabling developer tools and it's much faster in executing commands. In addition it supports zeroconf discovery. The only step needed is for the user to enter the PIN shown on the Android TV once they initiate pairing.
Discovery:

Pairing:

Once installed you can add buttons in lovelace to execute commands:

I marked it platinum from the start since I think it will make it easier to justify it during the review. Test coverage is 100%. For reference the requirements are below where I checked all of them except the non applicable ones:
Silver 🥈
This integration is able to cope when things go wrong. It will not print any exceptions nor will it fill the log with retry attempts.
Set an appropriateSCAN_INTERVAL(if a polling integration)RaisePlatformNotReadyif unable to connect during platform setup (if appropriate)ValueErroron invalid user input and raiseHomeAssistantErrorfor other failures such as a problem communicating with a device.availableproperty toFalseif appropriate (docs)Gold 🥇
This is a solid integration that is able to survive poor conditions and can be configured via the user interface.
ConfigEntryNotReadyif unable to connect during entry setup (if appropriate)Entities only subscribe to updates insideasync_added_to_hassand unsubscribe insideasync_will_remove_from_hass(docs)Entities have correct device classes where appropriate (docs)and leveragesEntity.entity_registry_enabled_defaultto disable less popular entities (docs)If the device/service API can remove entities, the integration should make sure to clean up the entity and device registry.Platinum 🏆
Best of the best. The integration is completely async, meaning it's super fast. Integrations that reach platinum level will require approval by the code owner for each PR.
PARALLEL_UPDATESconstantUses aiohttp or httpx and allows passing in websession (if making HTTP requests)Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.To help with the load of incoming pull requests: