[wpimath] Add plant constructor to Feedforward classes #7144
Conversation
|
/format |
There was a problem hiding this comment.
We already have a class for autocomputing feedforward and feedback: LinearPlantInversionFeedforward and LinearQuadraticRegulator respectively. We have classes that wrap them like ElevatorFeedforward, ArmFeedforward, and LTVUnicycleController.
wpimath's oringally intended design had users create a model from physical constants or feedforward gains via a LinearSystemId factory function. The feedforward, feedback, and physics simulation classes consumed this model. In the following years, we've added overloads that take feedforward gains directly, but that bloated the interface. We started with one way to do things, and now we have two. This PR adds a third. We should decide on one cohesive design instead.
Synthesizing specific feedforward gains feels like a step backwards, because LinearSystemId already gives you a more general model that the existing classes can consume. Creating a LinearSystem, synthesizing feedforward gains from it, then passing those gains to a physics sim class that creates another LinearSystem internally feels redundant.
wpimath/src/main/java/edu/wpi/first/math/controller/ControllerConstants.java
Outdated
Show resolved
Hide resolved
wpimath/src/main/java/edu/wpi/first/math/system/plant/FeedforwardUtil.java
Outdated
Show resolved
Hide resolved
I may have made the use case muddled. Apologies for that. I'm not sure this is adding a third? All this would do inherently is extract out kv, and ka from the Sim classes (also calculating kg for gravity systems). Now if the plant was made from the kv, ka overloads that would be silly, but if the plant was created from the physical overaloads then it would serve a purpose to then make a FF controller and a FB controller for testing/sim/teaching purpose. I suppose one could add methods to the sim class to handle extracting kv, ka, kg, but having feedback gains method would then be needed. I'm okay scrapping this and/or rewriting, but do you have any thought as to what to do? EDIT: Or I suppose methods to grab kv, ka out of LinearPlantInversionFeedforward and LQR would be a possibility? I'd like to make the footprint as small as possible. |
|
Again, you don't need the feedforward constants to construct a sim physics class. They already take a LinearSystem, so the extra step is unnecessary. |
I agree. I'm talking about grabbing the kv, ka constants to create a Feedforward controller. Unless there is a better method for use when running a Sim class that I don't know about. |
|
We have LinearPlantInversionFeedforward for that. Just add a LinearSystem constructor overload to the wrapper feedforward classes. |
Well I can that. Thanks. Should I make methods to get kp, kd from LQR? Or is there another possiblity? |
|
@calcmogul Like this for starters? I would like to do this for Flywheel as well, but wanted to check with you first. Plus the Flywheel will have type erasure issues. |
|
/format |
No, because k_p and k_d only make sense for a 2x1 (2 states, one input) system, and LQR supports any dimensionality. In the 2x1 special case, users can extract k_p via A PIDController constructor overload that takes an LQR<2, 1> doesn't sit well with me because you might as well just use the LQR instance you already have for control (granted, the code for that is much cleaner in C++ than in Java). It also feels like interface bloat for what was otherwise a clean separation; it's odd that such an advanced class gets mixed into a simple one's interface. |
wpimath/src/main/java/edu/wpi/first/math/controller/SimpleMotorFeedforward.java
Outdated
Show resolved
Hide resolved
| // B_d = A⁻¹(eᴬᵀ - I)B | ||
| // A = −kᵥ/kₐ | ||
| // B = 1/kₐ | ||
| // A_d = eᴬᵀ |
There was a problem hiding this comment.
Are these from a different PR? The original indentation was fine.
There was a problem hiding this comment.
This was spawned from the main branch.
There was a problem hiding this comment.
It looks like the formatter "fixed" it
There was a problem hiding this comment.
Do you want me to go ahead with fixing the indents.
There was a problem hiding this comment.
I'll have the wait on this till the Elevator and Arm FF's are merged. One that will save time on the indents and the period will be part of the constructors at that point.
|
/format |
|
@calcmogul Should I close this and reopen now that the FF classes have been updated or continue from this PR |
narmstro2020
force-pushed
the
SysIdUtil
branch
from
da95073 to
ac907f7
Compare
|
/format |
narmstro2020
changed the title
narmstro2020
changed the title
|
I'm going to close this PR for now. I might open up a similar one later, but I haven't had a chance to retool this at all recently. |
So I'm having a little difficulty naming and placing these utilities.
Here is the primary use case. One can create a DCMotorSim, ElevatorSim, etc. via LinearSystemId's physical methods. If you wish to test out code using the simulator with feedback and feedforward controllers you would need to determine gains. One could manually tune and in a real system one can gain help from sysId, but I thought it would be handy to have a means of determining estimated gains from the plants themselves not just as a way of testing code without a robot (in the off season, etc) but also as a potential teaching tool for the concepts of feedback and feedforward. Most of the logic here is derived from sysId. I suppose that these utilities could be used as sanity checks, but the primary use case in my mind would be to help the usability of the simulators.
I haven't really done much to this other than propose the basic concept. Documentation is needed, C++ is needed. All of which I will do, but I want to start this as a draft first.