Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Give more time to reset the BL-Touch after a failure to raise the probe #2655

Merged
merged 6 commits into from
Apr 2, 2020

Conversation

KevinOConnor
Copy link
Collaborator

In the event that a "raise probe" operation fails, the current code will issue a "reset" command to attempt to clear the error state. However, the "reset" command is only sent for 100ms. It appears a longer time is needed ( see #2267 (comment) ). This update reworks the code so the existing RETRY_RESET_TIME delay time of 1 second is now done while the "reset" command is being issued. This should hopefully give the BL-Touch more time to update its internal state.

@FanDjango - does this change look okay to you?

-Kevin

@FanDjango
Copy link
Contributor

I can only test this in a few days, printer is busy right now.

So I can only just comment tentatively:

c367667 Yes, although one hardly notices this. Currently pin goes up and I suppose if you look closely, you can see that the head also begins to move at the same time. Actually, in one version of my test code, I was purposely doing something like that to speed up the retract move + pin up operation by having them happen simultaneously. Now, with no retract between samples, this would not be an advantage.

For users who do retract between samples the effects of this change are zero as there would already have been a self.sync_print_time() invocation in self.probe_finalize().

4849414 Oh, yes. But for safeties sake maybe one could also apply the 1s reset time at this place also:

    def raise_probe(self):
        self.sync_mcu_print_time()
        if not self.pin_up_not_triggered:
            # No way to verify raise attempt - just issue commands
            self.send_cmd('reset')
            self.send_cmd('pin_up', duration=self.pin_move_time)
            self.send_cmd(None)
            return

@KevinOConnor
Copy link
Collaborator Author

c367667 [...] Now, with no retract between samples, this would not be an advantage.

FYI, this change is only to multi_probe_end() so it would only occur at the very end of the probing tool. It should therefore be an inconsequential slow-down. The goal of this change is to ensure we don't start a print while the pin is still rising.

4849414 Oh, yes. But for safeties sake maybe one could also apply the 1s reset time at this place also:

I'm only expecting that code path to run on really old clones. (I'd expect most users to leave pin_up_reports_not_triggered at its default.) I don't know what the sequence should be for the old clones (that can't verify the pin has been raised), so I just left it as it was before. I suspect you have more knowledge of what should be done on these old clones - so if you know of a preferred timing then that would be great. I'd implement it as a separate commit though.

Thanks,
-Kevin

@FanDjango
Copy link
Contributor

so it would only occur at the very end of the probing tool

I was aware of that. I only meant to say that: If someone does not retract between probes there is no advantage to a hack that I was playing with, namely to raise the pin and to start moving quasi simultaneously instead of serially. It was a bit OT, so sorry.

The goal of this change is to ensure we don't start a print while the pin is still rising.

Needs to be done. I agree.

@FanDjango
Copy link
Contributor

I suspect you have more knowledge of what should be done on these old clones

Actually that is not the case

But the internal processing that needs to be done in such a probe to effect a reset and perform the internal setup won't be so much different from probe type to clone type - and it would be safe to say that the longer the reset command itself is allowed to perform before further commands are sent to the probe, the better. Note that I am not talking about the duration of the command being held as PWM but rather the grace time after the command has been presented to the probe and a new command is given. In Marlin, both are equal as the PWM is upheld. In klipper one could easily implement this as command-time (with PWM active) followed by wait-time (with PWM inactive). I.e. command recognition time and command execution time. But I digress…

FYI, even at the current state I consider the probe.py to be adequate, I don't really have any problems with it, it works well. The only deficits that turn up are when "something fails", = recovery, but since that very seldom happens on a good setup, it is not an issue.

On the other hand, if someone has a faulty/flaky setup, of course he will run into the recovery scenarios but even if they are resolved well, he won't be happy and should need to fix his hardware.

@KevinOConnor
Copy link
Collaborator Author

But the internal processing that needs to be done in such a probe to effect a reset and perform the internal setup won't be so much different from probe type to clone type - and it would be safe to say that the longer the reset command itself is allowed to perform before further commands are sent to the probe, the better.

Agreed, but note that code path is not a failure case. It's the normal "raise the probe" code path. So, if we wait 1s to issue the reset then everyone with one of these old clones now waits an additional second on every probe.

An alternative would be to just remove the "reset", but it doesn't seem to harm anything and it may help on some old clones that can clear state quickly.

Another alternative would be to remove the pin_up and just issue the "reset" for pin_move_time. However, it was my understanding that some old clones do not raise the pin on a reset command.

-Kevin

@FanDjango
Copy link
Contributor

FanDjango commented Mar 30, 2020

do not raise the pin on a reset command.

Even ANTClabs have not reacted to my reports that RESET on their probes will also not always STOW the pin (contrary to their claims) when the probe is in a certain state.

It's the normal "raise the probe" code path

It is, in the if not self.pin_up_not_triggered: case...

Oh shucks. We need to raise as fast as possible if we have just touched the bed on a downward probe. Either that or/and move the nozzle up as fast as possible. I should have played around with the original code more than I did.

That's also why in Marlin I decided to just raise the probe and then after that, checking for an error condition ("is it raised?"), which raises right away and then spends some time checking, recovering

Does this explain why there is sometimes an observation and even an issue that a certain amount of think time goes by after the bed touch before the pin is taken up? 100ms or so? I mean beyond the other problems that were fixed (race condition etc.) On wednesday I will have the opportunity to test this PR and I will have a look at this specific topic as well.

I quote myself from #2653

The BLTouch, when probing in normal mode (and NOT in SW, aka "touch"-mode) is programmed internally as follows:

The last command, in this case DEPLOY, sets the probe to "want the pin to be deployed". If it gets pushed in, it will notice and it will issue a trigger signal, but it still "wants the pin to be deployed". Thus the buzzing feeling as it tries a couple of times to push the pin out again. This goes on for about 600ms and then the probes software will give up and go into blinking error state.

This means that firmware that moves stoward the bed must stop immediately on a trigger signal and then EITHER stow the probe prior to 650ms, bringing the probe into "wanting the pin to be up" state OR moving the nozzle/probe up as fast as possible prior to 650ms to get out of the "probe cannot keep the pin deployed" error situation.

@FanDjango
Copy link
Contributor

Let me repeat: it works quite well already. And things get complicated very quickly if you want to cover all the bases on this.

@KevinOConnor
Copy link
Collaborator Author

Does this explain why there is sometimes an observation and even an issue that a certain amount of think time goes by after the bed touch before the pin is taken up? 100ms or so?

No, because that code-path is only called when the user has explicitly set pin_up_reports_not_triggered: False in their config. Basically, that code-path only occurs when the user has told Klipper that they have an old clone that is not capable of checking that the probe has been raised. Under the normal case (where the user has not set that config option) then Klipper will issue the pin_up first and then check for an error (and then attempt recovery if needed).

On the other hand, if a user has mistakenly set that config option, then I think it could cause issues. If that is the case I suspect the solution would be to update the documentation to make it more clear that config option really should not be set unless absolutely necessary..

-Kevin

@KevinOConnor
Copy link
Collaborator Author

do not raise the pin on a reset command.

Even ANTClabs have not reacted to my reports that RESET on their probes will also not always STOW the pin (contrary to their claims) when the probe is in a certain state.

That's a good point. Separate to this topic, the test_sensor() code path wasn't actually running pin_up - it was assuming reset would do the pin_up. Since test_sensor() is an infrequent code path, I think it would make sense to run the full reset followed by the full pin_up sequence. I added commit 3566ed3 to this branch.

-Kevin

@KevinOConnor KevinOConnor force-pushed the work-bltouch-20200329 branch from 3566ed3 to 2f5d2a3 Compare March 30, 2020 17:21
@KevinOConnor
Copy link
Collaborator Author

I think it would make sense to run the full reset followed by the full pin_up sequence. I added commit 3566ed3 to this branch.

Actually, on second thought, the infrequent test_sensor() code only needs to run the "reset" if the sensor test actually fails. So, I redid it as new commit 2f5d2a3.

-Kevin

@FanDjango
Copy link
Contributor

FanDjango commented Mar 30, 2020

run the "reset" if the sensor test actually fails

Yes. So if "pin-up, touch mode, verify" fails, we get a retry with "reset, pin-up,...." which is exactly what the doctor ordered. In my experience, no further retries are needed, that single one should do it. Otherwise something is actually broken. But it doesn't hurt to retry a couple of times.

The "reset, pin-up" sequence is important as a replacement for all places where a "reset" is needed conceptually.

Looking forward to testing the PR on Wednesday

@KevinOConnor
Copy link
Collaborator Author

Thanks.

I don't know if you have access to a BLTouch v3.0 and BLTouch v3.1. If you do, it would also be good to see if pin_up_touch_mode_reports_triggered: True (the default) now works on those two devices. We're currently documenting (in docs/BLTouch.md and config/example-extras.cfg) that it needs to be turned off for the "BL-Touch v3", but that may been because the code was not actually issuing the "pin_up" command.

It would be great if we could document that both pin_up_touch_mode_reports_triggered and pin_up_reports_not_triggered should only be configured to False on old clones.

-Kevin

@FanDjango
Copy link
Contributor

I have a 3.0, a pre APR5/19 3.1, a current 3.1, a 2.0 and a clone as well.

It would be great if we could document that both pin_up_touch_mode_reports_triggered and pin_up_reports_not_triggered should only be configured to False on old clones.

That I can understand very well and I will put it on the agenda.

@FanDjango
Copy link
Contributor

BTW this check_end_time = self.send_cmd(None) at the end of a raise (especially when successful) costs us some time before things continue - I get the impresseion that cmd(None) is subject to the minimum command time. Please correct me if this is not correct.

Since we (for example) concat other commands directly behind each other, or look at Marlin, keeping the PWM active, couldn't we have the cmd(None) just turn off the PWM and not wait (what is it? 100ms 5x 20ms?) Don't have the code here right now. Works fine on my development version of bltouch.py

@KevinOConnor
Copy link
Collaborator Author

KevinOConnor commented Mar 30, 2020

I get the impresseion that cmd(None) is subject to the minimum command time.

Yes, and a delay of some kind is needed between the scheduling of two PWM updates even if the update is to turn off the PWM. Otherwise, it's possible to get obscure "Timer too close" errors in some corner cases. The reason is that the Klipper mcu code only supports queing one pwm update at a time within the mcu (if the schedule is deeper then additional updates get queued in the host serial buffer first). This is discussed a bit in #133. Currently Klipper always uses 100ms for this "minimum pin time" delay - I'm sure other values are possible, but I'm not sure it is worth optimizing given the severity of the error (and difficulty to test for it).

That said, it is not necessary to delay future toolhead moves for this period. (It's only necessary to ensure no future pwm updates occur during this delay.) So, it may be possible to optimize sync_print_time() to avoid adding dwell() calls for this extra time. That said, I'm not sure it would make a difference to the common lower_probe() and raise_probe() code paths because the look-ahead queue is already fully flushed on the "homing" code path - which leads to a 250ms restart regardless.

-Kevin

@KevinOConnor
Copy link
Collaborator Author

So, it may be possible to optimize sync_print_time() to avoid adding dwell() calls for this extra time.

FYI, I added that to this branch as commit 5271f35.

-Kevin

@KevinOConnor KevinOConnor force-pushed the work-bltouch-20200329 branch from 5271f35 to 9d8d099 Compare March 30, 2020 23:20
@KevinOConnor
Copy link
Collaborator Author

FYI - I also added another minor optimization (primarily in commit 9d8d099) to this branch. Normally the "pin_up" command comes after the homing code has fully "finalized" which includes querying the "mcu stepper position" for each of the Z steppers - which takes a few milliseconds per stepper. This commit should allow the host to send the pin_up slightly faster (in the stow_on_each_sample=True case). This change, though, is a bit tricky and I don't have a good way to test it.

-Kevin

@KevinOConnor KevinOConnor force-pushed the work-bltouch-20200329 branch from 9d8d099 to 5c46a79 Compare March 30, 2020 23:29
@FanDjango
Copy link
Contributor

FanDjango commented Apr 1, 2020

Ok, I am set up and have started testing.

In loose order I will report everything I am finding out, to remove some misconceptions floating around.

Currently using a After-Apr5/19 BLTouch V3.1 and when I move to older probes I will check these again.

Step 1: possible difference when holding PWM (case 1) or turning PWM off (case 2) after the DEPLOY command:

(Oh, and how am I doing this, sorry, I am using a modified Marlin's LCD commands to send commands directly to the probe, very convenient, with a judicious servo[Z_PROBE_SERVO_NR].detach(); that I added in the right places):

The following holds true for BOTH cases:

The BLTouch, when probing in normal mode (and NOT in SW, aka "touch"-mode) is programmed internally as follows:

The last command, in this case DEPLOY, sets the probe to "want the pin to be deployed". If it gets pushed in, it will notice and it will issue a trigger signal, but it still "wants the pin to be deployed". Thus the buzzing feeling as it tries a couple of times to push the pin out again. This goes on for about 600ms and then the probes software will give up and go into blinking error state.

This means that firmware that moves toward the bed must stop immediately on a trigger signal and then EITHER STOW the probe prior to 650ms, bringing the probe into "wanting the pin to be up" state OR moving the nozzle/probe up as fast as possible prior to 650ms to get out of the "probe cannot keep the pin deployed" error situation.

So it is NOT the held PWM which causes the probe to "want to be deployed"

Step 2: What's with this RESET procedure???:

Once again testing these by issuing single commands to the probe and using my finger to tickle the pin up and down

Here's some (maybe surprising facts):

If the probe is deployed and NOT in an alarm state, you can RESET it to your hearts desire, but it won't stow the pin. In that case you need to issue a STOW to stow the pin (if it is possible).

If the probe is in ALARM state, a RESET will stow the pin (if it can).

Here's a real tricky one: If you are in SW mode, a RESET will not stow the pin (because in SW mode you cannot ever get into an ALARM state. A RESET will turn off the SW mode in part only, as now the trigger will not respond any more on pin-push-up. You are now in a strange half-SW mode, pushing in the pin doesn't cause a trigger signal and the probe is more or less "dead". It's a BUG. An SW mode command gets you back into full SW mode, or a STOW gets you back into normal mode (and the probe will attempt to stow the pin, which might fail), or a DEPLOY will also work, but it will attempt to deploy the pin, which might not be so attractive and could fail.

Conclusion: If you want to reliably reset and stow the probe when you don't know what state it is in, you need to take care of these above quirks:

First, you do a STOW. This would stow the probe even if it is not in an ALARM condition. It would also cleanly terminate an open SW mode or half SW mode.

Second, in an ALARM condition the previous STOW would have failed (be ignored), so now a RESET is done. This will also stow the pin if it happens to be deployed.

That would the following sequence:

#define RELIABLE_RESET is sequence:
STOW(100ms command time, 550ms wait time).
RESET(100ms command time, 900ms wait time).
Final check for alarm, report success or failure of reset

Why the 550ms wait time on the STOW (just as for a DEPLOY, by the way)? A pin movement command might fail because something is physically broken. Then it will take the probe about 550ms of internal retries to recognize that and then go into an ALARM condition.

The final RESET will now reset such an ALARM condition and try to stow the probe.

This might then finally result in an ALARM condition once again which should be checked.

Edit:
First, I will test this with V3.0, V2.0 and clone, then I will..
Ok, so much for these preliminaries. Flashing klipper now and getting the bltouch test branch for klippy.

@FanDjango
Copy link
Contributor

FanDjango commented Apr 1, 2020

Trigger behaviour in normal mode when deployed:

BLTouch V3.1:
It will try to keep the pin down, see previous post. The trigger is not a 10ms pulse, it is actually similar to SW mode, it stay high until the probe goes into ALARM while it tries to (buzzing feeling) push the pin down again. I think ANTClabs reacted to the problems with CReality printers.

I think this is what makes the V3.1 a little finnicky in terms of how fast you need to STOW or move the nozzle up before it goes into ALARM mode. Slow controllers/printers might prefer SW mode for this.

BLTouch V3.0:
It will pull up the pin by itself when triggered and then let it simply drop down again (without impetus from the solenoid). It will only issue a short 10ms signal pulse, but it will do this each time you trigger it. So actually it would be ready for a new probe operation without a DEPLOY.

BLTouch V.20: Same behaviour as the V3.0 but signal is "more powerful" - I won't discuss what that means right here, no time, no space.

Clone (3D-Touch): Interesting differences: When triggered, it issues a single 10ms pulse, (powerful) and turns on the red led. It doesn't do anything to the pin (it limply drops down again). It never triggers again until you do the next DEPLOY command (even though the pin might be down). It is the only probe that can have the red LED turned on when the pin is down.

SW-Mode works as expected on all the above probes.

@FanDjango
Copy link
Contributor

@KevinOConnor
Ok, finally I am testing your new bltouch code on klipper. Currently BLTouch V3.1.

MULTI is used (no stow between probes): works fine

MULTI is off (stow between probes):
error at the end of the probing sequence, probe is raised, klippy dies before z axis moved up

pin_up_reports_not_triggered: True
pin_up_touch_mode_reports_triggered: True
pin_move_time: 0.675
#
stow_on_each_sample: True
sensor_pin: P1.22
control_pin: P1.23
x_offset: 33.0
y_offset: -5.0
speed: 5
lift_speed: 30.0

error:

Unhandled exception during run
Traceback (most recent call last):
  File "/$/OMVSF/Klipper/klippy/klippy.py", line 174, in run
    self.reactor.run()
  File "/$/OMVSF/Klipper/klippy/reactor.py", line 251, in run
    g_next.switch()
  File "/$/OMVSF/Klipper/klippy/reactor.py", line 278, in _dispatch_loop
    timeout = self._check_timers(eventtime)
  File "/$/OMVSF/Klipper/klippy/reactor.py", line 136, in _check_timers
    t.waketime = waketime = t.callback(eventtime)
  File "/$/OMVSF/Klipper/klippy/reactor.py", line 48, in invoke
    res = self.callback(eventtime)
  File "/$/OMVSF/Klipper/klippy/extras/bltouch.py", line 202, in raise_probe_on_trigger
    return self.raise_probe()
  File "/$/OMVSF/Klipper/klippy/extras/bltouch.py", line 125, in raise_probe
    success = self.verify_state(check_start_time, check_end_time, False)
  File "/$/OMVSF/Klipper/klippy/extras/bltouch.py", line 112, in verify_state
    return self.mcu_endstop.home_wait(check_end_time)
  File "/$/OMVSF/Klipper/klippy/mcu.py", line 105, in home_wait
    self._mcu.register_response(None, "endstop_state", self._oid)
  File "/$/OMVSF/Klipper/klippy/mcu.py", line 636, in register_response
    self._serial.register_response(cb, msg, oid)
  File "/$/OMVSF/Klipper/klippy/serialhdl.py", line 158, in register_response
    del self.handlers[name, oid]
KeyError: ('endstop_state', 14)

@FanDjango
Copy link
Contributor

Update: If I turn off the reactor to raise the probe on a trigger, it works fine. I will continue with my testing while maybe you can check that last commit: 5c46a79

@FanDjango
Copy link
Contributor

FanDjango commented Apr 1, 2020

Intermediate result

It would be great if we could document that both pin_up_touch_mode_reports_triggered and pin_up_reports_not_triggered should only be configured to False on old clones.

All of my probes including the clone want:

pin_up_reports_not_triggered: True
pin_up_touch_mode_reports_triggered: True

Setting one of these to false is semantically equivalent to turning off one of the checks being done.

I realize that the names describe exactly what feature/behaviour is referred to, but for the less informed user, these are cryptic.

I would much prefer:

cannot_verify_raise: False
cannot_test_probe: False

Change the eror message in self.test_sensor() to BLTouch test failed'. Then the user, if he gets that, immediately associates it with maybe turning on cannot_test_probe'.

Same goes for the verify, only here the error message is already "compatible".

Edit:

Or of course perhaps I prefer the opposite logic:

can_verify_raise: True
can_test_probe: True

@FanDjango
Copy link
Contributor

FanDjango commented Apr 1, 2020

Intermediate result

In unhomed state, PROBE_ACCURACY will deploy the probe...

Send: M84   <<<<< this turns off steppers
Recv: ok
Send: PROBE_ACCURACY
Recv: // PROBE_ACCURACY at X:69.500 Y:98.500 Z:4.951 (samples=10 retract=2.500 speed=5.0 lift_speed=30.0)
<<<<<probe is deployed>>>>>
<<<<<probe is stowed>>>>>
Recv: !! Error during homing move: Must home axis first: 69.500 98.500 -25.000 [0.000]

I'll open an issue, mabye. This is potentially destructive, the probe should never be deployed in an XY unhomed situation.

@FanDjango
Copy link
Contributor

Observation

If the probe is in an ALARM state, for whatever reason, then deploy actions will obviously fail:

Send: G28 Z
Recv: !! BLTouch failed to deploy
Recv: !! BLTouch failed to deploy

Just like with raise probe, a (perhaps) single retry would be a nice thing and this would get over the blinking state without needing to resort to BLTOUCH_DEBUG COMMAND=RESET.

Some users are putting a sequence of BTLOUCH_DEBUG commands into their [homing_override] or adding some other command macros to make resetting the BLTOUCH less tedious.

Example:

[homing_override]
set_position_z: 0
axes: z
gcode:
 BLTOUCH_DEBUG COMMAND=pin_up
 BLTOUCH_DEBUG COMMAND=reset
 G90
 G1 Z5 F720
 G28 X0 Y0
 G1 X69.5 Y98.5 F7200
 G28 Z0
 G1 Z5 F720

@FanDjango
Copy link
Contributor

Observation

I have reduced second homing speed to 1, probe speed and lift speed to 1, trying to make everything crazy slow - hoping to reproduce the problem (the imagined? problem) where not lifting up again fast enough might cause a problem. No such luck with this version of bltouch.py yet, even if the reactor to raise the probe on a trigger is turned off. Maybe you need not put so much effort in to that.

@FanDjango
Copy link
Contributor

All of the above comments hold valid for all my probes. I am getting tired of changing them over and over again and the little plugs and wires are wearing out. IMHO, apart from some exotic clones, the BLTouch V3.0 is probably the most critical one, because of its weak signal driver. The BLTouch V3.1 comes a close second, because it wants to be STOW-commanded or nozzle-retracted in such a timely fashion after trigger.

@FanDjango
Copy link
Contributor

Test and observation

I have, for stress testing purposes, done the following:

In raise_probe(self), before the actual pin_up command is issued, I have added some time consuming stuff. I looked for something innocuous, that would not affect processing, so I chose None, which uses 100ms.

        for retry in range(3):
            self.send_cmd(None)
            self.send_cmd(None)
            self.send_cmd(None)
            self.send_cmd(None)
            self.send_cmd(None)
            self.send_cmd(None)
            self.send_cmd(None)
            self.send_cmd(None)
            self.send_cmd(None)
            check_start_time = self.send_cmd('pin_up',
                                             duration=self.pin_move_time)
            check_end_time = self.send_cmd(None)

This has the (for testing purposes) desired effect of delaying the pin_up by some time, and demonstrates the following (I should have videoed this):

Pin touches bed. Movement stops ( good! ), pin_up doesn't arrive soon enough, probe starts blinking, retry takes effect and succeeds, nozzle moves up, homing continues.

By reducing the number of self.send_cmd(None) invocations, I can determine how much timing leeway there is for a successful pin_up that comes in time to avoid an alarm state.

Result I can put a maximum of four self.send_cmd(None) in that sequence, if I use more I will get the alarm state.

That means, I have 400ms leeway on my setup. That does not seem like much, somehow. But consider: Of the about 600+ms overall that I know of from using my scope, I have used 200ms at that point (from trigger to that code point). Does that correspond to your estimates @KevinOConnor ?

I suppose one could actually code a routine that determines the leeway on an unknown setup by repeatedly doing this with increasing delay until an alarm occurs. <- actually serious idea

@KevinOConnor
Copy link
Collaborator Author

That means, I have 400ms leeway on my setup. That does not seem like much, somehow. But consider: Of the about 600+ms overall that I know of from using my scope, I have used 200ms at that point (from trigger to that code point). Does that correspond to your estimates @KevinOConnor ?

That's odd. The sync_mcu_print_time() also adds a 100ms of delay, so if you put 400ms of additional delay, that would be 500ms of delay from notification to the start of the first pin_up command. There is some "serial round-trip time" as well and the pin_up command itself takes a few milliseconds to complete, but that should only add ~5-20ms. So, that still leaves a gap of ~100ms unaccounted for.

FYI, you can directly set the delay amount with something like self.send_cmd(None, duration=.550).

-Kevin

@KevinOConnor KevinOConnor force-pushed the work-bltouch-20200329 branch from 5c46a79 to 399e227 Compare April 1, 2020 15:15
@FanDjango
Copy link
Contributor

I understand. Such a change would break existing configs though. I'd be leery of doing that just for a different config name. Perhaps we can reword the documentation to make the impact more clear.

By all means, not my call to make

@FanDjango
Copy link
Contributor

Can I summarize your tests as follows then:....

re. 1: Yes, broken, drop. You were just testing to see if I really tested this :-))
re. 2: Big step forward, worth implementing. Maybe some people volunteer and test it?
re. 3: I believe so too, but more explanatory text is warranted. I will compose some text if you want and pass it on to you.

