klipper
klipper copied to clipboard
heater_fan: added hysteresis
I implemented some rudimentary hysteresis for heater_fan because the oscillations on my v0 got a bit annoying.
Tested on my enclosed voron v0.1 for the last couple weeks.
As far as I understand the code it should not change existing behaviorism unless enabled.
Signed-off-by: Adrian Joachim [email protected]
Thanks. As high-level feedback, I think it is fine to add hysteresis support to heater_fan. However, I'm not sure it is worthwhile to add a new config setting for it, as I can't see where anyone would not want to use hysteresis or that anyone would need to supply a custom hysteresis.
Separately, the indentation of the code change is difficult to understand. I think the code should probably be broken up into multiple if statements or otherwise clarified.
Cheers, -Kevin
Thanks. As high-level feedback, I think it is fine to add hysteresis support to heater_fan. However, I'm not sure it is worthwhile to add a new config setting for it, as I can't see where anyone would not want to use hysteresis or that anyone would need to supply a custom hysteresis.
I didn't want it to change exiting behavior in case it causes side effects. Also not entirely sure what a good fixed value would be, 5C works on my v0 but that's a sample size of 1.
Separately, the indentation of the code change is difficult to understand. I think the code should probably be broken up into multiple if statements or otherwise clarified.
Tell me about it, the 80 character limit kicked my ass there, looked a lot more readable before. Breaking it down into 2 if statements would yield a similarly ugly squished thing but I can do it if you prefer it.
I would have thought a hysteresis of 1 degree would be fine. It's just to avoid noise on the thermistor right?
-Kevin
Thermistor noise definitely is part of it but in my case it looks like where is some residual heat in other parts of the block that leak into the thermistor after the fan stops, I do get a slow rise of 2-3 degrees over a few minutes after the fan turns off.
Okay, thanks. That is interesting, and I've not seen that report before. All of my extruders will cool at a lower rate when the fan is turned off, but do not go up in temperature.
-Kevin
I admit that part is probably a bit of an edge case but is there any harm in having it configurable?
The worst thing a user could mess up with the configuration (assuming I understand my code right) would just have the fan not turn off which isn't a huge safety concern imo.
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.
I would still like to get this merged, can this be reopened or do I have to make anew request?
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.
@KevinOConnor Sorry to bother you but what do you want me to change to get this merged?
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.
@KevinOConnor I still would like to get this merged, please let me know what I can do there.
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.
Thanks. Alas, I haven't been able to get time to look at this further.
One high-level comment I have is that I wonder if a more generic fan speed system may be more appropriate for your (a bit uncommon) case. In previous PRs I've floated the idea of extending the "LED template" system to work with fans and PWM pins. (That system allows one to configure a "formula" based on exported "printer" settings - the result of the "formula" is automatically recalculated and applied.) That may be a more generic way of controlling a fan.
I'll be travelling for a couple of weeks, but will hopefully be able to look further at this after I return.
Cheers, -Kevin
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.
@KevinOConnor Can we keep this open until the alternative implementation becomes a thing?
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.
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.
@KevinOConnor Once again, can we keep this open until the alternative is merged/you tell me what to change to get it merged?
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.
@KevinOConnor keep open please
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.
@KevinOConnor keep open please
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.
@KevinOConnor keep open please