Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

temperature_fan: Improved code quality of fan curve and fixed hysteresis #466

Merged
merged 1 commit into from
Dec 23, 2024

Conversation

NokkOnEffect
Copy link
Contributor

@NokkOnEffect NokkOnEffect commented Dec 6, 2024

This pull request contains improvements to the code quality of the ControlCurve class to improve readability. Furthermore unnecessary operations were removed.
The current behaviour of the hysteresis when using a fan curve for control is incorrect and causes intermittent jumps between fixed pwm values without following the curve or reaching either end of the curve. This pull request contains an improved implementation.

Please be aware that this requests testing is limited as I'm currently not able to validate the behaviour in a live environment! I am open to feedback.

Checklist

  • pr title makes sense
  • squashed to 1 commit
  • added a test case if possible
  • if new feature, added to the readme
  • ci is happy and green

@NokkOnEffect NokkOnEffect requested a review from a team as a code owner December 6, 2024 13:14
@NokkOnEffect NokkOnEffect force-pushed the main branch 5 times, most recently from f67ca92 to cc34fa5 Compare December 6, 2024 20:35
@NokkOnEffect
Copy link
Contributor Author

NokkOnEffect commented Dec 6, 2024

I was able to test it now and as far as I can tell everything works fine.

Currently, a larger hysteresis causes larger jumps of the fan speed when heating up or cooling down as the implemented behaviour for when the temperature and fan speed are between the upper and lower curve is to keep the speed.
This behaviour is easily modifyable, such that for example the temperature controller could hug the upper or lower curve depending on whether the sensor is heating up or cooling down.

Copy link
Contributor

@rogerlz rogerlz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the tests havent been adjusted to reflect the removal of the parameter
either deprecate the parameter or add a line to config_changes.md

klippy/extras/temperature_fan.py Show resolved Hide resolved
klippy/extras/temperature_fan.py Outdated Show resolved Hide resolved
@NokkOnEffect NokkOnEffect requested a review from rogerlz December 11, 2024 19:12
@NokkOnEffect NokkOnEffect force-pushed the main branch 2 times, most recently from 8e92503 to 367cd85 Compare December 11, 2024 20:28
fixed errors and extended curve

Fixed hysteresis, extended curve and improved code quality

fix incorrect curve extension

removed controlled_fan, deprecated setting

update docs

removed deprecated param from tests

make ruff happy
@rogerlz rogerlz changed the title Temperature_fan: Improved code quality of fan curve and fixed hysteresis temperature_fan: Improved code quality of fan curve and fixed hysteresis Dec 23, 2024
@rogerlz rogerlz merged commit 20d9035 into KalicoCrew:main Dec 23, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants