MarlinFirmware / Marlin

Marlin is an optimized firmware for RepRap 3D printers based on the Arduino platform. Many commercial 3D printers come with Marlin installed. Check with your vendor if you need source code for your specific machine.
https://marlinfw.org
GNU General Public License v3.0
16.28k stars 19.23k forks source link

[BUG] [Possible Safety Issue] MAXTEMP_BED not triggering #13711

Closed marcio-ao closed 5 years ago

marcio-ao commented 5 years ago

Description

We build a test fixture for our printers which puts a 78.5 ohm resistor across the thermistor inputs to test MAX_TEMP. It works fine for the hot ends, but for the heated bed I am able to have Marlin show a temperature of 623C for the heated bed and Marlin does not error out.

We also tried 220 ohms and Marlin showed a temperature of 295C without restarting.

Our temperature related settings (this is on an Archim 2.0 board):

#define BED_MAXTEMP 150
#define TEMP_SENSOR_BED 7

#define TEMP_BED_RESIDENCY_TIME              1
#define TEMP_BED_HYSTERESIS                  5
#define TEMP_BED_WINDOW                      5

#define HEATER_MAXTEMP                     305

#define THERMAL_PROTECTION_PERIOD           15     // Seconds
#define THERMAL_PROTECTION_HYSTERESIS       30     // Degrees Celsius

#define THERMAL_PROTECTION_BED_PERIOD       15     // Seconds
#define THERMAL_PROTECTION_BED_HYSTERESIS   10     // Degrees Celsius

Steps to Reproduce

  1. Put a 78.5 ohm resistor in the thermistor port for the bed.
  2. Observe 623C showing up in the temperature
  3. Wait forever for a max temp

Expected behavior: The printer should display a MAX_TEMP error

Actual behavior: The printer shows 623C on the LCD and hums along without restarting.

shitcreek commented 5 years ago

define TEMP_SENSOR_BED 7

Are your hotends also set to 7?

marcio-ao commented 5 years ago

No, our hotend sensors are set to 5:

    #define TEMP_SENSOR_0                        5
    #define TEMP_SENSOR_BED                      7

    #define PREVENT_COLD_EXTRUSION
    #define EXTRUDE_MINTEMP                    120

    #define MAX_BED_POWER      206  // limits duty cycle to bed; 255=full current
    #define WATCH_TEMP_PERIOD   40  // Seconds
    #define WATCH_TEMP_INCREASE 10  // Degrees Celsius
marcio-ao commented 5 years ago

I don't think it matters, but more temp related settings:

#define PIDTEMP
#define PIDTEMPBED

    #define DEFAULT_Kp 21.00
    #define DEFAULT_Ki  1.78
    #define DEFAULT_Kd 61.93

  #define DEFAULT_bedKp                 286.02
  #define DEFAULT_bedKi                 54.55
  #define DEFAULT_bedKd                 374.90
shitcreek commented 5 years ago

From the config file:

5 : 100K thermistor - ATC Semitec 104GT-2/104NT-4-R025H42G (Used in ParCan & J-Head) (4.7k pullup) 7 : 100k Honeywell thermistor 135-104LAG-J01 (4.7k pullup)

If 5 works and it is the same sensor, I'd suggest setting it to 5 for the bed as well to see if that'll work.

marcio-ao commented 5 years ago

@shitcreek: Well, that's not really fixing the underlying problem. Why should the sensors be the same?

ghost commented 5 years ago

Could you say which Marlin Version/Release this is?

marcio-ao commented 5 years ago

@FanDjango: Commit 6a71df2925955934dd426f907214c0584f7de98b of Marlin 2.0

ghost commented 5 years ago

Okidoke, I'll go right away and test the same thing, mine is similarly recent. Gimme a mo...

shitcreek commented 5 years ago

Sorry, I may be confused. When you said

It works fine for the hot ends

and that the hotend sensor was set to 5, I assume you were using the type of sensor. Do you have a part number or make/model?

ghost commented 5 years ago

Same here, I confirm.

marcio-ao commented 5 years ago

@shitcreek: Our heat beds come with an integrated themistor, so it is different than the one used in our hotends. I do not know the exact part number, but we've sold tens of thousands of printers with 5 and 7 as the hotend and bed sensor numbers respectively, so I suspect it is correct :)

Sorry, I may be confused. When you said It works fine for the hot ends

The code for testing MAX_TEMP on hotends is correct, but the code for testing MAX_TEMP for beds probably isn't. I doubt it has anything to do with the exact sensor type used. If I were to swap those two numbers around, I expect the error would persist.

AnHardt commented 5 years ago

Will MAXTEMP_ERROR trigger when you start heating the bed?

ghost commented 5 years ago

No. But the PID regulation stops heating because the bed is at 250° (with my resistor in use).

marcio-ao commented 5 years ago

I would feel more comfortable if the printer panicked anytime any temperature exceeded the maximums, regardless of whether the bed was heating or not.

thinkyhead commented 5 years ago

There is no safety issue according to my reading of the code. If you were to try to heat the bed the heater wouldn't come on, because the temperature is already exceeding the target. The thermal protection feature observes the bed when you try to heat it, so it will catch the problem as soon as it sees the bed is behaving unusually - not changing temperature.

The MINTEMP/MAXTEMP code which is always enabled should catch the issue of a bizarre thermistor reading, but I believe that since the thermistor tables are clamped to the provided values the check is done at the hardware level — before the conversion into celsius. The check might be against the maximum bed temperature (plus 5).

Figure it out, if you can. I have to get some food now.

InsanityAutomation commented 5 years ago

@marcio-ao For low temp, it was disabled intentionally for pluggable hotends to not throw errors when not in use. Cant say anything about max or the bed though...

ghost commented 5 years ago

@marcio-ao So you are left with the question of why different behaviour of Nozzle Maxtemp and Bed Maxtemp, right?

marcio-ao commented 5 years ago

For low temp, it was disabled intentionally for pluggable hotends to not throw errors when not in use.

@InsanityAutomation: Yes and it was a smart change for MIN_TEMP. I like it.

So you are left with the question of why different behaviour of Nozzle Maxtemp and Bed Maxtemp, right?

@FanDjango: Yes, exactly.

There is no safety issue according to my reading of the code. If you were to try to heat the bed the heater wouldn't come on, because the temperature is already exceeding the target.

@thinkyhead: What if someone swapped the heaters around by mistake? Marlin asks to heat the extruder, but instead the bed gets really hot?

The most important thing for us is that it is testable in our production line. We are planning to short a resistor across the thermistor and expect Marlin to halt. If an immediate halt isn't implemented in Marlin upstream, I probably will just throw a check in the idle() loop for our own FW, just so we can have something that is testable without much fussing.

But, I still think this may be a useful fix in upstream, the current behavior is inconsistent with how the hot ends behave.

ManuelMcLure commented 5 years ago

Also, it makes sense that a MAXTEMP error would trigger even when not explicitly heating the bed or hotend in case a MOSFET fails "on". That would allow kill() to shut off the power supply (if configured) and add another layer of safety.

thinkyhead commented 5 years ago

With all the issues on my list, perhaps someone else would like to take point on this. I have looked through the code and see where extra tests may be added, but it sounds like you want to do a more thorough audit of all potential use-case and failure modes and revamp the temperature safety checks. I simply don't have time to complete that large a task with all the other pressing demands right now.

  #if HAS_HEATED_BED

+   if (degBed() > BED_MAXTEMP)
+     _temp_error(-1, PSTR(MSG_T_THERMAL_RUNAWAY), TEMP_ERR_PSTR(MSG_THERMAL_RUNAWAY, -1));

    #if WATCH_BED
      // Make sure temperature is increasing
shitcreek commented 5 years ago

I get you now.

I did some experimenting using different resistors and a potentiometer (since I don't have any resistor lower than 100 ohm except for 10s) using your settings and also with different temp_sensor_0 and temp_sensor_bed settings: With a potentiometer:

With resistors:

I hope this provides some clue into the issue. I will continue to investigate further.

marcio-ao commented 5 years ago

I have looked through the code and see where extra tests may be added, but it sounds like you want to do a more thorough audit of all potential use-case and failure modes and revamp the temperature safety checks.

@thinkyhead: I don't know who you were addressing with that comment, but all I was looking for was a simple check against MAX_TEMP -- exactly like the two liner you provided. I'll go ahead and add those particular lines to our own FW and if it has the desired effect I'll call it done. Thank you.

If it eventually makes it into the upstream branch, even better, as it will be one less difference between Lulzbot FW and upstream.

thinkyhead commented 5 years ago

If it eventually makes it into the upstream branch, even better

Keep us posted whether it works. (I'm unable to get dummy thermistors to work for testing this.) It's just my first quick-and-dirty off-the-cuff suggestion, and you might find better ways to integrate additional safety checks. I'll work with @shitcreek to nail down safety checking as time allows, pulling wires and breaking things intentionally to see what happens.

thinkyhead commented 5 years ago

To clarify, note that MINTEMP and MAXTEMP are specific conditions: 100% shorted or 100% disconnected. Consider this when deciding what error to throw in response to an unexpectedly high temperature reading on a connected but non-shorted thermistor. This may simply indicate the wrong thermistor ID is being used.

marcio-ao commented 5 years ago

Keep us posted whether it works. (I'm unable to get dummy thermistors to work for testing this.)

@thinkyhead: It works

I found that some of the temperature error messages were incomplete, so I fixed the macros and rolled it all into a PR https://github.com/MarlinFirmware/Marlin/pull/13756

I'll work with @shitcreek to nail down safety checking as time allows, pulling wires and breaking things intentionally to see what happens.

Any future improvements are welcome, but the BED_MAXTEMP was the only one we had an immediate concern about.

marcio-ao commented 5 years ago

Well, that's bizarre. Now the MAXTEMP isn't working for the hotends. Did anything change recently?

boelle commented 5 years ago

@marcio-ao is the issue still present?

marcio-ao commented 5 years ago

@boelle: I haven't seen this issue recently. I will close the ticket.

github-actions[bot] commented 4 years ago

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.