Fix FT Motion layer shifting

[narno2202]

Description

Changing the shaper frequencies during a print causes a layer shift due to the reset of the ft-motion parameters. The calibration test print is impossible. Removing the reset flag from M493 A and M493 B, restores correct motion.

Benefits

Allow shaping frequencies change during print without layer shifting. Expected behavior.

[thinkyhead]

I'm very curious to understand what it is about the reset –originally prescribed by Ulendo– that leads to the layer shift. It should be avoidable due to the M400 pause at the top of M493, but maybe some element of FT Motion is still underway when that call returns. Have we got any clue as to what is actually happening that makes this layer shift occur? Is it due to a sudden speed change, or an out-of-date or late update to the stepper position? Could it be related to the new motion architecture's use of FxdTiCtrl::loop to do userland maintenance of the motion system? I'd like to do a deeper dive to understand all the issues that can lead to the generalized symptom of "layer shift," and figure out whether we need to do a complete removal of the prescribed reset or whether we need to patch the motion system according to a deeper knowledge of the dynamics and achieve reliable and smooth transitions. Unfortunately the coders from Ulendo who originally submitted the code have not joined in to act as consultants, or they might have some insight from their own experiences. We may just have to get out ye olde ST-Link debugger and put on our lab coats.

[narno2202]

