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

BLTouch::z_extra_clearance() use probe offset instead of full travel #25655

Conversation

dfries
Copy link
Contributor

@dfries dfries commented Apr 8, 2023

Description

Using the full probe stroke defeats some of the speed advantage for using high speed mode, only use full probe stroke if the nozzle offset isn't available.

Requirements

BLTouch

Benefits

I developed this while probe.cpp still used z_extra_clearance() , now that probe.cpp doesn't use it, this clearance will not be used nearly as often. Traveling the full stroke vs much less isn't going to have as much impact on a print job.

@dfries dfries force-pushed the bugfix-2.1.x_BLTouch_z_extra_clearance branch 2 times, most recently from b1e87b8 to 4aa3ff2 Compare April 8, 2023 17:33
Using the full probe stroke defeats some of the speed advantage for
using high speed mode, only use full probe stroke if the nozzle offset
isn't available.
@dfries dfries force-pushed the bugfix-2.1.x_BLTouch_z_extra_clearance branch from 4aa3ff2 to 9e273aa Compare April 9, 2023 01:52
@thinkyhead thinkyhead force-pushed the bugfix-2.1.x_BLTouch_z_extra_clearance branch from 07baf9b to 4376091 Compare April 10, 2023 08:24
@thinkyhead
Copy link
Member

The reason we avoid using probe.offset.z in determining proper clearance is that this value is not always meaningful. The user may or may not have their probe.offset.z configured, especially on first-time setup. With a BLTouch we can require it to be a negative value, but even then, the user might still not have it right. If we provide a default value of "3" for Z_CLEARANCE_BLTOUCH_HS that will likely end up breaking some probes. We need to make a positive impression, and the extra 7mm is most guaranteed to be safe.

Meanwhile, I have been thinking of moving the "extra probe offset" to the Probe class so that it can be generalized to more kinds of probes. So maybe we can do that here, or over in #25631 where we've been working out the details for the past week.

thinkyhead added a commit to MarlinFirmware/Configurations that referenced this pull request Apr 10, 2023
@thinkyhead
Copy link
Member

I've warmed up to this idea, but I'm choosing to set the default to a very conservative value. Users may choose to change it for their own setups, and we can certainly pick better defaults for existing example configurations. As always, our main concern is not to break anyone's probes, so I think this is a good compromise.

@thinkyhead
Copy link
Member

Another thing occurs to me in the moment: We could make 7 the maximum value that gets returned by z_extra_clearance since anything over that is probably pointless.

thinkyhead added a commit to MarlinFirmware/Configurations that referenced this pull request Apr 10, 2023
@thinkyhead thinkyhead force-pushed the bugfix-2.1.x_BLTouch_z_extra_clearance branch from cb3d77a to 08069a6 Compare April 10, 2023 23:01
thinkyhead added a commit to MarlinFirmware/Configurations that referenced this pull request Apr 11, 2023
@dfries
Copy link
Contributor Author

dfries commented Apr 11, 2023

I have reviewed these changes and they look good to me.
Right now 7mm is specific to BLTouch (and hopefully any clones), moving it to probe.cpp would need the maximum stroke specified. Would it be better to setup that up now, or leave it BLTouch specific?

@thinkyhead
Copy link
Member

Would it be better to setup that up now, or leave it BLTouch specific?

For now I think it's fine to keep it specific to the BLTouch.

I'm running through G28 and G29 at the moment, testing the changes from #25631 where I've changed the code to apply any negative probe.offset.z to all clearance moves, since that is how they are documented in the configuration. I'm using a basic bed-slinger with a BLTouch and tuning the behavior to my preferences and to get it as fast as possible.

There are one or two other things I'd like to adjust, but those can wait. Specifically, when homing XY for the first time, with Z being unknown, there is an initial raise to ensure clearance. Then before the probe deploys for Z-homing there is another raise because Z is still not considered to be known. Since there was already one raise, if that raise was sufficient, it would be nice to be able to skip that second raise for probe deployment. I'm contemplating a clean way to handle that, without setting the "Z known" flag, since that flag is still useful. What I might do is set it "known" just before attempting Z homing, if the previous raise was done, and just always set it to "unknown" if Z homing fails. We'll see how that goes….

@dfries
Copy link
Contributor Author

dfries commented Apr 11, 2023

Here's my branch with all my bed probing related changes. The parts you've seen are out of date relative to your changes. I'm not making a pull request for the whole yet, because the later ones still need work, I just thought I'd push up my work and give an overview of my pain points and where I had been going with the changes I was submitting. Pain points of making up for a bad printer hardware design delivering 3.3V to a 5V BLTouch (probe_at_point detecting and retrying bad probes), using G30 to measure the height of a reasonably tall model to resume, G28 H Z home Z where there's an opening, and speeding up G29 when I'm still not sure why bugfix-2.1.x was taking so much longer.
https://github.com/dfries/Marlin/tree/bugfix-2.1.x_adaptive_probing

I had problems from the start with the BLTouch triggering early, not every 5x5 auto bed level, but I couldn't trust it. The "adaptive probing" helped by probing twice, comparing the value, if it was out of tolerance of 0.03mm, I would have it keep probing until it either got consistent numbers or if reached a limit and threw a firmware failure. I'm also using OctoPrint with a custom plugin to check the auto bed level numbers, and abort the job if any is 0.1 mm or greater from the average. This has also been useful when detecting if I forgot to clear the previous model (if it is small enough). Creality Ender 6 has a breakout board that the BLTouch is connected to, that board was giving it 3.3V, connecting the BLTouch directly to the main board with a longer shielded cable has resolved the triggered early while the probe was vary far away from the board, I'll still see repeated probes at a point, but it is much better.

I see other probe and compare algorithms will probe a fixed number of times, compare, if they are good go on, if they aren't close enough, it starts over at that location. I'm considering going to that algorithm, it should simplify the logic. It is possible that the current algorithm throws away the wrong value if enough bad values align and it fails when it shouldn't. I wrote it when I my bad values were way off. That's probably not typical.

The other theme of my changes is in recovering a failed print. G28 Z with a stock 2.0.9.3 firmware will Home Z with the probe at the center of the build plate, right where a model is likely to be. G28 H Z was very useful to Home wherever I could find a spot. G30 with a stock 2.0.9.3 would fail if the model was taller than roughly 6 mm. A change I hadn't included in pull request is "G30 disable probe sanity_check" pass /sanity_check/ false to probe_at_point. I assume G30 in this context would be executed by a user or by something that would be reading the output. That it isn't close to the build plate doesn't matter.
A height was asked for and a height can be provided. A subset of the changes on that branch are making it so the clearance is turned into current_position.z + probe offset + clearance. The config file I have says probe offset is added, I see you've mentioned that. I guess I'm not sure when exactly the current_position.z needs to be included so that G30 when used for a tall model will be able to raise for the next probe and at the end. That's the context for using current_position.z when doing a clearance. I would expect a good Home Z before starting to measuring a model.

Then there was finding my 5x5 auto bed probe time dramatically increased from 2.0.9.3 to bugfix-2.1.x branch for me went from 3:19 to 4:34, and that's where the bltouch avoid sending the same deploy() or stow() came from you've merged. There's another change you haven't seen "Add a new Z_PROBE_FEEDRATE_RAISE", I'm assuming the fast rate and slow rate when probing down are less than the printer's maximum rate because a slower rate means more time to react to the probe trigger and more time to stop and get a better reading. That doesn't apply when moving away from the board, because it isn't probing and the planner will know ahead of time where to stop. The default maximum for my printer is 10mm/s for Z so that's what I'm using.

You mentioned other ways of probing faster. Here's another one "Decrease the total time by moving Z at the same time as XY" I'm not really expecting that one to go in, but it does cut down the probe time. Currently when the last sample at a location is finished, Z goes all the way up, moves XY, then starts the next probe. With this change Z goes up enough to clear the build plate and a little more, then diagonally moves X/Y/Z to the next point. In this case most of the time for Z up is hidden in the XY movement. End result 2.0.9.3 3:19, bugfixes-2.1 + all changes 1:07, that's with 150mm/s XY, 10mm/s Z.

@thinkyhead thinkyhead merged commit c6e5648 into MarlinFirmware:bugfix-2.1.x Apr 11, 2023
mikezs added a commit to mikezs/Marlin that referenced this pull request Apr 12, 2023
* bugfix-2.1.x: (37 commits)
  🚸 Minor M43 improvements
  🐛 Fix BLTOUCH_HS_MODE config
  [cron] Bump distribution date (2023-04-12)
  🚸 BLTouch extra clearance for PROBE_PT_RAISE
  🚸 More clearance on fast probe failure
  🧑‍💻 Use largest_sensorless_adj in DELTA run_z_probe
  🧑‍💻 Update move_z_after_probing/homing
  🧑‍💻 Modify try_to_probe sanity-checking
  🧑‍💻 Clarify G28 R / R0
  🧑‍💻 Probe flag in do_z_clearance
  🧑‍💻 More debug in motion.*
  📝 Improve G30 description
  ✨ BLTOUCH_HS_EXTRA_CLEARANCE (MarlinFirmware#25655)
  [cron] Bump distribution date (2023-04-11)
  📝 Describe G34, spellcheck
  🩹 Fix BLTouch stow in homeaxis(Z)
  🚸 Fix G30 behavior
  🎨 Misc. probe-related cleanup
  [cron] Bump distribution date (2023-04-10)
  🔧 Update thermocouple 2 pin sanity check (MarlinFirmware#25627)
  ...

# Conflicts:
#	.github/workflows/bump-date.yml
#	Marlin/Configuration.h
#	Marlin/Configuration_adv.h
@GMagician
Copy link
Contributor

GMagician commented Apr 16, 2023

Not sure this PR is the cause but I got a G34 failure because now probe doesn't raise enough to let bltouch do a proper deploy. I try do step back and do a better check

@thisiskeithb
Copy link
Member

Not sure this PR is the cause but I got a G34 failure because now probe doesn't raise enough to let bltouch do a proper deploy. I try do step back and do a better check

See #25631. It started out as a bunch of probe/homing fixes and is now just G34 fixes.

Thinkyhead still needs to push some more changes before it's merged.

@GMagician
Copy link
Contributor

@thisiskeithb yep, I think you got it. Description seems talking about my issue. Thanks

EvilGremlin pushed a commit to EvilGremlin/Marlin that referenced this pull request May 8, 2023
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
EvilGremlin pushed a commit to EvilGremlin/Marlin that referenced this pull request May 17, 2023
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
tspiva pushed a commit to tspiva/Marlin that referenced this pull request May 25, 2023
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
Andy-Big pushed a commit to Andy-Big/Marlin_FB_Reborn that referenced this pull request Jul 11, 2023
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
EvilGremlin pushed a commit to EvilGremlin/Marlin that referenced this pull request Oct 27, 2023
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants