WPiLiib SimpleMotorFeedForward bug?

Technical Programming
1 
final double desiredAngularVelocityRadPerSec = 20;

SimpleMotorFeedforward feedforward1 = new SimpleMotorFeedforward(0, 0.1, 0);
final Voltage v1 = feedforward1.calculate(RadiansPerSecond.of(desiredAngularVelocityRadPerSec));
final Voltage v2 = feedforward1.calculate(RPM.of(Units.radiansPerSecondToRotationsPerMinute(desiredAngularVelocityRadPerSec)));
System.out.println("v1: " + v1.in(Volts));
System.out.println("v2: " + v2.in(Volts));

SimpleMotorFeedforward feedforward2 = new SimpleMotorFeedforward(0, 0.1, 0);
final Voltage v3 = feedforward1.calculate(RPM.of(Units.radiansPerSecondToRotationsPerMinute(desiredAngularVelocityRadPerSec)));
final Voltage v4 = feedforward1.calculate(RadiansPerSecond.of(desiredAngularVelocityRadPerSec));
System.out.println("v3: " + v3.in(Volts));
System.out.println("v4: " + v4.in(Volts));

outputs:

v1: 19.098593171027442
v2: 19.098593171027442
v3: 2.0
v4: 2.0
2

i dont think it is a bug rather a binary mistake in java.

3

v1 and v4 seem like the exact same line, and so do v2 and v3, how come there’s a change between them?
Are you sure the output is 19.09 and not 1.909?
Also, feedforward2 doesn’t seem to be in use, or have any difference from feedforward1.

4

If you add a print statement in between the two lines, you’ll get the correct result for each one, instead of the same result for both. The reason for this is that SimpleMotorFeedForward keeps an internal mutable voltage variable and updates/returns it, rather than returning a new object each call, to reduce memory allocations. This means that in your example, v1 and v2 are the same object (and v3 and v4).

This is the major footgun / confusing behavior with mutable units, and why I think we won’t roll out units more broadly in WPILib until 2027, when we should be able to switch to immutable only units given more memory and a better GC.

7 Likes
5

Thanks for the explanation; it was very clear and helpful!

However, I’m still confused about the new feedforward API.

The calculate(double) method is deprecated, and users are now expected to use calculate(Measure) instead. From my understanding, Measure can be created with any unit, which is why it’s used. However, I noticed that the SimpleMotorFeedforward class ignores the unit and only cares about the numerical magnitude during computation.

Here’s an example:

// kV is 1.0
SimpleMotorFeedforward feedforward = new SimpleMotorFeedforward(0, 1.0, 0);

// angularVelocity1 is "1 rad/sec"
final AngularVelocity angularVelocity1 = RadiansPerSecond.of(1);
System.out.println(feedforward.calculate(angularVelocity1).in(Volts));

// angularVelocity2 is "1 RPM"
final AngularVelocity angularVelocity2 = RPM.of(1);
System.out.println(feedforward.calculate(angularVelocity2).in(Volts));

Output:

1.0
1.0

It seems that 1 rad/sec is treated the same as 1 RPM, but these are not equivalent. The SimpleMotorFeedforward class is saying, that the voltage required to spin the motor at 1 rad/sec is the same as at 1 RPM. This is very, very confusing:

  1. Why doesn’t the feedforward API allow us to configure kV with specific units like Per<VoltageUnit, AngularVelocityUnit>?

  2. If we have to specify the setpoint as a Measure object, why can we only create this object from specific units? For example, if kV is configured in volts/(rad/s), we can only create a Measure using RadiansPerSecond.of(). That is to say, using RPM.of() would give the wrong result.

This raises a bigger question: What’s the point of using Measure if it doesn’t help distinguish between different units? Doesn’t this add to the confusion?

1 Like
6

There is little wrong with having non-normal functions that have side-effects and the failing test case is an unusually contrived edge case. But the unexpected behavior is impossibly subtle and dangerous that I hope it will be documented very well in code and the FRC Documentation. I think it goes something like this:

Do not assign the calculate(units) value of a SimpleMotorFeedforward object to more than one variable.

Is this problem anywhere else in WPILib that we should know about?

7

It was used in very few places. The allocation benefits (just a single allocation per call) don’t seem worth it, so we’ve decided to change everywhere that returned a mutable unit to no longer do so: [wpilib,wpimath] Don't use mutable units for return values by PeterJohnson · Pull Request #7369 · wpilibsuite/allwpilib · GitHub

2 Likes
8

You could allow for a variant that took a mutable object as a parameter to be modified. It give me a bit of an ick but is explicit in what happens and allows for the unit safety.

9

We could. Doesn’t seem worth it to me to add that complexity for a single allocation; plus this whole issue goes away in 2027 with more memory and better GC’s, so adding complexity to the API that we will remove in two years doesn’t seem worth it either.

1 Like