Make sure that move in raise_z_for_servo() gives enough space for servo to deploy

[lrpirlet]

Using M401, the servo was crashing into the bed when deploying...
It seems that original code did not take into account that zprobe_zoffset is negative for a servo mounted switch

I added some debug code

void raise_z_for_servo() {
  float zpos = current_position[Z_AXIS], z_dest = Z_RAISE_BEFORE_PROBING;
  z_dest += axis_known_position[Z_AXIS] ? zprobe_zoffset : zpos; 
#if ENABLED(DEBUG_LEVELING_FEATURE)    //lrplrplrp
    if (marlin_debug_flags & DEBUG_LEVELING) {
      SERIAL_ECHOPAIR("zpos = ", (float)zpos);
      SERIAL_ECHOPAIR("\nz_dest = ", (float)z_dest);
      SERIAL_ECHOPAIR("\nzprobe_zoffset = ", (float)zprobe_zoffset);
      SERIAL_EOL;
    }
#endif  //lrplrplrp
  if (zpos < z_dest) do_blocking_move_to_z(z_dest); // also updates current_position
}

that produced a move zpos = 4.5 mm too small for the probe (4.86 mm)

14:12:32.049 : N56 M401 *53
14:12:32.049 : zpos = 4.50
14:12:32.049 : z_dest = 1.14
14:12:32.049 : zprobe_zoffset = -4.86
14:12:32.049 : deploy_z_probe > current_position: (0.00, 0.00, 4.50)
14:12:32.346 : ok

I modified the code to read

void raise_z_for_servo() {
  float zpos = current_position[Z_AXIS], z_dest = Z_RAISE_BEFORE_PROBING;
  z_dest += axis_known_position[Z_AXIS] ? zprobe_zoffset*Z_HOME_DIR : zpos; //zprobe_zoffset = Z_PROBE_OFFSET_FROM_EXTRUDER a negative number for a switch on servo so combine with Z_HOME_DIR -1 to move so that there is enough space for the probe
#if ENABLED(DEBUG_LEVELING_FEATURE)    //lrplrplrp
    if (marlin_debug_flags & DEBUG_LEVELING) {
      SERIAL_ECHOPAIR("zpos = ", (float)zpos);
      SERIAL_ECHOPAIR("\nz_dest = ", (float)z_dest);
      SERIAL_ECHOPAIR("\nzprobe_zoffset = ", (float)zprobe_zoffset);
      SERIAL_EOL;
    }
#endif  //lrplrplrp
  if (zpos < z_dest) do_blocking_move_to_z(z_dest); // also updates current_position
}

This clears the space for the probe to deploy

14:50:58.825 : N59 M401 *58
14:50:58.825 : zpos = 4.50
14:50:58.825 : z_dest = 10.86
14:50:58.825 : zprobe_zoffset = -4.86
14:50:58.825 : do_blocking_move_to: (0.00, 0.00, 10.86)
14:51:05.216 : deploy_z_probe > current_position: (0.00, 0.00, 10.86)
14:51:05.513 : ok

The proposed request for change has no debug code and some comments

[KiteLab]

In my opinion the complete line is rubbish and unneeded.
If you drop this line you'll always end at z = Z_RAISE_BEFORE_PROBING before the servo is moved.
If you have been too deep, it's high enough then to not crash.
If you have been too high, part of the way to probe is already done before the servo is moved.
If we did not know where we are, we have been somewhere above z-min during boot, what was set as 0. We'd move the servo to Z_RAISE_BEFORE_PROBING mm above this point. That's safe as long we did not boot above z_max - Z_RAISE_BEFORE_PROBING. But before we know where we are, every move (away from an endstop) is potentially dangerous. That's not different from the version with the line discussed here. Indeed with this line we have a max_height of our path at current_position + Z_RAISE_BEFORE_PROBING +/- something. That's normally worse than 0 + Z_RAISE_BEFORE_PROBING.

You only have to care whether Z_RAISE_BEFORE_PROBING is big enough (with or without this line).

Even the decision if (zpos < z_dest) is not needed without the discussed line.

(The ideal safe setup for a z-min-probe (which needs a minimum height to deploy) is together with a z-max-endstop. You could home down but would have the safety when rising.)

EDITED 3 times.

All said above expects an empty bed.

