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

Unify run_z_probe #4343

Closed
wants to merge 1 commit into from
Closed

Conversation

AnHardt
Copy link
Contributor

@AnHardt AnHardt commented Jul 18, 2016

Unify run_z_probe
Implicates double touch for DELTAs when probing.
Introduce Z_PROBE_SPEED_FAST and Z_PROBE_SPEED_SLOW,
defaulting to homing_feedrate_mm_m[Z_AXIS] and homing_feedrate_mm_m[Z_AXIS]/2
#4261

@jbrazio jbrazio added this to the 1.1.0 milestone Jul 19, 2016
@@ -2475,7 +2424,7 @@ static void homeaxis(AxisEnum axis) {
DEBUG_POS("", current_position);
}
#endif
do_blocking_move_to_axis_pos(axis, endstop_adj[axis], set_homing_bump_feedrate(axis));
do_blocking_move_to_axis_pos(axis, endstop_adj[axis], get_homing_bump_feedrate(axis));
Copy link
Member

Choose a reason for hiding this comment

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

Looks like now this should be…

line_to_axis_pos(axis, endstop_adj[axis], get_homing_bump_feedrate(axis));

@thinkyhead thinkyhead mentioned this pull request Jul 19, 2016
@thinkyhead
Copy link
Member

thinkyhead commented Jul 19, 2016

I rebased this on the latest code, then decided to make another tweak. Namely, it seems best not to include homing_feedrate_mm_m[Z_AXIS] as a default configuration option value, but to use a "factor" instead, which is multiplied by the homing feedrate.

// Proportion of homing speed for the first approach when probing
#define Z_PROBE_FACTOR_FAST 1
// Proportion of homing speed for the second approach when probing
#define Z_PROBE_FACTOR_SLOW 0.5

@AnHardt AnHardt force-pushed the delta-double-touch branch from 5a65c15 to 8d0255c Compare July 19, 2016 23:34
@AnHardt
Copy link
Contributor Author

AnHardt commented Jul 19, 2016

Changing to factor here would (for me) be repeating the same error.
With absolute speeds we have the full flexibility to use a factor, in the config., or not.
I admit using a variable here is a somewhat unusual construct. Do you have a better idea to get homing_feedrate_mm_m[Z_AXIS] into the config. (array defined)

Why homing_feedrate_mm_m at all? To give a speed that should work for most systems. But in principe they should be independent. At least that was my takeaway from #3975.

@AnHardt AnHardt force-pushed the delta-double-touch branch from 8d0255c to 2d8eeb2 Compare July 19, 2016 23:48
@thinkyhead
Copy link
Member

thinkyhead commented Jul 19, 2016

I understand the point. But we cannot have (any more) configuration options that use named variables from the source code. So it has to be an absolute value or a factor related to some other reference speed.

Too bad the feedrates aren't all specified in one place.
Meanwhile, we could split them up a bit, like so…

#define HOMING_FEEDRATE_XY 50*60
#define HOMING_FEEDRATE_Z  4*60
#define HOMING_FEEDRATE {HOMING_FEEDRATE_XY, HOMING_FEEDRATE_XY, HOMING_FEEDRATE_Z, 0}  // set the homing speeds (mm/min)

. . .

#define Z_PROBE_SPEED_FAST HOMING_FEEDRATE_Z
#define Z_PROBE_SPEED_SLOW HOMING_FEEDRATE_Z / 2

Unify run_z_probe
Add double touch for DELTAs.
Introduce Z_PROBE_SPEED_FAST and Z_PROBE_SPEED_SLOW
  defaulting to homing_feedrate_mm_m[Z_AXIS] and homing_feedrate_mm_m[Z_AXIS]/2
@AnHardt
Copy link
Contributor Author

AnHardt commented Jul 20, 2016

Looks good to me. But i already hear Roxy moaning about to big changes in the configs.

The structure and order of the configs needs some more discussion, i think.
At the moment, for me, Configuration.h looks like after an earthquake.
With too much white space. Too long. Not in the order i would configure it for a new machine. ...
This should have a # in issues.

@thinkyhead
Copy link
Member

Configuration.h looks like after an earthquake

Too many options? All out of order? Organization would be good!

@thinkyhead
Copy link
Member

Ready to merge #4356

@thinkyhead thinkyhead closed this Jul 20, 2016
@AnHardt AnHardt deleted the delta-double-touch branch July 20, 2016 12:12
drewmoseley pushed a commit to drewmoseley/Marlin that referenced this pull request Feb 10, 2024
…lations

Update Version and Translations for 3.13.1
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.

3 participants