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

pid improvements #5955

Closed
wants to merge 24 commits into from
Closed

pid improvements #5955

wants to merge 24 commits into from

Conversation

dans98
Copy link

@dans98 dans98 commented Dec 24, 2022

This PR is related to improving PID control within the constraints Klipper
currently operates under. I've made changes to the calibration algorithm as
well standard positional form algorithm. Additionally I've added a standard
velocity form algorithm, that eliminates to potential for integral windup.

Please see one of the 2 commit messages for greater details, test results, and
links to relevant papers.

-Dan S.

I updated the pid calibration algorithm based on the processes outlined
in this paper. https://www.osti.gov/biblio/1646635

The update minimizes two undesirable phenomena, bias and asymmetry.
Tests of my own and others can be seen in this klipper discord thread
https://klipper.discourse.group/t/experimental-pid-improvement-changes/3604

I also updated the code to log Cohen-Coon calibration constants, because
They can be calculated with the data on hand and work better for high
thermal mass beds found in a lot of higher end printers.

Signed-off-by: Daniel Sherman <[email protected]>
I made some minor updates to the pid algorithm to improve it's accuracy.
1) I switched the integration step over to using the Trapezoidal rule
2) I replaced the integral cap windup prevention algorithm, with a
   a conditional algorithm, as Astrom & Hagglund considers it the best of
   the simple methods.
3) I corrected an issue with the smoothing algorithem related to very
   short smoothing periods.
4) I added some additional logic to prevent integral wind up when the heater
   is tuned for low temperatures and setting idle.

I also added a new velocity pid control algorithm. Velocity control
eliminates windup, but requires cleaner sensor readings.

Tests of my own and others, as well as links to papers can be seen in this
klipper discord thread.
https://klipper.discourse.group/t/experimental-pid-improvement-changes/3604

Signed-off-by: Daniel Sherman <[email protected]>
@dans98 dans98 changed the title Final pid improvements pid improvements Dec 24, 2022
@Sineos
Copy link
Collaborator

Sineos commented Dec 25, 2022

I'm using both PID functions since quite a while and have not noticed anything negative. Working stable and nicely.

What I find especially useful is the asymmetry reading, providing an indication between heating capacity and heat loss.
This lead to the conclusion that on my two printers the 50W cartridge in combination with the given hotend was too weak. Upgrading both to 70W fixed the asymmetry and are both controlled nicely by the algorithms.

Should @KevinOConnor consider this PR (I hope so), then this should be explained in the documentation.

I wish all of you a happy holiday season!

@The-Futur1st
Copy link

I have used that for couple of days, works great, provides much better values than it was. 'pid_v' provides an ability to get rid of first big overshoot. My biggest thanks to author.

@kejar31
Copy link

kejar31 commented Dec 27, 2022

Tried this today.. works well

@celtare21
Copy link

celtare21 commented Dec 28, 2022

klippy (7).log
Getting this when trying to use pid_v:
File "/home/pi/klipper/klippy/extras/heaters.py", line 281, in check_busy or abs(self.prev_der) > PID_SETTLE_SLOPE) AttributeError: ControlVelocityPID instance has no attribute 'prev_der'

@dans98
Copy link
Author

dans98 commented Dec 28, 2022

@celtare21 I see it, and i will fix that tonight. it looks like you are the first person to test this that uses m109/m104, as that looks to be what triggered it.

@celtare21
Copy link

celtare21 commented Dec 28, 2022

@celtare21 I see it, and i will fix that tonight. it looks like you are the first person to test this thats uses m109/m104, as that looks to be what triggered it.

Yeah I still use the slicer start/end gcode. Waiting for the fix then i'll try the velocity algorithm too, for now using the improved pid algo temps look both more stable and the overshoot is much lower than default

@dans98
Copy link
Author

dans98 commented Dec 28, 2022

@celtare21 you should be able to test it now!

@Maik35
Copy link

