Contribution diagnostic calculations are incorrect

[anssih]

Looking at the code, the contribution calculations do not seem correct.

The code has

      i_contrib = pid._integral * ki

but pid._integral has already been multiplied by ki within simple-pid.

For D the code has

      d_contrib = pid._last_output - output if pid._last_output is not None else 0.0

but as far as I can see it should be (error - last_error) / sample period.

The simple-pid module has support for getting the contributions, so AFAICS we could just use that instead:

      p_contrib, i_contrib, d_contrib = pid.components

[bvweerd]

Good one, taken into bugfix release v1.3.2-beta, please check

[anssih]

Thanks, seems correct now.

However, now I wonder if instead of (or in addition to) the i_contrib we should be showing the delta between the previous i_contrib and current i_contrib.
When the set point has been reached, i_contrib will be almost equal to the set point output value, while delta i_contrib would show only the actual effect it had on the control value compared to the previous control value (similar to what p and d contributions already show, since they are not cumulative in the same way).

[bvweerd]

I would opt for an extra diagnostics sensor then, see v1.3.3-beta

[anssih]

v1.3.3-beta does not contain any extra sensor and the contributions fix from v1.3.2-beta is now also removed.

[bvweerd]

Okay, I'm a bit confused why the tagging fails me in Github... I recreated the tags and the release v1.3.2-beta

[anssih]

The delta is backwards, should be new_i - last_i (so that larger current I results in more positive delta I).

[bvweerd]

fixed in v1.3.2