search

Klipper3d / klipper

Klipper is a 3d-printer firmware
GNU General Public License v3.0
9.58k stars 5.34k forks source link

Add support for named/multiple probes and update bed_mesh to support … #6714

Open cekim-git opened 1 month ago

cekim-git commented 1 month ago

Add support for named/multiple probes via [probe NAME] directive.

JamesH1978 commented 1 month ago

Thank you for submitting a PR, please be aware that you need to sign off to accept the developer certificate of origin, please see point 3 in https://github.com/Klipper3d/klipper/blob/master/docs/CONTRIBUTING.md#what-to-expect-in-a-review

Thanks James

cekim-git commented 1 month ago

My apologies... I have just amended the commit. Hopefully, that satisfies this requirement? If not, let me know. Should I edit the OP here to match?

ItsReckliss commented 1 month ago

Does this allow an eddy sensor and a bltouch to run at the same time?

cekim-git commented 1 month ago

Does this allow an eddy sensor and a bltouch to run at the same time?

Long answer:

In my case I am running both a TAP sensor (aka: 'probe') and a superPINDA (aka: 'pinda0') attached to Toolhead0 (the first of 6 heads attached via CAN/EBB36 pins PB9 and PB6 respectively). quad_gantry_level worked without edits by just adding a "probe: pinda0" to its configuration in printer.cfg. I can freely switch back and forth between them. BedMesh required the attached edits to function the same (switch freely between the 2 probes). The edits to bed_mesh.py were to replace the lookup for the "probe" object from using the string 'probe' and instead use a configuration value (aka: self.probe_name) retrieved from the printer.cfg values. I have also modified my macros to allow me to freely switch between TAP and PINDA as the primary Z-home values in [homing_override]. I simultaneously rely on the TAP sensor for detecting that the head has moved in Z relative to the gantry (an indication of multi-headed printer malfunction or tool-change) while relying on the PINDA for homing, QGL and meshing...

Without having tested bltouch/eddy I can't say for sure what they would require. Looking at the code for eddy - there is a reliance on the default "probe" object: self.cmd_helper = probe.ProbeCommandHelper( config, self, self.mcu_probe.query_endstop) self.probe_offsets = probe.ProbeOffsetsHelper(config) self.probe_session = probe.ProbeSessionHelper(config, self.mcu_probe)

I would expect there may need to be re-factoring here to request the new named probe rather than rely on the default probe object. Doing so would potentially allow decoupling as I have done with TAP/PINDA case and thus concurrent operation.

But, I would view this extension/PR as a first step in adding extensibility to the probe definition which then requires other objects wishing to de-couple from the hard-coded default "probe" object to make relatively simple edits to do so.

Short Answer: Potentially... Facilitating this is the precise intention of this modification, but as of yet changes and testing have not taken place with that combination. May require some edits to eddy/bltouch to remove reliance on default "probe" object often referenced directly rather than fetched with "lookup_object()"

ItsReckliss commented 1 month ago

I appreciate the effort you've put into this PR. I've never edited Klipper directly myself but I'm sure you have a decent understanding given you made this.

I was wondering about the structure for declaring multiple probes. Would it be feasible to add another line to the probe declaration, like this?

[probe <probe_type (e.g., "eddy_current", "microswitch", "bltouch")> <probe_name>]

This approach could allow users to:

  1. Declare the probe type: This way, you could include specific configuration options based on the type (e.g., Eddy needing a sensor type, BLTouch needing a control pin).
  2. Assign a unique name to each probe: This would help users manage multiple probes more clearly, making it easier to call a specific probe for different operations.

I faced an issue with declaring multiple probes and trying to use z_virtual_endstop for Z-homing when there were multiple “probe” boards involved. I realized that it might just require changing the board assignment to match the specific probe name. Still, I think the added declaration line could make configuration more intuitive for users, if possible.

cekim-git commented 1 month ago

Would it be feasible to add another line to the probe declaration, like this? [probe <probe_type (e.g., "eddy_current", "microswitch", "bltouch")> <probe_name>]

