firmware_retraction: Support nozzle lifting on G10

[flopana77]

Summary of this Pull request

Following popular demand, this submission finally brings reliable Z-Hop to Klipper :)

It enables nozzle lifting (zhop) when retracting using G10 command; fully configurable at runtime; in absolute or relative mode.
In addition, the submission introduces three different zhop movement styles (standard, ramp and helix) suitable for different use cases, see below. Clears retraction reliably to prevent nozzle crashes and other unwanted behavior after changes of the printing state (start, cancel or finish prints from virtual SD Card as well as via GCode streaming).
As addon, if also fixes a bug in the implementation as described below.

Below explanation and description is intended to be as complete as possible to facilitate the review process.
I did a self-review of this submission according to the steps in the CONTRIBUTING document and everything looks good to me. The code was also tested intensively, see below, without any issues.

Note: Apologies for the 5 follow up commits. Will do debugging on my dev branch in future :)

Is the submission free of defects and is it ready to be widely deployed?

The following safeguards were implemented, and tests were performed prior to this submission:

• The code was tested intensively using a test macro with a plethora of edge cases (included in the submission):

  • Various checks to see if printing state changes are properly handled.
  • Unretract in unretract state with and without zhop enabled in all three zhop styles, repeated to check repeatability.
  • Retract in retract state with and without zhop enabled in all three zhop styles.
  • Change of z_hop height while retracted.
  • Move Z while retracted including prior switch from absolute to relative mode and back in all three zhop styles.
  • Move XY while retracted in all three zhop styles.
  • For all three zhop styles, move XY while retracted and close enough to Z max to provoke an out-of-range Z move upon retract.

• Real-world test prints were performed including:

  • Impossible print: https://www.printables.com/model/488507-z-hop-based-torture-tests/ to ensure zhop is working and normal prints are not knocked-off the print plate.
  • Benchy with variable layer height between 0.2 and 0.28 mm to ensure that zhop is working with changing layer height as well.
  • Ultra Lattice cube torture test: https://www.thingiverse.com/thing:1850320 to ensure that the FW retraction works flawlessly over many retraction moves, including retractions at layer change.

• The previously identified main failure cases concerning z_hop functionality have been addressed as follows:

  • Failure Case 1: Nozzle crash on detraction, after absolute z move while retracted, as per first example provided by @Arksine in PR1617 on Jun 25, 2019. This was addressed as follows:
    • Fixed by registering new G1 and G0 commands as per second comment on Jul 11, 2019 in Issue1787 from @KevinOConnor .
    • Compared to PR4998, instead of toggling the zhop value, while retracted, G1 Z move commands are altered to account for the zhop. This makes it compatible with configs that themselves redefine G1 and G0.
    • It was tested, while retracted and once unretracted again, that any additional movement commands one may add to a macro-re-named G1 command still execute properly.
    • Z-Offset from a macro-re-named G1 command was also respected during tests as well as offsets and custom movements combined. This makes sense given that the code does not mess with offsets but instead changes the coordinates of G1 commands while retracted.
    • To address the comment from @FHeilmann on Mar 16, 2022 that redefining G0 and G1 could break bigger configs, tests show that this should not happen. As an additional safety measure, G1 and G0 handlers are only swapped if z_hop_height is greater zero, meaning z_hop is enabled. Users that renamed their G1 and G0 commands won’t be affected upon updating, given that the standard z-hop value is set to zero, keeping standard behavior. Users will therefore have to consciously enable FW retraction and test out the waters safely to see if FW retraction has any compatibility issues with their macros. If so, which is unlikely, users can simply disable FW zhop.
  • Failure Case 2: If the slicer issues a retract command at the end of a print or if a print is cancelled while retracted, the printer will be in retract mode. This may be an issue once a new print is started as the initial G10 command of the new print is ignored and a subsequent G11 command may result in a nozzle crash due to zhop down from the old print. This scenario is the second example provided by Arksine in PR1617 on Jun 25, 2019. This was addressed as follows:
    • First, a distinction must be made between printing via gcode streaming (default with OctoPrint) or via virtual SD Card (default with Mainsail, Fluidd, DWC2 for Klipper and also available with OctoPrint).
    • For gcode streaming, the stepper disable event is useful as OctoPrint disables steppers on cancel. To detect a starting print via streaming, the homing event is useful. If the user adds homing to the start gcode (which is standard) and a disable-stepper command to the end gcode (which is also standard), these two events reliably reset the firmware retraction before a new print starts via streaming.
    • When printing via virtual SD card, the reset-SD-file event allows detecting a new print starting given that the reset file method is called every time a print is started from virtual SD Card. To detect canceled and finished prints, besides the already mentioned disable stepper event, starting-, paused-, cancelled- and finished-print events were added to the print_stat module. These four events reliably reset the firmware retraction when a print ends, is canceled or before a new print starts via virtual SD Card and are an additional layer of redundancy to the beforementioned events.
    • Instead of adding new events, the corresponding state variables could be polled but the overhead of events is smaller and therefore this strategy was chosen.

• Additional failure cases concerning z_hop functionality that had not been identified so far have been addressed as follows:

  • Failure Case 3: G10 is issued before homing. This could set retract state and could lead to nozzle crashes for the same reason as in the finished/cancelled print case. This was addressed by adding a condition that all axes must be homed before G10 can be executed.
  • Failure Case 4: G10 is issued with a temperature lower than minimum extrude temperature. This would trigger an error but still set retract state. The issue was addressed by adding a check if the retract extrude move is possible.
  • Failure Case 5: SET_RETRACTION is called while in retract state and z_hop_height is increased. Although the probability of this case is low, it may lead to unwanted nozzle crash when unretracting. Addressed as described in Failure case 8 below.
  • Failure Case 6: A G10 command is issued very close to the maximum range of the Z axis, which may result in an out-of-range move and stop the print. This was addressed by adjusting the z_hop height depending on the z coordinate at the time the G10 command is issued. In the extreme case that the z coordinate is at z_max, z_hop height is reduced to zero.
  • Failure Case 7: The printer is in relative mode or was switched to relative mode while in retract state. This may lead to unpredictable z_hop moves. This failure case was addressed by checking the relative/absolute move state of the printer and either using an absolute Z coordinate of the z_hop move (in absolute mode) or the safe z_hop height (in relative mode).

• Additional failure cases concerning extruder moves that had not been identified so far have been addressed as follows:

  • Failure Case 8: SET_RETRACTION is called while in retract state. Although the probability of this case is low, this will clear the retract state, which may lead to local non-extrusion as the required unretract filament move is not done. Failure case 5 and 8 have been addresses by queuing SET_RETRACTION commands issued in retract state and executing them after the corresponding unretract command. Fun addon, queued commands can be listed using the GET_RETRACTION command.

• The code is fully commented to allow easy understanding and review. Comments can be removed before merge, if required.

• Besides Python modules (firmware_retraction.py and print_stats.py), Status_Reference.md, G-Codes.md and Config_Reference.md were revised to account for the additional functionality and make it easily accessible to the User.

• No code is commented out, all to-do references and comments on past implementations have been removed.

• Regression test macros and configs were included (therefore the additional commits...) to test behavior with and without Virtual SD Card module.

Does the submission provide a “high impact” benefit to real-world users performing real-world tasks?

• Who is the target audience and what size does it have?

  • The estimated audience size is larger than the 100-user threshold based on the comments of the community in:
    • Youtube: (446) Firmware retraction: Why you should try it out - YouTube --> 84k views on the video and approx. 150 comments. It is noted that the video negatively points out Klipper’s lack of support for zhop.
    • Discourse: Firmware retraction Z-HOP - Macros - Klipper --> The thread received over 5.4k views and is the fourth highest viewed thread in the Klipper Discourse.
    • Github PR#4998 (firmware_retraction: Reintroduce support for lifting Z on retract by wlhlm · Pull Request #4998 · Klipper3d/klipper (github.com)) --> Several users mentioned that this feature would be worth having implemented in Python rather than as macro to gain performance.

• What is the benefit of the contribution to the community?

  • SAFER PRINTING USING FIRMWARE RETRACTION WITH ZHOP
    As a result of this unsatisfied user need, macro implementations of zhop are being used by the community. These are prone to cause unwanted behavior, which may damage printers running Klipper firmware, even though the Klipper firmware is not to blame:

    • Numerous users have implemented or are using implementations of zhop via macros (e.g. Firmware retraction Z-HOP - Macros - Klipper).
    • These implementations typically only address Failure case 1. Most, if not all, other failure cases, especially not clearing retract state at print end or cancel to ensure that retraction works without issue when starting a new print, are not being addressed in macro implementations given that events cannot be easily accessed by macros.
    • Except for failure cases 5 and 8 which would be very difficult to implement as macro, the remaining failure cases could, but typically are not addresses in macro implementations.

  • LESS RESOURCE INTENSIVE THAN MACRO IMPLEMENTATIONS PREVENTS UNNECESSARY CPU LOAD

    • Macro implementations for z_hop functionality are available in the community. These implementations do not have to comply with quality standards and may lead to unwanted and hard to diagnose behavior.
    • Typically, macro implementations permanently overwrite G0/G1 commands as macros cannot be defined at runtime. The overwritten G0/G1 commands check every move on the retraction state using the jinja2 framework, which is resource intensive.
    • This additional CPU load may be required by other threads and could therefore cause unwanted behavior which would be almost impossible to diagnose.
    • Besides, in contrast to Macro implementations, the Python module implementation follows the recommendations in the Code Overview, which states that the “module system is the preferred method for adding new functionality to Klipper”.

  • THIS PULL REQUEST INCLUDES NEW Z_HOP MOVE STYLES, NAMELY RAMP ZHOP AND HELIX ZHOP AS WELL AS M101 AND M103 ALIASES USED BY SIMPLIFY3D

    • Two new z_hop move styles are implemented and thus the user has three z_hop move options:
      • “Standard”: This is the well-known and widely used vertical z_hop movement.
      • “Helix”: Helix combines the vertical lift movement with a circular movement of the nozzle. This increases the effective move length during nozzle lifting and therefore results thinner strings that break faster. It may therefore be beneficial for oozy filaments, reduces retraction length (see below) and is therefore the standard setting in BambuStudio.
      • “Ramp”: Ramp does not immediately cause the nozzle to lift vertically upon retraction but instead adds corresponding vertical movement component to the travel move following the retraction. This was proposed by YouTuber teachingtech as it allows reducing the total retract-travel-unretract motion system move length a tiny bit and may have slight print time reduction benefits, while maintaining or even improving stringing.
    • M103 and M101 aliases were implemented to support Simplify3D slicer as mentioned by BlackStump on Jun 1, 2019 in PR1617.

  • FIRMWARE RETRACTION INCLUDING ZHOP SIMPLIFIES THE OVERALL WORKFLOW

    • When changing e.g., the hot end or nozzle, retraction parameters can be easily updated with the current implementation as such changes usually only concern z-hop height, retraction length and speed.
    • Changes to the cooling may result in changed behavior of overhangs (curling), which can also be addressed with the proposed implementation without need for re-slicing.
    • Print farms with different printers can benefit greatly from a single-slicing workflow by reducing the time needed until a print is released to several different printers, each with their individual retraction setting including optimized zhop. In addition, errors of mistakenly using the “wrong” gcode for any given printer are reduced, thus saving cost.

  • FLEXIBILITY TO CHANGE NOT ONLY RETRACTION PARAMETERS BUT ALSO ZHOP HEIGHT AND ZHOP STYLE AT RUNTIME

    • Sometimes, prints tend to curl up during printing, especially on edges. By being able to change zhop parameter, it can be prevented to hit curled up edges which may otherwise knock the print off the build plate eventually. Hence, enabling zhop in Klipper increases print reliability and success rates, especially in challenging prints.
    • The proposed implementation enables zhop reliably, while maintaining the other benefits of firmware retraction such as change of other retraction parameters at runtime to improve prints (e.g. wet filament may require more retraction).
    • Including zhop allows the use of tuning towers to tune all retraction parameters without need of re-slicing the corresponding model.

  • REAL-WORLD PRINTS SHOWED PERFORMANCE BENEFITS USING FIRMWARE RETRACTION WITH Z_HOP RATHER THAN SLICER RETRACTION
    Note that all prints were carried out with a bowder extruder with long tube (over 600mm) to amplify stringing issues.

    • Retraction tower:

      • Slicer retraction: Minimum retraction 3mm, above the minimum retraction stringing reoccurred, time to print was 21:17 minutes.
        

      • FW Retraction “Standard”: Minimum retraction 3mm, above the minimum retraction stringing reoccurred but less than with slicer retraction due to higher acceleration set, time to print was 21:15 minutes.
        

      • FW Retraction “Ramp”: Minimum retraction 3mm, above the minimum retraction stringing reoccurred but considerably less than with slicer retraction due to higher acceleration set and diagonal move that prevents string formation during vertical movement before the horizontal travel move, time to print was 21:05 minutes.
        

      • FW Retraction “Helix”: Minimum retraction 2.5mm and hence less than all others, above the minimum retraction stringing did not reoccur, time to print was 22:11 minutes.
        

      • It is concluded that the FW Retraction with “Standard” and “Ramp” move is faster to print in most cases with slightly improved stringing. FW Retraction with “Helix” move is slightly slower but delivers superior stringing behavior. The shorter filament retraction moves further helps the longevity of the extruder, bowden tubes and couplings.

    • The Impossible print:

      • Slicer retraction: The print failed on several objects shortly before completion. Setting the optimal 3mm retraction length still resulted in visible stringing.
        

      • FW Retraction “Ramp”: The print failed on only one object shortly before completion. Setting the optimal 3mm retraction length resulted in noticeably less visible stringing compared to Slicer retraction.
        

      • It is concluded that FW Retraction with “Ramp” may be less prone to knocking prints off the build plate than Slicer retraction at comparable print times (sorry, my timer died in the middle of the ramp print…from Mainsail history, the print times are 20 sec apart, however they started at different bed temperatures).

    • Benchy (not super high speed…):

      • Slicer retraction: Print completed successfully. With optimal retraction length, limited stringing is visible. Print time was 38:55 minutes.
        

      • FW Retraction “Standard”: Print completed successfully. With optimal retraction length, limited stringing is visible, slightly less than Slicer retraction. Print time was 38:40 minutes.
        

      • It is concluded that FW Retraction slightly improves print times at the same quality as slicer retraction.

    • Lattice Cube:

      • Slicer retraction: Print completed successfully. With the optimal retraction length of 3mm, noticeable fine stringing is visible. Print time was 3 hours and 36:10 minutes.
        
        

      • FW Retraction “Helix”: Over 11800 retraction and zhop moves were performed by the firmware without issues. These include 571 retraction moves on layer change, which were previously the root cause of Failure Case 1. With the optimal retraction length of 2.5mm, fine stringing is still visible, however to a much lesser extent than with slicer retraction making for noticeably improved print quality. Print time was 3 hours and 38:11 minutes and thus comparable.
        
        

      • It is concluded that FW retraction with Helix move zhop improves print quality at the cost of minimal print time increase. A potential future improvement could be to combine helix and ramp movement to achieve better print quality without incurring minimally increased print times.

• Does the submission burden users with options they cannot reasonably configure?

  • No, current configs with [firmware_retraction] enabled will continue to work without issue given that all zhop related parameters are optional and defaults are set to disable zhop.

Is the copyright of the submission clear, non-gratuitous, and compatible?

Yes. Copyright was declared on the firmware_retraction.py file due to a basically complete re-write. No code was taken from 3rd party sources. The submission is signed-off as required.

Does the submission follow guidelines specified in the Klipper documentation?

Yes, the guidelines established in the Code_Overview.md regarding adding a host module have been followed.

Is the Klipper documentation updated to reflect new changes?

Yes:

• All changed commands (SET_RETRACTION and GET_RETRACTION) as well as the new CLEAR_RETRACTION command have been documented in the G-Codes.md file.
• The Config_Reference.md has been updated to include all new settings (z_hop_height, z_hop_style, verbose and config_params_on_clear).
• The Status_Reference.md has been updated to include unretract_length, z_hop_height, z_hop_style and the retract_state.
• No new webhooks were defined and therefore the API_Server.md was not edited.
• All changes are Backward-compatible and therefore the Config_Chages.md was not edited.
• No new documents were added and therefore, the Overview.md was not edited.

I hope you find this contribution worthy to get merged and look forward to any questions that may arise :)

[Matszwe02]

docs/Config_Reference.md has z_hop_style has a typo: uppercase vs lowercase z hop styles

[flopana77]

Hi Matszwe02. Thanks for your comment. Could you elaborate please. I am not sure if I understand correctly, given that the upper and lower case in the files is consistent from what I see. BR Florian

[Matszwe02]

Hi Matszwe02. Thanks for your comment. Could you elaborate please. I am not sure if I understand correctly, given that the upper and lower case in the files is consistent from what I see. BR Florian

z_hop_style: standard
The type of lifting movement of the nozzle if zhop moves are
enabled. Supported move types are standard, helix and ramp.
'Standard' corresponds to a simple vertical lift movement. 'Helix'
combines the vertical lift movement with a circular movement of
the nozzle. This increases the effective move length during nozzle
lifting and therefore produces thinner strings that break faster. It
may therefore be beneficial for oozy filaments and is the standard
setting in BambuStudio. 'Ramp' does not immediately cause the
nozzle to lift vertically upon retraction but instead adds the
corresponding vertical movement component to the travel move
following the retraction. This reduces overall travel distance and
may reduce print time slightly. The default value is 'standard'.

Here I marked the text. Not significant, but I had to think a bit before setting it to helix. Also, I made a simple fix for retraction at bed edges, you could check my PR :)

[flopana77]

I see. Good catch. Will fix it now. Regarding PR, see you over on my repo :)

[Zeanon]

I have one question/idea:
Wouldn't it be easier/better to code the retraction moves directly instead of calling G commands and modifying G1?

[flopana77]

Hi Zeanon. Thanks for your feedback. I am not quite sure what you mean by "code(ing) the retraction moves directly". Looking at the G2 and G3 command implementation, the same approach was taken, meaning G1 commands are called. This has the benefit that all the safety features built into Klipper are kept working. Hence, I believe that the Maintainers would likely consider a direct coding approach as less safe.
On the second part of your feedback regarding the modification of the g1 command, it actually is not. Instead, the command handlers are swapped to basically wrap the standard G1 command with an altered one. This wrapped G1 command then adds the zhop offset to each move while retracted, which allows respecting z moves while retracted. The reason I went with this approach is because I didn't want to touch GCode offsets, which some users may use in their own macros...Cheers Florian

[Zeanon]

Thanks for your in depth reply, that makes sense of course

[flopana77]

I am happy about any Likes you leave :p

[KevinOConnor]

Alas, I left a review last week, but it seems github lost my comments. :-(

I'll try again.

I have a few high level comments on this PR:

1. In general, it seems fine to add z hop support to the firmware_retraction module.
2. I think the actions taken on each G10/G11 may need to be simplified. It's not immediately clear to me when this code will raise the Z height (and to what), and I fear it wont be transparent to users as well.
3. This PR looks like a "rewrite" of the existing firmware_retraction module. These types of "big bang" changes are hard to manage. I think this PR should be split up.

To elaborate on simplification of G10/G11 - if I were to configure a z hop retraction of 10mm, I'd expect the toolhead to be raised by 10mm each time a G10 command is issued. If I understanding the comments and code here correctly, issuing a G10 command on this PR may raise the toolhead by 10mm, may not raise the toolhead at all, or may raise the toolhead by some other amount. It's important to me that Klipper always take the requested action or report an error (if the requested action is clearly not reasonable). It is important to me that Klipper never attempts to "guess what the user really wants".

I think it would help if there was a high-level description of what the G10 and G11 commands are "supposed to do", along with a summary of the various situations where those commands can't reasonably perform the requested action. I think that would help me review, and I think it would be ultimately helpful for users as well. As above, if these command can't perform their intended action then I think the code needs to report an error. Ultimately, if we conclude that G10/G11 would raise too many cryptic errors to be useful, then we may need to consider that z hop is better implemented in a slicer.

To elaborate on the "module rewrite" - in my experience, code "rewrites" tend to increase the risks of introducing a regression. It also increases maintenance work should a regression be introduced as it is harder to identify the specific change using git bisect and harder to revert with git revert (as some users may depend on the old behavior and some users may depend on the new options). For example, this code changes G10/G11 to call SET_VELOCITY_LIMIT and G17, and it is not clear to me if that may break existing setups.

I think the helix and ramp options should be separated into a different PR. In addition to the complexity they introduce, it's not clear to me if these options would be considered "experimental". Therefore I'm not sure if they are a good match for the master Klipper repository.

I also have a few minor comments. My main focus is on my comments above however. In no particular order:

I appreciate your detailed print tests. It's not clear to me if you are reporting that Klipper based z hop is improving quality in a way that a slicer could not reasonable accomplish? Or, perhaps you are reporting that not all slicers implement helix/ramp z hop, or not all slicers have z hop enabled? In the latter cases, an alternative to firmware z hop may be to expand z hop in slicers.

I don't think we should add "debugging options" (verbose) to the printer.cfg file. In general I don't think regular users can reasonably set these options, and I think developers can just as easily modify the python code if they need to perform debugging.

It isn't clear to me what the config_params_on_clear option does.

I think it would be preferable to add an option to the existing SET_RETRACTION command instead of adding a new CLEAR_RETRACTION command. I prefer to limit the number of new commands because I fear each new command increases the risk of a conflict with a user macro. Indeed, the existing GET_RETRACTION command is a left-over - going forward we would report state from SET_RETRACTION.

I don't think we should add M101 and M103 wrappers. There are just too many goofy legacy g-codes. If a user is using a less common slicer setup then I think it is reasonable for them to add macros for that slicer.

Thanks for working on this,
-Kevin

[flopana77]

Hi Kevin. Thank you for taking the time to review this PR! Please find my comments/answers/thoughts below:

I think the actions taken on each G10/G11 may need to be simplified. It's not immediately clear to me when this code will raise the Z height (and to what), and I fear it wont be transparent to users as well.

On a G10 command, the Z height is raised by the value specified for the z_hop_height parameter of the module in the Config.
If this parameter is not provided, the standard value is cero and hence, the Z height is not raised by default.
There is only one case in which the Z height is not raised by the specified value. This is, if the zhop move would go out of range. As you can see in below figure (at least Slic3r-based) slicers do not respect the maximum Z height of the printer and actually put illegal moves in the gcode (in fact, for helix moves, Orca Slicer and BambuStudio do not even respect X and Y build volume limits...I had a few prints, which had extrusion moves close to the build volume limits, stopping because of this). To prevent out of range moves ordered by the slicer, I added code that adresses this edge case and limits the Z height, to which G10 commands may raise, to the maximum Z height of the printer.

This PR looks like a "rewrite" of the existing firmware_retraction module. These types of "big bang" changes are hard to manage. I think this PR should be split up.

The PR is indeed a complete rewrite with little to nothing left of the previous code. Splitting up the PR is somewhat tricky. Having "ramp" and "helix" zhop move functionality separated out is doable. All additional safety measures over and above reliably clearing retraction on start, finish and cancel prints (e.g. not fully raising toolhead if the move would go out if range) can also be submitted separately.
However, I think that even the barebones PR, introducing only "standard" zhop with reliable retraction clearing and handler swapping, will be substantial. In addition, all other PRs would be built on it. Hence, I am not sure if this is the way to go and would very much appreciate your guidance on this.
An alternative option could be to separate FW retraction and zhop in two distinct modules. Similar to "virtual_sdcard" module, which auto loads the "print_stats" module, the new zhop enabling module (let's call it firmware_zhop) would auto-load the "firmware_retraction" module. This may make maintaining the code easier.

To elaborate on simplification of G10/G11 - if I were to configure a z hop retraction of 10mm, I'd expect the toolhead to be raised by 10mm each time a G10 command is issued. If I understanding the comments and code here correctly, issuing a G10 command on this PR may raise the toolhead by 10mm, may not raise the toolhead at all, or may raise the toolhead by some other amount. It's important to me that Klipper always take the requested action or report an error (if the requested action is clearly not reasonable). It is important to me that Klipper never attempts to "guess what the user really wants".

The code raises the toolhead immediately to the specified value except for the following two cases:

1. Raising the toolhead would lead to an out-of-range move. I added this feature because it would be incredibly painful to users to see a print fail close to the maximum z height because of a zhop move. I admit that I didn't document this properly and will gladly add this into the config reference.
2. The user has chosen "ramp" as zhop move style. This move style is called "slope" in the BambuStudio-based Orca Slicer. It basically combines the movement of raising the toolhead with the following travel move. Hence, if the user selects this move style, he should expect the Z height movement happening together with the travel movement. This behavior is documented in the config reference already.

Hence, from my perspective, the behavior is 100% predictable and does not make guesses about user wants.

I think it would help if there was a high-level description of what the G10 and G11 commands are "supposed to do", along with a summary of the various situations where those commands can't reasonably perform the requested action. I think that would help me review, and I think it would be ultimately helpful for users as well.

I absolutely agree that a more detailed description of the functionality including exception and safety features would make a lot of sense. I'll gladly write up a document, including pictures to ensure the user fully understands the functionality of the code. The reason I didn't do it yet was because I wasn't sure if adding a separate document to Klipper's documentation, dealing with retraction and zhop, would be acceptable/encouraged.

As above, if these command can't perform their intended action then I think the code needs to report an error.

I am happy to raise an error to make the user aware of any situation were a zhop move could not be performed as intended. However, such error should not stop an ongoing print as this would defeat the purpose of adding the code to prevent out-of-range moves, which actually do stop ongoing prints.

Ultimately, if we conclude that G10/G11 would raise too many cryptic errors to be useful, then we may need to consider that z hop is better implemented in a slicer.

So far during my usages, the number of situations in which an error would have been raised was cero (hence, I may have been overcautious regarding all sorts of edge cases...but then again, better safe than sorry).

Regarding the implementation in the slicer (as is standard now), I would like to make a general comment and maybe spawn a wider discussion. As per my view, the 3D printing workflow involves three software tools in general: 3D CAD software, Slicer and Firmware. Each of these tools has "natural" access to certain information:

1. 3D CAD software is used to create objects to the liking of the user. At the time of creation of a 3D object, the 3D CAD software obviously has access to the dimensions of the object as the user needs to specify them. Hence, this is where dimensions of 3D objects should be managed.
2. The slicer than takes the object and computes the toolhead paths. At the time of generating the toolhead paths, the slicer obviously has access to the quality and degree of detail the user wants to achieve when printing the object. This is because the user will choose a certain print setting (layer height, wall count and thickness, infill, etc.). Hence, this is where anything toolpath-related should be managed.
3. The firmware takes the generated GCode and makes the printer act as the perfect extrusion system that the slicer assumed when generating the toolpaths required to print the object with the dimensions and the quality the user has chosen beforehand. To do this, the firmware compensates for mechanical imperfections of the printer (e.g., bed warping via bed mesh, skew of the printer frame, gantry tilt, resonance frequencies via input shaper, etc.) and specific characteristics of the loaded filament (e.g., pressure advance). The information regarding the properties of the printer, including e.g., installed nozzle, and the loaded filament are ultimately known at the time of printing. This point in time does not necessarily have to coincide with the moment in time when the user slices the 3D object to be printed. Hence, the firmware should be where anything printer-and filament-related should be managed.

Especially the last point is where the workflow is currently flawed. If the user fails to provide the slicer with the correct properties the printer will have at the time of printing (e.g. loaded filament, installed nozzle), the print will likely fail. This is because currently the interface between the slicer and the printer is managed by the user and not automatically. There are two ways to address this issue:

1. The firmware communicates with the slicer. Bambu Studio managed to get this done for their specific hard- and software eco system, which is (somewhat) closed source. Generalizing this to work for any printer out there is probably very tough.
2. The firmware acts as the perfect extrusion system the slicer assumed and ensures that the prevalent conditions of the printer at the time of printing are considered accordingly. This is partially implemented already, given that e.g., bed warping is compensated via bed mesh, viscosity of the loaded filament is compensated for with pressure advance, etc.

Following this logic, retraction, zhop, wiping, thresholds when retraction is done, is nothing else than a compensation mechanism for oozing. Hence, just like bed mesh, pressure advance and input shaping, I see this functionality best located in the firmware, given that the information which filament is loaded and what printer is executing the generated gcode is only known with certainty once the "print" button is pushed. The same goes for extrusion, bed and chamber temperatures, maximum extrusion rates, minimum layer time, fan speeds, etc. To implement this, the firmware needs to know what kind of moves it is doing, which is information that the slicer must (and can) provide. This also involves some additional computations of the firmware at runtime and is therefore not easy to achieve if we want to ensure that Klipper remains the fastest firmware out there (I am working on parallelizing the gcode reading and the command execution to achieve exactly that). With enough time at hand, and sufficient information passed from the slicer, pretty much all filament and printer characteristics-related settings can be moved from the slicer to the firmware, thus eliminating the need to synchronize information between those two tools. I am curious about your and the community's thoughts on this.

To elaborate on the "module rewrite" - in my experience, code "rewrites" tend to increase the risks of introducing a regression. It also increases maintenance work should a regression be introduced as it is harder to identify the specific change using git bisect and harder to revert with git revert (as some users may depend on the old behavior and some users may depend on the new options). For example, this code changes G10/G11 to call SET_VELOCITY_LIMIT and G17, and it is not clear to me if that may break existing setups.

I understand that incremental, smaller changes to the codebase might be preferable to a complete rewrite, because they are easier to manage, debug, and revert if necessary. This approach is definitely less disruptive for end users as well. Therefore, I believe that splitting out the zhop functionality in a separate module would probably be the best solution. Your guidance on this is highly appreciated. In any case, I paid a lot of attention to ensure that the module behaves exactly the same as the previous code as long as the user does not deliberately change his config.

I think the helix and ramp options should be separated into a different PR. In addition to the complexity they introduce, it's not clear to me if these options would be considered "experimental". Therefore I'm not sure if they are a good match for the master Klipper repository.

"helix" and "ramp" zhop move styles are available in Orca Slicer and BambuStudio. "helix" (called "Spiral" in mentioned slicers) is slightly slower than standard zhop but prevents stringing quite effectively. For CoreXY printers, this is the way to go in my opinion. "ramp" (called "Slope" in mentioned slicers), is faster than standard zhop and also reduces stringing compared to standard, however less than "helix". This is the way to go for bed slingers imo. Neither of those zhop styles is considered experimental in the slicers and I therefore believe that firmware zhop should include these options. In fact, I'd rather see standard zhop excluded because it is definitely the inferior option to "ramp" and "helix".

I appreciate your detailed print tests. It's not clear to me if you are reporting that Klipper based z hop is improving quality in a way that a slicer could not reasonable accomplish?

Yes, that was a lot of fun. My daughter loves the lattice cubes torture test :) To some extent, your conclusion is right. AFAIK, current slicer implementations do not change acceleration settings for the retraction and zhop moves. Having access to the maximum values allowed on the printer, I included the change of these settings for the firmware retraction moves. That's why the results with FW retraction are (slightly) better than with slicer retraction. Of course, this could be implemented on the slicer. However, this would require the user to input the correct allowed accelerations in the slicer...And then we end up with above discussion again: which settings should be on the slicer and which ones on the firmware.
Just as a thought experiment: Is there any reason why bed mesh and pressure advance should not rather be implemented in the slicer than the firmware? Ultimately, we are talking about transferring a few measured numbers to the slicer, which could then calculate the move transformations and issue the corresponding gcode commands. I remember the "coasting" setting in Cura which tried to achieve something similar to pressure advance. Ultimately, the community somehow decided that making sure that the printer lays down plastic as intended is a task better taken care of by the firmware. That said, I don't see a difference between bed mesh, pressure advance, retraction, wiping, zhop, etc. All of these actions are performed by the printer aiming at making sure that plastic is layed down as expected and I therefore believe they have a legitimate reason to be taken care of by the firmware.

Or, perhaps you are reporting that not all slicers implement helix/ramp z hop, or not all slicers have z hop enabled? In the latter cases, an alternative to firmware z hop may be to expand z hop in slicers.

This is correct, Currently only BambuStudio and Orca Slicer offer helix and ramp. For the reasons mentioned above, I think the firmware is the best place to offer this functionality to the user.

I don't think we should add "debugging options" (verbose) to the printer.cfg file. In general I don't think regular users can reasonably set these options, and I think developers can just as easily modify the python code if they need to perform debugging.

Understood. In many cases, the verbose setting issues messages to the user, which should be changed to errors anyways. I am happy to change this accordingly.

It isn't clear to me what the config_params_on_clear option does.

The user may change retraction settings at runtime during a print. If the retract state is cleared (e.g. at the end of a print or after canceling a print), the question arises whether or not the changes made at runtime, using the SET_RETRACTION command, shall be kept for the next print or not. If the config_params_on_clear is set to FALSE, changes are kept and vice versa. The Standard value is TRUE. I believe that the user should have the possibility to make this choice himself.

I think it would be preferable to add an option to the existing SET_RETRACTION command instead of adding a new CLEAR_RETRACTION command. I prefer to limit the number of new commands because I fear each new command increases the risk of a conflict with a user macro. Indeed, the existing GET_RETRACTION command is a left-over - going forward we would report state from SET_RETRACTION.

I volunteer to implement this :)

