heater_fan: Add Heater Fan Self Check Logic

cneshi commented 8 months ago

Add a simple self-check functionality to heater_fans with tachometer signal.

Why do we want this?

This functionality is especially useful for hotend fans, which when they fail can result in some quite catastophic consequences for the hotend and surrounding parts.

Configuration

The user simply adds configurations related to fan tachometer (tachometer_pin, tachometer_ppr) and one additional configuration: min_rpm, which specifies the minimum expected RPM of the fan when it is enabled. When fan speed is detected to be below the minimum RPM. then klipper shuts down with an error. By default, min_rpm is 0 and thus the self-test functionality is not enabled.

Code description

Initialization, the heater_fan module checks for min_rpm config, and sets it to the specified value. A config error is thrown if tachometer configuration is not specified by the user but min_rpm is.
Event handler, a fan_check() event handler is created if min_rpm is enabled. This event handler is first called 3 sec after startup and every 1.5 sec afterwards.
Self-check logic, the main logic for fan self-check is implemented in fan_check() which is called periodically. Each time fan_check is called the current fan speed and it's related heater temperature is read. If heater temp is above heater_temp and current fan speed is lower than min_rpm, the error counter is incremented; otherwise the error counter is reset to zero. An error is thrown if the error counter reaches 4 and klipper is shutdown.

rogerlz commented 8 months ago

That's a very nice feature, but I'd love to be able to decide the fail behaviour, e.g. call a gcode, or shut down the heater + pause the print.

github-actions[bot] commented 8 months ago

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:

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.
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.
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.

camerony commented 8 months ago

Is the submission free of defects and is it ready to be widely deployed? The code has been tested and verified to work by multiple testers and LDO engineers

Regression: whitespace check : PASSED data dictionaries (klipper-dict-20230921.tar.gz): PASSED

bennierex commented 8 months ago

Would be nice to be able to specify the threshold for the error counter to cause a shutdown. With the fixed value of 4, it will take at least 6 seconds to shutdown after the fan fails.

You could also check if the previous x readings are below min_rpm. That way it will not trigger a shutdown on 4 misreadings on the tach pin during a long period of time.

cneshi commented 7 months ago

Would be nice to be able to specify the threshold for the error counter to cause a shutdown. With the fixed value of 4, it will take at least 6 seconds to shutdown after the fan fails.

You could also check if the previous x readings are below min_rpm. That way it will not trigger a shutdown on 4 misreadings on the tach pin during a long period of time.

I opted to make it a fixed value of 4 because I feel like that is a good general value for most situations. If we let the user specify the value, that is another config option they need to take time to read about, understand, and configure. Anyways that is just my opinion

The logic is already such that it must be 4 consecutive below min_rpm readings to trigger the shutdown - so misreadings do not accumulate over time.

Rudis1261 commented 7 months ago

Awesome stuff!

github-actions[bot] commented 7 months ago

Unfortunately a reviewer has not assigned themselves to this GitHub Pull Request and it is therefore being closed. 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.

Rudis1261 commented 7 months ago

@KevinOConnor Sorry for the direct ping, who are reviewers on here who are able to vet the change?

wuyoutech commented 6 months ago

I did almost the same thing, but I chose to modify fan.py instead of heaterfan.py so that this feature can cover all fans.

My friend's printer lost heater_fan while printing, and large pieces of material spilled on to the bed, The toolhead eventually collided with them and alomst destory everything.

I'm glad someone did the samething, I will commit my code for reference as soon as possible.

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

KevinOConnor commented 6 months ago

Thanks. It's fine with me if we want to check for tachometer errors. It does seem odd to me that we would add this code only to heater_fan fans though. It would seem that if we were to implement this code it would be preferable to implement it for all fans.

There is a PR #6429 that does something similar.

-Kevin

KevinOConnor commented 5 months ago

Thanks. As high-level feedback I think it is fine to raise an error on a malfunctioning fan. However, it seems odd to me that this check is only made available to heater_fan instances instead of on all fans (as is done in #6429). Also, the code here does not look correct to me as it seems like it would have the same issue described at https://github.com/Klipper3d/klipper/pull/6429#issuecomment-1905056206 .

-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.

Klipper3d / klipper