Given the way klipper works, this might not be necessary to accomplish your goal with named probes added to the base probe class. As for feasibility, I just don't know if provision for more than a "prefix" (second argument) exists... (you now have 3 arguments). However, a configuration might have a "type:" value for conditional functionality like this for example, but also.... there is the inherent function of class definition here.

In klipper there's a base presumption that [] will, if not already defined, try to open a file in extras/ named .py and find it dynamically. Such a file presumably contained a python object (loaded via the "load_config()" declaration in that file). That file may then either create a entirely new "class" or it may inherit an existing class (as is the case with eddy/bltouch inheriting "probe" - aka "PrinterProbe" class in python).

Further [ prefix] can be supported by the new class. So, Something like Eddy or Bltouch could themselves support this prefix and multiple named instances if they so choose having inherited the enhanced base probe class.

This approach could allow users to:

  1. Declare the probe type: This way, you could include specific configuration options based on the type (e.g., Eddy needing a sensor type, BLTouch needing a control pin).

See above, the nature of [class_name prefix} loading .py provides for this already, I think? Either via the derived class searching for differing configuration arguments period or relying on a "type" configuration value to determine which arguments are valid.

  1. Assign a unique name to each probe: This would help users manage multiple probes more clearly, making it easier to call a specific probe for different operations.

This is the exact intention of the base class (probe.py|PrinterProbe) supporting named probes. From this point forward, new classes and configurations can now reach these multiple probe objects by name. Either in the .cfg files or in the python. Whether a named probe is a child/derived class of probe.py (bltouch/eddy for example) or a simple pin/stop it would be reachable via name from this point forward.

In short, it is unclear to me if the klipper infrastructure supports arguments beyond 2... I just don't know, but it may not be required to resolve the issue you are describing IF named probes are supported. Both the differing configuration items (supported already by derived class objects corresponding to the first value in [class prefix]) and the ability to reference by name to "call a specific probe for different options" are addressed here, I think.

github-actions[bot] commented 2 weeks ago

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.

garethky commented 6 days ago

This should also come with some documentation updates:

cekim-git commented 4 days ago

This should also come with some documentation updates:

  • Add the new, optional, PROBE= parameter to the various GCode commands in GCode.md
  • Update to Configuration Reference to note that multiple [probe xxx] sections can be defined
  • Update to Status Reference to explain how data from various probes can be retrieved There are patterns in the existing docs for how to make these updates. Things like fans and extruders already do this.

This seems infinitely reasonable... I'll see what I can do while removing the duplicate macro-names above and update the PR

Arksine commented 4 days ago

FWIW, display.py demonstrates a pattern for registering "muxed" gcode commands that include a default when its possible to configure an extra with a standard section ([display]) and prefix sections([display my_lcd]. Below are the relevant lines:

https://github.com/Klipper3d/klipper/blob/f2e69a3703f97725fb0db626ff592a223e22b3a2/klippy/extras/display/display.py#L208-L213

What happens here is that if the user runs SET_DISPLAY_GROUP GROUP=mygroup it will execute the command for the default [display], however if the user runs SET_DISPLAY_GROUP DISPLAY=my_lcd GROUP=mygroup, it will execute the command for the [display my_lcd] object.

cekim-git commented 4 days ago

FWIW, display.py demonstrates a pattern for registering "muxed" gcode commands that include a default when its possible to configure an extra with a standard section ([display]) and prefix sections([display my_lcd]. Below are the relevant lines:

https://github.com/Klipper3d/klipper/blob/f2e69a3703f97725fb0db626ff592a223e22b3a2/klippy/extras/display/display.py#L208-L213

What happens here is that if the user runs SET_DISPLAY_GROUP GROUP=mygroup it will execute the command for the default [display], however if the user runs SET_DISPLAY_GROUP DISPLAY=my_lcd GROUP=mygroup, it will execute the command for the [display my_lcd] object.

Thank you, this was what I was hoping to find... some more common way to handle this issue... I figured it must exist.... this makes sense, I will employ this method.