Debug of G2-G3 for UBL

[Roxy-3D]

I think G2 & G3 have been broken for a while with various bed leveling schemes.

There is a lot of debug code (that can easily be ignored) in this Pull Request. That will be stripped out before any merge happens. But it may be useful for people to see what is going on with and without the modifications.

I'm trying to get G2 & G3 working for G26 with UBL. But it is my belief that Bi-Linear is also affected by the current code. I would appreciate if somebody can confirm or deny that claim!

I think these changes fix the problem for UBL. (I have not done super extensive QA to insure that yet). But I would like to hear any insight or opinions people have before I continue!

THANKS!

[thinkyhead]

Since arcs can now be drawn vertically and not just in the XY plane, this should be using the L (linear) axis, not Z.

  start_L = current_position[L_axis],

[Roxy-3D]

OK! Do you agree the use of start_L should be applied in the case of Bi-Linear also? Because the leveling is only applied, and never un-applied, I think Bi-Linear will see the nozzle move too much also.

[thinkyhead]

Actually, you might be right to only look at Z, since that's the axis that's getting munged.

But, is there any possibility of integrating UBL with the planner so no workaround is required? Or, could a "ubl.poke()" method be added which tells UBL to update its internal positioning when it might get out of sync with the planner?

[Roxy-3D]

But, is there any possibility of integrating UBL with the planner so no workaround is required?

Yes. And in fact... If you look at the original commented out code I was going to fix it by shifting the planner:: call to ubl.line_to_destination_cartesian(MMS_SCALED(feedrate_mm_s), active_extruder);

But instead... I decided to take a baby step and correct the problem with original planner:: code.

The big reason ubl_motion.cpp is a separate file is because it is more efficient from a CPU cycle count. It optimizes a number of special cases to reduce computation over head. It recognizes:

• movement within the same mesh row
• movement within the same mesh column
• more efficient diagonal movements across mesh lines.

But this comes at the cost of more code that is harder to understand. Any way... The reason for this explanation is to suggest doing the opposite of what you said. Perhaps it makes sense to move ubl's motion routine down into the planner so everybody can use the faster code?

With that said... Right now, UBL's motion routines are not compatible with your SEGMENT_LEVELED_MOVES stuff. If we don't worry about optimizing the CPU cycles to move across the mesh, this feature gets picked up automatically by UBL by moving to the planner's routines.

[thinkyhead]

As I vaguely recall, something in ubl_motion needs to be kicked off so it knows it needs to recalculate for a new XY position, and if positions are changed outside of its purview then it loses track… or something like that. Is that the general gist?

[Roxy-3D]

Ah... I'm not sure about that. That may have been true before all the encapsulation work was done. But right now... I'm not aware of an issue like.

[thinkyhead]

Arcs with movement in the linear axis are sort of like spirals, and that is a supported part of G2/G3, so it is important to update the linear axis on each segment.

[Roxy-3D]

I don't understand what you are trying to communicate. The problem UBL is seeing is the Z-Correction is being applied again and again and is cumulative. By resetting raw[l_axis] to the starting value of Z the code behave properly. Do we need some extra logic to handle your case?

[thinkyhead]

This change breaks arcs that have movement in the linear axis.

[Roxy-3D]

OK. But for sure the code is broken and doesn't work right in the normal Z-Axis.
Perhaps it needs more work. But the original Arc function got broken by the Linear Axis changes.

I don't know what has to be done to fix the Linear Axis side of this.

[thinkyhead]

Example: If a typical XY arc move is sent, where the destination Z is the same as the starting Z, then the value of linear_per_segment should be zero. If for some reason it isn't zero, perhaps leveling is being applied to the starting or ending Z coordinate at the wrong time during the arc move.

[Roxy-3D]

I think the root cause of the problem is the Z-Height Correction is being applied at a different time between the different leveling systems. I copied what the original Mesh Leveling did. It would take the current Z, add on the correction, and then call the planner::.

But in the case where l_axis == Z_AXIS, we can't keep incrementing the Z Axis for each segment. That causes the 'Air Print' mentioned here: #12386 (comment)

Right now... I have the changes wrapped with #if ENABLED(UBL).

    #if ENABLED(AUTO_BED_LEVELING_UBL)
      raw[l_axis] = start_L;
    #else
      raw[l_axis] += linear_per_segment;
    #endif

For the time being... Could we add one more qualifier to resolve the issue??? something like:

    #if ENABLED(AUTO_BED_LEVELING_UBL) && ENABLED(CARTESIAN)
      raw[l_axis] = start_L;
    #else
      raw[l_axis] += linear_per_segment;
    #endif

[thinkyhead]

That would be okay, but we should figure out a proper resolution soon since arcs are allowed to be slanted in the linear axis (Z for XY arcs). The way other leveling systems work, the Z adjustment is transparent to the high-level code, but the Z that gets passed on to the Z stepper in terms of steps is the adjusted version. (Same goes for XY with the planar matrix-based systems.) If UBL could move its Z adjustments to planner.apply_leveling/unapply_leveling for all cases —including least-segmented Cartesian— but still preserve its optimizations for straight lines and so on, that would give us the best of both worlds.

[Roxy-3D]

If UBL could move its Z adjustments to planner.apply_leveling/unapply_leveling for all cases...

I think I agree this is the best path forward. Let me think about it a little bit more. But it does seem obvious it would be best for all the bed leveling schemes to do the Z-Height correction at the same place.

[thinkyhead]

When in ZX or YZ orientation, the arc is done vertically, and the linear axis is either Y for ZX or X for YZ.

  #if ENABLED(CNC_WORKSPACE_PLANES)
    AxisEnum p_axis, q_axis, l_axis;
    switch (gcode.workspace_plane) {
      default:
      case GcodeSuite::PLANE_XY: p_axis = X_AXIS; q_axis = Y_AXIS; l_axis = Z_AXIS; break;
      case GcodeSuite::PLANE_ZX: p_axis = Z_AXIS; q_axis = X_AXIS; l_axis = Y_AXIS; break;
      case GcodeSuite::PLANE_YZ: p_axis = Y_AXIS; q_axis = Z_AXIS; l_axis = X_AXIS; break;
    }
  #else
    constexpr AxisEnum p_axis = X_AXIS, q_axis = Y_AXIS, l_axis = Z_AXIS;
  #endif

[cdedwards]

I've run this, and I no longer "air" print circles. All in all, well done.

[Roxy-3D]

Thank You for the confirmation! As it turns out... (We didn't know it at the time!) You could have stopped the "air print" of circles by just turning off ARC_SUPPORT.