klipper icon indicating copy to clipboard operation
klipper copied to clipboard

heater_fan: added hysteresis

Open chestwood96 opened this issue 1 year ago • 25 comments

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]

chestwood96 avatar May 19 '23 23:05 chestwood96

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

KevinOConnor avatar May 25 '23 16:05 KevinOConnor

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.

chestwood96 avatar May 25 '23 19:05 chestwood96

I would have thought a hysteresis of 1 degree would be fine. It's just to avoid noise on the thermistor right?

-Kevin

KevinOConnor avatar Jun 01 '23 20:06 KevinOConnor

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.

chestwood96 avatar Jun 02 '23 20:06 chestwood96

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

KevinOConnor avatar Jun 08 '23 00:06 KevinOConnor

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.

chestwood96 avatar Jun 08 '23 15:06 chestwood96

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.

github-actions[bot] avatar Jun 30 '23 00:06 github-actions[bot]

I would still like to get this merged, can this be reopened or do I have to make anew request?

chestwood96 avatar Jun 30 '23 12:06 chestwood96

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.

github-actions[bot] avatar Jul 15 '23 00:07 github-actions[bot]

@KevinOConnor Sorry to bother you but what do you want me to change to get this merged?

chestwood96 avatar Jul 15 '23 00:07 chestwood96

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.

github-actions[bot] avatar Jul 29 '23 12:07 github-actions[bot]

@KevinOConnor I still would like to get this merged, please let me know what I can do there.

chestwood96 avatar Jul 30 '23 19:07 chestwood96

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.

github-actions[bot] avatar Aug 16 '23 00:08 github-actions[bot]

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

KevinOConnor avatar Aug 26 '23 16:08 KevinOConnor

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.

github-actions[bot] avatar Sep 10 '23 00:09 github-actions[bot]

@KevinOConnor Can we keep this open until the alternative implementation becomes a thing?

chestwood96 avatar Sep 10 '23 17:09 chestwood96

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.

github-actions[bot] avatar Sep 25 '23 00:09 github-actions[bot]

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.

github-actions[bot] avatar Oct 09 '23 12:10 github-actions[bot]

@KevinOConnor Once again, can we keep this open until the alternative is merged/you tell me what to change to get it merged?

chestwood96 avatar Oct 12 '23 16:10 chestwood96

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.

github-actions[bot] avatar Oct 27 '23 00:10 github-actions[bot]

@KevinOConnor keep open please

chestwood96 avatar Oct 27 '23 22:10 chestwood96

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.

github-actions[bot] avatar Nov 14 '23 00:11 github-actions[bot]

@KevinOConnor keep open please

chestwood96 avatar Nov 14 '23 11:11 chestwood96

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.

github-actions[bot] avatar Dec 01 '23 12:12 github-actions[bot]

@KevinOConnor keep open please

chestwood96 avatar Dec 04 '23 07:12 chestwood96