robotpy / robotpy-wpilib

Moved to https://github.com/robotpy/mostrobotpy
https://robotpy.github.io
Other
169 stars 59 forks source link

Why no loops in IterativeRobot? #197

Closed CrazyPython closed 7 years ago

CrazyPython commented 8 years ago

As an experienced programmer, I don't undertsand why you shouldn't use loops in xxxPeriodic.

Avoid using loops, as unexpected conditions may cause you to lose control of your robot.

So no terminating for or while loops?

as unexpected conditions may cause you to lose control of your robot.

That's just a risk you have to take. Loops are a key aspect of programming. With enough testing, this can be prevented. And a timer can be used to interrupt the loop if it goes for too long.

virtuald commented 8 years ago

As an experienced programmer, you know enough to know when rules/guidelines are able to be broken. While those guidelines may be useful for you, that particular guideline is aimed at beginners.

I agree that typically for loops are fine, though there are exceptions.

Using a loop that runs more than a few iterations prevents the robot code from doing anything else (unless you use threads, which is often an antipattern in python) during that period of time. As a CSA, I've seen far too many robots hang because of unexpected conditions. This guideline is especially intended for this far-too-commonly-seen pattern:

def teleopPeriodic(self):

    if self.joystick.getTrigger():
        while not self.arm.isAtPosition():
            self.arm_motor.set(1)

        while not self.arm.fireComplete():
            self.arm_motor.set(0)
            self.fire_motor.set(1)

        self.fire_motor.set(0)

I'm sure it's obvious that if the arm sensor breaks, the first loop will never terminate. You certainly can fix this code by adding in a timer and testing it, but I think it's better to avoid it in the first place. I prefer teaching my students to use a stateful version instead:

arm_state = 'idle'

def teleopPeriodic(self):

    if self.joystick.getTrigger():
        if self.arm_state == 'idle':
            self.arm_state = 'move'

        if self.arm_state == 'move':
            self.arm_motor.set(1)

            if self.arm.isAtPosition():
                self.arm_state = 'fire'

        if self.arm_state == 'fire':
            self.arm_motor.set(0)
            self.fire_motor.set(1)

            if self.arm.fireComplete():
                self.arm_state = 'done'
    else:
        self.arm_state = 'idle'

        # unset motors in case trigger is released before action is done
        self.arm_motor.set(0)
        self.fire_motor.set(0)

Now obviously the first version is simpler, but the second version is easily interruptible and doesn't suffer from any potential infinite loops.

Of course, there's a lot of boilerplate in that second one, and it's harder to understand, which is why I created a state machine helper for magicbot.

def teleopPeriodic(self):
    if self.joystick.getTrigger():
        self.arm.fire()

class Arm:

    def fire(self):
        self.engage()

    @state(first=True)
    def move_arm(self):
        if self.isAtPosition():
            self.arm_motor.set(0)
            self.next_state('firing')
        else:
            self.arm_motor.set(1)

    @state
    def firing(self):
        self.fire_motor.set(1)
        if self.fireComplete():
            self.done()

    def done(self):
        super().done()

        # unset motors in case trigger is released before action is done
        self.arm_motor.set(0)
        self.fire_motor.set(0)

Still some boilerplate, but infinitely more readable than the second and safer than the first.

CrazyPython commented 8 years ago

@virtuald hm, what about auto_break(...) class wrapped around a for loop iterable with a overrided __iter__? seems like a good solution...

james-ward commented 8 years ago

Seems like a good solution to what? There is no problem. If you use loops in functions called by the main xxxPeriodic functions, you risk losing control if they never terminate. So avoid doing that - either by not doing it in the first place, or by working around it with some complicated structure. This is not a defect. @virtuald is right - anything other than "don't do it" is bad advice for rookie programmers. If you are wise enough to understand why it is a problem, you probably have the chops to work around it.

CrazyPython commented 8 years ago

@james-ward I mean a auto_break wrapper class in order to prevent inf loops, that auto breaks for too many iters

james-ward commented 8 years ago

I'm sure that would work. I just don't understand the point of this issue that you have lodged. Are there ways to use loops such that they won't fail to return control to the main program? - sure there are. But they shouldn't be used as a matter of course, because when you forget the magic incantation of wrapping the loop or other voodoo, your robot will break. You do it at your own risk (and it is a risk), and telling novices that the risk is acceptable is poor advice because they are in no position to make that judgement themselves. It is better for them to avoid the risk altogether.

virtuald commented 8 years ago

A wrapper like that would address the infinite loop problem, but it doesn't address the problem of the robot not being able to do something else while the loop is happening.

Since we're in python, technically you could wrap something like this with a generator, and that would address the problem of infinite looping and parallel execution -- though, as James says, it would break if you forgot to use the magic voodoo.

I've contemplated use of generators in RobotPy related stuff, but haven't really done anything there yet. The yeti framework uses asyncio + generator based coroutines, but I think there's still a bit of refinement needed before I would recommend something like to novices.