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

Add a PID to smoothen the apparent angular momentum given by KSP #2550

Merged
merged 18 commits into from
May 3, 2020

Conversation

pleroy
Copy link
Member

@pleroy pleroy commented May 2, 2020

With the default parameters, it avoids the glitches seen in Fubini oscillator.

Another attempt at alleviating #2519.

// from KSP.
PID<Bivector<AngularMomentum, ApparentPileUp>,
/*horizon=*/25,
/*finite_difference_order=*/5> pid_;
Copy link
Member

Choose a reason for hiding this comment

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

apparent_angular_momentum_, or apparent_angular_momentum_controller_; no need to speak Hungarian, but it would be useful to document what is being controlled.

Copy link
Member Author

Choose a reason for hiding this comment

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

apparent_angular_momentum_controller_

Sorry, something went wrong.

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

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

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);

Copy link
Member Author

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.

int size = errors_.size();
if (size <= horizon) {
// If we don't have enough error history, just return our input.
return apparent;
return process_variable;
Copy link
Member

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.

Copy link
Member Author

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_;
Copy link
Member

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_;

Copy link
Member Author

@pleroy pleroy May 3, 2020

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.

Copy link
Member

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>>.

Copy link
Member Author

Choose a reason for hiding this comment

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

Using Normed as discussed.

}

TEST_F(PIDTest, Geometry) {
PID<Velocity<F>, /*horizon=*/10, /*finite_difference_order=*/3> pid(
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

A vector of currents.

@eggrobin eggrobin added the LGTM label May 3, 2020
@pleroy pleroy merged commit 29570bd into mockingbirdnest:master May 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants