RobotDrive class in WPILibJ

[mikets]

Our team decided to experiment with different drive wheels, so we wrote code to call the RobotDrive class's different drive methods such as tankDrive and mecanumDrive. First we had mecanum wheels and it drove fine. Then we switched to regular wheels and change the code to call tankDrive instead. To our surprise, the right wheels were driving in opposite direction of the left wheels meaning when both left and right joysticks are pushed forward, the left wheels go forward but the right wheels go backward. Now, we do understand the left and the right wheel motors are mounted mirror imaged so we have set the right side of the wheels "inverted" with rightFrontMotor.setInverted(true) and rightRearMotor.setInverted(true). We did that and have code to spin each wheel independently to prove that all are driving "forward" when doing a xxxxMotor.set(0.5). As a result, mecanum drive is fine (actually more about that later). But when calling tankDrive, the robot just turns because the right wheels are spinning backwards. I finally downloaded the source code of WPILibJ and found the following code snippet.

  public void setLeftRightMotorOutputs(double leftOutput, double rightOutput) {
    if (m_rearLeftMotor == null || m_rearRightMotor == null) {
      throw new NullPointerException("Null motor provided");
    }

    if (m_frontLeftMotor != null) {
      m_frontLeftMotor.set(limit(leftOutput) * m_maxOutput, m_syncGroup);
    }
    m_rearLeftMotor.set(limit(leftOutput) * m_maxOutput, m_syncGroup);

    if (m_frontRightMotor != null) {
      m_frontRightMotor.set(-limit(rightOutput) * m_maxOutput, m_syncGroup); // <<<<<<
    }
    m_rearRightMotor.set(-limit(rightOutput) * m_maxOutput, m_syncGroup); // <<<<<<

    if (this.m_syncGroup != 0) {
      CANJaguar.updateSyncGroup(m_syncGroup);
    }

    if (m_safetyHelper != null)
      m_safetyHelper.feed();
  }

Notice the two right motors are negated (lines marked with <<<<<<).
I don't understand why the code is doing that. Since the motor direction is already corrected by previous calls to setInverted (or you may also call the setInvertedMotor method, they are doing the same thing), the code should not do any more negation anywhere. In fact, upon examining all the methods in this class, I have concluded that it has a lot of unnecessary negations scattered around. For example, mecanumDrive has "negate Y for joystick" in the code but not for tankDrive and arcadeDrive. For arcadeDrive and tankDrive, the caller has to do the Y negation. I actually think it should not negate Y for joystick because this decision belongs to the caller and depending on what kind of joystick you use. Joysticks, which are designed for airplane, so pushing forward means the plane is going down and therefore gives you a negative value versus xbox gamepad, for example, will give you positive number when pushing forward. So RobotDrive should not be in the business of negating for me. Also, since pushing the joystick to the right will give you a positive number on the X-axis, you would expect a positive "rotateValue" on arcadeDrive would make the robot to turn right but NO, wpilib makes it to turn left. I suspect nobody complains about it because people would just stick with one type of drive (either arcade, tank or mecanum but not switching between them) and they would just call setMotorInverted appropriately or negate the x/y axes of the joystick to compensate for it. But in my opinion, WPILib is plainly wrong. And of course, we did compensate for it by wrapping the RobotDrive class with our own class and correcting all these inconsistencies. But I would really like somebody to review the WPI RobotDrive code and tell me if I read this correctly. Maybe I am missing something.
BTW, since FTC switches to java this year, we have ported the equivalent of RobotDrive code to FTC so our library is actually shared between FTC and FRC. Our port removed all the inconsistencies in RobotDrive.
https://github.com/trc492/Ftc2016Fir...obotDrive.java

[Arhowk]

The only unnecessary negation is your unnecessary negation. The setInverted calls are designed for debugging purposes only (e.g. if a motor is accidentally wired backward) and are not intended to be included in a final build of the robot code (in the case of the RobotDrive class. That method is intended to be used over flipping motor leads because it is more verbose and easier to replicate. For drive train, there should only ever be one drive configuration). I suspect that you are incorrectly calling the mecanum drive class in such a way that it would undo the inversion that you applied unto it, leaving only the negation found inside of the RobotDrive class.

So RobotDrive should not be in the business of negating for me. Also, since pushing the joystick to the right will give you a positive number on the X-axis, you would expect a positive "rotateValue" on arcadeDrive would make the robot to turn right but NO, wpilib makes it to turn left.

These two statements are entirely contradictory. Firstly, you state that you should be in control of the specifics of the drive train (which... you are)... Secondly, you state that the rotate value of the motor should be inverted because that is more logical. Actually, on my controller, going right is actually a negative value which causes the robot to drive right. If this were switched to work with your controller, mine would stop working. Last year, we used a SpaceMouse that had Y inverted and X euclidean. The motors

suspect nobody complains about it because people would just stick with one type of drive (either arcade, tank or mecanum but not switching between them) and they would just call setMotorInverted appropriately or negate the x/y axes of the joystick to compensate for it.

No, because we just do this

 if(shouldBeArcadeDrive){
    drive.arcadeDrive(joy.getRawAxis(KXbox.Move), -joy.getRawAxis(KXbox.Rotate))
}else{
    drive.mecanumDrive(joy.getRawAxis(KXbox.Move), joy.getRawAxis(KXbox.Rotate))
}

Took me two seconds to write in this text box. Much easier than overthrowing the entire system and rewriting wpilib from scratch.

[mikets]

That's news to me that setInverted (or setMotorInverted) is for debugging purpose and user is not supposed to call them. How would WPILib knows how we mount our drive motors? What if our drive train has the left and right motors mounted in a way that it doesn't need the negation?

[yabberyabber]

@Mikets, This seems like a problem in wpilib. TankDrive expects you to have set the setInverted flags on the motors and MecanumDrive expects you to have not set those flags. It's inconsistent and unintuitive.

@arhowk What makes you say `setInverted` is for debugging purposes only? If you're going to set motor power in multiple places, it's much cleaner just to call setInverted once when you create the motor instead of putting a negative sign in every place you set the motor power.

if(shouldBeArcadeDrive){
    drive.arcadeDrive(joy.getRawAxis(KXbox.Move), -joy.getRawAxis(KXbox.Rotate))
}else{
    drive.mecanumDrive(joy.getRawAxis(KXbox.Move), joy.getRawAxis(KXbox.Rotate))
}

I think this code is a great example of why there is a problem in WPILib. Sure this example took two seconds to write, but they get more complicated than this. When libraries like WPILib are inconsistent it makes the code that uses them less readable and more prone to error.

I've noticed a lot of inconsistencies in WPILib. Team 973 also wraps a bunch of WPILib functionality to protect ourselves from the madness. Thanks, Mike, for pointing out the problem and sharing a clean solution.

[mikets]

Thanks Andrew. In the past, I have seen a lot of students wrote code that did trial and error. When the motors did not go the way they wished, they negate the power. For example, I've seen students doing tankDrive(0.5, -0.5) or tankDrive(-0.5, -0.5) just to go forward. They said as long as it works, it shouldn't matter. But eventually, the code became very confusing. So when I taught my students, I always tell them to test each individual motor by setting positive power and make sure they go forward by doing setInverted on motors that spin in the wrong direction. And in theory when all motors are going forward with positive power, there should never be negative value in any kind of drive: tankDrive, arcadeDrive and mecanumDrive, when going forward. If you have to negate the power, somebody screwed up. That's why I think RobotDrive class is wrong.