Deprecate command based programming in robotpy

[virtuald]

Bottom line: it sucks (in robotpy) and isn't pythonic, and I don't want to spend time to make it better

Creating a command based robot in python is clunky and leads to all sorts of problems. With the addition of state machine support, magicbot is more pythonic, more powerful, and easier to use.

I would propose using the warnings.warn mechanism to annoy users when they used the various command based components, and to point at a HTML page that explains why and alternatives.

[FRC2539]

We are just starting with Python, so perhaps our viewpoint will change in the future. Our first program, done only a month ago, was to modify the example command based program to drive a single motor on our prototyping system. We're pleased to say it worked well. So well, in fact, that we decided to try porting last season's program, which is C++ and command based. However, we have not gotten a chance to drive with it yet, though we hope to soon. (We're only meeting once a week during the off-season.)

Our team actually tried Python for Rebound Rumble but at that time they couldn't even get the program to start, so they went with C++. However, Python is now looking like a first-class citizen of the FRC ecosystem and many of the team programmers would rather use it. That being said, we have several years of experience with the command based paradigm and porting to something like magicbot is daunting. Here are a few use cases we have that we don't see an easy way to implement with magicbot, followed by few things we think mitigate the arguments against command based.

If there are existing solutions to the issues we've been puzzled by, listing them in the proposed warning message HTML page would probably help other teams looking to port their programs to Python.

Command-Based Tricks

In no particular order

Controller Flexibility

One of the really great things about the command based paradigms is that it makes controller coupling easy and flexible. The syntax for assigning an action to a button is just button.event(Command()). By contrast, the example given for magicbot is five lines:

try:
    if self.joystick.getTrigger():
        self.component.doSomething()
except:
    self.onException()

We have gotten into the habit of creating multiple controller layouts for different situations. For example, during development we just assign every last button on a controller to something, because it's fast for testing. At competitions we generally have two controllers (with two drivers), and we try to use buttons that are far away from each other to avoid accidentally triggering the wrong action (a lesson learned the hard way, sadly). For open houses we only want visitors to be able to drive the robot (slowly) and do a few simple actions that can't cause any damage.

With C++ we could include different files for each occasion. With Python we plan to run different functions in OI depending on the situation. To do the same with magic bot it looks like we'd either need to have conditional logic inside of teleopPeriodic() or assign different functions to teleopPeriodic.

With magicbot's seemingly much more verbose syntax for acting on a button press, that means we'd have at least five times as many lines, and possibly some extra logic checks on every tick of teleopPeriodic. That would make it a bit harder to see, at a glance, what action is assigned to a button.

Event Flexibility

The button framework, which ties in nicely with commands, has many events that can trigger (or cancel) a command. They are whenPressed, whileHeld, whenReleased, toggleWhenPressed, and cancelWhenPressed. These open up a lot of possibilities. For example, we have "modes" that are activated by pressing a button and deactivated by pressing it again using toggleWhenPressed. In the past we've used every button event except for whenReleased, though there are definitely some interesting use cases for it as well.

We don't understand exactly how button presses are related to actions in the magicbot examples, but it appears to either be the equivalent of whenPressed or whileHeld. The more complex options seem to be possible, but it also seems like they'd require more code and be less readable.

Concurrency

For shooting at a high goal, we need to rotate our robot to face the goal, move the shooter to the correct angle, and spin up the firing wheels. We could do them one at at time, but a desire to squeeze out every last second of performance led us to run them all in parallel. Since all three actions use sensors, there is no way to know which one will finish first or last, but we need all three to be finished before we launch the ball.

With command based, we can make three commands, place them in a command group to run in parallel, and add fire sequentially after that command group. It seems like that would be possible to program with magicbot state machines, but it would be more verbose and becomes increasingly complex as more actions happen in parallel.

Conflict Management

In a well written command based program, there is little danger of experimenting with commands. The scheduler ensures that commands that would conflict cannot be run in parallel. For example, if I am spinning the pickup system inward to pick up a ball and I try to run a command that spits it out, the scheduler will automatically stop the first command and give it a chance to clean up after itself before starting the second.

Being mindful of what your code is doing is important, but mistakes happen. Is there any mechanism in magicbot to make sure that different states or actions aren't using a motor that is being controlled by something else?

Reordering Steps

Every year we find that our initial plan would work better if steps were moved around. Since our commands are inside a command group, scheduled with addSequential and addParallel, it is often only a matter of dragging a line from one place to another. Anyone looking at the group can easily read through it and see when each command is triggered, since they always run from top to bottom.

It appears that in magicbot this is not the case. The methods in a state machine are ordered using a decorator. It is possible to skip a method or have them run out of order. That offers a great amount of flexibility and opens up interesting possibilities, but it is not easily grasped. Punishing people for not reading a decorator correctly is certainly fun, but not something we want to have happen during a last minute scramble in the pits.

Dashboard Feedback

Using the SmartDashboard, we can always see what commands our robot is currently running and cancel them if desired. We can also place buttons on the dashboard to run certain commands (generally commands that only get run in special circumstances instead of during a match).

Does magicbot provide any SmartDashboard feedback?

Possible Mitigations of Command Based

Too Much Boilerplate

Coming from C++ we have built up an abnormally high tolerance for boilerplate, but even there we, being the lazy people we are, were always trying to find ways to type less. Since Python is much more flexible, it would seem that it would have even better options to eliminate boilerplate.

It appears that someone has already started on this, since the Command class has its abstract methods initialize, execute, end, and interrupted replaced with empty methods, meaning that someone implementing a Command that doesn't need one of those can simply ignore it. In our code base we went a step further and created base classes for different types of commands we frequently use. That way we only need to implement the special cases.

When subclassing IterativeRobot, the periodic functions are the same for all modes (except Test) in command based, so we use the same method for all of them, resulting in less duplicated code. Someone feeling especially clever could even do something like self.teleopPeriodic = wpilib.command.Scheduler.getInstance().run and not implement those methods at all, though there are good reasons not to go that route.

Subsystems don't have much boilerplate to them, so with just a few simple changes we feel like we've dramatically reduced the amount of boilerplate we're typing. If other teams are interested, we'd be happy to try to generalize what we've done (once it's a bit more battle-tested) and it could be added to robotpy-wpilib-utilities.

