On Wed, Jan 16, 2013 at 12:16:20AM -0000, Michael Hudson-Doyle wrote:
> Antonio Terceiro <email address hidden> writes:
> 
> > Antonio Terceiro has proposed merging lp:~terceiro/lava-dispatcher/nexus into lp:lava-dispatcher.
> >
> > Requested reviews:
> >   Linaro Validation Team (linaro-validation)
> >
> > For more details, see:
> > https://code.launchpad.net/~terceiro/lava-dispatcher/nexus/+merge/143385
> >
> > Hi,
> >
> > This time is for real. :-)
> >
> > I don't see any pending issues, so know you can have a hard review on the code.
> > -- 
> > https://code.launchpad.net/~terceiro/lava-dispatcher/nexus/+merge/143385
> > You are subscribed to branch lp:lava-dispatcher.
> > === modified file 'lava_dispatcher/config.py'
> > --- lava_dispatcher/config.py	2013-01-10 21:37:58 +0000
> > +++ lava_dispatcher/config.py	2013-01-15 19:17:23 +0000
> > @@ -91,6 +91,9 @@
> >      arm_probe_config = schema.StringOption(default='/usr/local/etc/arm-probe-config')
> >      arm_probe_channels = schema.ListOption(default=['VDD_VCORE1'])
> >  
> > +    adb_command = schema.StringOption()
> > +    fastboot_command = schema.StringOption()
> > +    nexus_working_directory = schema.StringOption(default = None)
> 
> It's super nitty, but "default=None" (i.e. without the spaces) would be
> more conventional.

Fixed.

> >  class OptionDescriptor(object):
> >      def __init__(self, name):
> >
> > === added file 'lava_dispatcher/default-config/lava-dispatcher/device-types/nexus.conf'
> > --- lava_dispatcher/default-config/lava-dispatcher/device-types/nexus.conf	1970-01-01 00:00:00 +0000
> > +++ lava_dispatcher/default-config/lava-dispatcher/device-types/nexus.conf	2013-01-15 19:17:23 +0000
> > @@ -0,0 +1,40 @@
> > +client_type = nexus
> > +
> > +# The ADB command line.
> > +#
> > +# In the case where there are more than one android devices plugged into a
> 
> s/are/is/ or s/more than one/multiple/ (English grammar makes no sense)

Fixed

> > +# single host, this connection command must be overriden on each device to
> > +# include the serial number of the device, e.g.
> > +#
> > +#   serial_number = XXXXXXXXXXXXXXXX
> > +#   adb_command = adb -s %(serial_number)s
> > +adb_command = adb
> > +
> > +# The fastboot command.
> > +#
> > +# The same as above: if you have more than one device, you will want to
> > +# override this in your device config to add a serial number, e.g.
> > +#
> > +#   serial_number = XXXXXXXXXXXXXXXX
> > +#   fastboot_command = sudo fastboot -s %(serial_number)s
> > +#
> > +# Of course, in the case you override both adb_command *and* fastboot_command,
> > +# you don't need to specify `serial_number` twice.
> > +fastboot_command = sudo fastboot
> > +
> > +# Working directory for temporary. By default, the usual place for LAVA images
> > +# will be used.
> > +#
> > +# This is useful when the lava dispatcher is controlling Nexus phones that are
> > +# physically plugged to other machines. In this case, you should set
> > +# nexus_work_directory to a shared directory between the machine running the
> > +# dispatcher and the machine where the phone is plugged.
> 
> I don't entirely understand when this would be useful.

I will try to explain it better. :)

> > +# OBS: this shared directory must have the same path in both machines.
> 
> OBS?

heh, that's the equivalent of "N.B." in Portuguese, I guess it doesn't
exist in English. :) I will rewrite that doc, so for now I just removed
the OBS.