I totally agree, we need to understand the mecanism leading to layer shift. I did some more testing and found few things.
If we want to use flag.reset_ft within M493 S without layer shift during a print (unless it's ftMotionMode_DISABLED) trajMod().reset() in the reset() function solves the issue. But i'm still convinced that reset() is useless on frequency change ( all the shaping parameters are overwritten on update) and probably on mode change (never had problem on daily use).
Using M493 S0 when printing causes a layer shift equivalent to 32 steps on my printer. I change steps/mm in Configuration.h, the layer shift is still of 32 steps.
That's all for now : it's race time

[danbrianwhite]

@ulendoalex
Any help from Ulendo's side with understanding these details of FT-Motion?

[ulendoalex]

@danbrianwhite Hi Daniel, thanks for bringing this to our attention. FT-Motion uses several sets of buffers to create the motion data; I suspect on mode / frequency change something is not handled correctly within these buffers. We'll investigate this on our end. Could you please share the gcode used to perform this test, in case we need it to replicate the issue ?

[danbrianwhite]

@narno2202 do you have the failing gcode and can you upload it here?

[narno2202]

Here is the ringing tower gcode generated with Prusaslicer.
Just for remembering :

• always layer shift when changing frequencies during a print if reset() is called in M493. In my opinion, useless function here as all the values are overwritten, no more layer shifting after removal
• always layer shift during a print with M493 S0
• again layer shitt when changing mode during a print with reset() function including trajMod.reset()

For now on all my tests, reset() must be avoided after a print has begun (no problem with G28 in the iniital gcode and in a static phase). Switching back to standard motion with M493 S0 after a print has begun always causes a layer shift (no problem with G28 in the iniital gcode, ftMotionMode is disabled for homing and initialized after).

With these warnings and my modified code, i use FT_MOTION for all my prints with success.

ringing_towerfull.zip

[danbrianwhite]

@ulendoalex are the files and info from @narno2202 sufficient?

It would also be awesome if Ulendo could also test out the updates from the main Marlin repo and see if they run as expected compared to the original source.

[narno2202]

Ulendo just update their repository. I integrate all the ft_motion code into Marlin with some of the existing logic for multiaxis and multistepper. The update should solve layer shifting during print. Testing is in progress.

[ulendoalex]

@danbrianwhite @narno2202

Thank you for the files, we were able to root cause the issue. When shaping is applied, the trajectory "lags" a little behind the desired trajectory that is determined from the gcode instructions / planner. When the reset function is called, the difference in position between desired and the lagging trajectory is lost, and results in the layer shift you were seeing.

I'm linking a commit with proposed updates to fix this. Not invoking reset is a potential solution as well, but there are some cases that could still cause shifting, like a drastic change in the shaper frequency (i.e. 20 Hz to 80 Hz). The linked update changes how motion for a single block left in the planner is handled - it now aims to guarantee the lagging trajectory "catches up" when needed. With the update I was able to print the ringing_towerfull file without issue.

M493 S0 should be avoided / blocked during a print; there hasn't been any consideration to switching between FTM and classical motion control, so it is not expected to work.

Lastly, we have been using 06-20 distribution for the past week and have not noticed any issues with the FTM implementation so far.

PS:
The update is based on bugfix-2.1.x distribution date (2023-06-20). There are some unrelated updates as well.
S2AUlendo/MarlinCollaborative@0f55851

[thinkyhead]

Thanks for jumping into the fray @ulendoalex — I had a feeling there was some part of the motion that was not completing before the mode / parameter changes, and planner.synchronize wasn't enough to work around it. I also wasn't sure that we could figure out a comprehensive solution in a timely manner using the debugger / simulator even with the strongest coffee available.

After merging your runout-in-progress fixes from #26020 how much of this PR is still needed?

Do you think we need to add anything extra to planner.synchronize (M400) to ensure that it waits for the last queued block to complete before it returns? Simply waiting for the planner queue to empty works for regular Marlin motion because it keeps the block in the queue right up until the last pulse, but that may not be the case for FT Motion.

[thinkyhead]

Lastly, we have been using 06-20 distribution for the past week and have not noticed any issues with the FTM implementation so far.

That's good to hear! It was a pretty big refactor to put together 9-axis and multiple extruder support, and I always worry that I'll miss something in such a conversion.

The next official release is getting closer. I just have a bit more to do on the endstops refactor, then a few obscure bugs and typos to chase down, and then we'll be in the home stretch for version … 2.2.0. (Was going to be 2.1.3 but this is turning out to be a big update!) Feature-freeze is pretty much already in effect, so we'll do just a couple more weeks of testing and bug-fixes and tag the new release as soon we have a reasonable confidence level. I've been on a bit of a code modernization trip in recent weeks, but with 3D printer firmware field getting more competitive, I want this next release to be as bug-free as possible, so it's time to get back to rigorous testing and patching.

[thinkyhead]

@ulendoalex — Regarding the linked commit, it contains this line:

max_intervals = FTM_BATCH_SIZE * ceil((FTM_FS / FTM_BATCH_SIZE) * 0.0f);

It looks like that could simply be replaced with:

max_intervals = 0;

Or, have we already essentially dealt with the contents of that commit with #26020? I'm comparing these two sets of changes to see where they overlap and where they differ.

[thinkyhead]

@ulendoalex — I went ahead and brought in the linked commit S2AUlendo/MarlinCollaborative@0f55851, updating it to account for extra axes. As noted above, it appears that max_intervals should just be 0 for the given case, so I also made that change. I then removed the extra planner.synchronize() call at the end of M493 because we already call it at the start of M493 before applying any mode or parameter changes.

Please advise whether all the updates now incorporated into this PR are appropriate in light of the fact that they supplant parts of #26020, or whether some elements of both 03c0d83 and #26020 are required for completeness.

[narno2202]

If you want to test , i create a new branch im my repository with last Uldendo's code merged into last Marlin bugfix. I remove and change some code in marlin's originalft_motion.cppand stepper.cpp to be as close as possible with Ulendo's code

[thinkyhead]

Blocking M493 S0 -> S1 or M493 S1 -> S0 mode changes (disabling or enabling FTM) during a print should be kept…

@ulendoalex —

The concept of "during a print" is kind of tricky. A print job is considered active whenever a file on the SD Card / Flashdrive is being executed, or whenever the host has started the print job timer, or whenever the print job timer starts in response to hotend heating. Naturally, we want to allow M493 S0 or S1 to be called in a G-code file being run from the SD Card.

In general, it should be very rare for a G-code file or host console to send M493 at the wrong time, but we should include some way to handle it elegantly so that it can be called at any point and not be entirely ignored. The call to planner.synchronize at the start of M493 ensures that the planner will be empty before FT Motion gets enabled or disabled or mode-changed. Theoretically, it should behave just as though it is starting over from scratch.

What is it that can conceivably break if the mode is changed in-between moves during a print job, as opposed to changing modes when you're just messing about with G1 commands outside of a print job? Do we need to copy some extra states between FT and the standard motion system to ensure they are in synchronization with one another?

[thinkyhead]

I took a look at the other commits under this PR. I think the current state is good: planner.synchronize is called with M493 changes, reset_ft is still set set, and now it is handled properly in FTM).

