Skip to content

[2.0.x] Improve sync of planner / stepper position, asynchronous G92#10614

Merged
thinkyhead merged 4 commits intoMarlinFirmware:bugfix-2.0.xfrom
thinkyhead:bf2_synced_planner_set_position
May 6, 2018
Merged

[2.0.x] Improve sync of planner / stepper position, asynchronous G92#10614
thinkyhead merged 4 commits intoMarlinFirmware:bugfix-2.0.xfrom
thinkyhead:bf2_synced_planner_set_position

Conversation

@thinkyhead
Copy link
Member

@thinkyhead thinkyhead commented May 4, 2018

In relation to #10611

Background: Currently if we set the planner position directly, as with G92, we have to call stepper.synchronize so 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.synchronize at the start of G92 and the call to stepper.set_position_mm at the end of G92.

  • Problem 2: Many slicers insert G92 E0 at the start of each layer, presumably to keep the G-code smaller and to avoid the E coordinate growing too large to fit in a float (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 E also invokes stepper.synchronize, and this has been found to produce seams in the print due to the extra pause between layers. Some slicers may even use G92 E after every meter of filament, giving even more strange pauses. To make all axes recoverable, including E, the synchronize is necessary even for G92 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 G92 didn't require a call to stepper.synchronize then both problems would be solved.

Solution: Instead of requiring the stepper.count_position to be set at the same time as planner.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 block steps values directly to the step_count then throw away the block. This allows G92 to work without needing to call stepper.synchronize.


Also:

  • Audit code that calls stepper.synchronize and, 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.
  • Fix fwretract.retract (finally?) by taking advantage of this updated method to set the planner position.

@thinkyhead thinkyhead force-pushed the bf2_synced_planner_set_position branch 9 times, most recently from 54be647 to a4c55c0 Compare May 4, 2018 01:59
@thinkyhead thinkyhead force-pushed the bf2_synced_planner_set_position branch 12 times, most recently from 5dd9906 to ef6546c Compare May 4, 2018 09:44
@GMagician
Copy link
Contributor

Ignore me if I say things wrong, I have not enough knowledges, but what about saving active tool offset, G92 component as offset (as int32 to avoid overflows) and simply add these values as soon as something is added to planner?

@thinkyhead
Copy link
Member Author

thinkyhead commented May 5, 2018

Thanks for the question.

I rewrote all the coordinate handling code around the time of Marlin 1.1.6 so that the same physical position always ends up giving the same stepper counts. So unless NO_WORKSPACE_OFFSETS is enabled, the G92 offset for XYZ doesn't propagate to the planner or stepper counts. The G92 offset is subtracted from coordinates sent to Marlin with any commands that take XYZ coordinate values (G0, G1, G2, G3, etc.) Then all code in Marlin operates on native machine coordinates.

Although the XYZ planner and stepper positions will only be updated by G92 when NO_WORKSPACE_OFFSETS is turned on, the E axis should always be written to the planner/stepper positions if changed.

This PR isn't about the offset itself, but about the timing of any modification to the stepper's XYZ and E counts done through stepper.set_position, which G92 (and some other code) uses. This PR removes the need to call stepper.synchronize and wait in a loop for the correct moment (and which can still miss it). This change allows the stepper ISR to handle the update at the precisely correct moment and never get out of phase.

If we were to allow for an E "workspace offset" then behind the scenes after G92 E0 we'd be adding an ever-increasing float to E on input, or else adding an ever-increasing long to Esteps in the planner. This of course goes against the whole point of G92 E0, which is to prevent a loss of float precision at each power of 10, or else an overflow of the 32-bit-signed stepper position. As mentioned, neither of these is really a concern, but we still have to contend with G92 E0 and the timing of that change. Even if we used a long offset, it would still need to be altered at the right moment.

This PR solves the timing problem in a universal manner for all calls to stepper.set_position, allowing for any future code that might use it to work properly too.

@thinkyhead thinkyhead force-pushed the bf2_synced_planner_set_position branch from ef6546c to 84b0253 Compare May 5, 2018 10:44
@thinkyhead thinkyhead force-pushed the bf2_synced_planner_set_position branch from 84b0253 to 325d387 Compare May 5, 2018 23:21
@thinkyhead thinkyhead force-pushed the bf2_synced_planner_set_position branch from 325d387 to 62e7a9c Compare May 6, 2018 06:24
@thinkyhead thinkyhead merged commit f30241b into MarlinFirmware:bugfix-2.0.x May 6, 2018
@thinkyhead thinkyhead deleted the bf2_synced_planner_set_position branch May 6, 2018 08:10
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.

2 participants