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

[androiddebugbridge] initial contribution #9259

Merged
merged 19 commits into from
Jan 21, 2021
Merged

[androiddebugbridge] initial contribution #9259

merged 19 commits into from
Jan 21, 2021

Conversation

GiviMAD
Copy link
Member

@GiviMAD GiviMAD commented Dec 6, 2020

Signed-off-by: Miguel [email protected]

Hi openHAB community. I have created this binding to allow openHAB to connect to android devices using the adb protocol.
My motivation for this was to replace some adb commands that I was sending using the exec-binding (to an android device connected to my tv). With the binding I can also avoid to install de adb cli in the docker image and use the official image directly.
I also added some other channels like a player, a volume dimmer, and some to control the application opened. I've been using it for a couple of days and it seems to work correctly.
Hope it can be helpful for others.

Signed-off-by: Miguel <[email protected]>
@GiviMAD GiviMAD requested a review from a team as a code owner December 6, 2020 16:17
@fwolter fwolter added the new binding If someone has started to work on a binding. For a new binding PR. label Dec 6, 2020
@cweitkamp
Copy link
Contributor

Thank you very much for your contribution. Will it be possible to control / connect to Fire TV Sticks by this binding? I would be very much interested in that feature.

@GiviMAD
Copy link
Member Author

GiviMAD commented Dec 7, 2020

@cweitkamp. I'm happy to say it does :D , at least the Firestick 4k is the device that I'm using with the binding right now. Everything seems to work nicely unless the audio dimmer which I was not able to read/write from the adb. The audio dimmer works properly on the other device that I test with, my old nexus 5x.
As a comment maybe some improvements could be done on the DiscoveryService because the interface is not working correctly for me, I can't stop the search process. If you can give some advice on how to improve this I will be thankful.

@GiviMAD
Copy link
Member Author

GiviMAD commented Dec 10, 2020

I can't find the build for this pr on the ci. I think there was some kind of error and it was not created. Can you trigger a rebuild? Thanks in advance.

@cweitkamp cweitkamp added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Dec 11, 2020
@cweitkamp
Copy link
Contributor

Can you trigger a rebuild? Thanks in advance.

Yes, of course. You can do that on your own by adding the "rebuild" label to your PR.

@fwolter fwolter self-assigned this Dec 13, 2020
Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! I reviewed your code and here is my feedback.

@GiviMAD
Copy link
Member Author

GiviMAD commented Dec 16, 2020

I think everything is done.
Also the build fails to queue again and I can't edit the pr labels, if no more changes are required can you trigger the build again? Thanks!

@fwolter fwolter added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Dec 16, 2020
@GiviMAD GiviMAD requested a review from fwolter December 17, 2020 15:29
Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fwolter fwolter added the cre Coordinated Review Effort label Dec 18, 2020
@fwolter fwolter removed their assignment Dec 18, 2020
@cweitkamp cweitkamp self-assigned this Dec 20, 2020
@GiviMAD GiviMAD requested a review from cpmeister December 26, 2020 11:57
@cweitkamp
Copy link
Contributor

cweitkamp commented Jan 9, 2021

I installed a version containing your latest changes and will check on the "state_dectection_rules". Thanks.

One thing which came up after a power loss of the Fire TV is a recurring warning in my logs. I am not sure why, but it looks like the binding cannot connect anymore.

2021-01-09 14:21:03.978 [WARN ] [mmon.WrappedScheduledExecutorService] - Scheduled runnable ended with an exception: 
java.lang.IllegalStateException: connect() must be called first
	at com.tananaev.adblib.AdbConnection.open(AdbConnection.java:321) ~[?:?]
	at org.openhab.binding.androiddebugbridge.internal.AndroidDebugBridgeDevice.lambda$0(AndroidDebugBridgeDevice.java:267) ~[?:?]
	at java.util.concurrent.FutureTask.run(Unknown Source) ~[?:?]
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(Unknown Source) ~[?:?]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source) [?:?]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source) [?:?]
	at java.lang.Thread.run(Unknown Source) [?:?]
2021-01-09 14:21:03.986 [WARN ] [mmon.WrappedScheduledExecutorService] - Scheduled runnable ended with an exception: 
java.lang.IllegalStateException: connect() must be called first
	at com.tananaev.adblib.AdbConnection.open(AdbConnection.java:321) ~[?:?]
	at org.openhab.binding.androiddebugbridge.internal.AndroidDebugBridgeDevice.lambda$0(AndroidDebugBridgeDevice.java:267) ~[?:?]
	at java.util.concurrent.FutureTask.run(Unknown Source) ~[?:?]
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(Unknown Source) ~[?:?]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source) [?:?]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source) [?:?]
	at java.lang.Thread.run(Unknown Source) [?:?]
2021-01-09 14:21:03.994 [WARN ] [mmon.WrappedScheduledExecutorService] - Scheduled runnable ended with an exception: 
java.lang.IllegalStateException: connect() must be called first
	at com.tananaev.adblib.AdbConnection.open(AdbConnection.java:321) ~[?:?]
	at org.openhab.binding.androiddebugbridge.internal.AndroidDebugBridgeDevice.lambda$0(AndroidDebugBridgeDevice.java:267) ~[?:?]
	at java.util.concurrent.FutureTask.run(Unknown Source) ~[?:?]
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(Unknown Source) ~[?:?]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source) [?:?]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source) [?:?]
	at java.lang.Thread.run(Unknown Source) [?:?]
2021-01-09 14:21:04.002 [WARN ] [mmon.WrappedScheduledExecutorService] - Scheduled runnable ended with an exception: 
java.lang.IllegalStateException: connect() must be called first
	at com.tananaev.adblib.AdbConnection.open(AdbConnection.java:321) ~[?:?]
	at org.openhab.binding.androiddebugbridge.internal.AndroidDebugBridgeDevice.lambda$0(AndroidDebugBridgeDevice.java:267) ~[?:?]
	at java.util.concurrent.FutureTask.run(Unknown Source) ~[?:?]
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(Unknown Source) ~[?:?]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source) [?:?]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source) [?:?]
	at java.lang.Thread.run(Unknown Source) [?:?]

Signed-off-by: Miguel <[email protected]>
Signed-off-by: Miguel <[email protected]>
@GiviMAD
Copy link
Member Author

GiviMAD commented Jan 10, 2021

Now the thing connect again correctly. Thanks for pointing that.

@GiviMAD GiviMAD requested a review from cweitkamp January 17, 2021 11:00
@cweitkamp
Copy link
Contributor

cweitkamp commented Jan 17, 2021

Sry, I did non come back to you earlier. The latest version from last Sunday looks better. But can still see a warning in my logs when the binding tries to reconnect. I this situation my Fire TV Stick is in standby mode. Unfortunately it does not help to wake it up. I have to perform a full restart of the Fire TV Stick to establish a working connection again.

2021-01-17 15:59:20.441 [WARN ] [mmon.WrappedScheduledExecutorService] - Scheduled runnable ended with an exception: 
java.lang.IllegalStateException: connect() must be called first
	at com.tananaev.adblib.AdbConnection.open(AdbConnection.java:321) ~[?:?]
	at org.openhab.binding.androiddebugbridge.internal.AndroidDebugBridgeDevice.lambda$0(AndroidDebugBridgeDevice.java:271) ~[?:?]
	at java.util.concurrent.FutureTask.run(Unknown Source) ~[?:?]
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(Unknown Source) ~[?:?]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source) [?:?]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source) [?:?]
	at java.lang.Thread.run(Unknown Source) [?:?]

Copy link
Contributor

@cweitkamp cweitkamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some minor improvements

Signed-off-by: Miguel <[email protected]>
@GiviMAD
Copy link
Member Author

GiviMAD commented Jan 19, 2021

I can't get rid of the warning in the logs because the library close the connection once the fire stick is offline and I not able to detect this until it throws that exception. Once it throws the binding should change the thing state to offline and the warning should stop spamming.
I'll try to make a pr to the library to improve this in a future update but at the moment I couldn't get anything better or at least did manage to do it.
Let me know if more improvements are required and thanks for the feedback.

@GiviMAD GiviMAD requested a review from cweitkamp January 19, 2021 00:08
Copy link
Contributor

@cweitkamp cweitkamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience. I am fine with the current implementation. Improvements and new features can be added later.

Only two final comments because I recognized it in the latest commit. First one is inline. Second one - please review the code of the AndroidDebugBridgeHandler class because there are some if-statements without curly brackets.

I am done. 😉

Signed-off-by: Miguel <[email protected]>
Signed-off-by: Miguel <[email protected]>
Copy link
Contributor

@cweitkamp cweitkamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks.

@cpmeister Are all your comments addressed?

Signed-off-by: Miguel <[email protected]>
@GiviMAD GiviMAD requested a review from cpmeister January 20, 2021 21:20
@cpmeister cpmeister added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jan 20, 2021
@cweitkamp cweitkamp added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jan 21, 2021
@cweitkamp cweitkamp added this to the 3.1 milestone Jan 21, 2021
@cweitkamp cweitkamp removed the cre Coordinated Review Effort label Jan 21, 2021
@cweitkamp cweitkamp merged commit a693a4e into openhab:main Jan 21, 2021
@GiviMAD GiviMAD deleted the androiddebugbridge branch January 21, 2021 16:22
themillhousegroup pushed a commit to themillhousegroup/openhab2-addons that referenced this pull request May 10, 2021
* Initial contribution

Signed-off-by: Miguel <[email protected]>
Signed-off-by: John Marshall <[email protected]>
thinkingstone pushed a commit to thinkingstone/openhab-addons that referenced this pull request Nov 7, 2021
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new binding If someone has started to work on a binding. For a new binding PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants