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

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

Merged
merged 1 commit into from
Nov 7, 2023

Conversation

voidtrance
Copy link
Contributor

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
Copy link

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
Copy link
Contributor Author

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
Copy link

FHeilmann commented Nov 1, 2023

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
Copy link
Contributor Author

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
Copy link

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.

The PRINT_END macro in the Octopus config file contains a pair of
SAVE_GCODE_STATE/RESTORE_GCODE_STATE command. The purpose of those
commands is to allow the macro to perfom actions at the end of the
print and then restore the printer state as it was prior to the
macro's execution (restore coordinate system and speeds).

However, there was some confusion as to the effect of those calls,
particularly the RESTORE_GCODE_STATE. What is desired is that the
printer state is restored but the toolhead position is not.

To clarify the intention and prevent any issues, the "MOVE=0"
argument to RESTORE_GCODE_STATE is added as to explicitly prevent
any toolhead position changes and a comment is added to describe
the purpose of the commands.

Since having the commands in the PRINT_END macro is something that
is needed, the same commands have also been added to any of the
other configs which were missing it.
@FHeilmann FHeilmann merged commit f90d36c into VoronDesign:main Nov 7, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants