Suggestion for improving PROBE_CALIBRATE usability

Developers
17

If I’m understanding some of the proposals here, they involve disabling distance checks on PROBE_CALIBRATE and/or TESTZ. This would not be a good idea, because movements generated by these commands can still permanently damage an endstop. That is, in the example above with position_endstop: 0 and position_min: -1, a PROBE_CALIBRATE that commands 4mm of descent on the stepper is likely to damage the endstop.

Valid concern, and easily addressable (which is all in the OP as it stands). So the following conditions have to be met before the kin.check_move() error is ignored in toolhead.move() during probe calibration:

  1. A manual probe is in progress (See verify_no_manual_probe)
  2. Offending axis have to be the axis where the probe that is being calibrated is assigned as a virtual endstop - and only that axis.

These two checks ensure that:

  1. Move out of bounds errors are only ignored during an active manual probe procedure.
  2. We’re not enforcing invalid limits of the axis (an uncalibrated probe = uncalibrated position_endstop = invalid position_min.)
  3. We know there’s no endstop that can be ripped off on that axis (the endstop is the probe that is being calibrated).

Does that address your concern?

I’m not sure what is being proposed here. As before, we could implement the existing “carriage check” along with a new “estimated bed distance check during normal printing”, but I’m being told that is not what is being proposed.

I’ll try defining what i laid out in the OP as succinctly as i can.

The problem
Users have to specify a negative position_min when calibrating a probe that’s also used as an endstop (extremely common, all the 7 printers i’ve owned had Z axes that work this way), this is a problem for the following reasons:

important preface: This is seen described from the users perspective, assume no knowledge of klipper internals:

  1. It’s not intuitive, the minimum position on Z should intuitively be 0, that’s when the nozzle contacts the bed. This is ubiquitous in 3d printing. It is also how it’s explained in several places in the klipper documentation, i can provide sources if needed.
  2. It’s an extra step in the calibration process, with no benefits.
  3. After probe_calibration, the negative position_min is still in the config. The user’s axis is now fully allowed to travel beyond the safe limits of their printer. G0 Z-5 means broken hotend / toolhead, indentation in print surface, bent bed plate and other expensive situations. This is what i characterized previously as the “disaster” that the user has been instructed to set themselves up for. The user never intended this to happen - they want the printer to yell at them if they accidentally try to move beyond bed/nozzle contact - as much as it is possible given imperfect bed shapes, probe drift and what have we.
  4. (related to 3) The status quo is that we not only actively tell users to effectively disable the position_min: 0 limit - it’s a requirement to run PROBE_CALIBRATE, it will unexpectedly fail otherwise. There is nothing as frustrating as damaging your sparkly new hardware before you’ve even seen plastic coming out of it. It’s still frustrating if it happens months later.
  5. The bed_level documentation says position_min: -2. This is not enough for several probe types on the market. The user now has to edit the config a second time.
  6. The Move out of bounds error (which in normal circumstances are good) they’re getting doesn’t make much sense unless they know what’s going on under the hood (what you, Kevin, just explained in abstract and principal terms). In the user’s mind, there is no bounds on Z yet. They haven’t calibrated it.
  7. The position_min limit is arbitrary until the probe has been calibrated. The odds that it is correct on first run are astronomically small - there is simply no point in enforcing it during calibration provided that probe is used as a virtual endstop pin for that axis.
  8. The documentation does not tell the user to reset position_min to 0 even though the probe will cause the axis’ endstop_position to be equal to it’s z_offset. That means: z=0 equals nozzle/bed contact according to last calibration. Even if it did, it’s very easy to forget.

Feel free to counter or supplement these points.

My proposed solution, adapted to address your (Kevin’s) concerns

  1. Add an optional “triggering_axes” parameter to CommandError (or extend it as MoveError) in klipper/klippy/gcode.py:8
  2. Change all kinematic classes’ implementation of check_move() to aggregate rail indexes with offending limits, translate it to axes (xyz) and pass it along to the existing move.move_error call which would then pass it to the exception in klipper/klippy/toolhead.py:58
  3. Modify toolhead.move() to wrap the kin.check_move() call in a try/catch clause and check the axes on the error instance, if present. If the offending axis matches the axis for which the probe of a currently in progress manual_probe procedure (the inverse of verify_no_manual_probe) is used as the virtual_endstop - ignore the error and proceed with adding the move to the move_queue.

I’m not aware if there are any other steps that run kin.check_move() again when the move is flushed, if that’s the case, i would have to look at that too.

Currently known side effects of proposed solution

  1. Besides PROBE_CALIBRATE, ManualProbeHelper is also used for calibrating axis twist compensation, which means AXIS_TWIST_COMPENSATION_CALIBRATE will be affected. This my or may not be desired. If we want to avoid that i will find a way.

Feel free to critique the implementation and suggest alternative ways to accomplish this goal without major changes to klippers internal behavior. However, if y’all think there’s a deeper flaw with how the boundary checks are implemented (i don’t currently think that’s the case, even if i may have other architectural preferences), i will be happy to discuss that in another thread.

DIsclaimer
This is not intended to modify the behavior of any moves outside of an active manual_probe procedure. It’s not intended to address similar inconviences with bed_mesh, z_tilt, skew, z_thermal_adjust or any other Z correcting functionality. This only addresses probe calibration for virtual endstops, which is a very clear cut case where this boundary check serves no purpose and worsens the user experience.

I hope this makes it clear what i intended with this thread. I’m not saying there aren’t other areas with similar challenges worth discussing, i’m saying those are more complicated, and i prefer fixing one problem at a time. I do not think there’s any inherent problem in the boundary checks, but PROBE_CALIBRATE for virtual_endstops is one very clear exception, unless there’s something significant i’m missing. So far nothing has been brought up to counter this claim. 100% of my users who use probes, use them as virtual_endstops. I currently have over 26.000 image downloads.

Thank you for coming to my TED talk :joy: