Implement CLEAR for SET_KINEMATIC_POSITION

[twelho]

SET_KINEMATIC_POSITION CLEAR=<[X][Y][Z]> clears the homing status (resets the axis limits) without turning off motors. This is particularly useful when implementing safe Z homing in [homing_override] on printers with multiple independent Z steppers (where FORCE_MOVE can't be used):

# Forcibly move Z up (Z hop)
{% set homed = printer.toolhead.homed_axes | upper | list %}
{% set not_homed = ['X', 'Y', 'Z'] | reject('in', homed) %}
{% if not 'X' in homed or not 'Y' in homed %}
  SET_KINEMATIC_POSITION Z=0
  G0 Z{z_hop} F{bltouch_speed}
  SET_KINEMATIC_POSITION CLEAR={not_homed | join}
{% endif %}

This functionality was already available to the safe Z homing module via note_z_not_homed. As a bonus, the _motor_off implementations across various kinematics are also simplified.

I'm not 100% sure whether this should send the toolhead:set_position event since it only modifies the limits and not the position itself. The current consumer (singular, GCodeMove) of that event doesn't seem to need it, but maybe you have some other future plans?

[twelho]

Rebased on top of master and implemented a fix for KEEP=XYZ not keeping anything (bug with falsy values)

[KevinOConnor]

Thanks. As high-level feedback the change seems fine to me. I do have a couple of minor comments.

I'm not sure about adding a new CLEAR_KINEMATIC_POSITION command. I wonder if adding some kind of CLEAR=XYZ type of parameter to the existing SET_KINEMATIC_POSITION may be more appropriate. (Limiting builtin commands reduces possible conflicts with user macros.)

Is there a reason that clear_position() takes a list of axes numbers instead of a string of axes (eg, "xyz")?

If clear_positions() is to be a mandatory method of the kinematic classes it would be good to update docs/Code_Overview.md to include it in the "Adding new kinematics" section (bullet 5).

If you're rebasing this PR it would be nice to have the kinematic clear_position() change in a separate commit from the XXX_KINEMATIC_POSITION command. Though if you're not super familiar with git then it's not a requirement.

-Kevin

[KevinOConnor]

Also, to your earlier question - I don't think the code should call printer.send_event("toolhead:set_position") on a clearing of homing state. That event is intended to inform the gcode parsing code that it has to resynchronize its position with the toolhead code.

In general, I don't think the toolhead needs to be involved in a "clear homing state" request - the low-level code can just directly instruct the kinematic class of the change in state (as was done for note_z_home()). It is not necessary to flush the step generation either.

Along those same lines, maybe clear_homing_state() would be a better name than clear_position()?

-Kevin

[github-actions]

It looks like this GitHub Pull Request has become inactive. If there are any further updates, you can add a comment here or open a new ticket.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

[Sineos]

@twelho any updates?

[twelho]

I'm not sure about adding a new CLEAR_KINEMATIC_POSITION command. I wonder if adding some kind of CLEAR=XYZ type of parameter to the existing SET_KINEMATIC_POSITION may be more appropriate. (Limiting builtin commands reduces possible conflicts with user macros.)

That works as well, I can expose the functionality through CLEAR for SET_KINEMATIC_POSITION.

Is there a reason that clear_position() takes a list of axes numbers instead of a string of axes (eg, "xyz")?

set_position for kinematics takes in homing_axes as numerical values, so I copied that pattern for consistency.

If clear_positions() is to be a mandatory method of the kinematic classes it would be good to update docs/Code_Overview.md to include it in the "Adding new kinematics" section (bullet 5).

Thanks for the pointer, I'll update the the the docs as well.

If you're rebasing this PR it would be nice to have the kinematic clear_position() change in a separate commit from the XXX_KINEMATIC_POSITION command. Though if you're not super familiar with git then it's not a requirement.

Sure, I can do that.

[github-actions]

Thank you for your contribution to Klipper. Unfortunately, a reviewer has not assigned themselves to this GitHub Pull Request. All Pull Requests are reviewed before merging, and a reviewer will need to volunteer. Further information is available at: https://www.klipper3d.org/CONTRIBUTING.html

There are some steps that you can take now:

1. Perform a self-review of your Pull Request by following the steps at: https://www.klipper3d.org/CONTRIBUTING.html#what-to-expect-in-a-review
   If you have completed a self-review, be sure to state the results of that self-review explicitly in the Pull Request comments. A reviewer is more likely to participate if the bulk of a review has already been completed.
2. Consider opening a topic on the Klipper Discourse server to discuss this work. The Discourse server is a good place to discuss development ideas and to engage users interested in testing. Reviewers are more likely to prioritize Pull Requests with an active community of users.
3. Consider helping out reviewers by reviewing other Klipper Pull Requests. Taking the time to perform a careful and detailed review of others work is appreciated. Regular contributors are more likely to prioritize the contributions of other regular contributors.

Unfortunately, if a reviewer does not assign themselves to this GitHub Pull Request then it will be automatically closed. If this happens, then it is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

[twelho]

The changes are now implemented and I've tested that it works as intended on my hardware, so it's ready for review.

[b00gie-dev]

@twelho Awesome! Thank you for your work on that!

[github-actions]

Unfortunately a reviewer has not assigned themselves to this GitHub Pull Request and it is therefore being closed. It is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

[twelho]

Not stale, could someone please take a look at this?

[github-actions]

Unfortunately a reviewer has not assigned themselves to this GitHub Pull Request and it is therefore being closed. It is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

[github-actions]

Unfortunately a reviewer has not assigned themselves to this GitHub Pull Request and it is therefore being closed. It is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

[github-actions]

Unfortunately a reviewer has not assigned themselves to this GitHub Pull Request and it is therefore being closed. It is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

[b00gie-dev]

Can we please reopen this? It‘s still a wanted feature 😊

[austinrdennis]

I'm dying for this to be integrated! It's been tricky to work around this in my homing override and this addition would fix my only remaining problem with that setup.

[Surion79]

i would love to get this implemented

[b00gie-dev]

So what we need to do to get this feature? Someone has to accept it or what's the issue right now?

[twelho]

I believe it is in need of a reviewer. Should there be more changes requested, I'm happy to work on them to get this PR merged.

[pciavald]

Hi, i've managed to apply this PR.

However i'm observing the following behavior:

Recv: // printer['toolhead'].homed_axes =

G28 X

Recv: // printer['toolhead'].homed_axes = x

SET_KINEMATIC_POSITION CLEAR=X

Recv: // printer['toolhead'].homed_axes = yz

[twelho]

Hi, i've managed to apply this PR.

However i'm observing the following behavior:

Recv: // printer['toolhead'].homed_axes =

G28 X

Recv: // printer['toolhead'].homed_axes = x

SET_KINEMATIC_POSITION CLEAR=X

Recv: // printer['toolhead'].homed_axes = yz

Now that I look at this, this is intended behavior. SET_KINEMATIC_POSITION will always set the full kinematic position (for backwards compatibility), the CLEAR option just allows you to discard specific axis after everything has been set. If you want to ensure that un-homed axes remain un-homed, you need to add them to CLEAR, see the not_homed variable in the example in the description.

CLEAR is implemented this way for two reasons: backwards compatibility and to retain the declarative and atomic nature of SET_KINEMATIC_POSITION (i.e., after running the command, you will always end up in a predicable and repeatable kinematic state). This reduces the chances of catastrophic mistakes (crashing the gantry etc.). It also allows combining operations like SET_KINEMATIC_POSITION X=150 Y=150 CLEAR=Z. If you omit an axis, SET_KINEMATIC_POSITION will simply use the current position for it (which is the existing behavior).