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

feat: updated tool extrusion settings to respect printer config #1038

Conversation

kieraneglin
Copy link
Contributor

Fixes #1032

Updates the ToolheadSettings component to respect an extruder's max_extrude_only_velocity and max_extrude_only_distance when trying to update the default extrusion distance/speed values. Also adds a last-resort speed/distance check on extrusion/retraction commands

Signed-off-by: Kieran Eglin kieran.eglin@gmail.com

Comment on lines 118 to 119
const safeAmount = Math.min(amount, this.maxExtrudeLength)
const safeRate = Math.min(rate, this.maxExtrudeSpeed)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic isn't strictly necessary as this.valid does a pretty good job of ensuring correctness before issuing an extrude/retract command. However, I so like this as a guardrail since it allows us to be confident that an extruder command won't exceed machine limits even if there was a logic bug in this form.

Now that I'm typing it out, I'm wondering if Klipper would even respect a command that would exceed its specified limits. I should test that.

At any rate, if Klipper handles this or if it's generally unwanted, I'll remove this portion. Just let me know!

Copy link
Member

@pedrolamas pedrolamas Feb 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that Klipper will indeed complain if the value is above the max_extrude_only_distance (for both extrusion and retraction)

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should not be necessary (to be honest, even the valid check is probably overkill here), so I've gone ahead and reverted that small bit before doing the actual merge

@kieraneglin
Copy link
Contributor Author

I didn't know whether to title this as a feature or a bug fix or something else, so I defaulted to feat

matmen
matmen previously approved these changes Feb 13, 2023
Copy link
Member

@matmen matmen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@pedrolamas pedrolamas changed the title feat: updated tool extrusion settings to respect printer config settings feat: updated tool extrusion settings to respect printer config Feb 14, 2023
@pedrolamas pedrolamas merged commit 2d124cb into fluidd-core:develop Feb 14, 2023
@pedrolamas pedrolamas added the FR - Enhancement New feature or request label Feb 14, 2023
@pedrolamas pedrolamas added this to the 1.23.2 milestone Feb 14, 2023
@kieraneglin kieraneglin deleted the ke/tool-extrude-length-settings-bugfix branch February 14, 2023 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FR - Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to set tool extrude length > 50mm
3 participants