BLTOUCH_HS_MODE Fix for ASSISTED_TRAMMING_WIZARD

[qwewer0]

Description

Right now with BLTOUCH_HS_MODE, BLTouch push-pin stays deployed after each measured point, so when adjusting the bed we might accidently move our hand or something under the nozzle and hit the push-pin braking it, and even after the tramming wizard procedure is over the push-pin still stays deployed, which is obviously a bad idea.

ASSISTED_TRAMMING_WIZARD stow after each measurement with BLTOUCH_HS_MODE.

Benefits

BLTouch push-pin safety for BLTOUCH_HS_MODE.

Related Issues

Fixes #20059

[thisiskeithb]

The purpose of BLTOUCH_HS_MODE is to not stow the pin. Perhaps a sanity check should be added instead to alert the user to disable BLTOUCH_HS_MODE when used with ASSISTED_TRAMMING_WIZARD.

We're also already raising Z (comments right before the changed code):

// Users of Tramming Wizard might have a badly misaligned bed, so raise Z by the
// length of the deployed pin (BLTOUCH stroke < 7mm)

[qwewer0]

@thisiskeithb On ASSISTED_TRAMMING_WIZARD, we only do one measurement (manually) at a time, so I think a stow is appropriate.

[qwewer0]

We're also already raising Z (comments right before the changed code):

// Users of Tramming Wizard might have a badly misaligned bed, so raise Z by the
// length of the deployed pin (BLTOUCH stroke < 7mm)

Ok, I still think a stow is needed for ASSISTED_TRAMMING_WIZARD with BLTOUCH_HS_MODE, but then this needs more work.

[sjasonsmith]

Perhaps a sanity check should be added instead to alert the user to disable BLTOUCH_HS_MODE when used with ASSISTED_TRAMMING_WIZARD.

I don't think enabling ASSISTED_TRAMMING_WIZARD should prevent somebody from using BLTOUCH_HS_MODE when they want to probe their entire bed. Please don't solve this with a sanity check.

I think stowing a BLTouch is fine any time the wizard stops for user interaction. HS_MODE is there to avoid slowdowns when doing many probes in sequence.

[thisiskeithb]

I'd recommend increasing the existing Z raise if damaging your probe is a concern.

[qwewer0]

I'd recommend increasing the existing Z raise if damaging your probe is a concern.

My concern isn't that the printer would damage the push-pin, but rather that right now it stays deployed after each measured point and when adjusting the bed we might accidently move our hand or something under the nozzle and hit the push-pin braking it, and even after the procedure is over the push-pin stays deployed, which is obviously a bad idea.

[sjasonsmith]

@qwewer0 feel free to re-open this. I think you should update the comment above the line you modified about High-speed, though.

[qwewer0]

@sjasonsmith Got it.

[sjasonsmith]

Can you clarify one thing for me? I've never used G35, so I don't know the precise workflow.

Does it always perform a single probe then wait for the user? It does not probe all the corners one after the other?

[qwewer0]

Can you clarify one thing for me? I've never used G35, so I don't know the precise workflow.

Does it always perform a single probe then wait for the user? It does not probe all the corners one after the other?

@sjasonsmith This fix is only for ASSISTED_TRAMMING_WIZARD, which is only an LCD function, and that only probes a specific point if the user selects that point then hit "measure", after that, the printer waits for the user. G35 works like it did before probing all the points in one go, without stowing on BLTOUCH_HS_MODE.

[sjasonsmith]

Lol, I'm getting confused by all these different mechanisms that serve very similar purposes. Thanks for the description.

[sjasonsmith]

Oops, it looks like I broke CI with a recent merge. Ignore this from your PR, I'll get it fixed.

[sjasonsmith]

Can you add a call to probe.stow() any time the wizard exits? Your solution probably works with a BLTouch, since it auto-retracts after each probe. Other types of probes such as a "normal" servo probe will still be left deployed.

[qwewer0]

Can you add a call to probe.stow() any time the wizard exits? Your solution probably works with a BLTouch, since it auto-retracts after each probe. Other types of probes such as a "normal" servo probe will still be left deployed.

Then wouldn't there be a problem in between probes or PROBE_PT_STOW is enough for that?

[FanDjango]

BLTouch does not "auto-retract" after a probe. Not in SW-Mode and not in normal mode. It is us that retract the probe. I would "add a call to probe.stow() any time the wizard exits" as @sjasonsmith says. You can use PROBE_PT_STOW but then you won't get the PROBE_PT_RAISE that you probably want.

[sjasonsmith]

Some probes are manually or otherwise slow to deploy/stow. You should probably leave it deployed until they are completely done with it.

[sjasonsmith]

BLTouch does not "auto-retract" after a probe. Not in SW-Mode and not in normal mode. It is us that retract the probe. I would "add a call to probe.stow() any time the wizard exits" as @sjasonsmith says. You can use PROBE_PT_STOW but then you won't get the PROBE_PT_RAISE that you probably want.

So now with BLTOUCH_HS_MODE disabled, is this going to leave the pin deployed during travel? Do we need to always use PROBE_PT_STOW if using a BLTOUCH, regardless of HS mode?

[FanDjango]

Regardless of probe type or 'BLTOUCH_HS_MODE', you are responsible to issue the last call to probe_at_point(...) that you do, with a PROBE_PT_STOW, otherwise the probe (whatever kind) will not be stowed.

Trouble is, this PROBE_PT_STOW means you don't get a PROBE_PT_RAISE, thus all coders in the past have preferred to do the PROBE_PT__RAISE, probe the next location etc. etc. and finally do a STOW. They do that with a call to probe.stow() instead of coding logic to issue the last call to probe_at_point(...) with PROBE_PT_STOW as they typically would then want a separate Z-RAISE to be safe. And z_raise(...) is actually private in probe.cpp, adding to the problem.

This relates to G29, M48, G34, G35, PROBE_OFFSET_WIZARD...

Just my 2 cents on this, sorry.

[qwewer0]

By my test, without BLTOUCH_HS_MODE in SW-Mode the push-pin stow after each measurement, so I have no idea...

[FanDjango]

You are right. I just checked. It actually concerns only BLTouch and HS_MODE.

[FanDjango]

So my above statement is true only in the case of BTLOUCH combined with HS_MODE.

These final calls to probe.stow() all over the place are needed because probe_at_point(...) doesn't do the final STOW in BLTOUCH_HS_MODE, how should it know? It relies on the callers to do a final probe_at_point(...) with `'PROBE_PT_STOW', which they do not do because with other probes it is already stowed, so a BLTOUCH in HS_MODE would remain deployed.

[FanDjango]

Instead of unqualified probe.stow() operations, you would do:

#if ENABLED(BLTOUCH_HS_MODE)
  probe.stow()
#endif

Gonna test that in G34...

[FanDjango]

I'm testing yours right now, gimme a moment

[FanDjango]

Ok, looks much nicer now when the movement to and from the corners with the center is done with the pin of the probe up, even with 'BLTOUCH_HS_MODE'.

I like the TERN code - you are using PROBE_PT_STOW as it was intended. Now though there is less potential RAISE after the probe, but it looks like it should be enough because now the pin is raised.

Really sweet, movement looks more professional this way. No dangling pin, very little loss of responsiveness.

I wonder if now the

// In BLTOUCH HS mode, the probe travels in a deployed state.
  // Users of Tramming Wizard might have a badly misaligned bed, so raise Z by the
  // length of the deployed pin (BLTOUCH stroke < 7mm)
  do_blocking_move_to_z((Z_CLEARANCE_BETWEEN_PROBES) + TERN0(BLTOUCH_HS_MODE, 7));

could be reduced again?

[qwewer0]

// In BLTOUCH HS mode, the probe travels in a deployed state.
  // Users of Tramming Wizard might have a badly misaligned bed, so raise Z by the
  // length of the deployed pin (BLTOUCH stroke < 7mm)
  do_blocking_move_to_z((Z_CLEARANCE_BETWEEN_PROBES) + TERN0(BLTOUCH_HS_MODE, 7));

could be reduced again?

Not sure what you mean, it is now only do_blocking_move_to_z(Z_CLEARANCE_DEPLOY_PROBE);

Or did you meant it in G35? That it still better without stow so it needs some extra raise for BLTOUCH_HS_MODE.

[FanDjango]

Hmm. I will check. Yup. I was in G35. Disregard all the rest...

[FanDjango]

Is that STOW in DONE still needed, now that all probe operations even in BLTOUCH_HS_MODE do STOW always?
Because I haven't put that in and it works fine.

[qwewer0]

Is that STOW in DONE still needed, now that all probe operations even in BLTOUCH_HS_MODE do STOW always?
Because I haven't put that in and it works fine.

For BLTouch it doesn't need probe.stow() at the end, but @sjasonsmith wrote this:

Can you add a call to probe.stow() any time the wizard exits? Your solution probably works with a BLTouch, since it auto-retracts after each probe. Other types of probes such as a "normal" servo probe will still be left deployed.

[FanDjango]

And this one:

const float z_probed_height = probe.probe_at_point(screws_tilt_adjust_pos[tram_index], TERN(BLTOUCH_HS_MODE, PROBE_PT_STOW, PROBE_PT_RAISE), 0, true);

keeps "normal" probes deployed between moves to save time. Ok, I see.

[FanDjango]

Been playing with it and looks really good now

[sjasonsmith]

This looks good to me. I'll let it sit here overnight in case people have anything to add, and come back to it in the morning.

[qwewer0]

Might be a good idea to wait for #20439

[FanDjango]

@qwewer0 Why?

[qwewer0]

@FanDjango Maybe PROBE_PT_STOW_AND_RAISE would be fitting here too?

Marlin/Marlin/src/lcd/menu/menu_tramming.cpp

Line 48 in d63d22b

const float z_probed_height = probe.probe_at_point(screws_tilt_adjust_pos[tram_index], TERN(BLTOUCH_HS_MODE, PROBE_PT_STOW, PROBE_PT_RAISE), 0, true);

[FanDjango]

Maybe it would be, but PROBE_PT_STOW_AND_RAISE not in #20439. It was in there by mistake, I removed it.

I am playing with PROBE_PT_STOW_AND_RAISE on my test system. It got in there by mistake.

It should be in a separate PR, but I am probably not going to do it. Just proposing it...

[qwewer0]

I just saw it, that PROBE_PT_STOW_AND_RAISE isn't in that PR. 😋