Code architecture: Should pilot exist outside of the API?

[AhmedSamara]

(look at tag 1.0 to vie the old pilot).

The old pilot existed outside of the API. The basic structure was that ctrl_server.py was the class that built and owned all other subsystems, and pilot was just another client that called on that server, just like the CLI or Android app.

This makes sense in the context of the architecture, but it also results in some strange pilot code which instead of calling functions the way you normally would in python, looked like this:

self.call('follower', 'follow', {'heading': self.heading})

As opposed to:

follow.follow(heading)

Which I think is needlessly difficult to read and interpret.

My suggestion was to just make pilot another subsystem which was then called from the CLI.
Then we could just ssh in, start the server, possibly do some last minute checking on the bots systems, and then start pilot.run(), and yank out the NICs and put it in the waiting area.

@dfarrell07, what are your thoughts on this?

[dfarrell07]

There are basically two features you're asking for here, if I'm reading this correctly.

This makes sense in the context of the architecture, but it also results in some strange pilot code

First, that there's some difficult to understand code. +1, typically a code smell that calls for refactoring.

My suggestion was to just make pilot another subsystem which was then called from the CLI.

Second, call Pilot.run from the CLI instead of start.py. Also makes sense, start.py is from way before we had an API, Client, Server, etc. We've already moved testing from ./start.py -t to the simple/standard tox.

The old pilot existed outside of the API.

There is a good reason for this, which I'll detail later.

/me will post this now to get something shared ASAP, then give details in additional posts

[dfarrell07]

First, that there's some difficult to understand code. +1, typically a code smell that calls for refactoring.

This seems like a good use-case for helper functions.

self.call('follower', 'follow', {'heading': self.heading})

This^^ could turn into

self.follow(heading)

Using a simple helper like

def follow(heading):
    """Helper for calling the main line following function, Follower.follow. Passes heading.

    :param heading: Heading to pass to Follower.follow as its param
    :type heading: int
    :returns: Simple hand-off of result returned by the API call, so the follow fn

    """
    self.call('follower', 'follow', {'heading': heading})

[dfarrell07]

Second, call Pilot.run from the CLI instead of start.py. Also makes sense, start.py is from way before we had an API, Client, Server, etc. We've already moved testing from ./start.py -t to the simple/standard tox.

You could do this by simply adding CLI commands similar to the ones that already exist, and have them run basically the same code start.py uses.

For example

    def do_ping(self, raw_args):
        """Ping the control server on the bot.

        :param raw_args: Mandatory param for Cmd handler, not used.
        :type raw_args: string

        """
        reply_time = self.ctrl_client.ping()
        print "CtrlServer response time: {}ms".format(reply_time)

    def help_ping(self):
        """Provide help message for ping command."""
        print "ping"
        print "\tPing the control server on the bot."

Could be modified in the expected ways (name, docstring), plus a logic modification that basically dumps

if args.pilot:
    import pilot as pilot_mod
    print "Starting pilot"
    pilot = pilot_mod.Pilot()
    pilot.run()

In the do_pilot method.

You'd likely also want to pass the CLI's instance of CtrlClient to Pilot.run as a new param.

[dfarrell07]

The old pilot existed outside of the API.

There is a good reason for this, which I'll detail later.

The issue with putting Pilot "inside" the API, to call other resources directly (Follower.follow), is how it gets that direct access to basically all of the bot's instantiated resources. My first impression is that doing so would create a mess. Many things shouldn't be instantiated more than once. One big benefit of the current architecture is that there's a single, clear owner of the bot's resources (CtrlServer), and it serves those resources via a trivial-to-extend API to clients through a single, standard code path.

    def assign_subsystems(self):
        """Instantiates and stores references to bot subsystems.

        :returns: Dict of subsystems, maps system name to instantiated object.

        """

        self.follower = Follower()

        systems = {}
        systems["ctrl"] = self
        systems["follower"] = self.follower
        systems["driver"] = self.follower.driver
        systems["ir_hub"] = self.follower.ir_hub
        self.logger.debug("Systems: {}".format(systems))
        return systems

While you could write syntactically valid Python to get around this, but I don't think it's the best approach. You'd have to pass resources Pilot after they are created in CtrlServer, then add logic for providing access to the scattered methods it calls (which would be served up so clearly by the API via @lib.api_call decorators).

