VoronDesign / Voron-Trident

Voron Trident CoreXY 3D Printer design
578 stars 173 forks source link

Remove [SAVE|RESTORE]_GCODE_STATE from PRINT_END macro #136

Closed voidtrance closed 8 months ago

voidtrance commented 8 months ago

The sample configuration for some boards contain PRINT_END macros that include [SAVE|RESTORE]_GCODE_STATE command in them.

Unfortunately, those commands may have a negative effect in that the macro tries to perform some safety moves between them to move the toolhead away from the print. However, when the GCode state is restore, Klipper will move the toolhead to the last position before the GCode state is save. This will negate the safety moves and put the toolhead back where it was.

Fix this by removing the [SAVE|RESTORE]_GCODE_STATE command.

FHeilmann commented 8 months ago

Sorry for the comment create delete but I wanted to confirm my statements after I made them:

The Behavior you describe is exhibited when MOVE=1 is passed to RESTORE_GCODE_STATE according to Klippers documentation https://www.klipper3d.org/G-Codes.html#restore_gcode_state

The default for MOVE is 0 (relevant code line here: https://github.com/Klipper3d/klipper/blob/bee7ec720b1405e8a14ca1fc81e565f0e5ce7aa9/klippy/extras/gcode_move.py#L239)

So I don't think our configs have the issue you describe.

voidtrance commented 8 months ago

Sorry for the comment create delete but I wanted to confirm my statements after I made them:

The Behavior you describe is exhibited when MOVE=1 is passed to RESTORE_GCODE_STATE according to Klippers documentation https://www.klipper3d.org/G-Codes.html#restore_gcode_state

The default for MOVE is 0 (relevant code line here:

https://github.com/Klipper3d/klipper/blob/bee7ec720b1405e8a14ca1fc81e565f0e5ce7aa9/klippy/extras/gcode_move.py#L239)

So I don't think our configs have the issue you describe.

Hi, please see the updated commit message after the first comment.

There was an agreement on the Voron discord that the statements should be removed. My interpretation of their effect was wrong so I've revised the commit message. However, they should be removed nonetheless.

FHeilmann commented 8 months ago

I don't agree here. In-between the save and restore gcodes, multiple settings are changed by the macro. This includes a G90 for absolute positioning and changes to both extruder and movement accelerations. Not using a restore after these gcodes may result in the printer being in a state that the user neither expects nor wants.

Saving and restoring gcode State is a best practice (printer should be in the same state after the macro as it was before) and therefore they should remain.

voidtrance commented 8 months ago

I don't agree here. In-between the save and restore gcodes, multiple settings are changed by the macro. This includes a G90 for absolute positioning and changes to both extruder and movement accelerations. Not using a restore after these gcodes may result in the printer being in a state that the user neither expects nor wants.

Saving and restoring gcode State is a best practice (printer should be in the same state after the macro as it was before) and therefore they should remain.

The above sounds a bit contradictory to me. If the printer is supposed to be in the same state as it was before the macro, I would argue that the restore should include MOVE=1. Otherwise, it's not in the same state. I guess it all comes down to what you call "state".

By Klipper's definition of state (as in the information being saved by SAVE_GCODE_STATE), the position of the toolhead is part of the state. So, if the intent is to restore the state as it was before the macro, then the position should be included, not just the coordinate system.

From my reading of your comments, it sounds to me that "state" is defined only as the coordinate system only.

FHeilmann commented 8 months ago

I don't agree here. In-between the save and restore gcodes, multiple settings are changed by the macro. This includes a G90 for absolute positioning and changes to both extruder and movement accelerations. Not using a restore after these gcodes may result in the printer being in a state that the user neither expects nor wants. Saving and restoring gcode State is a best practice (printer should be in the same state after the macro as it was before) and therefore they should remain.

The above sounds a bit contradictory to me. If the printer is supposed to be in the same state as it was before the macro, I would argue that the restore should include MOVE=1. Otherwise, it's not in the same state. I guess it all comes down to what you call "state".

By Klipper's definition of state (as in the information being saved by SAVE_GCODE_STATE), the position of the toolhead is part of the state. So, if the intent is to restore the state as it was before the macro, then the position should be included, not just the coordinate system.

From my reading of your comments, it sounds to me that "state" is defined only as the coordinate system only.

Let's try to approach this from the other angle. Which issue is going to be solved by removing these lines? The presence of the G90 in the macro is a strong reason for me to not remove it, and unless a compelling reason is made why they should be removed I'm afraid we're not going to make any progress.