Leveling fixes

[tombrazier]

Description

This PR fixes a few small issues with leveling. Commits are:

eabf4c2: Make the check for leveling_active consistent between Planner::apply_leveling() and Planner::unapply_leveling(). No changes in logic. Mostly whitespace differences.

06511a3: With BILINEAR, LINEAR and 3POINT, leveling was not being re-enabled after G29 D because the leveling data was assumed to have been modified when it had not been. (Only local variables inside the G29 code had been updated.)

9817ff4: To re-enable leveling it is best to use the machinery in set_bed_leveling_enabled(). This will particularly mean BILINEAR uses the corrected code for unapplying leveling in b665b6e. It also reduces code duplication.

The pre-commit PLANAR (i.e. LINEAR and 3POINT) and BILINEAR code each roll their own alternative methods. In the case of PLANAR, this logic is wrapped up with code that corrects the Z distance and simply calling set_bed_leveling_enabled() would not suffice.

2376a78: Bed leveling is applied between skew correction and firmware retraction. So it is not enough simply to (un)apply leveling when switching it off/on. It is also necessary to unapply and reapply skew correction. This commit simply causes the entire modifier stack to be applied and unapplied on either side of enabling/disabling leveling.

b665b6e: unapply_leveling() is supposed to reverse the effect of apply_leveling(). However it uses the modified z position to calculate the fade scaling factor. This is incorrect. The correct fade scaling factor must depend on the unmodified z position. Put another way, the linear function y = (1-a).x + b is not the inverse of x = (1+a).y - b but this is the logic that is applied in these functions. This commit applies the correction inversion logic.

Requirements

One of the bed leveling algorithms.

Benefits

See above.

Testing

In general: Level with G29 using any mesh bed leveling algorithm and test with a mesh validation grid, e.g. with G26.

For specific commits:

06511a3:
Before:

>>> G29
Bilinear Leveling Grid:
      0      1      2      3
 0 +0.262 +0.144 +0.057 +0.122
 1 +0.228 +0.157 +0.049 +0.050
 2 +0.213 +0.253 +0.158 +0.098
 3 +0.241 +0.292 +0.263 +0.196
>>> M420
echo:Bed Leveling ON
echo:Fade Height 10.00
>>> G29 D
Bilinear Leveling Grid:
      0      1      2      3
 0 +0.267 +0.160 +0.062 +0.146
 1 +0.238 +0.168 +0.055 +0.061
 2 +0.217 +0.264 +0.179 +0.106
 3 +0.241 +0.298 +0.274 +0.223
>>> M420
echo:Bed Leveling OFF
echo:Fade Height 10.00

Leveling is not ON any more.

After:

>>> G29
Bilinear Leveling Grid:
      0      1      2      3
 0 +0.159 +0.173 +0.074 +0.136
 1 -0.055 +0.131 +0.018 +0.018
 2 -0.155 +0.258 +0.144 +0.066
 3 +0.155 +0.154 +0.090 -0.206
>>> M420
echo:Bed Leveling ON
echo:Fade Height 10.00
>>> G29 D
Bilinear Leveling Grid:
      0      1      2      3
 0 -0.434 -0.787 -1.085 -1.242
 1 -0.456 -0.754 -1.095 -1.311
 2 -0.474 -0.655 -0.971 -1.247
 3 -0.434 -0.595 -0.831 -1.122
>>> M420
SENDING:M420
echo:Bed Leveling ON
echo:Fade Height 10.00

Leveling is still ON after the patch.

9817ff4:
Ensure leveling is switched on after G29. Enable DEBUG_LEVELING_FEATURE and use M111 S32 to check the z offset gets correctly applied at the end of G29.

Bilinear Leveling Grid:
      0      1      2      3
 0 +0.304 +0.207 +0.114 +0.179
 1 +0.265 +0.201 +0.082 +0.068
 2 +0.237 +0.283 +0.204 +0.102
 3 +0.248 +0.312 +0.294 +0.219
G29 uncorrected Z:1.43
  current_position= X152.25 Y166.00 Z1.43 : Leveling OFF
  current_position= X152.25 Y166.00 Z1.21 : ...Now ON
  current_position= X152.25 Y166.00 Z1.21 : sync_plan_position
 corrected Z:1.21
<<< G29  X152.25 Y166.00 Z1.21