@FanDjango
Copy link
Contributor

FYI, you can directly set the delay amount with something like self.send_cmd(None, duration=.550).

I know. But the 4 x 100 looks more graphic in the code sequence when posting it here.

AFAIK .550 gets rounded to .560, we are looking at 20ms increments. Tomorrow I will binary search to the best closer value than by 100ms steps.

@fiferboy
Copy link

fiferboy commented Apr 1, 2020

I can definitely test with my clone probe once my current print is finished. I have always needed to have "pin_up_touch_mode_reports_triggered" set to False for this probe, so it will be interesting to see if it works with it set to True (along with "pin_up_reports_not_triggered").

I have also been unable to get this probe to honour the "stow_on_each_sample" set to False with the recent refactor. The best my clone ever worked was with @FanDjango's monolithic branch before it was split up into the four different phased rollouts (I think that version had some command timing differences that never made it into the final version?).

@fiferboy
Copy link

fiferboy commented Apr 2, 2020

My clone (TriangleLab 2019 NEW 3D Touch) behaves like @FanDjango's (when issued a pin_down through Klipper and triggered it will drop back down and turn the red light on). Sometimes when in an active probe it will do this before properly retracting the pin, but usually the retract is successful before the next probe would trigger (sometimes it isn't, though, which leads to a probing error).

