[1.1.x] Improve sync of planner / stepper position, asynchronous G92#10615
Merged
thinkyhead merged 4 commits intoMarlinFirmware:bugfix-1.1.xfrom May 6, 2018
Merged
Conversation
9684497 to
e740832
Compare
e740832 to
d1e5d15
Compare
d1e5d15 to
e8779e7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In relation to #10611…
Background: Currently if we set the planner position directly, as with
G92, we have to callstepper.synchronizeso the stepper positions aren't updated in the middle of stepping, which would lead to the stepper counts being out of phase. The rationale for this is that setting the stepper positions in the middle of a move breaks forward kinematics, which need to be relied upon to accurately get the high-level position from the steppers.Problem 1: It's still possible for steps to get out of sync with the planner, in spite of best intentions. The stepper method to set the position does turn off the stepper interrupt, but this comes too late. It's remotely possible that the stepper ISR could start processing the next block in-between the
stepper.synchronizeat the start ofG92and the call tostepper.set_position_mmat the end ofG92.Problem 2: Many slicers insert
G92 E0at the start of each layer, presumably to keep the G-code smaller and to avoid the E coordinate growing too large to fit in afloat(or to maintain its precision).(This concern is overblown. With 500 Esteps/mm, you could go through ~4.3km of filament before the stepper E value will grow too large to fit into an
int32_t.)The bigger problem is that
G92 Ealso invokesstepper.synchronize, and this has been found to produce seams in the print due to the extra pause between layers. Some slicers may even useG92 Eafter every meter of filament, giving even more strange pauses. To make all axes recoverable, including E, the synchronize is necessary even forG92 E. The most simple solution is to be loose about E and only synchronize for XYZ, but there may be good reason that the synchronize was added for E.If
G92didn't require a call tostepper.synchronizethen both problems would be solved.Solution: Instead of requiring the
stepper.count_positionto be set at the same time asplanner.position, it makes more sense to let the stepper ISR do this at the best moment. This PR accomplishes this by adding a block flag that instructs the ISR to copy the blockstepsvalues directly to thestep_countthen throw away the block. This allowsG92to work without needing to callstepper.synchronize.Also:
stepper.synchronizeand, where possible, either move the call to a later point to use up some cycles in code that needs to run anyway, or remove the call if it is superfluous.fwretract.retract(finally?) by taking advantage of this updated method to set the planner position.Counterpart to #10614