Draft
Conversation
d4a7f2f to
6eeb153
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is changing?
This PR does 2 things:
PrinterProbe.run_probe()that allows callers to force it not to retract on the final probing movePrinterProbeto change the default behavior ofrun_probe()to always retract. This causesPROBEto always retract on the final probing move.This allows
LoadCellProbeto always retract and leaves other probe types unchanged.Why is this changing?
Back when
run_probe()was originally written there was only 1 kind of probe, a pin based probe that was either triggered or not triggered. This allowed the homing code to check if the probe was triggered before movement and to not move the toolhead if the probe was triggered already. This prevents crashes.run_probe()was written as a primitive to be used by other code likeProbePointsHelperwhich manages retract moves between probe points. Sorun_probe()did not retract on the final move because it assumed that the caller would take responsibility for that.Now we have load cell probes, a kind of analog probe that cant tell if the probe is triggered just by looking at an analog value. It requires movement to trigger. This means every
PROBErequires movement to trigger. Multiple calls to thePROBEcommand continue to move the toolhead downwards, exerting ever increasing force on the toolhead and bed.So this changes allows LoadCellProbe to say that it does not want
PROBEto end without retracting. Further is creates a contract that any future callers of therun_probe()method will have to passalways_retract=Falseto take full responsibility for correctly retracting the toolhead.Alternatives Considered
Don't alter the signature of
run_probe()We could leave the signature of
run_probe()as-is and have an internal-only method inPrinterProbethat doesn't retract (i.e._run_probe()). This breaks the behavior in Z Tilt that usesrun_probe(). It doesn't appear thatProbePointsHelpercan be used here. Z Tilt seems to assume that the printer has a non-contact sensor (like a proximity sensor) and requires a manual probe to touch off the nozzle. A fix for that would require a callback per-probed point to be added toProbePointsHelper.Make retracting the default for all probe types.
This would drop the constructor argument and always default to retracting unless
always_retract=Falseis passed intorun_probe(). This might run into issues if there are macros out there using PROBE and expecting the toolhead not to move up afterwards.Make LoadCellProbe stay triggered until it retracts
Deciding when the probe has reset is actually very difficult. The zero point of the probe drifts over the course of the probing move, so comparing to the initial tare value isn't reliable. You could look at what the force value was just before the trigger, but even that isn't a reliable zero. When the toolhead is retracted upwards its 0 point changes again and might not match what it was when it triggered.
An alternate approach might be to track the Z position since the last probe and make sure it has moved upwards at least by some minimal value (0.1mm?) since the trigger. But I'm not sure how you would easily track that.
horizontal_z_clearancecan be quire small so it would have to be a small value to not block normal opperation.Checklist