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.

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

[wpimath] fully discretized ElevatorFF and ArmFF #7024

Open
wants to merge 63 commits into
base: main
Choose a base branch
from

Conversation

narmstro2020
Copy link
Contributor

@narmstro2020 narmstro2020 commented Sep 2, 2024

So in a similar setup with SimpleMotorFeedforward I've discretized the Elevator and the Arm.

Here is a summary of changes

  1. deprecated existing calculate methods and created 2 overloads using the units class and discretized state-space (the arm uses a different setup so only the methods exist)
  2. Rewrote/reorganized the derivations for the calculate methods in Simple and Elevator.
  3. Added appropriate items to protos and structs for new Elevator and Arm setup.
  4. Cleaned up units in the Java docs for constructors and methods.
  5. Further documentation/comment cleanup.
  6. A MutableMeasure field has been added that handles the output of the calculate methods without recreating a Measure object each time its called giving the GC a break.

Left to do

  1. The C++ version. I'm working on that now, but wanted some feedback first
    EDIT: DONE

Would be nice, maybe? Looking for feedback.

  1. Make static factories for creation of FF's and make the constructors private for better ergonomic usage. These factories would use the units class. SimpleFF would need one each for angular and one for linear.
  2. Use the units class throughout all of the FF's
  3. rename SimpleMotorFeedforward to SimpleFeedforward
  4. Put the A_d and B_d as private final fields precalculated during construction so the calculate isn't wasting time creating them on every call.

@narmstro2020 narmstro2020 requested review from a team as code owners September 2, 2024 23:01
@narmstro2020
Copy link
Contributor Author

@calcmogul It looks like our friend the ArmSimulationTest is back again.

@narmstro2020
Copy link
Contributor Author

narmstro2020 commented Sep 3, 2024

How do I fix the pregenerated files check? I cannot seem to find the error in the logs for the check.

@narmstro2020
Copy link
Contributor Author

/format

@narmstro2020
Copy link
Contributor Author

/format

@sciencewhiz
Copy link
Contributor

How do I fix the pregenerated files check? I cannot seem to find the error in the logs for the check.

You shouldn't be modifying generated files directly, in your case
wpimath/src/generated/main/java/edu/wpi/first/math/proto/Controller.java. Instead, modify the corresponding .proto file https://github.com/wpilibsuite/allwpilib/blob/main/wpimath/src/main/proto/controller.proto

@narmstro2020
Copy link
Contributor Author

narmstro2020 commented Sep 3, 2024

How do I fix the pregenerated files check? I cannot seem to find the error in the logs for the check.

You shouldn't be modifying generated files directly, in your case wpimath/src/generated/main/java/edu/wpi/first/math/proto/Controller.java. Instead, modify the corresponding .proto file https://github.com/wpilibsuite/allwpilib/blob/main/wpimath/src/main/proto/controller.proto

So don't modify Controller, modify controller.proto?

Revert Controller.java back to previous?

@narmstro2020
Copy link
Contributor Author

@sciencewhiz. Reverted back to original Controller.java and updated controller.proto, but I'm still getting a failure

@sciencewhiz
Copy link
Contributor

/pregen

@narmstro2020
Copy link
Contributor Author

/format

@narmstro2020
Copy link
Contributor Author

/format

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.

Given how many times this derivation is duplicated, it may be better to move a copy to wpimath/algorithms.md and reference the section heading in all the places that derivation is applicable.

@narmstro2020
Copy link
Contributor Author

Given how many times this derivation is duplicated, it may be better to move a copy to wpimath/algorithms.md and reference the section heading in all the places that derivation is applicable.

Agreed. I would like to finish the cleanup in the code first before moving it if that is alright with you?

@narmstro2020
Copy link
Contributor Author

/format

@narmstro2020
Copy link
Contributor Author

@calcmogul how's this look before I move it to the algorithms.md file?

@calcmogul
Copy link
Member

calcmogul commented Sep 14, 2024

There's like five copies, so idk. It'll be easier to review once it's deduplicated.

@narmstro2020
Copy link
Contributor Author

There's like five copies, so idk. It'll be easier to review once it's deduplicated.

Okay no problem. I'll get started on that.

@narmstro2020
Copy link
Contributor Author

And I have a first draft for algorithms.md

@narmstro2020
Copy link
Contributor Author

/format

@narmstro2020
Copy link
Contributor Author

@calcmogul Anything left to do/check on this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants