[AUD-022][High] Unify PID simulator time and frequency parameter semantics

[gabrielsantosphilips]

Audit finding

• ID: AUD-022
• Status: Verified defect
• Severity: High
• Confidence: High
• Audited revision: 2f479320d805a1f9f35ebe4afaaeeded48913a94

Problem

Step/ramp simulation passes UI kp, ki, and kd directly into the discrete incremental recurrence. Bode/root-locus code interprets the same values as continuous Kd*s^2 + Kp*s + Ki coefficients and then discretizes them. No Ki*Ts or Kd/Ts mapping reconciles the models, so views represent different controllers.

Source:

numerical-toolbox-cpp/simulator/controllers/PidController/application/PidSimulator.cpp

Lines 33 to 43 in 2f47932

	TransferFunction PidSimulator::GetPidTf() const
	{
	float kp = configuration.tunings.kp;
	float ki = configuration.tunings.ki;
	float kd = configuration.tunings.kd;
	return { { kd, kp, ki }, { 1.0f, 0.0f } };
	}
	TransferFunction PidSimulator::GetOpenLoopTf() const

and

numerical-toolbox-cpp/simulator/controllers/PidController/application/PidSimulator.cpp

Lines 109 to 133 in 2f47932

	std::size_t numSamples = referenceSignal.size();
	TimeResponse response;
	response.time.resize(numSamples);
	response.reference = referenceSignal;
	response.output.resize(numSamples, 0.0f);
	response.controlSignal.resize(numSamples, 0.0f);
	response.error.resize(numSamples, 0.0f);
	::controllers::PidIncrementalSynchronous<float> pid(configuration.tunings, configuration.limits);
	plant->Reset();
	for (std::size_t i = 0; i < numSamples; ++i)
	{
	response.time[i] = static_cast<float>(i) * dt;
	float y = plant->Output();
	response.output[i] = y;
	response.error[i] = referenceSignal[i] - y;
	pid.SetPoint(referenceSignal[i]);
	float u = pid.Process(y);
	response.controlSignal[i] = u;

Allowed Kd=0 also leaves a leading-zero polynomial that the root solver treats as full order.

Acceptance criteria

• Define one PID gain convention for all views.
• Derive time, Bode, and root-locus behavior from the same discrete controller.
• Trim leading polynomial zeros safely.
• Add cross-view pole and step-response consistency tests.

[gabrielfrasantos]

Fixed on claude/repo-issues-triage-pgbb6e. The convention choice is the part worth reviewing.

Deriving the mapping first, so the choice is explicit. The incremental recurrence is u[k] = u[k-1] + a0 e[k] + a1 e[k-1] + a2 e[k-2] with a0 = kp + ki + kd, a1 = -kp - 2kd, a2 = kd. Backward-Euler discretizing the continuous C(s) = Kp + Ki/s + Kd·s gives numerator (Kp + Ki·T) + (-Kp - 2Kd/T) z^-1 + (Kd/T) z^-2. Matching terms:

kp = Kp      ki = Ki·T      kd = Kd/T

I made the UI tunings the continuous gains Kp, Ki, Kd, and the time simulation now converts to the discrete incremental gains via ToDiscreteTunings before constructing PidIncrementalSynchronous. GetPidTf() was already written against the continuous convention, so this keeps the Bode and root-locus views as the reference and brings the step and ramp responses into line with them.

The alternative — treating the tunings as already-discrete and rewriting GetPidTf() to return (a0 z^2 + a1 z + a2)/(z^2 - z) — is equally defensible. I chose continuous because a user reading a Bode plot is thinking in Kp/Ki/Kd, and because the plant transfer function the controller is multiplied against is itself continuous, so mixing an s-domain plant with a z-domain controller would have been meaningless.

This changes the numbers in the step and ramp views for any saved configuration: with the default sample time, ki is now scaled by Ts and kd divided by it. Existing tunings will behave differently and will need re-tuning. If you would rather keep the time response bit-identical and move the frequency views instead, say so and I will flip it.

Leading-zero trimming is also in: GetPidTf() trims the numerator when Kd = 0, and DurandKerner::Solve now trims leading zero coefficients itself, so the root solver no longer treats a leading-zero polynomial as full order.

---

Generated by Claude Code