Reducing code complexity

[Sinani201]

One of the problem with our teams code this year was that teleop had a huge amount of if statements for each element of the control system (i.e. if this button is pressed, perform this function). This seems like bad practice and the the code isn't as maintainable as it could be.

Link to the code is here. Have any other teams struggled with this? How have you gotten around it?

Thanks,
Daniel, Team 1836

[SoftwareBug2.0]

If you just want to remove the if statements, here's a few things you can do:

if (xbox.isReleased(JStick.XBOX_A)) {
	usingCheesy = !usingCheesy;
}

can become:

usingCheesy=usingCheesy^xbox.isReleased(JStick.XBOX_A);

if(slowMode){
	driveGear.set(false);
}else{
	driveGear.set(normalGear);
}

can become:

driveGear.set(slowMode?false:normalGear);

Also, this:

if (shooterMode == SHOOTER_MODE_VOLTAGE) {
	lcd.println(DriverStationLCD.Line.kUser1,1,"Shooter mode:voltage ");
} else if (shooterMode == SHOOTER_BANG_BANG) {
	lcd.println(DriverStationLCD.Line.kUser1,1,"Shooter mode:bangbang");
} else if (shooterMode == SHOOTER_COMBINED) {
	lcd.println(DriverStationLCD.Line.kUser1,1,"Shooter mode:combined");
} else {
	lcd.println(DriverStationLCD.Line.kUser1,1,"Shooter mode:????????");
}

can become:

{
	const char *out;
	switch(shooterMode){
		case SHOOTER_MODE_VOLTAGE: out="Shooter mode:voltage "; break;
		case SHOOTER_BANG_BANG: out="Shooter mode:bangbang"; break;
		case SHOOTER_COMBINED: out="Shooter mode:combined"; break;
		default: out="Shooter mode:????????";
	}
	lcd.println(DriverStationLCD.Line.kUser1,1,out);
}

[SoftwareBug2.0]

I just noticed that I turned one of the snippets from Java into C++. Oh well.

[Ziv]

SoftwareBug2.0 makes some good suggestions, but I think they treat the symptoms rather than the root cause.

In many FRC games, the primary challenge when programming the robot is code organization. (Occasionally an especially challenging problem like vision tracking pops up, but even then there are enough tutorials around that the algorithm itself is not the obstacle.) I suggest using the command-subsystem style from the beginning of a project. It seems like overkill at the beginning when code is simple. However, the separation between the robot subsystems, higher-level commands, and operator interface it provides makes situations like those you complain about occur less frequently, if at all. It also makes the program far easier to change in the time pressure of competition.

[Hjelstrom]

I really don't recommend using the ? or ^ operators just to make your code smaller. It will just make the code harder to read and understand which is far more important than how big it is. The switch statement is a good idea though.

Instead, how about moving all of the code related to the shooter into a function and all of the code related to the drivetrain into another one, etc.

What we do is take the idea a little farther and use a separate class for each subsystem and that class even contains the speed controller objects, the sensors, etc. Within our classes though, there are plain old 'if' statements.

[RyanCahoon]

Originally Posted by Ziv

I suggest using the command-subsystem style

Implementing the command-subsystem paradigm using the base classes included in WPIlibJ also allows you to use event-driven programming using the Button classes, which can replace most - if not all - of your if statements with a couple lines at program initialization in the form:

Originally Posted by http://www.wbrobotics.com/attachments/article/11/WPILibCookbook.pdf

JoystickButton trigger = new JoystickButton(stick, Joystick.ButtonType.kTrigger.value);
trigger.whenPressed(new DoCoolStuffCommand());

[MikeE]

Originally Posted by Ziv

SoftwareBug2.0 makes some good suggestions, but I think they treat the symptoms rather than the root cause.

In many FRC games, the primary challenge when programming the robot is code organization. (Occasionally an especially challenging problem like vision tracking pops up, but even then there are enough tutorials around that the algorithm itself is not the obstacle.) I suggest using the command-subsystem style from the beginning of a project. It seems like overkill at the beginning when code is simple. However, the separation between the robot subsystems, higher-level commands, and operator interface it provides makes situations like those you complain about occur less frequently, if at all. It also makes the program far easier to change in the time pressure of competition.

I agree with Ziv - move to a cleaner extensible framework such as the command-based framework. It's not perfect and there is a bit of a learning curve but it does help tremendously with code modularity.
The biggest problem with the simple-loop organization is that there is no isolation between different robot functions - a small change in one place is fairly likely to affect other operations.
Moving to a modular approach means code chunks are more isolated and that is a lifesaver when you are fixing code 3 minutes before you match starts with no time to test!

[apalrd]

On the LV side of things, we use a few tricks to keep code organized which should port fairly well to any language:

