[1.1.x] PROBE_SELECTED etc. #8325

LVD-AC · 2017-11-08T09:52:56Z

also making G33 LCD independent from 3-point calibration menu

thinkyhead

Fix menu items and all is good!

thinkyhead · 2017-11-09T03:02:01Z

Marlin/ultralcd.cpp

@@ -2762,9 +2765,14 @@ void kill_screen(const char* lcd_msg) {
      MENU_ITEM_EDIT(float43, "Tx", &delta_tower_angle_trim[A_AXIS], -5.0, 5.0);
      MENU_ITEM_EDIT(float43, "Ty", &delta_tower_angle_trim[B_AXIS], -5.0, 5.0);
      MENU_ITEM_EDIT(float43, "Tz", &delta_tower_angle_trim[C_AXIS], -5.0, 5.0);
+      recalc_delta_settings(delta_radius, delta_diagonal_rod, delta_tower_angle_trim);


The change pointed out in #8324 is also needed here.

LVD-AC · 2017-11-09T10:59:31Z

Done

LVD-AC · 2017-11-09T19:01:41Z

"1st probe weird behavior" deals with #8298 and #8344

for one reason or another

probe_pt(dx, dy, stow_after_each, 1, false)

does not stop at z=-10

probe_pt(any_other_x, any_other_y, stow_after_each, 1, false)
probe_pt(dx, dy, stow_after_each, 1, true)

do stop as expected.

thinkyhead · 2017-11-10T04:23:35Z

for one reason or another…
probe_pt(dx, dy, stow_after_each, 1, true)

So the last argument to probe_pt (printable) being true causes a change in probe_pt behavior?

probe_pt(any_other_x, any_other_y, stow_after_each, 1, false);

And the same behavior change is seen when an XY move is needed, but not if the probe is to occur at the current position?

Interesting…

thinkyhead · 2017-11-10T04:33:33Z

If the printable parameter is causing a change in behavior, it will perhaps be due to the probe offset being used to determine whether a point is reachable. If the intention is to move the nozzle to the point, then printable parameter must be true. If the intent is to move the probe to the point, then printable should be false.

~~If you ask me, the logic in probe_pt appears to be incorrect. It currently reads:~~

  const float nx = rx - (X_PROBE_OFFSET_FROM_EXTRUDER), ny = ry - (Y_PROBE_OFFSET_FROM_EXTRUDER);

  if (printable
    ? !position_is_reachable(nx, ny)
    : !position_is_reachable_by_probe(rx, ry)
  ) return NAN;

~~I think it should be:~~

  if (printable
    ? !position_is_reachable(rx, ry)
    : !position_is_reachable_by_probe(rx, ry)
  ) return NAN;

  const float nx = rx - (X_PROBE_OFFSET_FROM_EXTRUDER), ny = ry - (Y_PROBE_OFFSET_FROM_EXTRUDER);

EDIT: Never mind the change. I see the intent is to only get a simpler bounds check.

~~Again: only if the intent is to probe specifically with the nozzle at the given position.~~ On the bigger scale, nothing in Marlin calls probe_pt with printable true except G33, so if it doesn't need to probe with the nozzle then the printable parameter can be removed, and probe_pt can always probe at the given probe position.

thinkyhead · 2017-11-10T05:01:32Z

So the printable parameter:

Is true by default, because that's what we usually want.
- Causes probe_pt to only check whether the probe can reach the point, and to ignore whether the nozzle can reach the point (position_is_reachable).
Is false only for G33.
- Causes probe_pt to check whether both the probe and nozzle can reach the point (position_is_reachable_by_probe).
- Causes software endstops to be disabled for the duration of the Z probe.

The value of printable is passed to run_z_probe as the short_move parameter.

A true value causes the second probe move to be short — no lower than -10.
A false value causes a long second probe move — the full height of the machine plus 10mm.

Do with that information what you will! If you want short moves from run_z_probe then you'd have to pass true to probe_pt, which means you have to use position_is_reachable to test whether the probe point is reachable.

Since G33 is the only function using the printable parameter, you can completely alter what it does and whether it is passed to run_z_probe for short_move. Every other use of probe_pt does the short move, so I would recommend the following changes:

Dump the short_move parameter to run_z_probe and always do the short move.
Use the printable parameter however fits G33 — probably continue to pass false.

thinkyhead · 2017-11-10T06:57:26Z

Additional changes:

Make recalc_delta_settings take no arguments instead of calling with the same globals.
Replace menu item lcd_recalc_delta_settings calls with recalc_delta_settings.
Fix a subtle bug in position_is_reachable_by_probe.
~~Don't require re-homing after recalc_delta_settings since steppers are known.~~
Make Temperature::updatePID an inline method.
Add comment to Temperature::PID_autotune mentioning M303.

LVD-AC · 2017-11-10T08:08:59Z

The printable parameter was introduced because of the testing if the position is reachable for eccentric probes was to restrictive. G33 probes at nozzle position; all other probe moves in Marlin probe at bed positions. This is basically the difference between calibration where one wants to know the z-height for the delta mechanism of carriages, rods and effector to be at a specific spot; and bed levelling where one wants to know the z-height at certain bed coordinates.

if (printable
   ? !position_is_reachable(nx, ny)
   : !position_is_reachable_by_probe(rx, ry)
 ) return NAN;

is the only thing it should do

Causes software endstops to be disabled for the duration of the Z probe.
The value of  printable  is passed to  run_z_probe  as the  short_move  parameter.

I didn't do that, and I can see no reason for it. CMIIW but do software endstops only limit moves of the nozzle, not the probe? In that case it is obsolete. And measured_z = run_z_probe(printable); is definitely wrong.

LVD-AC · 2017-11-10T08:14:42Z

•Don't require re-homing after recalc_delta_settings since steppers are known.

If this works, I could get rid of all the homing in between iterations for G33 which would speed up the procedure considerably. I'll give it a test...

thinkyhead · 2017-11-10T10:24:05Z

The printable parameter was introduced because of the testing if the position is reachable for eccentric probes was to restrictive.

Ok, I comprehend the reasoning. It does the right test for the situation. So, this all seems to be pretty good now!

I will await the results of your testing of the updated recalc_delta_settings before going forward with merge.

LVD-AC · 2017-11-10T15:23:05Z

Simulated an eccentric probe and with the latest commit all seems to work fine in G33 and G29;

need to do some more testing on the "homing" issue

LVD-AC · 2017-11-10T15:25:02Z

Marlin/Marlin_main.cpp

@@ -2412,7 +2412,7 @@ static void clean_up_after_endstop_or_probe_move() {

    const float nx = rx - (X_PROBE_OFFSET_FROM_EXTRUDER), ny = ry - (Y_PROBE_OFFSET_FROM_EXTRUDER);

-    if (printable
+    if (!printable


Strange nobody with an eccentric probe noticed this before: with G33 probe (printable = false) check if nozzle is reachable : default use the more restrictive probe is reachable check !!!

Argh, yeah that was me. I must have been doing too much crack in August. 0938c62

Thanks for this correction!

LVD-AC · 2017-11-10T15:27:47Z

Marlin/Marlin_main.cpp

@@ -5590,7 +5590,7 @@ void home_all_axes() { gcode_G28(true); }
                      r = delta_calibration_radius * 0.1;
          z_at_pt[CEN] +=
            #if HAS_BED_PROBE
-              probe_pt(cos(a) * r + dx, sin(a) * r + dy, stow_after_each, 1)
+              probe_pt(cos(a) * r + dx, sin(a) * r + dy, stow_after_each, 1, false)


printable in G33 should always be false

center probe probe_pt(dx, dy, stow_after_each, 1, false) no longer behaves weird; so that one is solved.

Is there any reason you can think of with this flag set to false why we wouldn't do the short_move probe in run_z_probe? That changes was introduced by my spirit animal @tcm0116 in f54e0fc#diff-1cb08de130a6ece2d1b5b9c37bcfef48R2384.

The flag just skips the test that the probe as well should stay within the printable area; it should do nothing else.

LVD-AC · 2017-11-10T16:01:50Z

Homing is required if one of delta parameters is changed; the @thinkyhead approach does not set the new coordinates right to what they should be referenced to the home position. e.g. a height change or end-stop change does not seem to affect the new coordinates, and a delta radius change seems to introduce a false height offset. So rolling back on this one...

LVD-AC · 2017-11-10T16:59:29Z

Did some more tests and all seems well now ... (ready for merge)

LVD-AC · 2017-11-10T17:38:35Z

I did some archive checks: in Marlin 1.1.4 it was still the original

   if (printable)
      if (!position_is_reachable_by_probe_xy(lx, ly)) return NAN;
    else
      if (!position_is_reachable_xy(nx, ny)) return NAN;

In 1.1.5 this was changed to (inverted logic):

    if (printable
      ? !position_is_reachable_xy(nx, ny)
      : !position_is_reachable_by_probe_xy(lx, ly)
    ) return NAN;

The same happened to the (printable = false) argument in the G33 probe_pt calls : In 1.1.4 they were all still there, In 1.1.5 half of them disappeared.

The measured_z = run_z_probe() from 1.1.4 was changed to measured_z = run_z_probe(printable) in 1.1.5 as well.

All these inaccuracies are pretty recent (between 1.1.4 and 1.1.5) and are causing quit some issues now in things that were thoroughly tested.

LVD-AC · 2017-11-10T18:28:34Z

Marlin/Marlin_main.cpp

@@ -2424,12 +2424,6 @@ static void clean_up_after_endstop_or_probe_move() {
        do_blocking_move_to_z(delta_clip_start_height);
    #endif

-    #if HAS_SOFTWARE_ENDSTOPS


Testing was done for printable = false that the nozzle stays within the printable radius; only the probe can move outside it (no testing on the probe with printable = false); so this is obsolete.

thinkyhead · 2017-11-11T01:09:59Z

does not set the new coordinates right to what they should be referenced to the home position. e.g. a height change or end-stop change does not seem to affect the new coordinates, and a delta radius change seems to introduce a false height offset. So rolling back on this one...

Interesting, anyway! I'll take a closer look at the kinematics conversions and see where the discrepancies are. If there are bugs to be fixed, maybe this could work in the future.

tcm0116 · 2017-11-11T04:34:54Z

That changes was introduced by my spirit animal @tcm0116

<https://github.com/tcm0116> I'm not sure if I'm honored or mortified...

LVD-AC · 2017-11-11T09:10:48Z

Some spiritual animal made the probes crash-proof, but tried to preserve the long probe on the first G33 probe by redefining the meaning and purpose of the printable parameter.

That answers issue #8298 from @tcm0116 ; the 'first probe to set the height' feature initially introduced to prevent damage from further probes apparently was no longer needed after 1.1.5 so I removed it, no big deal... damage control is now done on all probes in G33, G29 and elsewhere.

Doing a first probe was a bit "prevent damage after the damage is done" since such damage will be caused by the 1st instance of probe_pt. It only protected against scraping the bed between probes.

I had a good look at the differences between 1.1.4 and 1.1.5, I think we're back on track now with G33 and related issues.

LVD-AC mentioned this pull request Nov 8, 2017

[1.1] Patch G33's use of the term PROBE_MANUALLY #8318

Merged

LVD-AC changed the title ~~PROBE_SELECTED etc.~~ [1.1.x] PROBE_SELECTED etc. Nov 8, 2017

thinkyhead requested changes Nov 9, 2017

View reviewed changes

thinkyhead added F: Calibration C: LCD & Controllers labels Nov 9, 2017

thinkyhead mentioned this pull request Nov 10, 2017

[2.0.x] PROBE_SELECTED etc. #8324

Merged

thinkyhead force-pushed the 1.1.x-manual-probe branch from 5eb3ab1 to 2cfbb71 Compare November 10, 2017 05:28

thinkyhead approved these changes Nov 10, 2017

View reviewed changes

thinkyhead force-pushed the 1.1.x-manual-probe branch 6 times, most recently from 6addd40 to 15b5423 Compare November 10, 2017 06:49

LVD-AC commented Nov 10, 2017

View reviewed changes

LVD-AC and others added 3 commits November 10, 2017 19:17

PROBE_SELECTED etc.

8ce9306

Prevent position_is_reachable disaster

7aadfe3

Cleanup around updatePID

b0ff3a4

thinkyhead force-pushed the 1.1.x-manual-probe branch from dfdaa81 to b0ff3a4 Compare November 11, 2017 01:18

thinkyhead merged commit 2a54fd1 into MarlinFirmware:bugfix-1.1.x Nov 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1.1.x] PROBE_SELECTED etc. #8325

[1.1.x] PROBE_SELECTED etc. #8325

LVD-AC commented Nov 8, 2017 •

edited

thinkyhead left a comment

thinkyhead Nov 9, 2017

LVD-AC commented Nov 9, 2017

LVD-AC commented Nov 9, 2017 •

edited

thinkyhead commented Nov 10, 2017 •

edited

thinkyhead commented Nov 10, 2017 •

edited

thinkyhead commented Nov 10, 2017 •

edited

thinkyhead commented Nov 10, 2017 •

edited

LVD-AC commented Nov 10, 2017

LVD-AC commented Nov 10, 2017

thinkyhead commented Nov 10, 2017 •

edited

LVD-AC commented Nov 10, 2017

LVD-AC Nov 10, 2017 •

edited

thinkyhead Nov 11, 2017

LVD-AC Nov 10, 2017

thinkyhead Nov 11, 2017

LVD-AC Nov 11, 2017

LVD-AC commented Nov 10, 2017

LVD-AC commented Nov 10, 2017 •

edited

LVD-AC commented Nov 10, 2017 •

edited

LVD-AC Nov 10, 2017

thinkyhead commented Nov 11, 2017 •

edited

tcm0116 commented Nov 11, 2017 via email

LVD-AC commented Nov 11, 2017

[1.1.x] PROBE_SELECTED etc. #8325

[1.1.x] PROBE_SELECTED etc. #8325

Conversation

LVD-AC commented Nov 8, 2017 • edited

thinkyhead left a comment

Choose a reason for hiding this comment

thinkyhead Nov 9, 2017

Choose a reason for hiding this comment

LVD-AC commented Nov 9, 2017

LVD-AC commented Nov 9, 2017 • edited

thinkyhead commented Nov 10, 2017 • edited

thinkyhead commented Nov 10, 2017 • edited

thinkyhead commented Nov 10, 2017 • edited

thinkyhead commented Nov 10, 2017 • edited

LVD-AC commented Nov 10, 2017

LVD-AC commented Nov 10, 2017

thinkyhead commented Nov 10, 2017 • edited

LVD-AC commented Nov 10, 2017

LVD-AC Nov 10, 2017 • edited

Choose a reason for hiding this comment

thinkyhead Nov 11, 2017

Choose a reason for hiding this comment

LVD-AC Nov 10, 2017

Choose a reason for hiding this comment

thinkyhead Nov 11, 2017

Choose a reason for hiding this comment

LVD-AC Nov 11, 2017

Choose a reason for hiding this comment

LVD-AC commented Nov 10, 2017

LVD-AC commented Nov 10, 2017 • edited

LVD-AC commented Nov 10, 2017 • edited

LVD-AC Nov 10, 2017

Choose a reason for hiding this comment

thinkyhead commented Nov 11, 2017 • edited

tcm0116 commented Nov 11, 2017 via email

LVD-AC commented Nov 11, 2017

LVD-AC commented Nov 8, 2017 •

edited

LVD-AC commented Nov 9, 2017 •

edited

thinkyhead commented Nov 10, 2017 •

edited

thinkyhead commented Nov 10, 2017 •

edited

thinkyhead commented Nov 10, 2017 •

edited

thinkyhead commented Nov 10, 2017 •

edited

thinkyhead commented Nov 10, 2017 •

edited

LVD-AC Nov 10, 2017 •

edited

LVD-AC commented Nov 10, 2017 •

edited

LVD-AC commented Nov 10, 2017 •

edited

thinkyhead commented Nov 11, 2017 •

edited