MarlinFirmware / Marlin

Marlin is an optimized firmware for RepRap 3D printers based on the Arduino platform. Many commercial 3D printers come with Marlin installed. Check with your vendor if you need source code for your specific machine.
https://marlinfw.org
GNU General Public License v3.0
16.18k stars 19.21k forks source link

UBL in combination with fwretract zhop loses Z position #10200

Closed joris57 closed 5 years ago

joris57 commented 6 years ago

I recently switched on fwretract on a corexy printer with UBL enabled (bugfix 1.1.X of March 20) and noticed that a calibration object, I developed to tune retraction settings, came out higher and more flimsy that usual.

It turned out that the final actual Z position after the print was higher than the expected value, while the display still showed the expected value. G0 Z0 left the nozzle considerably higher than the printbed.

When either UBL was disabled (G29 D) or fwretract zhop set to 0 (via the LCD, from 0.2) he issue disappeared.

Configuration_adv.h.txt Configuration.h.txt

thinkyhead commented 6 years ago

Sorry about that! We've had some questionable patches to FWRETRACT lately. The Z hop should of course be reliably reversed. I'll take a look and see what's going on. If needed I'll post some code with extra debugging for you to test.

thinkyhead commented 6 years ago

Can I assume you're using genuine G10/G11 and not the auto-retract?

thinkyhead commented 6 years ago

I see there's already some debugging code in the FWRetract::retract method. Would you mind enabling it and giving it a test so we can see explicitly what's happening?

    // debugging
    SERIAL_ECHOLNPAIR("retracting ", retracting);
    SERIAL_ECHOLNPAIR("swapping ", swapping);
    SERIAL_ECHOLNPAIR("active extruder ", active_extruder);
    for (uint8_t i = 0; i < EXTRUDERS; ++i) {
      SERIAL_ECHOPAIR("retracted[", i);
      SERIAL_ECHOLNPAIR("] ", retracted[i]);
      SERIAL_ECHOPAIR("retracted_swap[", i);
      SERIAL_ECHOLNPAIR("] ", retracted_swap[i]);
    }
    SERIAL_ECHOLNPAIR("current_position[z] ", current_position[Z_AXIS]);
    SERIAL_ECHOLNPAIR("hop_amount ", hop_amount);
  //*/
thinkyhead commented 6 years ago

Due to the way that Z hop is implemented, it might simply be incompatible with bed leveling. (Because it "spoofs" the current Z position.) Hopefully there's some way to guarantee that it doesn't screw up the planner, but for the moment it may just have to be avoided.

joris57 commented 6 years ago

@thinkyhead Yes, using G10/G11, not auto-retract.

I commented out SERIAL_ECHOLNPAIR("] ", retracted_swap[i]);, due to a compile error and uncommented the debugging code in 2 locations in the source. Hope that is ok.

Here is the log: serial.log

thinkyhead commented 6 years ago

The log looks okay, so the logic is sound. I'll continue to delve.

joris57 commented 6 years ago

Did some more tests ...

Error does not occur with ABL, only with UBL.

Size of the error seems to be proportional to the bed level correction to be applied. So the error does not occur: -when the bed is level -when printing beyond the fade height -when bed leveling is switched off

thinkyhead commented 6 years ago

Does the same discrepancy appear if Z Fade is set to 0?

thinkyhead commented 6 years ago

I've posted a potential fix at https://github.com/thinkyhead/Marlin/archive/bf1_tool_change_debug.zip. This approach does the Z hop in a symmetrical manner which should result in a proportional move both up and down regardless of the Z height and fade. Give that a test, and if it still doesn't fix the issue then give it one extra test with Z fade disabled to see if that is the primary contributor.

thinkyhead commented 6 years ago

@joris57 — I've patched the Z hop issue (again) in the bugfix-x.x.x branches. I had forgotten to tell the planner about the change in Z position before doing the un-hop move. Hopefully this last patch solves the bug for real!

joris57 commented 6 years ago

@thinkyhead Back in the country... I tested this on yesterday's (10apr2018) bugfix-1.1.x and the issue still exists.

thinkyhead commented 6 years ago

the issue still exists.

Hmm, well then I just don't know. I'll have to do more testing.

Does the same discrepancy appear if Z Fade is set to 0?

joris57 commented 6 years ago

Yes, it also occurs after M420 Z0

thinkyhead commented 6 years ago

Hmm. Well all I know is that UBL follows a somewhat different methodology in its application of bed leveling correction to individual moves. It is possible that the Z lift on retract is being "eaten" by something in UBL so it doesn't restore on recover.

Above you mentioned that the error is proportional to the Z correction being applied. Does that error become less as the Z fade height is approached, or is it consistently the same amount of error until it gets past that height?

While I await your response I'll delve into the UBL movement code to see if anything obvious jumps out.

Roxy-3D commented 6 years ago

Well all I know is that UBL follows a somewhat different methodology in its application of bed leveling correction to individual moves.

Not seriously different. The only thing different is the correction is just applied to the destination coordinate in a move. If a move is split because it crosses mesh lines, the end of the first segment of the move becomes the 'destination'. And a Z-Height correction is done on that.

Stated differently... The starting Z-Height is assumed to be correct. Even if it is not correct, by the end of the first segment of the move... The Z-Height is correct.

It is possible that the Z lift on retract is being "eaten" by something in UBL so it doesn't restore on recover.

Yes... But if it is getting ate, my guess is there is some non-symmetrical correction or retraction happening.

thinkyhead commented 6 years ago

Not seriously different.

I wouldn't think so, especially since the move being affected is in Z-only.

Yes... But if it is getting ate, my guess is there is some non-symmetrical correction or retraction happening.

That was my thought, but I've made doubly sure that it's symmetrical so that the same fade factor should apply to the start and end heights. And fwretract.retract is meant to retain the same Z position on exit that it has on entry.

So far I can't find any reasonable cause why a Z-only move up should be asymmetrical to the down move, as if (according to the description) leveling is applied to one, but not the other.

Roxy-3D commented 6 years ago

So far I can't find any reasonable cause why a Z-only move up

Yeah... But you know... Those retractions should never recurse. It might make sense to add some

#define DEBUG_RETRACT_RECURSE
#define DEBUG_LEVELING_TRANSPOSE

sanity checks to the low level code. I bet we find exactly why there is a problem. And once you find it, it is easy to fix.

thinkyhead commented 6 years ago

If two G10 or two G11 happen in succession, the second one is ignored.

joris57 commented 6 years ago

@thinkyhead It looks like, contrary to what I claimed before, that even past the z-fade height the issue still occurs.

However, what I observed today, is the following:

I loaded the UBL grid with a highly exagerated value (to clearly notice the impact) with G29 P3 C5 R999

On bugfix-1.1.x of 20180320, the UBL grid value was applied to both G10 and G11 (with the same sign!), implying the bed dropped 10mm per G10/G11 instruction pair. Since there is no x/y movement, there should be no (additional) z correction at all.

However, on bugfix-1.1.x of 20180410, the UBL grid value was only applied to G11 (meaning the bed did not drop (other that the Zhop value) at G10 and dropped 5mm (minus he Z-hop value) at G11).

thinkyhead commented 6 years ago

I spent a while looking at the relationship between fwretract.retract and various parts of UBL tonight. While I didn't find anything definitive yet, I did patch a bug in planner.set_position_mm that would surely have affected this and many other functions in Marlin. Please give bugfix-1.1.x a test and see whether the patch has any impact on this issue. I'll pick up on debugging later, after a sleep break.

joris57 commented 6 years ago

Still the same : G10 seems to 'hop' normal, G11 seems to add the UBL grid value.

I also noticed that after the G10 hop the Z value, as reported by M114 is not adapted.

thinkyhead commented 6 years ago

After G10 the Z value should show no change in M114 (not even M114 D).

Now that I've dealt with the planner issue I will get back to deep debugging on fwretract.retract with UBL.

boelle commented 5 years ago

@joris57 still having issues with latest bugfix 2.0?

joris57 commented 5 years ago

@boelle No opportunity to test at the moment, so I'll close the issue.

github-actions[bot] commented 4 years ago

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.