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

feat: connect adb over wifi #29

Merged
merged 2 commits into from
Jun 18, 2024
Merged

Conversation

itsspriyansh
Copy link
Contributor

@itsspriyansh itsspriyansh commented Feb 27, 2024

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

Added script to achieve the functionality in android/utils/connect-device.ts.

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.

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 Outdated 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

@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.

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
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

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

Co-authored-by: Priyansh Garg <priyanshgarg30@gmail.com>
@garg3133
Copy link
Member

@itsspriyansh Have I not told you not to force push before? Doing force-push makes it impossible to review the diff and requires us to review the entire PR again.

Also, changes in other files are missing now from this PR, only one file is there now which can't be run on its own. Or is this intentional and you want aim to do the integration in other PRs?

@itsspriyansh
Copy link
Contributor Author

itsspriyansh commented Jun 12, 2024

@garg3133 Actually you haven't told me that. I will keep that in mind. And yes it is intentional. If we get the changes in other files merged, we will be overwriting them anyways in the next pr which I didn't think would be helpful. For now, I just wanted to add the script.

@garg3133
Copy link
Member

garg3133 commented Jun 18, 2024

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?

I don't think that's true. I just tried it myself by removing the adb binary from the platform-tools folder and it correctly re-installed the platform-tools SDK and added it back for me.

Can you try this in your system once more? And if it still doesn't work, let me know of the steps you followed.

@garg3133
Copy link
Member

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!".

I think those error messages are still useful. The main purpose of those error messages is so that user knows what we're trying to do internally and if the command fails continuously they can just try it themselves instead of using the tool.

@garg3133
Copy link
Member

I did some refactoring around the instructions and error messages in this PR, this is how the flow look like now:

image

Let me know if there's something that you think needs to be changed or fixed. Otherwise, I'll merge this PR later today.

Thanks a lot for working on this!

@garg3133 garg3133 merged commit 65c8698 into nightwatchjs:main Jun 18, 2024
2 checks passed
@itsspriyansh
Copy link
Contributor Author

@garg3133 Thanks for the merge!
Regarding the missing abd, I manually deleted adb binary inside platform-tools and then executed this command as below:

image

As you can see, it is not getting installed in my case.

@garg3133
Copy link
Member

@itsspriyansh This seems to be a bug in that case but the functionality is there. As you can see, it downloaded the platform-tools again to fix the missing adb binary but it seems that it wasn't able to replace the existing platform-tools directory with the new one for some reason.

Can you try to debug this?

@itsspriyansh
Copy link
Contributor Author

itsspriyansh commented Jun 24, 2024

@garg3133 What I have obeserved is that this issues lies with sdkmanager and not in our tool. I tried deleting adb and reinstalling it using the sdkmanager binary itself. However, it didn't work. If I clear all the contents of platform-tools or instead delete the folder itself, then it works. I was able to reproduce this issue in two of my devices (windows and ubuntu).

@itsspriyansh itsspriyansh deleted the wifi branch July 3, 2024 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connect Android devices to adb wirelessly.
4 participants