[2.0.x] Dual printing, losing position on toolhead switch?

[marcio-ao]

Has anyone here done dual nozzle printing on Marlin bugfix-2.0.x? We are seeing a strange coordinate shift after toolhead changes and we don't know whether this problem has anything to do with changes we might have made to our FW.

[marcio-ao]

Able to confirm that by changing "tool_change" to the bare essentials we need, the problem goes away:

void tool_change(const uint8_t tmp_extruder, const float fr_mm_s/*=0.0*/, bool no_move/*=false*/) {

  #if defined(LULZBOT_TOOLHEAD_CHANGE_WORKAROUND)
    if(tmp_extruder != active_extruder) {
      active_extruder = tmp_extruder;
      #if ENABLED(SWITCHING_NOZZLE)
        move_nozzle_servo(active_extruder);
      #endif
    }
    return;
  #endif
  ...
}

So, something in this function is broken.

[GMagician]

I use dual nozzle single extruder (Dondolo) but in my case x, y, z shifting is normal behavior but I have SWITCHING_NOZZLE & SWITCHING_EXTRUDER activated)

[thinkyhead]

We'd love to have a patch from someone who (a) knows how to code and (b) has a printer of this kind to test with.

[GMagician]

@thinkyhead for sure a little bit more info will be helpful. I still have a "maybe similar" printer but with no "cnf" attached I'm not sure. I reported "part" of my configuration and I hope @marcio-ao will give its cnf. If they match maybe I will be able to help, maybe not and then some other "matching point 'a'" may comes to help.

[marcio-ao]

@thinkyhead: If I have more time, I'll try to selectively enable parts of tool_change and try to narrow down the problem. I suspect it has something to do with the nozzle offsets that are later in that code.

@GMagician : I are using SWITCHING_NOZZLE, but not SWITCHING_EXTRUDER. However, my Marlin code is a bit modified since we have two servos that need to be commanded in opposite directions. I had to modify something in Marlin to support that.

[GMagician]

@marcio-ao are you sure your shiftings have nothing to do with HOTEND_OFFSET_X, HOTEND_OFFSET_Y or HOTEND_OFFSET_Z?

[marcio-ao]

@GMagician: I left those commented out. Perhaps this means the values are undefined and they don't default to zero? I'm not sure if that's the case though, I'm pretty sure I checked with M218 to make sure they were zero.

[GMagician]

As I remember C uninitialized array get 0 as value. But this is enforced by firmware

EDIT: but they are also stored in eeprom...sure M502 M500?

[thinkyhead]

…by changing "tool_change" to the bare essentials…

Just peeking at your example code, I see it doesn't do anything to update the current position so that it corresponds to the newly-selected nozzle. How does that work?

[marcio-ao]

@thinkyhead: We've always done the nozzle offsets in the slicer. The only thing we really need the tool_change for is the servo control as our prototype toolhead lifts and lowers the nozzles.

[marcio-ao]

Okay, here is what my toolhead_change looks like after I removed a few lines that weren't getting compiled in:

void tool_change(const uint8_t tmp_extruder, const float fr_mm_s/*=0.0*/, bool no_move/*=false*/) {
      const float old_feedrate_mm_s = fr_mm_s > 0.0 ? fr_mm_s : feedrate_mm_s;
      feedrate_mm_s = fr_mm_s > 0.0 ? fr_mm_s : XY_PROBE_FEEDRATE_MM_S;

      if (tmp_extruder != active_extruder) {
        #if defined(LULZBOT_NO_MOVE_ON_TOOLHEAD_CHANGE)
          no_move = true;
        #endif

        if (!no_move && axis_unhomed_error()) {
          no_move = true;
          #if ENABLED(DEBUG_LEVELING_FEATURE)
            if (DEBUGGING(LEVELING)) SERIAL_ECHOLNPGM("No move on toolchange");
          #endif
        }

        // Save current position to destination, for use later
        set_destination_from_current();

        DEBUG_POS("", current_position);
        DEBUG_POS("", destination);

        #if HAS_LEVELING
          // Set current position to the physical position
          const bool leveling_was_active = planner.leveling_active;
          set_bed_leveling_enabled(false);
        #endif

          #if ENABLED(SWITCHING_NOZZLE)
            // Always raise by at least 1 to avoid workpiece
            const float zdiff = hotend_offset[Z_AXIS][active_extruder] - hotend_offset[Z_AXIS][tmp_extruder];
            current_position[Z_AXIS] += (zdiff > 0.0 ? zdiff : 0.0) + 1;
            planner.buffer_line_kinematic(current_position, planner.max_feedrate_mm_s[Z_AXIS], active_extruder);
            move_nozzle_servo(tmp_extruder);
          #endif

          const float xdiff = hotend_offset[X_AXIS][tmp_extruder] - hotend_offset[X_AXIS][active_extruder],
                      ydiff = hotend_offset[Y_AXIS][tmp_extruder] - hotend_offset[Y_AXIS][active_extruder];

              SERIAL_ECHOPAIR("Offset Tool XY by { ", xdiff);
              SERIAL_ECHOPAIR(", ", ydiff);
              SERIAL_ECHOLNPGM(" }");

          // The newly-selected extruder XY is actually at...
          current_position[X_AXIS] += xdiff;
          current_position[Y_AXIS] += ydiff;

          // Set the new active extruder
          active_extruder = tmp_extruder;

        #if HAS_LEVELING
          // Restore leveling to re-establish the logical position
          set_bed_leveling_enabled(leveling_was_active);
        #endif

        #if ENABLED(SWITCHING_NOZZLE)
          // The newly-selected extruder Z is actually at...
          current_position[Z_AXIS] -= zdiff;
        #endif

        // Tell the planner the new "current position"
        SYNC_PLAN_POSITION_KINEMATIC();

        DEBUG_POS("S1", current_position);

        constexpr bool safe_to_move = true;

        // Raise, move, and lower again
        if (safe_to_move && !no_move && IsRunning()) {
          DEBUG_POS("Move back", destination);
          // Move back to the original (or tweaked) position
          do_blocking_move_to(destination[X_AXIS], destination[Y_AXIS], destination[Z_AXIS]);
        }
        #if ENABLED(SWITCHING_NOZZLE)
          else {
            // Move back down. (Including when the new tool is higher.)
            do_blocking_move_to_z(destination[Z_AXIS], planner.max_feedrate_mm_s[Z_AXIS]);
          }
        #endif
      } // (tmp_extruder != active_extruder)

      DEBUG_POS("S2", current_position);

      planner.synchronize();

      feedrate_mm_s = old_feedrate_mm_s;

    SERIAL_ECHO_START();
    SERIAL_ECHOLNPAIR(MSG_ACTIVE_EXTRUDER, (int)active_extruder);
}

Now, look at the output:

  current_position=(100.00, 100.00, 0.81) : 
  destination=(100.00, 100.00, 0.81) : 
Offset Tool XY by { 0.00, 0.00 }
  current_position=(154.28, 164.29, 1.43) : S1
  current_position=(154.28, 164.29, 0.81) : S2
echo:Active Extruder: 1
X:154.28 Y:164.29 Z:0.81 E:0.00 Count X:15428 Y:16429 Z:90

So what seems to be happening is two-fold. There is in fact a bug in the code, but it only appears when no_move is true, which is not generally the case in upstream Marlin. In our FW, I force no_move to always be true (because I added LULZBOT_NO_MOVE_ON_TOOLHEAD_CHANGE) since I didn't want the toolhead to be jogging up and down. However, when no_move is true, then the position gets saved in destination, but it never is restored.

As you can see from the lines "S1" and "S2" in the log, something -- I do not know what -- is causing the current_position to diverge quite a bit from the saved value. It's a bit odd, since the XY offsets are zero and nothing else is supposed to be updating the current_position, but something clearly is.

[marcio-ao]

So a possible fix to the code is to always restore the current_position from the value that was stored in destination, regardless of whether no_move is true or false. But I do not know whether it bears investigation as to why current_position is changing by so much (and by what).

[marcio-ao]

Okay, I added more debugging statements:

void tool_change(const uint8_t tmp_extruder, const float fr_mm_s/*=0.0*/, bool no_move/*=false*/) {
      const float old_feedrate_mm_s = fr_mm_s > 0.0 ? fr_mm_s : feedrate_mm_s;
      feedrate_mm_s = fr_mm_s > 0.0 ? fr_mm_s : XY_PROBE_FEEDRATE_MM_S;

      if (tmp_extruder != active_extruder) {
        #if defined(LULZBOT_NO_MOVE_ON_TOOLHEAD_CHANGE)
          no_move = true;
        #endif

        if (!no_move && axis_unhomed_error()) {
          no_move = true;
          #if ENABLED(DEBUG_LEVELING_FEATURE)
            if (DEBUGGING(LEVELING)) SERIAL_ECHOLNPGM("No move on toolchange");
          #endif
        }

        // Save current position to destination, for use later
        set_destination_from_current();

        DEBUG_POS("", current_position);
        DEBUG_POS("", destination);

        #if HAS_LEVELING
          // Set current position to the physical position
          const bool leveling_was_active = planner.leveling_active;
          set_bed_leveling_enabled(false);
        #endif

          DEBUG_POS("S1", current_position);

          #if ENABLED(SWITCHING_NOZZLE)
            // Always raise by at least 1 to avoid workpiece
            const float zdiff = hotend_offset[Z_AXIS][active_extruder] - hotend_offset[Z_AXIS][tmp_extruder];
            current_position[Z_AXIS] += (zdiff > 0.0 ? zdiff : 0.0) + 1;
            planner.buffer_line_kinematic(current_position, planner.max_feedrate_mm_s[Z_AXIS], active_extruder);
            move_nozzle_servo(tmp_extruder);
          #endif

          DEBUG_POS("S2", current_position);

          const float xdiff = hotend_offset[X_AXIS][tmp_extruder] - hotend_offset[X_AXIS][active_extruder],
                      ydiff = hotend_offset[Y_AXIS][tmp_extruder] - hotend_offset[Y_AXIS][active_extruder];

          //#if ENABLED(DEBUG_LEVELING_FEATURE)
          //  if (DEBUGGING(LEVELING)) {
              SERIAL_ECHOPAIR("Offset Tool XY by { ", xdiff);
              SERIAL_ECHOPAIR(", ", ydiff);
              SERIAL_ECHOLNPGM(" }");
          //  }
          //#endif

          // The newly-selected extruder XY is actually at...
          current_position[X_AXIS] += xdiff;
          current_position[Y_AXIS] += ydiff;

          DEBUG_POS("S3", current_position);

          // Set the new active extruder
          active_extruder = tmp_extruder;

          DEBUG_POS("S4", current_position);

        #if HAS_LEVELING
          // Restore leveling to re-establish the logical position
          set_bed_leveling_enabled(leveling_was_active);
        #endif

        DEBUG_POS("S5", current_position);

        #if ENABLED(SWITCHING_NOZZLE)
          // The newly-selected extruder Z is actually at...
          current_position[Z_AXIS] -= zdiff;
        #endif

        DEBUG_POS("S6", current_position);

        // Tell the planner the new "current position"
        SYNC_PLAN_POSITION_KINEMATIC();

        DEBUG_POS("S7", current_position);

        constexpr bool safe_to_move = true;

        // Raise, move, and lower again
        if (safe_to_move && !no_move && IsRunning()) {
          DEBUG_POS("Move back", destination);
          // Move back to the original (or tweaked) position
          do_blocking_move_to(destination[X_AXIS], destination[Y_AXIS], destination[Z_AXIS]);
        }

        #if ENABLED(SWITCHING_NOZZLE)
          else {
            // Move back down. (Including when the new tool is higher.)
            do_blocking_move_to_z(destination[Z_AXIS], planner.max_feedrate_mm_s[Z_AXIS]);
          }
        #endif

      } // (tmp_extruder != active_extruder)

      DEBUG_POS("S8", current_position);
      DEBUG_POS("S9", destination);

      planner.synchronize();

      feedrate_mm_s = old_feedrate_mm_s;

    SERIAL_ECHO_START();
    SERIAL_ECHOLNPAIR(MSG_ACTIVE_EXTRUDER, (int)active_extruder);
}

And here are the results:

X:100.00 Y:100.00 Z:0.81 E:0.00 Count X:15429 Y:16428 Z:-99
  current_position=(100.00, 100.00, 0.81) : 
  destination=(100.00, 100.00, 0.81) : 
  current_position=(154.28, 164.29, -0.20) : S1
  current_position=(154.28, 164.29, 0.80) : S2
Offset Tool XY by { 0.00, 0.00 }
  current_position=(154.28, 164.29, 0.80) : S3
  current_position=(154.28, 164.29, 0.80) : S4
  current_position=(154.28, 164.29, 1.43) : S5
  current_position=(154.28, 164.29, 1.43) : S6
  current_position=(154.28, 164.29, 1.43) : S7
  current_position=(154.28, 164.29, 0.81) : S8
  destination=(100.00, 100.00, 0.81) : S9
echo:Active Extruder: 1
X:154.28 Y:164.29 Z:0.81 E:0.00 Count X:15428 Y:16429 Z:91

So the current_position is messed up by 54.28 mm at output S1, immediately after set_bed_leveling_enabled(false); is called.

[marcio-ao]

This of course begs prompts the question: Why does bed leveling need to be turned off to change the toolhead?

[thinkyhead]

When the tool is changed (with no_move) the current position must change so that it corresponds to the position of the now-active nozzle. For example, if T1 is 20mm to the right of T0 then after T1 S1 (no move) the current X position should now be 20 larger than it was.

If the S1 flag isn't given then the tool is supposed to move as close as it can get to whatever the current_position was before the tool-change. In that case, ideally the logical position won't change. Of course, if the carriage is at the edge of the bed, then it cannot necessarily reach the required position.

The hotend_offset[Z_AXIS] also has to be accounted for.

Why does bed leveling need to be turned off to change the toolhead?

Bed leveling moves Z up and down as the XY position changes. The active nozzle is the one that's being adjusted. Its physical position over the bed is meant to remain constant. The other tool on the same carriage is essentially ignored, and the inactive nozzle's height over the bed is not considered.

So, imagine a bed that is tilted at a pronounced angle, for example, such that the T1 nozzle's distance to the bed is always 1mm closer than the T0 nozzle. Imagine that the T0 nozzle is 10mm from the bed, and the T1 nozzle is 9mm from the bed, and the tool is changed from T0 to T1. Even though its physical distance from the bed is 9mm, its logical position would be Z=10 at this point.

We have to apply a correction one way or another. The old code would look up the Z leveling offset being applied to the previous tool and subtract the Z leveling offset for the new tool and apply a correction. But this required the tool_change function to be knowledgable about bed leveling. Turns out the same thing could be accomplished by disabling leveling at the start of tool_change, then re-enabling it after changing the tool.

a possible fix to the code is to always restore the current_position from the value that was stored in destination, regardless of whether no_move is true or false.

So you can now see why that is not sensible.

[marcio-ao]

@thinkyhead: Your explanations make sense when the nozzle offsets are something other than zero. But you still haven't given an explanation for why the position is changing when the nozzle offsets are set to zero, as they are in my case.

[GMagician]

Which kind of bed leveling are you using? Only when ABL_PLANAR seems may alterate XY

[marcio-ao]

@GMagician : AUTO_BED_LEVELING_LINEAR