Maik35 commented Dec 29, 2022

Ich habe nach einem Klipperupdate das Problem das mein Drucker bei ca. 145C mit dem aufheizen abbricht was auch beim PID des Hotends so ist

@celtare21
Copy link

@celtare21 you should be able to test it now!

Can confirm that pid_v now works fine

@freakydude
Copy link
Contributor

Ich habe nach einem Klipperupdate das Problem das mein Drucker bei ca. 145C mit dem aufheizen abbricht was auch beim PID des Hotends so ist

@Maik35 Hier gehts um was völlig anderes. Am besten du versuchst dein Glück bei Discord in der Klipper Community https://discord.klipper3d.org/

@KevinOConnor
Copy link
Collaborator

Thanks. Very interesting. I have two high-level comments:

  1. On a procedural level, this PR is going to be hard to review and merge as it seems to be mixing functional changes with unrelated changes. (For example, changing the core algorithms in the same commit that renames variables, makes whitespace changes, changes filenames, and similar.) It's important that changes be made in small functional chunks so that we can utilize tools like git bisect and git revert. Viewing these commits right now it appears to be a "rewrite" of the code. It's fine if we ultimately obtain totally new versions of the code after several commits. But in my experience a single bulk replacement of the existing code with new code greatly increases the risk of introducing regressions.
  2. It's unclear to me when I should use a pid or pid_v controller. I suspect other users would be similarly unsure. It's also not clear to me if there are a sufficient number of users that would obtain a real-world benefit to a pid_v controller (some notes on this at https://www.klipper3d.org/CONTRIBUTING.html ). So, for the pid_v commit, I suspect we would need some kind of user facing document detailing the tests a user should run to determine if a pid_v controller should be used and the benefits the user should expect from utilizing it.

Cheers,
-Kevin

@KevinOConnor KevinOConnor added the pending feedback Topic is pending feedback from submitter label Dec 31, 2022
@celtare21
Copy link

+1 On the documentation. Would love to see all of it properly documented, comparisons between options etc. As well as what would a "proper" pid tuning session be like.

@freakydude
Copy link
Contributor

freakydude commented Jan 1, 2023

I totally agree with Kevin. I played around a bit with it. On difference I noticed is, that it takes longer to reach the target temperature but (if it was the calibrated one) it is hold with less tolerance. Nice!

I tried it with the heat bed... Same result but significant longer time to heat up. In my case I would stay with classic pid for it.

Using a target temperature, not calibrated, the algorithm has permanent problems...
Classic pid works better in this case ... But can't proof that, just a feeling.

Maybe it helps

@dans98
Copy link
Author

dans98 commented Jan 1, 2023

@KevinOConnor

Viewing these commits right now it appears to be a "rewrite" of the code. It's fine if we ultimately obtain totally new versions of the code after several commits. But in my experience a single bulk replacement of the existing code with new code greatly increases the risk of introducing regressions.

To an extent this is the case, but I will be happy to answer any questions about the changes i've made.

  1. It's unclear to me when I should use a pid or pid_v controller. I suspect other users would be similarly unsure. It's also not clear to me if there are a sufficient number of users that would obtain a real-world benefit to a pid_v controller (some notes on this at https://www.klipper3d.org/CONTRIBUTING.html ).

Generally speaking pid_v will almost always be better, because it intrinsically prevents integral wind up. The only time pid_v wouldn't be better would be when used with noisy sensor readings, because it uses a 1st and 2nd derivative. Thankfully it's easy to tell if pid_v will work or not. You do a test run, and if it doesn't settle in it won't work. The only thing you can really change would be to use different pid tuning parameters (more on that in a moment).

Astrom & Hagglund covers velocity at a high level int their books, for example section 3.5 of this one.
https://www.ucg.ac.me/skladiste/blog_2146/objava_92847/fajlovi/Astrom.pdf

Here are some comparison graphs from my hotend, using the same pid parameters.

The plot short hand:
origonal = the current control algorithm used by klipper
updated = the updated version of the original
velocity form = the new velocity control algorithm

Plot of the full test runs.

zoomed in showing the initial warm up with no wind-up/overshoot.

This plot shows the summation of absolute variance from the target temp, once they have all settled in. Zero variance would be a perfectly strait horizontal line.

Here is an example of my very noisy AC powered, high thermal mass bed, with a tortures target temperature of 40C. Velocity control just did not work with Ziegler-Nichols parameters, but when i switched to Cohen-Coon as @Sineos recommended I got far better results with positional and velocity form control, but velocity form was better overall.

The plot short hand:
positional - the current control algorithm used by klipper
velocity - the new velocity control algorithm
zn - Ziegler-Nichols
cc - Cohen-Coon
calibraded - pid parameters generated directly from a calibration run.
manual - pid parameters generated from a calibration run that have been manually tweaked to yield better performance.

Plot of the full test runs.

zoomed in showing the initial warm up. Because of the high thermal mass and large delay associated with my bed i always have overshoot, but it's worth noting velocity form always has less.

absolute variance

So, for the pid_v commit, I suspect we would need some kind of user facing document detailing the tests a user should run to determine if a pid_v controller should be used and the benefits the user should expect from utilizing it.

I'm, out of town at the moment, but i'll ty and write something up over the next few days, and i'll run it by the community on discourse.

@freakydude
Copy link
Contributor

freakydude commented Jan 2, 2023

That explains my observation. Also, I found the rest of your Klipper Discourse mentioned in one of your commits earlier very helpful for understanding.

I didn't realized that you did 3 things:

  • Modified the current PID algorithm
  • Additional implemented a PID Velocity variant
  • Updated the calibration algorithm for both

As I switched to your branch and switched at the same time to pid_v, my extruder and heater bed heat up time inceased. In fact, not really. In my start code, the printer starts to print if the target temp is reached the first time within a tolerance of 2.5%, With PID it overshoots the target temp after that - but starts printing. With PID_V the print starts at a later time point (after the classic PID would be settled down to the target temp).

Maybe it would help people with similiar observation, to describe in the documentation for Config_Reference.md on a high level that pid has high overshots on initial target temp changes in contrast to pid_v which tries to avoid them, resulting in a longer time to reach the target temp (or higher) the first time. But beside of that both algorithms result in a more accurate temperature.
In addition I found only in the WRITE_FILE description a word about the TOLERANCE parameter for PID_CALIBRATE. I would give the parameter more attention for people with noisy hardware or similar problems.

And finally, a question I asked myself and maybe you can answer this: Is there a way to calibrate more target temps? As I mentioned earlier - for example changing between PLA and ABS, my target temps would change from 210/65 to 245/115°C. What target temp would result into a more accurate result for both filaments? Just the average like 227.5/90°C?

Anyway, great work. Would really like to see this pull request merged!

(Sorry for my average english level)

@KevinOConnor
Copy link
Collaborator

Thanks for the additional information.

Generally speaking pid_v will almost always be better

What does it mean to be "better" in this context? Is it better in the mathematical sense of less deviation from requested, or better in the sense that users will obtain real-world prints with noticeably improved quality?

I do find this interesting. For what it is worth though, all the pid_v graphs look roughly the same to me at a high-level. All the extruder graphs settle at roughly 2 minutes, and the bed graphs (with the exception of "positional zn calibrated") generally settle between 3 to 5 minutes. These times aren't significant when compared to a typical print time of 2-5 hours. So, I'm not sure I'd add a new option to Klipper for pid_v.

Again, I do find this interesting, and I'll try to review your code to better understand it. I'll start with a focus on the pid_calibrate changes.

Cheers,
-Kevin

@dalegaard
Copy link
Contributor

Hi,

I'm seeing much more stable temperatures in my machine with this patch. I run an 80W heater and a 24 CFM fan, and the upstream code was never able to effectively regulate this combo for me.

I'm not currently using the velocity form.

Best regards,
Lasse

@dans98
Copy link
Author

dans98 commented Jan 5, 2023

What does it mean to be "better" in this context? Is it better in the mathematical sense of less deviation from requested, or better in the sense that users will obtain real-world prints with noticeably improved quality?

These times aren't significant when compared to a typical print time of 2-5 hours. So, I'm not sure I'd add a new option to Klipper for pid_v.

@KevinOConnor

The two points above are linked in my opinion, so I will see if I can answer your question and justify why in my opinion pid_v should exist as an option. I apologize upfront as I’ve had a really 48 hours at work thanks to some major release issue (not my work), so this might get a little rambly.

The most obvious way pid_v is better is that it drastically reduces or completely removes overshoot because of integral wind-up. In the grand scheme of things (as you mentioned), overshoot doesn’t affect print quality, or print time. However,It does help reduce the amount of material you waste printing skirts or using a custom priming routine.

For me that means I'm wasting several meters less filament per spool. I do a lot of small quick prints (less than 30 mins) for prototyping stuff, and I print hot so i get more oozing than others during the warmup.

Where pid_v is better, but in a far less obvious way, is keeping the temp on target.

bang bang, positional pid, velocity pid, etc are all doing the same thing, trying to balance a thermal equation. In the case of a hot end, you have the heater on one side putting heat in. On the other you have ambient temp, print temp, print speed, volumetric flow rate, cooling fan speed etc playing a part in taking heat out.

Each heater has what I like to call a work envelope. At the low end it’s overpowered for the given print parameters and will struggle to hold temp. At the high end, it’s under powered, and will struggle to hold temp. In the middle of the envelope is the sweat spot. Pid_v shines its brightest when you move away from the sweet spot.

Given the plethora of printers, hotends, thermistors, beds, and bed heaters on the market, it would be foolish to say every single user is going to see a marked improvement in print quality. If someone is running their printer with the hot end and bed in the sweet spot they might see a small or no improvement. Someone with their bed or hot end in the sweet spot but not the other should see some improvement. Someone with neither heater in the sweet spot should see a more significant improvment.

On my printer the bed is the most problematic heater. So I’ll use graphs for that.

First I should explain what these graphs are showing in more detail. For each pwm update interval i’m calculating variance as abs(target temp - reported temp). The graph is showing the individual variances summed over time. Thus the higher up the Y axis a curve goes the worse it is at keeping the heater at the target temp.|

my bed calibrated at 40C
40

and again at 75C
75

For both graphs just look at the “positional cc calibrated” & “velocity cc calibrated” curves. At 40C (well outside my beds sweet spot) you can see that velocity pid is far better at keeping the bed on target. At 75C (close to the middle of my beds sweet spot), the two algorithms are on par with each other.

Also take a look at the Y axis, velocity control finished both tests around 300, so it held its level of control relatively constant, while positional control didn’t.

I don’t have any graphs of it on hand, but the same thing will happen as i get close to the upper limit of my bed 110-120C. Velocity control maintains better control than positional control.

Holding tighter temperature control on the bed has very visible effects on print quality. As jslaker mentioned in discourse when the bed temp fluctuates you can get z banding/breathing, because beds do not heat up uniformly. And as he also mentioned cnc kitchen did a nice video on the topic.

The video is talking about bang bang vs pid, but the concept is universal. When the temp of the bed fluctuates, the bed will flex because the bed won't heat uniformly. Depending on what your bed is made from and how it is constrained, the effects can be from mild to extreme.

https://www.youtube.com/watch?v=9JyydfcOcD0

tighter control has positive effects on hot ends as well. if your hot end temp starts fluctuating to much you can get what looks like a case of inconsistent extrusion caused by a clocked nozzle or extruder that slips slightly.

@jslaker
Copy link

jslaker commented Jan 5, 2023

What does it mean to be "better" in this context? Is it better in the mathematical sense of less deviation from requested, or better in the sense that users will obtain real-world prints with noticeably improved quality?

@KevinOConnor

In addition to DanS' comments re: basic print quality, one of the issues I've personally encountered with the existing PID implementation is that auto-tuned variables will essentially never correctly converge for some bed/heater combinations due to overshoot. My suspicion is that the combination of a bed with high thermal mass with a thermistor on the opposite side of the bed from the heater (and therefore considerable lag time in response to saturated heater output) and high heater power is particularly susceptible.

As you noted in #4427, using TEMPERATURE_WAIT instead of M190 can help bandaid over this, but the downside is that bed temperature will continue to oscillate as the PID controller keeps hunting for the set point. In my case, the PID controller was never able to fully recover from overshoot with auto-calibrated terms and would run hot throughout a print.

I was able to get things behaving properly after some extensive manual tuning, but, given the complexity of PID tuning, I'd think that's a pretty big ask of most users.

In my testing, my manual terms still heat more quickly (read: are considerably more aggressive) than what I'm seeing with these changes, but the biggest difference is that it generated terms that would consistently settle at the set point in a reasonable amount of time.

I have seen improvement with my hotend as well, but in that case, it's more "holds setpoint more consistently" rather than "never reaches setpoint." :)

@pki791
Copy link

pki791 commented Jan 7, 2023

I use the bambulab hotend which heats up very quick, also the PID tune process is done within one minute. The tune returns almost similar values as the master branch. But as i changed to PID_V algorythm the heater is much more stable, overshoot less and holds the temperature during print within 0,5deg, also the range of pwm is much smaller. With PID it shoots peranently over 50%, with PID_V it is less than 20%. I vote to merge at least the PID_V algo.

Edit:
After some more testing my opinion is:

  • The PID_CALIBRATE routine should return additional PID , optional PID values for "Some Overshoot" etc.
  • pid_v should be availible as it gives good improovement, specially in high speed hotend like bambu.

@KevinOConnor KevinOConnor removed the pending feedback Topic is pending feedback from submitter label Jan 8, 2023
@KevinOConnor KevinOConnor self-assigned this Jan 8, 2023
@dans98
Copy link
Author

dans98 commented Jan 25, 2023

@KevinOConnor

I'm working on the documentation now, is there a place you want me to put the spreadsheet i created?
https://klipper.discourse.group/uploads/short-url/oR6MJ0s0I07T2LIXvgZB6iql5DY.zip

I thought about redoing it in pure js, but it seems its not possible to include html/js in md files.

@pki791
Copy link

pki791 commented Jan 26, 2023

What about a python script? Eventually i can help if You can't do python.

@MGunlogson
Copy link

@KevinOConnor

My memory of calc is hazy, but I believe the trapezoid rule is indeed more accurate when the underlying function is continuous, even if your samples are discrete. And temperature change definitely is continuous.

Trapezoid rule is more accurate unless your underlying data is discrete. For example, a sensor that counts jellybeans passing by. You can't have half a jellybean so trapezoid rule won't increase accuracy in that case.

Anyways, temp change isn't discrete values so AFAIK trapezoid rule will help quite a bit in our case.

@vkhodygo
Copy link

vkhodygo commented Jun 8, 2023

Wow, that escalated quickly. Folks, why don't you calm down a little bit.

@KevinOConnor, it's up to you to decide whether to merge this or not. However, and a lot of other people would probably agree with me here, one person however experienced they are is barely enough to maintain a project of this size, adding new functionalities or noticeably improving existing ones is far beyond this. There is nothing wrong with relying on people with experience in relevant areas.

I should have just listened to what so many others told me

Welcome to the club, that's open-source software for you.

@dans98 @CroXY3D There is no need for argumentum ab auctoritate here, you're well aware that's not how it works.

I do numerical science for living and I clearly understand the difference between various integration methods. However, I too failed to grasp it in this particular case. Note though I didn't have much time to read the whole history so that works against me. Could you and @InsanityAutomation ELI5 that to Kevin and other non-specialists including myself? The most basic relevant example should do the trick.

@freakydude
Copy link
Contributor

freakydude commented Jun 8, 2023

I've been thinking about whether or not to say something about this. I've been following the Pull Request from the beginning and I don't like to see it just die now.

I suggest that we all calm down a bit here first and look at the situation from the balcony.

Let me summarize my view:
This improvement has a good half year of discussion, implementation and various tests behind it. And a lot of heart and soul. That a bad regression is to be feared, I therefore consider it rather unlikely. The improvement is based on the initial paper, a theoretical improvement is not to be dismissed and at least here some people have already confirmed it in their scenario. None has reported a worsening so far.

On the other hand, the current PID implementation works well, so the risk of a new bug creeping in cannot be completely dismissed.

To reduce this risk, there are usually a few remedies in software development.

  • As Kevin suggests, and I don't think anyone here has seriously objected, small atomic commits, so that changes are easily understandable, ideally sorted logically. How small is small enough, of course, is open to debate. If one algorithm is replaced by another, only a complete commit makes sense. Here, even the complete difference of the pull request is far from atomic, but not really much code either. For me it would be reviewable - maybe you both just do it live in Discord and have a beer ...
  • Tests. Automated tests are the very best way to detect bugs and thus minimize risk and fear. Of the existing tests, none has ever broken. I think we could talk about using this pull request as a reason to add more automated tests.
  • Feature Toggles. If variant 1 and 2 are not good enough for other reasons, I would suggest to hide the modified PID algorithm (completely) behind a configuration entry "pid_new" and the velocity based one under "pid_v" and let all users out there run the code under "pid" as it has existed for a few years now. The solution allows that the code is mainline, but the users can switch to "pid_new" or "pid_v" experimentally for a longer while - until one dares to replace "pid" by "pid_new".

I think the aspects are valid, from my point of view it needs an announcement (or assistance), then it will be done for sure.

The recent discussion around single lines of code and theoretical optimums of these I consider quite petty. Here I would recommend to the involved ones to swallow that. Optimizing can always be done in the end before everyone loses interest.

I would be happy if this ends well and maybe is an occasion for upcoming PRs.

Many greetings and thanks for the great project

@Zeanon
Copy link

Zeanon commented Jun 8, 2023

On the other hand, the current PID implementation works well, so the risk of a new bug creeping in cannot be completely dismissed.

To be honest, the current implementation does work, but not that well for certain types of heaters, mainly high power PTC heaters like in the phaetus rapido(the reason why I switched to this implementation is that I had a temperature overshoot of 10°-15° when heating up which was far from ideal).
I get Kevin's point, which is why I already offered to help break down this PR and I also agree with you that one person is not enough to maintain a project of this size which is why I would suggest to get more people on board(I know there are already four).
I know this would need some time in the first place to get everyone up to date/discuss what is needed/expected from everyone but I think it would be worthwhile in the long run.
I can also again offer my help with the project, however that would be.

@jslaker
Copy link

jslaker commented Jun 8, 2023

To be honest, the current implementation does work, but not that well for certain types of heaters, mainly high power PTC heaters like in the phaetus rapido(the reason why I switched to this implementation is that I had a temperature overshoot of 10°-15° when heating up which was far from ideal).

To echo this: the impetus for @dans98 working on this code is that we observed certain printer setups where temperature would never correctly converge with the setpoint using the extent control loop and calibration routine. As I noted in a previous comment, there are years-old bug reports describing this behavior which makes sense given this code has been in place even longer.

The code in place now works reasonably well for most setups, but there are printers in the real world where it's not a matter of some marginal level of inaccuracy but being unable to start prints entirely without resorting to workarounds.

@MGunlogson
Copy link

I concur with recent comments. I really want to see this merged. I've been looking forward to it more than any other Klipper feature.

I have both a weak bed heater and Rapido hotend. The current algo overshoots hotend 15C and can't hold "extreme" bed temps. The new algorithm (PID_V) doesn't overshoot and holds bed temps from 40C-120C. That's 10C lower and 15C hotter than current algorithm can manage.

Many of us Klippers are using modified printers operating on the edges of heater capability. Improvement in PID control will allow me to print TPU and Polycarbonate without replacing my heaters or messing with PID parameters when I switch materials.

Plz try to work this out y'all. @dans98 put a ton of work into it and it has real benefits for some users.

@celtare21
Copy link

+1 on adding new features and improvements. To me, being reluctant to new stuff is only a step backwards. I understand not wanting to introduce regression or bugs, but staying in place is also a regression. Code is never perfect and we should always be looking to improve it. Just my 2 cents

@Trails5000
Copy link

Im with these guys, the current auto tune doesnt even work (ive had to fudge the numbers to the point where its dangerous for it to even spit out something reasonable in the past). The current code looks like it was a bit of a hack so it would be stable on 8 bit systems, its time to upgrade and move with the modern times.

This well meets the 100 user threshold. Please merge it, im tired of having to cherry pick commits because they aren't deemed worthy; even though people want them.

@dans98
Copy link
Author

dans98 commented Jun 8, 2023

@vkhodygo

The most basic relevant example should do the trick.

The simplest way to see this is to take a a piece of paper and set up an X/Y axis. Then from left to right, place some random dots. Then again from left to right connect the random dots with strait lines, just like if you tried to plot the random sensor data like I did in this response.
#5955 (comment)

They try and approximate the area under the interpolated dots with rectangles and trapezoids. The rectangles will have errors, the trapezoids will not. If I have time over lunch I will do a white board example.

@KevinOConnor
Copy link
Collaborator

I do appreciate feedback, however I feel some of the comments in this thread have been unacceptable.

It is important to me that all messages on official Klipper forums are polite, "up beat", and have a positive tone.

If I have time over lunch I will do a white board example.

Thanks, but I don't feel that will be a good use of your time. I feel we are "talking past each other". I do feel that I understand what you are saying (to wit, approximating the integral of a curve differs depending on how one approximates each subset). I do not feel you've understood what I am saying (to wit, the specific software implementation that sums a series of finite values has near equivalence with the existing software implementation that sums a series of finite values).

I am planning to take a break from this discussion for a few days. As a suggestion, I think it would help move this forward if we could focus the code and conversation on one primary topic (for example, changes to the existing pid algorithm, changes to the pid calibration tool, or a new pid algorithm). We seem to have gotten side tracked on changes to the existing pid algorithm, and it is not clear if that is even a priority. On any PR I review I am going to ask questions about the implementation and ask about alternatives - this is not intended to be criticism.

I do appreciate that you've put a lot of work into this and that you are trying to improve Klipper. Thank you.

-Kevin

@MGunlogson
Copy link

@KevinOConnor understandable. Perhaps we can follow a previous suggestion to have the new code split completely from the old implementation. "PID_NEW" for the modified version, and "PID_V" for new algo or something like that. That way if there's any bugs it won't affect existing installations. Tuning could be handled a similar way, with the new tuning algorithm used for the new PID algorithm and existing one remaining as is?

That way we can get this merged so Klippers can test it and get some real world experience on its performance. Could mark it experimental in the docs too, to discourage newbies from using it until it's cooked for a while.

@dans98
Copy link
Author

dans98 commented Jun 8, 2023

in a week or to I should be able to work on updating the velocity to address the the time intervals being able to very.

@Zeanon
Copy link

Zeanon commented Jun 22, 2023

I have just encountered an interesting phenomenon:
When setting my bed to 45° while printing, I got 50°(with pid profile tuned for 45°)
When setting it to 45 manually it still kept creeping up
When setting it to 50 with a profile tuned for 50, I got like 52
at 55 and above, I get what I request

@dans98
Copy link
Author

dans98 commented Jun 23, 2023

I have just encountered an interesting phenomenon: When setting my bed to 45° while printing, I got 50°(with pid profile tuned for 45°) When setting it to 45 manually it still kept creeping up When setting it to 50 with a profile tuned for 50, I got like 52 at 55 and above, I get what I request

@Zeanon

where you running mainline, or my branch?
If mine where you were running mine where you using pid or pid_v?
also what do you mean by setting it to 45 manually?

@jslaker had this issue with the old controler and his bed. The temp would overshoot, and just never come back down to the tarket. He got around it with a lot of manual tuning if memory serves.

Technically speaking any pid controller can have this issue.

  1. to much or to little smoothing.
  2. sensor going bad and generating more noise. (i had this happen when a pt100 started going bad)
  3. connection not fully seated and thus more noise (i had this after to many rounds of resonance testing)
  4. pid parameters to different for the current environment. (i had this as well when enclosed my printer, and insulated it)

@Zeanon
Copy link

Zeanon commented Jun 23, 2023

Sorry, I should have clarified
I am running your PID code(but with my implementation of PID Profiles added, that's why I had tuned variables for 45 and 50 ready to go)
The initial problem(printing something and getting 50° even though 45 was requested) happened with pid_v.
After that print finished I waited a bit, restarted and then set the temp to 45 in the Webinterface to make sure it was not just a one off.
It also crept higher(up to 47/48° until the idle timeout kicked in).
I also did the same thing but changed the control to PID and the result was basically the same.
As for changes in environment or stuff: it could ofc be that my thermistor is dying or the wire is bad somewhere but I doubt that since every temp above 50 is rock solid.

@dans98
Copy link
Author

dans98 commented Jun 26, 2023

@Zeanon

I'm ready to summit a PR just to add velocity pid control, and it incorporates your changes . What name and email address do you want me to use in the sign off?

Signed-off-by: first_name last_name username@domain

@Zeanon
Copy link

Zeanon commented Jun 26, 2023

@Zeanon

I'm ready to summit a PR just to add velocity pid control, and it incorporates your changes . What name and email address do you want me to use in the sign off?

Signed-off-by: first_name last_name username@domain

Thx for asking :)

Signed-off-by Vinzenz Hassert [email protected]

Would there be any changes to the velocity algorithm compared to the one in this PR? or would it be the same algorithm?(cause I want to keep using your improved auto tune since it provides way better results for me)

@dans98
Copy link
Author

dans98 commented Jun 26, 2023

@Zeanon

Would there be any changes to the velocity algorithm compared to the one in this PR? or would it be the same algorithm?(cause I want to keep using your improved auto tune since it provides way better results for me)

It's functionally the same, it's just updated to deal with the fact that we can't rely on the interval between pwm being constant.

to continue using my calibration revision, you should just be able to drop the calibration file into the appropriate location and restart. assuming the velocity pid pr request gets merged into master, I will then make another one to incorporate my calibration revision.

@dans98
Copy link
Author

dans98 commented Jun 26, 2023

for those that are interested here is the new minimal PR.
#6272

@Zeanon
Copy link

Zeanon commented Jun 26, 2023

Ah ok, then I'll have to grab the changes for the pwm intervals and drop them in my fork
Thx for the quick response

@dans98 dans98 mentioned this pull request Jun 26, 2023
@github-actions github-actions bot added the Stale label Aug 12, 2023
@github-actions github-actions bot closed this Aug 19, 2023
@KevinOConnor
Copy link
Collaborator

This PR was inadvertently closed due to a regression introduced by #6293.

-Kevin

@pki791
Copy link

pki791 commented Aug 24, 2023

Is this new PID functionallity now included in the main release or still not?

@dans98 dans98 closed this by deleting the head repository Aug 27, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Aug 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.