The difference is that I can't use:

pin_up_reports_not_triggered: True
pin_up_touch_mode_reports_triggered: True

It results in an error (probably not relevant for this issue but I'll attach a log anyway).

Also: stow_on_each_sample: False doesn't do anything for this probe for some reason. It will trigger, retract, and deploy each time instead of properly using touch mode.

Neither of these issues are particularly relevant to this case but they do seem to represent some difference in my probe behaviour. If there is something more I should test I would be happy to, and if I should open an issue for the other problems I am seeing I will be happy to do that as well.

bltouch-klippy.log

@KevinOConnor
Copy link
Collaborator Author

@fiferboy - Thanks. Just to be clear, this branch does not make it worse for you? Also, I understand you need to set pin_up_touch_mode_reports_triggered=False, but can you confirm that you do not need to disable pin_up_reports_not_triggered?

-Kevin

@fiferboy
Copy link

fiferboy commented Apr 2, 2020

Correct - this branch does not make anything worse. My working config has:

pin_up_reports_not_triggered: True
pin_up_touch_mode_reports_triggered: False

@FanDjango
Copy link
Contributor

AFAIK .550 gets rounded to .560, we are looking at 20ms increments. Tomorrow I will binary search to the best closer value than by 100ms steps.

Further testing shows me that a maximum leeway of 360ms is possible and still probe reliably with a BLTouch V3.1 on this machine (klipper and octoprint on ASUS Tinkerboard S, 32bit LPC1768 SBASE controller). This is independant on probe downward speed and lift speed. The error state experienced in this szenario occurs before any upward movement is begun.

        for retry in range(3):
            self.send_cmd(None, duration = .360)
            check_start_time = self.send_cmd('pin_up',
                                             duration=self.pin_move_time)
            check_end_time = self.send_cmd(None)

At least this confirms that there is not much time to diddle around before stowing this probe.

@FanDjango
Copy link
Contributor

@fiferboy

My clone (TriangleLab 2019 NEW 3D Touch) behaves like @FanDjango's (when issued a pin_down through Klipper and triggered it will drop back down and turn the red light on).

From your klippy.log:

BLTouch failed to verify sensor state; retrying.
BLTouch failed to verify sensor state; retrying.
BLTouch failed to verify sensor state

That's the result of setting pin_up_touch_mode_reports_triggered: True which causes the test_sensor() routine to be issued on the first deploy.

This routine takes the pin up (stow) and the sets touch mode. This probe does not then report a trigger, so I would actually surmise that it does not support "SW" touch mode at all, but who knows. Anyway, it means that SW mode with a pin-up cannot be "mis-used" to check that the probe is working. It doesn't really have any other disadvantage. This setting (to "false") just "turns off" the periodic check for functionality.

Oh, one possible disadvantage might be that if klipper ever supports using SW (optionally) for probing, that maybe that will not work either. We will see...


Sometimes when in an active probe it will do this before properly retracting the pin, but usually the retract is successful before the next probe would trigger (sometimes it isn't, though, which leads to a probing error).

I will do some more testing with my clone to see if I can discover some optimizations.

I have also been unable to get this probe to honour the "stow_on_each_sample" set to False with the recent refactor

What happens?

The best my clone ever worked was with @FanDjango's monolithic branch

We're getting there. Just need to identify some more details

@FanDjango
Copy link
Contributor

FanDjango commented Apr 2, 2020

@fiferboy

I rechecked my clone and confirm once more:

It supports pin_up_touch_mode_reports_triggered: True

BUT YOU ARE RIGHT and I never tested this before with this clone, stow_on_each_sample: false cannot be used.

Reason: It will never trigger again after the first trigger, even if the pin is down.

A very small addition to this PR will fix that. Lemme see if I can add something here...

@FanDjango
Copy link
Contributor

All I was able to do was to create PR #2678 against this branch here. I will let @KevinOConnor sort this out. But @fiferboy maybe you can look at the two very small code changes and test them? You need a new option in the config though, you can also see that in the PR #2678.

@FanDjango
Copy link
Contributor

CLONE BEHAVIOUR

Looking more closely at how the clone operates. When the pin is deployed and you trigger the probe, the red light goes on to show that the probe has been triggered. To re-arm it you need a new deploy, as stated before.

This new deploy, if the pin is already down, will do a very quick pin-up/down. Annoying but inconsequential, just costs a little time. So deploy timing on clones should be very "gracious" to make sure the probe is really deployed. A normal deploy is faster than a deployed-deploy, so to speak. So when stow_on_each_sample: False, deploy timing needs to be extended a little bit for safety, but the current timing seems to have enough leeway already.

@FanDjango
Copy link
Contributor

CLONE BEHAVIOUR cont.

Never had this with an original BLTouch:

Recv: // probe at 142.000,141.250 is z=2.511250
Recv: !! Probe samples exceed samples_tolerance
Recv: !! Probe samples exceed samples_tolerance
Recv: ok

@FanDjango
Copy link
Contributor

@KevinOConnor Ok, I have concluded this now and cannot currently find any show-stoppers in this.

Moving on to other things now. Thanks for your efforts and the new code.

Return the triggered state from verify_state() and update the callers
to raise the error (if needed).

Signed-off-by: Kevin O'Connor <[email protected]>
Simplify raise_probe() by separating out the pin_up_not_triggered
case.

Signed-off-by: Kevin O'Connor <[email protected]>
…lure

If an error is found during the pin_up_not_triggered check, then apply
the reset command for a full second (instead of just 100ms).  This
gives the bltouch more time to check its internal state.

Signed-off-by: Kevin O'Connor <[email protected]>
Be sure to fully raise the probe prior to starting any future toolhead
movements in the multi_probe_end() case.

Signed-off-by: Kevin O'Connor <[email protected]>
Some clones don't raise the pin on a reset and the ANTClabs BL-Touch
sometimes doesn't raise the pin either.

Rework the (infrequently called) sensor test code to always issue a
pin_up command before the touch command.  Also, perform a reset and
retry if the sensor test fails.

Signed-off-by: Kevin O'Connor <[email protected]>
@KevinOConnor KevinOConnor force-pushed the work-bltouch-20200329 branch from 399e227 to 5c8d15b Compare April 2, 2020 12:26
@KevinOConnor KevinOConnor merged commit 5c8d15b into master Apr 2, 2020
@github-pages github-pages bot temporarily deployed to github-pages April 2, 2020 12:27 Inactive
@KevinOConnor KevinOConnor deleted the work-bltouch-20200329 branch April 2, 2020 12:27
@KevinOConnor
Copy link
Collaborator Author

Thanks - I went ahead and merged this branch.

Further testing shows me that a maximum leeway of 360ms is possible and still probe reliably with a BLTouch V3.1 on this machine

That's odd. One way to diagnose this would be to produce an error (when using under 500ms of delay), then immediately issue a M112 command, and then attach the resulting Klipper log file here. The diagnostic info in the log contains the timing of the responses and actions.

-Kevin

@FanDjango
Copy link
Contributor

I can do that next week, I'll put it on my schedule.

@github-actions github-actions bot locked and limited conversation to collaborators Oct 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants