Making more reusable commands

[Joe Ross]

Last year, we used Java with the command framework. I was very happy with how it ended up, our code was the best organized and easiest to modify that it's ever been. We used RobotBuilder, which reduced much of the tediousness of creating subsystems. However, we ended up with over 60 commands.

We did end up with a lot of very similar commands, that could be abstracted to the following:

turning on a motor at a fixed speed
turning off the motor
extending a pneumatic solenoid
retracting a pneumatic solenoid
toggling a pneumatic solenoid.

For each of these, we made methods in the subsystem for a particular motor or a particular solenoid that translated a forward or reverse on the solenoid to an extend or retract (which depended on which way the solenoid was wired and plumbed).

I'd like to be able to reduce the number of very similar commands and subsystem methods

I had a few thoughts, but none of us are very experienced with OO design patterns, so I'm hoping someone has a better way.

1) create generic commands, and make the solenoid/motor in the subsystem public, and pass the solenoid/motor to the command. This is probably the simplest, but I lose the ability to easily translate a forward/reverse to an extend/retract in one place.

2) do #1, but extend the solenoid class to have a parameter to determine whether forward is extend or retract. This solves #1s problem, but then it makes it much harder to use robot builder

3) make a solenoid interface for extend/retract, and have the subsystem implement the interface. Then you could pass the subsystem to a generic command. However, you would be limited to a single solenoid per subsystem.

Does anyone have other ideas? What did your team do?

[Kevin Sevcik]

You can make commands generic-ish by passing parameters to a parameterized constructor, then having that command pass that parameter to a parameterized method of your subsystem. I'll type up a quick c++ example in a bit here and post it.

You might be able to pass a subsystem as a parameter, but i don't know off the top off my head how that would play with the "requires" functionality and all that.

[Andrew Lobos]

You can make commands generic-ish by passing parameters to a parameterized constructor, then having that command pass that parameter to a parameterized method of your subsystem. I'll type up a quick c++ example in a bit here and post it.

This is what we did in Java last year. There where generic commands like "SetFlywheel" that took parameters from the constructor and used them in the calls to the subsystem. These generic commands were implemented in CommandGroups that did specific tasks, like FarShot3 for full court 3pt.

Code is here if you'd like to take a look: https://github.com/FRCTeam225/2013Offseason/tree/master/src/org/team225

[Tom Bottiglieri]

We (on 254) usually just end up writing a bunch of boiler plate code to do this, but I think #3 is a solid way of solving this. Personally I don't make an abstraction until I have at least 3 things that need it (you know the whole bit about premature optimization...). If I had to, I'd make Subsystems implement Interfaces like "Extendable" or "Controllable" then make commands that handle that.

125 did this last year a little differently by subclassing subsystem, then building Commands that could control those types of subsystems.
https://github.com/kreeve/Nutrons2013Legal/blob/master/src/edu/neu/nutrons/ultimateascent/subsystems/OnOffSubsystem.java

[Tom Bottiglieri]

3) make a solenoid interface for extend/retract, and have the subsystem implement the interface. Then you could pass the subsystem to a generic command. However, you would be limited to a single solenoid per subsystem.

Build an interface that requires extend() and retract() methods, have the subsystem use it, then do whatever you need in the implementation of those methods

[Alan Anderson]

If you're worried about writing commands to control individual cylinders, you're probably not at the appropriately high level of abstraction. I suggest that you should be controlling subsystems, not actuators. The subsystem implementation should take care of the details of motor and relay and solenoid control based on commands for speed and state.

Don't extend or retract a cylinder. Instead, engage or disengage a brake, or lock or unlock a latch, or open or close a gripper. It would be appropriate to extend or retract an arm, if that's the motion you've designed it to do.

[yash101]

I really think that it would be nice to make some sort of Library to set up everything for you. That way, could have code like:

int main() {
init victor("shooter");
setSpeed("shooter", 127);
return 0;
}

And yes, that was probably incorrect C syntax.

[Alan Anderson]

I really think that it would be nice to make some sort of Library to set up everything for you.

That is what the RobotBuilder tool is for.

[notmattlythgoe]

If you're worried about writing commands to control individual cylinders, you're probably not at the appropriately high level of abstraction. I suggest that you should be controlling subsystems, not actuators. The subsystem implementation should take care of the details of motor and relay and solenoid control based on commands for speed and state.

