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.

Sign up for GitHub

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

Jump to bottom

feat: connect adb over wifi #29

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: connect adb over wifi #29

wants to merge 1 commit into from

Conversation

itsspriyansh
Copy link
Contributor

@itsspriyansh itsspriyansh commented Feb 27, 2024 •
edited

Feature Implemented

Added functionality to connect to Android Debug Bridge (ADB) wirelessly using a pairing code. This feature enhances the flexibility of connecting to devices, particularly useful in scenarios where USB connection is not feasible.

Changes Made

  1. Implemented the --wireless flag to enable wireless ADB connection.
  2. Modified the AndroidSetup class to support wireless connection setup.
  3. Added logic to generate and enter the pairing code for establishing wireless ADB connection.

Additional Notes

This feature adds convenience and flexibility to the ADB connection process, especially in scenarios where physical USB connections are impractical or unavailable. The implementation adheres to best practices and maintains compatibility with existing codebase and dependencies.

Resolves: #19

simplescreenrecorder-2024-03-04_23.35.12.mp4

@Biki-das
Copy link
Contributor

Nice work @itsspriyansh , but the implementation seems to be wrong, since it would be a complete walkthrough and not capturing it from adb , the user would manually enter the adb addr:port and pairing code to connect the same at the same time running the helper tool
see #19 (comment)
this sort of script has to be added

@itsspriyansh
Copy link
Contributor Author

Hey @Biki-das, thanks for pointing that out. I have corrected the changes now.

@garg3133
Copy link
Member

Great work on this @itsspriyansh.

But, this should not be a part of the main setup. The main setup is used to ensure that all the requirements that we would potentially need to download are present on the system, and if not, download the requirements. Connecting a device to adb wirelessly is not such a requirement and should be kept separate for the following reasons:

  • At the time of the initial setup, the user might not be ready with a device to connect to wirelessly or using a USB cable, they just want to setup all the requirements. And if we, in the middle of that setup, ask user to connect to the device as well, that creates friction as the user would now need to do much more work to complete the setup, on the contrary of just letting the tool handle everything.
  • Connecting/Disconnecting a device to adb is a repetitive task and user shouldn't need to run the setup command everytime they need to connect to an Android device wirelessly.

So, instead of asking user to connect to a device at the time of setup, we can provide user with a separate command that they can run to do connect to the Android device wirelessly, an number of time as they like to.

The command can be something like: npx @nightwatch/mobile-helper android --wireless-connection.

@garg3133
Copy link
Member

Btw, to pass the flags in the dev run, you'd need to prepend the arguments with a --. Ex. the above command in development would look like: npm run dev -- android --wireless-connection.

Also, I haven't reviewed the code yet, will do so in a while.

@itsspriyansh itsspriyansh changed the title connect adb over wifi feat: connect adb over wifi Feb 29, 2024
@itsspriyansh
Copy link
Contributor Author

@garg3133 I have made the suggested changes.

@garg3133
Copy link
Member

@itsspriyansh Looks good. Two things:

  • Can you also put some information for the user before asking the first question on how to get the details that are being asked in the questions below or the steps that the user should follow to get the answers? We can't expect user to know everything about setting up adb wirelessly before running the tool, it is kind of our job to instruct the user. (You can also later on help with a detailed doc for setting up adb wirelessly from Document Android SDK commands #20).
  • We don't expect the users to have the device connected with USB cable while trying to setup the wireless connection. The whole point of having wireless connection is for those who don't want to deal with the USB cables. Also, even if they have their device connected over USB cable, we expect them to remove the cable before trying to connect wirelessly.

@itsspriyansh
Copy link
Contributor Author

@garg3133 I have added the instructions. I have also update the video preview in the PR description. You may take a look at it to review the changes.

garg3133
garg3133 reviewed Mar 4, 2024
View reviewed changes
Copy link
Member

@garg3133 garg3133 left a comment

Choose a reason for hiding this comment

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

We would also need to provide a way for users to be able to disconnect a device as that is also not very straightforward.

Think of what flags we could use while running command for this purpose.

