Frix-x
Add additional z height safety checks to homing
Fixes issue #298. Request THOROUGH code review. I'm not that experienced with Jinja2 or klipper.
Sorry for all the unverified commits, I'm away from home and using Codespaces which doesn't have my git security key setup. It is actually me making those commits. I think a commit squash is probably in order too since I didn't anticipate making more than one commit. Forgive me, this is first real PR on an actual project.
Summary of changes:
-
Added ZHOPs to X and Y homing sections. Avoids potential nozzle contact with the bed during an edge case where the Z axis is already homed, but too low, then the user tries to home only the X or Y axes without re-homing Z.
-
Added a Z hop to any homing attempt where Z height is unknown (Z axis not homed). Avoids nozzle contact with the bed during an edge case where the printer power is lost or printer is emergency stopped with the nozzle too close to the bed and the user tries to re-home the machine.
Hello,
Thank you very much for this PR. This is a change on a very important part of the config, so I will definitely try it and check everything before merging it.
Be aware that I just moved into a new house and my printers aren't unpacked at the moment, so it could take a couple of days before I find some time to proceed :)
Also, I'm not sure you have seen #169 but it was already an attempt to improve the homing sequence we are aware that there is definitely room for improvements. So your PR is welcomed!
Take your time! I know how fast life can move so don't feel pressured to rush to merge or test. 👍
These edge cases are pretty unlikely to occur during normal use. I only found them because I was doing some testing.
I hadn't seen #169, but it looks like things didn't pan out. Hopefully we can get this working properly this time! Let me know if there are any issues with the PR and I'll try my best to fix it. I really like this project, so I want to see this through. Keep up the great work and thanks for this repo!
Also, if you're curious, I'm using Klippain on my V2.4 and that's what I do all my testing on as well.
On my first look, it looks like that something could be combined. Literally it is "if not z homed, then zhop, otherwise check always for z hop" I think this could be just added at the front as a single block instead of putting it individually in each homing section. It is literally used in every decision route, so it could also just got to the front. Only thing i don't know out of my head what printer.toolhead.position.x has as value if not homed. This would either mean a couple lines more of code for the single block at front solution.
I suggest using something like this: (identation might not be 100% correct)
{% if ('z' not in printer.toolhead.homed_axes) %}
{% if verbose %}
{ action_respond_info("Z height unknown, performing Z-hop...") }
{% endif %}
SET_KINEMATIC_POSITION X=0 Y=0 Z=0
G0 Z{homing_zhop} F{z_drop_speed}
{% elif (printer.toolhead.position.z < homing_zhop) %}
{% if verbose %}
{ action_respond_info("Z too low, performing Z-hop...") }
{% endif %}
G91
G0 Z{homing_zhop} F{z_drop_speed}
G90
{% endif %}
@Surion79 Ya, it probably isn't the most clean implementation. Let me revisit it and see if I can make it more elegant. I should get back to you sometime tomorrow.
well, my last post is basically the PR content (excluding the deletion of the Kinematic zhop under Z). Not sure what else to add for the functionality. But I am curious what else you have in mind :)
if Z is homed and at -5 (which is position_min in Klippain), it would scratch at a very tiny edge case. @Frix-x what do you think? change position_min to -4 in stepper_z oder add a variable zhop to reach the height of 5 if it is below 0?
I opened a new PR with my change and i tested it on my machine and this is not working. Because only homing X/Y will home Z in a fictious state of "current+5". And if z is never homed again, it will never have the real z height. and it is marked as homed for all running macros. even if you just home X and Y and then start a print, Conditional G28 will not trigger, messing up the whole print. The only clean solution would be to home with zhop and then do a M84 Z like it is possible in Marlin but not in Klipper to "unhome" only Z.
that was the issue why it didn't get through the first time with my old PR. There is no clean solution unless you unhome only Z explicietly after a fictuous z home.
I suggest using something like this: (identation might not be 100% correct)
{% if ('z' not in printer.toolhead.homed_axes) %}{% if verbose %}{ action_respond_info("Z height unknown, performing Z-hop...") }{% endif %}SET_KINEMATIC_POSITION X=0 Y=0 Z=0G0 Z{homing_zhop} F{z_drop_speed}{% elif (printer.toolhead.position.z < homing_zhop) %}{% if verbose %}{ action_respond_info("Z too low, performing Z-hop...") }{% endif %}G91G0 Z{homing_zhop} F{z_drop_speed}G90{% endif %}
I like this approach and I'll probably update this PR to incorporate the change, but I think you introduced a bug in your change.
SET_KINEMATIC_POSITION X=0 Y=0 Z=0
I think this line here is responsible for the Z height issue you're having. You're literally telling the printer it's at position 0, 0, 0 wherever it happens to actually be when homing any axis. Then when you home XY only, Z thinks it's a zero and does a Z hop and thus it thinks it's at "current position + 5" on Z.
You wouldn't see it before the change because that was previously in the Z homing section and directly after it's called the printer homes every axis. I think it's in there to prevent any weird "move out of range" errors when homing all axes.
Just put that line back where it came from and it should be fixed.
if Z is homed and at -5 (which is position_min in Klippain), it would scratch at a very tiny edge case. @Frix-x what do you think? change position_min to -4 in stepper_z oder add a variable zhop to reach the height of 5 if it is below 0?
We could just make the Z hop taller by a millimeter. Honestly though, we can't save the user from everything. I think that scenario is very unlikely compared to the scraping scenario I described. I'f you're at -5 on Z, you probably already crashed the nozzle and should be moving the toolhead by hand.
@austinrdennis seriously... look who did it here in the beginning.
Good catch. 🙏 I still think the use of that is wrong here, but the fault is mine. Did the removal of that line end up fixing the weird z homing behavior?
Forgive me, I had your PR commit and an older commit form this PR open at the same time and must have gotten confused. These commits were originally from a time where I had much less experience with Klipper.
well if your goal is to do a zhop check only if z is homed, then the macro would be still at the beginning just
{% if ('z' in printer.toolhead.homed_axes) %}
{% if (printer.toolhead.position.z < homing_zhop) %}
{% if verbose %}
{ action_respond_info("Z too low, performing ZHOP to rehome Z") }
{% endif %}
G91
G0 Z{homing_zhop} F{z_drop_speed}
G90
{% else %}
{% if verbose %}
{ action_respond_info("Z already safe, no ZHOP needed to rehome Z") }
{% endif %}
{% endif %}
{% endif %}
but that wouldn't fix "only x/y homing" when the printer shut down on "tap"
but that wouldn't fix "only x/y homing" when the printer shut down on "tap"
Ok, so we move SET_KINIMATIC_POSTION back to where it came from and that fixes that weird z height behavior and add this to the beginning of homing macro. What do you think of this?
{% if ('z' not in printer.toolhead.homed_axes) %}
{% if verbose %}
{ action_respond_info("Z height unknown, performing Z-hop...") }
{% endif %}
G91
G0 Z{homing_zhop} F{z_drop_speed}
G90
{% endif %}
{% elif (printer.toolhead.position.z < homing_zhop) %}
{% if verbose %}
{ action_respond_info("Z too low, performing Z-hop...") }
{% endif %}
G91
G0 Z{homing_zhop} F{z_drop_speed}
G90
{% else %}
{% if verbose %}
{ action_respond_info("CAUTION: Z already at safe height, no Z-hop needed.") }
{% endif %}
{% endif %}
that doesnt work. if z is not homed, it doesn't do a zhop. It requires the kinematic setting.
that doesnt work. if z is not homed, it doesn't do a zhop. It requires the kinematic setting.
Damn, ok I get why we need it. I see what you were dealing with in the earlier PR. What about this?
{% if ('z' not in printer.toolhead.homed_axes) %}
{% if verbose %}
{ action_respond_info("Z height unknown, performing Z-hop...") }
{% endif %}
G91
SET_KINEMATIC_POSITION Z=0
G0 Z{homing_zhop} F{z_drop_speed}
G90
M84
{% endif %}
{% elif (printer.toolhead.position.z < homing_zhop) %}
{% if verbose %}
{ action_respond_info("Z too low, performing Z-hop...") }
{% endif %}
G91
G0 Z{homing_zhop} F{z_drop_speed}
G90
{% else %}
{% if verbose %}
{ action_respond_info("CAUTION: Z already at safe height, no Z-hop needed.") }
{% endif %}
{% endif %}
It does the safety hop and then turns off the motors before homing the requested axis. Not a perfect solution, but you get the safety benefit and the downsides would be mostly mitigated. I'd test it but I'm in the middle of a super long print. I should be able to test tomorrow.
i had the same thought, but then you would intentionally unhome other axis as well, which is not desired. you have homed X and then you home Y and then you have unhomed X and Z
i even looked in klipper source code in G28. They do a manual toolhead move which we can't use, since they did not implement axis movements, just stepper movements on as gcode command
i had the same thought, but then you would intentionally unhome other axis as well, which is not desired. you have homed X and then you home Y and then you have unhomed X and Z
Unfortunately, I think this the best option we got. At least we can add more safety to the homing process here. I think un-homing all the unspecified axes is a small price to pay and if you home x and y in the same command. Just do a full G28/CG28 right before you print and you'd be set which is how the start_print macro is written anyway.
I don't think Klipper has a way to do all the things we want here. I wish force_move could be done on an entire bank of steppers, but it is what it is.
I'll do some testing with it to look for edge cases today then I'll commit the changes into this PR.
Ok, just tested it on my machine and it is operating well for me.
@Surion79 If you home an axis previously, you'll only invalidate an axis if z-height is unknown. If Z height is known, it will check for z hop via the other branch and the other axis (X or Y) will not be invalidated. It would exhibit that behavior only if you home x or y individually with an unknown z-height.
I don't see a problem with this other than it may confuse people. The printer should refuse to move if they try to do something unsafe with invalid axes or simply rehome the needed axes in some cases on it's own. I'll add some yellow warning messages into the console for the edge cases so people know their previously homed axes are now invalid and need to be homed again. That way people are not caught off guard and know why it's doing what it it's doing.
I'm ok with this slight inconvenience in the name of added plate safety, but I guess that's ultimately up to @Frix-x.
Ok, that should do it. Have a look and let me know if you disagree.
I couldn't get the text to change color without re-doing all the action_respond_info snippets into respond format and creating an extension macro. I know how to do it, it is just beyond the scope of this PR. Maybe I'll submit another PR to redo all that throughout the project to give us more flexibility with message format down the road.
@austinrdennis Seems we get the solution soon in klipper: https://github.com/Klipper3d/klipper/pull/6262
@austinrdennis Seems we get the solution soon in klipper: Klipper3d/klipper#6262
It's indeed something that would fix this problem very easily! So I'll keep this PR open in the meantime and we will do it cleanly when the Klipper PR is merged :)