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.

Sign up for GitHub

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

Jump to bottom

Probe offset wizard: STOW probe and loosen endstops when using PROBE_OFFSET_WIZARD_XY_POS #20414

Merged
merged 2 commits into from
Dec 11, 2020
Merged

Probe offset wizard: STOW probe and loosen endstops when using PROBE_OFFSET_WIZARD_XY_POS #20414

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f155a82
Update menu_probe_offset.cpp
FanDjango Dec 9, 2020
b4b742a
Remove redundant endstop loosener
FanDjango Dec 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions Marlin/src/lcd/menu/menu_probe_offset.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,11 +133,13 @@ void prepare_for_probe_offset_wizard() {

// Probe for Z reference
ui.wait_for_move = true;
z_offset_ref = probe.probe_at_point(wizard_pos, PROBE_PT_RAISE, 0, true);
z_offset_ref = probe.probe_at_point(wizard_pos, PROBE_PT_STOW, 0, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PullRequest #20344 was in draft when merged. So i think it was not tested.

The Idea of PROBE_PT_RAISE was, that there should be a z rise after probing. I didn't realize that PROBE_PT_RAISE excludes a STOW (but it's logic 🙃 ).

I think we should add the following after probing:

// Rise Nozzle after probing for probes, they don't rise when stowing.
#if EITHER(FIX_MOUNTED_PROBE, NOZZLE_AS_PROBE)
  do_z_raise(_MAX(Z_CLEARANCE_BETWEEN_PROBES, Z_CLEARANCE_DEPLOY_PROBE));
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too bad there is no PROBE_PT_RAISE_STOW as a combination.

I wonder what the tradeoff is in terms of code bytes to create that in probe.cpp (couple of bytes only I think). But that code increase would hit everyone that has a probe, and in here it only hits those that activate this functionality. Your thoughts?

Also I don't really want to mess around with probe.cpp any more in this lifetime.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Z_CLEARANCE_DEPLOY_PROBE will be used along with PROBE_PT_RAISE_STOW.

Other commands that probe multiple times/points only stow the probe at the end of the entire sequence. They only raise between points. They then provide an argument such as G29 E if your probe needs to stow after each probe. This is useful for probes such as solenoids which may overheat if deployed for too long.

Some probes require a slow or manual deploy/stow process, so you don't want to actually deploy/stow multiple times.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is an ideal though. This PR doesn't have to wait for a larger rework.

Copy link
Contributor

@sjasonsmith sjasonsmith Dec 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, I was confusing this with G35. Always looking at code out of context 😉

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add the following after probing:

@swissnorp I think that capability could make sense. I don't know if it belongs as part of the probe code itself or simply as a move at the end of the wizard. I can imagine there will be someone annoyed and reporting bugs if it were made the behavior after every probe.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too bad there is no PROBE_PT_RAISE_STOW as a combination.

I wonder what the tradeoff is in terms of code bytes to create that in probe.cpp (couple of bytes only I think). But that code increase would hit everyone that has a probe, and in here it only hits those that activate this functionality. Your thoughts?

Also I don't really want to mess around with probe.cpp any more in this lifetime.

@FanDjango I was thinking about the same even before you had posted it. 😄
But I finaly think its not needed.
I realy like the solution you provided in #20439

I think we should add the following after probing:

@swissnorp I think that capability could make sense. I don't know if it belongs as part of the probe code itself or simply as a move at the end of the wizard. I can imagine there will be someone annoyed and reporting bugs if it were made the behavior after every probe.

@sjasonsmith I didn't realize that it was a private function and do_z_clearance would be a better solution because do_z_raise does lift the nozzle even higher with a negative probe_offset.z. But I think #20439 is the way to go!

Marlin/Marlin/src/module/probe.cpp

Lines 259 to 267 in e817773

/**
* Raise Z to a minimum height to make room for a probe to move
*/
void Probe::do_z_raise(const float z_raise) {
if (DEBUGGING(LEVELING)) DEBUG_ECHOLNPAIR("Probe::do_z_raise(", z_raise, ")");
float z_dest = z_raise;
if (offset.z < 0) z_dest -= offset.z;
do_z_clearance(z_dest);
}

ui.wait_for_move = false;

#endif

SET_SOFT_ENDSTOP_LOOSE(true); // Disable soft endstops for free Z movement

sjasonsmith marked this conversation as resolved.
Show resolved Hide resolved
// Move Nozzle to Probing/Homing Position
ui.wait_for_move = true;
current_position += probe.offset_xy;
Expand Down Expand Up @@ -173,7 +175,6 @@ void goto_probe_offset_wizard() {
ui.goto_screen([]{
_lcd_draw_homing();
if (all_axes_homed()) {
SET_SOFT_ENDSTOP_LOOSE(true); // Disable soft endstops for free Z movement
z_offset_ref = 0; // Set Z Value for Wizard Position to 0
ui.goto_screen(prepare_for_probe_offset_wizard);
ui.defer_status_screen();
Expand Down