Enable use of different ADB commands depending on the device properties

[ollo69]

This PR is related to issue #271.
I changed the _fill_in_commands method that now is generic in BaseTV. Method now works in this way:

• load default commands (old androidtv) defined as constants
• based on device_class provided during initialization or detected by device properties, replace default commands with specific command defined in a new file called devices_definition.json. This new file can be used for:
  • define new device class
  • define the rule to detect the new class when class is defined as auto
  • replace the default commands with specific command related to the device class

The file is configured to support firetv, chromecast (old android version) and android11 devices based on information get in related issue #271, but I could only test it on firetv because I don't have other android devices.

There are a lot of file changed, but main changes are in basetv.py. All other changes are just little adjustment to use the command loaded during the call to _fill_in_commands and to add the parameter device_class on initilization.

[FlagX]

Hi, thank you for investing time in this. I tested it on a fire tv cube and it seems to work. It can detect the playing state for netflix but it seems not to work with amazon prime for me. Should this work at the moment?

[ollo69]

I think that fireTV need some tuning with last software release. This is the reason for having the json file that can contain correct command and introduce new devices class to not break compatibility with older devices. For instance with my fireTV I discovered that the right command to get running application is ps -A | grep u0_a and not ps | grep u0_a. You should play with JSON file to discover right command, that we can create a new device class to support this.
Of course a little change HomaAssistant side have to be done to allow provide different device classes in configuration, but this will be a very simple change to introduce when we will be ready.

[JeffLIrion]

Thanks for taking the initiative on this.

Let me start by saying that I have very little time these days to work on this project. As such, (1) small & simple pull requests are much easier to get merged in and (2) I rely heavily on the unit tests, test coverage, and linting checks that CI performs.

General comments about this pull request:

1. I strongly believe that before changing any of the commands used, the get_properties method should call multiple methods to get the power, current app, state, etc., rather than sending one huge and complicated command and parsing the result (Split update() / get_properties() into multiple ADB commands #255).
2. I don't see a benefit to reading commands from a JSON file. I think it would be easier to simply import them from a .py file. If you want, you could make struct-like classes for storing the commands on a per-device basis, as you've done in the JSON file.
3. I would prefer the commands be stored as hidden attributes rather than in a dictionary.

[ollo69]

Got your point. Not so simple to keep this PR small, because is impacting the logic that the library is implemented. Also not so simple for me to find the time to work on this, for sure only w.e are available.
I will work on your suggestion at point 1), so for the moment don't spend your time on this PR.
Related to unit tests, I agree with you, I know that must be fixed to work with new implementation, but I would have a first feed-back before working on it

[JeffLIrion]

Formatting the code using black as a separate pull request would help to minimize the diff. I created a pull request that does that: #272. I can merge that, if you'd like.

