Code Review

Technical Programming
1

Hello CD, Over the past 9 years that I have been a part of First I have gone from having a programming mentor to becoming the programming mentor for our team. I’m not the best at programming, I know just enough to get in trouble. Could you guys go over and give me some pointers on what we can do better this year based on our code from 2023? We had many problems last year ranging from our robot arm trying to break itself randomly when we would power up to some simple bugs that were eventually worked out.

Our swerve drive code base was from dirtbikerxz’s BaseFalconSwerve repo on GitHub then the rest was us.

Any feedback will be very much appreciated by myself and our team.

2

Your commands are too big. Write smaller commands and compose them into larger groups, rather than writing huge monoliths.

Command factories are generally easier to do this with than command classes.

4 Likes
3

Whenever you have to control subsystem state (ie. switching intake between states: intaking, outaking, off) it is good practice to have an enum with your setpoints inside, rather than a bunch of if statements and comments.

The largest befits are:

  • ease of changing setpoints
  • ease of adding new states
  • readability & brevity

For the below function you are controlling your intake.

You instead could make an enum like below (ignoring “sensor” bc idrk whats going on w/ that)

public static enum IntakeState {

  INTAKE(1f, -1f, false), OUTAKE(-1f, 1f, true), OFF(0f, 0f, false);
  
  public final double vspeed, hspeed;
  public final boolean led;

  private IntakeState(double vspeed, double hspeed, boolean led) {
    this.vspeed = vspeed;
    this.hspeed = hspeed;
    this.led = led;
  }

and have a function like

public void intake(IntakeState state) {
  setVIntakeSpeed(state.vspeed);
  setHIntakeSpeed(state.hspeed);
  setLed(state.led);
}

And then doing logic on ur state becomes super concise. Especially compared to 10 nested conditionals.

I hope this is something that helps.

7 Likes
4

This is very helpful! we will give this a try. Last year we had 2 switches on our co-pilot station that would run the intake. one with a sensor to hold the game piece in our intake so we could score on the floor and the other would let it pull the game piece fully into our robot to allow our arm to pick it up.

5

Did you determine what was the problem?

It rarely hurts to put in safety measures:

  • limit switches
  • slew rate limiters
  • motor current limits
  • if statements to check for and prevent dumb actions
  • run at sloooow speeds to startup a new system
  • PID limiters to knock down big step changes
  • PID constants that are conservative initially and sneak up on good operating values to avoid breakage during tuning
1 Like
6

We never found what the problem was. It would happen very randomly. The robot would be fine for all of our qual matches on Friday then on Saturday morning we would fire up the robot to do a systems check. When we enabled the arm would just start to attempt to go to the moon and slam into our bowl part of the intake its setpoint was 0 the entire time. We took the chains off to prevent the arm from moving and the motors would just spin at full speed even if the encoder met our highest software limit. then after rebooting the robot 2 or 3 times, it would just work. it was very odd.

7

So many possibilities. A common pitfall of students is looking for exact when these analog machines are far from it. For example, if(arm.position() == 0) arm.stop() should check for <=.

The arm may have been at 0 when stowed but soon thereafter it could be -0.0001.

I don’t see any faults like these, though.

Looking at your code shows how a few well placed comments can help reviewers a lot.

For example, my team uses PID controllers and sticks with the WPILib example of calculate(currentValue, setpointValue)

Why you code calculate(currentValue - setpointValue) is not obvious because we never use that construct (although I participate in CD to learn about new ways of doing things). Maybe I’d figure it out given a lot of time that I and many reviewers do not have.

8

This topic was automatically closed 365 days after the last reply. New replies are no longer allowed.