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

Fix bed crash during selftest #1703

Open
wants to merge 2 commits into
base: MK3
from

Conversation

Projects
None yet
2 participants
@leptun
Copy link

commented Apr 6, 2019

This PR fixes crashing of the head during selftest when it lowers Z to find point.
Changes:
-Lower stepping of z to avoid touching when pinda is mounted higher in holder (Live Z < 1000)
-Does not use hard coded value for the bed point location.
The point was still off on my MK3 on Y axis, so I added an offset of 4mm on Y to the point. This fixed the issue.

This needs testing on the MK25(S) and MK3S.
Issue #1680

@wavexx

This comment has been minimized.

Copy link

commented Apr 6, 2019

Is the pinda expected to be exactly above the mark while lowering? (I didn't pay attention to this - I assumed it wouldn't matter except for calibration)

@leptun

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

This is so that without steel sheet it triggers higher and does not touch/push the bed with the nozzle

@leptun

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

Pinda gets triggered not only by the points, but also the traces on the bed. However it is lower on the traces and it might slightly crash and push the bed.

Attached are hex files for testing. Not sure if this would work on MK2 though. If anyone can test that would be great.
FW370-Build2201_fix_selftest_Z_crash.zip

@wavexx

This comment has been minimized.

Copy link

commented Apr 6, 2019

If you look at the earliest report I've found (#605, linked video: https://www.youtube.com/watch?v=q1546BJgfIQ&feature=youtu.be) that definitely looks like the case.

I'm waiting for a print to finish and I'll give this a test later on.

