kliment / Sprinter

Firmware for RepRap printers and similar devices
432 stars 329 forks source link

Fatal: Sprinter has no way of knowing if thermistor is disconnected #205

Closed tenaciousRas closed 11 years ago

tenaciousRas commented 11 years ago

Expected: if the thermistor is physically removed from the hotend (due to an accident) while printing, Sprinter should be able to recognize that even though power is being applied the temperature readings are not acting as they should because there's been a hardware failure. In other words, Sprinter has no way of avoiding or detecting this fatal scenario. Once this event occurs it is likely the hotend will be destroyed by overheating, or worse could occur to the user.

Actual: Sprinter will let you burn your hotend off the carriage, and if you're really neglectful it could become a fire hazard to you, your family, and/or possessions.

Please fix this.

tenaciousRas commented 11 years ago

or ask the community to submit a patch?

kliment commented 11 years ago

Mintemp, maxtemp and watchdog are all available in settings. The maxtemp will trigger if the thermistor is shorted, the mintemp if it's disconnected, and the watchdog if it's not reacting to the hotend as you described. Enable as desired.

Kliment

On 10/03/2012 08:10 PM, Free wrote:

Expected: if the thermistor is physically removed from the hotend (due to an accident) while printing, Sprinter should be able to recognize that even though power is being applied the temperature readings are not acting as they should because there's been a hardware failure. In other words, Sprinter has no way of avoiding or detecting this fatal scenario. Once this event occurs it is likely the hotend will be destroyed by overheating, or worse could occur to the user.

Actual: Sprinter will let you burn your hotend off the carriage, and if you're really neglectful it could become a fire hazard to you, your family, and/or possessions.

Please fix this.

— Reply to this email directly or view it on GitHub https://github.com/kliment/Sprinter/issues/205.

tenaciousRas commented 11 years ago

Errr...why did you close this bug? Let's assume the watchdog works (I don't think it does), then shouldn't it be enabled by default since this hardware failure is fatal and never desired? Is there a performance penalty if it's enabled, and if so, isn't that a problem? You're right: MAXTEMP and MINTEMP are not related to this issue report, but watchdog probably is. Which watchdog implementation are you referring to specifically and making claims that it works? Last time I checked, Marlin inherited Sprinter's watchdog implementation, and Marlin's is broken. Do I have to be the one to test whether watchdog really works, and if so, is there a safe way to perform the test?

tenaciousRas commented 11 years ago

Where is the watchdog implementation, filenames please?

kliment commented 11 years ago

I will not enable it by default because a number of users prefer it this way, and noone but yourself has wanted it on by default. If you need it, enable it. It did work when I last used it. If it doesn't work for you, please report that. The implementation is at https://github.com/kliment/Sprinter/blob/master/Sprinter/heater.cpp#L331

tenaciousRas commented 11 years ago

Are users who expressed that preference to you aware of the side-effect? Should somebody who's new to 3D printing have to figure this out for themselves? Why would I want this timer disabled by default unless it has a performance impact?

The code you cited is proven in the field? Specifically, the algorithm is to constantly check "if(watch_raw + 1 >= current_raw)"? Shouldn't the firmware keep track of at least one state variable - this one -- and if the event occurs shouldn't it stop this train? Have you tested that code on your hardware? You want me to test it, and then report that it's broken, right?

kliment commented 11 years ago

I don't actually want anything, I'm very happy with the current situation. The code has been tested in the field, but hardly anyone uses it anymore. I'm not interested in putting more attention into it, given it's a solved problem. The issue with keeping it on by default is that the default value is not suitable for very large heater block thermal mass, so it tends to mistrigger on those. I got a number of complaints about that so now it's off by default. If you need it, enable it. Please let this be now.

tenaciousRas commented 11 years ago

You just admitted the watchdog is not suitable = broken for a "very large heater block"? Thus I'll conclude the watchdog is not enabled by default because it 'breaks' a segment of the user population. It indeed looks like the implementation you seem so confident in impacts performance also, though negligibly. I've already provided very strong arguments as to why this should be a key feature that's enabled by default. It seems you haven't addressed those arguments and went on some other tangent. Some community review this has been...

edef1c commented 11 years ago

It's impossible to provide a meaningful default value - hotends vary widely in thermal mass. Enabling it by default with settings for a large thermal mass provides a false sense of security.

tenaciousRas commented 11 years ago

@nathan7, it's not impossible. With the currently proposed algorithm (in code), yes it is impossible. Thereforethe feature is broken as I keep screaming at the wall called this project's dev team.

The feature needs to be based on signal analysis. The problem to solve is not, "has the temperature risen in a given amount of time" it is "is the rate of change of temperature increasing if the hotend is on" -- and that can be roughly calibrated to the thermal mass of the hotend.

What's needed here is some kind of signal analysis. I'm looking at this on Marlin, but could care less about this project. You guys go burn your hotends off and don't tell your users about the fire risk this poses!

If you want to see how a real OSS community handles agnostic bug reports -- without emotional involvement or personal insult - refer to https://issues.apache.org/bugzilla/. Search for any myriad of bugs. Hardcore OSS dev teams don't mind if users are frustrated with broken features -- of course they are! Thanks for using our product and helping make it better! Please contribute code patches too!

A 2nd order Thiel Sen algo will probably fix this but it will take me a minute to get it to perform in O(n log n).

tenaciousRas commented 11 years ago

Not sure if that's the right way to describe that -- it needs an approximation of dT/dt. There are papers describing how to get theil-sen approx. w/O(n log n) or better constrained by integer values and fixed memory spaces. Maybe you guys know how to handle that already.

edef1c commented 11 years ago

You seem to know how to implement it. What's stopping you from doing so? Pull requests are always welcome.