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 for dualx sudden layer shift (#19151) #19756

Merged
merged 4 commits into from
Oct 16, 2020

Conversation

nb-rapidia
Copy link
Contributor

@nb-rapidia nb-rapidia commented Oct 15, 2020

Description

As described in #19151, sometimes a "sudden layer shift" occurs when printing with DUAL_X_CARRIAGE enabled. After extensively debugging using an emulator, we identified that the cause was most likely related to the stepper.set_directions() call in dual_x_carriage_unpark() in motion.cpp.

What's suspicious about this line is that it sets the directions even if no unpark operation occurs, meaning that the directions are set on every g0/g1 command. Furthermore, the stepper ISR also invokes set_directions, meaning there is potentially some risk of a concurrency error (though admittedly it's hard to see exactly how.). This could be fixed by inserting planner.synchronize() before the call to set_directions in dual_x_carriage_unpark(), but this would essentially induce synchronous motion (due to this routine running every g0/g1 call.)

The fix implemented in this PR is to move set_directions into the switch statement, where it will only run if an actual unpark operation occurs. A planner.synchronize() call is inserted before set_directions in the DXC_AUTO_PARK_MODE case just in case it turns out that there is a potential concurrency issue here. (Perhaps somebody with more experience with this particular code can decide whether or not there is a risk and if this change would pose a problem.)

For some reason, this fix alone caused small circles to cause some jitter when printing small circles. It's not clear why this would be related, but we double-checked and confirmed that the jitter only occurs when this fix is used. Given that the change only affects the direction bits, we decided that (for reasons not fully understood), the most likely explanation is that the count_direction bits are somehow not being set correctly at the start of a move. So we force the directions to be set again at the start of each move in stepper.cpp regardless of whether or not it would otherwise be necessary.

We have so far run 10 prints using this fix, and we did not once see a layer shift occur. Our statistics (which we can elaborate on if needed) indicate that there is at least a 90% chance that this change did indeed resolve the sudden layer shift error. However, the exact cause of the sudden layer shift bug is still not fully understood, so we'd greatly appreciate if somebody with a better understanding of the firmware could look at the proposed change and make a guess as to why it seems to resolve the problem.

Benefits

Likely fixes the sudden layer shift that is frequently observed when using dualx carriages. #19151

Configurations

The same configurations used as in #19151. (Link.)

Related Issues

#19151

@thinkyhead
Copy link
Member

Thanks for the patch and the long explanation! It has been known for a while that the direction state of the stepper was getting lost or set incorrectly, and that "forcing" the direction state in some places would solve issues, but I wasn't sure where to apply the changes.

One of the things that Marlin does is to write the DIR pin state only when it is changing, according to saved state bits. It might be good to add "force-set" flags too, so that when a tool-change is done and a new motor is selected, the DIR pin state can always be written the first time. Although, in this case, I think the real solution for Dual-X is to make sure that the DIR pin bits correctly go with the actual stepper driver, and not just the axis.


I've also wanted to run the firmware under simulation to see what's going on, but haven't put together a Dual-X build yet. One thing we can try is to add "assert" checks to make sure the DIR state bits and actual DIR pin states are what we want at various points in the code, and when one of those asserts inevitably fails we'll be closer to knowing where the DIR state is getting lost. My theory, as stated above, is that we just have to make sure that X1 and X2 each get their own direction bits, and that will likely solve it, although initializing the DIR bits on a tool-change is not a bad idea.

@thinkyhead thinkyhead merged commit 418b3e5 into MarlinFirmware:bugfix-2.0.x Oct 16, 2020
@Roxy-3D
Copy link
Member

Roxy-3D commented Oct 16, 2020

Merged!!!! I'll move my Configuration.h settings over and start testing ASAP!

Thank You @nb-rapidia !!!!!

Zorchz pushed a commit to Zorchz/Marlin-1 that referenced this pull request Oct 17, 2020
Zorchz pushed a commit to Zorchz/Marlin-1 that referenced this pull request Oct 17, 2020
thinkyhead added a commit to thinkyhead/Marlin that referenced this pull request Oct 21, 2020
Speaka pushed a commit to Speaka/Marlin that referenced this pull request Oct 23, 2020
Speaka pushed a commit to Speaka/Marlin that referenced this pull request Nov 2, 2020
vgadreau pushed a commit to vgadreau/Marlin that referenced this pull request Dec 9, 2020
tharts pushed a commit to tharts/Marlin that referenced this pull request Jan 6, 2021
kpishere pushed a commit to kpishere/Marlin that referenced this pull request Feb 19, 2021
W4tel-BiDi pushed a commit to W4tel-BiDi/Marlin that referenced this pull request Apr 5, 2021
thinkyhead added a commit to thinkyhead/Marlin that referenced this pull request Apr 28, 2021
thinkyhead added a commit to thinkyhead/Marlin that referenced this pull request Apr 29, 2021
thinkyhead added a commit that referenced this pull request Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants