[BUG] Shared PID constants only work for first extruder

[thatcomputerguy0101]

Did you test the latest bugfix-2.1.x code?

Yes, and the problem still exists.

Bug Description

When PID_PARAMS_PER_HOTEND is disabled, the PID constants that are supposed to be shared between all extruders will only work for the first extruder. For any other extruders, the PID algorithm receives zero for all constants. This causes PID mode to always output a power of zero, resulting in the temperature oscillating around the PID_FUNCTIONAL_RANGE threshold as if it were a bang-bang controller.

I have also attempted to reconfigure the PID constants, which has no affect on the output since all PID assignment goes to the values for extruder 0, but each extruder will still attempt to use independent values. This problem can be worked around by enabling PID_PARAMS_PER_HOTEND.

Bug Timeline

Likely when PID_PARAMS_PER_HOTEND was introduced

Expected behavior

Whenever PID_PARAMS_PER_HOTEND is disabled, the PID constants should be shared between all extruders.

Actual behavior

Extruder 1 (the second one) oscillates around 10 degrees below its target temperature, resulting in a false thermal runaway shutdown.

Steps to Reproduce

Enable PIDTEMP, disable PID_PARAMS_PER_HOTEND, optionally enable PID_DEBUG, attempt to heat up any extruder other than extruder 0, and observe its temperature readings. In the PID debug output, all three intermediate variables are stuck at zero.

Workaround

Enable PID_PARAMS_PER_HOTEND, copy the PID values from DEFAULT_Kp to each index within DEFAULT_Kp_LIST, and repeat for Ki and Kd.

Version of Marlin Firmware

2.1.1

Printer model

BIBO2 Touch

Electronics

Stock, with TFT removed

Add-ons

No response

Bed Leveling

MBL Manual Bed Leveling

Your Slicer

Cura

Host Software

Repetier Host

Don't forget to include

• A ZIP file containing your Configuration.h and Configuration_adv.h.

Additional information & file uploads

My configuration file: BIBO2 Configuration.zip

I also confirmed that it was the pid constants at zero and not the sensor inputs by adding the following line to PIDRunner::get_pid_output()

SERIAL_ECHOLNPGM("P:", tempinfo.pid.Kp, " I:", tempinfo.pid.Ki, " D:", tempinfo.pid.Kd);

Graph of the temperature and heater output before thermal runaway false alarm (at 200 °C target):

[ellensp]

#elif defined(PID_PARAMS_PER_EXTRUDER)
  #error "PID_PARAMS_PER_EXTRUDER is deprecated. Use PID_PARAMS_PER_HOTEND instead."

PID_PARAMS_PER_EXTRUDER is not used in bugfix-2.1.x

This check was added 5 Aug 2016

[ellensp]

I agree that pid debug is showing 0's eg echo:E1: Input 31.02 Output 0.00 pTerm 0.00 iTerm 0.00 dTerm 0.00 for tool 1 while heating.

so some sort of bug does seem to exist in bugfix 2.1.x

[ellensp]

digging around I see that the pid values are being loaded into hotend 0 and hotend 1 variables

eg with adding a simple serial echos