The correction from 1.43 to 1.21 is consistent with the measured grid, the nozzle's end position (bearing in mind the nozzle to probe offset) and the fade height.

b665b6e:
Before:

>>> G29
Bilinear Leveling Grid:
      0      1      2      3
 0 +0.306 +0.185 +0.103 +0.147
 1 +0.274 +0.193 +0.074 +0.055
 2 +0.248 +0.281 +0.191 +0.102
 3 +0.259 +0.318 +0.289 +0.208
>>> M114
X:152.25 Y:166.00 Z:1.21 E:0.00 Count X:11107 Y:10456 Z:536
>>> M420 S0
echo:Bed Leveling OFF
echo:Fade Height 10.00
>>> M420 S1
echo:Bed Leveling ON
echo:Fade Height 10.00
>>> M114
X:152.25 Y:166.00 Z:1.22 E:0.00 Count X:11107 Y:10456 Z:538

The Z value changes with M420 S0 followed by M420 S1.

After:

>>> G29
Bilinear Leveling Grid:
      0      1      2      3
 0 +0.325 +0.204 +0.125 +0.164
 1 +0.285 +0.205 +0.099 +0.066
 2 +0.272 +0.297 +0.217 +0.127
 3 +0.296 +0.365 +0.341 +0.262
>>> M114
X:152.25 Y:166.00 Z:1.22 E:0.00 Count X:11107 Y:10456 Z:555
>>> M420 S0
echo:Bed Leveling OFF
echo:Fade Height 10.00
>>> M420 S1
echo:Bed Leveling ON
echo:Fade Height 10.00
>>> M114
X:152.25 Y:166.00 Z:1.22 E:0.00 Count X:11107 Y:10456 Z:555

The Z value does not change with M420 S0 followed by M420 S1.

[thinkyhead]

Thanks for this patch. Let's check more cases to make sure there aren't any coincidences in the results. I'll try out some things in the debugger and with M114 D to check the maths. Some things look wonky, but I might just be misunderstanding the way it works.

[thinkyhead]

The problem in a nutshell…

Leveling:
Zplan = Zcart + Zoffs + (Zmesh * FADE(Zcart))

Unleveling:
Zcart = Zplan - Zoffs - UNFADE(Zplan, Zmesh)

We don't know exactly what UNFADE should do, based only on Zplan and Zmesh. If a mesh correction of 0.1 was being applied at 0.5 (e.g., at Z=5 with a fade of 10) so you got 5.05, working backward we can only know that something close to 5.05 * 0.5 * 0.1 was applied to get 5.05, but we can't glean from 5.05 and 0.1 alone that Z was 5 or that 0.5 is definitely the modifier applied to 5.0. Maybe there is some way to work it out, but I can't come up with any.

[tombrazier]

@thinkyhead I always got told off by my maths teachers for not showing my working! Here you go...

Zplan = Zcart + Zoffs + (Zmesh * FADE(Zcart))

and function FADE() is 1 - Zcart / z_fade_height. So

Zplan = Zcart + Zoffs + (Zmesh * (1 - Zcart / z_fade_height))
      = Zcart + Zoffs + Zmesh - Zmesh * Zcart / z_fade_height
      = Zcart * (1 - Zmesh / z_fade_height) + Zoffs + Zmesh 

This is a simple linear function of Zcart which is easily inverted

Zcart * (1 - Zmesh / z_fade_height) = Zplan - Zoffs - Zmesh

Zcart = (Zplan - Zoffs - Zmesh) / (1 - Zmesh / z_fade_height)

This is one of three linear functions that may be in play depending on whether Zcart is <= 0 or >= z_fade_height. The other two are the ones you were asking about above:
Zcart = z_no_fade = Zplan - Zoffs - Zmesh
Zcart = z_full_fade = Zplan - Zoffs

Here's a graph of the forward apply_leveling() function with (unrealistic but visible on the graph!) values of Zoffs == 1 and Zmesh == 2:

(vertical axis is Zplan and horizontal is Zcart).

So the unapply_leveling() logic has to first work out which line segment is in play. This is fairly simple.

If Zcart <= 0 then it will be Zcart = z_no_fade = Zplan - Zoffs - Zmesh and so we can test for z_no_fade <= 0.

If Zcart >= z_fade_height then it will be Zcart = z_full_fade = Zplan - Zoffs and we can test for z_full_fade >= z_fade_height.

If neither are true then the inverse logic above applies and

Zcart = (Zplan - Zoffs - Zmesh) / (1 - Zmesh / z_fade_height)
      = z_no_fade / (1 - Zmesh / z_fade_height)

Note there is also a pathological case where z_fade_height <= Zmesh. In this case the middle line segment is horizontal or has a negative gradient, e.g.

In this case there are more than one possible values for Zcart for some values of Zplan. This isn't really an issue as long as we pick one of them. We're just making sure that CurrentPosition has a Z value which is consistent with where the steppers are - none of them is the "right" one. The code just ends up picking the first one it tries, i.e. Zcart = z_no_fade.

[tombrazier]

PS I have done more testing than I showed above. That was mainly to give an idea of the type of testing I have done and others could do. Specifically, I have tested multiple values of Zcart <= 0, Zcart >= z_fade_height and values in between.

[tombrazier]

PPS I also have plans to use the fixed offset plus fadable offset for BILINEAR to minimise the standard deviation of the fadable offsets. Fixed offset would be the average Z value of the mesh points and then the whole mesh gets shifted to compensate. I think this will reduce issues with z fading, especially for those whose Z=0 is any distance from the average bed height.

[thinkyhead]

Thanks for elaborating! It took me some more time to finally visualize how the fade amount maps predictably within the fade region and how that is reversible.

Note there is also a pathological case where z_fade_height <= Zmesh…

There may not be much sense in having a z_fade_height under 2mm, so if that was enforced it would eliminate the potential for this kind of edge case.

… then the whole mesh gets shifted to compensate. I think this will reduce issues with z fading …

Yes, that should also help, as we want the deviations to be as small as possible. The extra Z offset found in MBL was left out of ABL bilinear leveling originally just to save cycles on float processing, but now with some cycles to spare it should be restored as an option.

[thinkyhead]

The reason this line exists is to prevent re-enabling leveling when you have a broken (incomplete) mesh.

[tombrazier]

Yes, but you don't have a broken mesh at this point because the changes are held in local variable abl.

[tombrazier]

That's new behaviour in #23868. I should have included this change then.

[thinkyhead]

Yes, but you don't have a broken mesh at this point because the changes are held in local variable abl.

No, there is no temporary mesh being stored in the abl variable. The bed mesh is altered directly as it is probed.

[tombrazier]

No, there is no temporary mesh being stored in the abl variable. The bed mesh is altered directly as it is probed.

Not any more since #23868. Line 733:

            abl.z_values[abl.meshCount.x][abl.meshCount.y] = z;

and then later on line 808

        COPY(bedlevel.z_values, abl.z_values);

This was so that G29 D would not alter the mesh, as documented.

[thinkyhead]

I see, then technically it's alright in its current form.

I didn't realize the new code was adding so much extra SRAM cost to PROBE_MANUALLY, which is meant to be the cheaper option. It would be helpful to provide a switch to use the old in-place modification – for the benefit of tiny boards that really need to squeeze out extra SRAM space.

In general, whenever making any change that will impact SRAM usage more than a little, it's good practice to make the extra cost optional, rather than impose it. That way we can still move forward and add new intelligence using large buffers on more powerful boards, but also keep old configurations' build sizes from continually increasing in size.

[tombrazier]

That makes sense to me, especially as I started this 3D printing lark with a 128k Sanguino and even now am still using an ATmega2560 based board (the AVR architecture is quite dear to my heart).

How about making the dry run functionality optional? I can't think of a way of offering dry run that actually works but does not end up temporarily holding two copies of the mesh.

Edit: Oh, I see, not temporarily but permanently in the case of PROBE_MANUALLY.

[tombrazier]

Whilst we're at it, I don't think we actually need to store abl.eqnAMatrix. It just stores (x, y, 1) for each probe point and later uses the x and y to build a vector (x, y, 0). So the 1 is redundant. And the x and y can be calculated when abl.eqnAMatrix is used to print the topography map. That would save 3 * GRID_MAX_POINTS_X * GRID_MAX_POINTS_Y * sizeof(float) bytes if PROBE_MANUALLY is used with AUTO_BED_LEVELING_LINEAR).

[tombrazier]

@thinkyhead I'll attack these if you would like me to.