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
Improve support for Sony TV devices #233
Conversation
…mber. To ensure consistent hdmi_input values between devices running Android TV, besides the fact that they do not match (e.g. port number 1 might equal HW5). Additionally hardware identifiers are more in compliance with the Android TV Input Framework. Each vendor can access the hardware definition as specified in tv_input module (TV Input HAL) through their implementation of the TV Input Manager service to access TV-specific hardware.
Can you please split off the app name constants (i.e., |
androidtv/constants.py
Outdated
|
||
#: Get the HDMI input | ||
CMD_HDMI_INPUT = "dumpsys activity starter | grep -o 'HDMIInputService\\/HW[0-9]' -m 1 | grep -o 'HW[0-9]'" | ||
CMD_HDMI_INPUT_SEARCH = r" | grep -E -o '(ExternalTv|HDMI)InputService/HW[0-9]' -m 1 | grep -o 'HW[0-9]'" |
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'd rather not have a constant that's only used once.
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.
Again, it seems Chrome on iOS failed me. I see that you also used it in a unit test.
Still, I don't think it's necessary to have this constant. Your unit test could simply strip off "dumpsys activity starter" from the command.
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.
Nice suggestion! Instead of stripping dumpsys activity starter
, I decided to replace dumpsys activity starter
with the dumpsys output as specified in test case.
@@ -28,20 +30,30 @@ | |||
#: Determine whether the device is awake | |||
CMD_AWAKE = "dumpsys power | grep mWakefulness | grep -q Awake" | |||
|
|||
#: Define current app | |||
CMD_DEFINE_CURRENT_APP = "CURRENT_APP=$(dumpsys window windows | grep -E 'mCurrentFocus|mFocusedApp') && " + VAR_CURRENT_APP_EXTRACTION |
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 constant seems unnecessary. Same goes for the other CMD_DEFINE_CURRENT_APP_*
constant.
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.
In which way do you see this as potentially unnecessary?
Besides the main objective to deduplicate the code, as the majority of the complexity is already defined in VAR_CURRENT_APP_EXTRACTION
.
The other use case I had in mind, using this module from a Python script and being able to use CURRENT_APP
(directly) in a custom command using adb shell without the additional (CMD_CURRENT_APP
) command to obtain the current_app
.
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.
My comments from earlier were based on looking at this pull request on my phone, and the "Find in page" functionality wasn't working right. So I do see that these constants are used and that they make the code a bit more DRY.
My main concerns are that
- Adding more constants makes the pull request harder to review -- it's hard to see what is actually being changed.
- Aside from this pull request, it's easier to read the code if the full command is spelled out:
# I need to check what `VAR_CURRENT_APP_EXTRACTION` does
CMD_DEFINE_CURRENT_APP = "CURRENT_APP=$(dumpsys window windows | grep -E 'mCurrentFocus|mFocusedApp') && " + VAR_CURRENT_APP_EXTRACTION
# Here I know exactly what the ADB shell command is doing
CMD_CURRENT_APP = "CURRENT_APP=$(dumpsys window windows | grep mCurrentFocus) && CURRENT_APP=${CURRENT_APP#*{* * } && CURRENT_APP=${CURRENT_APP%%/*} && echo $CURRENT_APP"
The other use case I had in mind, using this module from a Python script and being able to use
CURRENT_APP
(directly) in a custom command using adb shell without the additional (CMD_CURRENT_APP
) command to obtain thecurrent_app
.
The way I see it, anyone doing that is a power user. I think such a person would be more likely to look through these constants and piece together what they need in order to achieve their desired outcome OR simply formulate their ADB shell command from scratch rather than to pick out constants from this module.
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.
To assist with the review of the code, below is the diff of fully written commands by changed constant:
CMD_CURRENT_APP
-CURRENT_APP=$(dumpsys window windows | grep mCurrentFocus) && CURRENT_APP=${CURRENT_APP#*{* * } && CURRENT_APP=${CURRENT_APP%%/*} && echo $CURRENT_APP
+CURRENT_APP=$(dumpsys window windows | grep -E 'mCurrentFocus|mFocusedApp') && CURRENT_APP=${CURRENT_APP#*ActivityRecord{* * } CURRENT_APP=${CURRENT_APP#*{* * } && CURRENT_APP=${CURRENT_APP%%/*} && CURRENT_APP=${CURRENT_APP%\\}*} && echo $CURRENT_APP
CMD_CURRENT_APP_GOOGLE_TV
-CURRENT_APP=$(dumpsys activity a . | grep -E 'mResumedActivity' | cut -d ' ' -f 8) && CURRENT_APP=${CURRENT_APP%%/*} && echo $CURRENT_APP
+CURRENT_APP=$(dumpsys activity a . | grep mResumedActivity) && CURRENT_APP=${CURRENT_APP#*ActivityRecord{* * } CURRENT_APP=${CURRENT_APP#*{* * } && CURRENT_APP=${CURRENT_APP%%/*} && CURRENT_APP=${CURRENT_APP%\\}*} && echo $CURRENT_APP
CMD_HDMI_INPUT
-dumpsys activity starter | grep -o 'HDMIInputService\\/HW[0-9]' -m 1 | grep -o 'HW[0-9]'
+dumpsys activity starter | grep -E -o '(ExternalTv|HDMI)InputService/HW[0-9]' | grep -o 'HW[0-9]'
CMD_LAUNCH_APP
-CURRENT_APP=$(dumpsys window windows | grep mCurrentFocus) && CURRENT_APP=${{CURRENT_APP#*{{* * }} && CURRENT_APP=${{CURRENT_APP%%/*}} && if [ $CURRENT_APP != '{0}' ]; then monkey -p {0} -c " + INTENT_LAUNCH + " --pct-syskeys 0 1; fi
+CURRENT_APP=$(dumpsys window windows | grep -E 'mCurrentFocus|mFocusedApp') && CURRENT_APP=${{CURRENT_APP#*ActivityRecord{{* * }} CURRENT_APP=${{CURRENT_APP#*{{* * }} && CURRENT_APP=${{CURRENT_APP%%/*}} && CURRENT_APP=${{CURRENT_APP%\\}}*}} && if [ $CURRENT_APP != '{0}' ]; then monkey -p {0} -c " + INTENT_LAUNCH + ' --pct-syskeys 0 1; fi
CMD_LAUNCH_APP_GOOGLE_TV
-CURRENT_APP=$(dumpsys activity a . | grep -E 'mResumedActivity' | cut -d ' ' -f 8) && CURRENT_APP=${{CURRENT_APP%%/*}} && if [ $CURRENT_APP != '{0}' ]; then monkey -p {0} -c " + INTENT_LAUNCH + " --pct-syskeys 0 1; fi
+CURRENT_APP=$(dumpsys activity a . | grep mResumedActivity) && CURRENT_APP=${{CURRENT_APP#*ActivityRecord{{* * }} CURRENT_APP=${{CURRENT_APP#*{{* * }} && CURRENT_APP=${{CURRENT_APP%%/*}} && CURRENT_APP=${{CURRENT_APP%\\}}*}} && if [ $CURRENT_APP != '{0}' ]; then monkey -p {0} -c " + INTENT_LAUNCH + ' --pct-syskeys 0 1; fi
Note: If you plan to compare these commands via android.adb_command service in Home Assistant, be sure to remove any escaped characters (e.g. \\
, {{
, }}
) or to populate placeholders ({0}
).
In regard to having more constants/variables, by simply looking at the above diffs you can see there is a lot of duplicate complexity, this should simplify any future changes to CURRENT_APP
logic, as already mentioned in #233 (comment).
@@ -15,6 +15,8 @@ | |||
INTENT_LAUNCH = "android.intent.category.LAUNCHER" | |||
INTENT_HOME = "android.intent.category.HOME" | |||
|
|||
# Specify application identifier as CURRENT_APP from dumpsys output previously assigned to CURRENT_APP | |||
VAR_CURRENT_APP_EXTRACTION = 'CURRENT_APP=${CURRENT_APP#*ActivityRecord{* * } CURRENT_APP=${CURRENT_APP#*{* * } && CURRENT_APP=${CURRENT_APP%%/*} && CURRENT_APP=${CURRENT_APP%\\}*}' |
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 isn't used.
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.
Actually, I guess it is used. Chrome on my phone only found it once on this page. So maybe it's OK.
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.
Actually this is the core "logic" in regard to extracting the application identifier from the dumpsys output, either it being from dumpsys window windows
or dumpsys activity a .
.
It does the following operations:
${CURRENT_APP#*ActivityRecord{* * }
Attempt to extract application activity frommResumedActivity
ormFocusedApp
${CURRENT_APP#*{* * }
Fallback to extract application activity frommCurrentFocus
, in casemFocusedApp
is not defined${CURRENT_APP%%/*}
Remove extraneous activity details for extracted application, leaving only identifier${CURRENT_APP%\\}*}
Delete trailing curly brace from identifier of extracted application, in case application has no activity
CMD_LAUNCH_APP_CONDITION = "if [ $CURRENT_APP != '{0}' ]; then monkey -p {0} -c " + INTENT_LAUNCH + ' --pct-syskeys 0 1; fi' | ||
|
||
#: Launch an app | ||
CMD_LAUNCH_APP = CMD_DEFINE_CURRENT_APP.replace('{', '{{').replace('}', '}}') + ' && ' + CMD_LAUNCH_APP_CONDITION |
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 know it's not DRY to copy & paste, but given that this module is simply a bunch of constants, I think it's acceptable here. Also, I think it's much more readable.
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 certainly agree in regard to readability, also slightly consider this to be a despicable solution, but it does escape all curly braces in the referenced constant to accompany the formatting done with this constant definition.
Although the complexity of extracting the CURRENT_APP
variable from dumpsys output, from my experience, having multiple occurrences with duplicate logic, it's very easy to overlook certain occurrences when updating logic in regard to CURRENT_APP
.
This is the main reason I took the DRY approach, by introducing the CMD_DEFINE_CURRENT_APP
and VAR_CURRENT_APP_EXTRACTION
constants, for reusability and to simplifying maintenance of CURRENT_APP
logic.
…put Manager service activities.
|
||
#: Get the current app for a Google TV device | ||
CMD_CURRENT_APP_GOOGLE_TV = "CURRENT_APP=$(dumpsys activity a . | grep -E 'mResumedActivity' | cut -d ' ' -f 8) && CURRENT_APP=${CURRENT_APP%%/*} && echo $CURRENT_APP" |
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.
It looks like you changed the command here (old first, new second):
CURRENT_APP=$(dumpsys activity a . | grep -E 'mResumedActivity' | cut -d ' ' -f 8) ...
CURRENT_APP=$(dumpsys activity a . | grep mResumedActivity) ...
Why the change? I don't own a Google TV device, so I can't test whether this works.
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.
For this particular change I looked at this Home Assistant Community topic (reference from #207). Indicating that both mCurrentFocus
and mFocusedApp
are no longer specified in the dumpsys window windows
output.
Then I checked the alternative command (dumpsys activity a . | grep mResumedActivity
) to obtain application activity details, which is also working on my Sony TV running Android 8.0.0, and noticed it has the same ActivityRecord entry as mFocusedApp
.
mResumedActivity: ActivityRecord{35dbb2a u0 com.google.android.tvlauncher/.MainActivity t2719}
mFocusedApp=AppWindowToken{c791eb8 token=Token{a6bad1b ActivityRecord{35dbb2a u0 com.google.android.tvlauncher/.MainActivity t2719}}}
Rather than using the existing solution which relies on space (indentation) to extract the application activity details, I decided to change this approach to be more generic by looking for the ActivityRecord instead.
tests/test_constants.py
Outdated
mLastStartReason=startActivityAsUser | ||
mLastStartActivityTimeMs=6 Feb 2021 22:30:46 | ||
mLastStartActivityResult=2 | ||
mLastStartActivityRecord: | ||
packageName=com.sony.dtv.tvx processName=com.sony.dtv.tvx | ||
launchedFromUid=10134 launchedFromPackage=com.sony.dtv.tvx userId=0 | ||
app=ProcessRecord{edc6562 3904:com.sony.dtv.tvx/u0a134} | ||
Intent { act=android.intent.action.VIEW dat=content://android.media.tv/passthrough/com.sony.dtv.tvinput.external%2F.ExternalTvInputService%2FHW2 flg=0x10040000 cmp=com.sony.dtv.tvx/.MainActivity (has extras) } | ||
frontOfTask=true task=TaskRecord{9eecc8c #2830 A=com.sony.dtv.tvx U=0 StackId=1 sz=1} | ||
taskAffinity=com.sony.dtv.tvx | ||
realActivity=com.sony.dtv.tvx/.MainActivity | ||
baseDir=/system/priv-app/Tvx/Tvx.apk | ||
dataDir=/data/user/0/com.sony.dtv.tvx | ||
stateNotNeeded=false componentSpecified=false mActivityType=0 | ||
compat={320dpi always-compat} labelRes=0x7f090048 icon=0x7f02006d theme=0x7f0b01c2 | ||
mLastReportedConfigurations: | ||
mGlobalConfig={1.0 ?mcc?mnc [en_GB] ldltr sw540dp w960dp h540dp 320dpi lrg long hdr land television -touch -keyb/v/h dpad/v appBounds=Rect(0, 0 - 1920, 1080) s.3} | ||
mOverrideConfig={1.0 ?mcc?mnc [en_GB] ldltr sw540dp w960dp h540dp 320dpi lrg long hdr land television -touch -keyb/v/h dpad/v appBounds=Rect(0, 0 - 1920, 1080) s.3} | ||
CurrentConfiguration={1.0 ?mcc?mnc [en_GB] ldltr sw540dp w960dp h540dp 320dpi lrg long hdr land television -touch -keyb/v/h dpad/v appBounds=Rect(0, 0 - 1920, 1080) s.3} | ||
taskDescription: iconFilename=null label="null" primaryColor=ff8c0000 | ||
backgroundColor=ff303030 | ||
statusBarColor=ff000000 | ||
navigationBarColor=ff000000 | ||
launchFailed=false launchCount=0 lastLaunchTime=-2d11h49m7s584ms | ||
haveState=false icicle=null | ||
state=RESUMED stopped=false delayedResume=false finishing=false | ||
keysPaused=false inHistory=true visible=true sleeping=false idle=true mStartingWindowState=STARTING_WINDOW_REMOVED | ||
fullscreen=true noDisplay=false immersive=false launchMode=3 | ||
frozenBeforeDestroy=false forceNewConfig=false | ||
mActivityType=APPLICATION_ACTIVITY_TYPE | ||
waitingVisible=false nowVisible=true lastVisibleTime=-2m50s467ms | ||
connections=[ConnectionRecord{fee4c3e u0 CR com.sony.dtv.osdplanevisibilitymanager/.OsdEnabler:@246a4f9}] | ||
resizeMode=RESIZE_MODE_RESIZEABLE | ||
mLastReportedMultiWindowMode=false mLastReportedPictureInPictureMode=false | ||
supportsPictureInPicture=true | ||
supportsPictureInPictureWhilePausing: true | ||
mLastHomeActivityStartResult=0 | ||
mLastHomeActivityStartRecord: | ||
packageName=com.google.android.tvlauncher processName=com.google.android.tvlauncher | ||
launchedFromUid=0 launchedFromPackage=null userId=0 | ||
app=ProcessRecord{8d5fa46 22638:com.google.android.tvlauncher/u0a162} | ||
Intent { act=android.intent.action.MAIN cat=[android.intent.category.HOME] flg=0x10800100 cmp=com.google.android.tvlauncher/.MainActivity } | ||
frontOfTask=true task=TaskRecord{1142677 #2719 A=.TvLauncher U=0 StackId=0 sz=1} | ||
taskAffinity=.TvLauncher | ||
realActivity=com.google.android.tvlauncher/.MainActivity | ||
baseDir=/data/app/com.google.android.tvlauncher-yGwF-XP6Zf5hV0DbQ9Z4gA==/base.apk | ||
dataDir=/data/user/0/com.google.android.tvlauncher | ||
stateNotNeeded=true componentSpecified=false mActivityType=1 | ||
compat={320dpi always-compat} labelRes=0x7f12002b icon=0x7f0f0000 theme=0x7f130007 | ||
mLastReportedConfigurations: | ||
mGlobalConfig={1.0 ?mcc?mnc [en_GB] ldltr sw540dp w960dp h540dp 320dpi lrg long hdr land television -touch -keyb/v/h dpad/v appBounds=Rect(0, 0 - 1920, 1080) s.3} | ||
mOverrideConfig={1.0 ?mcc?mnc [en_GB] ldltr sw540dp w960dp h540dp 320dpi lrg long hdr land television -touch -keyb/v/h dpad/v appBounds=Rect(0, 0 - 1920, 1080) s.3} | ||
CurrentConfiguration={1.0 ?mcc?mnc [en_GB] ldltr sw540dp w960dp h540dp 320dpi lrg long hdr land television -touch -keyb/v/h dpad/v appBounds=Rect(0, 0 - 1920, 1080) s.3} | ||
taskDescription: iconFilename=null label="null" primaryColor=ff37474f | ||
backgroundColor=ff303030 | ||
statusBarColor=ff263238 | ||
navigationBarColor=ff000000 | ||
launchFailed=false launchCount=0 lastLaunchTime=-3m0s967ms | ||
haveState=true icicle=Bundle[mParcelledData.dataSize=1216] | ||
state=STOPPED stopped=true delayedResume=false finishing=false | ||
keysPaused=false inHistory=true visible=false sleeping=false idle=true mStartingWindowState=STARTING_WINDOW_SHOWN | ||
fullscreen=true noDisplay=false immersive=false launchMode=2 | ||
frozenBeforeDestroy=false forceNewConfig=false | ||
mActivityType=HOME_ACTIVITY_TYPE | ||
waitingVisible=false nowVisible=false lastVisibleTime=-2m58s442ms | ||
resizeMode=RESIZE_MODE_UNRESIZEABLE | ||
mLastReportedMultiWindowMode=false mLastReportedPictureInPictureMode=false | ||
mStartActivity: | ||
packageName=com.sony.dtv.tvx processName=com.sony.dtv.tvx | ||
launchedFromUid=10134 launchedFromPackage=com.sony.dtv.tvx userId=0 | ||
app=null | ||
Intent { act=android.intent.action.VIEW dat=content://android.media.tv/passthrough/com.sony.dtv.tvinput.external%2F.ExternalTvInputService%2FHDMI100004 flg=0x10400000 cmp=com.sony.dtv.tvx/.MainActivity (has extras) } | ||
frontOfTask=false task=TaskRecord{9eecc8c #2830 A=com.sony.dtv.tvx U=0 StackId=1 sz=1} | ||
taskAffinity=com.sony.dtv.tvx | ||
realActivity=com.sony.dtv.tvx/.MainActivity | ||
baseDir=/system/priv-app/Tvx/Tvx.apk | ||
dataDir=/data/user/0/com.sony.dtv.tvx | ||
stateNotNeeded=false componentSpecified=false mActivityType=0 | ||
compat=null labelRes=0x7f090048 icon=0x7f02006d theme=0x7f0b01c2 | ||
mLastReportedConfigurations: | ||
mGlobalConfig={1.0 ?mcc?mnc [en_GB] ldltr sw540dp w960dp h540dp 320dpi lrg long hdr land television -touch -keyb/v/h dpad/v appBounds=Rect(0, 0 - 1920, 1080) s.3} | ||
mOverrideConfig={0.0 ?mcc?mnc ?localeList ?layoutDir ?swdp ?wdp ?hdp ?density ?lsize ?long ?ldr ?wideColorGamut ?orien ?uimode ?night ?touch ?keyb/?/? ?nav/?} | ||
CurrentConfiguration={1.0 ?mcc?mnc [en_GB] ldltr sw540dp w960dp h540dp 320dpi lrg long hdr land television -touch -keyb/v/h dpad/v appBounds=Rect(0, 0 - 1920, 1080) s.3} | ||
launchFailed=false launchCount=0 lastLaunchTime=0 | ||
haveState=true icicle=null | ||
state=INITIALIZING stopped=false delayedResume=false finishing=false | ||
keysPaused=false inHistory=false visible=false sleeping=false idle=false mStartingWindowState=STARTING_WINDOW_NOT_SHOWN | ||
fullscreen=true noDisplay=false immersive=false launchMode=3 | ||
frozenBeforeDestroy=false forceNewConfig=false | ||
mActivityType=APPLICATION_ACTIVITY_TYPE | ||
resizeMode=RESIZE_MODE_RESIZEABLE | ||
mLastReportedMultiWindowMode=false mLastReportedPictureInPictureMode=false | ||
supportsPictureInPicture=true | ||
supportsPictureInPictureWhilePausing: false | ||
mIntent=Intent { act=android.intent.action.VIEW dat=content://android.media.tv/passthrough/com.sony.dtv.tvinput.external/.ExternalTvInputService/HDMI100004 flg=0x10400000 cmp=com.sony.dtv.tvx/.MainActivity (has extras) } | ||
mLaunchSingleTop=false mLaunchSingleInstance=true mLaunchSingleTask=false mLaunchFlags=0x10000000 mDoResume=true mAddingToTask=false | ||
""" | ||
hdmi_input = self._hdmi_input(dumpsys_output) | ||
|
||
self.assertEqual(hdmi_input, '') |
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.
Going forward I would highly recommend to enforce tests cases that actually assert commands against dumpsys output fixtures, which would for sure give me more confidence about (future) changes made to these constants.
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.
That's a good idea. I'd thought about it but never took action on it, so thanks for contributing this.
… into sony-tv-support
I would have been open to any suggestions for renaming and having them addressed in this pull request.
I guess my #233 (comment) was not helpful with reviewing the difference? I'm glad you managed in the end and really like your approach to outputting fully written commands in tests (#235) for reviewing purposes. |
Your comment sparked my idea. By including a test for it, I can guarantee that the old and the new commands are both being reported correctly. Anyways, I just figured it was easier to make some of the renaming changes myself (and also update the commands test so that it passes) rather than to comment the changes I wanted on your pull request and have you make the changes. |
current_app
not representing focused application when screen is turned off (by Picture Off action).}
character incurrent_app
when focused application has no activity.hdmi_input
. (fixes Sony TV HDMI input detection #227)Disclaimer: This solution was developed on a Sony Bravia KD-55XD7005 (PKG6.6545.0255EUA) running Android 8.0.0.
Test cases written for other devices are based on output reported in relevant conversations, although I have no way to actually verify this solution works on other devices.