-
Notifications
You must be signed in to change notification settings - Fork 69
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
Add a PID to smoothen the apparent angular momentum given by KSP #2550
Conversation
// kd = 0.0025 s | ||
// } | ||
template<typename Q> | ||
Q GetPidFlagOr(std::string_view const name, Q const default) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Principia style is to keep acronyms in upper case in camel case identifiers, see ICRS
, ParseTT
, ParseTAI
, etc. This should say PID.
I would want this to return an optional and drop the Or
but obviously this does not work because we cannot deduce Q, unless we build a mechanism to declare flags. Open an issue to track that, we don’t need it yet but we likely will at some point, the flags are inevitably becoming useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
numerics/pid_body.hpp
Outdated
@@ -56,7 +66,7 @@ Value PID<Value, horizon, finite_difference_order>::ComputeValue( | |||
Variation<Value> const derivative = | |||
FiniteDifference(errors, Δt, finite_difference_order - 1); | |||
|
|||
return actual + (kp_ * proportional + ki_ * integral + kd_ * derivative); | |||
return set_point + (kp_ * proportional + ki_ * integral + kd_ * derivative); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This last addition should not be part of the PID controller, move it to the call site.
numerics/pid.hpp
Outdated
// Adds the error between the process variable and the set-point to the state | ||
// of the PID and returns an updated process variable derived from the | ||
// set-point and the control variable. | ||
Value ComputeValue(Value const& process_variable, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename, templatize further, and pass an Instant
.
ControlOutput ComputeControlOutput(Controlled const& process_variable,
Controlled const& set_point,
Instant const& t);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling the types Input
and Output
, it's clearer.
numerics/pid_body.hpp
Outdated
int size = errors_.size(); | ||
if (size <= horizon) { | ||
// If we don't have enough error history, just return our input. | ||
return apparent; | ||
return process_variable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this becomes return κp * errors_.back()
, but it is somewhat disturbing that we need to introduce this other proportional constant κp (here κp = 1).
I guess this means that during startup we have a more heavy-handed proportional controller.
It might actually make some sense to experiment with other values of κp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Experimenting is very time-consuming, and this part is used for 500 milliseconds at the beginning anyway.
numerics/pid.hpp
Outdated
// of the PID and returns an updated process variable derived from the | ||
// set-point and the control variable. | ||
Value ComputeValue(Value const& process_variable, | ||
Value const& set_point, | ||
Time const& Δt); | ||
|
||
private: | ||
double const kp_; | ||
Inverse<Time> const ki_; | ||
Time const kd_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The types become:
Derivative<ControlOutput, Controlled> kp_;
Derivative<ControlOutput, Product<Controlled, Time>> ki_;
Derivative<ControlOutput, Variation<Controlled>> kd_;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Controlled
is a Bivector
. Our templates are not ready to compute this Derivative
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, but note that then you need Quotient<ControlOutput, Difference<Controlled>>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using Normed
as discussed.
numerics/pid_test.cpp
Outdated
} | ||
|
||
TEST_F(PIDTest, Geometry) { | ||
PID<Velocity<F>, /*horizon=*/10, /*finite_difference_order=*/3> pid( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this test, control a Position
, and make the control output a Current
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A vector of currents.
With the default parameters, it avoids the glitches seen in
Fubini oscillator
.Another attempt at alleviating #2519.