Don't extend or retract a cylinder. Instead, engage or disengage a brake, or lock or unlock a latch, or open or close a gripper. It would be appropriate to extend or retract an arm, if that's the motion you've designed it to do.

This is the way it should be done. The parts (solenoids, motor controllers, encoders, etc.) should be contained to each of the different subsystems and how you control those different parts should be strictly limited to the methods you make available to the commands. Commands should never have direct access to any of the devices set up in a subsystem.

[Kevin Sevcik]

This is the way it should be done. The parts (solenoids, motor controllers, encoders, etc.) should be contained to each of the different subsystems and how you control those different parts should be strictly limited to the methods you make available to the commands. Commands should never have direct access to any of the devices set up in a subsystem.Agreed, but there's still value in parameterized Commands for things like 4ndr3wl is talking about. We did the same last year so we could have a Shoot command that we passed an enum to pick which set of speeds+angle we were firing at. As opposed to having a ShootFar, ShootNear, etc. command. Which means if we needed a new set of speeds+angle, we could just add another element to the array and another constant to the enum. Instead of adding yet another command.

[notmattlythgoe]

Agreed, but there's still value in parameterized Commands for things like 4ndr3wl is talking about. We did the same last year so we could have a Shoot command that we passed an enum to pick which set of speeds+angle we were firing at. As opposed to having a ShootFar, ShootNear, etc. command. Which means if we needed a new set of speeds+angle, we could just add another element to the array and another constant to the enum. Instead of adding yet another command.

Completely agree, we use a drive straight command that takes a power and a distance so we can reuse it.

[Jefferson]

We were fully command-based last year except for solenoids. The vast majority of the time, we extend/retract solenoids based on a single button push making simple if/else statements in the TeleopPeriodic function the easiest to implement and modify.

[Fifthparallel]

You need to abstract business logic (interactions between subsystems, etc) away from functionality -- i.e. have business logic executed inside your commands and actually functionality (like extend/retract for a piston) be part of an API that gets thrown into that subsystem. Our 2013 C++ code isn't the best example, but it does have some examples -- https://github.com/FRC-Team-1410/UA2013-FRC1410-Robot

[notmattlythgoe]

We were fully command-based last year except for solenoids. The vast majority of the time, we extend/retract solenoids based on a single button push making simple if/else statements in the TeleopPeriodic function the easiest to implement and modify.

I guess my question is, why not design the systems the solenoids use as a subsystem? We had subsystems this year that were just a single solenoid (trigger, shooter elevation, hopper, and pickup arm). We like to separate the solenoids into subsystems because we feel that it makes the project easier to read and practices good OOD technique by locking those devices into a subsystem that only a command requiring that subsystem is allowed access to control.

[Joe Ross]

If you're worried about writing commands to control individual cylinders, you're probably not at the appropriately high level of abstraction. I suggest that you should be controlling subsystems, not actuators. The subsystem implementation should take care of the details of motor and relay and solenoid control based on commands for speed and state.

That is exactly what we did last year. However, after writing the same code in a subsystem for the 6th solenoid and the 3rd on/off motor, it gets kind of old. For easy things like a solenoid or on/off motor, I'm not against giving up a little bit of self documenting code, if it makes writing the code easier and less error prone.

You can make commands generic-ish by passing parameters to a parameterized constructor, then having that command pass that parameter to a parameterized method of your subsystem. I'll type up a quick c++ example in a bit here and post it.

I'm now thinking about a generic "super" command that you can pass a solenoid to. Then make a bunch of commands that extends the "super" command. This commands would be named appropriately per the function and would pass the appropriate solenoid or motor in the constructor and any other parameters, and not need any more code.

[Alan Anderson]

...after writing the same code in a subsystem for the 6th solenoid and the 3rd on/off motor,...

It sounds like you were right when you don't have the experience you need with Object Oriented programming. Instead of writing the same code in multiple places for multiple solenoids, you probably could have extended the Solenoid class with the methods you wanted. Extension though Inheritance is probably one of the best tools you can use for the kind of configurable control you're talking about.

[MrRoboSteve]

Here's a nice example of a command that takes a parameter.


#include "TurnSpecifiedDegreesCommand.h"
#include "math.h"

// If we're within this # of degrees to target, then we're good.

#define DEGREES_PRECISION 2


