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] Add start intent channel #12438

Merged
merged 4 commits into from
Apr 30, 2022

Conversation

GiviMAD
Copy link
Member

@GiviMAD GiviMAD commented Mar 6, 2022

Signed-off-by: Miguel Álvarez Díez miguelwork92@gmail.com

This pr add a new channel for start application intents.
Also fixes the discovery and revert to use the device serial number as identifier on both discovery methods.

@GiviMAD GiviMAD requested review from cweitkamp and lolodomo March 6, 2022 20:37
@jaywiseman
Copy link

I'm using this version of the binding.

https://docs.smarthomej.org/3.2.12-SNAPSHOT/org.smarthomej.binding.androiddebugbridge.html

This version has the Intent channel defined already.

I have these items working with the binding I'm referencing above.

https://community.openhab.org/t/android-debug-binding-adb-notes-for-others/133777

Best, Jay

@lsiepel lsiepel added bug An unexpected problem or unintended behavior of an add-on enhancement An enhancement or new feature for an existing add-on labels Mar 6, 2022
@lsiepel
Copy link
Contributor

lsiepel commented Mar 6, 2022

Did a quick scan for this PR. Usually it is preferred to seperate bug and enhancements PR. Anyway, did you get in contact with previous contributer that changed the identifier for discovery?

@GiviMAD
Copy link
Member Author

GiviMAD commented Mar 6, 2022

@lsiepel, yes he is one of the reviewers also I mentioned him in the issue, and for the reference I'm the original contributor.

I was working on the new channel when I discovered the issue so I fixed it to been able to tested it.

@GiviMAD
Copy link
Member Author

GiviMAD commented Mar 6, 2022

@jaywiseman I have no clue, I don't think it will work without changes.
Edit:
If you are referring to the start-package channel yes, I think both should work.
The start-intent channel added by this pr is meant to call 'am start' passing more arguments apart from the package as a uri and action and some extra parameters, to been able to use other app intents.

@wborn wborn removed the bug An unexpected problem or unintended behavior of an add-on label Mar 7, 2022
@GiviMAD
Copy link
Member Author

GiviMAD commented Mar 15, 2022

@cweitkamp did you have a chance to look at this?
We can take a different path and use the ip as identifier if you think that connect through adb from the MDNS discovery is too much.

@cweitkamp cweitkamp linked an issue Mar 18, 2022 that may be closed by this pull request
@cweitkamp
Copy link
Contributor

Sry for my late reply. As I already stated in #11881 (comment) for me it looks like the Mac address is the only unique identifier available in both discovery service implementations.

The mDNS discovery in general does not require to enable usb debugging in general as requested in the documentation. Imo your approach will fail to discover a new device as long as this requirement is not given. The question now is if we want to restrict the discovery results to rely on this setting or not.

May I ask you to slit this PR into two separate ones? One for the discovery issue and one for the enhancement to add the new channel. Thanks.

@GiviMAD
Copy link
Member Author

GiviMAD commented Mar 19, 2022

Sorry but I can’t agree @cweitkamp.



First of all we don’t have the mac address in both discovery methods, the way currently implemented on the normal discovery is totally wrong and I couldn’t find an easy way to get it, also the serial number used util that pr seems the best option and with this changes is available on both sides.







Also currently we have broken a rudimentary but functional discovery that works for all android devices, just for adding one for discover FireTV devices, which is not the only target of this binding, so for me it’s more a problem right now, so if we could not find a good solution quickly I think its better to remove the new discovery method and implemented it again when we know how to do without break the current one.











And finally as I said before I have added the fix to this pr because I discovered it was broken at that moment, I can move it to a separate one if is needed, but seems like and extra work for me and I really don’t see the point as the pr is not huge.

GiviMAD added 2 commits March 20, 2022 11:21
Signed-off-by: Miguel Álvarez Díez <miguelwork92@gmail.com>
Signed-off-by: Miguel Álvarez Díez <miguelwork92@gmail.com>
@GiviMAD GiviMAD force-pushed the androiddebugbridge/start_input branch from ea92454 to a32d000 Compare March 20, 2022 10:23
@GiviMAD GiviMAD changed the title [androiddebugbridge] add start intent channel and fix discovery [androiddebugbridge] add start intent channel Mar 20, 2022
@GiviMAD
Copy link
Member Author

GiviMAD commented Mar 20, 2022

Removed all discovery related changes.
So the problem with the discovery remains open as the device MAC address is not available in the adb discovery method.

Copy link
Contributor

@jlaur jlaur 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 the PR. I have made a few minor comments.

Signed-off-by: Miguel Álvarez Díez <miguelwork92@gmail.com>
@GiviMAD GiviMAD requested a review from jlaur April 3, 2022 08:09
@lolodomo
Copy link
Contributor

@GiviMAD : there is not too much to do so that I can merge your PR.

@GiviMAD
Copy link
Member Author

GiviMAD commented Apr 30, 2022

@GiviMAD : there is not too much to do so that I can merge your PR.

Sorry, almost forgot about it, I was messing around trying other things. Thanks for pinging me, I'll go for it.

Signed-off-by: Miguel Álvarez Díez <miguelwork92@gmail.com>
@GiviMAD GiviMAD requested a review from lolodomo April 30, 2022 18:07
Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

LGTM

@lolodomo
Copy link
Contributor

@jlaur : is it ok for you (as you reviewed this PR without approving it) ?

@lolodomo lolodomo merged commit f4b888a into openhab:main Apr 30, 2022
@lolodomo lolodomo added this to the 3.3 milestone Apr 30, 2022
@jlaur jlaur changed the title [androiddebugbridge] add start intent channel [androiddebugbridge] Add start intent channel May 23, 2022
andan67 pushed a commit to andan67/openhab-addons that referenced this pull request Nov 6, 2022
* [androiddebugbridge] add start intent channel

Signed-off-by: Miguel Álvarez Díez <miguelwork92@gmail.com>
andrasU pushed a commit to andrasU/openhab-addons that referenced this pull request Nov 12, 2022
* [androiddebugbridge] add start intent channel

Signed-off-by: Miguel Álvarez Díez <miguelwork92@gmail.com>
Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>
psmedley pushed a commit to psmedley/openhab-addons that referenced this pull request Feb 23, 2023
* [androiddebugbridge] add start intent channel

Signed-off-by: Miguel Álvarez Díez <miguelwork92@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[androiddebugbridge] discovery for non firestick devices is broken
7 participants