Klipper3d / klipper

Klipper is a 3d-printer firmware
GNU General Public License v3.0
8.98k stars 5.17k forks source link

Minor tmc driver improvements #6601

Closed KevinOConnor closed 1 month ago

KevinOConnor commented 1 month ago

A couple of minor enhancements that I noticed after the merge of #6139 .

  1. Simplify the TMCtstepHelper() code.
  2. There is no need to warn the user about setting high_velocity_threshold when using sensorless homing as the sensorless homing can save/restore thigh.

@leptun - it would be great if you could take a look at these changes.

-Kevin

nefelim4ag commented 1 month ago

Defining CoolStep above homing velocities effectively broke sensorless homing. Setting a threshold equal to the homing speed also does not work.

So, I suggest unconditionally resetting it to 0xfffff for sensorless homing.

thigh handling works like a charm, tested with: CoolStep THRS: 50 mm/s High Velocity THRS: 55 mm/s Homing speed: 60 mm/s

leptun commented 1 month ago

I think that is expected? defining a custom coolstep threshold shall be used at all times. So if the coolstep velocity is already set, it doesn't get overridden to 0xFFFFF during the homing move. I actually need to have the coolstep threshold set to 15mm/s during homing, or else the stallguard triggers during the acceleration move. With a non-zero coolstep threshold during homing, I can decrease the stallguard sensitivity to reduce false positives (alongside decreasing currents), making the homing tap softer and more precise.

If you need such a high coolstep threshold, then you should probably use a script that reduces the coolstep threshold at the beginning of home then restores it at the end. This is the exact approach I use for lower homing currents:

[gcode_macro G28]
rename_existing: G28.1
description: Advanced TMC homing move
gcode:
  {% set x = 'X' in params %}
  {% set y = 'Y' in params %}
  {% set z = 'Z' in params %}
  {% set all = not (x or y or z) %}
  {% if all or x %}
    SET_TMC_CURRENT STEPPER=stepper_x CURRENT=0.15
  {% endif %}
  {% if all or y %}
    SET_TMC_CURRENT STEPPER=stepper_y CURRENT=0.18
  {% endif %}
  {% if all or z %}
    SET_TMC_CURRENT STEPPER=stepper_z CURRENT=0.3
  {% endif %}

  G28.1 {rawparams}

  {% if all or x %}
    SET_TMC_CURRENT STEPPER=stepper_x CURRENT={printer['tmc2130 stepper_x'].run_current} HOLDCURRENT={printer['tmc2130 stepper_x'].hold_current}
  {% endif %}
  {% if all or y %}
    SET_TMC_CURRENT STEPPER=stepper_y CURRENT={printer['tmc2130 stepper_y'].run_current} HOLDCURRENT={printer['tmc2130 stepper_y'].hold_current}
  {% endif %}
  {% if all or z %}
    SET_TMC_CURRENT STEPPER=stepper_z CURRENT={printer['tmc2130 stepper_z'].run_current} HOLDCURRENT={printer['tmc2130 stepper_z'].hold_current}
  {% endif %}
nefelim4ag commented 1 month ago

@leptun cares about sensorless homing reliability @nefelim4ag cares about CoolStep current reduction while printing

So, we did a small talk about coolstep threshold and approaches. CoolStep threshold can be used to get reliable sensorless homing and avoid false positivities on the acceleration phase. That is the reason why we honor config value in this specific code if defined.

(we have crazy different setups like Prusa MK3 and 0.5A steppers (possibly high inductance), and RatRig V-core 3.1 with LDO 2504 and 1.768A RMS (low inductance))

My doubt there was that such a low threshold used for homing would be unreliable for printing speeds on CoreXY because if one motor decreases current other one can just trigger motor stall and steps skipping.

I decreased the cool step threshold from 150 mm/s to 50 mm/s, to test reliability on the dummy test print (where I got step skips before with SEIMIN=1 (25% of rated current)), but so far, so good I didn't get step skips (30 min with 300% speed up to 1000 mm/s & 20k acceleration).

So let's assume for now, the threshold for homing speeds shall work for printing in the correct setup. Someone with doubt (like me) who does not use sensorless homing, can set the threshold to value appropriate to personal taste.

And leave the code as is, it works.

Thanks

KevinOConnor commented 1 month ago

Thanks. I'm guessing the comments above are about the existing behavior of not always clearing tcoolthrs during sensorless homing (that is, only set it to 0xfffff if it was zero). This PR should not have changed the behavior of tcoolthrs. If there was a regression introduced in this PR, let me know.

Separately, I don't have a particular preference on sensorless homing wrt tcoolthrs. Unless there is a problem, I'd just assume keep the behavior as it was.

Cheers, -Kevin