@@ -7386,9 +7393,6 @@ static bool lcd_selfcheck_pulleys(int axis)
((READ(Y_MIN_PIN) ^ Y_MIN_ENDSTOP_INVERTING) == 1)) {
endstop_triggered = true;
if (current_position_init - 1 <= current_position[axis] && current_position_init + 1 >= current_position[axis]) {
current_position[axis] += (axis == X_AXIS) ? 13 : 9;

This comment has been minimized.

Copy link
@wavexx

wavexx Apr 6, 2019

what was the original intention here?

This comment has been minimized.

Copy link
@leptun

leptun Apr 6, 2019

Author

Those were the hard coded values for mk2 after it checked for loose pulleys. The selftest does as follows:
MK2/MK2.5(S) : Home X -> Check pulleys and move X axis to Z probe position -> The same thing for Y axis -> Raise Z -> lower Z to point
Newer printers : Home X -> Check Axis length X-> Check Axis length Y -> move to point position while raising Z -> lower Z to point
With this PR the head is moved to the point while being raised AFTER checking X and Y also on older printers without using hard coded values.

@wavexx

This comment has been minimized.

Copy link

commented Apr 6, 2019

I gave this a shot in two runs: one without adjusting the position (just to test the half-stepping) and one as-it-is: much better in every respect.

Z stops at a safe distance now in any case.

With the position correction Y alignment is perfect, while X looks slightly off (probe is at +1mm in my case). Did you simply correct those 4mm by measuring? I wonder if the constants are wrong to begin with?

@leptun

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

The values used are the uncorrected values used in XYZ calibration. Without XYZ calibration the offset is unknown, so the point location is off. I could use corrected values, but they are unavailable right after you assemble the printer or you run wizard. What I could do and probably will is checking weather stored data is valid, and if yes use the corrected values. If not then use this 4mm offset

@wavexx

This comment has been minimized.

Copy link

commented Apr 6, 2019

What about using the calculated end-stop distance for XY and the know bed size instead?

This should be better than a simple offset from the bottom-left and does not make assumptions about the build.

@wavexx

This comment has been minimized.

Copy link

commented Apr 6, 2019

Just to make the it clear, I mean calculating the offset from the center of the axis instead of the left offset only.

@leptun

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

The problem is that the bed can be offset by assembly by the belt holder.
On Mk2 - Mk2.5S (and in between) the min position can be moved by a few mm and MAX position can't be found
On Mk3 and Mk3S the MIN and MAX positions can be shifted by the same piece while still having the same axis length
Edit: Might not be the same on MK3S, but the same idea applies. I'll try to implement it as i suggested before. I'll push the changes if it compiles, but can't test them. Can you do it?

@leptun

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

Also on mk3/s the X axis min/max position can be affected by the size and position of the bundle of wires.

@wavexx

This comment has been minimized.

Copy link

commented Apr 6, 2019

I didn't think about this.

Then I wouldn't change anything really: I wouldn't use any stored value for a self-test procedure. It's much better to have a consistent test in any scenario.

@leptun

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

With what I proposed to change it would work as follows:
-If printer is new, then no data available so use 4mm offset
-if you run wizard the data will be erased before selftest; 4mm offset
-if printer is assembled and calibrated and you run selftest you shouldn’t have a problem with using those stored values.
-if you made some changes to the printer you should run wizard anyway to recalibrate the printer, thus not using what might be wrong data. 4mm offset

If for some reason you run selftest and the values are wrong, you will simply probe next to the point and push the bed a little. Undesirable, but not that problematic.

There is a bigger issue though. If you run selftest from menu after succeeding the printer will autohome. This should be disabled as there is no point where it is probing. Will try to address this issue as well. If you find the piece of code responsible for this, please let me know

@wavexx

This comment has been minimized.

Copy link

commented Apr 6, 2019

The way I see it, a self-test (and more generally a calibration procedure) should never rely on pre-recorded data for consistency. You know the test is going to behave always in the same way, irregardless of the values stored. This makes debugging a lot easier. It's a little off, but it can never be more off by any other reason.

@wavexx

This comment has been minimized.

Copy link

commented Apr 6, 2019

Isn't the homing done just a few lines later in ultralcd.cpp:7030?

@leptun

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

Hmmm, that might be it. It is somewhat weird that it is in the middle of the selftest. Now thinking about it, maybe it is queued to be processed after it finishes. Making the changes now.

@leptun

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

I suppose you have a MK3 printer. Could you please test how this hex file performs selftest. It should do as I said above. It would be best if you could test the scenarios I listed.

I will also continue to test tomorrow morning.
FW370-Build2201-1_75mm_MK3-EINSy10a-E3Dv6full-EN_ONLY.zip

@wavexx

This comment has been minimized.

Copy link

commented Apr 6, 2019

It's an MK3, but it's not so "original" anymore ;). Can I just rebuild with the current PR?
I wonder if the homing could be needed during the wizard, after the self-test part has finished?

@leptun

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

I have made changes that I am not sure work. I'll do it, but be warned I am not sure how it is going to do this.
As for the homing, it was done as to home all if wizard was not active. If it was active, it wouldn't home. Now it doesn't home at all and is just lifted more before checking hotend, bed and filament sensor.

Edit: meant to say wouldn't

@wavexx

This comment has been minimized.

Copy link

commented Apr 6, 2019

I'm not too keen on f09d719. Overall, think about it: I would have never discovered the issue in the first place: it would have never crashed in my case. I'd rather not use the existing offsets during the self-test.

@leptun

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

Well you would have crashed anyway before any of these changes since the values did not use existing offset data

@wavexx

This comment has been minimized.

Copy link

commented Apr 6, 2019

@leptun

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

I’ll revert the last commit then. It is true it should be dumb, but I think the first two commits should still be applied

@wavexx

This comment has been minimized.

Copy link

commented Apr 6, 2019

@leptun leptun force-pushed the leptun:MK3_fix_selftest_Z_crash branch from dd6c154 to 8e1948c Apr 6, 2019

@wavexx

This comment has been minimized.

Copy link

commented Apr 15, 2019

Ok, sorry for the delay but I could test it now. No homing performed, just Z being raised.
I think it's perfect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.