Skip to content

UBL G29 T current position fix #12845

Merged
thinkyhead merged 7 commits intoMarlinFirmware:bugfix-1.1.xfrom
spinorkit:bugfix-1.1.x-G29_T-fix
Feb 27, 2019
Merged

UBL G29 T current position fix #12845
thinkyhead merged 7 commits intoMarlinFirmware:bugfix-1.1.xfrom
spinorkit:bugfix-1.1.x-G29_T-fix

Conversation

@spinorkit
Copy link

Account for x and y probe offset when indicating current position [] on grid when displaying mesh map.

Also, adjust the default Safe Home position to be on a grid point to allow easy validation, i.e. we expect the grid z offset value at the home position to be very close to 0, depending on how reproducible the z probing is.

Description

After doing a G29 P1, the mesh topology displayed using G29 T would show larger than expected z errors when compared with the z height displayed on the LCD screen.

This turned out to be due to the code for finding the nearest mesh point to the current nozzle position not taking the x and y z probe offsets into account.

Benefits

This fix makes it easier to verify the mesh using the LCD readout z value.

Related Issues

#if ENABLED(AUTO_BED_LEVELING_UBL)
//Try to home on a grid point to better allow checking mesh center
#define Z_SAFE_HOMING_X_POINT ((GRID_MAX_POINTS_X/2)*(X_BED_SIZE-2*MESH_INSET)/(GRID_MAX_POINTS_X-1)+MESH_INSET) // X point for Z homing when homing all axes (G28).
#define Z_SAFE_HOMING_Y_POINT ((GRID_MAX_POINTS_Y/2)*(Y_BED_SIZE-2*MESH_INSET)/(GRID_MAX_POINTS_Y-1)+MESH_INSET) // Y point for Z homing when homing all axes (G28).
Copy link
Member

@thinkyhead thinkyhead Jan 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of these settings is to allow users to override a default. If you think this is a more sensible default (closer to the center of the bed) when UBL is enabled, then this code should be in Conditionals_post.h as the fallback when there are no Z_SAFE_HOMING_*_POINT values set. There's no way we're going to propagate this to every configuration file as the "elegant" default for UBL.

Copy link
Author

@spinorkit spinorkit Jan 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, will do, thanks. Those expressions cause homing to occur on a grid point as close as close as possible to the center of the bed. It will only be exactly at the center of the bed if GRID_MAX_POINTS_X is odd (current default is 10).
The rationale for this:
I recently turned on UBL leveling for the first time. I was expecting that after a
"G28\nG29 P1\nG28\nG29 T" sequence, the "current value" in the table would be close to 0 because my z probe is pretty reproducible. When the value was an order of magnitude greater than I was expecting I spent quite a while rechecking my system etc. Eventually I realized the discrepancy was due to two things, first the center of the bed homing position was not on a grid point but halfway between grid points, and second, the grid point marked as the "current position" was not taking the horizontal z probe offsets into account.
My thinking was that these changes could save someone from wasting a similar amount of time in the future, although putting it in Conditionals_post.h means no newbie will get the fix because the Z_SAFE_HOMING_*_POINT lines are not commented out by default in Configuration.h.
An alternative would be to default GRID_MAX_POINTS_X to 9 or 11 rather than 10.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the center of the bed homing position was not on a grid point

It is seldom the case that the homing position is exactly on a grid point. If that matters, it would be better to fix the issue that causes it to matter rather than to force users to make their homing position align exactly to a grid point.

An alternative would be to default GRID_MAX_POINTS_X to 9 or 11 rather than 10.

Whatever works best. I'm trying to avoid adding too many conditions and repetitions of the same options in the configuration files, as this complicates developing a configuration utility and generally adds more noise the the configs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is seldom the case that the homing position is exactly on a grid point. If that matters, it would be better to fix the issue that causes it to matter rather than to force users to make their homing position align exactly to a grid point.

I would imagine most users use the 'calculated' default homing position rather than setting it explicitly, especially if SAFE_HOMING is defined, as is recommended when using UBL. I.e. I doubt many new users would adjust the SAFE_HOMING position.
The homing position only matters because it is used as the z origin when measuring the grid and therefore one would expect the z offset measured there to be zero to within the homing error standard deviation.
However, with the current defaults, you cannot see this in the G29 T table, which, as a newbie to UBL, confused me because I thought the UBL scheme, or perhaps my probe, was not working well.

So this is simply about improving the experience for new users by having better defaults rather than forcing anything. The issue that causes this to matter is only that the current defaults can make things confusing for new users of UBL. Configuration.h already advises "If using a Probe for Z Homing, enable Z_SAFE_HOMING also!", so why does it not make the home position a nice one for the G29 T report?

An alternative would be to default GRID_MAX_POINTS_X to 9 or 11 rather than 10.

Whatever works best. I'm trying to avoid adding too many conditions and repetitions of the same options in the configuration files, as this complicates developing a configuration utility and generally adds more noise the the configs.

The two options I can see for improving the defaults that require modification of configuration.h would be:

  1. comment out the lines #define Z_SAFE_HOMING_X_POINT ((X_BED_SIZE) / 2) // X point for Z homing when homing all axes (G28). #define Z_SAFE_HOMING_Y_POINT ((Y_BED_SIZE) / 2) // Y point for Z homing when homing all axes (G28). so the new defaults I added in conditionals_post.h are actually used by default, or

  2. change the line #define GRID_MAX_POINTS_X 10 to #define GRID_MAX_POINTS_X 9 or #define GRID_MAX_POINTS_X 11.

I have a slight preference for option 1 because it works whether or not GRID_MAX_POINTS_* is odd.

@Roxy-3D Roxy-3D self-assigned this Jan 9, 2019
@Roxy-3D
Copy link
Member

Roxy-3D commented Jan 9, 2019

I think this looks good... I'm supportive of merging it...

@thinkyhead
Copy link
Member

I'm in favor of merging it once there is also a PR for bugfix-2.0.x.

@thinkyhead
Copy link
Member

@spinorkit — Have you had time to adapt this to bugfix-2.0.x yet?

@spinorkit
Copy link
Author

spinorkit commented Jan 25, 2019 via email

@thinkyhead
Copy link
Member

Looking forward to it!

@thinkyhead thinkyhead merged commit c935a67 into MarlinFirmware:bugfix-1.1.x Feb 27, 2019
thinkyhead added a commit that referenced this pull request Feb 27, 2019
@spinorkit
Copy link
Author

Thanks Scott.

thinkyhead pushed a commit that referenced this pull request Sep 2, 2019
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