> > +nexus_working_directory =
> > +
> > +connection_command = %(adb_command)s shell
> > +
> > +enable_network_after_boot_android = false
> > +android_adb_over_usb = true
> > +android_adb_over_tcp = false
> >
> > === added file 'lava_dispatcher/device/nexus.py'
> > --- lava_dispatcher/device/nexus.py	1970-01-01 00:00:00 +0000
> > +++ lava_dispatcher/device/nexus.py	2013-01-15 19:17:23 +0000
> > @@ -0,0 +1,168 @@
> > +# Copyright (C) 2012 Linaro Limited
> > +#
> > +# Author: Antonio Terceiro <email address hidden>
> > +#
> > +# This file is part of LAVA Dispatcher.
> > +#
> > +# LAVA Dispatcher is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 2 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# LAVA Dispatcher is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.    See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along
> > +# with this program; if not, see <http://www.gnu.org/licenses>.
> > +
> > +import subprocess
> > +import pexpect
> > +from time import sleep
> > +import logging
> > +import contextlib
> > +
> > +from lava_dispatcher.device.target import (
> > +    Target
> > +)
> > +from lava_dispatcher.downloader import (
> > +    download_image
> > +)
> > +from lava_dispatcher.utils import (
> > +    logging_spawn,
> > +    mkdtemp
> > +)
> 
> OK, so this is probably mostly the dispatcher's fault for being
> confusing, but there are a few things I don't really understand here.
> If I get you to explain them to me in excruciating detail, maybe we can
> make the code clearer for the next person.
> 
> > +class NexusTarget(Target):
> > +
> > +    def __init__(self, context, config):
> > +        super(NexusTarget, self).__init__(context, config)
> > +        self._powered_on = False
> 
> This seems like an assumption.  How likely is it that the device is
> actually powered down?  If it's important that it is, maybe we should
> power it down explicitly (although probably not in __init__).

OK.

> > +        self._working_dir = None
> > +
> > +    def deploy_android(self, boot, system, userdata):
> > +
> > +        boot = self._get_image(boot)
> > +        system = self._get_image(system)
> > +        userdata = self._get_image(userdata)
> > +
> > +        self.reboot_os()
> 
> So, let me get this straight.  There are a few states we could be in:
> 
> 1. At fastboot.
> 2. Powered down.
> 3. In android.
> 4. Hung.
> 
> We want to be in the fastboot state.  Maybe we should call a function
> that makes that clear?  I guess I'm saying that you should name
> functions in this class by the state they result the device being in,
> rather than what they do, "intent revealing names" and all that.  In
> addition, I think I'd find this clearer if the internal methods had
> _underscore names.
> 
> If we're in state "4. Hung" we're doomed?

I think we can power it off. I'll think of a way of forcing the system
to hang and test that.

> > +        self.fastboot('erase boot')
> > +
> > +        self.fastboot('flash system %s' % system)
> > +        self.fastboot('flash userdata %s' % userdata)
> > +
> > +        self.deployment_data = Target.android_deployment_data
> > +        self.deployment_data['boot_image'] = boot
> > +
> > +    def power_on(self):
> > +        self.reboot_os()
> > +        self.reboot_bootloader()
> 
> I don't understand why reboot_bootloader is here.  It seems to me that
> we're in the same situation as deploy_android -- we don't know the state
> and we want to be in fastboot.

There is a weirdness with fastboot: if you don't reboot it before
booting the actual kernel, it will enter charging mode and won't boot
without physical intervention.

> > +        self.boot_test_image()
> > +
> > +        self._powered_on = True
> > +        proc = self.adb('shell', spawn = True)
> > +        proc.sendline("") # required to put the adb shell in a reasonable state
> > +        proc.sendline("export PS1='%s'" % self.deployment_data['TESTER_PS1'])
> > +        self._runner = self._get_runner(proc)
> > +
> > +        return proc
> > +
> > +    def reboot_os(self):
> > +        # tell android to reboot. A failure probably means that the device is not
> > +        # booted on android, and we ignore that.
> 
> It would be nice not to make assumptions here.

Sure.

> > +        self.adb('reboot', ignore_failure = True)
> > +        sleep(10)
> 
> Are these sleeps really necessary?  AIUI, issuing a fastboot command
> will wait for the device if it's not yet ready for it.

I will check that.

> > +    def reboot_bootloader(self):
> > +        self.fastboot('reboot')
> > +        sleep(10)
> > +
> > +    def boot_test_image(self):
> > +        self.fastboot('boot %s' % self.deployment_data['boot_image'])
> > +        self.adb('wait-for-device')
> > +
> > +    def power_off(self, proc):
> > +        # there is no way to power off the Nexus while USB is plugged on; even
> > +        # if you remove power, it will stay on.
> > +        pass
> 
> :( Maybe we'll get some of Andy Green's usb disconnector devices.

No, I discovered a way to consistently turn power off now (see my
response to Andy's comments)

> > +    @contextlib.contextmanager
> > +    def file_system(self, partition, directory):
> > +
> > +        if not self._powered_on:
> > +            self.power_on()
> > +
> > +        mount_point = self.get_partition_mount_point(partition)
> > +
> > +        with self.make_filesystem_readwrite(mount_point):
> 
> I /thought/ we generally didn't make modifications to /system.  But I
> guess you wouldn't have done this unless it was needed...

This was needed when I was trying to copy the mkfifo binary into the
device, I can probably remove it now.

> > +            host_dir = '%s/mnt/%s' % (self.working_dir, directory)
> > +            target_dir = '%s/%s' % (mount_point, directory)
> > +
> > +            subprocess.check_call(['mkdir', '-p', host_dir])
> > +            self.adb('pull %s %s' % (target_dir, host_dir), ignore_failure = True)
> > +
> > +            yield host_dir
> > +
> > +            self.adb('push %s %s' % (host_dir, target_dir))
> > +
> > +    def get_partition_mount_point(self, partition):
> > +        lookup = {
> > +            self.config.data_part_android_org: '/data',
> > +            self.config.sys_part_android_org: '/system',
> > +        }
> > +        return lookup[partition]
> > +
> > +    @contextlib.contextmanager
> > +    def make_filesystem_readwrite(self, mount_point):
> > +        if mount_point  == '/system':
> > +            self._runner.run("mount -o remount,rw %s" % mount_point)
> > +        yield
> > +        if mount_point  == '/system':
> > +            self._runner.run("mount -o remount,ro %s" % mount_point)
> > +
> > +    def get_device_version(self):
> > +        # this is tricky, because fastboot does not have a visible version
> > +        # number. For now let's use just the adb version number.
> > +        return subprocess.check_output(
> > +            "%s version | sed 's/.* version //'" % self.config.adb_command,
> > +            shell = True
> > +        ).strip()
> > +
> > +    def adb(self, args, ignore_failure = False, spawn = False):
> > +        cmd = self.config.adb_command + ' ' + args
> > +        if spawn:
> > +            return logging_spawn(cmd, timeout = 60)
> > +        else:
> > +            self._call(cmd, ignore_failure)
> > +
> > +    def fastboot(self, args, ignore_failure = False):
> > +        self._call(self.config.fastboot_command + ' ' + args, ignore_failure)
> > +
> > +    def _call(self, cmd, ignore_failure):
> > +        logging.debug("Running on the host: %s" % cmd)
> > +        if ignore_failure:
> > +            subprocess.call(cmd, shell = True)
> > +        else:
> > +            subprocess.check_call(cmd, shell = True)
> 
> It would be nice to have a timeout for all calls somehow.  Not sure how, though.

I will look at that.

> > +    def _get_image(self, url):
> > +        sdir = self.working_dir
> > +        image = download_image(url, self.context, sdir, decompress=False)
> > +        return image
> > +
> > +    @property
> > +    def working_dir(self):
> > +        if (self.config.nexus_working_directory is None or
> > +            self.config.nexus_working_directory.strip() == ''):
> > +            return self.scratch_dir
> > +
> > +        if self._working_dir is None:
> > +            self._working_dir = mkdtemp(self.config.nexus_working_directory)
> > +        return self._working_dir
> > +
> > +
> > +target_class = NexusTarget
> 
> Sorry for all the questions!  Having a slow-brained day today it seems.

Nothing to be sorry for - all of them were very useful!

-- 
Antonio Terceiro
Software Engineer - Linaro
http://www.linaro.org