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

[cekim-git]

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

• backwards compatible with default [probe]
• update bed_mesh.py to support named probes via probe: configuration directive
• quad_gantry_level already supports passing this configuration item
• Add PROBE= argument to macros (PROBE_ACCURACY PROBE=other_probe)
• Internally lookup_object(self.probe_name) can be used to get the probe

[JamesH1978]

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]

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]

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

[cekim-git]

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]

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]

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 [<class_name>] will, if not already defined, try to open a file in extras/ named <class_name>.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 [<class_name> 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 <class_name>.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.

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.

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]

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.

[Wulfsta]

This is a bit of an odd pattern to use - it would make much more sense to specify which probe to use as an input into the command. For instance, QUERY_PROBE would become QUERY_PROBE PROBE=<name>, with a default name for the backwards compatibility case.

[cekim-git]

The functionality you describe is already there...

Any of the existing commands can be run as you describe in the existing edited version of probe.py:
PROBE PROBE=pinda0
QUERY_PROBE PROBE=pinda0
PROBE_ACCURACY PROBE=pinda0
and so on...

If no "PROBE=" is provided it defaults to "probe" by default.

This is accomplished _get_helper - which looks at the self.probe_name by default, but checks the gcmd for a PROBE= directive...

The added functionality you are highlighting is that of re-naming commands which primarily serves to avoid causing an error by registering duplicate macros.

These renamed macros end up being duplicate functions. The following will perform the same action as the code stands today:
PROBE PROBE=pinda0
OR
PROBE_PINDA0

So, this could be removed by blocking the registration of these macros for any but the default probe. In one of my personal iterations, I just put a try/catch around the registration to block this duplication.

[garethky]

+1 to this being out of step with the way existing commands are set up. Looks like you are defining QUERY_PROBE _MY_PROBE and I don't know of that pattern anywhere in the codebase.

[cekim-git]

The named macros are not integral to the underlying functionality at all...

I will remove them.

Frankly, they are mostly there as a placeholder for how best to avoid the duplicate macros and/or raise the issue of potentially re-factoring the probe command helper, but at this point I think that's too much of a distraction and best to isolate the addition of this functionality first.

So, I'll remove them and add a check for any previous registration to prevent the duplicate registration and corresponding error.

Again, for anyone confused - the functionality of PROBE_ACCURACY PROBE=name is already there... this issue is that there is, in my initial PR, ALSO the ability to use PROBE_ACCURACY_NAME not really intentionally, but as a consequence of avoiding duplicate macro registration for the Nth probe by just appending the name...

[garethky]

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.

[cekim-git]

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]

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:

klipper/klippy/extras/display/display.py

Lines 208 to 213 in f2e69a3

	gcode.register_mux_command('SET_DISPLAY_GROUP', 'DISPLAY', name,
	self.cmd_SET_DISPLAY_GROUP,
	desc=self.cmd_SET_DISPLAY_GROUP_help)
	if name == 'display':
	gcode.register_mux_command('SET_DISPLAY_GROUP', 'DISPLAY', None,
	self.cmd_SET_DISPLAY_GROUP)

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]

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:

klipper/klippy/extras/display/display.py

Lines 208 to 213 in f2e69a3

	gcode.register_mux_command('SET_DISPLAY_GROUP', 'DISPLAY', name,
	self.cmd_SET_DISPLAY_GROUP,
	desc=self.cmd_SET_DISPLAY_GROUP_help)
	if name == 'display':
	gcode.register_mux_command('SET_DISPLAY_GROUP', 'DISPLAY', None,
	self.cmd_SET_DISPLAY_GROUP)

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.