-We STRICTLY separate the operating code from the HMI/Auton code. This isn't quite like the command/action garbage of the command framework, we JUST abstract the actual button inputs, debouncing/rising-edge triggering, and scaling before passing the data into the code. For example, our drivetrain code has inputs of desired shift state (enumerated), and doubles for left/right drivetrain powers. The slab subsystem has a desired state (enumerated), which it controls to.

-HMI code just takes buttons from the button data and switches or scales it to the units used in the primary code, and sends it to the primary code. Auton operates slightly differently, since Auton is procedural by the nature of Beescript, but the end result is data passed to the primary code blocks in the same task.

-We organized all of the code into a single RT thread. While this initially seems like a bad idea, we could organize data transfer far more efficiently and deterministically by passing data through wires and data structures, and guaranteeing order of operations through the code. We read all of the data from the FPGA, scale it into bus_inputs, and make that bus available to all subsystems and HMI/Auton. Likewise, the subsystems all have access to bus_outputs which they write the outputs they are responsible for, and at the end of the iteration the data is all scaled to raw units and written to the FPGA. bus_state is used to store the publicly-accessible state information for each system, and is the primary means of data-passing between subsystems. Since we are single-threaded, we don't have to worry about missing events from other threads due to timing jitter, temptations to write bad code using blocking calls, or the order of operations in actions between multiple threads. We also don't have to deal with data access and data issues in read-modify-write operations because the code is single-threaded.

-All three busses are stored in shift registers and data in them is carried over between iterations. This allows a VI which executes last (e.g. the roller system) to send data to a system which executes before it, but 10ms later. This is commonly used to reset the input diagnostics which are checked first, and also allows items to be calculated at a lower frequency than the main loop frequency (e.g. new DS data is received at a lower frequency than the control loop, but it uses latest data every loop and iterates the loop anyway).

-All systems operate using either math algorithms or state-machines. Several use both. State-machines are a highly recommended way to reduce clutter by grouping desires to change state and actions to perform when changing states into a single area of the code, and resulting in a definitive state for this iteration. If the state is enumerated, you can use it to do table lookups for positions, motor command values, etc. and keep calibrations in simple data tables, which is more organized then writing them directly in code.

[Iaquinto.Joe]

Originally Posted by apalrd

-SNIP-
-All systems operate using either math algorithms or state-machines. Several use both. State-machines are a highly recommended way to reduce clutter by grouping desires to change state and actions to perform when changing states into a single area of the code, and resulting in a definitive state for this iteration. If the state is enumerated, you can use it to do table lookups for positions, motor command values, etc. and keep calibrations in simple data tables, which is more organized then writing them directly in code.

I agree with the bee. I recommend reprogramming your bot, OP, using Finite State Machines during the offseason. The process very much reduces complexity, and leaves no memory leaks. Diagramming each subsystem of your bot will give you exactly what needs to go into the code.

[Ginto8]

Originally Posted by Sinani201

One of the problem with our teams code this year was that teleop had a huge amount of if statements for each element of the control system (i.e. if this button is pressed, perform this function). This seems like bad practice and the the code isn't as maintainable as it could be.

The code you provided actually seems pretty clean to me; the logic is clear, and I can't think of any direct improvements that can be made. I will however echo others' suggestion to use the Command-based setup, as it makes button-mapping code much easier to maintain. Internally, the logic is the same, but it's encapsulated in nice things like button.whenPressed(). As an example, here's our OI class, which handles input from the drivers. A snippet from it shows that it can simplify things greatly:

        shootButton        .whenPressed(new Shoot());
        shootFullButton    .whenPressed(new Shoot(SpinUp.SPEED_FULLCOURT));
        spinDownButton     .whenPressed(new SpinDown());
        spinDown2Button    .whenPressed(new SpinDown());
        recordEncoderButton.whenPressed(new PrintAutonomousMove(0.6, 0.5));
        tomahawkButton     .whenPressed(new SpinTomahawk(true));
        tomahawkBackButton .whenPressed(new SpinTomahawk(false));
        feederAwayButton   .whenPressed(new FeederTurn(false));
        feederTowardButton .whenPressed(new FeederTurn(true));

[SoftwareBug2.0]

Originally Posted by Ziv

SoftwareBug2.0 makes some good suggestions, but I think they treat the symptoms rather than the root cause.

In many FRC games, the primary challenge when programming the robot is code organization. (Occasionally an especially challenging problem like vision tracking pops up, but even then there are enough tutorials around that the algorithm itself is not the obstacle.) I suggest using the command-subsystem style from the beginning of a project. It seems like overkill at the beginning when code is simple. However, the separation between the robot subsystems, higher-level commands, and operator interface it provides makes situations like those you complain about occur less frequently, if at all. It also makes the program far easier to change in the time pressure of competition.

My earlier suggestions were indeed fixing only superficial problems, and you probably want to do more than just what I pointed out. I'll get to some suggestions about that in a second.

But first let me say that I disagree about what the primary problem is in organizing robot code. It is understanding the underlying system and its limitations. You have to discover the flaws in the underlying system, and know how to work around them. For example, there was a time years ago, when you couldn't call atan2() because it would take so long that you'd cause the watchdog timer to go off. So then we had to implement lookup tables. Similarly, my team has hit several WPIlib bugs this year. And we discovered that the amount of bandwidth that was supposed to be available to each robot really wasn't.

Now on to some higher-level code suggestions: apalrd is on the right track. You don't need to use seperate classes and functions for everything, but you there are several different layers that make sense to use.

Here are three possible layers:
1) Reading buttons
2) Generating goals for each of the subsystems
3) Running each of the subsystems

(In autonomous mode, layers 1-2 would be replaced but 3 would be unchanged)

This year, my team actually used the command based stuff, and I can't recommend it with a straight face. You can kind of get towards a problem some call "callback hell".

I understand there are some differences in opinion once you get toward style stuff, but here's an example of how I like to see code. This controls a 30-pt climber, which sadly had some mechanical issues and didn't make it onto our robot.

enum Climber_state{...};
Climber_state climber(Climber_state,bool limit_l_bot,bool limit_l_top,bool limit_r_bot,bool limit_r_top,bool climb_button){
	... //stuff with no side-effects
}
struct Climber_output{
	int left,right;
};
Climber_output get_outputs(Climber_state){
	... //pure function, no side effects
}
void write_climber_motors(Climber_output){
	... //actually poke the motors
}

A little bit about the mechanism: There are two sets of slides, each of which is controlled by a motor, and each of which has a limit switch at the top and bottom.

Going back to concrete, lower level suggestions, about the quickest change to improve the code would be to just start your function with a bunch of lines of the form:

bool cheesy_mode_button=xbox.isReleased(JStick.XBOX_A);

There are a few places where the same code reading the inputs is repeated. Whenever that happens, alarm bells should be going off in your head.

[Sinani201]

Originally Posted by Iaquinto.Joe

I agree with the bee. I recommend reprogramming your bot, OP, using Finite State Machines during the offseason. The process very much reduces complexity, and leaves no memory leaks. Diagramming each subsystem of your bot will give you exactly what needs to go into the code.

I know a bit about finite state machines because I've been learning Erlang, but how would you recommend implementing one in Java?

[MrRoboSteve]

Consider an arm that's going to throw something. When it first starts up, it needs to initialize, and then it waits. When it throws, it needs to launch a process, and then waits for a signal that throwing is complete, and then retracts. It then waits for the next throwing command.

This is typed in code that's mostly Java, but not compiled or tested.

An enum that represents your states.

public enum State {Initializing, Waiting, StartThrowing, Throwing, StartRetracting, Retracting}

then in your logic

public State state;

...

// This switch is run repeatedly, either in a while loop if you're in an iterative robot or as part of a command in the command based robot.

switch (state)
{
  case Initializing:
    // Do whatever one time initialization logic here
    state = Waiting;
    break;
  case Waiting:
    if (arm_is_triggered) { // however you're triggering the arm
      state = StartThrowing;
    }
    break;
  case StartThrowing:
    // Do whatever one-time logic needed to start throwing -- e.g., open solenoid
    state = Throwing;
    break;
  case Throwing:
    if (done_throwing) { // however you're detecting throwing is done -- e.g., limit switch, timer
      // do whatever's needed to stop throwing
     state = StartRetracting;
    }
    break;
  case StartRetracting:
    // Do whatever is needed to start retracting
    state = Retracting;
    break;
  case Retracting:
    if (done_retracting) {
      // do whatever's needed to stop retracting
      state = Waiting;
    }
    break;
  default:
    printf("Unknown state!\n");
    break;
}

[apalrd]

In LV and C I usually use a typedef enum to store the state, and a switch for the states. LV it cleaner in this with a case structure.

The state in C is usually stored as a global variable, with an accessor function to adhere to double-buffering (storing the previous state when the set state is different from the current state).

In LV we store the state in bus_state which is a data structure which is essentially global to us. Alternatively you could store it in a shift register locally or as a global variable.

Since a typedef enum can be used as a uint, you can use it to index an array. We use this frequently to determine the state of outputs or setpoints based on state without additional code to explicitly set the output.

[cgmv123]

Create separate classes for all your systems. Even if you're only going to be creating one object, it's still worth it.

Since we end up using Tank Drive every year, I made a TankDrive class last year. We just create an object with the appropriate parameters, call drive.drive(); in our main teleop loop, and forget about it. It's that simple.