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

Klipper velocity pid #6272

Closed
wants to merge 2 commits into from
Closed

Klipper velocity pid #6272

wants to merge 2 commits into from

Conversation

dans98
Copy link

@dans98 dans98 commented Jun 26, 2023

A stand alone pr the contentions the minimum amount of changes required to incorporate the velocity pid control algorithm.

@dans98 dans98 mentioned this pull request Jun 26, 2023
@dans98
Copy link
Author

dans98 commented Jun 26, 2023

This pr is part of the effort to break PR 5955 into smaller commits.

@Zeanon
Copy link

Zeanon commented Jun 27, 2023

Is it intentional that this code:

        if target_temp == 0.:
            self.d1 = 0.
            self.d2 = 0.
            self.pwm = 0.

got reduced to this:

        if target_temp == 0.:
            self.pwm = 0.

?

@dans98
Copy link
Author

dans98 commented Jun 27, 2023

@Zeanon

Yea it's intentional, The derivatives don't need to be zeroed, and since they are smoothed it's better thast they aren't.

@Zeanon
Copy link

Zeanon commented Jun 27, 2023

One small change I would make is to change it to this:

        if target_temp > 0.:
            self.pwm = max(0., min(self.heater_max_power, self.pwm + p + i + d))
        else:
            self.pwm = 0.

That way the edge case of having a target temp below 0 is also covered

@dans98
Copy link
Author

dans98 commented Jun 27, 2023

I'm not sure if you can actually request a sub 0 temperature. from my quick test in mainsail, I get an error.

@github-actions
Copy link

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.

@dans98
Copy link
Author

dans98 commented Jul 11, 2023

@KevinOConnor can you assign a reviewer to this?

@ganzgustav22
Copy link

Looking forward to finally have a non-broken PID heating in Klipper. Maybe another half a year and then it'll be merged.

@KevinOConnor KevinOConnor self-assigned this Jul 19, 2023
@scottmudge
Copy link
Contributor

Been testing this, in my (anecdotal) experience, and after recalibrating my PID values with it enabled, my temperatures do seem much more stable. Instead of overshooting at initial heating by ~1C (I have recalibrated multiple times in an enclosure with stock PID implementation, it never went away), it now gracefully reaches target and stays within +/- 0.1C. Variance before was usually +/- 0.5-0.8C during the course of the print.

It does take slightly longer to reach the target (e.g., 5-8 seconds longer for a target of 245C at the extruder), but I'll take that over a more oscillatory/variant overall temperature.

@danorder
Copy link

danorder commented Aug 5, 2023

To chime in i can confirm similar results to @scottmudge on my hotend using a 60watt on a volcano block. prior it would take a bit to setting now it just coasts up to target. Albeit the settle time wasn't overly long just not great in comparison to pid_v. enclosed pic is pidtuning then ramping up to target 240
pid

@freakydude
Copy link
Contributor

freakydude commented Aug 7, 2023

Just to remark that: This velocity pid algorithm seems not working very well if you (as in my example) calibrating the hotend with pid_v for 210 °C and then try to set a target temperature of 240 °C. The temperature drifts step by step upper and didn't settle back to 240. I also have a volcano hotend with 64W heater.

But it works, if the target temperature is the one calibrated.

pidv-c210-s240-i243

In contrast, the base pid algorithm has no problem with the same task. So it seems like it has it's pros and cons.

@danorder
Copy link

danorder commented Aug 7, 2023 via email

@danorder
Copy link

danorder commented Aug 8, 2023

@freakydude. We should have different results since i failed to mention Im using a semi proprietary clone. This also drops temps more rapidly with connected block supports to a large aluminum plate. (v400 design) Not quite ideal much harder to heat properly. As apposed to the intended e3d design or how phaetus goes about it.. To add further clarity. but i suppose alloy would be the other variable since as far as i know e3d has a special blend on their aluminum blocks vs generic clones.

@dans98
Copy link
Author

dans98 commented Aug 8, 2023

@freakydude what you are seeing is a side effect of the old pid calibration algorithm. I have not seen this when the new calibration method was used, but that won't come till after the pr is mainlined.

@freakydude
Copy link
Contributor

@freakydude what you are seeing is a side effect of the old pid calibration algorithm. I have not seen this when the new calibration method was used, but that won't come till after the pr is mainlined.

Could be true, I tested your "big" pull request a (long) while ago. And I can't remember seen this problem. I could retest the same case if wished to double check?

@dans98
Copy link
Author

dans98 commented Aug 10, 2023

You can if you want to!

@KevinOConnor
Copy link
Collaborator

Thanks. As high-level feedback, the code seems fine to me. I have some questions on the high-level impact of this change.

How should a user choose the PID parameters for pid_v? Do they just use the same pid parameters that they were previously using for pid and just enable pid_v with them?

How should a user determine if they should choose pid or pid_v?

Similarly, just for my own information, what kinds of heaters do you expect would benefit the most from pid_v? For example, heated beds vs extruders, high power extruders, heaters that can change temperatures rapidly, etc.

I'm not sure how to evaluate the high-level benefits of making this change to Klipper. On the one side it may help users that are struggling with a heater that is not robust with the current PID code, but on the other hand it may add unnecessary complexity for users. (That is, there is a risk that many users spend time researching and testing pid vs pid_v, but find that they don't observe a real-world quality difference with either choice - thus resulting in lost time for users.) Do you have recommendations on tests that users could perform and the metrics we should look at to evaluate the overall benefit? An example of a metric and goal would be something like: get users to test at least 20 different printers, and look for at least two heaters that oscillate with extremes that are 2 degrees less than the oscillations on pid. As before, I'm not sure what kind of metrics make sense nor the goals we should look for.

Cheers,
-Kevin

@Sineos
Copy link
Collaborator

Sineos commented Aug 12, 2023

As indicated by other comments, I see the same pattern on my machines:

  • Asymmetric systems, i.e. heat gain faster than loss due to insulation (in my case also an insulated AC bed).
  • Interestingly, I see no benefit at the other extreme: heat loss faster than heat gain, i.e. due to non-insulated hotend and full fan.
  • High power systems with low thermal mass (in my case an 80W cartridge on a Dragon hotend)

They seem to benefit from pid_v in that they no longer overshoot and seem to be a bit more stable afterwards. To be fair, I can see a small improvement, but the current pid does a good job on my end as well.

What I can also confirm: Both implementations currently perform worse when tuning the PID at a low setpoint, e.g. hotend at 210°C and then go to 270°C. But I always tune my PID at the high end anyway, and have not noticed any adverse effects when going lower.

What I really miss from the previous implementation is the (a)symmetry value during auto-tuning, which can be interpreted as a heat gain / heat loss ratio. I see this as a real added value, since it allows improving the hardware in a more targeted way, i.e. to give the user an indication if the system would benefit from insulation and/or more power.

In my opinion, it gives the user a choice: If in the abundance of different constellations / systems pid performs better, use pid, otherwise try pid_v and see if it improves.

@danorder
Copy link

danorder commented Aug 13, 2023

@KevinOConnor After further testing or rather different hardware scenarios/printers. It may be wise to add a description regarding wat general thermal characteristics may need 1 or the other. On what i'd call a faulty volcano clone with large thermal loss using pc inserts / steel screws going into a large plate. Pid_v doesn't seem to heat quick enough to deal with slicer fan changes in some scenarios. However if its "proper" (no supports leaking alot of heat) Id be overshooting slightly with regular pid which pid_v solves. The other scenario pid_v solves my heavy insulated dc bed at 30v being unable to settle or drop temps for ages , unless i fix the values by hand. My regular bed without insulation / usual 24v however i see no benefit per say. So it would seems quite hardware dependent either, option should seems to be able to handle a plethora of hardware scenarios. Such as the issues with certain beds needing smooth time fixs / ac beds. otherwise 'most" beds probably work better under pid. As mentioned the variable seems to be insulation / high-power. Otherwise on hotends power to thermal loss. In my situation id be using pid on the clone hotend/proprietary effector if i were a regular user. However, Once i fix the thermal loss i'll be returning to pid_v (replacing the support screws with titanium / or complete removal like the regular e3d volcano design.)

@dans98
Copy link
Author

dans98 commented Aug 14, 2023

