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

Rc bug fix - Fix for issue #2931 #3001

Merged
merged 2 commits into from
Mar 6, 2016

Conversation

AlwaysTinkering
Copy link

This change should fix issue #2931

If Z_SAFE_HOMING is set, then Z_RAISE_BEFORE_HOMING doesn't happen until after X and Y have been homed. This is actually unsafe on many printers. Disabling Z_SAFE_HOMING is also undesirable, because of the many benefits when using a head mounted Z probe, such as not probing when the probe isn't over the bed.

This change allows the Z raise before X and Y are homed, even if Z_SAFE_HOMING is set. The risk of raising Z before homing, is that if Z is already near max, and the Z_MAX_POS is too high, then it could be a problem. This is noted on the comment for Z_RAISE_BEFORE_HOMING. The risk of not raising Z before homing X and Y, is that the Z probe and head may be crashed through bed leveling knobs, screws, clips, etc. on the way to X and Y home if the head was already at 0, or the head was out of bounds.

I believe that raising Z before homing X & Y (when that is set) is better than homing X and Y first, then raising Z anyway, which is how it works with Z_SAFE_HOMING set in the current RcBugFix branch.

@thinkyhead
Copy link
Member

I believe it's possible that the small change you propose could cause unexpected results. But maybe you can confirm that with your change these do what they are supposed to do:

  • If you home X and Y, then home Z after, it should safe-home — lifting Z only once!
  • If you try to home Z-only when X and Y are unknown you should get an error.

There's been some effort to simplify these options a bit. The purpose of Z_RAISE_BEFORE_HOMING is only to raise Z before deploying a Z probe. Thus it was only ever considered important to do this just before homing Z. For Z_SAFE_HOMING that means the procedure would generally be:

  • Home X and Y
  • Move to XY: Z_SAFE_HOMING_X_POINT, Z_SAFE_HOMING_Y_POINT
  • Raise Z by Z_RAISE_BEFORE_HOMING
  • Use the Z Probe to do homing

Since Z is going to get raised and homed as part of this procedure anyway, I agree it makes the best sense to do the raise first. I just want to make sure that removing the Z_SAFE_HOMING condition at the start of G28 doesn't cause a double-raise or anything weird.

Note that currently this (Z_SAFE_HOMING) only applies if Automatic Bed Leveling is in use. But it has been suggested that we allow Z_SAFE_HOMING even if automatic bed leveling isn't in use, because some printers might still want use a Z probe to home Z even though they're not using automatic bed leveling.

Anyway, the number of options has begun to get confusing, so I proposed that we simplify them and just specify the clearance that a probe requires, then let Marlin figure how much to raise Z in each case…

https://github.com/MarlinFirmware/MarlinDev/pull/366

@AlwaysTinkering
Copy link
Author

I would definitely prefer a more comprehensive approach to solving the Z raise issues. This is just what I was using to get around the issue. If this can be addressed as part of a broader solution, simplifying the number of Z related options, that is a much better approach.

@thinkyhead thinkyhead added Bug: Confirmed ! PR: Bug Fix S: Don't Merge Work in progress or under discussion. labels Mar 2, 2016
@thinkyhead
Copy link
Member

Just marking this "Don't Merge" while we get some more testing and feedback on this.

@thinkyhead thinkyhead added Needs: Testing Testing is needed for this change and removed S: Don't Merge Work in progress or under discussion. labels Mar 6, 2016
thinkyhead added a commit that referenced this pull request Mar 6, 2016
@thinkyhead thinkyhead merged commit 6e64895 into MarlinFirmware:RCBugFix Mar 6, 2016
@jbrazio jbrazio modified the milestone: 1.1.0 Jul 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug: Confirmed ! Needs: Testing Testing is needed for this change PR: Bug Fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants