Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make SimpleMotorFeedforward only support discrete feedforward #6647

Merged
merged 41 commits into from
Jul 17, 2024
Merged

Make SimpleMotorFeedforward only support discrete feedforward #6647

merged 41 commits into from
Jul 17, 2024

Conversation

narmstro2020
Copy link
Contributor

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.

calcmogul
calcmogul requested changes May 22, 2024
View reviewed changes
Copy link
Member

@calcmogul calcmogul left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requiring fields in the constructor that some use cases may never use is a code smell. RobotDrive used to be like that; it didn't know what kind of drivetrain it wanted to be, so it took all motors and figured out validation at runtime.

In this case, the class doesn't know whether it wants to be continuous or discrete. At least the API on main doesn't ask for stuff it doesn't use. This PR's refactor would really make more sense as two separate classes, tho I don't like that either; everything teams do is discrete, not continuous.

@narmstro2020
Copy link
Contributor Author

narmstro2020 commented May 22, 2024 •
edited
Loading

@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
Copy link
Contributor Author

Something more like this?

@narmstro2020
Copy link
Contributor Author

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

@narmstro2020
Copy link
Contributor Author

/format

@narmstro2020 narmstro2020 marked this pull request as ready for review June 12, 2024 19:44
@narmstro2020 narmstro2020 requested review from a team as code owners June 12, 2024 19:44
calcmogul
calcmogul requested changes Jun 12, 2024
View reviewed changes
wpimath/src/main/java/edu/wpi/first/math/controller/SimpleMotorFeedforward.java Outdated Show resolved Hide resolved
wpimath/src/test/java/edu/wpi/first/math/trajectory/ExponentialProfileTest.java Outdated
import java.util.List;
import org.junit.jupiter.api.Test;

class ExponentialProfileTest {
private static final double kDt = 0.01;
private static final SimpleMotorFeedforward feedforward =
new SimpleMotorFeedforward(0, 2.5629, 0.43277);
new SimpleMotorFeedforward(0, 2.5629, 0.43277, 0.01);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the kDt constant instead of hardcoding it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oof

@calcmogul calcmogul changed the title Set period of Feedforwards in constructor. Make SimpleMotorFeedforward only support discrete feedforward Jun 12, 2024
@narmstro2020
Copy link
Contributor Author

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.

@narmstro2020
Copy link
Contributor Author

/format

@narmstro2020
Copy link
Contributor Author

/format

calcmogul
calcmogul requested changes Jun 24, 2024
View reviewed changes
Copy link
Member

@calcmogul calcmogul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a bunch of places where the "current velocity" of the feedforward is set to a measurement instead of the current or previous setpoint. For cases where the setpoint is constant, you can just use the same value for both the "current velocity" and "next velocity".

wpilibcExamples/src/main/cpp/examples/EventLoop/cpp/Robot.cpp Outdated Show resolved Hide resolved
wpilibcExamples/src/main/cpp/examples/Frisbeebot/cpp/subsystems/ShooterSubsystem.cpp Outdated Show resolved Hide resolved
@narmstro2020
Copy link
Contributor Author

/format

calcmogul
calcmogul requested changes Jun 24, 2024
View reviewed changes
Copy link
Member

@calcmogul calcmogul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't all of them, but you get the idea. Look for instances of "Setpoint" and "Encoder" in the same line.

wpilibc/src/test/native/cpp/simulation/StateSpaceSimTest.cpp Outdated Show resolved Hide resolved
wpilibcExamples/src/main/cpp/examples/DifferentialDriveBot/cpp/Drivetrain.cpp Outdated Show resolved Hide resolved
wpilibcExamples/src/main/cpp/examples/DifferentialDriveBot/cpp/Drivetrain.cpp Outdated Show resolved Hide resolved
wpilibcExamples/src/main/cpp/examples/DifferentialDrivePoseEstimator/cpp/Drivetrain.cpp Outdated Show resolved Hide resolved
wpilibcExamples/src/main/cpp/examples/DifferentialDrivePoseEstimator/cpp/Drivetrain.cpp Outdated Show resolved Hide resolved
wpilibcExamples/src/main/cpp/examples/SwerveBot/cpp/SwerveModule.cpp Outdated Show resolved Hide resolved
wpilibcExamples/src/main/cpp/examples/SwerveDrivePoseEstimator/cpp/SwerveModule.cpp Outdated Show resolved Hide resolved
wpilibcExamples/src/main/cpp/examples/SysId/cpp/subsystems/Shooter.cpp Outdated Show resolved Hide resolved
.../src/main/java/edu/wpi/first/wpilibj/examples/differentialdriveposeestimator/Drivetrain.java Outdated Show resolved Hide resolved
...ain/java/edu/wpi/first/wpilibj/examples/drivedistanceoffboard/subsystems/DriveSubsystem.java Outdated Show resolved Hide resolved
@narmstro2020
Copy link
Contributor Author

narmstro2020 commented Jun 24, 2024 •
edited
Loading

@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
Copy link
Contributor Author

/format

@narmstro2020
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Contributor Author

narmstro2020 commented Jun 24, 2024 •
edited
Loading

@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
Copy link
Member

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
Copy link
Contributor Author

@calcmogul
I think we're good now.

@PeterJohnson PeterJohnson merged commit 30c7632 into wpilibsuite:main Jul 17, 2024
30 checks passed
@KangarooKoala KangarooKoala mentioned this pull request Jul 17, 2024
Merged
@narmstro2020 narmstro2020 deleted the FeedForwardPeriods branch August 29, 2024 20:48
@narmstro2020 narmstro2020 mentioned this pull request Sep 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@calcmogul calcmogul calcmogul approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@narmstro2020 @calcmogul @PeterJohnson