--- a/Marlin/src/module/settings.cpp
+++ b/Marlin/src/module/settings.cpp
@@ -3142,6 +3142,7 @@ void MarlinSettings::reset() {
       #define PID_DEFAULT(N,E) DEFAULT_##N
     #endif
     HOTEND_LOOP() {
+      SERIAL_ECHOLNPGM("here e", e," Kp ",float(PID_DEFAULT(Kp, ALIM(e, defKp))));
       PID_PARAM(Kp, e) =      float(PID_DEFAULT(Kp, ALIM(e, defKp)));
       PID_PARAM(Ki, e) = scalePID_i(PID_DEFAULT(Ki, ALIM(e, defKi)));
       PID_PARAM(Kd, e) = scalePID_d(PID_DEFAULT(Kd, ALIM(e, defKd)));

resulting in a serial log of

here e0 Kp 22.20
here e1 Kp 22.20

so the single defaults values are being set for all hotends.

so far I cant find where the next stage is... perhaps this will help someone more awake than I.

[thatcomputerguy0101]

```c++
#elif defined(PID_PARAMS_PER_EXTRUDER)
  #error "PID_PARAMS_PER_EXTRUDER is deprecated. Use PID_PARAMS_PER_HOTEND instead."

PID_PARAMS_PER_EXTRUDER is not used in bugfix-2.1.x
This check was added 5 Aug 2016

Whoops, my bad. I remembered the wrong name when trying to copy it here. I did mean PID_PARAMS_PER_HOTEND in every place I said PID_PARAMS_PER_EXTRUDER.

I believe the reason why only the first extruder's pid constants are set is within the PID_PARAM definition, in temperature.h, starting line 85.

#define PID_PARAM(F,H) _PID_##F(TERN(PID_PARAMS_PER_HOTEND, H, 0 & H)) // Always use 'H' to suppress warning
#define _PID_Kp(H) TERN(PIDTEMP, Temperature::temp_hotend[H].pid.Kp, NAN)
#define _PID_Ki(H) TERN(PIDTEMP, Temperature::temp_hotend[H].pid.Ki, NAN)
#define _PID_Kd(H) TERN(PIDTEMP, Temperature::temp_hotend[H].pid.Kd, NAN)
#if ENABLED(PIDTEMP)
  #define _PID_Kc(H) TERN(PID_EXTRUSION_SCALING, Temperature::temp_hotend[H].pid.Kc, 1)
  #define _PID_Kf(H) TERN(PID_FAN_SCALING,       Temperature::temp_hotend[H].pid.Kf, 0)
#else
  #define _PID_Kc(H) 1
  #define _PID_Kf(H) 0
#endif

However, I believe this is the intended behavior of a partial implementation, with the other half needing to be done whenever temp_hotend is read.

[GMagician]

PID_PARAM is supposed to call _PID_Kp, _PID_Ki, _PID_Kd with H or 0 (0 & H) as argument so it should always use E0 PID values (when PID_PARAMS_PER_HOTEND is disabled). This is what I understand of such line

[GMagician]

I suspect issue is in get_pid_output. It's in a class that has "hotend" instances so each one has its own values arrays of values

[GMagician]

maybe something like:

  float Temperature::get_pid_output_hotend(const uint8_t E_NAME) {
    const uint8_t etmp = TERN(PID_PARAMS_PER_HOTEND, HOTEND_INDEX, 0);

in temperature.cpp and use 'etmp' in const float pid_output = hotend_pid[etmp].get_pid_output();
may be a starting point...

[Bob-the-Kuhn]

I added a couple of SERIAL_ECHO lines to the debug print routine in temperature.cpp. As best I can tell the temp_hotend for tool 1 is also zero. But according to @ellensp, it is getting set correctly in settings.cpp. Which means something is stepping on the tool1 values.

    FORCE_INLINE void debug(const_celsius_float_t c, const_float_t pid_out, FSTR_P const name=nullptr, const int8_t index=-1) {
      if (TERN0(HAS_PID_DEBUG, thermalManager.pid_debug_flag)) {
        SERIAL_ECHO_START();
        if (name) SERIAL_ECHOLNF(name);
        if (index >= 0) SERIAL_ECHO(index);
        SERIAL_ECHOLNPGM(
          STR_PID_DEBUG_INPUT, c,
          STR_PID_DEBUG_OUTPUT, pid_out
          #if DISABLED(PID_OPENLOOP)
            , " pTerm: ", work_pid.Kp, " iTerm: ", work_pid.Ki, " dTerm: ", work_pid.Kd
          #endif
        );
SERIAL_ECHOLNPGM("  P: ", tempinfo.pid.Kp, "  I: ", tempinfo.pid.Ki, "  D: ", tempinfo.pid.Kd);
SERIAL_ECHOLNPGM(" P2: ", Temperature::temp_hotend[index].pid.Kp, " I2: ",Temperature::temp_hotend[index].pid.Ki, " D2: ", Temperature::temp_hotend[index].pid.Kd);
      }
    }
  };

Results for t0:

Recv: echo:E
Recv: 0: Input 23.13 Output 0.00 pTerm: 0.00 iTerm: 0.00 dTerm: 0.00
Recv: P: 14.79 I: 0.01 D: 7935.00
Recv: P2: 14.79 I2: 0.01 D2: 7935.00

Results for t1:

Recv: echo:E
Recv: 1: Input 28.69 Output 0.00 pTerm: 0.00 iTerm: 0.00 dTerm: 0.00
Recv: P: 0.00 I: 0.00 D: 0.00
Recv: P2: 0.00 I2: 0.00 D2: 0.00

[GMagician]

What I suspect, but can't do tests now, is that _PID_K? macros alway store values to array index 0, when you read all data from eeprom but PIDRunner class functions read from extruder passed index

[ellensp]

Yes I suspect the same.. I think #define PID_PARAM(F,H) PID##F(TERN(PID_PARAMS_PER_HOTEND, H, 0 & H) is wrong and should be #define PID_PARAM(F,H) PID##F(H)
As even without PID_PARAMS_PER_HOTEND there are still pid values for each hotend.

[Bob-the-Kuhn]

I'm confused. Seems like Temperature::temp_hotend[index].pid.Kp is giving me different values than PID_PARAM(Kp, index) for index 1 but the same values for index 0.

SERIAL_ECHOLNPGM("  P: ", tempinfo.pid.Kp, "  I: ", tempinfo.pid.Ki, "  D: ", tempinfo.pid.Kd);
SERIAL_ECHOLNPGM(" P2: ", Temperature::temp_hotend[index].pid.Kp, " I2: ",Temperature::temp_hotend[index].pid.Ki, " D2: ", Temperature::temp_hotend[index].pid.Kd);
SERIAL_ECHOLNPGM(" P3: ", PID_PARAM(Kp, index), " I3: ",PID_PARAM(Ki, index), " D3: ", PID_PARAM(Kd, index));

results in for tool1:

Recv: P: 0.00 I: 0.00 D: 0.00
Recv: P2: 0.00 I2: 0.00 D2: 0.00
Recv: P3: 14.79 I3: 0.01 D3: 7935.00

[thatcomputerguy0101]

Yes I suspect the same.. I think #define PID_PARAM(F,H) PID##F(TERN(PID_PARAMS_PER_HOTEND, H, 0 & H) is wrong and should be #define PID_PARAM(F,H) PID##F(H)
As even without PID_PARAMS_PER_HOTEND there are still pid values for each hotend.

My understanding was that it was the other way around; it is wrong wherever the PID values is read (within PIDRunner::get_pid_output). That way, PID0 is used as the definitive PID source, and the rest are ignored by both sides whenever PID_PARAMS_PER_HOTEND is disabled.

I don't have any ideas for a solution without significant edits since PIDRunner is currently given the heater struct, not the index of the hotend (since it is used by all heaters), so it is unable to determine the correct PID values to use.

[GMagician]

What I think is to change:

      static PIDRunnerHotend hotend_pid[HOTENDS] = {
        #define _HOTENDPID(E) temp_hotend[E],
        REPEAT(HOTENDS, _HOTENDPID)
      };

to:
What I think is to change:

      static PIDRunnerHotend hotend_pid[HOTENDS] = {
        #define _HOTENDPID(E) temp_hotend[TERN(PID_PARAMS_PER_HOTEND, E, 0 & E)],
        REPEAT(HOTENDS, _HOTENDPID)
      };

give it a try and I'll write a PR if it works

[thatcomputerguy0101]

I think the problem with that is that the structs within temp_hotend stores more than just the PID information (such as the target temperature), and only the PID information should be shared.

[GMagician]

Yep but I hope that what above is done on initialization while other info are dynamic...

[Bob-the-Kuhn]

In settings.cpp I replaced the macros with the expanded versions and now tool1 has a nice PID waveform about the setpoint.

     - PID_PARAM(Kp, e) =      float(PID_DEFAULT(Kp, ALIM(e, defKp)));
     - PID_PARAM(Ki, e) = scalePID_i(PID_DEFAULT(Ki, ALIM(e, defKi)));
     - PID_PARAM(Kd, e) = scalePID_d(PID_DEFAULT(Kd, ALIM(e, defKd)));
     + Temperature::temp_hotend[e].pid.Kp =      float(PID_DEFAULT(Kp, ALIM(e, defKp)));
     + Temperature::temp_hotend[e].pid.Ki = scalePID_i(PID_DEFAULT(Ki, ALIM(e, defKi)));
     + Temperature::temp_hotend[e].pid.Kd = scalePID_d(PID_DEFAULT(Kd, ALIM(e, defKd)));

[Bob-the-Kuhn]

@GMagician - I did your change (and backed out my change to settings.cpp) and now the output stays off no matter what setpoint I give it.

[GMagician]

I think it's a little bit hard than that, I lose myself to find out what is what.
hotend_pid is array of PIDRunnerHotend
PIDRunnerHotend is defined as PIDRunner<hotend_info_t, 0, PID_MAX>
hotend_info_t is struct PIDHeaterInfo<hotend_pid_t>
that is HeaterInfo + hotend_pid_t

so @Bob-the-Kuhn you're right, it can't run because only 'hotend_pid_t' part of the struct should be copied from "index 0"

[GMagician]

But what I said is correct. when loaded from eeprom only temp_hotend[0].pid is loaded but each PIDRunner class uses its own temp_hotend[x].pid values.

[GMagician]

If PID_PARAM(F,H) write value to all temp_hotend it may works, but this macro is also used to read values...

[GMagician]

One solution that may be tryed is to split PID_PARAM to SET_PID_PARAM and GET_PID PARAM then
GET_PID PARAM uses current code while
SET_PID_PARAM does something like HOTENDS LOOP to set value to all (only when PID_PARAMS_PER_HOTEND is disabled of course)

@ellensp & @Bob-the-Kuhn what do you think about that?

[GMagician]

@thatcomputerguy0101 I tryed a different approach. May you test it?

[GMagician]

I forgot M502 & M500 are required

[Bob-the-Kuhn]

Looks like you got it.

Nice PID waveforms being controlled about the setpoint on E1. Debug print statements showing the expected PID paramaters.

[GMagician]

Yep, the problem now is LCD edit.. I have to figure out what to do

[GMagician]

Ok, now PR is ready, whomever may do a complete test (part has been done by @Bob-the-Kuhn).
LCD & autotune edits and a new "PID working as expected" test. I have no H/W to test with

[thisiskeithb]

Fixed in #24673

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.