Also, refactoring the get_properties method so that it calls several other methods (#255) should preserve current behavior while making it easier to change future behavior. In addition, it eliminates the need for really complicated shell commands and parsing the results of those commands.

Then this task simply amounts to tailoring the ADB shell commands used by the various "Properties" methods (example).

As I see it, that is the cleanest path forward.

[ollo69]

Formatting the code using black as a separate pull request would help to minimize the diff. I created a pull request that does that: #272. I can merge that, if you'd like.

I think that have code black formatted would help a lot. So yes please, merge that PR.

Also, refactoring the get_properties method so that it calls several other methods (#255) should preserve current behavior while making it easier to change future behavior. In addition, it eliminates the need for really complicated shell commands and parsing the results of those commands.

I do not understand if you are working on this or just suggesting how to. I just would avoid to make same changes that you are working on.

[JeffLIrion]

I do not understand if you are working on this or just suggesting how to. I just would avoid to make same changes that you are working on.

I'm not working on it.

[barmazu]

Hi @ollo69, et al.
By the way the ADB commands you used, they will work fine on both Android 9 & 11.
At least to the point I was able to test them (they seem to work fine on Android 9 Shield TV 2015 and Android 11 TCL TV).
Putting them into "Android 11" dictionary makes no sense at this point. As far as I understand current code and author intention - a major redesign is needed, I mean from the ground up. Really extensive task... (due to lack of time recently I'm not working on this, if you ask), for now I'm pretty happy with just updated ADB commands variables 👍 - simply - it just works (for me). Maybe putting them into the current code will not hurt anybody? More testing in real world would be nice, I guess (they are, after all, entry point to make things work in this module regardless of its architecture).

Thanks & Merry X-mas

[bosche83]

This PR is related to issue #271.
I changed the _fill_in_commands method that now is generic in BaseTV. Method now works in this way:

• load default commands (old androidtv) defined as constants
• based on device_class provided during initialization or detected by device properties, replace default commands with specific command defined in a new file called devices_definition.json. This new file can be used for:
  • define new device class
  • define the rule to detect the new class when class is defined as auto
  • replace the default commands with specific command related to the device class

The file is configured to support firetv, chromecast (old android version) and android11 devices based on information get in related issue #271, but I could only test it on firetv because I don't have other android devices.

There are a lot of file changed, but main changes are in basetv.py. All other changes are just little adjustment to use the command loaded during the call to _fill_in_commands and to add the parameter device_class on initilization.

[JeffLIrion]

In the time since this pull request was opened, I have:

• Formatted the code using black
• Split the get_properties() methods into multiple calls to other methods
• Implemented a framework for customizing ADB commands, as well as tailoring ADB commands based on the device properties

The bad news is that this pull request is very outdated.

The good news is that these changes could be made much more easily and/or support for Android 11 (or any OS) can be achieved by enabling users to specify ADB shell commands themselves via the customize_command() method:

python-androidtv/androidtv/basetv/basetv.py

Lines 102 to 117 in 26a4d74

	def customize_command(self, custom_command, value):
	"""Customize a command used to retrieve properties.
	Parameters
	----------
	custom_command : str
	The name of the command that will be customized; it must be in `constants.CUSTOMIZABLE_COMMANDS`
	value : str, None
	The custom ADB command that will be used, or ``None`` if the custom command should be deleted
	"""
	if custom_command in constants.CUSTOMIZABLE_COMMANDS:
	if value is not None:
	self._custom_commands[custom_command] = value
	elif custom_command in self._custom_commands:
	del self._custom_commands[custom_command]

I don't know much of anything about config flow in HA, but I think you could add options for configuring these commands:

python-androidtv/androidtv/constants.py

Lines 37 to 51 in 26a4d74

	# Customizable commands
	CUSTOM_CURRENT_APP = "current_app"
	CUSTOM_CURRENT_APP_MEDIA_SESSION_STATE = "current_app_media_session_state"
	CUSTOM_LAUNCH_APP = "launch_app"
	CUSTOM_RUNNING_APPS = "running_apps"
	CUSTOM_TURN_OFF = "turn_off"
	CUSTOM_TURN_ON = "turn_on"
	CUSTOMIZABLE_COMMANDS = {
	CUSTOM_CURRENT_APP,
	CUSTOM_CURRENT_APP_MEDIA_SESSION_STATE,
	CUSTOM_LAUNCH_APP,
	CUSTOM_RUNNING_APPS,
	CUSTOM_TURN_OFF,
	CUSTOM_TURN_ON,
	}

[ollo69]

Sorry guys,

I was busy from when I open this PR and happy to see that @JeffLIrion find time to work on this theme.

I don't know much of anything about config flow in HA, but I think you could add options for configuring these commands:

I think that we can add an option flow to manage this in config flow, something very similar to the wizard used used to create app list. Just not totally sure that HA teams like this approach. I will analyze this method and try to work HA side.

I think this PR does not make anymore sense, can we close this?

[JeffLIrion]

I think this PR does not make anymore sense, can we close this?

Yeah, I'll close it.

Just not totally sure that HA teams like this approach.

Why do you think they wouldn't like this approach?

Of course, it's not as nice as if this package could use the right commands for every device and OS under the sun. But that's just not possible. By all means, contributions are welcome and it would be nice to have those device-specific _cmd_* methods utilize the device properties and automagically provide the right commands. But that won't happen unless people contribute those fixes.

State detection is the same way: it's not possible to work for every app on every device, so this package provides a way for users to configure their own state detection logic.

Without the ability to customize ADB commands, users could be stuck with a broken integration for a month or longer until it gets fixed here and a new HA release gets published.

[ollo69]

Why do you think they wouldn't like this approach?

Maybe they can consider not correct allow user to define command, but maybe not, I agree with you that detection rule is the same approach, and they are there in option flow. I'll implement this in config flow, we will see feedback in review...