-
-
Notifications
You must be signed in to change notification settings - Fork 19.5k
PID loop improvements #14746
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
Merged
thinkyhead
merged 8 commits into
MarlinFirmware:bugfix-2.0.x
from
mikeshub:MarlinCreatorProSKR1_3
Jul 28, 2019
Merged
PID loop improvements #14746
thinkyhead
merged 8 commits into
MarlinFirmware:bugfix-2.0.x
from
mikeshub:MarlinCreatorProSKR1_3
Jul 28, 2019
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These changes add a few improvements. The bed PID loop will settle faster with these changes. The bed autotune function will now give useable tunes instead of tunes that result in lots ringing. The output of the PID loop is much more readable because there is no reason to debug two PID loops at the same time.
For now if it won't save significant CPU or SRAM then it's fine to leave it as it is. In the future it will be nice to have all the PID heater-plus-temp-sensor units on the same footing. Another PR discusses making a class to embody PID. I'm thinking a small set of mix-in classes could work. |
Cool thanks! |
thinkyhead
added a commit
to thinkyhead/Marlin
that referenced
this pull request
Mar 12, 2025
Restore a lost element of MarlinFirmware#14746 Co-Authored-By: mikeshub <[email protected]>
Merged
thinkyhead
added a commit
to MarlinFirmware/Configurations
that referenced
this pull request
Mar 12, 2025
MarlinFirmware/Marlin#14746 MarlinFirmware/Marlin#27740 Co-Authored-By: mikeshub <[email protected]>
thinkyhead
added a commit
that referenced
this pull request
Mar 13, 2025
Followup to #14746 Co-authored-by: mikeshub <[email protected]>
EvilGremlin
pushed a commit
to EvilGremlin/Marlin
that referenced
this pull request
May 15, 2025
Followup to MarlinFirmware#14746 Co-authored-by: mikeshub <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This pull request adds a few improvements to the PID loops.
#define MIN_POWER
&#define MIN_BED_POWER
The biggest improvement on this PR is to temp changes when doing double nozzle prints. If the new temp is less than the PID functional range this won't have much impact. However, if the drop is more than the functional range this change helps significantly with the undershoot of the target. The logic behind this new define is that to heat a nozzle it needs power. Therefore the power should never be completely off when the error is within the functional range. Now the nozzle undershoots by only a couple of degrees instead of 10 - 15 in some cases.
This value is pretty easy to obtain. Enable PID debug output. Set the lowest temp a hotend or bed will be used at and wait for the PID loop to stabilize. Make note of the output. Set the define to 10 to 20 lower than the value where the loop stabilized at.
There is no reason that the functional range should be ignored by the bed PID loop. This change has led to much faster settling times as the target temp is reached without the I term being saturated. Now that they work the same would it make sense to get rid of the bed loop and have all the PID work done in one place? I can make a strong arguments both for and against on that. I am leaning towards don't combine them.
There is no reason to debug two hotend PID loops at once. Only outputting the info for the active hotend makes reading or capturing to a text or CSV for graphing far easier. I can't come up with a scenario where one would need the output of both loops simultaneously. If there is one, what is the benefit of making the vast majority of use cases worse to accommodate a random edge case that will most likely never even happen?
I have no idea why this one got rejected on one of my previous PRs. You can try yourself if you are a "seeing is believing" type, but take my word for it the "no overshoot" tunings are the only useable ones for the PID autotune on the bed. The other two result in unstable loops. The oscillations do not become unbounded, but they do not stop with the classic tunings. The classic is still the best for the hotend tunings. This change will allow the end user to use M303 and the U flag to have good tunings for their bed. Why would we not want this?
Benefits
See description.
Related Issues
Most likely none. I doubt word has gotten out that the bed PID loops are now usable as I've been making these PRs for my personal use with no fanfare.
It looks like the temp files got some love while I've been sorting out my upgrade to SKR1.3 on my Creator Pro. I'm in way too deep right now to resolve this, but can in a day or two once I have what I'm working on put to rest. If someone felt like resolving this for me I would greatly appreciate it.