Skip to content

indi_celestron_aux: stop PID corrections when adaptive tuning is disabled#1318

Open
Copilot wants to merge 3 commits into
masterfrom
copilot/fix-adaptive-tracking-corrections
Open

indi_celestron_aux: stop PID corrections when adaptive tuning is disabled#1318
Copilot wants to merge 3 commits into
masterfrom
copilot/fix-adaptive-tracking-corrections

Conversation

Copy link
Copy Markdown

Copilot AI commented May 8, 2026

Adaptive PID tuning in indi_celestron_aux could still influence tracking after tuning was turned off because PID correction output continued to be applied and controller state could carry over between tracking sessions. This change gates PID correction application to active tuning state and resets control state on tracking reset.

  • Track-rate correction now follows tuning state

    • In TimerHit(), PID correction is only computed/applied when the corresponding tuner is actively tuning (isActivelyTuning()).
    • When tuning is off, tracking rate falls back to predictive tracking only.
  • Reset PID controller state when tracking is reset

    • In resetTracking(), both axis controllers now call reset() after construction to clear accumulated error/integrator state.
    • Existing adaptive tuner resets in resetTracking() are retained to clear prior tuning history before a new session.
  • Small readability cleanup

    • Renamed altitude correction temp variable from pidCorrectionAl to pidCorrectionAlt.
double pidCorrectionAz = 0;
if (m_az_pid_tuner && m_az_pid_tuner->isActivelyTuning())
    pidCorrectionAz = m_Controllers[AXIS_AZ]->calculate(0, -offsetSteps[AXIS_AZ]);
trackRates[AXIS_AZ] = predRate[AXIS_AZ] + pidCorrectionAz;
Original prompt

Problem

The indi_celestron_aux driver exhibits an anomaly where PID adaptive tracking corrections remain active even after adaptive tuning is switched off. This happens because:

  1. The AdaptivePIDTuner retains its internal state (including a non-zero integral value Ki) after tuning is stopped.
  2. The main driver loop in celestronaux.cpp continues to apply the PID calculation results to the tracking rate regardless of whether adaptive tuning is currently enabled.
  3. The PID controllers are not explicitly reset to zero error when tracking starts, potentially carrying over "wind-up" or old error states.

Proposed Changes

This PR implements the following fixes:

  • Conditional PID Application: Modified the TimerHit() tracking logic to only apply PID corrections (m_Controllers->calculate()) if the respective axis's adaptive tuner is actually in an active tuning state (isActivelyTuning()).
  • Explicit Controller Reset: Added calls to m_Controllers->reset() within resetTracking() to ensure that the PID error state and integrator are zeroed whenever tracking is initialized or reset.
  • Tuner Reset: Ensured the adaptive tuners are reset when tracking starts to prevent old data history from affecting the new tracking session.

These changes ensure that when adaptive tuning is disabled, the mount correctly reverts to pure predictive tracking based on the calculated celestial rates.

Affected Files

  • indi-celestronaux/celestronaux.cpp

The following is the prior conversation context from the user's chat exploration (may be truncated):

User: Check if it is possible that the PID adaptive tracking in indi_celestronaux driver is still active even when it is switched off (but the integral value is non-zero). I have observed some anomaly with tracking and I am suspecting an error in this logic.

Assistant: Based on the investigation of the indi_celestron_aux driver source code, there is indeed a logic path where the adaptive tuning could affect tracking even if it is not explicitly active, specifically if the integral value is non-zero.

Analysis of the Adaptive Tuning Logic

In adaptive_tuner.cpp, the AdaptivePIDTuner maintains its own internal state for the PID gains (m_currentKp, m_currentKi, m_currentKd).

  1. State Persistence: The reset() method (lines 89–108) specifically notes that it does not reset the current gains to initial values:
    // Note: Current Kp, Ki, Kd are NOT reset here by default.
    // They hold the last tuned values or initial values.
  2. Tuning Activation vs. Gain Usage: The stopActiveTuning() method (lines 73–77) sets m_is_tuning_active = false. This prevents the analyzeErrorAndAdjustGains() function from being called, which stops the driver from modifying the gains further. However, it does not reset the gains to a "neutral" or "default" state.
  3. Gain Retrieval: When the main driver calls getAdaptedGains() (lines 154–159), it simply returns the current values:
    void AdaptivePIDTuner::getAdaptedGains(double &outKp, double &outKi, double &outKd) const
    {
        outKp = m_currentKp;
        outKi = m_currentKi;
        outKd = m_currentKd;
    }

Observed Anomaly

If the adaptive tuner was active and accumulated a significant Integral (Ki) value to compensate for steady-state error, and then tuning was switched off, that Ki value remains stored in the tuner. If the driver continues to pull these "adapted" gains and applies them to the motor controllers, the non-zero integral component will continue to influence the tracking behavior.

If the internal state of the PID controller (which uses these gains) is not cleared when tracking is toggled or when the tuner is disabled, the "wind-up" or offset created by the integral term would persist. This matches your suspicion that the integral value is still active even when the adaptive logic is technically "off."

Conclusion

The current implementation treats "Active Tuning" as a process of discovery rather than a toggle for the application of the gains. To fix this, the driver would need to either:

  • Reset gains to defaults when tuning is disabled.
  • Ensure that the PID controllers are reset when the tracking state changes to prevent old integral sums from causing anomalies.

User: When adaptive tunning is switched off the gains should not be applied at all. Your discovery matches my observation. Propose minimal changes to the code which changes the behaviour to, in addidion to disable changes of weights, switches applications of any PID corrections to the tracking rate. Regardless of the values of Kx and accumulated errors. The PID controllers should be reset to zero error when the control is switched on. Can you generate PR for this?

Assistant: I have analyzed the indi_celestron_aux driver code, and your suspicion regarding the PID adaptive tracking logic is correct.

Currently, the driver co...

This pull request was created from Copilot chat.

Copilot AI changed the title [WIP] Fix adaptive tracking corrections in indi_celestron_aux driver indi_celestron_aux: stop PID corrections when adaptive tuning is disabled May 8, 2026
Copilot finished work on behalf of jochym May 8, 2026 11:31
Copilot AI requested a review from jochym May 8, 2026 11:31
@jochym
Copy link
Copy Markdown
Contributor

jochym commented May 8, 2026

I was hunting this anomally for some time. The PID corrections are added to the tracking rates even if PID is switched off (!). This is made worse by the fact that the default Ki for Alt is 1 not zero. This makes Alt wonder slightly in fairly random fashion. It is a nasty trap for a newbie. This PR additionally resets the controllers on switch ON to clear the state. @knro - please review and possibly merge it before next release - this is fairly non-intrusive and obvious fix. The workaround for this bug is to set all PID coeffs to 0.

While conditional application of PID corrections is obvious, I am not sure the reset code is fully correct. Can you verify this @knro ?

@jochym
Copy link
Copy Markdown
Contributor

jochym commented May 8, 2026

BTW. The build errors have nothing to do with this change - we have some problems with libs on some builds.

@jochym jochym requested a review from knro May 8, 2026 11:56
@jochym jochym marked this pull request as ready for review May 10, 2026 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants