firmware_retraction: Standard nozzle lifting on G10

[flopana77]

Summary of this Pull request

This is the follow-up to PR #6239, which considers the feedback received.

It enables standard nozzle lifting (zhop) when retracting using G10 command; fully configurable at runtime; in absolute or relative mode. 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).

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, repeated to check repeatability.
  • Retract in retract state with and without zhop enabled.
  • Change of z_hop height while retracted.
  • Move Z while retracted including prior switch from absolute to relative mode and back.
  • Move XY while retracted.

• Real-world test prints were performed (retraction towers).

• Previously identified main failure cases concerning z_hop functionality have been addressed:

  • Failure Case 1: Nozzle crash on detraction, after absolute z move while retracted
  • 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.
  • Failure Case 3: G10 is issued before homing. This is currently handled by Klipper's error handling.
  • Failure Case 4: G10 is issued with a temperature lower than minimum extrude temperature. This is currently handled by Klipper's error handling.
  • 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. This is addressed by issuing a warning to the user that SET_RETRACTION is not available in retract state.
  • 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 is handled by Klipper's error handling.
  • 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).
  • Failure Case 8: SET_RETRACTION is called while in retract state. Although the probability of this case is low, in the current implementation this will clear the retract state, which may lead to local non-extrusion as the required unretract filament move is not done. This is addressed by issuing a warning to the user that SET_RETRACTION is not available in retract state.

• 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 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?

Yes, as discussed in PR #6239 .

• What is the benefit of the contribution to the community? As discussed before:

  • SAFER PRINTING USING FIRMWARE RETRACTION WITH ZHOP
  • LESS RESOURCE INTENSIVE THAN MACRO IMPLEMENTATIONS PREVENTS UNNECESSARY CPU LOAD
  • FIRMWARE RETRACTION INCLUDING ZHOP SIMPLIFIES THE OVERALL WORKFLOW
  • FLEXIBILITY TO CHANGE RETRACTION PARAMETERS AND ZHOP HEIGHT AT RUNTIME
  • REAL-WORLD PRINTS SHOWED PERFORMANCE BENEFITS USING FIRMWARE RETRACTION WITH Z_HOP RATHER THAN SLICER RETRACTION

• 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 the zhop related parameter is optional and default is 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 the new setting (z_hop_height).
• The Status_Reference.md has been updated to include unretract_length, z_hop_height 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 this minimalistic version is easier to review. I am happy to comment and explain if needed. Cheers, Florian

[github-actions]

Thank you for your contribution to Klipper. Unfortunately, a reviewer has not assigned themselves to this GitHub Pull Request. All Pull Requests are reviewed before merging, and a reviewer will need to volunteer. Further information is available at: https://www.klipper3d.org/CONTRIBUTING.html

There are some steps that you can take now:

1. Perform a self-review of your Pull Request by following the steps at: https://www.klipper3d.org/CONTRIBUTING.html#what-to-expect-in-a-review
   If you have completed a self-review, be sure to state the results of that self-review explicitly in the Pull Request comments. A reviewer is more likely to participate if the bulk of a review has already been completed.
2. Consider opening a topic on the Klipper Discourse server to discuss this work. The Discourse server is a good place to discuss development ideas and to engage users interested in testing. Reviewers are more likely to prioritize Pull Requests with an active community of users.
3. Consider helping out reviewers by reviewing other Klipper Pull Requests. Taking the time to perform a careful and detailed review of others work is appreciated. Regular contributors are more likely to prioritize the contributions of other regular contributors.

Unfortunately, if a reviewer does not assign themselves to this GitHub Pull Request then it will be automatically closed. If this happens, then it is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

[KevinOConnor]

Thanks. I apologize for the delay in responding.

Unfortunately, this PR appears to be a "rewrite" of the existing firmware_retract module. That is, there is a single commit that makes white space changes, moves existing code to subfunctions, alters parameters of existing commands, adds new commands, alters other modules, and adds new functionality.

I will not merge a "rewrite". My experience has shown the risk of regression is too high, and the options for handling a possible regression is too limited.

I appreciate your work on this. However, due to other constraints on my time, I feel you should know that it is unlikely I'll be able to review z_hop functionality in the short or medium term.

-Kevin

[dotorg]

Having followed both this, and the prior, pull requests, I think there needs to be more clarification about what is going to be accepted to address this pretty significant functional limitation in Klipper before people stop trying to get this change in. The prior discussion pointed out, and it seemed to be in agreement, that a rewrite was necessary, and it wasn't feasible to patch the functionality into firmware_retract. If that is the case, how is this an issue now?

This functionality needs to get into Klipper, and flopana77 has done this twice now. Before they, or anyone else, takes up trying to do it in a way that is going to be acceptable, I think there needs to be clarification on it. (Honestly, it probably gives anyone thinking of contributing anything beyond the most trivial a bit of pause.)

[DuncanMcPherson]

I agree with @dotorg. I can't claim to be a python expert but from what I have been able to understand, it won't be possible to add this functionality without a rewrite. At this time, the only potential suggestion I have is potentially creating a new module for this new feature to allow for easy reversion and to allow for targeted testing. Knowing how some of these things work however, that may not be feasible.

[YanceyA]

@flopana77 With the decision by Kevin this PR might be best actioned in the DangerKlipper fork where there is more review resource and appetite for new functionality.

https://github.com/DangerKlippers/danger-klipper

[KevinOConnor]

Thanks for the feedback.

I understand the concerns. I agree that zhop functionality seems useful and seems like it would make sense to have in Klipper. I feel there are good reasons to avoid "rewrites" though. One of the challenges is that I have no way to evaluate which implementation is "better" - the existing one or the new one. I don't use this module (neither firmware retract nor firmware retract with zhop), so it's not something I can evaluate on the merits.

One way forward would be to just say "it only adds functionality and doesn't alter any existing capabilities", but alas a rewrite in a single commit doesn't give me that option as a reviewer. That is, when I look at the code, I just see "old code" and "new code" with no good way to confirm nothing is different. Even when I get small independent changes that I can evaluate in a review it is often possible that something goes wrong because of something unexpected (see #6303 for an example - no hard feelings there - just that these things happen during development).

I don't think it's fair to the existing users and existing author for me to replace one module with another, and I don't think it would be fair to the new users and new author when I get the next request in a year or so. So, my experience has been - don't do "rewrites".

There's also another obvious question "why not allow users to choose instead of Kevin" - which is fine. I raised
https://klipper.discourse.group/t/possible-klipper-plugins-instead-of-macros/9819 recently to see if there was interest in that. I didn't see much feedback though.

So, yeah, I agree it's a tough spot but I don't have a good solution. And, given all the time issues I have now, I can't devote significant time to this.

-Kevin

[flopana77]

Sorry for the radio silence. Real life hit...if there is interest, I could separate zhop out into a separate module. This should be doable with acceptable effort and would solve the rewrite issue. Hence, if this would be a way forward, I'm happy to put the work in. @KevinOConnor, please let me know if this would be OK with you. BR Florian

[KevinOConnor]

Thanks. I think we need to figure out some long-term direction plans - as described at https://klipper.discourse.group/t/possible-klipper-plugins-instead-of-macros/9819 and https://klipper.discourse.group/t/new-submission-process-for-feature-enhancements/9818/11 . Hopefully we'll have consolidated on a plan going forward in the next couple of weeks.

-Kevin

[YanceyA]

@flopana77 Your PR is being used in the dangerklipper fork and there a question from a discord discussion you.might be able to answer.

Hey all, About firmware_retraction I'm still trying to figure out if my workflow is wrong or if the new featur eisn't working .
With the new code, retraction parameters are reset to config_params every time CLEAR_RETRACTION is called, wich seems normal. But also when Homing, motors off, start/ end print ... I don't understand why this mandatory. Is it the right place to discuss it ?
TBH, for now I override firmware_retraction.py by moving line 228 to line 210 (I ❤️ the allow_plugin_override feature ). it seems to work but I haven't had time to go further yet.

[github-actions]

Unfortunately a reviewer has not assigned themselves to this GitHub Pull Request and it is therefore being closed. It is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

[dfredell]

I would be excited to see this in main line! It fixed my issue perfectly the first try. I was having issues with the print overhangs curling up, then the nozzle was colliding with that ripping the print off of the bed. I had a few failed prints. Same exact GCode in these, just switched to the @flopana77 branch, and set a z_hop_height.

main line of klipper

flopana77:PR_basic_zhop branch