Klipper3d / klipper

Klipper is a 3d-printer firmware
GNU General Public License v3.0
8.99k stars 5.18k forks source link

feature: shutdown klipper when fan failure #6429

Closed wuyoutech closed 4 months ago

wuyoutech commented 6 months ago

Shutdown Klipper when tachometer cannot receive the speed signal

Tachometer only reads the fan speed (maybe displayed in some places) for now, Monitoring fan speed and turning off printer when fan fails helps avoid hardware damage and additional looses

code has been tested on my printer and everything works fine.

related pr: https://github.com/Klipper3d/klipper/pull/6364

PS: English is not my native language, please ignore any grammar issues, and I would be very pleased if you point out my issue.

Signed-off-by: Dayong Wang wuyoutech@gmail.com

trofen commented 6 months ago

I do not agree that after this PR, the absence of an rpm signal will lead to a forced shutdown of the klipper, and the user will not be able to configure this behavior. Despite the fact that the function seems useful in general, I believe that the user has the right to describe the behavior of the klipper when the "low fan speed" event occurs and run a custom macro similar to the one implemented in the filament runout sensor.

wuyoutech commented 6 months ago

I do not agree that after this PR, the absence of an rpm signal will lead to a forced shutdown of the klipper, and the user will not be able to configure this behavior. Despite the fact that the function seems useful in general, I believe that the user has the right to describe the behavior of the klipper when the "low fan speed" event occurs and run a custom macro similar to the one implemented in the filament runout sensor.

I think "the absence of an rpm signal" should be very rear, fans normally work for months to years before death, this is a significant difference from "filament runout event". I referred to verify_heaters.py when coding, because I believe they have a similar logic: some hardware failures that cannot be ignore. So I decide to terminate everything.

Consider the cost and benefits of configuring and testing custom macros, especially when the probability of occurrence is very low, I decided to keep everything simple. I am still hesitating whether to add a configuration to allow users to disable rpm check when enabling tachometer. But I think using custom macros is completely a bad idea, printer.cfg is already to long

JanB97 commented 6 months ago

I am right now in the middle of configuring a new hotend and parts fan. I agree that there absolutely has to be a option to not do anything if the rpm is low - otherwise printing with abs would kill the printer if it's used for parts fan like on mine. @wuyoutech one of my fans has a "locked rotor sensor" not a tachometer, so it's just yes or no. Would it be possible to implement that as well with this change?

Sineos commented 6 months ago

I agree with @wuyoutech. If I configure a PWM fan AND no signal is received ALTHOUGH a fan speed is requested THEN error out hard (if I read the code correctly, it is implemented like this). Just to look at some RPM figures in the web-interface, I do not need an RPM fan, but they make sense to secure against hardware failure and then there is no "maybe" but a clear error condition.

JanB97 commented 6 months ago

I agree with @wuyoutech. If I configure a PWM fan AND no signal is received ALTHOUGH a fan speed is requested THEN error out hard (if I read the code correctly, it is implemented like this). Just to look at some RPM figures in the web-interface, I do not need an RPM fan, but they make sense to secure against hardware failure and then there is no "maybe" but a clear error condition.

You are right, I didn't realize that. I still don't agree with there being no option but I feel it's much less critical.

KevinOConnor commented 6 months ago

Thanks. It's fine with me if we want to check for tachometer errors. I noticed a couple of things in this PR, however:

-Kevin

wuyoutech commented 6 months ago

sorry for my mistake..

Raising an error would be a change in behavior, and it would therefore require a change to docs/Config_Changes.md

Now it has been added to PR, I did miss this part.

This PR does not look correct to me because it does not take into account timing of fan speed changes. It is possible that a fan update could be scheduled several seconds in the future, but if I understand the code correctly, this PR would start checking for zero tachometer reports immediately. That could cause incorrect shutdown errors.

The code contains a counter that records the time when request speed > 0 and tachometer = 0. shutdown will be triggered only when tachometer speed is 0 for 3 consecutive seconds.

cneshi commented 5 months ago

Since this is similar to the PR I posted, I propose we modify this one slightly and close mine. I think we could add a integer min_rpm option or a simple boolean check_tacho option to the config, and enable the rpm checks only if it is config option is present. This feature will then be disabled by default and can be enabled (individually for each fan) by those users who want to use it.

camerony commented 5 months ago

sorry for my mistake..

Raising an error would be a change in behavior, and it would therefore require a change to docs/Config_Changes.md

Now it has been added to PR, I did miss this part.

This PR does not look correct to me because it does not take into account timing of fan speed changes. It is possible that a fan update could be scheduled several seconds in the future, but if I understand the code correctly, this PR would start checking for zero tachometer reports immediately. That could cause incorrect shutdown errors.

The code contains a counter that records the time when request speed > 0 and tachometer = 0. shutdown will be triggered only when tachometer speed is 0 for 3 consecutive seconds.

The main issue with not including a min_rpm or perhaps the rpm at 100% is that fans slow down before failing completely from my personal experience. This would still cause major issues with the hotend or other components before triggering a shutdown.

I would also consider added a calculation of the RPM based on the % fan, to determine expected rpm during a print.

Sineos commented 5 months ago

The main issue with not including a min_rpm or perhaps the rpm at 100% is that fans slow down before failing completely from my personal experience. [...]

I would also consider added a calculation of the RPM based on the % fan, to determine expected rpm during a print.

IMO, this cannot realistically be done by the average user and would only make it overly complicated. There are too many variables to take into account like

I have yet to see a fan loosing performance except when the bearing is dying, and this is typically accompanied by ugly noises.

kyleisah commented 5 months ago

I'm seeing a lot of conflicting arguments here for what is best to do in the event of tachometer/fan failure, and just wanted to share some thoughts/ideas about how to best integrate fan failure events with the goal of keeping everyone happy. This may come off as rambling but maybe some ideas here will spark some interest or ideas.

I will begin by saying this: I think not providing the option to execute gcode in the event of a fan failure is a mistake, but I can understand the importance of leaving something related to possible health and safety out of the hands of the general/new user. I would not like my printer to just shutdown in the event a fan fails or tachometer readings dip below a threshold. I'm good enough at making macros that I could make the machine do exactly what I believe is necessary to properly and safely handle an event like this without just shutting down without warning.

I think tach failure handling should be a requirement on [heater_fan]s with tach enabled, and the option provided for other types of fans that support tach.

Here's what I'm proposing:

[heater_fan Hotend_Fan]
.
.
.
#tachometer_pin:
#tachometer_ppr:
#tachometer_poll_interval:
.
#tachometer_safety_enable: # Can be true or false to enable action on failure (Would only be optional on fans that are not heater_fans)
#tachometer_safety_rpm_threshold: # The minimum rpm the fan can be before safety_delay_time begins monitoring
#tachometer_safety_delay_time: # Time in seconds that tach must be below tach_safety_rpm_threshold to trigger an action
#tachometer_safety_gcode: # A list of gcode commands to execute in the event of tach failure. If nothing is configured, klipper will shut down after delay_time is met. (Perhaps the docs can mention that having M112 at the end of the gcode is a good idea?)

I think these configurations, although lengthy, would provide the most overall benefit to the broadest audience.

Feel free to pick this apart. Maybe I am missing something, or someone here has an even better idea.

Thank you all for your time. 😄

KevinOConnor commented 5 months ago

The code contains a counter that records the time when request speed > 0 and tachometer = 0. shutdown will be triggered only when tachometer speed is 0 for 3 consecutive seconds.

I don't think that will work correctly. Consider the case where the fan is off, a user issues a 10 second long move followed by fan enable (eg, G1 X100 F600 and G1 M106). For the 10 seconds during the move the fan code proposed in this PR will think the fan is on, but that command hasn't yet been sent to the mcu. That is, note that Klipper schedules events in advance.

-Kevin

github-actions[bot] commented 4 months ago

It looks like this GitHub Pull Request has become inactive. If there are any further updates, you can add a comment here or open a new ticket.

Best regards, ~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

Rudis1261 commented 3 months ago

@Sineos It's happened to me on multiple occasions where the rapid movement of my printer broke wires in the loom and other components are for the most part covered already. For example the hotend itself not heating at an expected rate will trigger klipper to fail, and typically you can find the issue relatively fast.

But the more subtle hotend fan wires breaking is painful. The hotend starts to heat creep until it cannot overcome the force anymore and you are SOL. The correct solution is to use wires that are motion-rated, but even then it can still fail before expected, and this can have catastrophic results. At the minimum, it's a pain in the ass to disassemble, or waste power time, and plastic. Worst case can lead to damage to the printer. I am really keen on this feature, especially because my printers are in the garage.

I would like for it to rather kill halt the print / pause the print than having to spend and hour or two trying to fix my seized up hotend.

Rudis1261 commented 3 months ago

The code contains a counter that records the time when request speed > 0 and tachometer = 0. shutdown will be triggered only when tachometer speed is 0 for 3 consecutive seconds.

I don't think that will work correctly. Consider the case where the fan is off, a user issues a 10 second long move followed by fan enable (eg, G1 X100 F600 and G1 M106). For the 10 seconds during the move the fan code proposed in this PR will think the fan is on, but that command hasn't yet been sent to the mcu. That is, note that Klipper schedules events in advance.

-Kevin

I am very much considering running a forked version of this MR for my printers, however you raise a valid concern by the looks of it. Given my main purpose would be for my hotend fan, this wouldn't be that big a consideration/concern I think. How would you go about making this work for all scenarios though?

KevinOConnor commented 3 months ago

How would you go about making this work for all scenarios though?

I'd guess the code could store the schedule time of the last transition from fan off to fan enable, and then only shutdown if the fan is both not spinning and the time is greater than the last fan enable time. I'm not fully sure though.

-Kevin

Rudis1261 commented 3 months ago

How would you go about making this work for all scenarios though?

I'd guess the code could store the schedule time of the last transition from fan off to fan enable, and then only shutdown if the fan is both not spinning and the time is greater than the last fan enable time. I'm not fully sure though.

-Kevin

Thanks for the insight 🙏🏻

Clearly I don't know enough about how klipper works and python 🤣 I will have to try that if it gets to it. For now this works ok for the heater fan