[marcio-ao]

@GMagician: It looks like AUTO_BED_LEVELING_LINEAR is ABL_PLANAR. So does this mean that the bug is actually in the code for ABL_PLANAR? That would explain why only we are seeing it. I think most printers have moved away from planar leveling.

[GMagician]

Not sure.. I read something like:

void set_bed_leveling_enabled(const bool enable/*=true*/) {
  [CUT]
  #else // OLDSCHOOL_ABL
  [CUT]
if (!enable)
        // When disabling just get the current position from the steppers.
        // This will yield the smallest error when first converted back to steps.
        set_current_from_steppers_for_axis(
          #if ABL_PLANAR
            ALL_AXES
          #else
            Z_AXIS
          #endif
);

but I have to investigate

[GMagician]

set_current_from_steppers_for_axis should copy all axis curposition from stepcounter value. Don't know exactly logic behind.

[marcio-ao]

@GMagician: I think this explains the offset. Often when tool_change is called, the steppers are moving, so this can potentially introduce a positional error. I do not think it is correct for set_current_from_steppers_for_axis to be called by set_bed_leveling_enabled when set_bed_leveling_enabled is getting called from tool_change

[thinkyhead]

…make sense when the nozzle offsets are something other than zero…
…the nozzle offsets are set to zero…

But are there two nozzles, spaced apart from each other?

[thinkyhead]

Often when tool_change is called, the steppers are moving,

I'm pretty sure planner.synchronize is called at the start of tool_change (or gcode_T). If not, it probably should be, or planner.synchronize should be moved to an earlier point in tool_change.

[GMagician]

I'm pretty sure planner.synchronize is called at the start of tool_change

I don't see it...

That code is the only part I saw that may change X & Y

[marcio-ao]

But are there two nozzles, spaced apart from each other?

@thinkhead: I explained earlier in this thread that the nozzle offsets are handled by our slicer. So as far as Marlin is concerned, the nozzles are in the same location.

[GMagician]

You may try to add a planner.synchronize call just at beginning of Tool_Change as suggested by @thinkyhead, since I think is not really present but it should

[thinkyhead]

… the nozzle offsets are handled by our slicer… So as far as Marlin is concerned, the nozzles are in the same location.

Ah right, I forgot you were doing a hack.

Ok, so Marlin is not currently coded for that. The firmware currently assumes that it will know the hard-coded XY nozzle offset so it can manage the movement limits, adjust the current position on tool-change, etc. You'll need to add a new option to the configuration which specifies that nozzle offsets are always handled externally, then have Marlin leave out all its nozzle offset handling when that option is enabled.

[GMagician]

@thinkyhead what if offsets are all 0? it could be handled and Marlin should think nozzles are in the same position even if are not. Then slicer will handle the offset moving head in another "physical" position

[thinkyhead]

@GMagician — It's not possible at the pre-compiler level to check if the nozzle XY offsets are 0, but we could add a static_assert to throw an error in that case.

Or, someone with coding skills and a vested interest in this customization could add code throughout Marlin to account for this special case and modify behavior wherever it's needed.

[marcio-ao]

@thinkyhead: It's not an error to have both nozzles in the same place, for example there are hotends out there, such as the Cyclops from E3D, that have no offsets.

Besides, you are missing the point. The bug isn't that Marlin cannot support a XY_OFFSET of zero (it certainly can, because once I bypass the code in tool_change, it prints just fine with a nozzle offset of zero), the bug is that there is an error in set_bed_leveling_enabled when AUTO_BED_LEVELING_LINEAR is enabled where it adds an offset that shouldn't be there.

[marcio-ao]

It's not possible at the pre-compiler level to check if the nozzle XY offsets are 0, but we could add a static_assert to throw an error in that case.

No, don't do this. Marlin already supports an offset of zero -- I made successful prints with it. The only bug is the bug in AUTO_BED_LEVELING_LINEAR which has nothing to do with nozzle offsets.

[thinkyhead]

for example there are hotends out there, such as the Cyclops from E3D, that have no offsets.

That's what SINGLENOZZLE is for.

[thinkyhead]

there is an error in set_bed_leveling_enabled when AUTO_BED_LEVELING_LINEAR is enabled where it adds an offset that shouldn't be there

As this is an open source project, it is most helpful for contributors who have the equipment that exhibits the issue to figure out how to fix it and contribute a PR. Those of us who don't have the equipment will have a much harder time developing a solution.

[GMagician]

@marcio-ao, thinkyhead is right about

Those of us who don't have the equipment will have a much harder time developing a solution.

I can suggest you to analyze set_current_from_steppers_for_axis to check values it read and check if they are wrong. These values are got from part of code that were involved in big changes maybe something has been lost..

[marcio-ao]

@thinkyhead, @GMagician: The flip side of this is that those of us with the equipment don't necessarily have the understanding of Marlin internals to come up with a correct fix, nor does a fix that works for me necessary work for everyone else -- case in point, the easiest fix for me is to bypass all the leveling and positioning code in tool_change since it isn't really accomplishing anything for me.

Anyhow, one of the take-away from this discussion is that I should probably modify our FW to let Marlin handle the nozzle offsets -- the reason we have always done this in the slicer is historical and is probably something we should move away from. I'll experiment with that. For one thing, it seems like what is exposing the bug is the fact that I am forcing no_move to be true in tool_change ... when that argument is left as false, the position is saved and restored.

-- Marcio

[GMagician]

@marcio-ao I agree with you about knowledge and that with offset 0 on both extruders Marlin should works as expected since is slicer that will change "destination" according to its new offsets.
@thinkyhead and I have suggested you to call planner.synchronize at the very beginning, have you done it?

[marcio-ao]

@thinkyhead and I have suggested you to call planner.synchronize at the very beginning, have you done it?

I'm about to give it a try, although I guess I don't really understand the rationale for it. Isn't the ISR still moving the steppers while the toolhead change is taking place?

[GMagician]

@marcio-ao I checked code to see differences with no_move = true vs no_move = false and Marlin miss only a do_blocking_move_to(destination[X_AXIS], destination[Y_AXIS], destination[Z_AXIS]); call nothing more

[marcio-ao]

@GMagician: You are exactly correct, this is what causes the bug. What is happening is that the position before the call to set_bed_leveling_enabled is saved in destination by the following line:

// Save current position to destination, for use later
set_destination_from_current();

So if set_bed_leveling_enabled changes the position, it will be restored by do_blocking_move_to when no_move = true, but not when no_move = false. The latter causes a problem when using AUTO_BED_LEVELING_LINEAR, since that leveling routine is changing the position.

I guess the thought is that if a planner.synchronize is put in there, it will keep set_bed_leveling_enabled from changing the position? I'll let you know if it makes a difference.

[marcio-ao]

@GMagician, @thinkyhead: Sorry for the delay in testing the suggested fix. It looks like putting a planner.synchronize(); fixes the issue:

void tool_change(const uint8_t tmp_extruder, const float fr_mm_s/*=0.0*/, bool no_move/*=false*/) {
    planner.synchronize();
    ...
}

[marcio-ao]

Also, rather than doing what I was doing above, I also tried specifying the nozzle offset in Marlin. This works fine, except that the tool-head crashes into the min endstop when switching to the rightmost nozzle. I thought Marlin was supposed to automatically take travel limits into account when using nozzle offsets? This does not seem to be happening.

[thinkyhead]

the tool-head crashes into the min endstop when switching to the rightmost nozzle

Sounds like some extra logic will be needed to (1) make sure not to move outside of the configured boundaries and (2) make sure the final nozzle position is correctly set as current_position, synchronized to the planner, and reported to the host.

[thinkyhead]

The planner.synchronize() patch has been made to the bugfix branches.

[marcio-ao]

This has been fixed.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.