Now that we have both these use cases, I'm more inclined towards having the following commands:

# connecting a device
npx @nightwatch/mobile-helper android connect
# ^ will only show a list of devices connected because for USB or emulator connections
# we don't need to do anything to establish a connection.

# connecting a device wirelessly
npx @nightwatch/mobile-helper android connect --wireless
# ^ what's being done in this PR

# disconnecting a device
npx @nightwatch/mobile-helper android disconnect
# ^ will show a list of all the connected devices and ask a further question on which device to disconnect.

For this PR, let's just keep the scope to the second command above and the rest we can do later on.

src/commands/android/utils/adbWirelessConnect.ts Outdated Show resolved Hide resolved
src/commands/android/utils/adbWirelessConnect.ts Outdated Show resolved Hide resolved
src/commands/android/utils/adbWirelessConnect.ts Show resolved Hide resolved
src/commands/android/utils/adbWirelessConnect.ts Outdated Show resolved Hide resolved
@itsspriyansh
Copy link
Contributor Author

itsspriyansh commented Mar 4, 2024 •
edited

@garg3133 I have updated the instructions and the video preview.

As for now, I have not implemented the use of connect argument. The script is currently running with
npx @nightwatch/mobile-helper android --wireless command.
I would require some more clarification regarding what cases to handle when connect is passed as argument.

@garg3133
Copy link
Member

garg3133 commented Mar 6, 2024

@itsspriyansh The flow is superb! Thank you. I'll do a more thorough review of the code later, but this looks good.

Regarding the connect argument, what I'm thinking is:

  • If only connect argument is passed, we'll ask if they want to connect a device wirelessly.
    • if the answer is yes, we'll move to the flow in this PR.
    • if the answer is no, we'll just display all the connected devices (by running adb devices command)
  • If connect is passed with --wireless flag, we'll use the flow from this PR.
  • If disconnect argument is passed, we'll ask user to choose the device to disconnect (with a "All devices" option at the end) and then run the adb disconnect command accordingly.

But any of this does not need to be implemented in this current PR. The work on this PR is mostly complete I think.

@itsspriyansh
Copy link
Contributor Author

Thank you @garg3133
I am excited for further contribution.

garg3133
garg3133 reviewed May 16, 2024
View reviewed changes
src/commands/android/index.ts Outdated Show resolved Hide resolved
src/commands/android/index.ts Outdated Show resolved Hide resolved
src/commands/android/utils/adbWirelessConnect.ts Outdated Show resolved Hide resolved
src/commands/android/utils/adbWirelessConnect.ts Outdated Show resolved Hide resolved
src/commands/android/utils/adbWirelessConnect.ts Outdated Show resolved Hide resolved
src/commands/android/utils/adbWirelessConnect.ts Outdated Show resolved Hide resolved
src/commands/android/utils/adbWirelessConnect.ts Outdated Show resolved Hide resolved
src/commands/android/utils/adbWirelessConnect.ts Outdated Show resolved Hide resolved
@itsspriyansh @garg3133
Add --standalone flag for Android setup (nightwatchjs#27)
7053c19 
Co-authored-by: Priyansh Garg <priyanshgarg30@gmail.com>
@itsspriyansh
Copy link
Contributor Author

@garg3133 I have now handled the case when adb is not found. However, while testing this, I found that we will be able to install the missing binary using the --setup flag only if the entire platform-tools folder is missing and not just a specific binary inside it. However this isn't handled in the tool. Do we need to consider this case?

@itsspriyansh
Copy link
Contributor Author

itsspriyansh commented May 23, 2024 •
edited

Whenever a command fails to run using the execBinarySync function we get an error message saying "failed to run ...command". I don't think in this case we need to show this message to the users since we are already showing messages like "pairing successful" or "pairing failed!". Can we consider implementing suppressOutput parameter in the execBinarySync function to avoid this?
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garg3133 garg3133 garg3133 left review comments

@Gdakgdak22 Gdakgdak22 Gdakgdak22 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Connect Android devices to adb wirelessly.
4 participants
@itsspriyansh @Biki-das @garg3133 @Gdakgdak22