Skip to content
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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

cloakedcode
Copy link

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):

  1. Everything Mental did on the original module (with potentially minor tweaks from me)
  2. Everything I did in the rewrite, development followed the Discourse thread
  3. A refactor commit to clarify/simplify the attachment verification methods

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.

mental405 and others added 3 commits June 11, 2023 13:32
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]>
@cloakedcode
Copy link
Author

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.

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]>
@kdb424
Copy link

kdb424 commented Jun 16, 2023

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.

@kdb424
Copy link

kdb424 commented Jun 27, 2023

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

@BelgarionNL
Copy link

feel free to ask me questions. I can reproduce the error every single time. once I paste in the newer dockable_probe.py

@kdb424
Copy link

kdb424 commented Jun 28, 2023

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.

@cloakedcode
Copy link
Author

cloakedcode commented Jun 28, 2023

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.

@kdb424
Copy link

kdb424 commented Jun 28, 2023

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 CALIBRATE_Z with the module loaded. It will cause a panic shown in the previous log the moment the probe touches the bed.

@cloakedcode
Copy link
Author

While you're in there troubleshooting, can you try two things for me?

  1. Does the same issue occur when running PROBE_Z_ACCURACY?
  2. Try changing line 121 ish in dockable_probe.py to not use the event reactor time in querying the endstop but last movement time, like this:
curtime = self.printer.lookup_object('toolhead').get_last_move_time()

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.

@kdb424
Copy link

kdb424 commented Jun 29, 2023

1. Does the same issue occur when running `PROBE_Z_ACCURACY`?

No, it probes the nozzle as the nozzle endstop is the Z endstop, not the probe.

2. Try changing line 121 ish in `dockable_probe.py` to not use the event reactor time in querying the endstop but last movement time, like this:
curtime = self.printer.lookup_object('toolhead').get_last_move_time()

This had no effect on logs as best as I can tell. They look nearly identical.

As promised, full log here.
https://gist.github.com/kdb424/cd9bd3bb45a1e62aedb753cbaca32854

@cloakedcode
Copy link
Author

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).

@kdb424
Copy link

kdb424 commented Jul 1, 2023

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).
@cloakedcode
Copy link
Author

@kdb424 Can you put this code through its paces? Hopefully it works with and without auto Z calibration.

cloakedcode@19139f1

@churls5495
Copy link

@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.

@cloakedcode
Copy link
Author

cloakedcode commented Jul 7, 2023

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 (G28) must happen before attaching the probe (ATTACH_PROBE) or an error will be raised. If the correct options are specified, a typical usage would not require any probe-specific gcode commands to be run (e.g bed mesh: G28 followed immediately by BED_MESH_CALIBRATE) as the probe will be auto-attached if needed. In other words, if you're overriding built-in Klipper gcodes to wrap them in "attach probe" and "detach probe", that's unnecessary with this module.

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:

1/5 > 2a
^     |
4a  < 3
      ^ dock sits here

detach:

1
|
2d  >  3  >  4d
       ^ dock sits here
  • 1 and 5 are your approach_position
  • 3 is your dock_position
  • 4d is your detach_position
  • 4a and 2d are identical
  • 4a/2d and 2a don't have config options but are instead specified by overriding the gcode movements

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:

[gcode_macro MOVE_TO_APPROACH_PROBE]
rename_existing: MOVE_TO_APPROACH_PROBE_BASE
gcode:
    MOVE_TO_APPROACH_PROBE_BASE
    G1 <2a>

[gcode_macro MOVE_TO_EXTRACT_PROBE]
gcode:
    G1 <4a>
    MOVE_TO_APPROACH_PROBE_BASE

[gcode_macro MOVE_TO_INSERT_PROBE]
gcode:
    MOVE_TO_APPROACH_PROBE_BASE
    G1 <2d>

@churls5495
Copy link

If Z is required for the dock, Z homing (G28) must happen before attaching the probe (ATTACH_PROBE) or an error will be raised.

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.

@cloakedcode
Copy link
Author

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 homing_override config section/module can be used.

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.

@cloakedcode
Copy link
Author

@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.

@kdb424
Copy link

kdb424 commented Jul 12, 2023

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.

@github-actions
Copy link

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.

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.
@Hobbss27
Copy link

@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.

im running both currently if you need imput. im using the base klicky config with autoz from protoloft.

@cloakedcode
Copy link
Author

@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!

@ammmze
Copy link

ammmze commented Apr 13, 2024

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.

Traceback (most recent call last):
  {omitted for brevity}
  File "/home/pi/klipper/klippy/extras/bed_mesh.py", line 747, in cmd_BED_MESH_CALIBRATE
    self.probe_helper.start_probe(gcmd)
  File "/home/pi/klipper/klippy/extras/probe.py", line 444, in start_probe
    pos = probe.run_probe(gcmd)
  File "/home/pi/klipper/klippy/extras/probe.py", line 174, in run_probe
    pos = self._probe(speed)
  File "/home/pi/klipper/klippy/extras/probe.py", line 123, in _probe
    epos = self.mcu_probe.probing_move(pos, speed)
AttributeError: 'DockableProbe' object has no attribute 'probing_move'

I also have a suggestion...

I was working through getting this integrated into my print start macro working from the example:

SET_DOCKABLE_PROBE AUTO_ATTACH_DETACH=0
G28
ATTACH_PROBE                             # Explicitly attach the probe
QUAD_GANTRY_LEVEL                        # Tram the gantry parallel to the bed
BED_MESH_CALIBRATE                       # Create a bed mesh
DETACH_PROBE                             # Manually detach the probe
SET_DOCKABLE_PROBE AUTO_ATTACH_DETACH=1  # Make sure the probe is attached in future

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 AUTO_ATTACH_DETACH. For example:

