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.27k stars 19.23k forks source link

[BUG] G34 not working properly #25560

Closed V1EngineeringInc closed 1 year ago

V1EngineeringInc commented 1 year ago

Did you test the latest bugfix-2.1.x code?

Yes, and the problem still exists.

Bug Description

When running G34 on any bugfix branch after the 14th when the bed levels itself it then does not move down again to deploy the probe properly.

The leveling works as expected, bed moves out of the way, probe deploys, it probes properly, does all three points, and on the third point it adjusts but immediately probes again without moving down enough causing errors. I forced it to clear and each time after they adjust it does the same thing and does not move to deploy the probe fully.

The 13th of this month works, 18th and 23rd do not.

Bug Timeline

between 3/14 and 3/18

Expected behavior

It should move to deploy the probe after adjusting the leveling.

Actual behavior

It adjusts the tram, then does not move to deploy the probe and tries to take the next reading.

Steps to Reproduce

G28 G34

Version of Marlin Firmware

bugfix 2.1.x

Printer model

MP3DP

Electronics

SKR Pro

Add-ons

No response

Bed Leveling

ABL Bilinear mesh

Your Slicer

Prusa Slicer

Host Software

SD Card (headless)

Don't forget to include

Additional information & file uploads

No response

V1EngineeringInc commented 1 year ago

Marlin.zip

classicrocker883 commented 1 year ago

check the differences of the code in the files, compare and contrast certain files like probe.cpp or probe.h.

there may be some values or parameters that are different in the versions that dont work.

hexitex commented 1 year ago

I can confirm this happens on z auto alignment as well, does three probes on left, makes a tiny lift then does normal lift, moves right, does three probes and after last one does a tiny lift but cannot deploy probe as too close to bed. This is new behaviour from three-ish weeks ago when it worked fine.

V1EngineeringInc commented 1 year ago

Glad it has been confirmed on printers other than ones I designed and set up the firmware for.

thisiskeithb commented 1 year ago

When running G34 on any bugfix branch after the 14th

Would you be able to test some bugfix-2.1.x commits to narrow this down?

The strategy is to find a commit from some point in the "distant" past where the feature works. Then, test a commit from halfway between that date and today…. And then you keep going to the commit halfway in between your "known to work" commit and your "known to be broken" commit until you find the exact commit where it broke.

If you started from a point 256 commits in the past, it would take no more than 8 tests to find the exact commit that broke it.

Most probe/tramming related commits have been within the past few weeks, so it shouldn't take long to find.

V1EngineeringInc commented 1 year ago

To the best of my abilities, it was the PR's on 3/14, but I do not see any way that directly affects this. I did test a handful so it is between 3/14 and 3/18 (nothing works after the 18th I was working backwards). I was hoping someone who made those or knows more might spot the issue without me testing each days PR's, I can If I need to but I do not have time for a couple days.

thinkyhead commented 1 year ago

I'm in the midst of trying to obtain consistent probe behavior across different probing procedures. I've added more debugging to get a better idea where and how to adjust things. Please test the latest code…

Repeat this procedure, if needed, to demonstrate inconsistencies. From these logs I can get a better idea of where the raises are occurring and apply additional refinements.

hexitex commented 1 year ago

I think this change is the culprit https://github.com/MarlinFirmware/Marlin/pull/25498 . Enabling HS mode on the bltouch solved the issue - I don't do c code very well but did spot some relationship with hsmode in a confusing custom tern function :) around lines 910+ in probe

classicrocker883 commented 1 year ago

module/probe.cpp line ~916

if ENABLED(BLTOUCH)

// Reset a BLTouch in HS mode if already triggered if (bltouch.high_speed_mode && bltouch.triggered()) bltouch._reset();

endif

// Use a safe Z height for the XY move const float safe_z = _MAX(current_position.z, SUM_TERN(BLTOUCH, Z_CLEARANCE_BETWEEN_PROBES, bltouch.z_extra_clearance()));

