AlexVerrico / Octoprint-ThermalRunaway

Octoprint plugin to provide some basic thermal runaway protection
GNU Affero General Public License v3.0
5 stars 1 forks source link

[BUG] False thermal runaways detected/M112 sent for no reason after updating to V3.0.x #27

Open AlexVerrico opened 3 years ago

AlexVerrico commented 3 years ago

This is an issue that is caused by a redesign of the logic used for checking if a thermal runaway has occurred. Version 3.0.2 adds a config option to the plugin to revert to pre-V3 logic. By default the plugin still uses V3 logic. You will need to manually uncheck this option in settings: image

mikeperalta1 commented 3 years ago

+1 I believe this is happening to mine as well

2021-06-11 21:56:48,313 - octoprint.plugins.ThermalRunaway - WARNING - thermalHighCount == 1 for T0
2021-06-11 21:56:50,313 - octoprint.plugins.ThermalRunaway - WARNING - thermalHighCount == 1 for T0
2021-06-11 21:56:52,314 - octoprint.plugins.ThermalRunaway - WARNING - thermalHighCount == 1 for T0
2021-06-11 21:56:54,310 - octoprint.plugins.ThermalRunaway - WARNING - thermalHighCount > 2 for T0
2021-06-11 21:56:54,317 - octoprint.util.comm - INFO - Force-sending M112 to the printer
2021-06-11 21:56:54,326 - octoprint.util.comm - INFO - Changing monitoring state from "Operational" to "Offline after error"
2021-06-11 21:56:54,330 - octoprint.plugins.ThermalRunaway - CRITICAL - Thermal Runaway (over temp) caught on heater T0. Reported temp is 30.99, set temp is 0.0 
2021-06-11 21:56:54,340 - octoprint.plugins.action_command_notification - INFO - Notifications cleared

You can see it calls kill() because I have the temperature set to 0, while the reported temperature is close to ambient.

mikeperalta1 commented 3 years ago

Just noticed the part about unchecking the "pre-v3" logic box. That works for me also. Perhaps when thermal runaway is detected, a notification message box could inform the user that if they suspect a false positive, they may wish to check the settings?

BobVul commented 3 years ago

I'm not sure the current (V3+) logic as implemented works in any situation?

current < min, a given while heating:

https://github.com/AlexVerrico/Octoprint-ThermalRunaway/blob/9cf5230948a47c2624cf32fe09629a6a3217fe52/octoprint_ThermalRunaway/__init__.py#L190

low = current:

https://github.com/AlexVerrico/Octoprint-ThermalRunaway/blob/9cf5230948a47c2624cf32fe09629a6a3217fe52/octoprint_ThermalRunaway/__init__.py#L192

thermalLowCount = 1:

https://github.com/AlexVerrico/Octoprint-ThermalRunaway/blob/9cf5230948a47c2624cf32fe09629a6a3217fe52/octoprint_ThermalRunaway/__init__.py#L196


Later, in the same loop iteration (!!):

thermalLowCount == 1, because it was just set earlier in the same iteration

https://github.com/AlexVerrico/Octoprint-ThermalRunaway/blob/9cf5230948a47c2624cf32fe09629a6a3217fe52/octoprint_ThermalRunaway/__init__.py#L294

Then the main condition is current <= low:

https://github.com/AlexVerrico/Octoprint-ThermalRunaway/blob/9cf5230948a47c2624cf32fe09629a6a3217fe52/octoprint_ThermalRunaway/__init__.py#L300

But earlier, in the same iteration, low = current, so of course now current <= low / current == low.

And we have a false detection of a thermal runaway.

The same applies for high.


Looks like there are other questionable situations, like when the test at 2 (https://github.com/AlexVerrico/Octoprint-ThermalRunaway/blob/9cf5230948a47c2624cf32fe09629a6a3217fe52/octoprint_ThermalRunaway/__init__.py#L267) pases and it gets set back to 1, also setting low = current ... only to run into the test at 1 further below.


Actually, thinking about it some more, I'm beginning to question whether thermal runaway detection works in pre-V3 mode either?

So at the top we have low = current

https://github.com/AlexVerrico/Octoprint-ThermalRunaway/blob/9cf5230948a47c2624cf32fe09629a6a3217fe52/octoprint_ThermalRunaway/__init__.py#L192

Wouldn't all tests below this point always pass, since current < low is always false, since it gets set earlier in the same iteration?

BobVul commented 3 years ago

Hmm, looking at it again, there does need to be a place where initial low/high are set when we're out of the safe boundary.

Maybe moving that set to inside the if int(self.heaterDict[heater]['thermalLowCount']) == 0: block would work? That say it's only set once, when it transitions out of safe, rather than being set at the beginning of every iteration.