Replace max_accel_to_decel with minimum_cruise_ratio

[KevinOConnor]

This is a proposal to remove the existing max_accel_to_decel parameter and replace it with a new minimum_cruise_ratio parameter. This was proposed by Piezo at https://klipper.discourse.group/t/proportional-acceleration-control/3970 .

This doesn't change the underlying "accel to decel" system. Instead, it renames the user-facing parameter to make it easier to describe and easier to understand.

It can be painful to make changes like this due to the possibility of breaking existing configs, but I think there are good reasons to pursue a change.

The max_accel_to_decel system was designed to avoid moves that immediate go from acceleration to deceleration by enforcing a minimum amount of movement at cruising speed between that acceleration and deceleration. It does this by reducing the top speed of moves. The system can be useful to mitigate heavy vibration effects from short zigzag moves that some slicers emit - in particular on "gap fill" moves. Unfortunately, describing this system as a "pseudo acceleration" is confusing and not intuitive.

As Piezo described, the system can be thought of instead as enforcing a "minimum cruise ratio" - that is, it enforces a minimum ratio between the distance travelled under cruise relative to the distance travelled under acceleration and deceleration. Describing the system as enforcing a "ratio on the distance travelled" I think is more approachable to users and it more closely describes the original intent of the feature.

Some math can show how max_accel_to_decel and minimum_cruise_ratio are effectively the same.

The top speed of any set of moves is calculated using the formula: end_velocity^2 = start_velocity^2 + 2*accel*move_distance

The max_accel_to_decel enforces an additional constraint on that top speed: end_velocity^2 = start_velocity^2 + 2*max_accel_to_decel*move_distance

The relationship between max_accel and max_accel_to_decel can be viewed as a ratio, and thus one could view the above formula as: end_velocity^2 = start_velocity^2 + 2*(ratio*max_accel)*move_distance

And then one can see how that ratio is equally applicable to distance: end_velocity^2 = start_velocity^2 + 2*max_accel*(ratio*move_distance)

As a natural implication of specifying the parameter as a ratio, this PR updates the code to maintain that ratio if the underlying max_accel parameter is changed. This is similar to how the code updates the internal junction_deviation parameter if max_accel changes. (And, indeed, this PR is similar to commit 0025fbf1 where the user-facing junction_deviation was replaced with square_corner_velocity.)

@dmbutyugin - FYI.

I'm not sure how the frontends may react to this parameter change. @pedrolamas , @meteyou - FYI.

-Kevin

Edit: Fixed reference to Piezo in original Discourse message.

[Piezoid]

@pedrolamas Here's the patch I'm using for fluidd

```diff commit e3e9aedc58b3a409d1201e34e5f40eff41c7d9c9 Author: Maël Kerbiriou Date: Thu May 20 18:19:40 2021 +0200 rename accel_to_decel to min_cruise_ratio diff --git a/src/components/widgets/limits/PrinterLimits.vue b/src/components/widgets/limits/PrinterLimits.vue index 84ec73ac..20dd9b2b 100644 --- a/src/components/widgets/limits/PrinterLimits.vue +++ b/src/components/widgets/limits/PrinterLimits.vue @@ -74,17 +74,18 @@ lg="6" > @@ -115,12 +116,8 @@ export default class PrinterLimits extends Mixins(StateMixin, BrowserMixin) { } } - get decel () { - const max = this.$store.getters['printer/getPrinterSettings']('printer.max_accel_to_decel') || this.accel.max / 2 - return { - current: this.$store.state.printer.printer.toolhead.max_accel_to_decel, - max - } + get min_cruise_ratio () { + return { current: 100.0 * this.$store.state.printer.printer.toolhead.minimum_cruise_ratio } } get scv () { @@ -139,8 +136,8 @@ export default class PrinterLimits extends Mixins(StateMixin, BrowserMixin) { this.sendGcode(`SET_VELOCITY_LIMIT ACCEL=${val}`, this.$waits.onSetAcceleration) } - setDeceleration (val: number) { - this.sendGcode(`SET_VELOCITY_LIMIT ACCEL_TO_DECEL=${val}`, this.$waits.onSetDeceleration) + setMinCruiseRatio (val: number) { + this.sendGcode(`SET_VELOCITY_LIMIT MINIMUM_CRUISE_RATIO=${val / 100.0}`, this.$waits.onSetMinCruiseRatio) } setSCV (val: number) { diff --git a/src/globals.ts b/src/globals.ts index 21ec52d1..a26ead93 100644 --- a/src/globals.ts +++ b/src/globals.ts @@ -442,7 +442,7 @@ export const Waits = Object.freeze({ onKlipperFirmwareRestart: 'klipperFirmwareRestart', onSetVelocity: 'onSetVelocity', onSetAcceleration: 'onSetAcceleration', - onSetDeceleration: 'onSetDeceleration', + onSetMinCruiseRatio: 'onSetMinCruiseRatio', onSetSCV: 'onSetSCV', onSetRetractLength: 'onSetRetractLength', onSetRetractSpeed: 'onSetRetractSpeed', ``` *Edit:* fixed `s/min_cruise_ratio/minimum_cruise_ratio/`

[leptun]

@KevinOConnor I think you tagged the wrong person :)

[KevinOConnor]

@KevinOConnor I think you tagged the wrong person

I apologize. I did mess up there. I've updated the original post.

-Kevin

[pedrolamas]

@KevinOConnor from the perspective of Fluidd (and I believe Mainsail too), this is a breaking change as an expected status key has been replaced with another.

Impact on existing installs is just that the "Accel to Decel" control would not show the current value correctly, but would still allow to change it we use SET_VELOCITY_LIMIT ACCEL_TO_DECEL=x and that still works.

Assuming this change goes ahead, in Fluidd I will need to ensure we check if either key is present and show the respective control, ensuring both new and legacy behavior is supported.

Thanks @Piezoid, but from a Fluidd maintenance perspective it is not that simple, as we need to ensure that users with older Klipper versions are still supported, so legacy behavior must also be supported.

[meteyou]

@KevinOConnor thx for the ping! I only have one question about the value. I was surprised several times while testing. I assumed that the value is the % of the max_accel. i.e., the value can be calculated in this way:

max_accel / max_accel_to_decel = minimum_cruise_ratio

As an example: 10000 / 7000 = 0.7

And you could say, "it is 70% of max_accel", but the value is 0.3. Exactly "the other way round". Is there a description/reason for this decision? Or am I just thinking the wrong way? I have read the description text in the config references and now understand the value. I must not think about the conversion.

@pedrolamas I check if there is a minimum_cruise_ratio value in the toolhead object. If it doesn't exist, I display the old field. This is a simple fallback, and everything is backward compatible. Have I missed something?

[pedrolamas]

@pedrolamas I check if there is a minimum_cruise_ratio value in the toolhead object. If it doesn't exist, I display the old field. This is a simple fallback, and everything is backward compatible. Have I missed something?

That is exactly what I wrote (I think), you did not miss anything.

[KevinOConnor]

I check if there is a minimum_cruise_ratio value in the toolhead object. If it doesn't exist, I display the old field. This is a simple fallback, and everything is backward compatible.

That sounds reasonable to me.

We can plan out when to make a change like this (assuming there are no major objections). I'm thinking merge in late January (after the holidays).

-Kevin

[github-actions[bot]]

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.

[pedrolamas]

Kind of ironic seeing the GitHub bot complaining on a Pull Request from Kevin! 🙂

[dmbutyugin]

My 2 cents: I think overall the change makes a lot of sense, especially if people are using acceleration control in the slicer. I only find the current option going a bit against the other commonly accepted parameters - usually the ratio or percentages in the other GCode commands work this way - the larger the value, the faster the print goes, or the operation is applied more aggressively. Here it's the opposite - the higher the value, the slower the print will go. Alas I don't have a very good suggestion for a replacement - max_accel_ratio would be a misnomer (as there's also deceleration). But in case others would find a suitable name for a parameter like this - I'd prefer that slightly instead.

[KevinOConnor]

Thanks for the feedback Dmitry.

I only find the current option going a bit against the other commonly accepted parameters

Yeah - that occurred to me as well. And, I too could not come up with a better name.

I guess we could avoid the word "minimum" so that it doesn't seem similar to the other style of parameters. Maybe something like enforced_cruise_ratio..

-Kevin

[meteyou]

@KevinOConnor, please do not forget that if you change the name, Pedro and I must do this in the GUIs first (and orcaslicer). So, if the name changes, we need a lead time to change and release it again.

[KevinOConnor]

So, if the name changes, we need a lead time to change and release it again.

Agreed.

No changes at this point. Happy to discuss alternate names if there are suggestions.

-Kevin

[KevinOConnor]

FYI, if there are no further comments, I'll plan to commit this (minimum_cruise_ratio) this Friday.

Cheers, -Kevin

[FOG-Yamato]

It's been a bit over a month since that "Friday", is there any update regarding this PR? I've been using it already for a couple of months myself and it works a lot better than the previous solution.

[KevinOConnor]

This PR got side tracked a bit due to some possible concerns I had with naming. However, it doesn't seem like there is a "better name", so I have merged this PR with the original "minimum_cruise_ratio" name.

-Kevin

[SoftFever]

@KevinOConnor,

Is it possible to maintain the max_accel_to_decel function for a certain grace period? I am the maintainer of the open-source slicer, OrcaSlicer, and many community contributors, including myself, have implemented quite a few special optimizations for Klipper. One such optimization is a feature similar to the newly introduced minimum_cruise_ratio, but it's implemented on the slicer side and utilizes max_accel_to_decel.

The disruptive API changes introduced in this PR will result in G-code generated by the current OrcaSlicer not working if they have this feature enabled after updating Klipper.

Could you consider applying a "grace period" approach by reinstating the max_accel_to_decel compatibility and deprecating it after the grace period ends (say, a 1-year period)? This would ensure that we can safely assume most users have upgraded to the latest versions of Klipper and Orca Slicer.

[KevinOConnor]

@SoftFever

Is it possible to maintain the max_accel_to_decel function for a certain grace period?

That is certainly possible. This PR did contain compatibility wrappers so that the max_accel_to_decel config setting and SET_VELOCITY_LIMIT ACCEL_TO_DECEL=x would continue to work. Can you elaborate on what is not working, or what additional compatibility wrappers are necessary?

-Kevin

[pedrolamas]

Can you elaborate on what is not working, or what additional compatibility wrappers are necessary?

As @JamesH1978 pointed out in discord, the issue seems to be related with custom macros that use the max_accel_to_decel property that is now missing from printer.configfile.settings.printer!

There are quite a few macros on GitHub that use this variable:

https://github.com/search?q=%22printer.configfile.settings.printer.max_accel_to_decel%22&type=code

[SoftFever]

@SoftFever

Is it possible to maintain the max_accel_to_decel function for a certain grace period?

That is certainly possible. This PR did contain compatibility wrappers so that the max_accel_to_decel config setting and SET_VELOCITY_LIMIT ACCEL_TO_DECEL=x would continue to work. Can you elaborate on what is not working, or what additional compatibility wrappers are necessary?

-Kevin

You're right! I somehow misinterpreted the changes in docs/G-Codes.md as actual code changes, haha. The SET_VELOCITY_LIMIT ACCEL_TO_DECEL=x command is still working. All is well. The sky is still blue and the glass is still green here :)

Cheers!

[KevinOConnor]

The SET_VELOCITY_LIMIT ACCEL_TO_DECEL=x command is still working. All is well.

Okay, thanks. I was receiving conflicting information on a compatibility problem with OrcaSlicer. I'm happy that OrcaSlicer is still working well.

-Kevin

[KevinOConnor]

the issue seems to be related with custom macros that use the max_accel_to_decel property that is now missing from printer.configfile.settings.printer

Yeah, I'm aware some macros broke. The change to printer.toolhead.max_accel_to_decel was a documented change, while the change to printer.configfile.settings.printer.max_accel_to_decel was not documented. It's possible we can emulate one or both of these, but it gets to be a little tricky to do so. Both because we do want to notify downstream that these settings are implicitly different, and because the backend code is not expecting to have to create these compatibility values.

I'm open to suggestions. If the problem is severe enough we can add in wrappers for a few months.

-Kevin

[tastyratz]

I've been watching the discussion thread around this since it came about. I think this is a much more logical choice and it's the right move. I do think some tweaks to the documentation could make it less confusing, however.

Previously, faster speeds and less time cruising meant raising the Max_accel_to_decel acceleration value. Converting this is not fully clear from the configuration example page here: https://www.klipper3d.org/Config_Reference.html#printer

If my accel is set to 12k and ATD is 8k and not 6k (0.5) then on the surface it seems like setting 0.66 raises the cruise ratio (faster print at cost of vibration). I'm guessing, however, that lowering this value to 0.33 would be the actual translation. Is that correct?

In that instance, I propose an additional line in the description such as "Setting a higher value here will effectively reduce acceleration during short moves"

[FOG-Yamato]

I've been watching the discussion thread around this since it came about. I think this is a much more logical choice and it's the right move. I do think some tweaks to the documentation could make it less confusing, however.

Previously, faster speeds and less time cruising meant raising the Max_accel_to_decel acceleration value. Converting this is not fully clear from the configuration example page here: https://www.klipper3d.org/Config_Reference.html#printer

If my accel is set to 12k and ATD is 8k and not 6k (0.5) then on the surface it seems like setting 0.66 raises the cruise ratio (faster print at cost of vibration). I'm guessing, however, that lowering this value to 0.33 would be the actual translation. Is that correct?

In that instance, I propose an additional line in the description such as "Setting a higher value here will effectively reduce acceleration during short moves"

Yes, the ratio is basically 1 - (max_accel_to_decel/max_accel) which is indeed a bit counterintuitive.