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

Firmware retraction fix and extends zhop features. #211

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

fbeauKmi
Copy link
Contributor

@fbeauKmi fbeauKmi commented Apr 13, 2024

As discuss on Discord, DK firmware and according to issue #204 and PR #206 . I made this for a while some change to fit my use case. I guess it can fix the issue.

Changes from actual firmware_retraction

  • introduces reset_on_events parameter. (false : keep retracted state on events, true: reset retracted state and restore config values).
  • Ability to use SET_RETRACTION while retracted
  • Ability to do zhop even if retract_length=0.0
  • Always do zhop as relative moves

Checklist

  • pr title makes sense
  • squashed to 1 commit
  • added a test case if possible
  • if new feature, added to the readme
  • ci is happy and green

@rogerlz rogerlz force-pushed the firmware_retract branch 2 times, most recently from 69783aa to ea5b1db Compare April 23, 2024 21:26
@richfelker
Copy link

I've been testing this and it works for me except that it still produces warning spam on G10 and G11 commands that should be idempotent. This messes up the start of every print, where the start gcode needs to have left the extruder in retracted state, but Cura also emits its own retraction for the first travel move. Supporting this kind of idempotency instead of double-retracting and messing up priming is, from my perspective, one of the compelling motivations for firmware retract in the first place, so it should not be treated as something abnormal that produces warnings.

As discussed on Discord, I would also like to see the issue resolved in a way that makes it possible to use Z-hop and get the right properties for how fw retract is supposed to work. This probably means something like having it so entering retracted state lifts Z by the configured amount, but explicitly moving in Z after that loses the offset and moves to the actual commanded Z height, and so that exiting retracted state only returns to the logical, non-offset Z height (doing nothing if Z was moved since the retract).

@fbeauKmi fbeauKmi requested a review from a team as a code owner June 24, 2024 11:25
@fbeauKmi
Copy link
Contributor Author

I've been testing this and it works for me except that it still produces warning spam on G10 and G11 commands that should be idempotent. This messes up the start of every print, where the start gcode needs to have left the extruder in retracted state, but Cura also emits its own retraction for the first travel move. Supporting this kind of idempotency instead of double-retracting and messing up priming is, from my perspective, one of the compelling motivations for firmware retract in the first place, so it should not be treated as something abnormal that produces warnings.

I removed the console outputs

As discussed on Discord, I would also like to see the issue resolved in a way that makes it possible to use Z-hop and get the right properties for how fw retract is supposed to work. This probably means something like having it so entering retracted state lifts Z by the configured amount, but explicitly moving in Z after that loses the offset and moves to the actual commanded Z height, and so that exiting retracted state only returns to the logical, non-offset Z height (doing nothing if Z was moved since the retract).

This point doesn't fit my workflow but I think we can do something configurable, for example a boolean clear_zhop_on_z_moves parameter to let the user choose. Unclear to me , does this apply to relative and absolute movements?

@rogerlz
Copy link
Contributor

rogerlz commented Jul 1, 2024

@fbeauKmi this looks good to me.. would you mind rebasing? ty

@fbeauKmi
Copy link
Contributor Author

fbeauKmi commented Jul 2, 2024

@fbeauKmi this looks good to me.. would you mind rebasing? ty

@rogerlz If you don't mind, I'd like to wait for feedback before merging. I'm not sure clear_zhop feature is useful for relative moves. TY

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants