Handle probe tripping again during probe move deceleration

[DauntlessAq]

Background and issue:

LinuxCNC's motion planner implements logic for how to handle the motion.probe-input hal pin rising, normally described as the probe 'tripping'.

However, there is currently a issue in this logic. If the probe trips again during probe move deceleration following an initial probe hit, an error indicating that the probe has tripped "during a non-probe move" occurs.

Semantically, this is misleading. The machine cannot decelerate to a stop instantly, so additional probe triggers are to be expected if the switch has non-zero bounce. You cannot have a probe move without a deceleration phase.
I and many others, upon seeing the "probe tripped during a non-probe move" error message, expected that the issue was bounce during the subsequent pulloff move, not the probing move itself.
In fact, due to generally slower feedrates during probing moves vs. pulloff moves, if this error message is seen during probing, it is very likely to be from the probe move and not the pulloff move.

So, while various solutions of course exist in HAL to successfully combat this issue, ultimately the error message is not correct and there is a strong reason to add logic to uniquely identify probe trips during probe deceleration moves.
Therefore, the simplest fix would be to check the motion state, which remains emcmotStatus->motionType == EMC_MOTION_TYPE_PROBING until probing deceleration is finished (nb. the motion type is exposed as the hal pin motion.motion-type OUT), and use this to customise the error message.

However, I believe that it makes sense to go further.
As the "probe tripped during a non-probe move" error cannot currently be inhibited by an INI[TRAJ] config option (unlike the options for probe trips during jogging or homing), once the "probe tripped during probe deceleration error" is added, it makes sense to be able to optionally inhibit this error in the same INI[TRAJ] manner as for probe trips during jogging or homing.

As with the current probe trip inhibition options, the NO_PROBE_DECEL_ERROR is currently configured to not be enabled by default, preserving existing behaviour.

However, they are some arguments for enabling the inhibition by default (and having an option to disable):
The current inhibition options are set in a manner which stops the machine during movement. However, when the probe trips during probe deceleration, the machine is already stopping, so the error is somewhat redundant.
But I believe that since the additional probe triggers are still considered erroneous (a "perfect" probe would have no bounce), they should be reported and then disabled by choice.

Explanation of existing logic:

This logic is implemented in process_probe_inputs(); in src/emc/motion/control.c

The current logic (not to be taken literally) is essentially:

// (read the probe input)

// if (emcmotStatus->probing):
//      if probe is triggered:
//            handle probe trigger reporting
//            emcmotStatus->probing = 0
//      (else if (move finished): ...)

// (^ The above is simple, but the line "emcmotStatus->probing = 0" will become an issue below for the next cycle.)

// else if probe is triggered:
//      if making a coordinated move: 
//           Report Error: "Probe tripped during non-probe move!!!"
//      if homing:
//           Report Error: "Probe tripped homing!!!" unless suppressed by [TRAJ]->NO_PROBE_HOME_ERROR = 1
//      if jogging (free or teleop):
//           Report Error: "Probe tripped during jog (joint or axis reported separately)" unless suppressed by [TRAJ]->NO_PROBE_JOG_ERROR = 1

So, currently, there are a number of ways in which the probe tripping can be handled depending on the machine state (some of which can be suppressed).
However, this actually disguises just how simple the logic is for the probe tripping during coordinated moves. Currently, the result is either: the first time the probe has triggered during a commanded probing move
OR
Error: Probe tripped during non-probe move

As discussed, this turns out to be an issue, because the machine's deceleration is not instant.
Once the probe triggers, further triggers are considered to be triggers "during a non-probe move"

Modifications:

Main logic in control.c as discussed above
Add INI[TRAJ]NO_PROBE_HOME_ERROR option
Modify tests so the expected result includes the additional inhibit variable so the automated tests do not fail.

(The build with these modifications passes the automated tests, but I haven't tested it besides this)

Any feedback on the error message and docs wording would also be really appreciated!
Thanks!

[andypugh]

I am not sure that there is any value in inhibiting the inhibition of the probe signal during the deceleration phase.
I think that I would have been tempted to simply have

718     } else if (!old_probeVal && emcmotStatus->probeVal && emcmotStatus->motionType != EMC_MOTION_TYPE_PROBING) {

Though that's a bit untidy, and your version may be clearer to read.

I would have the inhibit during decelleration default to "on" in any case. It's an established bug and nobody has ever argued that erroring on contact bounce during a probe is desired behaviour.

[DauntlessAq]

Andy, thanks for looking at this!

I did in fact initially start by adding a single line like that.
Just wasn't sure if there was appetite for changing existing behaviour, even if almost everyone seemed to consider it a bug, I didn't want to propose that as the only option.

I suppose the only thing that your change complicates is that someone could observe a second 'trigger' using halscope to examine motion.probe-input, but might be confused as to why no action seemed to be taken.
Currently, any probe trigger is always handled with either an error message (which can be optionally inhibited in some cases) or is the valid result of a probe move.
With this change, we're adding a third category of probe trigger where it is silently ignored.

But the docs can explain this.
(and in control.c can always just add a comment by your line stating that probe deceleration error is silently ignored as a result)

Although, the other benefit to specifically catching the probe decel error within the coordinate move conditional (even if it's then handled silently with no action) is that there might be some scope to clean up the logic a little.

From my understanding, if all coordinated move probe trips are caught and handled, then we can put all the jog/homing probe trigger logic under 'else' (which it isn't currently), despite a coordinated move and (jogging/homing) being mutually exclusive?

[andypugh]

I have asked for other opinions on the developers mailing list.
However, so far, there has only been one private reply, which isn't visible here:
https://sourceforge.net/p/emc/mailman/emc-developers/thread/CAN1%2BYZXkndKDbw0o%2BwbqGgcn%3De5M370-yt1cPCkNzMriM8R19Q%40mail.gmail.com/#msg59216195

[mozmck]

This sounds like a good bug fix to me - and I agree with Andy that a disable option for it would probably not be very useful.

[DauntlessAq]

In which case, I'd propose this:

    } else if (!old_probeVal && emcmotStatus->probeVal) {
        // not probing, but we have a rising edge on the probe.
        // this could be expensive if we don't stop.

        if(!GET_MOTION_INPOS_FLAG() && tpQueueDepth(&emcmotInternal->coord_tp)) {
            // running an command
            if (emcmotStatus->motionType != EMC_MOTION_TYPE_PROBING) {
                tpAbort(&emcmotInternal->coord_tp);
                reportError(_("Probe tripped during non-probe move."));
                SET_MOTION_ERROR_FLAG(1);
            }
        }

The reason for the nested if statements (along with increased the logic being a bit more readable), is that separating out the check for motion type from the check for a whether a command is being run means that the rest of the home/jog checking logic can be placed in an 'else' block which is only executed when a command is not being run.

Since only one type of abort should be able to take place at any given time, and the motion planner can only be in one state at a given time, this change does improve clarity.
(unless I'm wrong and there is something I'm not aware of)

Code here:
DauntlessAq@d69a9fb
(shows lots of lines changed but almost all are just indentation, passes tests)

That said, I don't mind dropping the addition of the 'else' if it's out of scope for the initial bugfix (especially to get this into 2.9)!

[andypugh]

I like the new code.
Can you re-base the pull request on 2.9, without the new INI entry? I think that this is a bug fix.

Partly it's a bug fix because it says "on non-probe move" when it still is a probe move, albeit the post-trigger phase.

[andypugh]

Closed as fixed in 2.8