/*
* This command turns the number of degrees specified in the dashboard field.
*/
TurnSpecifiedDegreesCommand::TurnSpecifiedDegreesC ommand(float degreesToTurn)
: CommandBase("TurnSpecifiedDegreesCommand") {
Requires(chassis);
this->degreesToTurn = degreesToTurn;
}

// Called just before this Command runs the first time
void TurnSpecifiedDegreesCommand::Initialize() {
this->finished = false;
this->startingAngle = sensorSubsystem->GetHorizontalAngle();
this->goalAngle = startingAngle + this->degreesToTurn;
printf("TurnSpecifiedDegreesCommand %f\n", this->degreesToTurn);
}

// Called repeatedly when this Command is scheduled to run
void TurnSpecifiedDegreesCommand::Execute() {
float currentAngle = sensorSubsystem->GetHorizontalAngle();
float angleDifference = goalAngle - currentAngle;

SmartDashboard::PutNumber("Goal angle", goalAngle);
SmartDashboard::PutNumber("Angle difference", fabs(angleDifference));
printf("Angle difference %f goalAngle %f currentAngle %f\n", angleDifference, goalAngle, currentAngle);

float turnRate = 0;

if (fabs(angleDifference) < DEGREES_PRECISION) {
chassis->Stop();
finished = true;
} else {
// We slow our rate of turn as we get close to the angle we want.
// These values are guesses. A PID would be better here.
if (angleDifference > 10 || angleDifference < -10) {
turnRate = 0.7;
} else {
// Look at changing this at competition
turnRate = 0.6;
}

SmartDashboard::PutNumber("turn rate", turnRate);

if (angleDifference > 0) {
chassis->TurnLeft(turnRate);
} else {
chassis->TurnRight(turnRate);
}
}
}

// Make this return true when this Command no longer needs to run execute()
bool TurnSpecifiedDegreesCommand::IsFinished() {
return finished;
}

// Called once after isFinished returns true
void TurnSpecifiedDegreesCommand::End() {
printf("TurnSpecifiedDegreesCommand completed.\n");
}

// Called when another command which requires one or more of the same
// subsystems is scheduled to run
void TurnSpecifiedDegreesCommand::Interrupted() {
}

[Joe Ross]

125 did this last year a little differently by subclassing subsystem, then building Commands that could control those types of subsystems.
https://github.com/kreeve/Nutrons2013Legal/blob/master/src/edu/neu/nutrons/ultimateascent/subsystems/OnOffSubsystem.java

Thanks, that's very similar to what I was thinking about.

[Jefferson]

I guess my question is, why not design the systems the solenoids use as a subsystem? We had subsystems this year that were just a single solenoid (trigger, shooter elevation, hopper, and pickup arm). We like to separate the solenoids into subsystems because we feel that it makes the project easier to read and practices good OOD technique by locking those devices into a subsystem that only a command requiring that subsystem is allowed access to control.

By doing it the way you describe, 4 lines of code turn into a subsystem with two additional functions and two commands. Each of those objects add to compile time, which turned into well over a minute with all of our commands in 2013. We'll just have to disagree which option is easier to read. The risk of other code writing to the solenoids, while possible, is not worth the complexity created by using commands.

[Ziv]

Tom already mentioned my favorite approach to the problem :). It allows for both abstracting away implementation details (the outside code never sees a solenoid) and easy parametrization (just describe what it means for a subsystem to be on or off and you get a bunch of methods and commands for free). Many shooters in 2013 (including 125's and apparently 330's) were well-suited for this approach, because, unlike shooting baskets or balancing in 2012, shooting frisbees and pyramid pull-ups generally didn't need to be done in more than one or two ways.

It's not frequent that there's such a nice common abstraction for all of a robot's subsystems, but every robot deserves a bit of thought about what code can be shared before throwing the towel in and making it in RobotBuilder. (If you're bored by repetition, don't make the repetition easier without first trying to avoid the repetition altogether. RobotBuilder does the first but not the second.) When designing a set of abstractions, I find the most important thing is to be extremely picky, even if it feels petty. You should *like* the result because you think it's *pretty* (or at least useful, if you don't see how anyone could use that adjective about code :rolleyes:). The 30 minutes of code architecture debate might only save you 30 seconds in the future, but the 30 minutes can be before the robot is ready to test while the 30 seconds will probably be right before a match.