Cleanup and consolidate probe conditionals for clarity

[thinkyhead]

Reduce repetition and give names to groups of conditions to clarify the code.

[jbrazio]

Why Z_MIN in HAS_Z_MIN_PROBE_PIN ?

[thinkyhead]

It reflects the naming of the pin. If the pin was called PROBE_PIN then by convention, this would be HAS_PROBE (but you can see why that's confusing, given the naming of some other conditionals, like HAS_ANY_PROBE and HAS_Z_PROBE.) It's because of this confusion that I even added _PIN to the end in contravention.

Maybe we should consider BED_PROBE_PIN as the pin name. But BED almost always refers to heaters/thermistors and the bed, not things that probe the bed… Naming things can be tricky.

[thinkyhead]

It's really Z_MIN_PROBE_ENDSTOP that I most want to rename. It's too damn vague.

[Blue-Marlin]

Indeed we have removed the possibility to have z-max, x-max, x-min, y-max, y-min probes/servo-endstops. So z-min is the only remaining one. So PROBE seems to be enough to describe.

A possible concept for the use and pin configuration of a probe could be

// If you have a probe, select one of the next three options.
Z_MIN_PROBE_ENDSTOP ->                USE_PROBE_AT_PROBE_PIN_FOR_HOMING_AND_PROBING
DISABLE_Z_MIN_PROBE_ENDSTOP ->        USE_PROBE_AT_PROBE_PIN_FOR_PROBING_ONLY
Z_MIN_PROBE_USES_Z_MIN_ENDSTOP_PIN -> USE_PROBE_AT_Z_MIN_PIN_FOR_HOMING_AND_PROBING
// If you don't have a probe, a z-min endstop, connected to a Z_MIN_PIN will be used to home towards z-min. No probing is possible.

The names are a bit long, but hopefully self explaining.

[thinkyhead]

we have removed the possibility to have z-max, x-max, x-min, y-max, y-min probes/servo-endstops

Looks like nobody told homeaxis() that fact…

#if HAS_SERVO_ENDSTOPS
  // Engage an X or Y Servo endstop if enabled
  if (_Z_SERVO_TEST && servo_endstop_id[axis] >= 0) {
    servo[servo_endstop_id[axis]].move(servo_endstop_angle[axis][0]);
    if (_Z_PROBE_SUBTEST) endstops.z_probe_enabled = true;
  }
#endif

[thinkyhead]

Would it make sense for someone to enable the probe for homing, but not for probing? If not, then there's no need to specify "use the probe for probing." You only need to specify that it needs to (also) be used for homing, or not. For example…

#define USE_PROBE_FOR_HOMING // The probe is also used to home Z

This might then be handed by conditionals, similar to this…

#if Z_MIN_PROBE_PIN == Z_MIN_PIN
  #define Z_MIN_PROBE_USES_Z_MIN_ENDSTOP_PIN // set if not already set
  #define USE_PROBE_FOR_HOMING               // always set with this... right?
#endif

#if DISABLED(Z_MIN_PROBE_USES_Z_MIN_ENDSTOP_PIN) // if not set by user or pin-matching…
  #if DISABLED(USE_PROBE_FOR_HOMING)
    #define DISABLE_Z_MIN_PROBE_ENDSTOP // probe pin only for probing
  #else
    #define Z_MIN_PROBE_ENDSTOP         // use probe pin for homing too
  #endif
#endif

Again, a proper table / chart would help to make sense of this. I'm putting it together in a spreadsheet along with how these options actually manifest in the code.

Z_MIN_PROBE_ENDSTOP                   off   on    off   on    off   on    off   on
DISABLE_Z_MIN_PROBE_ENDSTOP           off   off   on    on    off   off   on    on
Z_MIN_PROBE_USES_Z_MIN_ENDSTOP_PIN    off   off   off   off   on    on    on    on
---------------------------------------------------------------------------------------
Good Combination:                     ?     Y     N     Y     Y     N     N     N
G28 uses Probe:                       N     Y     N     Y     Y     Y     Y     Y

If these options are, in fact, mutually-exclusive, then they really should be combined into a single user-facing option.

[jbrazio]

Would it make sense for someone to enable the probe for homing, but not for probing

This makes no sense, they should use a endstop for that.
Specially in the future where "probe" will be an object probe thus with the objective of "probing".

From you logic table I would say we could go with one option to rule them all.

[thinkyhead]

one option to rule them all

I've updated the table above to reflect the most up-to-date information that I have. Only 4 (or maybe 3) combinations out of 8 possible combinations are "legal."

[thinkyhead]

Additional question. If a probe exists, is it required to set one of Z_MIN_PROBE_ENDSTOP or Z_MIN_PROBE_USES_Z_MIN_ENDSTOP_PIN. Or can they both be left disabled?

[jbrazio]

Z_MIN_PROBE_ENDSTOP active with DISABLE_Z_MIN_PROBE_ENDSTOP active is it really a valid configuration ? I would not consider it..

Additional question. If a probe exists, is it required to set one of Z_MIN_PROBE_ENDSTOP or Z_MIN_PROBE_USES_Z_MIN_ENDSTOP_PIN. Or can they both be left disabled?

From a functionality standpoint.. if you have a probe you need to have it connected somewhere, we give the user two options to connect the probe to, so one must be selected if a probe is exists.

[thinkyhead]

The first sanity check only ensures that there is some kind of pin available:

/**
 * A probe needs a pin
 */
#if !PROBE_PIN_CONFIGURED
  #error A probe needs a pin! Use Z_MIN_PROBE_USES_Z_MIN_ENDSTOP_PIN or Z_MIN_PROBE_PIN.
#endif

It doesn't require either Z_MIN_PROBE_USES_Z_MIN_ENDSTOP_PIN or Z_MIN_PROBE_ENDSTOP to be set.
All it requires is either:

• Z_MIN_PROBE_PIN is defined, or
• Z_MIN_PIN is defined and Z_MIN_PROBE_USES_Z_MIN_ENDSTOP_PIN is enabled.

[Blue-Marlin]

Additional question. If a probe exists, is it required to set one of Z_MIN_PROBE_ENDSTOP or Z_MIN_PROBE_USES_Z_MIN_ENDSTOP_PIN. Or can they both be left disabled?

If non(e) of them is defined the probe is connected nowhere -> we don't have a probe.

[thinkyhead]

So basically Z_MIN_PROBE_ENDSTOP is saying that there is no separate endstop. The probe is the endstop. (aka, NO_Z_ENDSTOP_ONLY_Z_PROBE).

[jbrazio]

Shit, all this names are making be brain twist.

So basically Z_MIN_PROBE_ENDSTOP is saying that there is no separate endstop. The probe is the endstop.

Yes.

[thinkyhead]

Yes.

…and it's connected to the Z_MIN_PROBE_PIN.

[thinkyhead]

See, I also keep imagining a situation (highly unlikely) where there's a Z min endstop attached to the Z_MIN_PIN, but then there's also a probe connected there (thus requiring Z_MIN_PROBE_USES_Z_MIN_ENDSTOP_PIN). I gather that we are not accounting for that situation, which would require a pair of Normally-Open switches.

[jbrazio]

Wait.. I've gone into the documentation and we have this:
Enable Z_MIN_PROBE_ENDSTOP to use _both_ a Z Probe and a Z-min-endstop on the same machine.

For my machine where I do not have a z-endstop other than the probe I use the following settings for RC4:

//#define Z_MIN_PROBE_ENDSTOP
#define Z_MIN_PROBE_USES_Z_MIN_ENDSTOP_PIN
//#define DISABLE_Z_MIN_PROBE_ENDSTOP

Which you have listed as a non working setup.

-edit-
Nevermind this is a hack for Hephestos 2.
The board has a specific "probe pin" for those inductive 12V sensors, but uses a "special" 5V inductive sensor which is connected to the Z-endstop pin.

[thinkyhead]

listed as a non working setup

Sure about that…? 😄

Z_MIN_PROBE_ENDSTOP                   off
DISABLE_Z_MIN_PROBE_ENDSTOP           off
Z_MIN_PROBE_USES_Z_MIN_ENDSTOP_PIN    on 
-----------------------------------------
Good Combination:                     Y  
G28 uses Probe:                       Y  

[jbrazio]

Nevermind this is a hack for Hephestos 2.
The board has a specific "probe pin" for those inductive 12V sensors, but uses a "special" 5V inductive sensor which is connected to the Z-endstop pin.

[thinkyhead]

Ok, so here's some more brain-twisting. If the documentation says that Z_MIN_PROBE_ENDSTOP allows you to have both a Z min endstop and a Z min probe, how then does it also mean that the Z probe is used for homing, as stated above? :

Z_MIN_PROBE_ENDSTOP -> USE_PROBE_AT_PROBE_PIN_FOR_HOMING_AND_PROBING

The documentation thus implies that a separate Z min endstop switch exists, but that it must also be connected to the Z_MIN_PROBE_PIN since that is the pin used for homing.

Or is USE_PROBE_AT_PROBE_PIN_FOR_HOMING_AND_PROBING a misstatement?

[thinkyhead]

Z_MIN_PROBE_ENDSTOP active with DISABLE_Z_MIN_PROBE_ENDSTOP active is it really a valid configuration ? I would not consider it..

And yet the documentation says you must enable Z_MIN_PROBE_ENDSTOP if you enable DISABLE_Z_MIN_PROBE_ENDSTOP because otherwise it does nothing.

// This only affects a Z probe switch if you also have a separate Z min endstop
// and have activated Z_MIN_PROBE_ENDSTOP above.
// If you're using the Z MIN endstop connector for your Z probe, this has no effect.
//#define DISABLE_Z_MIN_PROBE_ENDSTOP

Note that the only place it is used is in pins.h:

#if ENABLED(DISABLE_Z_MIN_PROBE_ENDSTOP) || DISABLED(Z_MIN_PROBE_ENDSTOP) // Allow code to compile regardless of Z_MIN_PROBE_ENDSTOP setting.
  #undef Z_MIN_PROBE_PIN
  #define Z_MIN_PROBE_PIN    -1
#endif

[thinkyhead]

Note therefore the redundancy of this in endstops.cpp:

#if HAS_Z_MIN_PROBE_PIN && ENABLED(Z_MIN_PROBE_ENDSTOP)

Since DISABLED(Z_MIN_PROBE_ENDSTOP) leads directly to !HAS_Z_MIN_PROBE_PIN.

This is probably sanity-checked already too. Still, better safe than sorry…

[thinkyhead]

It seems as though these might be reducible to two options:

• Z_MIN_PROBE_USES_Z_MIN_ENDSTOP_PIN also means: "don't use Z_MIN_PROBE_PIN."
  • In other words, it seems to mean the same thing as DISABLED(Z_MIN_PROBE_ENDSTOP).
• USE_Z_PROBE_FOR_HOMING means: "use the probe for homing (also)."

[jbrazio]

So if you set USE Z PROBE FOR HOMING it means ignore endstop pin and use the probe connected in the probe pin as an endstop ?

[Roxy-3D]

@> Would it make sense for someone to enable the probe for homing, but not for probing?

For people with a Z-probe, it is very possible that they use the Z-Probe (out of necessity) to home the machine. But if they have Mesh Bed Leveling, or their machine's mechanics are very good, they may not choose to spend the time on each print to actually probe. So "Yes!" Some people do use the probe to home but do not routinely probe with it.

See, I also keep imagining a situation (highly unlikely) where there's a Z min endstop attached to the Z_MIN_PIN, but then there's also a probe connected there (thus requiring Z_MIN_PROBE_USES_Z_MIN_ENDSTOP_PIN). I gather that we are not accounting for that situation, which would require a pair of Normally-Open switches.

No. I don't think this is true. During the early days of Auto Bed Leveling when people were printing parts and converting their printers to use it, some people chose to leave the Z-Min end stop in place. Probably mostly so they could back track easily if they chose to do that. Back then the question was "Can I leave the Z-Min endstop connected?" The answer was "Yes... But if you have normally open switches, you wire them in parallel. If you have normally closed switches, you would wire them in series." There are people with both examples out there today running Marlin.

Getting good, intuitive names for these things is important! That is how I ended up going in the ditch. I made some assumptions that just were not true. (With a Delta you home towards Max, so I figured to be safe I didn't want the Z-Min-Pin defined. Wrong answer!)

[jbrazio]

Roxy but in that case the probe is still a probe, user just opted out not to do any G29 voodoo. The question here I believe it's to use a probe only as a endstop.

Technically you're right, what differentiates a probe from a endstop is either it is mounted to the chassis or to the tower.

[thinkyhead]

Still a lot to mull over here. I probably need to keep working on my flowchart to make full sense of it all.

[Roxy-3D]

I don't think we want to do this... But one way to clean up the names would be to head in this direction. What if we had:

#define Z_MIN_on_PIN xxxx
#define Z_PROBE_on_PIN yyyy

At that point, depending upon what is defined, and the values they have, it is clear what is going on. For example, if only Z_PROBE_on_PIN is defined, it is clear what to do in both the probing case and the homing case. Similarly, if only Z_MIN_on_PIN is defined, it is clear there is no probe and we are going to use that value for homing.

In the case where they are both defined there are two sub cases. If they are both the same, well... That is straight forward, for either homing or probing we use the same pin number. If they are defined to different pins, it is still clear what to do. Use the appropriate one!

Note: Part of the problem we have right now is the configuration of these pins is 'Brittle'. If things are not specified exactly correctly things break. In the above example, it is not 'Brittle'. If the user over specifies the situation such as setting both of the above values to the same pin, the firmware would not care. Life goes on and everybody is happy. And there are no new threads asking for help!

[Roxy-3D]

The problem is it is way too complicated. If it was just that, OK, it would be obvious what to do. But it isn't. I have options to #define DISABLE_Z_MIN_PROBE_ENDSTOP and for #define Z_MIN_PROBE_ENDSTOP and #define ENDSTOPS_ONLY_FOR_HOMING. If we combine those with the two you mentioned we have 32 different things that happen and I have no idea how to set things up. After all, if I do an ENABLE_PROBE what does DISABLE_Z_MIN_PROBE_ENDSTOP and Z_MIN_PROBE_ENDSTOP do? I don't have a clue.

My vote would be to get rid of enough #define's that we can turn on any combination of what is left and the 'Right' thing happens.

Next you have to factor in USE_X_MAX_PLUG and you jump up to 64 combinations. Use the X_MAX_PLUG for what???? Is it mutually exclusive with something? I don't know. If the title of the #define was USE_X_MAX_PLUG_FOR_Z_PROBE it would be a lot clearer to me.

[jbrazio]

You're completely right, there are too many options and most of them are badly named.

[Roxy-3D]

I don't have the answer. And I don't care what the answer ends up being. But I'm telling you, if I'm having a hard time getting those #define's set up so my printer works, for sure other people are having trouble too.

And that is kind of why I wanted to explore the question "What doesn't work if we start with just two #defines?" I'm not even saying, lets do that. I'm just hoping that will shed some light on what direction we should go. If only one or two bizarre corner cases don't work... We can find a way to work around that. It might be to change what the two #defines specify. Or who knows, maybe we add a 3rd #define that only gets turned on in some weird case and we clearly document most people should not be enabling it.

One more note: I think that set of #define's like USE_X_MIN_PLUG are there just because that avoids having the user specify a pin. If that is true, let's add one that says #define USE_UNPOPULATED_EXTRUDER_DIRECTION_PLUG too! This maybe a bad analogy, but it is kind of like the religious view of never ever using a 'goto'.

The honest truth is every once in a while the code can be written clearer if you are willing to use a goto. Or if you have problems believing that, compare how much code it takes to write a recursive decent parser if you can't do a setjmp() and longjmp() to unwind the stack when you hit an error. Those are the ultimate goto on steroids. I promise you, the setjmp()/longjmp() version is going to be simpler and cleaner code!

PS. I really am for super clean code! But if a goto really makes the code simpler and cleaner... I won't even blink! (And I'm really not dis'ing and throwing rocks at ThinkHead! It might read like that, but I'm definitely not doing that.)

[tiagom62]

In my case the whole confusion stemmed from incorrect configuration comments and lack of sanity checks for invalid configurations which both i believe thinkyhead addressed.

Perhaps there is a cleaner way to address this but is it worth the effort for something that works?

[thinkyhead]

It also dawned on me that for many boards, only three endstop plugs are defined, then assigned to min or max pins later.

[thinkyhead]

Once again, much food for thought. I will work on my Venn diagram and get back to this over the weekend…

[thinkyhead]

To help with the confusion in configurations, I think it will help to give a good set of example setups in the comments. Basically saying, if you have this probe and this endstop and this connector set these options… and if you have this probe and this endstop and this connector set these options…

[tiagom62]

Agreed.. Examples in the config would be very helpful.

[Roxy-3D]

Once again, much food for thought. I will work on my Venn diagram and get back to this over the weekend…

And my guess is... Nobody involved thinks these drastic changes go into RC-6. Right now, it probably makes the most sense to live with what we got.

I came back and edited this in: This Merge Request isn't about anything as drastic as what I'm saying we should consider. If these changes help, "Hell Yes!!!" do them. But I'm of the opinion the over haul of all the Z-Probe complexity needs to wait until we can work through all the issues in a development branch. My reaction is not a "No!" vote on this merge!

[thinkyhead]

If you have viewed the changes you can see there are no user-facing changes except the extra sanity. I just wanted to be thorough, and it's a good discussion, important to consider from the user POV. At any rate, a configuration utility with simplicity first will help, and the new documentation is going to be invaluable. Thinking about video as an adjunct to that next.

[thinkyhead]

I'm merging this as it is mostly harmless.
I would like to get feedback on whether the following test is in fact valid:

#elif ENABLED(DISABLE_Z_MIN_PROBE_ENDSTOP) && DISABLED(Z_MIN_PROBE_ENDSTOP)
  #error DISABLE_Z_MIN_PROBE_ENDSTOP requires Z_MIN_PROBE_ENDSTOP to be set.
#endif

I included this because the comment on DISABLE_Z_MIN_PROBE_ENDSTOP implies this is correct.

[Blue-Marlin]

Z_PROBE_ALLEN_KEY vanished. ???

[thinkyhead]

Z_PROBE_ALLEN_KEY => MECHANICAL_PROBE.

[jbrazio]

@thinkyhead After all this discussion we kept the same names ?..
:-/

[thinkyhead]

That's right. My PR was never about changing the names, only about clarifying the C++ code and catching bad configurations. I still haven't decided the best way to proceed regarding the configuration options, given the information I have. And I've been in the process of "getting dumped" so I haven't had the best focus for making Venn diagrams recently.

[jbrazio]

But I would suggest to try to fix the names before GA, otherwise we may keep having the same issues over time. @thinkyhead I know we may be crossing the line here for an RC but it's for a really good cause, @Roxy-3DPrintBoard will not get mad at us this time for sure. :-P

[ghost]

Cause of confusion about Allen Key is my thoughtless implementation for MECHANICAL_PROBE.
Please look at that bad implementation...
I implemented the MECHANICAL_PROBE by overwriting many of Z_PROBE_ALLEN_KEY.
I wasn't able to realize that it's foolish.
I'm regretting now what I’ve done.

Thus, I've submitted a PR #3725 for fixing it.
I hope that This PR will settle confusion...

[thinkyhead]

@esenapaj Thanks for the update. I will evaluate and perhaps merge later!

[Blue-Marlin]

Maybe, all our asking has been too indirect.
All what we want to know is:

If we have a probe
  Where is it connected?
  Do we want it to be used for homing?

#define PROBE_USES_PIN <pin> // can be Z_MIN_PIN or Z_MIN_PROBE_PIN or any other pin number
#define USE_PROBE_WHEN_HOMING_TO_Z_MIN

Where a not defined PROBE_USES_PIN or '-1' implies, we do not have a probe.
Where a not defined USE_PROBE_WHEN_HOMING_TO_Z_MIN implies to use the endstop.
Having defined USE_PROBE_WHEN_HOMING_TO_Z_MIN without a valid PROBE_USES_PIN could ether throw an error, or could be ignored silently (the endsop is used for homing).

[thinkyhead]

@Blue-Marlin It would be interesting to see if that approach makes the configuration any nicer. In general we've been going in the direction of hiding pins, using options to automatically select them. Adding a pin-based option to the main configuration (as opposed to it being automated, with an override for special needs in the advanced configuration) would seem to be a step back towards the techie side.

[Blue-Marlin]

@thinkyhead
It was just an idea where i hoped for, my mum can understand it.

I have a little exercise for you.
Please try to connect a probe to the Z_MIN_PROBE_PIN on a RAMPS with display.

Which files did you open to get the right information?
Which files did you change to make it work without soldering?

http://reprap.org/wiki/File:Arduinomega1-4connectors.png
https://github.com/MarlinFirmware/Marlin/blob/RC/Marlin/pins_RAMPS_14.h
https://github.com/MarlinFirmware/Marlin/blob/RC/Marlin/Configuration.h

[thinkyhead]

Which files did you open to get the right information?
Which files did you change to make it work without soldering?

Yes, it's a tricky prospect. I expect most probes that you can buy come with instructions on the best pins to use, or there's advice on the reprap wiki. Having not installed a probe on my machine, I haven't had to go through the process. I'd like to see us try to accommodate the most common setups, at least, and then offer additional options when users can't use some pins, either because they don't exist or are in use by something else.