Marlin: Weird discontinuous movement — CoreXY

[Dabihu]

Bug Report

• Description: Weird not continuous movement at a specific speed and configuration
• Expected behaviour: Linear movement
• Actual behaviour: Discontinuous movement
• Steps to reproduce:
  Use default 1.1.6 configuration with these modification:
  

  define COREXY

define DEFAULT_AXIS_STEPS_PER_UNIT { 100, 100, 4000, 500 }

(M502 used, but EEPROM disabled, so M500 have no effect)

Example command:
G1 F1000 X100 Y100

More options:

• F in range: 849..1154
• Diagonal move (only one motor used (X or Y) to movement)
• CoreXY or CoreYX
• 1.1.6 or bugfix-1.1.x (Hash: 5a1f1305f01837e18d6d797f39c1d150fc89f620)

F1000 video (bad movement): https://youtu.be/wIX8tTg1UyI
F1155 video (good movement): https://youtu.be/CGDx70LtVvE

Behavior is independent from motor driver. The step signal is discontinous in such settings.
Clicking noise in the good video caused by a faulty idler not by the motor.
Configuration.h.txt
Configuration_adv.h.txt

[coolio986]

I also have had this on version 1.1.3 - 1.1.5 (have not tried new versions) using CoreXY only. X/Y and E motors "pause" for a short period (100-200ms) then continue movement. I can confirm in the above post that it occurs with these conditions:

• F in range: 849..1154
• Diagonal move (only one motor used (X or Y) to movement)
• CoreXY or CoreYX

[tcm0116]

I've noticed something similar on my Cartesian machine. Long travel movements appear to be split into 2 or more movements with a slight pause between.

[thinkyhead]

@tcm0116 Does this only happen with bed leveling on? Or, does it not matter? Try adding a log line to the planner.buffer_line function so we can see whether the move is generating a single stepper block. Then, once we know whether it's a single stepper block, then we can check whether there's something in the stepper ISR that could lead to such a pause.

[coolio986]

I have bed leveling turned off. I will add a log line and report back.

[tcm0116]

For me, it was happening during printing with Bilinear leveling on during
long travel movements. As such, I do think it was happening at the grid
boundaries, so my issue may not be directly related to this one. However,
there could be an underlying issue with the planner stitching together
continuous movements in certain scenarios.

[AnHardt]

Is a display involved?

[bjarchi]

For me, it was happening during printing with Bilinear leveling on during
long travel movements. As such, I do think it was happening at the grid
boundaries, so my issue may not be directly related to this one. However,
there could be an underlying issue with the planner stitching together
continuous movements in certain scenarios.

I saw this happen for a bit after I had started using bilinear and it was quite frustrating because it showed up in my prints. I eventually realized that the problem was my z-jerk setting. At some point I had set z-jerk to 0.0 for testing and accidentally committed it with M500. With z-jerk at 0, the planner is forced to stop at any grid point where the sign of z-accel changes (at least I think that was the sufficient condition).

Resetting my z-jerk to a small non-zero value (0.3) restored continuous linear movement across grid point boundaries, even when the leveling direction changes.

[Dabihu]

Default configuration doesn't have display, eeprom or bed leveling. So nothing. But of course everything is connected, but not working.

[AnHardt]

What is the start position?

G1 F1000 X100 Y100

Is only then a 'one motor move' if the start position is somewhere with X==Y.
Is the start position in the videos constant?

[Dabihu]

Video is misleading, because it's move from X100 Y100 to X0 Y0.
In CoreXY or CoreYX setup diagonal (45 degree) move use only one motor.
Briefly dX=dY then X motor is used, dX=-dY then Y motor is used.
Start, end, length doesn't matter:
Start from X0 Y0
G1 F1000 X0 Y0 <- no move
G1 X20 <- good
G1 X120 Y100 <- bad
G1 X20 <- good
G1 X70 Y50 <- bad
G1 X0 Y0 <- BAD

New effect, if you move from X70 Y50 to X0 Y0 the movement is discontinuous. (Run twice the code)

[coolio986]

I do not have a screen attached when printing, I am not using bed leveling.
When printing at 15mm/s (900mm/m) doing a circle / square / rectangle/ anything, with 45deg infill this issue occurs for me.

The motor making the moves (X or Y, but not both), and E both pause for brief moments.

[tcm0116]

@thinkyhead my issue is definitely unrelated to this one. Mine is occurring at the bilinear grid boundaries as the printer is coming to a complete stop as it crosses the boundary. It seems to be a function of the amount of change in Z over the grid and the speed of the movement. In any case, I'll move my issue elsewhere.

[bjarchi]

@tcm0116 Did you see my earlier comment about checking your z-axis jerk setting?

my issue is definitely unrelated to this one. Mine is occurring at the bilinear grid boundaries as the printer is coming to a complete stop as it crosses the boundary. It seems to be a function of the amount of change in Z over the grid and the speed of the movement. In any case, I'll move my issue elsewhere.

This is exactly what I saw when I first set up bilinear leveling, due to accidentally having set z-jerk to zero.

Use M205 to check - 'advanced settings', z jerk will be labeled Zx.xx ...
and use M205 Zx.xx (ex M205 Z0.30) to set to a non-zero value

[bjarchi]

@coolio986 FWIW, this could occur with jerk set to 0 (or maybe even too low?) on any axis, if the planner is forced to stop due to motion constraints.

If you don't have EEPROM enabled it's not super likely - the values in configuration.h you posted look OK - but it could be getting set by a startup script or something - in which case it would still persist until the next machine restart or until it's reset by gcode. Might be worth using M205 to make sure something hasn't set one of your jerk values too low.

[Roxy-3D]

Does this issue happen when you cross over from one EEPROM version number to the next one? I'm seeing something similar except with the BL-Touch probes going wonkers and the nozzle crashing into the bed. Once I do a M502, M500, M501 everything is OK again.

[tcm0116]

@bjarchi I did see your comments about the jerk settings. My z axis jerk was set to the default of 0.4. I raised it all the way to 5, but still got the stops. I also played around with the acceleration, which only confirmed that the movement is being slowed/stopped at the bilinear grid lines.

@Roxy-3D I have not tried to reset the EEPROM, so I will give that a try. However, the printer is working well otherwise, so I don't think that'll resolve the issue. I need to add some printouts to the planner to see exactly what's going on. It sort of seems like an issue with the jerk processing, but I need to investigate further.

[Dabihu]

Start from X0 Y0
G1 F1000 X40 Y25
Logic analyser connected to Step pins (Channel0: X, Channel1: Y)
Full movement:

Zoom:

More zoom:

Timeline and changes in excel:
X40_Y25.xlsx
Raw logic data:
X40_Y25.zip

[AnHardt]

@Dabihu
Thanks for the numbers.
Does the stepper interrupt use doublestep mode from F1155 on?

[Dabihu]

No. No difference, but it's continuous.
Same setup, F1155:
Full:

Zoom:

Timeline and changes in excel:
F1155_X40_Y25.xlsx
Raw logic data:
F1155_X40_Y25.zip

[Bob-the-Kuhn]

@Dabihu - I've been able to re-create your issue. I'm about four levels into the software but not yet at root cause. I do know that it's getting screwed up because the start stepping rate is higher than the maximum/nominal rate.

Hopefully I'll have root cause tomorrow and a solution.

[thinkyhead]

One of these changes might solve the issue. Please try each one and see if it helps:

- if (moves_queued && !UNEAR_ZERO(previous_nominal_speed)) {
+ if (moves_queued > 1 && !UNEAR_ZERO(previous_nominal_speed)) {

---

   // Always split the first move in two so it can chain
   if (!blocks_queued()) {
+    previous_nominal_speed = 0;
     DISABLE_STEPPER_DRIVER_INTERRUPT();

---

-   #define _BETWEEN(A) (position[A##_AXIS] + target[A##_AXIS]) >> 1
+   #define _BETWEEN(A) (position[A##_AXIS] + target[A##_AXIS]) / 2

Since I've reverted the changes in bugfix, you can start with one of these branches:

• https://github.com/thinkyhead/Marlin/tree/backup_1_split_first
• https://github.com/thinkyhead/Marlin/tree/backup_2_split_first

[Dabihu]

Sorry thinkyhead, but this branches doesn't helps at all. It's worse than before.

Tried without any modification and the 3 modification separately, but all time this happened:

• Configuration changed:
  

  define COREXY

define DEFAULT_AXIS_STEPS_PER_UNIT { 100, 100, 4000, 500 }

• No home, nothing. Just give this command after upload:
  G1 F1000 X10 Y10

X motor moves like before (Length=sqrt(2)10mm), non continuously to X=10,Y=10
After reach this position the X motor moves forward more AND Y motor too continuously sqrt(2)10mm length then stops.

• Give the same command again:
  G1 F1000 X10 Y10
  X motor and Y motor moves continuously sqrt(2)*10mm length then stops.

• Repeat this as long as you want.

[thinkyhead]

Since I've reverted the changes in bugfix, you can start with one of these branches:

Sorry thinkyhead, but this branches doesn't helps at all. It's worse than before.

Those branches I linked should have been broken 32 minutes ago, as they contained the same errors.

Now however, they are patched.

[Dabihu]

Better, but the same where we started.

• MarlinFirmware:bugfix-1.1.x merged, where the first of three modification was in the code, same movement.
• After that applied the other two modification in one step, same movement.

[hg42]

@Dabihu the bugfix-1.2.x/2.0.x should not contain any of the three mods

[Dabihu]

bugfix-1.1.x / planner.cpp:

• Line 1265 not changed:
  if (moves_queued > 1 && !UNEAR_ZERO(previous_nominal_speed)) {
• Line 1444 modified:

define _BETWEEN(A) (position[A##_AXIS] + target[A##_AXIS]) / 2

• Line 1446 added:
  previous_nominal_speed = 0;

Or I don't understand what I should do.

[hg42]

I'm not sure, @thinkyhead thinks in his head :-), but these three changes were attempts to solve a problem that is solved now in both bugfix branches.

I suggest, you simply use the head of the branch without modifying it yourself

Apart from that,
the first seems to be in the bugfix branch, already. I guess thinkyhead knows why. It seems to be ok from my view. The count of queued moves will probably not go below zero, but it shouldn't hurt.
The second also shouldn't hurt, because dividing by two is the original idea, somewhat optimized by the shift, but I think the compiler will optimize it anyways.

The last one might cause ramp down, ramp up between moves, because I had this when I used this line while testing with the other bug (which could be caused by that bug).
I would not include it.

[hg42]

but let me test it myself...

[hg42]

ok, for me (using corexy and bugfix-2.0.x/32bit) the split move results in a ramp down and up in the middle.
It doesn't matter if I use the 2nd and 3rd mod

Do you experience the same behavior?

[hg42]

Apart from that,
the first seems to be in the bugfix branch, already. I guess thinkyhead knows why. It seems to be ok from my view. The count of queued moves will probably not go below zero, but it shouldn't hurt.

Well, something in my head didn't want to see that it's
moves_queued > 1
I thought about it as if it would be
moves_queued > 0
which would be the same like before.

So, this one is the reason why it ramps down and up between the first two moves in the current queue.

I am not sure, if @thinkyhead intentionally included this change in the latest commit.
I assume it's unintentionally, because he started from his backup branches.
This wasn't the variant, I tested here.
My patch was applied to the original PR #8611

I suggest you remove the "> 1" from moves_queued

Note, I am not using bed leveling currently. My tests only addressed the weird movements.
I don't know, if your original issue will be solved.

[hg42]

I just read the whole thread and I guess I'm on the wrong track here.
I'll go to bed no and think about it again tomorrow. Sorry, if I told non-sense.

At least one fact remains: moves_queued > 1 doesn't work for me.

[thinkyhead]

moves_queued > 1 doesn't work

Note that the bugfix branches have this patched since just before @Dabihu last posted.

[AnHardt]

@thinkyhead 's tree suggested patches are a typical reaction of an experienced programmer having no time to test himself but has to react somehow and fast to an avalanche of new issues.

If you have the idea an array index might become below zero it's a good idea to go back to the code used before.

- if (moves_queued && !UNEAR_ZERO(previous_nominal_speed)) {
+ if (moves_queued > 1 && !UNEAR_ZERO(previous_nominal_speed)) {

But the effect is - the second half of the splitted move will start at zero speed - always - like without splitting the first move the second started always with zero, but this time in the middle of the first (splitted) move.

In

   // Always split the first move in two so it can chain
   if (!blocks_queued()) {
+    previous_nominal_speed = 0;
     DISABLE_STEPPER_DRIVER_INTERRUPT();

the suspicion is a somehow uninitialized variable. Does not hurt, but would n't have worked before we invented splitting.

-   #define _BETWEEN(A) (position[A##_AXIS] + target[A##_AXIS]) >> 1
+   #define _BETWEEN(A) (position[A##_AXIS] + target[A##_AXIS]) / 2

makes sense when your suspicion is, one of the variables might not be an integer but float. With time to verify you will find - both are int32_t - but that takes a while.

Why this selection of suggestions? Because he believed in the correctness of his own code but not mine.

---

And now let's return here to this (https://github.com/MarlinFirmware/Marlin/issues/8573#issuecomment-347469235) problem. The 'split fist move' patch is not related to that and was by purpose splitted out to 'Pausing at bilinear grid boundaries #8595'
Sadly i have no idea what's going wrong here. Looks a bit as if the next timer interval in the Bresnham is calculated wrong sometimes. But under what circumstances?

[AnHardt]

Did i understand right now? For the not continuous move on DELTAs it's not a requirement to have a 'one motor move only' but it occurs even if only one motor is involved.

[AnHardt]

The stuttering moves are "single moves" - not further subdivided by grid lines, or delta-subdivision or similar. (now splitted by "first-move split" - but this issue is older)

[Dabihu]

@AnHardt Sometimes I don't understand what everybody talking about, what I wrote is the most simple case, where the bug appear.

• Default settings with some modification. (Wrote above)
• Simple short or long move (G1 F1000 X10 Y10)
  In CoreXY it's use only the X motor.
• Almost diagonal simple move (G1 F1000 X40 Y25)
  Use X and Y motor. Step timings captured above with logic analyser.
• No leveling, nothing. That's all.
• Used Arduino IDE 1.8.5 compiler.

[hg42]

@AnHardt

Note that the bugfix branches have this patched since just before @Dabihu last posted.

I don't think so...
He (using bugfix-2.0.x?) and I merged 13h and 12h ago. @thinkyhead added a followup 10h ago which disabled the moves_queued > 1.
So we both got the separated split moves (with ramps in between).
I don't see this follow up in bugfix-1.x.x, that explains why others did not have this problem.
That's why I got confused, because I read "it works" and " it still doesn't work" at the same time.
Also, I first tested with my version, which didn't include the "> 1", so I didn't expect, it wouldn't be solved in bugfix-2.0.x

but we got it finally, everything's ok :-)

[hg42]

He (using bugfix-2.0.x?) and I merged 13h and 12h ago.

no, @Dabihu uses 1.1.6. Which I can't (32bit). So I'm lost here.
And my assumption about the follow up commit was wrong for his case (but not mine).

I am on the way trying to reproduce the issue, because I also have a corexy and want to see, if bugfix-2.0.x does behave similar. I'll use @Dabihu's last description. Let's see.

[hg42]

unfortunately my oscilloscope isn't able to capture this.
Does it create such pauses when running very slow, say F100?

At least all moves I tried from F100 to about F6000 were totally smooth.
I multiplied the length and my 0,0 is in the center, so I moved between -160, -100 and +160, 100 if that matters in some way. But the requested moves didn't do the same thing.

I also found, that @thinkyhead also removed the "> 1" from bugfix-1.x.x at a similar point in time, but used a different title, so @Dabihu got this other behavior, too.

[AnHardt]

@Dabihu
Please try (stepper.cpp)

@@ -742,12 +742,13 @@ void Stepper::isr() {

       eISR_Rate = adv_rate(e_steps[TOOL_E_INDEX], OCR1A_nominal, step_loops_nominal);

     #endif

-    SPLIT(OCR1A_nominal);  // split step into multiple ISRs if larger than  ENDSTOP_NOMINAL_OCR_VAL
-    _NEXT_ISR(ocr_val);
+//    SPLIT(OCR1A_nominal);  // split step into multiple ISRs if larger than  ENDSTOP_NOMINAL_OCR_VAL
+//    _NEXT_ISR(ocr_val);
+    _NEXT_ISR(OCR1A_nominal);
     // ensure we're running at the correct step rate, even if we just came off an acceleration
     step_loops = step_loops_nominal;
   }

   #if DISABLED(LIN_ADVANCE)

While cruising constantly this is about the only place where the timing could get a variation.
Do i have to dig in SPLIT() or do i have to search for something else.

[Dabihu]

Because I don't know what cause the issue, I could not trigger with other parameters.
CoreXY needed, 100 axis step per unit needed, and F in the range 849..1154.
And as mentioned before not all direction cause the issue.
Maybe it depends on cpu clock frequency or system architecture too.

[Dabihu]

@AnHardt I used bugfix-1.1 83a1a701032a100dbbfa617c3fafdae6bf962eb4 as base. It has the issue.
Changed what you wrote, but didn't helped. (Tried G1 F1000 X10 Y10)

[hg42]

I can not reproduce this on bugfix-2.0.x and 32bit with

• 94cb412e45b8e719775960312a9241f39e2a5ae2 (should be around that mentioned commit from bugfix-1.1.x)
• 91fb6231b2a779c86b15a552add2e16aa09ce5f1 (next and last time the branches were merged)

Can you run bugfix-2.0.x? does it happen there, too?

[hg42]

given that I have LPV1768 this may be an 8bit or AVR issue.
Or it is 1.1.x vs. 2.0.x.

[Dabihu]

Tried bugfix-2.0.x 4d3c3d0b7384038e12c6eddb14bf717985b8dd7a too. Same result.
Just two modification in the Configuration.h:

define COREXY

define DEFAULT_AXIS_STEPS_PER_UNIT { 100, 100, 4000, 500 }

[hg42]

do you know a commit, where it still worked?
the usual strategy would be to narrow the range between the working and failing commits by going up the tree with the working commit and down with the failing commit.
I usually tag the results with OK1, OK2, ... and the failing with FAIL1, FAIL2, ...
At the end of the process, you hopefully get the one commit that caused the hassle.
Sometimes this is a really big commit (like merging), but often it's a simple commit and we could easiliy see, what change does cause this.
I would do this myself, but this only works if I can reproduce the problem (which I tried).

[hg42]

I used your steps/unit which also worked.

[Dabihu]

First iteration:
1.1.0-RC7: Working great!
1.1.0-RC8: Have this issue.

[Bob-the-Kuhn]

I'll check out RC7.

I have a solution that I'm almost ready to show to the world. I need to do some testing on delta configurations first.

[tcm0116]

@Bob-the-Kuhn I've got some free time this evening if you want some help
testing on a Delta and/or Cartesian machine.

[Bob-the-Kuhn]

I'll attach the file in an hour or two.

The root cause is the planner doesn't properly handle in all cases that the CORE machines always have one stepper that is running faster than the head. The result is negative acceleration steps being passed to the stepper routine in a band of feed rates. The stepper software doesn't react nicely to the negative steps.

[hg42]

but why I don't see this on my machine (also corexy)?

[Bob-the-Kuhn]

Here's the new planner.cpp for testing.
planner.zip

@hg42 - I expect that you're not using the affected feed rates. If you give me your steps_per_unit and default jerk settings I'll calculate the feed rate band where this occurs.

[AnHardt]

@Bob-the-Kuhn
Where did you start from? Diff gives a lot unrelated changes to me, in today's bugfix branches.

[Bob-the-Kuhn]

I just double checked the uploaded ZIP file. Those are my intended changes.

Jerk, speeds and rates are all related.

[Dabihu]

@Bob-the-Kuhn Your source was around 972248c3334ae5432afdcd180fea80653d41beb1 but I can't find the correct one.

[Bob-the-Kuhn]

As best I can tell, today's bugfix1.1.x planner.cpp is the same as it was about a week ago before all the split (and revert) activities.

[Bob-the-Kuhn]

See PR #8680 for a better explanation of what I did.

[Bob-the-Kuhn]

Yep - I'm not working off the latest code base. I'll be back after I've figured out the problem and have done a few sanity checks.

[hg42]

M92 X80.000000 Y80.000000 Z400.000000 E162.000000
M205 S0.000000 T0.000000 B20000 X20.000000 Y20.000000 Z0.400000 E5.000000

[Bob-the-Kuhn]

The following applies for a COREXY where the X and Y Jerk maximums are the same and the steps_per_unit are the same.

The G1 minimum feed rate (mm/minute) where you can see this issue is Jerk * 60 / ALPHA where ALPHA varies between 1 (head going at multiples of 90 degrees) to SQRT(2) (head moving at odd multiples of 45 degrees).

The G1 maximum feed rate (mm/minute) where you can see this issue is always Jerk * 60

For your system the G1 feed rate range is 849 - 1200 mm/minute.

G1 X100 Y100 F1000 should show the issue for you. You'll see 10mS - 30mS pauses in the step pulses.

[Dabihu]

@Bob-the-Kuhn Your code is working on 802ae73b137d401f957587e8b1b5b174480cdc44.
I think you are on a good path and your theory make sense. I could test again 12 hours later.

[hg42]

I testet at 900, 1000, 1155
May be the tmc2130 in standalone mode does soften this...it's moving totally smooth with a very constant sound. I am sure I would here those ticks. I should probably disable interpolation on the tmc.

[hg42]

I switched my TMCs into 16 usteps non-interpolating spread cycle mode CFG1=VIO CFG2=VIO (I accidentally chose 12V and then there were two...).

Now I have a lot more noise (wow...didn't remember), but still no irregular movement. Should I see or hear it like in the video?

The movement is still smooth. Only the motor has some cycling sound changes, but it's more like an interference, it's going up and down, no klicking or such.
Also this cycling changes in frequency when choosing higher step rates and it continues up to F6000.

[hg42]

I used these gcode files to test:

test-non-continuous-movement.zip

there is no dramatic change at any step rate...

[hg42]

there is no difference in sound between your code and before (assuming that I added your code correctly, not sure about some lines).
planner.cpp.txt

[Dabihu]

Sorry everyone. I touched the mousepad (close and comment...)...
So, again...
@hg42 I don't know what is the different. Try to use the default settings from the commit, with the two modification I wrote at the beginning:

define COREXY

define DEFAULT_AXIS_STEPS_PER_UNIT { 100, 100, 4000, 500 }

It doesn't matter, that your printer have different settings or accelerations in real life. If you change something, than the speed will change too where the issue appears or not.
The driver could not change the issue, because it works at a frequency of several orders of magnitude higher. But this is why I used the steps pin with logic analyzer.

I tested your gcodes at 802ae73b137d401f957587e8b1b5b174480cdc44 and all shows the issue. (Not all time, but in the F range and directions)

[hg42]

I cannot exactly create the same scenario, because I am on 32bit and need bugfix-2.0.x. Also the configuration must include the board etc. But apart from that I did everything to make it them same as yours.

The point is, if I do not have the issue, it may be a bit more complicated than @Bob-the-Kuhn thinks.
He gave a clear calculation what would be my frequency range to see the issue and it isn't there.
So I expect there is an additional dependency.

However, if the issue is solved for those experiencing it, this would be great.
I only hope it doesn't hurt somewhere else if it is not fully reproducible, which probably means not fully understood.

But let's see with the final patch.

[hg42]

Is there anyone with a 32bit board and having the issue?

[Dabihu]

@Bob-the-Kuhn One question.
In theory the
block->nominal_speed = block->millimeters * inverse_secs;
is the same as
block->nominal_speed = max_stepper_speed;
But only when one axis moving. For complex move I think this is not so simple.
Not the LOOP_XYZE routines are buggy?
Core has A B C axis and it's calculation doesn't used in the limitations (speed, safe_speed, acceleration, jerk) and corrections.

[Dabihu]

I have no idea what cause different operation under 32bit. LPC1768 is much faster. I don't understood fully the code, but if there is a buffer for line segments and the buffer fill timing calculation wrong, than maybe the 8 bit processor could not fill the buffer fast enough, but 32 bit could. The pause hasn't a constant length. It has very big fluctuation under one gcode line. See the first picture from logic analyzer. That was only one gcode line! One (un)continuous movement.

[hg42]

yes, something like this would make sense

[Bob-the-Kuhn]

I just verified in 2.0.x that the 2560 has this issue and the LP1768 does not show it.

---

block->nominal_speed = max_stepper_speed;

In the old (current) code that isn't true. In the new code it is true.

In the old code nominal_speed was the head's distance divided by the time it took the head to travel that distance.

[Bob-the-Kuhn]

Here's the planner.cpp file from the PR I created earlier this evening. This one is based on the current code base.
planner.zip

I couldn't figure out why I couldn't rebase to the current code base so i manually saved each branch to a local directory and deleted my repository. With a new fork all is working as expected.

[thinkyhead]

I couldn't figure out why I couldn't rebase to the current code base

To make sure no one can write to your branch while you're working on it, be sure to uncheck this box:

[thinkyhead]

Updated 1.1.x and 2.0.x bugfix branches with the new Jerk code. Let's see if it addresses this issue without having any deleterious effects on the other kinematics!

[Dabihu]

@thinkyhead It looks like this has solved the problem. I will run some test, that I did everything well.

[Dabihu]

@Bob-the-Kuhn Ok, that was my problem 8 hours ago.
Used latest bugfix-1.1 86b65e52c4dd72cf55a3f943fd861602d8421772
Restored the planer.cpp before your modifications from 3b30cc90f173e8bdc0a74423b795cc51eaa22758
You completely removed old code lines: 1250..1262

  LOOP_XYZE(i) {
    const float jerk = FABS(current_speed[i]), maxj = max_jerk[i];
    if (jerk > maxj) {
      if (limited) {
        const float mjerk = maxj * block->nominal_speed;
        if (jerk * safe_speed > mjerk) safe_speed = mjerk / jerk;
      }
      else {
        ++limited;
        safe_speed = maxj;
      }
    }
  }

But if I comment it, and don't use your modifications, the bug is gone.
So my questions:

• It's needed?
• Needed your modifications?
• Is the issue fixed or just causes other bug because safe_speed now different the issue disappears?

[Bob-the-Kuhn]

There's definitely more ways than one to get rid of the stuttering.

The proposed change doesn't address the concern of jerk not being applied in the "bad" band of feed rates.

[Bob-the-Kuhn]

To make sure no one can write to your branch while you're working on it, be sure to uncheck this box:

@thinkyhead - I didn't realize that box pertained to the branch. I had assumed that it applied to editing the comment.

The problem I was having was not associated with changes being done after I pulled the PR.

As best I can tell GitHub was grabbing an old copy of bugfix when I went to create a new branch from the default. I changed the default from bugfix-1.1.x to bugfix-2.0.x, still had the problem, changed the default back and still had the problem. I deleted the local & GitHub copies of the repository and then forked Marlin. The new repository is working correctly.

[thinkyhead]

My go-to support desk line: "Try unplugging it and plugging it back in."

[thinkyhead]

The proposed change doesn't address the concern of jerk not being applied in the "bad" band of feed rates.

Bob, Is that more or less a concern for CoreXY? Perhaps it makes sense to use the old jerk code for cartesians and deltas, but only apply your updated jerk code for CoreXY — or other systems where there might be such a "bad band" of values.

[Bob-the-Kuhn]

Cartesian can use either.

CORE needs the new.

I need to investigate the delta math.
UPDATE - Delta is the same as Cartesian as far as _buffer_steps() is concerned so it can use either.

---

I was able to figure out what the Průša safe speed code was doing. It's doing almost the same thing as the new code. Both look for the worst case jerk-to-stepper-speed-ratio and then multiply that by the nominal speed to get the safe speed. The only difference is, if the only stepper that needs acceleration in NOT the fastest stepper, then the Průša code sets the safe speed slower than needed which means a longer acceleration ramp so it takes a little longer to execute the block.

You have to admire the author for the brevity of the code. On the other hand it's hard to understand.

---

Here's the Průša safe speed code re-arranged a little with much better variable names. Start optimizing and replacing the floating point divides by floating point multiplies then you'll get back to the original code.

  uint8_t limited = 0;
  float speed_ratio = 1;
  LOOP_XYZE(i) {
    const float abs_current_speed = FABS(current_speed[i]);
    if (abs_current_speed > max_jerk[i]) {
      if (limited) {
        const float temp_speed_ratio = max_jerk[i]/abs_current_speed;
        if (speed_ratio > temp_speed_ratio) speed_ratio = temp_speed_ratio;
      }
      else {
        ++limited;
        speed_ratio = max_jerk[i]/abs_current_speed;
      }
    }
  }
  float safe_speed = block->nominal_speed * speed_ratio; 

[Bob-the-Kuhn]

The Marlin code converts the delta & scara motions to equivalent X, Y & Z motions before _buffer_steps() is called. As far as _buffer_steps() knows, delta & Cartesian robots are the same.

The new code can be used in all circumstances.

[Dabihu]

Before I wrote my last comment I made a new planner.cpp where only just the necessary changes have made:
planner.cpp.wrong.txt
This is from commit 3b30cc9 and your modification, but at right place.

This code has discontinuous movement, but I realized, that I made an error, because in line 1258 I forget to multiply by min_axis_accel_ratio.

So after I realized, that yes, your code is making the same as Průša safe speed code, I deleted it.
This is the minimal necessary changes compare to commit 3b30cc9:
planner.cpp.txt
It's need testing, but I think it will work.

For @thinkyhead question, the safe_speed is the same or lower, than with Průša safe speed code. So yes I think it will work for every type.

[thinkyhead]

Start optimizing and replacing the floating point divides by floating point multiplies then you'll get back to…

Yeah… I'm the fool who figured that this:

if (jerk * safe_speed > mjerk) safe_speed = mjerk / jerk;

…might be faster than…

if (safe_speed > mjerk / jerk) safe_speed = mjerk / jerk;

Multiply instead of divide, right? Who knows?

[Bob-the-Kuhn]

I did a little test program. On a 16MHz 2560, floating point multiplies averaged 2.7uS while the floating point divides averaged 6.9uS.

Replacing divides with multiplies saves time.

Your change to the Průša code does save time.

@Dabihu - I tested your code. It works as desired. The only thing I didn't like is the values for block->entry_speed, block->nominal_speed and safe_speed are different. Since the ratios are correct the acceleration calculation is correct. Since the stepper uses block->nominal_rate then we get the correct stepper speed.

I'm going to stick with my code because it's much easier to understand, the numbers are consistent, is almost the same speed, doesn't have the hole the Prusa code has.

[Dabihu]

I expected much larger difference (5x..10x) in multiply or divide. Most microcontrollers have only multiply unit and don't have hardware divide unit and atmel is no exception. (Some STM32F have)

My code is not my code. It's your code, but I don't changed the positions and lines, just what were absolutely necessary. No more and no less. And yes I think that your code solved the problem.

--- a/Marlin/planner.cpp
+++ b/Marlin/planner.cpp
@@ -1045 +1044,0 @@ void Planner::_buffer_steps(const int32_t (&target)[XYZE], float fr_mm_s, const
-  block->nominal_speed = block->millimeters * inverse_secs;           //   (mm/sec) Always > 0
@@ -1082 +1081 @@ void Planner::_buffer_steps(const int32_t (&target)[XYZE], float fr_mm_s, const
-  // Calculate and limit speed in mm/sec for each axis
+  // Calculate and limit speed in mm/sec for each axis, calculate minimum acceleration ratio
@@ -1083,0 +1083 @@ void Planner::_buffer_steps(const int32_t (&target)[XYZE], float fr_mm_s, const
+  float max_stepper_speed = 0, min_axis_accel_ratio = 1; // ratio < 1 means acceleration ramp needed
@@ -1085,0 +1086,2 @@ void Planner::_buffer_steps(const int32_t (&target)[XYZE], float fr_mm_s, const
+    NOMORE(min_axis_accel_ratio, max_jerk[i] / cs);
+    NOLESS(max_stepper_speed, cs);
@@ -1091,0 +1094,2 @@ void Planner::_buffer_steps(const int32_t (&target)[XYZE], float fr_mm_s, const
+  block->nominal_speed = block->millimeters * inverse_secs;           //   (mm/sec) Always > 0
+
@@ -1238,8 +1241,0 @@ void Planner::_buffer_steps(const int32_t (&target)[XYZE], float fr_mm_s, const
-  /**
-   * Adapted from Průša MKS firmware
-   * https://github.com/prusa3d/Prusa-Firmware
-   *
-   * Start with a safe speed (from which the machine may halt to stop immediately).
-   */
-
-  // Exit speed limited by a jerk to full halt of a previous last segment
@@ -1248 +1244 @@ void Planner::_buffer_steps(const int32_t (&target)[XYZE], float fr_mm_s, const
-  float safe_speed = block->nominal_speed;
+  float safe_speed = block->nominal_speed * min_axis_accel_ratio;
@@ -1250,13 +1245,0 @@ void Planner::_buffer_steps(const int32_t (&target)[XYZE], float fr_mm_s, const
-  LOOP_XYZE(i) {
-    const float jerk = FABS(current_speed[i]), maxj = max_jerk[i];
-    if (jerk > maxj) {
-      if (limited) {
-        const float mjerk = maxj * block->nominal_speed;
-        if (jerk * safe_speed > mjerk) safe_speed = mjerk / jerk;
-      }
-      else {
-        ++limited;
-        safe_speed = maxj;
-      }
-    }
-  }

[thinkyhead]

Replacing divides with multiplies saves time.

Question is, can the compiler be smart enough to replace a divide with a multiply, as in my example above? I think under most circumstances the compiler is supposed to be true to your code, even if it could figure out that trick. But maybe applying _Os to functions can get the compiler to be extra clever.

[AnHardt]

@Dabihu
As far i remember the difference for integer divisions and multiplications on 8bit AVRs is bigger.
For floats, better than factor 2 justifies https://github.com/MarlinFirmware/Marlin/pull/8722/commits/aa1cbdcd81cd31456f18df5d6e232fcdf68b3269

[thinkyhead]

I believe also that 1.0 / x (reciprocal) is an optimized operation with most fp libraries, so costs less than other divisions. It may depend on whether x is an even p * 2 ^ q value….

[Dabihu]

Don't write before sleep... Hardware divider probably doesn't used in fp operation. Yes the factor is not so big.

I am thinking of close the issue. @Bob-the-Kuhn code implemented in the bugfix branch and nobody could confirm the previous code why not caused issue on 32 bit.