Actually, at the moment reset_ft is not being set for frequency changes with M493 A/B. But maybe that is fine…?

[narno2202]

Perhaps, i got it !!!. When M493 S0 is thrown during a print, setting stepper axis position with last planner position ie stepper.set_axis_position(X_AXIS, planner.position.x) suppresses layer shifting and the print continues as it should. The code is inserted in the switch (newmm)... case of M493.cpp. Code used just for testing onyl with XYZE :

case ftMotionMode_DISABLED: stepper.set_axis_position(X_AXIS, planner.position.x);
         stepper.set_axis_position(Y_AXIS, planner.position.y);
         stepper.set_axis_position(Z_AXIS, planner.position.z);
         stepper.set_axis_position(E_AXIS, planner.position.e);
case ftMotionMode_ENABLED: ...

[ulendoalex]

@narno2202

3. Calibration test print : no layer shift on frequency or mode change (unless M493 S0 :) )  but a little pause on every change

The pause is expected.

4. During all the moves, i ear a regular noise ("clang  clang") on all axes with a little sudden move

Is this new since integrating the fix? Could you please provide details on how to recreate this? I did not see / notice this in our testing.

@thinkyhead

I took a look at the other commits under this PR. I think the current state is good: planner.synchronize is called with M493 changes, reset_ft is still set set, and now it is handled properly in FTM).

Actually, at the moment reset_ft is not being set for frequency changes with M493 A/B. But maybe that is fine…?

You are right. It should be reverted so that it is being set.

Blocking M493 S0 -> S1 or M493 S1 -> S0 mode changes (disabling or enabling FTM) during a print should be kept…

@ulendoalex —

The concept of "during a print" is kind of tricky. A print job is considered active whenever a file on the SD Card / Flashdrive is being executed, or whenever the host has started the print job timer, or whenever the print job timer starts in response to hotend heating. Naturally, we want to allow M493 S0 or S1 to be called in a G-code file being run from the SD Card.

In general, it should be very rare for a G-code file or host console to send M493 at the wrong time, but we should include some way to handle it elegantly so that it can be called at any point and not be entirely ignored. The call to planner.synchronize at the start of M493 ensures that the planner will be empty before FT Motion gets enabled or disabled or mode-changed. Theoretically, it should behave just as though it is starting over from scratch.

What is it that can conceivably break if the mode is changed in-between moves during a print job, as opposed to changing modes when you're just messing about with G1 commands outside of a print job? Do we need to copy some extra states between FT and the standard motion system to ensure they are in synchronization with one another?

Understood. There isn't anything in FTM modes depending on print job status. The suggestion to disable the switching was a solution to prevent the layer shift issue from happening. I see now why this isn't easily defined.

Yes, I do believe something is missing that would keep the motion systems in sync. Perhaps @narno2202 has already solved it in their latest comment. I will suggest if the code to synchronize the motion systems ends up too complex that the user is instead locked in to one or the other at compile time.

[narno2202]

@ulendoalex , thank's for you answer. Ok for the pause at layer change.

Regarding the noisy motion, It's a periodic noise which is louder when speed increase. In my memory, this was not the case before your fix and it's not present in current bugfix-2.1.x with ft_motion enabled.

As said before, i merge your ft_motion code in Marlin keeping multi axis, adding @thinkyhead remarks and with the code in my last comment : no more layer shifting when mode or frequencies change during a print without the need for reset() function. This is also the case when enabling or diasabling ft_motion during a print.

@thinkyhead , if you want, i can do a new PR with all these changes for better testing.

[ulendoalex]

@ulendoalex , thank's for you answer. Ok for the pause at layer change.

Regarding the noisy motion, It's a periodic noise which is louder when speed increase. In my memory, this was not the case before your fix and it's not present in current bugfix-2.1.x with ft_motion enabled.

As said before, i merge your ft_motion code in Marlin keeping multi axis, adding @thinkyhead remarks and with the code in my last comment : no more layer shifting when mode or frequencies change during a print without the need for reset() function. This is also the case when enabling or diasabling ft_motion during a print.

@thinkyhead , if you want, i can do a new PR with all these changes for better testing.

@narno2202 I'm not having success reproducing the noise neither with a higher speed print nor high speed moves. Is there a speed at which you begin to notice it ?

[narno2202]

