1190280 - [mozdevice] add support for wireless adb connection

Status: RESOLVED WONTFIX

(Testing :: Mozbase, defect)

Description

[Ying Ruei Liang [:KK]]

In mozdevice 0.46, if we create mozdevice.ADBAndroid with IP address and port as initial argument, it throw error about it while constructing. I've hacked some code and part of its functions work fine. Since these functions all use adb, it should work for the others.

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

See also bug 1140406, in case using adb.py is an alternative for you.

Comment 2

[Ying Ruei Liang [:KK]]

Hi, James. I found that you're the author of the codes that reject device serials that containing ":" character, so I need your help and advice.

As I know, even device serial contain ":" character, ADBCommand.command_output() and ADBDevice.shell_output() both work. Does ":" character still broke other stuff or there's something I miss?

Comment 3

[James Graham [:jgraham]]

I'm really sorry but I honestly don't remember what the issue was here. It might have been that device numbers with a : in were being treated as an ip address by adb, and so failing. Therefore if you are actually using an ip address rather than a serial number with a : that could well be fine, but you probably want to make that case explicitly supported in the code so that the : check still exists for the non-ip-address case.

Comment 4

[Ying Ruei Liang [:KK]]

Attachment: Use re to checkout device serial format

Hi, Bob. Although this patch doesn't break anything on my computer, in case it won't work on other devices or use case, is it possible to validate this patch? Also, any suggestion for this patch?

Comment 5

[Bob Clary [:bc] (inactive)]

Comment on attachment 8642943 [details] [diff] [review]
Use re to checkout device serial format

Ying, your attachment is empty. Please check how you created it and I'll take a look when you upload it again.

Comment 6

[Ying Ruei Liang [:KK]]

Attachment: Use re to checkout device serial format

I'm sorry about that, I've uploaded it again.

Comment 7

[Bob Clary [:bc] (inactive)]

Comment on attachment 8643055 [details] [diff] [review]
Use re to checkout device serial format

Review of attachment 8643055 [details] [diff] [review]:
-----------------------------------------------------------------

Looking at http://developer.android.com/tools/help/adb.html#wireless, when more than one device is connected, using adb usb or adb tcpip 5555 will fail with 

error: more than one device and emulator

In addition, the steps involved in setting up a wireless adb connection do not seem to provide any benefit, require adb connect #.#.#.# instead of specifying a device serial number and in fact happen to fail on my local linux box. It would be helpful to understand the full use case of what you are trying to do and why.

Thanks for the interest in adb.py!

::: testing/mozbase/mozdevice/mozdevice/adb.py
@@ +604,5 @@
>                  return
>              device = devices[0]
>  
>          def is_valid_serial(serial):
> +            if re.match('^\d+\.\d+\.\d+\.\d+:\d+$|^usb:.*', serial):

Note that this will be true only if serial is of the form of an ip address or begins with usb:. You will end up rejecting normal usb device serial numbers.

@@ -604,5 @@
>                  return
>              device = devices[0]
>  
>          def is_valid_serial(serial):
> -            return ":" not in serial or serial.startswith("usb:")

Note this returns True if serial does not contain a colon or if it begins with usb:

@@ +612,1 @@
>  

Assuming the regular expression was correct in allowing ip addresses, usb: prefixes and device serial numbers, you could have written this as a simple return without the need for the conditional. For example,

return re.match(pattern, serial)

for some appropriate pattern.

Although the pattern '^\d+\.\d+\.\d+\.\d+:\d+$|^usb:.*' is not sufficient, if it had been you could have written it simpler as '((\d+\.){3})\d+:\d+$|usb:'
See https://docs.python.org/2/library/re.html#regular-expression-syntax for more details on regular expression syntax.

@@ +615,5 @@
>              if not is_valid_serial(device):
> +                err_msg = ("Device serials '%s' is invalid. "
> +                           "Pass the output from ADBHost.devices() for the "
> +                           "device instead" % device)
> +                raise ValueError()

You separate the message into a separate variable but don't use it. It is also not clear to me if ADBHost.devices() would be appropriate for your use case enabling adb over tcp/ip.

Comment 8

[Ying Ruei Liang [:KK]]

Hi bob, thanks for your feedbacks!

I'm currently working on a project, Mozbench, which its goal is to provide a testing framework for browsers (focus on firefox for now) on many platforms. So it's easy for me to found that Mozbench can connect B2G with IP address as device serial but failed on Android, although they're both Android based. The reason is simple, we use DeviceManager interface for connecting B2G and ADBHost interface for Andoird.

I spent some time reading their code and found that the implementations are really similar. So I think there's no reason that DeviceManagerADB supports wireless connection but ADBHost can't.

To be frankly, without wireless connection won't brother me at this stage, I just want to support more functionalities as possible. But after more devices released, like FxOS TV, which born without USB port, wireless connection would be necessary. 

BTW, why these two similar classes exist and which is encouraged to use?

Comment 9

[Ying Ruei Liang [:KK]]

Attachment: Use re to checkout device serial format

This patch should allow user to specify device serial as IP address for remote connection. 

BTW, will device serial start with "usb:" in recently adb? I never see this form no matter connected with USB cable or remotely.

Comment 10

[Bob Clary [:bc] (inactive)]

(In reply to Ying Ruei Liang [:KK] from comment #9)
> Created attachment 8647507 [details] [diff] [review]
> Use re to checkout device serial format
> 
> This patch should allow user to specify device serial as IP address for
> remote connection. 
> 

KK: I still don't understand the use case here. Do your b2g devices automatically start up with adb tcpip enabled? My flame doesn't but it is possible to set a property to force it.

If the device doesn't start with adb tcpip enabled, then the device must be connected to a host via usb in order for adb tcpip to be enabled. If you already have a usb connection to the device, what is the benefit of enabling adb over tcpip?

If the device does start with adb tcpip enabled, then I can understand the potential use though relying on network access for adb instead of usb would cause me to worry about network disconnections.

I don't think just allowing the ADBDevice parameter device to contain a host:port will get you what you want. You will still need to issue an adb connect in the same way that devicemanagerADB does. If we decide to support this use case we would need implementations of connect and disconnect as well.

If the use case is to have a host machine  with directly connected devices, then a client machine that issues adb commands to the devices via the host over the network, then I believe adb.py supports that through the adb_host, adb_port parameters.

for example, on the host cleo where the devices are connected via usb:

$ adb kill-server
$ adb -a fork-server server
OK

On the client machine where you want to issue commands:

$ adb -H cleo  -P 5037 devices
List of devices attached 
e479da4f	device

wlach, davehunt: Looking at the checkins related to this in devicemanagerADB, you seem to be the people to ask about this.

devicemanagerADB does seem to support connecting to a device via IP:PORT when the device is running adbd in tcpip mode, but is that really useful since you need enable tcpip over usb first anyway?

Is just using the ADBDevice/adb.py with the adb_host and adb_port parameters sufficient for KK's use case?

> BTW, will device serial start with "usb:" in recently adb? I never see this
> form no matter connected with USB cable or remotely.

jgraham added support for it in bug 1050896

You can see it in action via:

$ adb devices -l
List of devices attached 
HT018P800097           device usb:1-8

$ adb -s usb:1-8 shell
# ls
acct                 init                 sd-ext
cache                init.goldfish.rc     sdcard
config               init.mahimahi.rc     sys
d                    init.rc              system
data                 mnt                  ueventd.goldfish.rc
default.prop         proc                 ueventd.mahimahi.rc
dev                  root                 ueventd.rc
etc                  sbin                 vendor

Comment 11

[Bob Clary [:bc] (inactive)]

Comment on attachment 8647507 [details] [diff] [review]
Use re to checkout device serial format

Review of attachment 8647507 [details] [diff] [review]:
-----------------------------------------------------------------

setting the review to blank for now. Let's decide the use case and if we go this route we will need an implementation of connect and disconnect as well.

Comment 12

[Dave Hunt [:davehunt] [he/him] ⌚BST]

wlach, davehunt: Looking at the checkins related to this in devicemanagerADB, you seem to be the people to ask about this.

This was added to support issuing ADB commands to remote devices. These devices are still connected via USB, but to a remote machine running ADB, which we target via the host and port options.

Comment 13

[Geoff Brown [:gbrown]]

It has been a long time now without defining a compelling use case.

Comment 14

[Bob Clary [:bc] (inactive)]

We *might* need this with bitbar and battery current draw testing. In that case adb dumpsys batterystats only updates when usb is disconnected though we are looking into the case where a low voltage insufficient to charge is enough to get batterystats to update. The use case would be something like

connect to the device via tcp/ip
turn off the usb port
communicate with the device over tcp/ip
get updated batterystats during a test
turn on the usb port
disconnect the tcp/ip device

we can file a new bug if we find that later that we do need the support.