Closed thatcomputerguy0101 closed 2 years ago
#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
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
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.
```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.
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
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
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...
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
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
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.
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
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.
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
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.
Yep but I hope that what above is done on initialization while other info are dynamic...
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)));
@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.
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"
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.
If PID_PARAM(F,H) write value to all temp_hotend it may works, but this macro is also used to read values...
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?
@thatcomputerguy0101 I tryed a different approach. May you test it?
I forgot M502 & M500 are required
Looks like you got it.
Nice PID waveforms being controlled about the setpoint on E1. Debug print statements showing the expected PID paramaters.
Yep, the problem now is LCD edit.. I have to figure out what to do
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
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.
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 thePID_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 introducedExpected 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
, disablePID_PARAMS_PER_HOTEND
, optionally enablePID_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 fromDEFAULT_Kp
to each index withinDEFAULT_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
Configuration.h
andConfiguration_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()
Graph of the temperature and heater output before thermal runaway false alarm (at 200 °C target):