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.

Sign up for GitHub

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

Leveling fixes #24188

Merged
merged 12 commits into from
May 19, 2022
Merged

Leveling fixes #24188

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
eabf4c2
Consistency between apply_leveling() and unapply_leveling()
tombrazier May 5, 2022
06511a3
G29 D (dryrun) should not change leveling state
tombrazier May 16, 2022
9817ff4
Re-enable leveling using the proper mechanisms for BILINEAR - leave P…
tombrazier May 16, 2022
2376a78
Enable/disable leveling in the correct positon within the modifier stack
tombrazier May 16, 2022
b665b6e
Correctly apply leveling, using the unleveled height as opposed to th…
tombrazier May 16, 2022
547494a
Merge branch 'bugfix-2.0.x' into pr/24188
thinkyhead May 17, 2022
2cbf0a0
cleanup
thinkyhead May 17, 2022
769ddba
Common name for bedlevel objects
thinkyhead May 19, 2022
3cf4540
mesh_index_to_xpos => get_mesh_x
thinkyhead May 19, 2022
d5f724a
Use common accessors
thinkyhead May 19, 2022
59459ed
Merge branch 'bugfix-2.0.x' into pr/24188
thinkyhead May 19, 2022
90d82d1
cleanup
thinkyhead May 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions Marlin/src/feature/bedlevel/bedlevel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,15 +70,17 @@ void set_bed_leveling_enabled(const bool enable/*=true*/) {
if (planner.leveling_active) { // leveling from on to off
if (DEBUGGING(LEVELING)) DEBUG_POS("Leveling ON", current_position);
// change unleveled current_position to physical current_position without moving steppers.
planner.apply_leveling(current_position);
planner.leveling_active = false; // disable only AFTER calling apply_leveling
planner.apply_modifiers(current_position);
planner.leveling_active = false; // disable only BETWEEN calling apply_modifiers() and unapply_modifiers()
planner.unapply_modifiers(current_position);
if (DEBUGGING(LEVELING)) DEBUG_POS("...Now OFF", current_position);
}
else { // leveling from off to on
if (DEBUGGING(LEVELING)) DEBUG_POS("Leveling OFF", current_position);
planner.leveling_active = true; // enable BEFORE calling unapply_leveling, otherwise ignored
// change physical current_position to unleveled current_position without moving steppers.
planner.unapply_leveling(current_position);
planner.apply_modifiers(current_position);
planner.leveling_active = true; // enable BETWEEN calling apply_modifiers() and unapply_modifiers()
planner.unapply_modifiers(current_position);
if (DEBUGGING(LEVELING)) DEBUG_POS("...Now ON", current_position);
}

Expand Down
13 changes: 6 additions & 7 deletions Marlin/src/feature/bedlevel/mbl/mesh_bed_leveling.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,20 +102,19 @@ class mesh_bed_leveling {
return z1 + delta_a * delta_z;
}

static float get_z(const xy_pos_t &pos
OPTARG(ENABLE_LEVELING_FADE_HEIGHT, const_float_t factor=1.0f)
) {
#if DISABLED(ENABLE_LEVELING_FADE_HEIGHT)
constexpr float factor = 1.0f;
#endif
static float get_z_correction_fixed() {
return z_offset;
}

static float get_z_correction_fadable(const xy_pos_t &pos) {
const xy_int8_t ind = cell_indexes(pos);
const float x1 = index_to_xpos[ind.x], x2 = index_to_xpos[ind.x+1],
y1 = index_to_xpos[ind.y], y2 = index_to_xpos[ind.y+1],
z1 = calc_z0(pos.x, x1, z_values[ind.x][ind.y ], x2, z_values[ind.x+1][ind.y ]),
z2 = calc_z0(pos.x, x1, z_values[ind.x][ind.y+1], x2, z_values[ind.x+1][ind.y+1]),
zf = calc_z0(pos.y, y1, z1, y2, z2);

return z_offset + zf * factor;
return zf;
}

#if IS_CARTESIAN && DISABLED(SEGMENT_LEVELED_MOVES)
Expand Down
19 changes: 8 additions & 11 deletions Marlin/src/gcode/bedlevel/abl/G29.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -729,7 +729,6 @@ G29_TYPE GcodeSuite::G29() {

#endif

abl.reenable = false;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@thinkyhead thinkyhead May 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@tombrazier tombrazier May 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

idle_no_sleep();

} // inner
Expand Down Expand Up @@ -916,31 +915,29 @@ G29_TYPE GcodeSuite::G29() {
if (DEBUGGING(LEVELING)) DEBUG_POS("G29 corrected XYZ", current_position);
}

// Auto Bed Leveling is complete! Enable if possible.
planner.leveling_active = !abl.dryrun || abl.reenable;
// Sync the planner from the current_position
if (planner.leveling_active) sync_plan_position();

#elif ENABLED(AUTO_BED_LEVELING_BILINEAR)

if (!abl.dryrun) {
if (!abl.dryrun || abl.reenable) {
if (DEBUGGING(LEVELING)) DEBUG_ECHOLNPGM("G29 uncorrected Z:", current_position.z);

// Unapply the offset because it is going to be immediately applied
// and cause compensation movement in Z
const float fade_scaling_factor = TERN(ENABLE_LEVELING_FADE_HEIGHT, planner.fade_scaling_factor_for_z(current_position.z), 1);
current_position.z -= fade_scaling_factor * bbl.get_z_correction(current_position);
// Auto Bed Leveling is complete! Enable if possible.
set_bed_leveling_enabled(true);

if (DEBUGGING(LEVELING)) DEBUG_ECHOLNPGM(" corrected Z:", current_position.z);
}

#endif // ABL_PLANAR

// Auto Bed Leveling is complete! Enable if possible.
planner.leveling_active = !abl.dryrun || abl.reenable;
} // !isnan(abl.measured_z)

// Restore state after probing
if (!faux) restore_feedrate_and_scaling();

// Sync the planner from the current_position
if (planner.leveling_active) sync_plan_position();

TERN_(HAS_BED_PROBE, probe.move_z_after_probing());

#ifdef Z_PROBE_END_SCRIPT
Expand Down
58 changes: 35 additions & 23 deletions Marlin/src/module/planner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1587,13 +1587,14 @@ void Planner::check_axes_activity() {

#if ENABLED(ENABLE_LEVELING_FADE_HEIGHT)
const float fade_scaling_factor = fade_scaling_factor_for_z(raw.z);
#elif DISABLED(MESH_BED_LEVELING)
#else
constexpr float fade_scaling_factor = 1.0;
#endif

raw.z += (
#if ENABLED(MESH_BED_LEVELING)
mbl.get_z(raw OPTARG(ENABLE_LEVELING_FADE_HEIGHT, fade_scaling_factor))
mbl.get_z_correction_fixed() +
fade_scaling_factor ? fade_scaling_factor * mbl.get_z_correction_fadable(raw) : 0.0
#elif ENABLED(AUTO_BED_LEVELING_UBL)
fade_scaling_factor ? fade_scaling_factor * ubl.get_z_correction(raw) : 0.0
#elif ENABLED(AUTO_BED_LEVELING_BILINEAR)
Expand All @@ -1605,37 +1606,48 @@ void Planner::check_axes_activity() {
}

void Planner::unapply_leveling(xyz_pos_t &raw) {
if (!leveling_active) return;

if (leveling_active) {
#if ABL_PLANAR

#if ABL_PLANAR
matrix_3x3 inverse = matrix_3x3::transpose(bed_level_matrix);

matrix_3x3 inverse = matrix_3x3::transpose(bed_level_matrix);
xy_pos_t d = raw - level_fulcrum;
inverse.apply_rotation_xyz(d.x, d.y, raw.z);
raw = d + level_fulcrum;

xy_pos_t d = raw - level_fulcrum;
inverse.apply_rotation_xyz(d.x, d.y, raw.z);
raw = d + level_fulcrum;
#elif HAS_MESH

#elif HAS_MESH
#if ENABLED(MESH_BED_LEVELING)
const float z_correction_fixed = mbl.get_z_correction_fixed();
#else
constexpr float z_correction_fixed = 0.0f;
#endif

#if ENABLED(ENABLE_LEVELING_FADE_HEIGHT)
const float fade_scaling_factor = fade_scaling_factor_for_z(raw.z);
#elif DISABLED(MESH_BED_LEVELING)
constexpr float fade_scaling_factor = 1.0;
float z_correction_fadable =
#if ENABLED(MESH_BED_LEVELING)
mbl.get_z_correction_fadable(raw);
#elif ENABLED(AUTO_BED_LEVELING_UBL)
ubl.get_z_correction(raw);
#elif ENABLED(AUTO_BED_LEVELING_BILINEAR)
bbl.get_z_correction(raw);
#endif

raw.z -= (
#if ENABLED(MESH_BED_LEVELING)
mbl.get_z(raw OPTARG(ENABLE_LEVELING_FADE_HEIGHT, fade_scaling_factor))
#elif ENABLED(AUTO_BED_LEVELING_UBL)
fade_scaling_factor ? fade_scaling_factor * ubl.get_z_correction(raw) : 0.0
#elif ENABLED(AUTO_BED_LEVELING_BILINEAR)
fade_scaling_factor ? fade_scaling_factor * bbl.get_z_correction(raw) : 0.0
#endif
);
const float z_full_fade = raw.z - z_correction_fixed;
const float z_no_fade = z_full_fade - z_correction_fadable;
tombrazier marked this conversation as resolved.
Show resolved Hide resolved

#if ENABLED(ENABLE_LEVELING_FADE_HEIGHT)
if (!z_fade_height || z_no_fade <= 0.0f)
raw.z = z_no_fade;
else if (z_full_fade >= z_fade_height)
raw.z = z_full_fade;
else
raw.z = z_no_fade / (1.0f - z_correction_fadable * inverse_z_fade_height);
#else
raw.z = z_no_fade;
#endif
}

#endif
}

#endif // HAS_LEVELING
Expand Down