Make SimpleMotorFeedforward only support discrete feedforward

[narmstro2020]

This PR sets period as a constructor argument along with adding another constructor overload to accomodate.
The constructor now creates the feedforward, plant, r and nextR variables used by the calculate(current, next, dtSeconds) overload.

The calculate(current, next, dtSeconds) overload is now calculateDiscrete(current, next).
The calculate(velocity, acceleration) overload is not calculateContinuous(velocity, acceleration).
The calculate(velocity) overload is unchanged. I'm not married to these name changes.

I will eventually move on to Elevator and Arm and C++ for all 3 based on the feedback I receive.

[narmstro2020]

@calcmogul I just caught myself leaving in the period as a final public field right before sending this. So no problem there in removing it.

So is there any value in leaving the continuous overload in the class at all?

EDIT: I can put the equation in as a comment if it isn't there already just for reference.

[narmstro2020]

Something more like this?

[narmstro2020]

Just wanted to check up on this PR. Haven't received any other feedback in awhile. Should I close it or make modifications?

[narmstro2020]

/format

[narmstro2020]

So I believe I have a first draft of the C++ SimpleMotorFeedforward. I've yet to change the tests and other dependent classes.
When feeding in r and nextR into the LinearPlantInversion object I went without the private field and directly fed the values into the object. Is this correct thinking or should I do this in a similar way to the Java class?

I also removed the field for the Java class for plant as it wasn't necessary to keep.

Will work on tomorrow and add any feedback from here as well.

[calcmogul]

I fixed the build errors not related to deprecated usage in examples. The diff drive voltage constraint test is failing for some reason. That class has always been buggy... it doesn't actually stay within the voltage limits sometimes.

[narmstro2020]

Thank you. Any problems in the .h and .cpp files for SimpleMotorFeedforward

[calcmogul]

They seem fine now.

[narmstro2020]

Thank you for the help on the C++. Just curious why did you decide not to use the plant inversion class like in the Java version and compute it directly.

[narmstro2020]

I'm getting some weird units not found error messages in several of the examples when modifying the appropriate Calculate methods.

An example would be Evenloop's Robot.cpp.

EDIT: Nevermind. I figured it out.

[narmstro2020]

I am getting a weird issue that seems non connected with FlywheelBangBangController's example.

[narmstro2020]

FlywheelBangBangController\cpp\Robot.cpp(98): error C2039: 'LinearSystemId': is not a member of 'frc' FlywheelBangBangController\cpp\Robot.cpp(98): error C3083: 'LinearSystemId': the symbol to the left of a '::' must be a type FlywheelBangBangController\cpp\Robot.cpp(98): error C2039: 'FlywheelSystem': is not a member of 'frc' FlywheelBangBangController\cpp\Robot.cpp(98): error C3861: 'FlywheelSystem': identifier not found

EDIT: Nevermind. I found the issue here as well.

[calcmogul]

why did you decide not to use the plant inversion class like in the Java version and compute it directly

SimpleMotorFeedforward is a mostly constexpr class. LinearPlantInversionFeedforward isn't constexpr because Eigen isn't in general. SimpleMotorFeedforward uses a 1x1 system, so we can easily implement the feedforward by hand in a constexpr context.

It's also more efficient that way in Java because it avoids several object allocations.

[narmstro2020]

I think this is good except for some failing tests, that I'll need some help rectifying.

I think they involve what you mentioned earlier.

[narmstro2020]

@calcmogul How’s this looking?

[calcmogul]

I still don't know what to do about the failing trajectory constraint test. I tried reimplementing the constraint with a different algorithm, and it got the same wrong answer.

[narmstro2020]

Would the test have been originally designed with continuous in mind or does it not really matter?

[calcmogul]

It's possible, but even that shouldn't cause voltages around 22 V for a 10 V limit.

[narmstro2020]

@calcmogul I found a typo in DifferentialDriveVoltageTest and fixed it, but I'm not sure if this will fix the issue or not.

EDIT: That didn't directly fix it. I went back to the original test and read the comment at now line 51.
Based on the original construction of the test I made similar modifications to the assertAlls.

Testing it in Java first. Wanted your thoughts before I proceed to C++ Went ahead with C++ fix. I think this might be good to go now depending on your thoughts on the Diff Drive Voltage test fix

[narmstro2020]

/format

[narmstro2020]

/format

[narmstro2020]

/format

[narmstro2020]

@calcmogul I'm going to go ahead and make these changes, but would it be better to have a single velocity overload for calculate that takes the one setpoint and uses the overload with two setpoint for us

Other than that I've fixed all of the examples and tests that you've indicated and I double checked the list myself.

[narmstro2020]

/format

[narmstro2020]

@calcmogul BTW. I think I've had in my head a disparity between encoder value and current setpoint for feedforward for a long time and this just helped me separate that disparity.

[calcmogul]

would it be better to have a single velocity overload for calculate that takes the one setpoint and uses the overload with two setpoint for us

That seems reasonable. The API doc for that overload can mention that it's for when the reference doesn't change continuously.

[narmstro2020]

@calcmogul How do I get around the ambiguous call to overloaded function in C++ Since the old version is still there and deprecated I believe it's causing this conflict

[calcmogul]

You could remove the default argument and provide two separate overloads: a non-deprecated one-arg (velocity) version and a deprecated two-arg (velocity, acceleration) version.

[narmstro2020]

@calcmogul I think we're good now.