@ulendoalex , few precisions. The periodic noisy motion is not truly related to speed, it's always present and increase with speed not in frequency but in intensity (it's louder). I notice it when i merge your last code in current bugfix (no special sound in bugfix with ft_motion). If you don't notice it with your printer, could it be related to stepper drivers ? (mine are TMC2226)
The solution for layer shitfing is only valid with your last ft_motion implementation merged into Marlin. If fails in current bugfix.

[narno2202]

@thinkyhead , actually, i have Marlin code with Ulendo's implementation of ft_motion (far simpler than in current Marlin) and another Marlin code with only the needed functions of last Ulendo's code. The 2 versions are fully functionnal with no layer shift (frequency change ,mode change, disabling ft_motion during a print). I can do a PR for one or the other. It's up to you as i don't know which one is better.

[RV-from-be]

@narno2202
In the version of your branch https://github.com/narno2202/marlin/tree/pr_ulendo_ft_motion_fix, you must have a warning under stepper.cpp with void isr (). You must initialize 'interval' by hal_timer_t interval = 0; line 1497.

[narno2202]

@RV-from-be , thank's, it's fixed in the 2 repositories.
@ulendoalex , the periodic noise appears when i merge the new void FxdTiCtrl::runoutBlock() (in either branch). There is a relation between noise and FTM_STEPPERCMD_BUFF_SIZE, the best value is around (FTM_STEPS_PER_LOOP * FTM_POINTS_PER_LOOP)/2, better result but still present.

[ulendoalex]

\

@RV-from-be , thank's, it's fixed in the 2 repositories. @ulendoalex , the periodic noise appears when i merge the new void FxdTiCtrl::runoutBlock() (in either branch). There is a relation between noise and FTM_STEPPERCMD_BUFF_SIZE, the best value is around (FTM_STEPS_PER_LOOP * FTM_POINTS_PER_LOOP)/2, better result but still present.

@narno2202
Thank you for this detail. If it is related to the definition FTM_STEPPERCMD_BUFF_SIZE, it could be due to computing limitations. The updated function shouldn't have had a performance impact, however. In any case, could you please share details about the board you are using to test, as well as your Configuration_adv.h file? I can try to suggest some better values to see if we can eliminate this behavior.

[narno2202]

@ulendoalex, again thank's for your help. My board is a MKS Monster8 V1 with TMC 2226 stepper drivers (1 X, 2 Y, 3 Z and 1 E). There is no performance impact in my print. I have tried diiferent values for the other buffers but the better result is when i increase FTM_STEPPERCMD_BUFF_SIZE.
Configuration_adv.zip

I imagine doing a PR with your last code merged in Marlin, @thinkyhead remarks anf my mods as all the layer shifting issues ares solved for frequency and mode change. M493 S0 during a print works now too. There is already a branch in my repositoy (https://github.com/narno2202/Marlin.git) with all the changes : PR_Ulendo_FT_MOTION_fix

[ulendoalex]

@ulendoalex, again thank's for your help. My board is a MKS Monster8 V1 with TMC 2226 stepper drivers (1 X, 2 Y, 3 Z and 1 E). There is no performance impact in my print. I have tried diiferent values for the other buffers but the better result is when i increase FTM_STEPPERCMD_BUFF_SIZE. Configuration_adv.zip

I imagine doing a PR with your last code merged in Marlin, @thinkyhead remarks anf my mods as all the layer shifting issues ares solved for frequency and mode change. M493 S0 during a print works now too. There is already a branch in my repositoy (https://github.com/narno2202/Marlin.git) with all the changes : PR_Ulendo_FT_MOTION_fix

@narno2202
Thanks for the details and the file. It does appear that processor is more powerful than my test setup, so I won't suggest any tweaks to the other parameters at this time. A few other requests:

• Are you indeed configured to drive all 7 steppers that you mentioned ?
• Could you investigate if the noise is isolated to a particular stepper or axis ? Perhaps by jogging X/Y/Z/E independently to see if they all exhibit the noise.
• Could you please confirm if that this commit exhibits the issue: narno2202@85e7980 ? I am going to do another test in case I missed something the first time around.

Thank you!

[narno2202]

@ulendoalex

• all steppers drive a stepper motor (so 7 steppers, 7 motors)
• the noise is heard on all axis, even on individual move
• i notice the issue when i merge your last code into Marlin, branch PR_Ulendo_FT_MOTION_fix in my repository, Marlin is bugfix2.1.x 5d26f7 (05/07 in my country or 07/05 in US). The noise was noticed when the new void FxdTiCtrl::runoutBlock() and related changes are pushed into Marlin's code (full Ulendo implementation). Same noise exists with void FxdTiCtrl::runoutBlock() and related changes with initial ft_motion implementation in Marlin.

[thisiskeithb]

Superseded by #26074

[ulendoalex]

@ulendoalex

* all steppers drive a stepper motor (so 7 steppers, 7 motors)

* the noise is heard on all axis, even on individual move

* i notice the issue when i merge your last code into Marlin, branch `PR_Ulendo_FT_MOTION_fix` in my repository, Marlin is  bugfix2.1.x  `5d26f7` (05/07 in my country or 07/05 in US). The noise was noticed when the new `void FxdTiCtrl::runoutBlock()` and related changes are pushed into Marlin's code (full Ulendo implementation). Same noise exists with  `void FxdTiCtrl::runoutBlock()` and related changes with initial `ft_motion` implementation in Marlin.

@narno2202
Thanks for the information again. I did another round of tests but I'm still unable to replicate the noise. I have tried using the code directly from your branch, as well as incorporating your configuration settings. A few things you can try, though I expect won't have any effect:

• Disable INPUT_SHAPING_X and INPUT_SHAPING_Y.
• Set the Trinamic #define INTERPOLATE to false.
• Disable the Trinamic EDGE_STEPPING.

Finally, do you have any other printers you are testing on? I wonder if we can closer match our environments.

[narno2202]

@ulendoalex ,I have tried your suggestions, but the result is the same, modifying delay_before_delivery doesn't change anything. I have only one printer for testing. Again thank's for your help

[ulendoalex]

@narno2202
Thank you for trying.
I think the next step would be to try to debug remotely using your machine. We can dedicate some time to that in the near future; please let me know what you think and if there is a timeline you'd like to target. By the way, could you share a video of the noise? That may help us recognize if it occurs on our side.

[narno2202]

@ulendoalex, i try sound recording with my camera, so here are the two files.
First file : sensroless homing (FT_MOTION is disabled in Marlin when homing), only the first 10 seconds
Second File : ft_motion with a 100mm X move then a 100mm Y move

home.mp4

ft_motion.mp4

[narno2202]

@ulendoalex , i do more test. I apologize, the periodic noise is only heard when i do a manual move with screen's menu. No periodic noise while printing with FT_MOTION enabled.

[ulendoalex]

@ulendoalex , i do more test. I apologize, the periodic noise is only heard when i do a manual move with screen's menu. No periodic noise while printing with FT_MOTION enabled.

@narno2202
This makes somewhat more sense, as the changes should only impact behavior when there is one block remaining in the planner. Thanks for sharing the audio. Would you be able to add a print statement like "SERIAL_ECHOLN("runoutBlock call");" to "FxdTiCtrl::runoutBlock()" and observe the serial during one of the moves ? I'd like to test if for some reason the function is incorrectly invoked multiple times per block. If not, I will send you a patch for the buffer management to see if it is related to that.

[narno2202]

I am on vacation for 2 weeks, i don’t have access to my printer. I will try ASAP. Le jeudi 27 juillet 2023, ulendoalex ***@***.***> a écrit :

…

@ulendoalex <https://github.com/ulendoalex> , i do more test. I apologize, the periodic noise is only heard when i do a manual move with screen's menu. No periodic noise while printing with FT_MOTION enabled. @narno2202 <https://github.com/narno2202> This makes somewhat more sense, as the changes should only impact behavior when there is one block remaining in the planner. Thanks for sharing the audio. Would you be able to add a print statement like "SERIAL_ECHOLN("runoutBlock call");" to "FxdTiCtrl::runoutBlock()" and observe the serial during one of the moves ? I'd like to test if for some reason the function is incorrectly invoked multiple times per block. If not, I will send you a patch for the buffer management to see if it is related to that. — Reply to this email directly, view it on GitHub <#26014 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A7GYKSOFFMJLPKHOYZTULL3XSJ2BPANCNFSM6AAAAAAZSBZ3B4> . You are receiving this because you were mentioned.Message ID: ***@***.***>