How should a user choose the PID parameters for pid_v? Do they just use the same pid parameters that they were previously using for pid and just enable pid_v with them?

That's' correct, they use the same parameter's for pid_v as they would pid. The only caveat to that is that' pid_v is less forgiving of a noisy signal, or a bad tune/calibration (and that will be corrected with the modification of pid calibration).

How should a user determine if they should choose pid or pid_v?
First I would say they should look at the amount of initial overshoot they are getting and determine if they are ok with it.

On a hot end

  1. Excessive initial overshoot can potentially cause several issues that detract from an otherwise perfect looking first layer.

    • stringing
    • blobbing
    • elephant's foot
  2. If you use a silicone sock, you can run the hot end temps higher without needing to worry about the overshoot exceeding the temperature the sock is rated for (usually only a problem for people printing more exotic filaments).

On a bed

  1. If the bed has excessive overshoot it can cause the same visual defects the hot end can.
  2. if you run high bed temps the overshot, can drastically reduce the life of the adhesive holding the heater to the bed. I've seen this first hand in the railcore community. it's expensive to deal with and a potential fire hazard.

Second, I would say they should decide if they are ok with the temperature fluctuations they see when the heater is up to temp. If the heater has a decent tune/calibration pid_v generally shows less deviation from the target temperature. For a hot end that means more consistent extrusion, and for a bed that means less z banding.

Similarly, just for my own information, what kinds of heaters do you expect would benefit the most from pid_v? For example, heated beds vs extruders, high power extruders, heaters that can change temperatures rapidly, etc.

Imo, to many variables are at play to be able to make any kind of definitive statement about who will and won't see a benefit.

Do you have recommendations on tests that users could perform and the metrics we should look at to evaluate the overall benefit?

I'd say to look at the things I mentioned in the response to the "How should a user determine if they should choose pid or pid_v" question.

@dans98
Copy link
Author

dans98 commented Aug 14, 2023

What I can also confirm: Both implementations currently perform worse when tuning the PID at a low setpoint, e.g. hotend at 210°C and then go to 270°C. But I always tune my PID at the high end anyway, and have not noticed any adverse effects when going lower.

That's to be expected imo. If you tune low you will get parameters aimed at holding the heater at the lower temperature. Higher temperatures require more aggressive parameters, because you will be loosing heat to the surrounding environment faster.

@danorder
Copy link

@dans98 re "Imo, to many variables are at play to be able to make any kind of definitive statement about who will and won't see a benefit" Yes agree. From the look of varying scenarios. Probably a few hardware situations in docs should provide a degree of examples. So far on a few machines ive seen 1 or the other reported behaviors. Another thing that has come up,
my none insulated bed has issues dropping temp with the pid cycle flashing on slightly vs staying off etc. The situation here is my room temps around 30ish c so it can't cool as quick. so a temporary situation. I could either rerun pid etc, or B just wait since the room is hot or use petg etc. As a example of a none insulated ac/ high dc volt bed with similar but, temporary behavior. So perhaps a mention of ambient temps playing a role , Things like rapidos/ceramic heaters / high watts low thermal mass. possibly benefiting more with pid_v where regular pid remains the "generic should cover most scenarios option' Maybe for none specific generic hardware. Some disclaimer like pid_v will not solve underlying "thermistor problems / heater assembly" problems. Check these if pid seems problematic.

@KevinOConnor
Copy link
Collaborator

Okay, thanks. I guess my high-level feedback is that I'm not sure how to judge how useful this change is for users in general. It doesn't seem to be a "must have" in that the current PID generally works for most users (it's certainly not optimal, but PIDs rarely are). Without being able to quantify an improvement it's hard for me take on additional code complexity. (I also don't think users want to spend time and effort choosing between "unoptimal PID algorithm A" vs "unoptimal PID algorithm B".)

Also, I guess you are saying that the only way users will know to use pid vs pid_v is to try both and see how long each one takes to stabilize on a particular temperature?

-Kevin

@dans98
Copy link
Author

dans98 commented Aug 27, 2023

Also, I guess you are saying that the only way users will know to use pid vs pid_v is to try both and see how long each one takes to stabilize on a particular temperature?

-Kevin

They need to look at the initial overshoot, and how well it does at maintaining steady state.

@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants