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.

Sign up for GitHub

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

Jump to bottom

[BUG] The PID routine does not follow pid algorith, the I term cannot be negative #27734

Closed
1 task done
feldi12 opened this issue Mar 10, 2025 · 2 comments · May be fixed by #27740
Closed
1 task done

[BUG] The PID routine does not follow pid algorith, the I term cannot be negative #27734

feldi12 opened this issue Mar 10, 2025 · 2 comments · May be fixed by #27740

Comments

@feldi12
Copy link

feldi12 commented Mar 10, 2025 •
edited
Loading

Did you test the latest bugfix-2.1.x code?

Yes, and the problem still exists.

Bug Description

The temp_iState is defined by call to constrained in:

Marlin/Marlin/src/module/temperature.h

Lines 225 to 226 in ee35929

const float max_power_over_i_gain = float(MAX_POW) / Ki - float(MIN_POW);
temp_iState = constrain(temp_iState + pid_error, 0, max_power_over_i_gain);

This call makes it impossible for the temp_iState to be negative. In theory of operation of PID controller this should be an integral over time of error, and there is no reason for it to be only positive. As it is now, this does not allow for the I term to act in opposition to d term in some cases, and in general deviates from expected behaviour

Bug Timeline

The constrain call was introduced all the way back in commit 1db7013

Expected behavior

Currently we are introducing constraints to maximum and minimum value of integral. This can be done and can help PID stabilize faster, but should not be done behind user back. Ideally, this would be controlled by parameters to be configured by user based on their use case, with separate max and min constraints. By default some negative value should be allowed.

Actual behavior

The PID constraints the integral from becoming negative.

Steps to Reproduce

N/A

Version of Marlin Firmware

2.1.2.2

Printer model

No response

Electronics

No response

LCD/Controller

No response

Other add-ons

No response

Bed Leveling

None

Your Slicer

None

Host Software

None

Don't forget to include

  • A ZIP file containing your Configuration.h and Configuration_adv.h.

Additional information & file uploads

As the PID does not follow actual PID behaviour, some PID tuning techniques are not applicable to this implementation. It also may negatively affect stability of temperature in some configurations.
The bug is not dependant on Configuration.h and Configuration_adv.h., so the files are not attached.

P.S: Because ideal solution would provide new features ( control over integral limits ) this could be also considered feature request. Please move as you see fit.
@feldi12
Copy link
Author

feldi12 commented Mar 12, 2025 •
edited
Loading

I made some improvements to the code and tested them. The graphs below show heating process for my Anycubic Mega i3 S, tha improvement is more visible when heating to 230C. The temps stabilize faster and I believe during printing it also tracks better. I feel like my prints look a bit better. I'll make a pull request for the changes in a moment and link this issue to it.

The improvements:

  • When heating up, handover to PID with maximum value of integral term - after all we are heating from way lower temp. This way at handoff we stay at max heater power and there is no kink at handover. I did not make similar change for when we cool from higher temperature, as heater power cannot go below zero anyway.
  • Increased handover distance to 20 - again to smooth things up, particularly when cooling down.
  • Allow integral term to become negative. As usually when cooling up making this too large will actually slow down stabilizing, I limit the term from integration to 1/4 of max power. I believe this helps during printing when there are sudden changes in filament flow.

Cheers!

Image
Image
Image
Image

This was referenced Mar 12, 2025
Closed
Open
@thisiskeithb thisiskeithb linked a pull request Mar 12, 2025 that will close this issue
Fixes and improvements to PID. #27740
Open
@thinkyhead
Copy link
Member

Moving discussion to #27740.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fixes and improvements to PID. feldi12/Marlin_work
2 participants
@thinkyhead @feldi12