SET_DOCKABLE_PROBE AUTO_DETACH=0         # Disable auto detach, so it remains attached after homing
G28                                      # Home all axis (this will also attach the probe, but won't detach)
QUAD_GANTRY_LEVEL                        # Tram the gantry parallel to the bed
BED_MESH_CALIBRATE                       # Create a bed mesh
DETACH_PROBE                             # Manually detach the probe
SET_DOCKABLE_PROBE AUTO_ATTACH_DETACH=1  # Make sure the probe is attached in future

self.toolhead.manual_move([None, None, return_pos[2]+2],
self.travel_speed)
self.auto_detach_probe(return_pos)

Copy link

@dim1triy dim1triy Apr 13, 2024

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)

@cloakedcode
Copy link
Author

@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).

@ammmze
Copy link

ammmze commented Apr 15, 2024

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.

👍🏻 Very good point and makes perfect sense.

@ProfDrYoMan
Copy link

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. :/

ProfDrYoMan added a commit to ProfDrYoMan/klipper that referenced this pull request Apr 15, 2024
This method was ripped out of the base class and copy-pasted
to each subclass.
@cloakedcode
Copy link
Author

@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.

@mental405
Copy link
Collaborator

Lemme get out the WD40.

@ProfDrYoMan
Copy link

I noticed a small issue.
Using manual z-probe calibration and then in the end accepting the measured height leads into an error since there is no [probe] section anymore.
Also manually tuning z-offset and the saving throws the same type of error.

@cloakedcode
Copy link
Author

@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".

@ProfDrYoMan
Copy link

Oh. I take the AR to check again. It’s also fluidd „latest“.

@ProfDrYoMan
Copy link

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! :)

@KevinOConnor
Copy link
Collaborator

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.
-Kevin

@cloakedcode
Copy link
Author

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

it is mostly attempting to unify an existing capability in a way that does not require as many complex macros

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'm concerned the more we add "macro-able" functionality to core Klipper the larger the ongoing load will be on reviewers

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

core functionality of this new module can be accomplished by macros today.

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

Even if we did merge this, many users would continue using their existing macros.

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.

I would prefer to hold off on merging functionality like this for several more months.

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.

I do agree we ultimately want to improve support for dockable probes.

What are your thoughts on how support would be improved if not this module?

@novaplusplus
Copy link

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.

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.

@ammmze
Copy link

ammmze commented Apr 25, 2024

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:

[plugin dockable_probe]
source: https://github.com/cloakedcode/klipper
# optionally configure a branch other than the default branch
branch: work-annex-probe
 # optionally specify an order to control the order plugins are layered in
order: 0

And perhaps we could expect a plugin repo to contain a manifest file called klipper-plugin.yaml that looks something like the following. And klipper upon startup ensure those are cloned and at the requested branch and then setup the symlinks listed in the manifest (or perhaps even change how files are loaded to look in the files listed in the manifest, allowing the klipper working dir to remain clean).

name: dockable_probe
files:
 - src: klippy/extras/dockable_probe.py
   dest: klippy/extras/dockable_probe.py # perhaps default dest == src?

@kdb424
Copy link

kdb424 commented May 19, 2024

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.

@novaplusplus
Copy link

novaplusplus commented May 20, 2024

Macros are absolutely horrid to use, and feel like hacks.

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.

@BillyNate
Copy link

My PR from September replaces the approach_position/detach_position/dock_position and gcode usage for two concise route settings. How about that?

@ProfDrYoMan
Copy link

Hi @cloakedcode,

latest klipper generates

Unhandled exception during connect
Traceback (most recent call last):
  File "/home/mks/klipper/klippy/klippy.py", line 175, in _connect
    self._read_config()
  File "/home/mks/klipper/klippy/klippy.py", line 141, in _read_config
    self.load_object(config, section_config.get_name(), None)
  File "/home/mks/klipper/klippy/klippy.py", line 130, in load_object
    self.objects[section] = init_func(config.getsection(section))
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/mks/klipper/klippy/extras/dockable_probe.py", line 637, in load_config
    config.get_printer().add_object('probe', probe.PrinterProbe(config, msp))
                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: PrinterProbe.__init__() takes 2 positional arguments but 3 were given

with this PR.

@ProfDrYoMan
Copy link

ProfDrYoMan commented Jun 22, 2024

Last working Klipper is g6cd17420, if this helps to nail down the problem. Not in my capability range to fix this.

@vospascal
Copy link

d4bae4d you can see here the sigiture fo PrinterProbe has changed..

@vospascal
Copy link

Last working Klipper is g6cd17420, if this helps to nail down the problem. Not in my capability range to fix this.

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).
@cloakedcode
Copy link
Author

@ProfDrYoMan / @vospascal I've pushed an update. Let me know how it goes.

@ProfDrYoMan
Copy link

Thanks @cloakedcode. Printer is in maintenance mode. Mode take a while. I hope somebody else is reporting positive feedback.

@53Aries
Copy link

53Aries commented Jul 6, 2024

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.

@marbocub
Copy link
Contributor

marbocub commented Aug 1, 2024

I have used this module for more than half a year on a CoreXY and a Cartesian. I report the results as they work.

  • CoreXY: stable, works excellent, very easy to setup since in case of using a standard fixed XY position dock.
  • Cartesian: stable, works great, but it took a bit time to setup since I used an fixed XYZ position dock, a hardware Z limit switch for approaching the dock, [homing_override] in config and override five MOVE_TO_*_PROBE macros. on the other hand, the names of macros that should be overridden are good and easy to understand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.