supermerill / SuperSlicer

G-code generator for 3D printers (Prusa, Voron, Creality, etc.)
4.05k stars 515 forks source link

SET_VELOCITY_LIMIT ACCEL_TO_DECEL klipper support added. #4087

Open legend069 opened 5 months ago

legend069 commented 5 months ago

gcode now emits SET_VELOCITY_LIMIT ACCEL ACCEL_TO_DECEL instead of m204 commands with the deceleration value. users can now choose a % of the extrusion roles deceleration value from 1% 10 100% of the extrusions roles acceleration value.

currently klipper's base deceleration values are set in the firmware, if it's at 50k there it will use 50k for all deceleration moves regardless of acceleration value set by m204/SET_VELOCITY_LIMIT.

i'm unsure if "ACCEL_TO_DECEL" is needed for input shaper to work correctly

"work arounds" are using custom_gcode - in between extrusion role changes or a klipper macro "work arounds" are mentioned a fair bit in Voron discord

supermerill commented 5 months ago

Can I change it to have only deceleration_factor, and just deactivate it if 0 ?

btw, I'll remove the "future stuff", better to add in the future, when it's ready.

supermerill commented 5 months ago

Another thing: There is a "use next acceleration for deceleration" setting. I think it's useful to be in sync with it. What it should do:

legend069 commented 5 months ago
  • if deceleration_factor >0, then disable travel_deceleration_use_target (it's done in configmanipulation.cpp)

yeah that makes sense, would help not confusing users with that setting. although i'm wondering if i should add a check in configmanipulation.cpp for firmware, since this new setting is enabled by default at 50% thus disabling travel_deceleration_use_target could upset a few users not using klipper. or should the default value be set based on firmware to avoid this potential issue ?

  • if deceleration_factor >0, and not klipper, then use that instead of next acceleration (it's in gcode.cpp, search for travel_deceleration_use_target)
  • if deceleration_factor >0 and klipper, then don't bother splitting the travel in two pieces.

if (m_config.travel_deceleration_use_target == true && m_config.deceleration_factor == 0) sufficient ? can we assume(hope) other firmwares would adapt this deceleration feature in the future, so we can avoid the check for firmwares here?

supermerill commented 5 months ago

or should the default value be set based on firmware to avoid this potential issue ?

Yes, this may be "better". But there is currently no way to have multiple defaults based on conditions. I can see another way to do it:

Maybe it's a bit complicated? But that way, travel_deceleration_use_target keeps its current behavior in every way and deceleration_factor an be default to a no-0 value, and it can have a use even with travel_deceleration_use_target.

What do you think?

legend069 commented 5 months ago

if travel_deceleration_use_target is set and it's not klipper: disable deceleration_factor

do you mean just change the value instead of disabling the ability for the user to edit the option ? currently in configmanipulation.cpp the gcode flavor doesn't get loaded into memory, so a few more changes would need to be made to get this bit to work? bool have_klipper_support = config->option<ConfigOptionEnum<GCodeFlavor>>("gcode_flavor")->value == gcfKlipper;

i partially want to keep the toggle_field for travel_deceleration_use_target so it will force users to read it's updated tool tip(current one is a little confusing to understand)

supermerill commented 5 months ago

do you mean just change the value instead of disabling the ability for the user to edit the option ?

no, don't change the value but disable the ability to edit the option as in this case, it isn't used.

i partially want to keep the toggle_field for travel_deceleration_use_target so it will force users to read it's updated tool tip(current one is a little confusing to understand)

I don't think it's a good practice. If you really want that, a pop-up at the startup with a "what's new, what you should take care of" is much better.

Let me think of it again What I would prefer, is to "replicate the klipper behavior" as much as possible with other firmwares, at least for travels, to reduce the gap between them, to avoid confusion. That means : . if no travel_deceleration_use_target and no klipper , then still do the compute but with deceleration_factor current_acceleration as the decel speed . if no travel_deceleration_use_target and klipper , then let klipper handle the different decel speed, via your modification. . if travel_deceleration_use_target , then use deceleration_factor next_acceleration (also for klipper)

is it better?

note: I saw that orcaslicer has 'accel_to_decel_enable' and 'accel_to_decel_factor'. Is it the same as your initial proposal?

legend069 commented 5 months ago

What I would prefer, is to "replicate the klipper behavior" as much as possible with other firmwares, at least for travels, to reduce the gap between them, to avoid confusion.

ok i'm with you now! so need to edit it so when it's doing the travel_deceleration_use_target loop it can utilize the deceleration_factor basically lets the user choose the travels deceleration by deceleration_factor of the extrusion roles acceleration value

have i got that right?

essentially improving travel_deceleration_use_target by adding deceleration_factor with it.

note: I saw that orcaslicer has 'accel_to_decel_enable' and 'accel_to_decel_factor'. Is it the same as your initial proposal?

yeah pretty much, with your suggestions regarding it. i feel like it would be an 'improvement' to how it's done in orca it's been on my to-do list for over a year now to add this into the code along with more 'calibration' bits. but one thing at a time ☺

supermerill commented 5 months ago

Yes, exactly. Seems good.

printing it moves to the 'midway' point

Not exactly midway, as it depends of the speeds & acceleration, but I think you already know.

legend069 commented 5 months ago

i thought about splitting the Extrusion roles gcode line so it can be manipulated to have deceleration values, but do we know the firmware can actually support this? since it would endup making .gcode file alot larger

(for a simple straight line ) turning

m204 p1000 t5000 ;
G1 x10 y10 E5

into

m204 p1000 t5000 ; first point acceleration
G1 x 8 y 8 e2.5 ; extrude to 'midway point'
m204 p500 t5000 ; deceleration adjust
G1 x2 y2 e2.5 ;  extrude remaining line
m204 p1000 t5000 ; return acceleration to normal to start another line

for more complex shapes this would result in transforming the simple gcode line to multiple lines that eventually do the same thing. max_gcode_per_second could affect/break the splitting too ?

legend069 commented 4 months ago

there are 2 bugs i've found (1 is fixed) using a model with 3 perimeters

  1. when it sets the acceleration for the 'midway' point it doesn't write the new acceleration value because of the write acceleration function checks for the last acceleration value. it doesn't consider the travel acceleration changes.(fixed)
  2. after it finishes the internal perimeter(0) it starts internal perimeter(1) it moves across to start that perimeter with the default acceleration, the travel acceleration for that move is correct. but i guess this doesn't matter since this move is a 'travel move' and the print shouldn't be affected by it. (not fixed)

klipper acceleration changes are still correct. untested other firmwares but would assume it will be fine.

it's still setting the correct acceleration values before it start extruding the ER role

supermerill commented 4 months ago

thanks. I'll merge it into the 2.7 once the merge is finished.

for more complex shapes this would result in transforming the simple gcode line to multiple lines that eventually do the same thing.

If the user/vendor set the setting, then it's something they are willing to compromise for.

max_gcode_per_second could affect/break the splitting too ?

I don't think so, it's applied before:

legend069 commented 3 months ago

klipper changed this and added Minimum cruise ratio PRl so any config that has been updated if a .gcode file has ACCEL_TO_DECEL it would throw an error.

personally i just want any deceleration moves to be the same as the acceleration moves. i don't think there is any point in live adjustments for this new Minimum cruise ratio i'm considering adding a checkbox to keep the ACCEL_TO_DECEL for the klipper users that don't update as often but then again adding in "support" for old features isn't the best way of doing things.

with the checkbox true it would append SET_VELOCITY_LIMIT ACCEL={xxxx} ACCEL_TO_DECEL={xxxx} with the checkbox false it would append M204 Sxxxx whats your thoughts on this @supermerill

legend069 commented 1 month ago

i'm wanting to add in a klipper box for SCV and Minimum cruise ratio but the GUI would need to be changed to make it look better. adding in the SCV and Minimum cruise ratio to new columns on the right should make it look "nicer"

image