[dfarrell07]

I'd almost need to implement it, or start doing so, to be totally sure about all of this. Not going to be able to give more feedback tonight (sick, have to get up early), but I'll think about the problem. How urgent is this, @AhmedSamara?

[dfarrell07]

Then we could just ssh in, start the server, possibly do some last minute checking on the bots systems, and then start pilot.run(), and yank out the NICs and put it in the waiting area.

As I was trying to fall asleep last night, I realized that this might be a third feature you're requesting. Are you also working on how to kick off Pilot, then disconnect totally, for the contest?

If so, you could do it in a tmux session using either the current ./start.py -p method or the CLI method I described above. Just leave it running, disconnect from tmux, kill your SSH session and rip the NICs.

[AhmedSamara]

Well that was one aspect of this that I wanted to make sure we could account for at competition time, but I think the CLI-pilot would still be able to do that since the bot generally continues moving even when the CLI has been closed, so I don't think that part would become too much of an issue.

What I'm concerned about is just how strange the code gets, because of this.

I guess wrapping everything in headers is reasonable, but I was hoping to not have to add so much code just to add wrappers to things that already seemed so strange.

[dfarrell07]

What I'm concerned about is just how strange the code gets, because of this.

I think what seems "strange" here is that you're working with an API, instead of directly with instances of objects (like you'd almost always do in EE/ECE, and really even undergrad CS). This is pretty common in larger projects, however (we couldn't do remote clients, like the CLI and Android app, without one). The standard answer for consuming APIs via clean code is wrapper-type logic. You can do that wrapping in elegant ways, some variant of a lib. I'll think about this more.

[AhmedSamara]

I'll just go ahead and implement this, the plan is to get a basic pilot working by the end of spring break, and I think it's better to just continue following the current style instead of wasting more time re-structuring things even though I'm not entirely happy with it.

[dfarrell07]

but I think the CLI-pilot would still be able to do that since the bot generally continues moving even when the CLI has been closed, so I don't think that part would become too much of an issue.

That's just because the last command sent CLI -> CtrlClient -> CtrlServer -> driver/follower/whatever was still being executed. If it set the bot to drive forward, it would keep doing so until another message to stop.

[dfarrell07]

I'll just go ahead and implement this, the plan is to get a basic pilot working by the end of spring break, and I think it's better to just continue following the current style instead of wasting more time re-structuring things even though I'm not entirely happy with it.

+1

We should talk more about whatever you're not happy with. It's likely a fixable issue, and one that should be fixed. :)

[AhmedSamara]

That's just because the last command sent CLI -> CtrlClient -> CtrlServer -> driver/follower/whatever was still being executed. If it set the bot to drive forward, it would keep doing so until another message to stop.

The important point there being that once something is called from the CLI, it can keep going and it doesn't necessarily have to be run from ./start.py -p.

Writing helper functions isn't an issue perse, it just feels sloppy to me that we have all this functionality, I'm just writing identical functions on the other side of (hehehe) the API to do the same thing done in each class. It just feels sloppy and I think it'll make it hard for future members to follow the code and understand what's going on.

[dfarrell07]

It really comes down to how complex/messy it ends up being to put Pilot in the same process space as the CtrlServer (managing bot resource instances), instead of the process spaces of start.py or the CLI (addressing API via CtrlClient).

The more I think about it, the less messy it sounds, which is good because I initially thought it was going to be very ugly. I'm tempted to take a quick stab at implementing the change on a branch, or at least starting it (warning: I frequently don't find time for things). Or maybe it's better to let someone else do it? @AhmedSamara?

[AhmedSamara]

Well I've already started implementing the old way on pilot_dev, the "helper function and API" way.

I can take a stab at trying to implement it if you have time to just describe what you're thinking. Would it be the same as what I described before?

[dfarrell07]

I can take a stab at trying to implement it if you have time to just describe what you're thinking.

I can try to write up a nice overview of the changes, or how I'd do them. I'm not familiar with prios of various issues and if this one blocks anything, so feedback on when it needs to be done by would be helpful. This weekend sometime?

[AhmedSamara]

@mynameis7