Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UBL in combination with fwretract zhop loses Z position #10200

Closed
joris57 opened this issue Mar 23, 2018 · 25 comments
Closed

UBL in combination with fwretract zhop loses Z position #10200

joris57 opened this issue Mar 23, 2018 · 25 comments

Comments

@joris57
Copy link

joris57 commented Mar 23, 2018

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
Copy link
Member

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
Copy link
Member

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

@thinkyhead
Copy link
Member

thinkyhead commented Mar 23, 2018

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
Copy link
Member

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
Copy link
Author

joris57 commented Mar 24, 2018

@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
Copy link
Member

thinkyhead commented Mar 25, 2018

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

  • Can you also test to see if Z Fade has anything to do with it?
  • Have you also tested with ABL?

@joris57
Copy link
Author

joris57 commented Mar 26, 2018

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
Copy link
Member

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

@thinkyhead
Copy link
Member

thinkyhead commented Mar 28, 2018

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
Copy link
Member

@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
Copy link
Author

joris57 commented Apr 11, 2018

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

@thinkyhead
Copy link
Member

thinkyhead commented Apr 12, 2018

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
Copy link
Author

joris57 commented Apr 12, 2018

Yes, it also occurs after M420 Z0

@thinkyhead
Copy link
Member

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
Copy link
Member

Roxy-3D commented Apr 12, 2018

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
Copy link
Member

thinkyhead commented Apr 12, 2018

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
Copy link
Member

Roxy-3D commented Apr 13, 2018

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
Copy link
Member

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

@joris57
Copy link
Author

joris57 commented Apr 14, 2018

@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
Copy link
Member

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
Copy link
Author

joris57 commented Apr 16, 2018

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
Copy link
Member

thinkyhead commented Apr 18, 2018

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.

thinkyhead added a commit that referenced this issue Sep 22, 2018
@boelle
Copy link
Contributor

boelle commented Feb 20, 2019

@joris57 still having issues with latest bugfix 2.0?

@joris57
Copy link
Author

joris57 commented Mar 3, 2019

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

@joris57 joris57 closed this as completed Mar 3, 2019
@github-actions
Copy link

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.

@github-actions github-actions bot locked and limited conversation to collaborators Jul 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants