[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Enable adb port in tramp adb

From: Ashi
Subject: Re: Enable adb port in tramp adb
Date: Wed, 12 Nov 2014 09:44:32 +0800

Hi, Michael,

On Tue, Nov 11, 2014 at 9:25 PM, Michael Albinus <address@hidden> wrote:
Ashi <address@hidden> writes:

> Hi, Michael,

Hi Zhongwei,

> I've updated the patch:
> - add tramp-adb-connect-if-not-connected feature
> - remind user if unnecessary port number is provided in USB mode
> - discover port number for user if port number is not provided in TCP mode

Thanks. We're making progress!

As usual, I've played with this. And as usual, I do nitpick :-)

Not at all! I real appreciate your review and helping! I've learned a lot in it.

> +(defcustom tramp-adb-connect-if-not-connected nil
> +  "Try to run adb connect if provided device is not connected currently. It is
> +in tramp-adb-get-host-for-execution."

The Emacs coding guidelines do require the first line of a docstring to
be exactly a sentence (or two sentences, if they fit). Everything else
must start in the next line. See (info "(elisp) Documentation")

> +  :group 'tramp
> +  :version "24.4"

Well, Emacs 24.4 has been released. Your patch cannot be merged to that
anymore ... use 24.5.

> +  :type '(choice (const nil) (const t)))

I would rather use :type 'boolean

> +(defun tramp-adb-search-host-in-devices (host devices)
> +  "Returns host:port string by searching host in the devices list. If
> +failed, return nil."

Again, docstring conventions.

> +  (if (null devices) nil

(when devices

> +           (if (tramp-adb-execute-adb-command
> +                '["adb" nil "" "/" nil] "connect" exe-name)

You create a dummy vector, because tramp-adb-execute-adb-command adds an
additional argument when there is a host name. This looks like a hack;
better would be to fix this in tramp-adb-execute-adb-command. For
example, you could check whether (car args) is "shell". Or something
like this.

> +             ;; If port is not provided, adb will connect to default port number
> +             ;; 5555, which must be appended to return to get a full host name.
> +             (if port
> +                 exe-name
> +               (format "%s:%s" host "5555"))))

For the time being it might be OK to hardwire 5555. However, it is
always a good idea to make it configurable, for example via
tramp-methods. Look, how a default port is configured in tramp-sh.el.

> -     (setq devices (mapcar 'cadr (tramp-adb-parse-device-names nil)))
> +     (append devices (mapcar 'cadr (tramp-adb-parse-device-names nil)))

Did you forget "(setq devices"?
No, I've set devices in local variable initialization:  (devices (list exe-host))) 

Well, then I made some tests. I have the following settings:

# adb devices
List of devices attached
015d24bc55282409        device device

In Emacs, I type "C-x f /adb: TAB". The completion buffer looks then

Possible completions are:
adb:015d24bc55282409:                  adb:

Hmm, the last entry isn't like it should be.

Yes, it is a bug. I haven't fixed it yet in this update. I'll update the patch again when I solve this.

When I type "C-x f /adb:android-c05807abea645fdf: RET" everything is
OK. Now I try "C-x f /adb: RET" and I get an error
message. Still OK.

Now I set tramp-adb-connect-if-not-connected to t, and try again "C-x f
/adb: RET", and I don't get a Lisp error. Only in the
connection buffer, I see

Process *tramp/adb* exited abnormally with code 1
error: device offline

"device offline" is a error in adb itself, I guess we can't handle it in tramp. When got this error, I usually restart my device and try again... not a good solution though :/

And when I check it from the shell, there is

# adb devices
List of devices attached
015d24bc55282409        device device      offline

Is it intended like this?

> Best regards,
> Zhongwei

Best regards, Michael.
Best regards,

Attachment: adb_port_enable.patch
Description: Text Data

reply via email to

[Prev in Thread] Current Thread [Next in Thread]