MarlinFirmware
FTMotion: Call planner.synchronize() before modifying ftmotion configurations
Any changes to shaping frequency or shaper, and also changes to smoothing time produce layer shifts. This is because axis synchronisation calculates how much to delay each axis based on those properties, and when they are changed mid print, the printer needs to instantly jump to a different position.
Description
Requirements
Benefits
Configurations
Related Issues
In M493.cpp, the call to Stepper::ftMotion_syncPosition() was removed with the refactoring. Just set the update flag to true and call stepper.ftMotion_syncposition() at if (flag.update) ....
Just a precision prefer stepper.ftMotion_syncPosition(), if you print a resonance test gcode (change shaping frequency on every layer change) , planner.synchronize() is not enough to avoid layer shift.
We could call ftmotion.plan_runout_block() after synchronising there, that would add enough time for all smoothing and all shaping to finish stepping.
In my mind, 2 things are important: wait for the "planner to synchronize" and set the correct position. With stepper.ftMotion_syncPosition(), before all the changes, no runout_block was necessary. Just test it in the simulator.
I will soon do a pass on this to add setters, and have those setters do the synchronization, because we also have to duplicate this behavior for menu items, and setters will ensure everything inherits the appropriate amount of synchronization.
Why is setting the position important at all? That position is not even used inside ftmotion. I think it is all about adding a runout block so all echoes and smoothing settles
You can also switch from standard to FT_MOTION in an ongoing print. If you only call planner_synchronize , you will have a layer shift; that's why I added stepper.ftMotion_synchronize function a long time ago.
You can also switch from standard to FT_MOTION in an ongoing print. If you only call planner_synchronize , you will have a layer shift; that's why I added stepper.ftMotion_synchronize function a long time ago.
Should planner.synchronize itself include a call to stepper.ftMotion_synchronize as needed, so that all things that want to introduce a pause also ping FT Motion?
planner_synchronize is already in stepper.ftMotion_synchronize, but stepper.ftMotion_synchronize is only useful for FT_MOTION. We could just add position sync if FT_MOTION is active in planner_synchronize. One function to rule them all.
One more remark, calling planner.synchronize is only useful when FT_MOTION is active and there is an ongoing print.
The 2 functions could be :
#if ENABLED(FT_MOTION)
void Stepper::ftMotion_syncPosition() {
// Update stepper positions from the planner
AVR_ATOMIC_SECTION_START();
count_position = planner.position;
AVR_ATOMIC_SECTION_END();
}
#endif // FT_MOTION
in stepper.cpp and
/**
* Block until the planner is finished processing
*/
void Planner::synchronize() {
TERN0(FT_MOTION, const bool ft_busy = ftMotion.busy);
while (busy()) idle();
#if ENABLED(FT_MOTION)
if (ftMotion.cfg.active && ft_busy)
// Update stepper positions from the planner
stepper.ftMotion_syncPosition();
#endif
}
in planner.cpp.
Position is synchronized only if the printer is busy. Keep Stepper::ftMotion_syncPosition() to deal with private variable and function.
I don't get why this would make any difference:
AVR_ATOMIC_SECTION_START();
count_position = planner.position;
AVR_ATOMIC_SECTION_END();
The layer shifts come from axis synchronisation suddenly changing how much it delays axes, which then see an instantaneous jump in position
I think what we could do is scheduling a runout block after synchronising the planner, and then waiting for it to finish.
It's the only way to avoid layer shift when you switch between standard motion and FT_MOTION during an ongoing print. Currently, in the simulator, there is no layer shift when you change shaping frequencies or generator type, but it's present when we change smooting value, and it's not solved by adding planner.synchronize.
I see. The layer shifts when changing frequency, shaper or smoothing won't be visible in the simulator. FTMotion doesn't loose track of current position, it kind of travels back or forward in time, so the steppers skip
When I change smooting time when printing an input shaping test file in the simulator, the layer shift is obvious if the value is enough. Nothing happens with frequency or generator change. It's not a good idea to change parameters on the fly, but it's mandatory for calibration.
E Smoothing in particularly is something i want to change on the fly if the extruder doesn't sound great with a particular filament.
Thinking if i should close this pr since it will require more careful thought
Thinking if i should close this pr since it will require more careful thought
Some of it makes sense, like having more standardized setters that are shared by G-code and UI. I can cherry-pick other adjustments from this PR and merge them separately, whether you want to close it or not. But, we can work out the details on the Discord of what synchronization is needed, and where, when changing settings.
Right, I think what you did is great, but my naive calls to synchronize don't fix what I intended to fix
While tracking moving_axis_flags while trying to fix setting smoothing time during an ongoing print (the goal was to change smoothing time when the axis doesn't move), I realize that once the flag is set to true for an axis, thisd is for all the print. The probability of if (oldBusy && !busy) moving_axis_flags.reset(); is near zero. If the goal is to set a valid motion flag for each block,
// Set busy status for use by planner.busy()
const bool oldBusy = busy;
busy = stepping.is_busy();
if (oldBusy && !busy) moving_axis_flags.reset();
should be
// Set busy status for use by planner.busy()
busy = stepping.is_busy();
and
#define _SET_MOVE_END(A) do{ \
if (moveDist.A) { \
moving_axis_flags.A = true; \
axis_move_dir.A = moveDist.A > 0; \
} \
}while(0);
LOGICAL_AXIS_MAP(_SET_MOVE_END);
should be
#define _SET_MOVE_END(A) do{ \
if (moveDist.A) { \
moving_axis_flags.A = true; \
axis_move_dir.A = moveDist.A > 0; \
} \
else \
moving_axis_flags.A = false; \
}while(0);
LOGICAL_AXIS_MAP(_SET_MOVE_END);
For sure calling planner.synchronize doesn't fix changing smoothing time during a print, nor all my various tries.
@narno2202 — I've applied your PR #28214 but I'm not sure it's entirely correct, per your comment above. We want the moving_axis_flags and axis_move_dir flags to be set based on the fact that a block has motion in certain axes. We don't want to set it based on whether or not there is a STEP command in the ftm-command queue because these STEP commands can be very sparse for any given axis and we would miss the flag most of the time.
So, we still need to set these flags at the start of the processing of each block, and we need to clear them when each block has completed, (or at least, when the ftm-command queue has reached its end).
So let us continue to assess the way endstops and probes function with FTM and make sure we are getting the same kind of behavior that we get from standard motion when it comes to checking endstops and probes. Keep in mind that we need to be able to check these during printing too, and not just in very controlled situations.
@thinkyhead , with the previous code, once moving_axis_flag is set, it stays set for the entire print (tested many times with the same result). @dbuezas's code set axis_moving_flag when a block is loaded the PR does the same.
As I understand, axis_moving_flag is only important if there is a machine dysfunction and for aborting print from SD card if an endstop is triggered.
For homing only one block is loaded so the flag is always correct as the direction. axis_moving_flag could be reset when a block is discarded as it was the case for axis_did_move a long time ago, but buffer management was different and discard_planner_block_protected() is called just before a new block is loaded so it's not the good place.
Other option is to fall back to the old code which set the moving flag in the stepper ISR and assume that temperature ISR is enough to catch a triggered end stop. I can't test these because I'm using sensorless homing.
with the previous code, once
moving_axis_flagis set, it stays set for the entire print
I don't doubt your changes are an improvement but I suspect there are still going to be some issues.
As I understand,
axis_moving_flagis only important if there is a machine dysfunction…
Maybe the case for FT Motion, but with standard motion axis_did_move is tested any time endstops are enabled, so it is a necessary element of homing, probing, and abort-on-trigger.
For homing only one block is loaded so the flag is always correct as the direction.
That is currently true, but a homing or side-probing move could theoretically be done with leveling enabled, or there may be kinematics that require segmentation even for homing moves. So the endstop logic is agnostic about that, and all it cares about is whether the axis is currently moving towards the endstop.
I'll dive into the simulator and mess around for a while to get a better understanding of how these motion/direction flags are behaving. I'll see whether they are in sync with actual motion, persistent through the entire move, and so on.
@thinkyhead , I've made changes to use axis_did_move which is set in stepper.stepper::ftMotion_stepper(). The value is updated on every stepper ISR. Homing and probing is fine with my machine (remember I use sensorless homing). Could you have a look https://github.com/narno2202/Marlin/tree/FTM_Move_Dir_Test