Add 'safe_z_lift' module for safe lifting of (unhomed) Z axis

[rschaeuble]

Introduce new module: safe_z_lift.

When the printer is at or near Z-max, operations that require a Z hop (like safe_z_home or
dockable_probe deployment) can crash into the axis' mechanical limits. The safe_z_lift module allows the printer to
perform these lifting moves while monitoring a specific endstop (Z-max endstop). If the endstop is triggered during the
lift, the movement stops immediately, but the operation proceeds without error.

Checklist

• pr title makes sense
• added a test case if possible
• if new feature, added to the readme
• ci is happy and green

[rschaeuble]

Before this gets merged, it would be great if a few people could (carefully) test this with their particular setup. I didn't have any issues with this so far, but I can't test any possible setup out there.

[rschaeuble]

This is of course not the only way to implement this. Alterantives:

• define the max endstop pin in stepper_z; put the new code into an existing module (where?)
• make this module always-on, so that even if the user doesn't configure it, safe_z_home and dockable_probe don't need two code paths (one with safe_z_lift, one without).
• make this more generic, so it can be used for every axis. Don't think this is worth the effort.

Looking forward to input on this.

[dalegaard]

Make this an instance variable - we have a connect handler where we can do this lookup once, so it doesn't need to be done for every lift.

[rschaeuble]

Fixed.

[dalegaard]

This can be merged with the inner if, so we end up with if..elif..else

[rschaeuble]

Fixed.

[dalegaard]

Should just be called _z_lift because it internally figures out if it can be done safely or not.

[rschaeuble]

Fixed.

[dalegaard]

There are a bunch of places that use this pattern, so I think this should be introduced in a different patch if we want to do it.

[rschaeuble]

Inlined for now.

[dalegaard]

Do this in klippy:mcu_identify handler instead. As the system is currently set up, that's the only safe place to register endstop-motor bindings. See e.g. bltouch implementation for example.

[rschaeuble]

Fixed.

[dalegaard]

Call this _lift or _z_lift as it might not be safe and caller shouldn't care.

[rschaeuble]

Fixed.

[dalegaard]

* define the max endstop pin in stepper_z; put the new code into an existing module (where?)
* make this module always-on, so that even if the user doesn't configure it, safe_z_home and dockable_probe don't need two code paths (one with safe_z_lift, one without).

I think this approach is a little nicer. It lets you set different endstops for different motors, so I guess you could make it easier to express having multiple endstops and stopping at the first one that triggers. It would need an always-on module to handle the shared code.

* make this more generic, so it can be used for every axis. Don't think this is worth the effort.

If there's a shared module it becomes easier - the shared module can find all motors that touch the axis that is moving(e.g. z), and then add the endstops for all those to the group of endstops being moved towards.

If you want to implement the more general approach that'd be great, but as the approach you are showing here could be trivially extended in the future to be more general, I don't particularly mind moving forward with the current approach.

Best regards,
Lasse

[rschaeuble]

@dalegaard: if you can assist with some guidance, I'm certainly willing to make this a more generic module.

I think the most complex topic is extending the current endstop config. We don't have min_endstop and max_endstop, but just enstop_pinz (and homing_positive_dir` to make it even more complex ;-) ). So we'd have to add an "other side of the axis" endstop. Not sure how best to name this.

Having this would allow a much more generic command though. Something like SAVE_MOVE AXIS="x" DIST=100. Maybe with an additional ALLOW_UNSAFE=0|1 flag for cases in which you'd like a safe move, but would be fine with an unsafe one (like safe_z_home and dockable_probe) require it internally.

Naming: I suggest naming this module "safe_move".

Here's a basic implementation plan:

• Rename module to safe_move, Gcode command to SAVE_MOVE
• Add "opposite_endstop_pin" (or however we want to name it) to stepper.py.
  • This will take care of registering it.
• Save mode code would check the rails for the given axis to see if there's endstop in the desired direction.
  • If not, and error is reported, unless ALLOW_UNSAFE is set.
  • safe_z_home and dockabe_probe are changed to use the new module.

Does that sound reasonable?

[rschaeuble]

I rewrote it, going for the more generic approach.

Still has a few open points/issues:

• safe_move.py: see TODO "is curtime the correct time to use?". I'm really not sure if reactor.monotonic()is the right timestamp to use here.
• sensorless homing: trying to use the virtual endstop for both directions results in "pin virtual_endstop used multiple times in config". Should/can we offer a nice solution here?
• SAFE_MOVE axis=z dist=-50 speed=5 with probe (Klicky-00) attached. Move stops on contact or when distance has been covered. Then Z is lifted up (very small distance, maybe 2mm), and the following error is reported:
  Must home axis first: 200.000 260.000 227.000 [0.000]. Not sure yet what to do about this (or if I even should do something about this).
  • EDIT: see next comment about homing for an explanation.
• Would this work unchanged on a printer with polar (or other strange) kinematics?

[rschaeuble]

Ok, so I just realized that in the direction configured for homing this is basically a homing procedure. In -Z direction this means that on my printer it will attach the probe (if not yet attached) before and detach it afterwards.
That explains the error got in testing: I hadn't home X and Y before the test, so it couldn't detach the probe.

Now what does that mean for the opposite_endstop? Do we want full support for all homing parameters there as well (including second homing, reducing current etc.)? That would make this feature rather huge and probably require lots of additional code.
On the other hand, for what I intended this feature, it would be fine if we would/could all the additional logic of HomingMove for all directions, and let the user of the Gcode set everything up (run current, attach probe).

@dalegaard (and other Kalico maintainers): I'd like to hear your opinion on that.

[dalegaard]

I think it would make more sense config wise if the user either set endstop_pin(like today) or endstop_min_pin+endstop_max_pin. The "opposite" pin I think will be quite confusing in practice, especially for helping debug, because it'll sometimes be the maximum and sometimes the minimum position. Kalico would pick the endstop pin matching the direction of travel. If the user provided endstop_pin, it is equivalent to either endstop_min_pin or endstop_max_pin depending on the homing direction. The other pin is unavailable/errors when user attempts to use it.

I think this also solves the other question, because the endstop pins are now equivalent and are simply picked between when homing starts. That way we can even use sensorless homing for this safety functionality(though we may need to add some extra stuff there to make homing current per-endstop-and-stepper or something).

For now I don't think we need to let users actually interact directly with the added endstop - as long as dockable probe, safe_z_home, and friends can use it, that's fine imo. PrinterHoming currently has a probing_move, and a similar endstop_move function could be added here, that takes the endstops and move to perform, and returns if an endstop was activated during the move or not(and if yes, where). safe_z_tilt(+friends) would then simply use this function to perform the lift.

Best regards,
Lasse

[rschaeuble]

@dalegaard

I reworked stepper.py; it now wants endstop_{min,max}_pin or endstop_pin.
I also added PrinterHoming.endstop_move, which is used by the safe_move module.

I think the main remaining issue with using sensorless for that is duplicate pin usage. Not sure if we can allow that without the user having to configure duplicate_pin_override. Haven't really spent time looking into this yet, though.

My motivation for exposing SAFE_MOVE is twofolder: it makes testing easier/quicker, and it could be useful for homing_override macros. We can remove if at the end, though, if you'd prefer that.
Keeping the safe_move module is probably useful, as it saves redundant code in safe_z_home and dockable_probe.

The latest changes are not yet really tested; going to do that later today. Just wanted to know if that's going in the right direction.

[dalegaard]

I've done comments where it makes sense.

Best regards,
/Lasse

[dalegaard]

Goes away.

[rschaeuble]

I'll keep this until everything else is fine and we make the final decision about the SAFE_MOVE Gcode command.

[dalegaard]

For the actual endstops, I think it should be reworked so we now always group endstops in to either min or max. The legacy endstop is used as either min or max depending on the homing direction. It should be possible to completely forget about the legacy endstop once the config has been read and stuff has been set up.

I would introduce an EndstopCollection class which holds the previous endstops and endstop_map for a PrinterRail, and all endstop handling moves to that. PrinterRail then gets endstops_min and endstops_max which are EndstopCollections. If the user gave endstop_pin, we figure out based on the homing direction if the endstop should go in min or max at that point.

Primary endstop means the endstop of the first motor on the rail, so e.g. [stepper_z] has the primary endstop, and [stepper_z0] can have a different endstop, in which case it would be secondary. The distincting is mainly needed for homing with probes, because the probes z_offset replaces the position_endstop value. In the new scheme there would be two primary endstops per rail, one for min and one for max. Legacy disappears completely.

Best regards,
/Lasse

[rschaeuble]

@dalegaard: I implemented your suggestion with EndstopCollection. At least I hope so ;-)
It certainly makes PrinterRail a lot easier. It's still a lot of code though. Either it really has to be that complex, or I'm missing the right abstraction to make it easier.

This still needs testing, and I will make another pass trying to simplify it (and to review for correctness), but that might have to wait until the weekend. Would be nice to get some quick feedback though if that's going in the right direction.

I will look at your suggestions about moving get_endstops_for_direction to the kinematics after we're happy with stepper.py. One large construction site at a time ;-)

[rschaeuble]

I improved the code in steppers.py. At least for me readability is better now. Tested it thoroughly as well (with the hardware configuration I have available here).

@dalegaard: is this now more or less how you envisioned it? If yes, I'd look at moving get_endstops_for_direction to the kinematics.

[dalegaard]

This is looking really good overall! I think getting the direction coordination moved to kinematics will tie it together nicely. Right now we still can't use this with deltas or any other kinematic that doesn't have 1-1 rail-axis mapping. That includes wanting to use this new system for x or y on a CoreXY.

Great job on this!

Best regards,
Lasse

[dalegaard]

The endstop might trigger AR exactly the end position the caller requested. Then the positions would be the same but the endstop may or may not have triggered. The caller may want to do something different depending on if the endstop was hot or not, so I think returning it is good. The second return element could be a string/enum thing with "full_move", "hit_endstop", or "already_at_endstop" to signify what happened.

[dalegaard]

Might be good to expose "allow_unsafe" to gcode as well, so macro packs can benefit from safe home when the printer supports it.

[rschaeuble]

Implemented both changes.
the "move result" is not yet exposed further up the chain, but I plan to export stats for the safe_move module.

[rschaeuble]

I started work on a get endstops method in Kinematics. A few observation:

1. For CoreXY, just returning the correct endstop (min/max for the requested axis) is not enough. We also need to cross-connect endpoints of one rails with the steppers of the other rail. CoreXY.init() already does this for the homing direction; we just need to extend this so it happens for both directions. Without the cross-connection, one of the steppers will not be stopped immediately but with a slight delay. Which we definitely don't want to happen. Using any_complete instead of multi_complete (more on that later) should also work, but cross-connecting for all directions seems the better solution.

2. CoreXZ follows the same prinicple as CoreXY, just with different axis combinations.

3. Cartesian is simple. As long as there is only one toohead. IDEX is more complex, and I have no way of testing it.

4. Delta: We can't cross-connect the rails, as that would break concurrent homing of all three towers. A SAFE_MOVE towards +Z would move each tower until it either hits its endstop or has covered the requested distance. Which sounds safe to me, but it's not really what I would expect, as that would also move the toolhead in X/Y. So I think we want to replace multi_complete with any_complete (which has to be implemented), so we stop the move when any endstop triggers.
   Where I'm completely unsure, as I have no clue about the mathematics of delta printers, is whether a SAFE_MOVE in X or Y is actually possible while guaranteeing that the nozzle will never engrave the bed.

5. Polar: Z is trival (same as Cartesian). I assume (but am not certain) that we could cross-connect the arm and bed rails and to an endstop_move using any_complete with all configured enstops of bed and arm. But to be honest: this is just a guess; I understand Polar printers even less then deltas ;)

Overall, I feel confident implementing this for CoreXY, CoreXZ and non-IDEX Cartesian. So I did that; for everything else an error is raised. If should be relatively easy for someone with a respective printer to add support for SAFE_MOVE for the other kinematics.

Once we're happy with this part as well, I'm going to collect the other, smaller issues that are still open, add a few more, and put them into a new comment, so it's easier to keep an overview of what still has to be done. But I think the most difficult part is done :-)

[rschaeuble]

I think all comments/suggestions/issues of the discussion so far have been handled/implemented.

Here are a few more points from my notes

• homing_move sends homing:homing_move_begin/homing:homing_move_end events. Asendstop_move uses homing_move internally, it also sends these events.
  • Looks safe to keep it that way.
• home_rails, which calls homing_move, sets homing accel and current, and calls _reset_endstop_states. Should endstop_move do that as well? On the one hand, this looks useful. On the other hand, this requires knowledge about the rails, which safe_move does intentionally not have.
  • I think not having this feature is good enough for now, even though this also means that sensorless can't be used for safe_move.
• If we want to allow sensorless homing to be used for safe_move as well, we'd have to use allow_multi_use_pin for the endstop pin. Any reasons not to do so, at least for virtual_endstop pins?
  • Not supporting sensorless should be fine for now. After all, save_move's main target is the Z axis, which I've never seen being homed sensorlessly.
• should we use safe_move for retracts during homing/probing as well? I think we should.
• safe_move should abort with an error if the endstop is already triggered before starting the move. This situation is indistinguishable from a broken endstop wire, and thus not safe.
• Should we introduce a base class for all kinematics with default methods? Would be cleaner design-wise, but doens't have to be in this PR.
• Needs a docs page with recommendations for endstop setup (so already_at_enstop never happens during normal operation.

@dalegaard: I would appreciate your feedback on these points.
Also, if you think some of these can/should be handled later in a separate PR, please say so.