I don't think we should add M101 and M103 wrappers. There are just too many goofy legacy g-codes. If a user is using a less common slicer setup then I think it is reasonable for them to add macros for that slicer.

No issue, this is in fact a perfect case to use Macros. I'll remove accordingly.

Thanks for working on this,

It is my sincere honor. Klipper is a great firmware and project. I have used Klipper pretty much since my first day in the 3D printing. The potential of the architecture is tremendous, and I really believe that we should make full use of it. Ultimately, it is this great architecture which can enable really smart printers. My contribution is a push in this direction, and I therefore hope that if finds your and the community's approval.

Cheers,
Florian

[KevinOConnor]

I would like to make a general comment and maybe spawn a wider discussion. As per my view, the 3D printing workflow involves three software tools in general: 3D CAD software, Slicer and Firmware.

For what it is worth, from a user perspective I think there would ideally be only two pieces of software: the cad software, and the 3d printer. From a user perspective I think having a separate "slicer" step is rather bizarre. Consider a 2d-printer, would it make sense for users to open a file in a word processor, then save it to some intermediate file, open it in some "page layout program", and then send it to their ink printer as the final step? I much prefer just hitting "print" and getting a nice printout of what was on my screen. Alas, we have the separate "slicer" step as a legacy of 3d printing.

From a developer point of view, to me, the "slicer" is the software that represents the "planning" stage. (It would have been preferable if that software was a "library" of sorts, or otherwise hidden from the overall user experience, but the "planning" software is still needed.) That "slicing" software takes an object and produces a series of discrete steps, that in total will result in the creation of the requested object. The 3d printer firmware (Klipper) is tasked with executing each of those individual steps reliably. That is, in general, Klipper doesn't know the "why" of each step, nor what the future steps will be, nor what all the past steps were - it primarily focuses on performing each discrete step, mainly one step at a time.

That is why it is important that Klipper always execute each step faithfully, or report an error. It can't make guesses on what actions to take on each step, because failure to follow the steps will throw off the whole plan. Beyond the philosophical, in practice it leads to terrible results. Users trying to figure out why their prints are coming out with low quality and no clues to explain why the printer is behaving erratic.

Is there any reason why bed mesh and pressure advance should not rather be implemented in the slicer than the firmware?

I see bed_mesh implemented in Klipper because it improves its ability to faithfully move from one coordinate to anther coordinate. The slicer is not tasked with moving each motor - it's tasked with specifying a move from location to another in cartesian coordinates (relative to the bed). We know that in some setups the bed is not flat, so the firmware can compensate for that and better accomplish the requested movement step. That is, it can faithfully move the toolhead horizontally (relative to the bed) as it was requested to do.

Similarly with pressure advance - the slicer is requesting a move with a particular volume (or cross sectional area) of filament. Klipper implements pressure advance so that it can better accomplish each extrusion step. That is, it can adjust the extruder stepper motor signals so that there is a more consistent volume at each point along the requested extrusion movement.

The code raises the toolhead immediately to the specified value except for the following two cases: ... Raising the toolhead would lead to an out-of-range move.

That case seems to me to be "guessing what the user really wants". It is important to me that Klipper not do that. If the user made a request to take a particular action then Klipper should either take that action or report an error.

The user has chosen "ramp" as zhop move style.

For what it is worth, this seems quirky to me. If I understand you correctly it would set a state in Klipper such that it would change the behavior of future movement commands. That seems like it could lead to a lot of confusion and I'm not sure it is a good fit for Klipper.

Thanks again for working on this. As high-level feedback, though, I think it is worth pointing out that I don't personally think of z hop as a particularly compelling feature. I don't use z hop on any of my printers. If I give advice to new users, my advice is to not enable z hop. I've found it leads to blobbing, zits, increased ooze, and longer print times. With good retraction and pressure advance, I've found that z hop isn't needed on my printers. I'm also not aware of users clamouring for improved z hop support. I understand every printer is different, some exotic filaments need particular settings, and there are certainly good optimizations I'm not aware of. I only point this out because we seem to be having some pretty lengthy high-level discussions on what I would consider a "niche feature".

I'll try to give some thought on how we could improve Klipper so that more features like the above could be implemented without having to go through a lengthy code review process. It's clear that z hop is too complex for macros, but maybe Klipper could be enhanced in some other way to make it more modular. I'll give it some thought.

Thanks again. I appreciate your contributions.
-Kevin

[pedrolamas]

Personally, I do see value in z-hop, especially in slower printers as it avoids scarring of surfaces while the hot nozzle moves above them.

I normally use a 0.2 or 0.4 z-hop as I found that to be enough to get a perfect smooth print surface.

[gandy92]

Please let my add my 2ct from my very personal user perspective.

Since changing over from Marlin, I've learned to value most of Klippers functionality and the versatility of the macro language, but the one thing I was always missing is a decent implementation of z-hop. I've tested and quickly abandoned some of the publicly available attempts to implement this in macros for basically the same reasons @flopana77 mentions in their motivation to create this PR in the first place.

I understand that there are reasons not to use z-hop, and there may be combinations of printers, materials and slicing skills that can do perfectly without. But then, the same may hold true for a number of other Klipper features that are highly valued by the part of the community that needs them. Personally, I see the value in z-hop as a firmware feature and I'd highly appreciate this PR to be accepted rather sooner than later. My personal philosophy -and I'm far from getting there- is to let the slicer produce gcode that can work with different types of materials - some of which may require z-hop, maybe depending on their age. Also, being able to dial in the z-hop settings with the calibration capabilities of Klipper or changing them midprint where necessary would be a huge benefit.

Regarding the different styles of z-hop as proposed by this PR: I'm not quite seeing the point as to how there would be any guessing involved: As the user, I decide if I want the head to raise immediately (traditional z-hop) or avoid the chances for blobs by raising and lowering while moving to the new position (z-hop with ramp). In both cases, by choosing the z-hop mode, I know how the printer will move, even when I tell Klipper to not move directly from the position of retract to the next position, by choosing spiral mode. For me as a user, this while affair appears to be no more quirky (as in "not quirky at all") than the firmware modulating extruder moves to perform pressure-advance, or axis moves to perform skew correction or input shaping.

If the printer ran into physical limits triggering an error and shutting down the firmware, ruining an otherwise perfect print, I'd find that rather annoying, well knowing that I should have reduced the available print volume in the slicer just because I might decide later to run the print with z-hop enabled. From a user perspective, I find it reassuring to know that the z-hop implementation will rather limit z-hop moves to stay within the physical print volume than provoking that error. Actually, I'd expect that kind of safe-guard. Remembering my first attempts at macros, I found it quite questionable, how a stupid mistake on my part, that would move the head outside the bed area, would provoke an error of the same severity as a thermal runaway, leading to firmware shutdown, loss of hotend cooling and eventually heat creep if not restarted promptly. Or that the same shutdown occurs just by saving a setting. But that is another high level discussion 😃

[flopana77]

Hi Kevin.
Thanks for taking the time to reply and I promise that I'll make it short this time :)

I agree that the slicer step is somewhat quirky and that an integration of it in the firmware would be the way to go.
In fact, we sort of have the reverse happing already, with Orca Slicer incorporating Mainsail in the Slicer as separate Tab. Not the ideal development but somewhat expected as Marlin&Co simply do not have the computation power to get there. Klipper does though and therefore I believe not playing this strength would be a missed opportunity (not saying that this is happening, just talking general).

Re: standard, helix and ramp zhop --> Standard and helix lift the nozzle vertically after retracting the filament and before the upcoming travel move.

Ramp retracts the filament and combines the vertical zhop move with the upcoming travel move. Above and below figures should make this better understandable. After the zhop height is reached, all three styles behave exactly the same, executing all moves with the zhop offset considered until unretracted.

Re: raising an error if the slicer has issued an illegal command versus Klipper being smart enough to figure this out and making sure the print is executed as the user intended --> I believe the majority of users would go for the latter. Please don't misunderstand this statement. I fully agree that Klipper should NEVER alter moves if those can be executed without altering the printed object and without damaging the printer. Travel moves in Z direction, like zhop, do not impact the printed object and hence, checking if those travel moves would go out of range prevents otherwise unnecessarily failed or stopped prints IMHO.

Re philosophical discussions --> I certainly did not intend to question the place of bed mesh leveling, pressure advance, input shaping and all the other great features in Klipper that make it the best firmware around! Rather than that, I wanted to point out that firmware retraction, zhop, wiping, etc. aim at improving print quality and reliability and therefore also belong into the firmware. Preventing oozing and saving curly prints are not actions a user typically choses to do but rather has to do to get a good print. Thus, I admit that zhop is not a convenience feature like bed mesh but rather a safety feature that allows increasing the success rate of difficult prints.

Re users clamouring for improved z hop support --> Before I started working on this, I read the contribution guidelines. Those mention that contributions should have a real life impact of at least 100 users. Consodering a few conversations I followed on reddit in te past, counting the likes the PR has received so far, plus looking at reactions I saw only on teachingTechs video on Firmware retraction (see below on comments directed at Klipper), the user base that actively voiced their desire for this feature stands around 50 now. I believe that it is safe to assume that the grey number is considerably higher and thus the threshold is easily surpassed. I guess @pedrolamas and @gandy92 point in a similar direction.

Finally, I would appreciate your advice on how to proceed. I am happy to restructure the PR, strip features or close the PR altogether. I am a huge fan of your work and trust that you will give it due consideration.

Cheers
Florian

PS: Sorry, I'm not good at writing short stuff it seems :)

[Sineos]

From a user perspective, I find it reassuring to know that the z-hop implementation will rather limit z-hop moves to stay within the physical print volume than provoking that error.

This is a perfect example of how user perspectives might vary:
I would hate it, if the printer silently does something else than commanded. This is completely unpredictable and might lead to unpredictable results.
If I force the printer into a state he cannot fulfill, I expect it to stop and tell me and not do something else, hoping it is according to my wishes.

[Matszwe02]

From a user perspective, I find it reassuring to know that the z-hop implementation will rather limit z-hop moves to stay within the physical print volume than provoking that error.

This is a perfect example of how user perspectives might vary:
I would hate it, if the printer silently does something else than commanded. This is completely unpredictable and might lead to unpredictable results.
If I force the printer into a state he cannot fulfill, I expect it to stop and tell me and not do something else, hoping it is according to my wishes.

I think there are 2 possible solutions

• add some option to limit z hop to physical limits (so the user chooses if the z hop height gets altered or not) like limit_to_build_volume: true
• or inform user with a warning instead of an error that z hop couldn't be performed fully and it needed to shorten to match build volume

From user experience, this z hop feature works great, I've been printing with helix pretty much since the PR on my voron v0.

[flopana77]

Hi Sineos.

I would hate it, if the printer silently does something else than commanded. This is completely unpredictable and might lead to unpredictable results.
If I force the printer into a state he cannot fulfill, I expect it to stop and tell me and not do something else, hoping it is according to my wishes.

I would hate this as well and admit that my PR is incomplete in this point. I guess the key issue here is maintaining predictability for the user. My approach here would be to provide the user with a warning in case the printer did not perform a requested action. If the user considers this an undesired behavior, because he prefers the printer to perform commands no matter if they may stop a print, then the user can choose to disable these safeguards. This is exactly what the car industry is doing with all the driving aids. They are enabled by default, issue a warning if the behavior of the car is altered by the driving aid and can be turned off if the driver if he chooses to do so (most manufacturers allow turning off safety features..not all). This is easy to implement and follows Matszwe02's suggestion.

That said, @Matszwe02, I am happy to read that the code works well for you. As I mentioned over in my repo, I worked on preventing helix moves throwing an error when moving out of the build volume. Given the discussion we are having here on whether such features are in-line with Klipper's overall philosophy, I'll start a separate branch where you can get access to the state of this work if you are interested =)

Cheers
Florian

[Sineos]

My 2ct:

• Move out of range errors do indicate that you are about to violate the physical limits of the printer that will lead to a crash. It does not mean "Hey pal, slow down, in about 1 meter you gonna hit a roadblock"
• This does not have a warning character but is and stays an error
• You have an unpredictable number of printer designs and odd / weird / ingenious printer designs out there

I have seen too many processes in my professional life that work by a "shit in / shit out, but who cares" philosophy. This and also my support activities here have taught me a number of things:

• Users rather would like to silence sanity checks / warnings / errors than to fix the root cause
• If there is a way to misuse something, then it will be done
• Finding the reason for errors that are the consequence of such "backdoor activities" is a major pain

And sorry to say, I do not agree with your automotive example: They allow you to turn off functions that have an "assistance" or "comfort" character, but they do not allow you to turn off core safety features. Move out of range is a core safety thing.

And lastly: I really do not understand this widespread notion to allow apparently faulty input and expect the software to magically heal this.

[flopana77]

Hi Sineos. I share your point of view, especially regarding errors and users not fixing the root cause, and I additionally believe that user errors, which can be fixed by the firmware, should be fixed with a warning. Plastic and user time are money. Hence, if the firmware aborts a print, that could also have completed successfully with a warning, the firmware basically wastes user money.
Re Automotive example, I am not quite sure where the line is drawn between an "assistance" and a "core safety" feature. Which category are e.g., emergency breaking. traction control or electronic stability control? All of these safety feature behave as described AFAIK.
Anyways, the beauty of the open-source community is that everyone is allowed to have an opinion and a point of view. Having this diversity of thoughts and challenging the status quo every now and then is what drives innovation. Cheers, Florian

[Zeanon]

I get both of your sides so I would propose a compromise:
Instead of aborting or just raising a warning, how about raising a warning and pausing.
That would still waste time but not filament.
Or you could add an option which defines whether the print should abort, pause or just get a warning (I know kevin wants to have the least amount of config options necessary to not confuse new users which is totally understandable but for that I would also suggest to split the Klipper docs into a basic doc for normal users and an advanced doc for all the advanced power user features, that way we as power users can have all the fine grained control we want and new users won't get confused)

[KevinOConnor]

Before I started working on this, I read the contribution guidelines.

Thanks. I agree that z hop seems like a useful feature to add to Klipper's firmware_retract module. I apologize if my comments came across as negative or dismissive. I did want to make it clear though that there is a challenge in me reviewing z hop as I don't use the feature.

I also think that the Klipper development process could be improved - in particular it may be preferable to have some type of system where these kinds of features could be reasonably added to Klipper without a lengthy code review. I'm not sure the best way to proceed on that, and I will continue to give it thought.

I would appreciate your advice on how to proceed.

If you are looking to merge in the short term, my advice would be to simplify the initial implementation - simple vertical z hop, no "smart" error avoidance, minimal unrelated code changes (eg, avoid calling SET_VELOCITY_LIMIT).

If you feel other features are mandatory then I suspect we're looking at a longer time frame (likely months out). As I think we (Klipper community) may need to figure out how we'll handle the growing range of features over the long term.

My approach here would be to provide the user with a warning in case the printer did not perform a requested action.

My main comments on this PR are above. I'm expanding on my comments on "raise errors vs try to proceed anyway" to provide some additional context on my thoughts on this topic.

I additionally believe that user errors, which can be fixed by the firmware, should be fixed with a warning. Plastic and user time are money. Hence, if the firmware aborts a print, that could also have completed successfully with a warning, the firmware basically wastes user money.

I understand. I can certainly see where some users would want that. I, however, definitely do not want this behavior on my printers.

I've been "scarred" by software "trying to guess what I really want" too many times. It was one of the core reasons why I decided to write my own 3d printer firmware.

The main issue is that the programmers consistently guessed wrong. They thought the circumstances leading to the error were trivial, but too often they were actually due to some other fundamental problem (for example, some other incorrect code placed the toolhead too close to a boundary). The "guessing about errors" code then magnified the problem. Ultimately I was left with "spaghetti" and no reasonable way to figure out how I got into that crazy state. If I had just been told there was a problem, I could have easily fixed it and then gone on to enjoy hundreds of hours of excellent quality prints. The inverse approach led me to lose massive amounts of lost time trying to "guess what other people guessed" so I could "guess how to avoid those guesses"..

As I said above, I've been "scarred" by "error avoidance". I do not claim that my approach (raise an error on an invalid request) is definitely the "right way" or even the "best way". It will, however, continue to be my approach.

Travel moves in Z direction, like zhop, do not impact the printed object [...]

Z hop definitely does impact the printed object. If it didn't impact the printed objects we wouldn't consider adding it to Klipper.

Regarding the different styles of z-hop as proposed by this PR: I'm not quite seeing the point as to how there would be any guessing involved

My comments on "guessing" are about "error avoidance". The act of determining a well formed user request is not valid and commanding some other action in its place (or no action at all) to avoid presenting the user with an error. My comments on the type of z hop (spiral, vertical, ramp) is not related.

Instead of aborting or just raising a warning, how about raising a warning and pausing.

This is what Klipper does today in practice. An error raised by a g-code command will cause the virtual_sdcard system to halt reading of future commands. The toolhead will stop moving. The frontends will alert the user of an issue. The heaters and motors remain active. A user can take an action and proceed if they so choose.

Admittedly, it would be challenging for a user to meaningfully proceed after such an error (if they are even immediately present to address it). There does seems to be some confusion on Klipper's error handling though.

Klipper has two distinct error handling systems - an error and a shutdown. The shutdown system is intended for safety issues (a notable increase in the risk to the printer hardware, its environment, or its users). A shutdown is designed to rapidly turn off all heaters and motors. A typical g-code error does not invoke a shutdown event. It is propagated up to the higher layers with the intent of stopping the "scheduled plan" to alert the user that the scheduled events could not be completed as requested.

Cheers,
-Kevin

[flopana77]

Hi Kevin

I agree that z hop seems like a useful feature to add to Klipper's firmware_retract module. I apologize if my comments came across as negative or dismissive. I did want to make it clear though that there is a challenge in me reviewing z hop as I don't use the feature.

Thank you and certainly no apology needed. Maybe it is time for you to start using zhop once available in Klipper :p

If you are looking to merge in the short term, my advice would be to simplify the initial implementation - simple vertical z hop, no "smart" error avoidance, minimal unrelated code changes (eg, avoid calling SET_VELOCITY_LIMIT). If you feel other features are mandatory, then I suspect we're looking at a longer time frame (likely months out). As I think we (Klipper community) may need to figure out how we'll handle the growing range of features over the long term.

Thank you for your advice! I will condense the PR to submit a "minimum viable product" version with the following features:

1. Reliable retract state clearance to avoid nozzle crashes (this requires events in print_stats).
2. No changes in moves to avoid errors but instead throwing an error.
3. Remove "Ramp" (I would like to keep Helix if possible as it is basically a standard zhop move with helix motion instead of a straight vertical move. I promise I won't include a smart error avoidance for out-of-range moves :p )
4. Only change speed and accelerations (not square corner velocity) for zhop moves. This is kind of required to make sure the moves execute as fast as possible. Otherwise, the zhop moves may execute at extrusion move speed in case the slicer issues the command to increase speed after the G10 command. Please let me know if this is ok or if there is any other way to ensure fast speed for zhop moves.
5. Merge CLEAR_RETRACTION and GET_RETRACTION commands into SET_RETRACTION via additional parameters.
6. Remove the M101 and M103 wrappers.
7. Remove verbose-setting and replace with proper error messages.
8. Remove the config_params_on_clear config parameter and instead add this as parameter to the SET_RETRACTION command as means for the user to decide what happens when retraction is cleared (I like this actually better because it allows deciding at runtime if changes made, using SET_RETRACTION, shall be kept once retraction is cleared).

Please confirm if this is all ok for a short-term merge or what else I shall change. Also, should I remove the extensive commenting?

As I said above, I've been "scarred" by "error avoidance". I do not claim that my approach (raise an error on an invalid request) is definitely the "right way" or even the "best way". It will, however, continue to be my approach.

This is well understood. I would suggest including a section in the "Developer Documentation" document which clearly outlines the design philosophy of Klipper, let's call it "Design Requirements for Submissions". The section would states principles like:

• No wrappers or other implementations that can easily be done by the user with simple macros.
• No codes that try avoiding or fixing user errors.
• Few Run-time commands with parameters are preferable over config settings.
• No "big bang" rewrites but gradual step-by-step submissions (via multiple commits in one PR?)
• No commenting/Extensive commenting?
• No changes at runtime of moves planned by the slicer?
• ...etc...

For what it is worth, after all the (hopefully fruitful and at least somewhat enjoyable) conversations above, this may then just be the most valuable part of this whole PR :p

Z hop definitely does impact the printed object. If it didn't impact the printed objects we wouldn't consider adding it to Klipper.

Sorry, bad formulation from my side. I meant that zhop moves do not impact the shape of the printed object given that zhop are travel moves and not extrusion moves. What I wanted to express was that changing extrusion moves is a NoGo even for me...

An error raised by a g-code command will cause the virtual_sdcard system to halt reading of future commands. The toolhead will stop moving. The frontends will alert the user of an issue. The heaters and motors remain active. A user can take an action and proceed if they so choose. Admittedly, it would be challenging for a user to meaningfully proceed after such an error (if they are even immediately present to address it). There does seems to be some confusion on Klipper's error handling though.

Would it be acceptable to save the gcode state upon error occurrence (including the virtual SD card line is applicable) so that the user has the possibility of resuming a print that was stopped due to an error? Also, to ensure that the print is not ruined with blobs when the printer runs into an error, would it be acceptable to raise the nozzle and have the toolhead go to homing position? If so, I may look into this...

Cheers and thanks for taking the time to comment in detail :)
Florian

[KevinOConnor]

Thank you for your advice! I will condense the PR to submit a "minimum viable product" version

Thanks. A minimum version I think would help me review the code and get it merged.

I'm struggling to comment on your list of proposed changes though, largely because the list seems to be relative to the current PR, which I don't have a good understanding of.

That said, the list seems a bit expansive to me. I think a good first step would be to target a minimal implementation of z hop on retract - a vertical raise of the toolhead on G10 and return to previous z height on G11. I'd avoid changes to acceleration, I'd avoid changes to GET_RETRACTION, I'd avoid helix mode, and I'd avoid other changes unless they are directly related to the core z_hop functionality. To be clear, we can certainly make those changes later on - it would just be a lot easier to review if the changes came one at a time. The basic Z lift is still a lot of work due to the need to avoid bed crashes during subtle corner cases, so once that is in, ideally other enhancements will be less of an issue to get reviewed and merged.

Cheers,
-Kevin

[flopana77]

Hi Kevin

I'm struggling to comment on your list of proposed changes though, largely because the list seems to be relative to the current PR, which I don't have a good understanding of.

Sorry for that. No issue, I'll follow your guideline and work on this minimalistic version asap.

Cheers and thanks for taking the time to comment in detail :)

BTW, I didn't mean to comment my last post in detail but actually the entire discussion ;)

Once I have a minimalistic version of the PR available, I'll close this one and start a new one.

Cheers
Florian

[Matszwe02]

Hi Kevin

I'm struggling to comment on your list of proposed changes though, largely because the list seems to be relative to the current PR, which I don't have a good understanding of.

Sorry for that. No issue, I'll follow your guideline and work on this minimalistic version asap.

Cheers and thanks for taking the time to comment in detail :)

BTW, I didn't mean to comment my last post in detail but actually the entire discussion ;)

Once I have a minimalistic version of the PR available, I'll close this one and start a new one.

Cheers Florian

Hi Florian, I found a critical bug in your dev branch, you can check my PR :)

[flopana77]

Hi Matszwe02. Thanks for pointing this out. I was working on some new stuff on the dev branch and stopped midway because I decided getting my CR10S Pro back to life. Can you check if the error occurs on my master branch also? As far as the initial code goes, the error you describe should not happen. Hence, this is probably an artefact from my half-started, half-finished experiment on the dev branch. In any case, please send me the offending gcode to have a look. Cheers. Florian

[flopana77]

Hi All. Sorry for the delay. I am still busy with real-world stuff at the moment. I hope to get to coding the spec'ed down version in the coming week. Cheers, Florian

[Sineos]

I do not think that these are valid changes. E.g. gcode_shell_command.py is an unofficial "extra" and not part of Klipper main-line

[flopana77]

Correct. Forget about these changes, they are not meant for merging. I am testing the minimal version of the zhop code at the moment. Once it is working, this PR will be closed, and a "clean" PR will be submitted. Cheers. Florian

[flopana77]

Dear All. As previously mentioned, I am closing this PR in favor of the minimalistic zhop code as per PR #6311 .

Cheers, Florian