fan: shutdown printer when a fan fails

[onnlucky]

This PR shuts down the printer with a message when a fan with a tachometer doesn't read a fan speed when it should.

Basically, if we are at least ten seconds after the fan was last scheduled to turn on, and the rpm is zero, then it reports an error.

[Sineos]

Thank you for submitting a PR, please be aware that you need to sign off to accept the developer certificate of origin, please see point 3 in master/docs/CONTRIBUTING.md#what-to-expect-in-a-review

In addition you might want to cross-check the discussion here: https://github.com/Klipper3d/klipper/pull/6429

[KevinOConnor]

Thanks. We can certainly add a shutdown safety check for tachometers.

A couple of notes on the implementation:

• It's not valid to perform a safety check from the get_status() method as nothing assures that method is called regularly. It gets called upon request from a GUI (the API server) or from a macro. So, if those sources don't request a fan status then the safety check wouldn't be run.
• The fan speed requests are queued well in advance of the reception of tachometer reports. It isn't immediately clear to me that this code handles that correctly (for example, will it incorrectly shutdown if a fan enable is queued far in the future, or will it correctly shutdown if there are a constant stream of fan speed changes being queued). Can you briefly explain how the code is handling these types of corner cases?
• I think it would be preferable for the safety checking code to be in the FanTachometer class and not the Fan class. (To get access to the requested fan speed, the Fan class can call a new self.tachometer.note_fan_speed() on speed changes.)
• For what it is worth, the tachometer_fault_action config options seems a little unwieldy to me. Perhaps implement a tachometer_minimum_rpm option instead (and a setting of zero would disable shutdowns)?
• I agree it would be useful to disable shutdowns for fans. For what it is worth, I don't have a preference if the default is to shutdown or the default is to not shutdown. If it is felt the default should be to shutdown then Config_Changes.md should state that, and if the default is not changing then we would not add a new entry to Config_Changes.md.

Cheers, -Kevin

[onnlucky]

• It's not valid to perform a safety check from the get_status() method as nothing assures that method is called regularly. It gets called upon request from a GUI (the API server) or from a macro. So, if those sources don't request a fan status then the safety check wouldn't be run.

Makes sense.

• The fan speed requests are queued well in advance of the reception of tachometer reports. It isn't immediately clear to me that this code handles that correctly (for example, will it incorrectly shutdown if a fan enable is queued far in the future, or will it correctly shutdown if there are a constant stream of fan speed changes being queued). Can you briefly explain how the code is handling these types of corner cases?

It tracks the scheduled print time. And when verifying, it also uses print times. In this last version, it actually uses the mcu's counter_state update event time (in print time). So unless the MCU is running behind its own schedule, this should work. (Right?)

It now only tracks when the fan first turns on. And allows for 10 seconds of grace time for the fan to start.

There are corner cases this wont catch, indeed. Like the moment the speed is scheduled to become 0, there will be no more checking until after that time. And if the fan is turning on and off in less the grace period, it will never check. We can probably make that grace period a lot smaller, I haven't experimented with that too much yet.

With a lot more code, we could make a queue of times the fan should be on or off, and when a counter events come in, check where we should be in that queue. But that is probably overkill. And will still not be able to catch error if fans are turned on for less than the grace period.

• I think it would be preferable for the safety checking code to be in the FanTachometer class and not the Fan class. (To get access to the requested fan speed, the Fan class can call a new self.tachometer.note_fan_speed() on speed changes.)

It's now callbacks from the mcu counter state event. Which is setup to happen once per second.

• For what it is worth, the tachometer_fault_action config options seems a little unwieldy to me. Perhaps implement a tachometer_minimum_rpm option instead (and a setting of zero would disable shutdowns)?

The relationship between speed and rpm is not particularly well defined. On the other hand, perhaps fans can fail, while still running, but very slowly? I'll try this in a next update.

• I agree it would be useful to disable shutdowns for fans. For what it is worth, I don't have a preference if the default is to shutdown or the default is to not shutdown. If it is felt the default should be to shutdown then Config_Changes.md should state that, and if the default is not changing then we would not add a new entry to Config_Changes.md.

👍

Cheers, -Kevin

Thanks for the feedback.

[onnlucky]

Apologies for not moving faster, I did not have a lot of time recently.

I've now made a queue for the checks. I've tested it decently well on my machine. And I've reworked the setting to be tachometer_min_rpm. Defaults to 1, which will just check the fan is at least running when it should. Can be set to something close to the maximum rpm of the fan. The code checks rpm >= speed * min_rpm.

I've hardcoded a 2.5 seconds period for the fan and measurements to catch up. Shorter also works for me. But maybe some fans are slower to get up to speed. But making this too long means we wont be checking when the fan switches speeds a lot.

[yell3D]

why a forced shutdown and not a fanfailure_gcode: thats defaults to shutdown if empty? iirc, pause print, turnoff heaters, send telegram alert.

Imho defaulting to 1 instead of 0 is no good, features that kill your print should always be opt-in.

[onnlucky]

why a forced shutdown and not a fanfailure_gcode: thats defaults to shutdown if empty? iirc, pause print, turnoff heaters, send telegram alert.

Mostly because I don't know that much about klipper. And it sounds like a good idea. If this is more in line with how klipper would normally work, I'll give that a try.

Imho defaulting to 1 instead of 0 is no good, features that kill your print should always be opt-in.

On my machine, I have two fans with tachometers, the hotend fan, and the part cooling fan. Should either fail, the print will likely fail or at least be of low quality. Worse case, my hotend will be encased in a blob. That is why these fans have tachometers. And stopping the print on failure is a good idea.

(It is a Sovol sv06ace. And sovol's klipper fan failure implementation is horrible, prone to false positives.)

[yell3D]

i'm not affiliated with Klipper project in anyway, just curios why. I never leave my printer unattended for a longer duration so if a fail occurs my macros pause the print so i can fix the issue (or disable if it keeps throwing false positive). Not a huge fan of halt_and_catch_fire.

If you get a deathblob while the printer is paused with deactivate heater you are also getting it when the printer was shutdown. Moving to a pause position is also likely more desirable mid print since you will ooze anyways.

I would argue an active chamber vent would reduce the blob issue so having control over the printer is likely better.

[github-actions[bot]]

Thank you for your contribution to Klipper. Unfortunately, a reviewer has not assigned themselves to this GitHub Pull Request. All Pull Requests are reviewed before merging, and a reviewer will need to volunteer. Further information is available at: https://www.klipper3d.org/CONTRIBUTING.html

There are some steps that you can take now:

1. Perform a self-review of your Pull Request by following the steps at: https://www.klipper3d.org/CONTRIBUTING.html#what-to-expect-in-a-review If you have completed a self-review, be sure to state the results of that self-review explicitly in the Pull Request comments. A reviewer is more likely to participate if the bulk of a review has already been completed.
2. Consider opening a topic on the Klipper Discourse server to discuss this work. The Discourse server is a good place to discuss development ideas and to engage users interested in testing. Reviewers are more likely to prioritize Pull Requests with an active community of users.
3. Consider helping out reviewers by reviewing other Klipper Pull Requests. Taking the time to perform a careful and detailed review of others work is appreciated. Regular contributors are more likely to prioritize the contributions of other regular contributors.

Unfortunately, if a reviewer does not assign themselves to this GitHub Pull Request then it will be automatically closed. If this happens, then it is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards, ~ Your friendly GitIssueBot

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