-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dockable_probe module #6247
base: master
Are you sure you want to change the base?
dockable_probe module #6247
Conversation
annexed_probe: added dock_retries configuration option annexed_probe: renamed decouple_speed to detach_speed for consistency Annexed_Probe.md: initial commit of documentation Config_Reference.md: added annexed_probe config section annexed_probe: retry attempts must be greater than 1 annexed_probe: redefine default speeds annexed_probe: changed delayed detach to optional Annexed_Probe.md: added section for delayed_detach option Annexed_Probe.md: (fixed invalid characters and whitespace errors Annexed_Probe.md: Added additional documentation and gcode reference Config_Reference.md: Removed extra spaces for annexed_probe and added allow_delayed_detach parameter G-Codes.md: added annexed_probe gcode reference annexed_probe.py: changed initial position execution to after docking completed annexed_probe: rename to dockable_probe dockable_probe: remove ability to set manual_probe_state dockable_probe: change docking points to discrete coordinates instead of vectors Annexed_Probe.md: fixed typo in tool velocities dockable_probe: corrected typos dockable_probe.py - Corrected typo in _do_detach() dockable_probe: Python3 compatibility and misc fixes in parseCoord - map() returns a iterable which is not sliceable. - Fix a bug where the coordinate vector could be resized. - Better error reporting. Signed-off-by: Maël Kerbiriou <[email protected]>
* A number of code changes were requested by PR reviewers (PR Klipper3d#4328), those have been applied. All of the changes applied were cosmetic, mostly adding early returns. * The last remnants of the original module name 'annexed_probe' have been replaced by 'dockable_probe', including doc references. dockable_probe: Edit docs for brevity and consistency Most importantly I tried to clarify the dock_fixed_z functionality. There should be no changes to the meaning or understanding in these docs but hopefully they're easier to ingest. Strip out dock_fixed_z, infer it, and add explicit z_hop This greatly simplifies the logic and reduces config/docs complexity. Add auto-attach-detach feature with gcode to set option at runtime While trying to debug why the probe couldn't be used as a virtual endstop, I simplified the code further to more closely resemble that of bltouch.py. This makes it easier to reason about and less brittle as there are fewer unique processes to accommodate this type of probe. As a result of less bespoke handling and only hooking into the default probe functionality to inject attaching/detaching, it's possible now to use the standard safe_z_home config section. The Euclid probe requires opposite attach/detach routines. See docs/Dockable_Probe.md for more info. Signed-off-by: Alan Smith <[email protected]>
'check_open_attach' was ignored as there was no way to invert the 'open'/'triggered' check in spite of 'False' being a valid value for the config option. Removing the dummy sensors for the dock sense pin made the logic easier to follow/read at the expense of validating the sensor exists before using it. Signed-off-by: Alan Smith <[email protected]>
To make changes as a result of this PR process visible (e.g. fixing whitespace), I won't rewrite the git commits unless requested. If I should be doing something different, please let me know. |
Signed-off-by: Alan Smith <[email protected]>
Specifying a default obscures the fact that a required coordinate option may be missing from the config. The result, when an option is missing, is a cryptic error message (an unhandled exception). All coordinate options are required, there is no need to specify a default and handle the missing data elsewhere. Signed-off-by: Alan Smith <[email protected]>
1b10069
to
82e3db7
Compare
I've been running this for 4 days, and I've found it much easier to configure, more concise, better to control, and more reliable than the previous offering. I will continue to test any changes added and report back if I have any issues, but this is a wonderful addition for my printer. EDIT: Now running this on all of my printers, notably a Tri-Zero and Doom Micron. Running flawlessly. |
There is some error in this module that needs updated. Auto Z works with Mental's module, but produces an error with this one. protoloft/klipper_z_calibration#105 |
feel free to ask me questions. I can reproduce the error every single time. once I paste in the newer dockable_probe.py |
Captured a partial log to help make this easier to debug https://pastebin.com/TPwAd247. I haven't pinpointed what the change was in this module as opposed to Mental's, but that module works correctly, this module causes a firmware crash from that, which points at the error being here. |
I'm afraid the cause of this bug isn't obvious and I'm away from home and printers for a week and can't debug. I suspect it's something to do with the probe state query/changes occurring but that shouldn't interact with the stepper commands. Does the issue appear with both modules loaded and running a normal 'PROBE' command? What about more complex probing like 'BED_MESH_CALIBRATE'? I'm trying to determine if it's only triggered by the exact series of commands in the Z calibration gcode or if having both modules loaded can trigger the bug. I need more logs. 😋 Please include the full log and complete repro steps, I'll be back in front of a printer eventually to repro locally. |
I'll get you a full log tonight. I can run bed mesh, QGL, Z tilt, ect all fine. It triggers when you touch the probe to the bed after measuring the nozzle and the probe body. I'm not getting any errors outside of that exact situation. Repro steps is simply to run a |
While you're in there troubleshooting, can you try two things for me?
Restart the Klipper service and try the Z calibration again. I guess that would create two log files and both might be helpful in shedding light on where the bug is hiding. Let me know how that goes! Thanks for your help. |
No, it probes the nozzle as the nozzle endstop is the Z endstop, not the probe.
This had no effect on logs as best as I can tell. They look nearly identical. As promised, full log here. |
I'm not sure but this might be caused by dockable_probe.py lines 587-593. Try deleting them entirely and test whether the probe behaves normally (regular probe movement and bed mesh). If all is well try Z calibration. My suspicion is there's an ordering conflict with the endstop triggering and the attempt to query the endstop state which creates this opaque "invalid sequence" error (that doesn't point at what's wrong with the sequence). |
That won't even let it start probing. No Z movement before panic. No usable log here because that isn't relevant to auto-z as even trying to QGL or anything probe related dies. |
The endstop triggers are waited on during a homing move before the probe is marked as finished (see homing.py:101).
@kdb424 Can you put this code through its paces? Hopefully it works with and without auto Z calibration. |
@cloakedcode, could you elaborate on how this works with a bed slinger? I'm currently using a custom module that requires an endstop to first locate the dock before attaching probe. I'm not seeing how your module homes Z before picking up the probe. More info here. |
tl;dr - You have there a setup that is just different enough that you'll need to do a little extra configuration to make it work but that's by design. (A note about probe support: Not all probe setups are meant to be fully supported out of the box, there are simply too many variations and new ones come along regularly. If this gets merged, I foresee probe designers documenting the necessary Klipper config to make their designs work, similar to below, rather than custom Klipper modules/extensions.) That looks to be a QuickDraw probe/dock mounted to an Ender 3. Klicky and QuickDraw probes operate with the same attaching/detaching procedures. For attach: move near the dock, move to the probe, move away from the dock with the probe. For detach: move near the dock, move to the dock, move away from the dock leaving the probe behind. The difference with your setup vs a CoreXY machine is that yours has the dock mounted at a fixed Z height (i.e. Z = 0). That's the first position example in the docs. Also read the explanation of the position config options. If Z is required for the dock, Z homing ( The subtlety with your setup is the dock is mounted backwards compared to a Klicky dock where the probe is pulled out of the dock towards the bed. This complicates things slightly but easily doable as the movement commands are meant to be overridden in this scenario (similarly for Euclid). The attach movements for your probe can be, looking at the printer from the front:
detach:
This is similar to a Euclid probe as seen in the last positioning example. All that's needed now is to implement the additional movements:
|
This is the part I'm trying to understand. How is Z homed before the probe is attached? I assume it's coded in your module; I'd just like to understand the details. |
Z homing is not handled by this module. The standard mechanisms in Klipper for homing should be used. For example, if the printer setup requires the XYZ axises be homed in a specific order, the IIUC for your setup the Z endstop is configured as a standard Z endstop (i.e. defined in a stepper_z section). This is compatible with the module but homing is not implemented in this module. |
@kdb424 / @BelgarionNL Have you tried the special branch/fix linked above with the auto Z calibration? I'm afraid I cannot test it as I don't have a printer with a nozzle probe. AFAIK this is the last outstanding issue with the module. |
I no longer have a printer with both a nozzle Z and a dockable probe, so I won't be of much use on that front unfortunately. Sorry that died before I helped you get it sorted. |
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:
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, PS: I'm just an automated script, not a human being. |
As discussed on the Klipper discourse[1], it's ideal for the probe to be attached before moving to the first probing point (e.g. during a QUAD_GANTRY_LEVEL) to avoid large movements to and from the dock.
im running both currently if you need imput. im using the base klicky config with autoz from protoloft. |
@Hobbss27 Yes please! Please run this special branch code everywhere you can (i.e. anywhere you've got a klicky/dockable probe). It should work with and without protoloft's auto Z module. Thank you for volunteering! |
Huge thanks to @cloakedcode for putting this together. I really hope we can finally get this thing merged soon. I'm only just starting to test it and hit a snag...it does look like there was a change in the last few days that breaks it though.
I also have a suggestion... I was working through getting this integrated into my print start macro working from the example:
However, because I am using a virtual z endstop (i.e. the probe), that G28 can't home all axis. So instead I have to home just x and y, and then since i can't home z, I am having to manually move to my "safe_z_home" to ensure I start in a safe position before picking up the probe. In addition, I couldn't figure out how to get the safe z home configuration to re-use it, so instead i've just copied it. But if I ever have to change it, I have to remember to update both places now. So what I would suggest is being able to turn off auto detach and auto attach separately, while still allowing setting both with
|
self.toolhead.manual_move([None, None, return_pos[2]+2], | ||
self.travel_speed) | ||
self.auto_detach_probe(return_pos) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add for fix problem after last update acdf8bb
def probing_move(self, pos, speed):
phoming = self.printer.lookup_object('homing')
return phoming.probing_move(self, pos, speed)
@ammmze thanks for the kinds words. I will address that breaking update when I've got some free time. Frustrating that it comes now while this PR has been open for almost a year. If you read through the PR comments and the Discourse thread you'll see there are many ideas for tweaks to the process, each to alleviate tiny pain points (relative to this remaining not in mainline Klipper). I've resisted changing the code in hopes that its stability and simplicity would win it faster review and adoption into the code base. The jury's still out on if that plan is working. For your specific issue, rather than changing the module to add more knobs and twiddles (that a hapless user could accidentally damage their machine with), what do you think about forgoing the extra time savings of that first detach/attach and instead putting the G28 call before disabling the auto-attach-detach? Alternatively, "homing_override" will let you control the order of the homing axes and allow for inserting an explicit "ATTACH_PROBE" before homing Z (the explicit call doesn't respect the "auto" setting). |
👍🏻 Very good point and makes perfect sense. |
Hi @cloakedcode, thanks for your effort. Using it since a while now. Excellent. Also flexible enough, I think. Is there any way to get movement into the PR merge. I was about to send a note to Kevin, but then realized that pushing will not help and did not. Other stuff like the new Eddy probes get in fast. :/ |
This method was ripped out of the base class and copy-pasted to each subclass.
@ammmze / @dim1triy / @ProfDrYoMan Added a patch to address that recent Klipper change. @ProfDrYoMan I don't know what the delay is here. The code has been stable for months and used widely. If anyone out there has an in with Kevin or another maintainer, please let me know! Or point them to this PR. AFAIK the only hold up is having a maintainer to approve and merge. |
Lemme get out the WD40. |
I noticed a small issue. |
@ProfDrYoMan What web UI are you using? I just double-checked on Fluidd v1.28.1 and pressing the save button after adjusting the Z offset gives me a menu and picking "Z_OFFSET_APPLY_PROBE" puts the new offset under a "dockable_probe" section. All that to say, "works for me". |
Oh. I take the AR to check again. It’s also fluidd „latest“. |
My problem. I did put the [dockable_probe] section completely into an include config file. I do not like to mess with the original printer.cfg to much. This prevented saving. Having at least the section header and the z_offet config in printer.cfg solved the isssue. => All good. Please merge! :) |
Thanks for working on this. I see how this functionality can be useful, but I would prefer to hold off on merging functionality like this for several more months. I understand the functionality of having a module like this - it allows users a more standardized way to handle dockable probes and it generally improves the error handling for them. Right now, users are accomplishing the same end-goal using a variety of macros (often customized) and those macros can be complex and error prone. However, the core functionality of this new module can be accomplished by macros today. Even if we did merge this, many users would continue using their existing macros. To wit, the module isn't adding new fundamental capabilities, it is mostly attempting to unify an existing capability in a way that does not require as many complex macros. My main concern is that I'd prefer to try to tackle the generic problem of "gcode macro scalability" before merging individual gcode macro scalability improvements. I've written a little bit about this at https://klipper.discourse.group/t/possible-klipper-plugins-instead-of-macros/9819 . In particular, I'm concerned the more we add "macro-able" functionality to core Klipper the larger the ongoing load will be on reviewers and I fear that load is already too high today. Thanks again for working on this. I do agree we ultimately want to improve support for dockable probes. |
Hi @KevinOConnor, I can see the burden on reviewers is high here and that there's more to be reviewed than you have resourcing for, I feel your pain. I appreciate you taking the time to look into this and write out a thoughtful response. Community
I fully agree, that is the goal and mostly all this module accomplishes. However, that unification is very important, I think, for the Klipper community. Since starting on this module, I've noticed it comes up on Discord or other and is followed by "we'll never have that in Klipper" or some other discouraged and negative view on Klipper's evolution. This makes me sad. The other thing I've seen is more forking or wrapping of Klipper such that there's more and more divergence from running original/unmodified Klipper. Instead the community is putting effort into not contributing to Klipper. I think this is very bad for Klipper and the community and steps should be taken to improve morale and increase trust. Trust that Klipper is getting the features the users want and that there's no need to circumvent your maintenance. I get that your time is limited and that you can't make everyone happy but this feels important for the Klipper project to address head-on.
I get where you're coming from on principle, and I agree, why add a feature that can be coded by users to meet their needs. However, in this particular case, I think the ongoing development, debug, and maintenance time of the macros by their maintainers, plus the support time on Discourse and Discord by community members, should also be taken into account. I suspect that cost, just in person-hours -- never mind that that effort/expertise isn't contributing to Klipper itself -- weighs up well against the load on reviewers (I suspect much higher). Code
Yes but I don't think you ever intended macros to be the state machine that these macros require (e.g. Klicky macros). They modify 6+ core g-codes and push macros to their limits. And I get that's where the plugins discussion was supposed to go, but that seems to have fizzled. This module doesn't modify g-codes, it hooks directly into the probing actions. While macros can accomplish something similar, doesn't the complexity and "that's not what that's for" that these macros have become, warrant a feature in Klipper? Future
That's entirely possible, and I'm happy to take the feedback that this needs to have a greater impact on users, but I've already seen many people use this code on new builds or switching from a macro system. I will happily investigate if people would convert to this feature if it were merged, if that will have any sway on your decision.
What can I do? The plugin discussion stalled last year (only very recently having gotten a couple more posts) and I don't see any hints of a consensus or the beginnings of an implementation. What will be coming in several months? People have been using and recommending this code for nearly a year, it sounds cruel to delay it further unless the future holds a known solution.
What are your thoughts on how support would be improved if not this module? |
To be fully blunt, that's how I've felt for a while. Convenience/simplification/ease of use features in general just feel like such a low priority and I've seen a lot people elsewhere (whether or not involved with development) just immediately crapping on any suggestion to improve those areas. Macros for this sort of thing, or anything complex, is such a hacky nightmare and feels far more potentially failure prone than having a proper module done. Went through this myself with the orbiter filament sensor, had its internal state machine stuff fully lock up my printer more than once until I just gave up. Things like KAMP could be made into features of the existing modules among many other examples, overriding built-in commands with macros just makes my skin crawl. "You don't need a new feature added, just use macros" feels like "You don't need a bicycle or a car, you can just walk". I know I'm just a random shmuck who hasn't directly contributed to the project so I don't exactly have much of a leg to stand on but I just really dont agree with that way of thinking about it at all. |
While I would really like to see this integrated into klipper, I suppose a middle ground could be making it easier to extend klipper functionality. Something similar to HACS if you are familiar with home assistant or external components in esphome. It provides a way to easily install external components, so that maintenance of those can be done separately by a subset of the overall community. I would love to have a way to do this declaratively instead of having to install from the shell. So instead of having to manually clone a 3rd party repo, run the installer which then creates symlinks it would cool if we could declaratively do it:
And perhaps we could expect a plugin repo to contain a manifest file called
|
I was one of the first to test this, and I'm still using it. While I would hate for this to not upstream, I will continue to track the progress and continue to run this. Macros are absolutely horrid to use, and feel like hacks. It's an absolute shame to not have this in klipper proper. |
Something I really would like to see from all this is a more user friendly and smooth macro/scripting system of some kind if new "macro-able" modules aren't on the table. I avoid using remotely complicated macros if I can at all help it because it always feels like such a kludge to do so, and chasing bugs in macros sucks big time... A big yes from me for being able to more easily integrate third-party modules. I currently just... don't update klipper at all because of dockable_probe.py sitting in the klippy directory. I know it's not the end of the world if it just stays at one version, but if I ever do want to update it, there's a whole song and dance I have to go through to do so. |
My PR from September replaces the |
Hi @cloakedcode, latest klipper generates
with this PR. |
Last working Klipper is g6cd17420, if this helps to nail down the problem. Not in my capability range to fix this. |
d4bae4d you can see here the sigiture fo PrinterProbe has changed.. |
i think its 6cd1742 not g6cd17420 ;) |
In commit d4bae4d, as part of a larger group of changes[1], significant pieces in the way probes are implemented and used has been reworked. These changes accommodate that rework but are backwards incompatible with earlier versions. I have tested this on a single machine (auto-Z, non-virtual-endstop), please test your printer with these changes before proceeding as normal (if you break your printer, that's not on me).
@ProfDrYoMan / @vospascal I've pushed an update. Let me know how it goes. |
Thanks @cloakedcode. Printer is in maintenance mode. Mode take a while. I hope somebody else is reporting positive feedback. |
I just want to add in, I've been using various Klicky probes on various printers for years now. It's ALWAYS such a huge headache to get it setup and working right and usually involves reprinting parts that get broken due to Klicky macros causing collisions. I had been using dockable_probe.py from Danger Klipper, pasted into my mainline Klipper extras for awhile but after updating Klipper tonight the dockable_probe.py module broke. So back to ANNOYING and cumbersome Klicky macros. I'm going to try this branch out but I really really wish it was just integral to Klipper and I didn't have to mess around so much. Even if I could drop a single module into mainline, like I was doing with dockable_probe, I would be happy. |
I have used this module for more than half a year on a CoreXY and a Cartesian. I report the results as they work.
|
This is a redesign of the work done in #4328 to create a
[dockable_probe]
module to support Klicky-style probes that are physically detached most of the time from the toolhead.How we got here
The original PR has been sitting for a long time without attention and I took it upon myself to get it integrated into the main branch. Along the way I ended up simplifying the code and distilling it down to what is basically a handful of glorified movement commands (see the
MOVE_TO_*_PROBE
g-codes).Prior art
As mentioned, this is based on Mental's work but is significantly different. Rather than try to accommodate many different types of probe hardware and configurations, I targeted Klicky-style probes with fixed X/Y and fixed Z configurations. Other hardware and setups can override the movement g-codes where necessary to support their probes. See the
docs/Dockable_Probe.md
docs for setup examples.Testing
I've been running this code for about half a year on a bed slinger without issues (probe dock mounted Z 0). I put the code on a CoreXY today and successfully got it running there as well (probe dock mounted at fixed X, Y, used as virtual endstop for Z).
Back in Fall '22, I posted on the Klipper Discourse about this code and got help with wider tests and ironed out a couple bugs. There's been little chatter there until recently (a user with a very unique printer design) so I'm thinking it's been in the wild long enough to prove stable.
Commits
I broke this down into three commits (rebased and squashed commits):
Let me know if this should be broken down in other ways.
Finally
Thanks for your time and consideration! I know myself and others will be happy to have this feature native in Klipper.