Marlin: [DELTA] M666 offsets 2 * home_bump_mm(axis) mm

[otvald]

After moving up for final axis endstop detection, the internal position (last destination) is 2 * home_bump_mm(axis) ( in my case 10 mm) offset.

When applying axis offset, this additional offset should be cleared, since we are at (per definition) axis=0.

diff --git a/Marlin/Marlin_main.cpp b/Marlin/Marlin_main.cpp
index e2399af..ec436c4 100644
--- a/Marlin/Marlin_main.cpp
+++ b/Marlin/Marlin_main.cpp
@@ -2465,6 +2465,8 @@ static void homeaxis(AxisEnum axis) {
   #if ENABLED(DELTA)
     // retrace by the amount specified in endstop_adj
     if (endstop_adj[axis] * axis_home_dir < 0) {
+      // Reset axis position
+      current_position[axis] = 0;
       sync_plan_position();
       #if ENABLED(DEBUG_LEVELING_FEATURE)
         if (DEBUGGING(LEVELING)) {

[Blue-Marlin]

When applying axis offset, this additional offset should be cleared, since we are at (per definition) axis=0.

No. When the move to endstop_adj[axis] has finished we are at home.

Makes no sense there. Not before the move is done.

  #if ENABLED(DELTA)
    // retrace by the amount specified in endstop_adj
    if (endstop_adj[axis] * axis_home_dir < 0) {
      sync_plan_position();
      #if ENABLED(DEBUG_LEVELING_FEATURE)
        if (DEBUGGING(LEVELING)) {
          SERIAL_ECHOPAIR("> endstop_adj = ", endstop_adj[axis]);
          DEBUG_POS("", current_position);
        }
      #endif
      line_to_axis_pos(axis, endstop_adj[axis]); // sets current_position[axis] to endstop_adj[axis]
    }
  #endif

  // Set the axis position to its home position (plus home offsets)
  set_axis_is_at_home(axis);

[otvald]

@Blue-Marlin What is your recommended fix then? The snippet you quote is the 'stock' code, isn't it?

We do agree that there is a problem, right?

Test it easily by e.g. M666 X0 Y10 Z0.01 Then you will see that X is at the top, Y is 20 mm down and Z is 10.1 mm down from the endstop... Especially X and Z should have same height, but are 10mm offset! (Numbers assume home_bump_mm = 5.0)

When we hit the endstop, what is our position on that axis? 0 or 10.0? It should be 0 at that point!

The implementation, as I decode it:

1. Hit endstop
2. Reset Axis (current_position[axis] = 0)
3. Move to -5.00 mm on axis (assume home_bump_mm=5.00, in this example)
4. Then do a move on the axis towards 10.00 using line_to_axis_pos
5. In line_to_axis_pos, current_position[axis] = 10.00 (our destination)
6. During this move we will trigger endstop
7. We then reach the code you quote above
8. But first we actively mess up by doing sync_plan_position();. This will apply current_position to the internal position in planner. At this point the position will be 10.00 on the axis (It should have been 0, if you ask me),
9. When trying to adjust endstop we want move 'adj' mm.
10. When planning (line_to_axis_pos) we then move from last position (10 to adj), thus adding 10mm to our path, since adj is negative.

Therefore, before applying adjust, I would reset the position in the planner to reflect '0'. This is our starting fixpoint!

Please comment on the above!

[thinkyhead]

But first we actively mess up by doing sync_plan_position()

That line stood out for me, as well. I think you might be onto something here…

[Blue-Marlin]

I would not concentrate on this code block to much.
In M666 X0 Y10 Z0.01 the values are all positive.
I can't find a negation of them.
axis_home_dir is always positive for a DELTA.

if (endstop_adj[axis] * axis_home_dir < 0) {
  unlikely to be executed
}

[otvald]

@Blue-Marlin @thinkyhead

Sorry, in my attempt to explain, I accidentally wrote M666 X0 Y10 Z0.01 of course I mean M666 X0 Y-10 Z-0.01 .. Point is that I wanted to make some endstop offsets and wrote them from the top of my head!... Happy to see that you took the time to try and understand my explanation, as it took me quite some time to explain..

I would not concentrate on this code block to much.

Oh, you didn't look at my explanation, that is fine, but you do agree that there is a problem, right?

Please confirm that it is working on your delta! It is NOT working without the suggested fix on my machine!

If you do not have a delta, please read (with an open mind, that there actually may be an error). If you need extra details and debug info, I'll provide as further documentation!

I may lack the understanding of sync_plan_position(), but when looking at the code (as I tried to explain above, but I'll go into details here):

inline void sync_plan_position() {
  #if ENABLED(DEBUG_LEVELING_FEATURE)
    if (DEBUGGING(LEVELING)) DEBUG_POS("sync_plan_position", current_position);
  #endif
  planner.set_position_mm(current_position[X_AXIS], current_position[Y_AXIS], current_position[Z_AXIS], current_position[E_AXIS]);
}

In planner.cpp:

void Planner::set_position_mm(const float& x, const float& y, const float& z, const float& e)
...
long nx = position[X_AXIS] = lround(x * axis_steps_per_mm[X_AXIS]),
         ny = position[Y_AXIS] = lround(y * axis_steps_per_mm[Y_AXIS]),
         nz = position[Z_AXIS] = lround(z * axis_steps_per_mm[Z_AXIS]),
         ne = position[E_AXIS] = lround(e * axis_steps_per_mm[E_AXIS]);
    stepper.set_position(nx, ny, nz, ne);
...

What this does is applying the current_position[X Y Z E] to the internal position[X Y Z E] in the planner. I.e. the planner will now be explicitly set in position for future path calculations!

Before calling the sync we have just done this:

// Move slowly towards the endstop until triggered
  line_to_axis_pos(axis, 2 * home_bump_mm(axis) * axis_home_dir, get_homing_bump_feedrate(axis));

Looking at what happened there:

inline void line_to_axis_pos(AxisEnum axis, float where, float fr_mm_m = 0.0) {
  float old_feedrate_mm_m = feedrate_mm_m;
  current_position[axis] = where;
 <snip>

So now, current_position[_AXIS] is definately nonzero (2 * home_bump_mm, 10.00 mm in my examples). This serves as the destination endpoint, out of reach, well above the endstop, and ensures we hit the endstop.

After hitting the endstop, nothing modifies the current_position. If you do a debug of the internals you will also see that the current position is 10.00 at this point! I.e. current_position[axis] is 10.00

Now, please explain my misunderstanding of what sync_plan_position() does: It explicitly sets (in the planner) the position[_AXIS] reflecting the 2 * bump value (10.00mm) (which is the current value of current_position[_AXIS], as I just derived above.

In the subsequent line_to_axis_pos(axis, endstop_adj[axis]);, the planner will try to make a move from position[_AXIS] which reflects 10.00, not 0.00, to current_position[_AXIS] reflecting adj, making the movement 10 mm longer than needed!

---

I may suggest that immediately after

  // Move slowly towards the endstop until triggered
  line_to_axis_pos(axis, 2 * home_bump_mm(axis) * axis_home_dir, get_homing_bump_feedrate(axis));

do a current_position[axis]=0 instead of my suggestion in the delta code only. This will also affect the code in #if ENABLED(Z_DUAL_ENDSTOPS) which may suffer same problem (but I have NOT looked into this).

[otvald]

@thinkyhead
In commit 1e57a6af73866c8e05c2972e17b5a2576bfcc9c2, you change this:

+inline void do_blocking_move_to_axis_pos(AxisEnum axis, float where, float fr_mm_m = 0.0) {
+  current_position[axis] = where;
+  do_blocking_move_to(current_position[X_AXIS], current_position[Y_AXIS], current_position[Z_AXIS], fr_mm_m);
+}
+

-  destination[axis] = -home_bump_mm(axis) * axis_home_dir;
-  line_to_destination();
-  stepper.synchronize();
+  do_blocking_move_to_axis_pos(axis, -home_bump_mm(axis) * axis_home_dir, homing_feedrate_mm_m[axis]);

Here, you actively override current_position[axis] in do_blocking_move_to_axis_pos, as it internally is used in set_destination_to_current. Problem is that the original implementation uses line_to_destination, which does not rely on (or modify) current_position, but modifies destination directly. So there is no sideeffect. This means that after calling line_to_destination();, the current_position[AXIS] was not modified, but it was modified by your code change!

In the old code,

  // Set the axis position as setup for the move
  current_position[axis] = 0;
  sync_plan_position();

current_position[axis] would still be 0 later down. But your change would modify it! Next time sync_plan_position() is called after do_blocking_move_to_axis_pos, it will be with the modified current_position[axis].... This sideeffect seem to be preserved line_to_axis_pos in the current RC.

So this was working in AnHardt's implementation, but I would have preferred an explicit current_position[axis] = 0; prior to sync_plan_position(), as he does elsewhere.

[thinkyhead]

A lot of reading… and my eyes are tired. I will get to this later. @otvald would you be able to make a PR that simply fixes the issue and submit it?

[otvald]

Just added PR #4476 .

[Blue-Marlin]

@otvald
You can't find the message because i deleted it, because it was rubbish. Somehow mixed two cases. Sorry.

[AnHardt]

Tested #4476 on a Cartesian - works.
This doesn't make me wonder, because there are no endstop_adjs for these type of printers and the set_axis_is_at_home(axis) is right after this.

[boelle]

@otvald Is this one fixed in latest RCBugFix ?

[boelle]

@thinkyhead @jbrazio i think we can close this one as there are no response anymore, not saying its been fixed but the likelihood is there