An Exception Will Kill Your Program

Exceptions are going to happen, even in strongly typed languages like Java and C++. The roboRIO knows this and will restart the program if it detects a crash, so in some sense the current situation isn't too bad.

But, it could be better. The great thing about command based is that almost all interesting actions happen inside Scheduler.run(). If we wrap that method in a try/except block, we can largely guarantee that a last minute change won't kill the robot outright. Additionally, because the scheduler has access to all running commands we can easily cancel the commands that were running and might now be in an inconsistent state.

Conclusion

Command Based, magicbot, and even SimpleRobot all have their places. They provide different ways of approaching problems and make some things better and other things worse. Adding a new one is great, but deprecating an existing and recommended one without a clear migration path seems ill advised.

Obviously, we are very, very new to programming the robot with Python. However, over the past month we've tried to learn a lot and consider the different options. Hopefully our questions will spur improved documentation and make robotpy-wpilib an even better starting place for FRC teams.

Since we're only meeting once a week currently, we may not have much to add until next Wednesday when we, hopefully, get to test our ported driving code on our actual robot.

[virtuald]

Thanks for the in-depth response. I'll try to find time to address more of your concerns this weekend.

I will say that a large part of why this issue exists are that the limited times that I've dealt with Command based programming I've found it to be highly annoying -- just as I'm sure someone new to Magicbot would say about it. :) There's only a finite amount of time that I can personally spend on RobotPy, and as you can imagine I'd rather spend that time working on things that I care about. However, if others were to step in and make command-based programming a first-class citizen in RobotPy, I'm not opposed to that. I just don't have the time, experience, or will to do so myself.

[virtuald]

Controller flexibility

I agree that this is something that magicbot is not good at. I've contemplated ways to address this problem, but haven't really hit on anything that I really liked.

It would be trivial to implement a similar framework (or co-op the existing framework perhaps) that could cooperate with magicbot components, notionally something like:

def createObjects(self):
  self.button = Button(joystick=1, button=3)
  self.button.whenPressed(self.fireComponent.shoot)

The key would be to ensure that the button/etc components run first, so that they can trigger actions in the other components.

Event flexibility

This seems to be the same as above, or are there other event things in Command-based?

Concurrency, Conflict management

I'll have to think about a good response for these. Magicbot doesn't explicitly address these, but I think if one adheres to the magicbot philosophy then these are actually less of an issue. More documentation would help.

Reordering steps

These are valid criticisms.

Dashboard feedback

I extensively use NetworkTables for feedback, and have found SmartDashboard extraordinarily clunky for dealing with large numbers of variables, so I end up using TableViewer exclusively instead.

Magicbot provides extensive networktables integration, see this section of the docs. Though it's not documented very well, each state machine provides network tables variables that can be used to tune the state machine.

Command based mitigations

I welcome pull requests that address these issues.

[FRC2539]

Event Flexibility: The other concern, which doesn't seem to be clearly addressed in the magicbot documentation, is how to stop a running action cleanly. The events whileHeld, cancelWhenPressed and toggleWhenPressed do that. For example, if we set a motor's speed in initialize() and set it back to zero in end(), we can use whileHeld to manually spin the motor, and know that when we release the button the motor will stop.

Dashboard Feeback: Can tunables be automatically stored on the roboRIO, or is that information lost when the robot is shut down?

[virtuald]

Event flexibility: I can definitely put together a button framework for Magicbot. It's certainly needed.

Regarding stopping actions, this isn't explicitly called out in the documentation (though, I will definitely need to add it), but a key part of my philosophy with robot code is this: During a robot control loop iteration, if a component is not commanded to do something, it should do nothing (or a sensible default action).

When dealing with components, this is something that one needs to build in yourself. You can see this in the Shooter component in the documentation: at the end of the 'execute' function, self.enabled is set to False, thus disabling the component during the next iteration unless something tells it otherwise.

However, state machines have this idea explicitly built into them -- the state machine will only execute if someone calls engage during the control loop iteration*, else done will be called and it will cease execution.

In my opinion, this pattern makes it significantly easier to reason about the behavior of components. If you didn't just tell it to do something, then it isn't doing it (and this property makes it easier to find the component that is causing it if it's doing something). I think it's more problematic for runaway components to continue to do something even if nobody is telling it to do so.

*On occasion it's useful to continue doing something until it is completed, which is why the state machine decorators support the 'must_finish' parameter, which will cause a particular state to continue executing until something causes it to exit that state. It's less problematic for a state machine to have potentially infinite execution, as the ordering of actions is usually well defined and easy to reason about and identify.

Dashboard feedback: Currently the information is lost on shutdown. If we upgrade pynetworktables to NT3 (tentatively planned, but it's a lot of work), then one can take advantage of persistent variables and the like... though, I've not explored that much.

Other comments

Concurrency: Ok, so looking at this particular example, I see your point:

group.addParallel(self.move_arm_cmd)
group.addParallel(self.rotate_robot_cmd)
group.addParallel(self.spin_shooter_wheel_cmd)

Vs.

class MetaShooter(StateMachine):

    @state
    def wait_for_shoot(self):
        if self.arm.in_position() and self.robot.at_position() and \
           self.shooter.at_speed():
            self.next_state('do_the_shoot')

    @state
    def do_the_shoot()
        self.shooter.fire()

I think this speaks to the core difference between command-based and how I think about Magicbot then -- in command-based, sequences of actions are very important; whereas for Magicbot I think of it more about "do things when a component is ready", as opposed to "when a sequence is completed". I'm not sure I would go as far as to say one is more correct than the other though.

This also speaks to the infancy of Magicbot -- I could definitely see a meta-component that could implement scheduling like the command group. Maybe?

class MetaShooter(Group):

    def init(self):
        self.add(self.arm.engage,
                 self.robot.rotate,
                 self.shooter.spin_up)
            .until(self.arm.in_position,
                   self.robot.at_position,
                   self.shooter.at_speed)
        self.add(self.shooter.fire)

That's still a bit wordy, needs some work.

Conflict resolution: By (a) separating actions from execution (only telling motors to do things in the execute function) and (b) ensuring that a motor/etc is only directly controlled by a single component, a user can effectively ensure that they won't have conflicts. If a user doesn't abide by that... then what can we do?

Lower level components that interact directly with motors I think are really the ones that have to deal with this the most -- if something tells a shooter to enable and something else tells it to disable, who should win? At the moment, the way I deal with this is by guaranteed execution ordering (implicitly in teleopPeriodic and by order of location in class declaration), and so the 'last command wins'.

In your particular example of spit-in and spit-out of a ball -- if a single component is implementing it, then it can only be bringing a ball in, or spitting a ball out. There's no conflict here -- last command wins. I can't think of something at the moment that 'last command wins' wouldn't be able to solve.

Conclusion

There's a lot in Magicbot that heavily depends on philosophy or convention to ensure that the user does things correctly. I'm open to changes that address those problems, and at the least I hope I will be able to update the documentation to reflect some of these deeper philosophies.

If you find more constructs that just don't work well in Magicbot, I'm interested to hear more about them. If you want me to review your magicbot port of your code, I might be able to find some time to do that too.

[vanjac]

We found some similar issues with command based robots after we used them in 2017. We ended up making our own implementation based on Python Generators, which we found to have similar capabilities to commands but be cleaner and a bit more "pythonic". If anyone is interested, I wrote an explanation for how it works: https://seamonsters-2605.github.io/docs/seamonsters-generators/

[auscompgeek]

With the current proposed rewrite of the command framework, it seems like the API might actually be reasonable.

[auscompgeek]

@vanjac just had a chance to take a look at your coroutine implementation, looks neat!