maybe this code can be looked at again.

line ~960

under if (!isnan(measured_z)) it appears asdo_z_clearance was changed from do_blocking_move_to_z .

i dont know the significance but there must be a reason why, im just saying maybe it should be looked over. just a hunch.

jamespearson04 commented 1 year ago

I think this change is the culprit https://github.com/MarlinFirmware/Marlin/pull/25498 . Enabling HS mode on the bltouch solved the issue - I don't do c code very well but did spot some relationship with hsmode in a confusing custom tern function :) around lines 910+ in probe

@hexitex must be right, all the other commits between the 14th and 18th don't affect the probing itself, just the points that UBL request be probed.

V1EngineeringInc commented 1 year ago

Sorry I forgot to add the extra detail, here is my output.

g34log.txt

V1EngineeringInc commented 1 year ago

I enabled HS mode and that did not help. It errored out in a whole different way, and tried to drive my Z through my extruder. It is still missing the clearing move.

G34 with HS.txt

jamespearson04 commented 1 year ago

Have you tried changing Z_CLEARANCE_MULTI_PROBE and Z_AFTER_PROBING to around 5 or higher? your Z_CLEARANCE_MULTI_PROBE is 1, which would be too low if your z nozzle to probe offset is over 1

ETA: I know that nozzle to probe should be automatically added, but with the recent changes it may have been affected, and it's easyish to test

jamespearson04 commented 1 year ago

Yeah, behaviour was changed in 49f1cc8: do_z_clearance(current_position.z + (big_raise ? 25 : Z_CLEARANCE_BETWEEN_PROBES)); becomes: do_z_clearance(Z_CLEARANCE_BETWEEN_PROBES);

I don't know if the intention was to do the addition inside the function instead, but that hasn't been done.

hexitex commented 1 year ago

Might be worth adding that PROBETOOL seems to have been introduced in the time window, which I think was defaulted to 0. Sorry don't have access to machine at the moment to run debugging, I will later

dwzg commented 1 year ago

Since 49f1cc8, my Nozzle is crashing into the printbed during G34. Had no issues with G34 beforehand and reverting that commit fixes the problem for me. Don't know if this is connected to this issue though

V1EngineeringInc commented 1 year ago

Have you tried changing

I tried changing all of them to 10, still doesn't work.

jamespearson04 commented 1 year ago

I think replacing line 797 in Marlin/src/module/motion.cpp should fix it:

    const float  zdest = _MIN(zclear, Z_MAX_POS);

With:

    float zdest = zclear + current_position.z;
    zdest = _MIN(zdest, Z_MAX_POS);

Unfortunately I can't test that for myself as I don't have dual z axes

V1EngineeringInc commented 1 year ago

Errored out, Screenshot 2023-04-01 155638

So I tried, const float zdest = _MIN(zclear + current_position.z, Z_MAX_POS); That compiled but the first point failed.

jamespearson04 commented 1 year ago

My bad try the above, but without const (above comment corrected in case anyone else tries it)

V1EngineeringInc commented 1 year ago

Okay , sorry for the delay, I started with a fresh copy of the bugfix, that fix you posted 2 above worked!

Now it is moving down twice, same as the g29 issue.

jamespearson04 commented 1 year ago

Interesting, the fix above shouldn't add any extra moves, just ensure the ones that were already there actually move upwards. Does it do a double move for G29 for you, or just on G34? also is that for every probe it does, or only some?

I suspect the commit that broke G34 fixed the double lift on G29, so if it's not there on G29, the same changes will just need applying over to G34.

jamespearson04 commented 1 year ago

To add, if it only happens on G34 now, it's likely because G34 provides extra clearance to guard against gantry slope causing collisions, so in that case it's worth considering whether or not it's undesireable behaviour. A description/video of when it does the double lift would be helpful (is it before every probe? or just before switching sides?)

jamespearson04 commented 1 year ago

More info on this: The bug occurs because of this "dirty trick", that makes Marlin think the bed is further than it is: https://github.com/MarlinFirmware/Marlin/blob/bugfix-2.1.x/Marlin/src/gcode/calibrate/G34_M422.cpp#L168-L169

In most situations this will shift the z coordinates up by around 3-6mm, so without the above fix, when probe.probe_at_point() calls do_z_clearance(Z_CLEARANCE_BETWEEN_PROBES) or do_z_clearance(Z_CLEARANCE_MULTI_PROBE) after triggering, where Z_CLEARANCE_MULTI_PROBE = Z_CLEARANCE_MULTI_PROBE = 5, the resulting clearance lift can be as low as 0-2mm.

As the probe is likely to trigger at a height of about 2mm anyway, the shift will move this up to around 5mm. As a result, the amount of clearance provided is reduced by this amount.

V1EngineeringInc commented 1 year ago

Interesting, the fix above shouldn't add any extra moves, just ensure the ones that were already there actually move upwards. Does it do a double move for G29 for you, or just on G34? also is that for every probe it does, or only some?

It actually seems to do it on all G28, 29, 34. I end up sitting at z=26.4mm after G28 and/or G34. I am going to change all of my Z moves to separate numbers so I can try to figure out what is actually going on. It used to sit at Z=10, My probe offset is 3.2 so for some reason that gets doubled to get the 6.4 part and most of my other Z moves are set to 10mm so it could be any of them. I made a log it that helps I am not seeing the info to figure it out.

Basic g28 log, 4-1-g28.txt

I will be testing more in a couple hours.

jamespearson04 commented 1 year ago

Yeah, I've been going through and doing some testing of my own to get to the bottom of it, I know what the cause is, just trying to find the best solution.

V1EngineeringInc commented 1 year ago

I just gave the BLTouch HS mode another try....much better. No added move, and it is super smooth.

thisiskeithb commented 1 year ago

Here's a copy of my comment from PR #25631:

I haven't tested a BLTouch-based config yet, but this PR solves the issue with the nozzle dragging across the bed after the final iteration of G34/Z_STEPPER_AUTO_ALIGN on a Biqu B1 SE Plus with dual+independent Z drivers & motors (stock only has a single Z motor/leadscrew) that has a strain gauge hotend (NOZZLE_AS_PROBE).

For reference, I'm using the following Z_CLEARANCE_* settings:

#define Z_CLEARANCE_DEPLOY_PROBE    2 // Z Clearance for Deploy/Stow
#define Z_CLEARANCE_BETWEEN_PROBES  2 // Z Clearance between probe points
#define Z_CLEARANCE_MULTI_PROBE     2 // Z Clearance between multiple probes
//#define Z_AFTER_PROBING           5 // Z position after probing is done

Full config: Biqu_B1_SE_Plus_Dual_Independent_Z.zip

thisiskeithb commented 1 year ago

Please test https://github.com/MarlinFirmware/Marlin/pull/25631 and report your findings in that PR.

V1EngineeringInc commented 1 year ago

Flawless!!!! I ran it with BLTOUCH HS mode and it was great. G34 had the right offest to account for potential bed tilt. G29 did the minimal, single, offset.

Back to perfect

classicrocker883 commented 1 year ago

changing #define Z_CLEARANCE_DEPLOY_PROBE from 10 to 5 fixed the similar issue using the Aquila.

no problem with 427 creality board, but it seems like the probe likes the value to be 5 or less.

thisiskeithb commented 1 year ago

changing #define Z_CLEARANCE_DEPLOY_PROBE from 10 to 5 fixed the similar issue using the Aquila.

You should be able to use 10/any value you want.

Please report your finding in PR #25631

V1EngineeringInc commented 1 year ago

Should I close this issue and just follow the PR?

jamespearson04 commented 1 year ago

I'd keep it open until the PR gets merged in, it's not technically resolved until then.

github-actions[bot] commented 1 year 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.