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

Account for coordinate space offsets #4402

Merged

Conversation

thinkyhead
Copy link
Member

@thinkyhead thinkyhead commented Jul 25, 2016

A fundamental flaw has been found in the management of coordinates in various functions.

Background: Marlin has long had a home_offset array that changes the home position. It used to apply on the next G28 but it was updated to take effect immediately on M206. The home_offset was later applied to the software endstops. More recently the position_shift array was added, which remembers any redefinition of the current_position (such as with G92) and also applies it to the software endstops.

The Basic Problem: When you specify coordinates for Marlin now, the current coordinate space must be taken into account. So, for example, when saving points for Mesh Bed Leveling, we need to subtract these offsets so that if we shift the coordinate space, we can still get the right mesh points.

The Planner Problem: When we send coordinates to the planner and stepper code regions, for cartesian machines we have always set them to the current "logical position" — the position that includes the coordinate space offsets. This is fine. The planner and stepper functions can return coordinates to us in the current coordinate space with no extra figuring required. However, on DELTA the planner and stepper positions are translated, so they must always use the "raw position" for their figuring. So we have to take care here.

The Bed Leveling Problem: As mentioned, Mesh Bed Leveling always stores the "raw position" so its mesh can still apply as the coordinate space shifts. The grid-based mesh used by DELTA and the planar grid leveling used by Automatic Bed Leveling are still stuck in a non-moving coordinate space, so after G92 or M206 the leveling compensation is likely wrong. Furthermore, the arguments to G29 presume a non-moving coordinate space, and will produce bad results in a shifted coordinate space.

The Solution: Basically, everywhere that coordinates are specified, we need to audit and ensure that the most appropriate coordinates are being stored. When performing bed leveling, it would be best to store the "raw position" values instead of the shifted coordinates. And when doing bed probing, it would be best to treat input coordinates as being in the current coordinate-space, and to make sure to include the coordinate-space offsets for the probe points.

This PR includes only some of that work so far, but will continue to accumulate commits patching these overlooked aspects of the system.

@thinkyhead
Copy link
Member Author

thinkyhead commented Jul 25, 2016

@Roxy-3D, @AnHardt As long as we're focusing on bed leveling, this would be a good time to wrangle bed leveling coordinates when probing and when doing compensation.

I believe currently the "logical coordinates" are stored as the XYZ in the all the bed level matrices (except for Mesh Bed Leveling). If that's the case, then it's possible that a G92 or M206 could result in the incorrect adjustments in some bed leveling functions.

In the case of simple least-squares tilt-compensation, I believe the planner just decides how to adjust Z on a move-by-move basis. So at the planner level it probably continues to do the right thing, tilting the destination based only on the distance.

I believe that planner.adjusted_position also does the right thing in a shifted coordinate space, since it only takes a given XYZ and translates it by some small amount in a linear direction in 3D space. So it's coordinate-space agnostic.

In the case of Grid Leveling (as on Delta) the coordinate-space offsets may matter more, since I believe it uses the grid points in a way similar to MBL. So in the case of Grid Leveling we might need to store raw coordinates.

@thinkyhead thinkyhead merged commit 169c21b into MarlinFirmware:RCBugFix Jul 25, 2016
@thinkyhead thinkyhead mentioned this pull request Jul 25, 2016
thinkyhead added a commit to thinkyhead/Marlin that referenced this pull request Jul 25, 2016
thinkyhead added a commit that referenced this pull request Jul 25, 2016
CONSULitAS pushed a commit to CONSULitAS/Marlin-K8200 that referenced this pull request Aug 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.

1 participant