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

Fix bilinear levelling z offset #6251

Merged
merged 2 commits into from
Apr 6, 2017

Conversation

benlye
Copy link
Contributor

@benlye benlye commented Apr 5, 2017

Since run_probe was altered to return the probe Z position rather than the nozzle Z position bilinear levelling has been broken because the Z-offset has been applied twice - once in the run_probe function, and then again in the G29 code for bilinear levelling. This results in Z0 being below the bed when the Z-offset is negative.

On the assumption that the change to run_probe is needed elsewhere, this PR removes the extra addition of the Z offset in the G29 code for bilinear levelling.

thinkyhead and others added 2 commits April 4, 2017 16:53
Since run_probe was altered to return the probe Z position rather than the nozzle Z position bilinear levelling has been broken because the Z-offset has been applied twice - once in the run_probe function, and then again in the G29 code for bilinear levelling.
@benlye
Copy link
Contributor Author

benlye commented Apr 5, 2017

Fixes the issue described in #6233, caused by a5e085c, committed in #6181.

@benlye benlye mentioned this pull request Apr 5, 2017
@MagoKimbra
Copy link
Contributor

zoffset is an offset that can be given with the subcommand Z, in that case we PROBE_MANUALLY. If you are not from the command zoffset Z is 0 ...

zoffset = code_seen('Z') ? code_value_axis_units(Z_AXIS) : 0;
#if HAS_BED_PROBE
  zoffset += zprobe_zoffset;
#endif

Since we are with the PROBE_MANUALLY HAS_BED_PROBE is false then zoffset is zero or the value given by the command Z.

@benlye
Copy link
Contributor Author

benlye commented Apr 5, 2017

OK. If I understand you correctly, my change is going to 'fix' the Z probe offset being applied twice but only by accident, and is also going to break passing a Z subcommand to G29.

In that case, is the way to address the double-addition of the Z probe offset to the bilinear correction grid simply to remove these lines?

        #if HAS_BED_PROBE
          zoffset += zprobe_zoffset;
        #endif

@benlye
Copy link
Contributor Author

benlye commented Apr 5, 2017

I compiled and tested with my initial proposed change reverted and the zoffset += zprobe_zoffset; line commented instead and bilinear levelling is working correctly and giving the expected output and correction.

Let me know what to do with this PR - I'm happy to update/close/resubmit as appropriate.

@MagoKimbra
Copy link
Contributor

you're right!

autonumous pushed a commit to autonumous/MarlinFirmware-BigBoxV1.1 that referenced this pull request Apr 5, 2017
follow some recent change:s committed in
MarlinFirmware#6181

problem identified and fix suggested in:
MarlinFirmware#6251
MarlinFirmware#6233
@thinkyhead thinkyhead merged this pull request into MarlinFirmware:RCBugFix Apr 6, 2017
@benlye
Copy link
Contributor Author

benlye commented Apr 6, 2017

@MagoKimbra @thinkyhead - I'm a little confused now. This was merged as-is, with my original proposed change, which seemed to be flawed. Won't the Z parameter to G29 now be ignored?

benlye added a commit to benlye/Marlin that referenced this pull request Apr 6, 2017
The fix in MarlinFirmware#6251 for bilinear Z offset was flawed and broke the Z parameter of G29 for bilinear levelling.  This is reverted and a different fix is used for the double-addition of the Z-probe offset to the bilinear correction grid.
thinkyhead added a commit that referenced this pull request Apr 7, 2017
Better fix for bilinear Z offset and G29 Z (update to #6251)
@benlye benlye deleted the benlye-bilinear-fix branch April 7, 2017 07:29
@thinkyhead
Copy link
Member

Thanks for #6258. That makes more sense.

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.

3 participants