[lrpirlet]

@KiteLab
Thanks for you comment.
I sort of agree with you. However, I did not want to change the original algorithm. The ternary operator was there before, probably set to minimize the needed move. Here, I wanted to make sure that my servo don't kill the Arduino by pumping too much power...
My only modification was to make sure that both values are of the same sign (that both added move are in the same direction).
Maybe a diff would better show what I changed?

1751 1751    
1752 1752      void raise_z_for_servo() {  
1753 1753        float zpos = current_position[Z_AXIS], z_dest = Z_RAISE_BEFORE_PROBING;  
1754  -      z_dest += axis_known_position[Z_AXIS] ? zprobe_zoffset : zpos;  
1754 +    z_dest += axis_known_position[Z_AXIS] ? zprobe_zoffset*Z_HOME_DIR : zpos; //zprobe_zoffset = Z_PROBE_OFFSET_FROM_EXTRUDER a negative number for a switch on servo arm  
1755 +                                                                              //so combine with Z_HOME_DIR -1 to move enough away from bed for the probe  
1755 1756        if (zpos < z_dest) do_blocking_move_to_z(z_dest); // also updates current_position  
1756 1757      }  
1757 1758    

[KiteLab]

We are in a PR here. The diff is one click away.

I agree.
Your code is better than the buggy current.

[thinkyhead]

This is looking pretty good as a solution to the issue. It should be rebased onto RCBugFix so it can be merged there, and then later (pretty soon) it will be included in RC4. Feel free to ask for assistance if there's any difficulty in doing the rebase. You'll probably need to do a new PR after rebasing it.

[lrpirlet]

@thinkyhead
thanks Scott. I sure would appreciate some hints about rebase...
I did pull the RC and created 3 branches: M401_crashes_servo_on_bed, G28_raizes_Z_3_times and a branch to motorize_the_manual_mesh_leveling...

All Branches were created against Marlin 1.1.0 Release Candidate 3 - 01 December 2015 commit 5a54204f723f7fd110b57d19f0de01cecd8d47e7

Would it be as simple as issuing git rebase RC M401_crashes_servo_on_bed followed by git rebase G28_raizes_Z_3_times.
Will I loose my work on the motorize_the_manual_mesh_leveling branch???

Of course, I can clone the RCBugFix branch and re-do a PR from there...

[Wackerbarth]

Base your rebase on RCBugFix

On Jan 27, 2016, at 3:36 PM, lrpirlet notifications@github.com wrote:

@thinkyhead
thanks Scott. I sure would appreciate some hints about rebase...
I did pull the RC and created 3 branches: M401_crashes_servo_on_bed, G28_raizes_Z_3_times and a branch to motorize_the_manual_mesh_leveling...

All Branches were created against Marlin 1.1.0 Release Candidate 3 - 01 December 2015 commit 5a54204

Would it be as simple as issuing git rebase RC M401_crashes_servo_on_bed followed by git rebase G28_raizes_Z_3_times.
Will I loose my work on the motorize_the_manual_mesh_leveling branch???

Of course, I can clone the RCBugFix branch and re-do a PR from there...

—
Reply to this email directly or view it on GitHub.

[lrpirlet]

@Wackerbarth, Thanks for your hint that gave me part of what I need/want to do...
I issued git rebase RCBugFix M401_crashes_servo_on_bed on my pc... M401_crashes_servo_on_bed branch out now from RCBugFix... and I lost nothing :-)

Sorry for the following newbie questions (I guess yes but do not want to mess everything without first asking)

Should I now git push origin M401_crashes_servo_on_bed to update github,
close this 2924 thread
and on github submit a new PR from the rebased branch M401_crashes_servo_on_bed ?

[Wackerbarth]

If you do a

git push -f ..............

of that same branch to your GitHub account, this PR will be updated and then reflect your revised version just as if it had been the original version. It is not necessary to create a new PR for the purpose.

[lrpirlet]

Ok, seems that my rebase did work... Thanks @Wackerbarth I did learn some more..

[thinkyhead]

This weekend we are prepping the next release candidate, to get users past some bugs affecting LCD menus and a few other things. So I've rebased this and it should be going in shortly.

[lrpirlet]

@thinkyhead Thanks for merging it...
Branch deleted to keep my fork tidy