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 position shift with MBL #4310

Merged
merged 9 commits into from
Jul 18, 2016

Conversation

thinkyhead
Copy link
Member

@thinkyhead thinkyhead commented Jul 15, 2016

Addressing #4266

When MBL is enabled and a position is set with G92 (or other functions —like gcode_T tool-change— that reorient the current position) the destination ends of the mesh-split lines are calculated at the wrong positions. As a result we see strange moves after a tool-change when MBL is enabled.

  • Also note that MBL can actually handle up to 9 x 9 grids.
  • Split out the mbl.active check so other line buffering is possible.
  • Since mesh_buffer_line always uses destination, use it as the target
  • Along that line, make mesh_buffer_line_to_destination instead
  • Apply other optimizations to reduce code size (by 654 bytes)

@thinkyhead
Copy link
Member Author

@epatel Note the main bug-fix here: We forgot to include the newly-inroduced position_shift[axis] when calculating the edges of the splits. So any code that modified position_shift would cause incorrect moves once MBL was enabled.

As a bonus, I realized that MBL can handle a slightly finer grid – up to 9 x 9 – because the x_splits/x_splits bit-masks use the grid index (0 - 7). That will help with the larger beds.

@epatel
Copy link
Contributor

epatel commented Jul 16, 2016

One thing. Removing the recursion is not good. I hope you test everything else to verify, my brain hurts trying to follow all changes.

Below is one case that will fail when doing like that. When moving "From" to "To" the first split will be on the X boundary (B) because that will be the first "if" statement that gets selected, then split A is lost which would have been found by the recursion.

img_20160716_132716090

@thinkyhead thinkyhead mentioned this pull request Jul 17, 2016
@thinkyhead
Copy link
Member Author

Makes sense!

@thinkyhead thinkyhead force-pushed the rc_mbl_position_shift branch 2 times, most recently from 69722c4 to ab32a9c Compare July 17, 2016 02:51
@thinkyhead
Copy link
Member Author

thinkyhead commented Jul 17, 2016

I realize the changes are many, and I didn't have them well laid out. So I went back and re-did the commits in order of obviousness, leaving off the dropped recursion.

  • The first thing was just to find the pattern and do the most obvious reduction to bring the code size down a little bit.
  • Then there was a little more redundancy in setting nz and ne for next end-point.
  • Next, since it's being used like other move functions, just use destination (instead of x, y, z, e arguments). This puts off making a copy on the stack until needed.

@thinkyhead thinkyhead force-pushed the rc_mbl_position_shift branch 10 times, most recently from e1e7b8e to 3af7e10 Compare July 17, 2016 19:24
@thinkyhead thinkyhead force-pushed the rc_mbl_position_shift branch 2 times, most recently from 33c0c6f to 3702987 Compare July 17, 2016 21:22
@thinkyhead thinkyhead merged commit 3422103 into MarlinFirmware:RCBugFix Jul 18, 2016
@thinkyhead thinkyhead deleted the rc_mbl_position_shift branch July 18, 2016 01:31
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants