0035 — PidController's error sign is inverted twice and the net convention is undocumented

ID	0035
Class	DOCUMENTATION
Severity	2
Status	ready
Models	Trawl/PIDController
Found	2026-09-22

Evidence

src/trawl/PidController.cpp:55-60 forms the error the wrong way round:

double ref         = m_inReference->GetPortValue(dT, adX)[0];
double measurement = m_inMeasurement->GetPortValue(dT, adX)[0];
double error       = measurement - ref;
m_output           = m_PID.CalcOutput(error, dT);

The usual convention is reference - measurement. Read on its own, that line says the gains of any model wired to this class must be negated.

sfh::filters::PID::CalcOutputWithoutTimeshift then negates every one of the three parts (fhlib/src/filters/PID.cpp:56-64):

double dDerPart  = sfh::math::Bound(-m_dKD * dTimeDer, -m_dMaxDerPart, m_dMaxDerPart);
double dPropPart = -m_dKP * m_dErrorNow;
double dTimeInt0 = m_dIntPart - m_dKI * m_dErrorNow * dDeltaT;

The two inversions cancel exactly, so the composition of the two classes is the standard convention

Out = kP (reference - measurement)
    + kI integral(reference - measurement) dt
    + kD d(reference - measurement)/dt,

and a gain therefore takes the sign of the plant it drives, not the opposite of it.

Neither class says so. PidController.h's parameter table documents ProportionalGain as "Proportional gain" and stops, sfh::filters::PID has no comment on the sign at all, and the only worked example in the tree, SimplePIDController.xml, uses ProportionalGain = "1" with IntegratorGain and DerivativeGain at zero on a plant whose sign the file does not state, so it settles nothing.

Confirmed by experiment in tests/AuvClosedLoop_Test.cpp, Auv_ClosedLoopSignsFollowThePlant: with the plant signs Auv establishes — a positive RudderYaw raises the heading, a positive RudderDive lowers the pitch, a positive ThrustPower raises the speed — the three working gain sets are positive, negative and positive respectively. A uniformly negative set, which PidController.cpp:58 on its own implies, diverges on two of the three loops.

Effect

Anyone reading PidController.cpp and not following CalcOutput into fhlib concludes that the gains must be negated, and writes an XML in which two thirds of the loops run away. The AUV plan for this repository reached exactly that conclusion and specified negative gains for all three of its loops before the fixture was built.

The existing trawl models are unaffected, because their gains were found by experiment against the assembled behaviour rather than derived from the source.

The inverse error is equally available: a reader who finds the negation in fhlib first and does not notice the subtraction in PidController also lands on the wrong sign.

Possible fix

Documentation only. Do not change PidController.cpp:58 and do not change sfh::filters::PID: either edit silently flips the meaning of every gain in every existing trawl model, and the composition is already correct.

1. State the composed convention in the PidController Doxygen block, as the formula above, together with the sentence that a gain takes the sign of the plant.
2. Expand the ProportionalGain, IntegratorGain and DerivativeGain rows of the parameter table to say that a plant whose output falls when the controller output rises needs negative gains.
3. Add a comment at PidController.cpp:58 naming the second inversion in sfh::filters::PID and saying that the pair is deliberate, so the line is not "corrected" by a later reader.
4. Upstream, sfh::filters::PID::Initialize deserves the same one-line note. That part is not in this repository.

Test that would prove it

Auv_ClosedLoopSignsFollowThePlant already asserts the composed sign directly: it runs a fixture whose three errors have known signs at the start and asserts the sign of each controller's first output. A pure-unit version belongs next to PIDController_Test.cpp, which today is a single regression comparison and asserts nothing about the sign: drive a Trawl/PIDController with a constant reference above a constant measurement and assert that the output of a positive-gain controller is positive.

Risk

None from documenting it. The risk is in the fix that looks obvious and is not: changing either subtraction would invert every Trawl/PIDController in every existing trawl model at once, and the trawl regressions would catch it only as an unexplained baseline diff.