Planner trapezoidal nominal_rate fix

[HoverClub]

Fix nominal_rate range in planner

In Planner::calculate_trapezoid_for_block(), block->nominal_rate could be lower than MINIMAL_STEP_RATE on entry. this produces invalid acceleration results as it attempts to match the exit rate from the previous block. The initial_rate and final_rate are NOLESS'ed to MINIMAL_STEP_RATE at entry. However, block->nominal_rate isn't so you get some weird trapezoid outputs. The stepper fixes the worst case issues but the bad data causes the laser trap calculation to produce wrong results.

This happens when the feed rate is set lower than MINIMAL_STEP_RATE . MINIMAL_STEP_RATE is set to 120 (a #define in the middle of planner.cpp) - I'm not sure how this number was derived? I've changed this value to be calculated which should give better results - unless there is some reason, other than isr counter overflow, for the slowest step rate to be limited?

Requirements

#define LASER_POWER_TRAP, #define LASER_FEATURE

Run this Gcode:

G21
G90
G91
g1y10s0 F10 ; reduce this until if fails
g1y10s10
g1y10s20
g1y10s30

Planner output: (ir = initial_rate, nr = nominal_rate, fr = final_rate) => ir:120 nr:45 fr:120 ==> step acc:0->4294967293 decel: 270->266 ==> ir:120 nr:45 fr:120 ==> step acc:0->4294967293 decel: 270->266

==> ir:120 nr:45 fr:120 ==> step acc:0->4294967293 decel: 271->267 ==> ir:120 nr:45 fr:120 ==> step acc:0->4294967293 decel: 271->267

==> ir:120 nr:45 fr:120 ==> step acc:0->4294967293 decel: 270->266 ==> ir:120 nr:45 fr:120 ==> step acc:0->4294967293 decel: 270->266

==> ir:120 nr:45 fr:120 ==> step acc:0->4294967293 decel: 271->267

[sjasonsmith]

How did you discover this? I've seen some weird acceleration related to that 120 Hz rate in the past when I was capturing analyzer traces of movements (for real or in the simulator), but I don't remember whether those issues were fixed with some of other other step timing issues that @descipher and/or @tombrazier resolved a while back.

[HoverClub]

I found it initially when using a puny laser that needed to move slowly to cut - it's easy to visually see over/under burn (related to step rate) with a laser.

The code decelerates on entry from MINIMAL_STEP_RATE to the slow target rate then accelerates on exit from the slower rate to MINIMAL_STEP_RATE for each move - resulting in a sawtooth trap shape.

I'm still unsure what the function of MINIMAL_STEP_RATE is - there must have be a reason why it's there (other than ISR counter overflow)? https://github.com/MarlinFirmware/Marlin/issues/20150 https://github.com/MarlinFirmware/Marlin/issues/107

[tombrazier]

Great work tracking this down. The comment in the code says MINIMAL_STEP_RATE is limited to prevent timer overflow.

  // Limit minimal step rate (Otherwise the timer will overflow.)
  NOLESS(initial_rate, uint32_t(MINIMAL_STEP_RATE));
  NOLESS(final_rate, uint32_t(MINIMAL_STEP_RATE));

The comment goes back to Eric's original commit of the source code in 2011. Back then the minimum was 32Hz. Three days later he changed it to 120Hz in a commit with comment "Fixed lookup table bug." and it's been the same ever since. Along with this code there was also a line in a different place that limited block->nominal_rate to >= 32 (and later 120). This line disappeared in commit 65934eee9c6ae792c708bc1cea9996c8a5df67f5, comment "A lot of changes in the planner code". This commit also introduced MINIMUM_PLANNER_SPEED which was used for planning as the minimum speed the planner would use for the start or end of a trapezoidal profile. Much later, I improved on MINIMUM_PLANNER_SPEED in #26198. I think commit 65934eee9c6ae792c708bc1cea9996c8a5df67f5 probably missed a trick because as far as I can tell the introduction of MINIMUM_PLANNER_SPEED did not make up for the removal of the lower bound on block->nominal_rate.

The stepper timer has always been 2MHz for 16MHz ATmega2560 which, with a 16 bit timer, means timer/counter 1 will overflow at frequencies below 30.5Hz. So that's where 32Hz comes from. However, calc_timer() (now calc_timer_interval()) has always protected against timer overflows so even in the original code the 32Hz limit seems to have been unnecessary. And I do not understand why there was a the change to 120Hz. The lookup table (for calculating step timing) has always been able to handle any step rate.

120Hz is pretty slow for 3D printing equating to 1.5mm/s for X and Y on a 80 steps/mm printer. So I'm guessing this is why this has not been spotted and explored before.

Anyway, it is not clear to me that there is any need for the MINIMAL_STEP_RATE lower bounds. I would be interested to know what would happen if they were just removed entirely.

[tombrazier]

On a related note, for the use case described with a laser that required really slow movement it might make sense to increase the microstepping level so that step rates don't get so low in the first place. Not a solution to the bug but likely a good idea if you're approaching the minimum rate the stepper interrupt can run at.

[sjasonsmith]

I found it initially when using a puny laser that needed to move slowly to cut - it's easy to visually see over/under burn (related to step rate) with a laser.

This feels very similar to something we fixed in the past, but perhaps it's a slightly different scenario. We might have fixed long pulses when starting from a standstill, versus yours transitioning form block to block.

Here is the link to the old bug with a bunch of discussion for historical reference. It has a bunch of analyzer captures and charts in it which may appear somewhat similar to what you were seeing.

• #20150

[tombrazier]

This feels very similar to something we fixed in the past, but perhaps it's a slightly different scenario. We might have fixed long pulses when starting from a standstill, versus yours transitioning form block to block.

It was the long pulses from standstill that we fixed.

[tombrazier]

@GelidusResearch Could you elaborate? I think that it doesn't matter how low the step rates gets because in the stepper module it will not step any slower than 32 steps per second. That's built into Stepper::calc_timer_interval().

[GelidusResearch]

@GelidusResearch Could you elaborate? I think that it doesn't matter how low the step rates gets because in the stepper module it will not step any slower than 32 steps per second. That's built into Stepper::calc_timer_interval().

Certainly, with the AVR and a period of 32Hz that sets OCR1A at ~62500, if there is any delay or latency in the ISR processing this could result in an overflow. We have no preemption capability to assert priority so if the processor is heavily loaded with other IRQ's this period value could be unstable. Of course we would need significant built up testing to see that potential issue.

We have a window of ~500us before it overflows. I think that's what Eric may have encountered and thus he went to 120Hz for safety. So with the AVR 16bit timers @ 16Mhz and lower we may have issues near the 32Hz minimum for some users.

[tombrazier]

@GelidusResearch Having given it more thought and reminded myself what the code does, I believe overflow is not a concern. The stepper interrupt timer gets reset to zero on compare match. It makes no difference how close it was to the maximum count of the timer when this happens. This must be the case for all MCUs because Stepper::isr() assumes that the counter will have started again at zero when the interrupt was triggered.

[HoverClub]

Removing the MINIMAL_STEP_RATE NOLESS in planner will invalidate the laser trapezoidal and dynamic mode power calculations as the physical step rate will be forced higher by stepper.cpp at low feed rates. It also means that the block accel/decel calcs/counts will be incorrect. This may cause other issues so it may be better to keep the NOLESS checks using the calculated MINIMAL_STEP_RATE to avoid any other consequences?

I have tested removal on a slow Laser and a 3D printer but that is no guarantee that it woks in all situations.

[tombrazier]

Removing the MINIMAL_STEP_RATE NOLESS in planner will invalidate the laser trapezoidal and dynamic mode power calculations as the physical step rate will be forced higher by stepper.cpp at low feed rates. It also means that the block accel/decel calcs/counts will be incorrect. This may cause other issues so it may be better to keep the NOLESS checks using the calculated MINIMAL_STEP_RATE to avoid any other consequences?

Excellent point. All said, I now think the original implementation of this PR was about right.

[mh-dm]

I'm coming from #27013 where this issue was mentioned. There, I also noticed the potential for the MINIMAL_STEP_RATE bound to cause issues (and I saw some "weird trapezoid outputs", though only once, while editing/building up my test gcode). While I also initially wanted to remove MINIMAL_STEP_RATE, that would cause issues in the case where acceleration_steps_per_s2 is high relative to initial/final_rate. @HoverClub mentioned "block accel/decel calcs/counts will be incorrect" which I think is referring to the same thing.

Finally, my fix in #27013 was to limit the initial and final rates to block->nominal_rate. This is not the ideal solution but it's a fix with no potential for invalid acceleration (because there won't be any):

  NOLESS(initial_rate, uint32_t(MINIMAL_STEP_RATE));
  NOLESS(final_rate, uint32_t(MINIMAL_STEP_RATE));
  NOMORE(initial_rate, block->nominal_rate);
  NOMORE(final_rate, block->nominal_rate);

Now the ideal solution would be sort of limiting:

NOLESS(initial_rate, uint32_t(SQRT(acceleration_steps_per_s2 / 2)));
NOLESS(final_rate, uint32_t(SQRT(acceleration_steps_per_s2 / 2)));

which is basically the same limit as

  // The minimum possible speed is the average speed for
  // the first / last step at current acceleration limit
  minimum_planner_speed_sqr = 0.5f * block->acceleration / steps_per_mm;

just in steps rather than mm.

We can't just add the former because SQRT is slow on microcontrollers. Instead the proper fix is to make sure that the minimum_planner_speed_sqr limit is correctly followed in all cases and correctly flows through such that the initial_rate and final_rate that calculate_trapezoid_for_block() (implicitly) gets are correct. The bad news is that it's annoyingly hard. The good news is that I have a working prototype change that I'm planning to send out for review soon. (It kinda depends on #27013 but maybe I can send it as a WIP)

[sjasonsmith]

@HoverClub, @mh-dm, and @tombrazier, what is your disposition toward this PR right now?

• @tombrazier recently said "I now think the original implementation of this PR was about right.", although I think that is not the current state of this PR.

• @mh-dm has related changes in flight in the other PR right now. We probably need to select one of these changes to take precedence and go in first.

[tombrazier]

Regarding #27013, I don't fully understand the conflicting lines in that PR. @mh-dm can you explain them? (Probably best to do so in that PR).

I think this PR is pretty much right at commit 3b887b468b3fd8a063f60c29e02b1d3dc2ef64f3. In that commit, the planner will not plan a move that is slower than the stepper module can step and block->nominal_rate will never be less than initial_rate or final_rate.

However MINIMAL_STEP_RATE should be explicitly tied to min_step_rate in Stepper::calc_timer_interval(). i.e. they should be the same value and declared in stepper.h. Note also that depending on whether the MCU is an AVR this value is (F_CPU) / 500000U or uint32_t(STEPPER_TIMER_RATE) / HAL_TIMER_TYPE_MAX.

There is also a rounding error that remains, but it might not be too important. MINIMAL_STEP_RATE is an integer but STEPPER_TIMER_RATE / HAL_TIMER_TYPE_MAX generally won't be. That is not a problem for how Stepper::calc_timer_interval() works. But if the planner ended up planning a move at the integer-truncated speed then there would be a slight difference between the planned speed and the speed that results from Stepper::calc_timer_interval(). This could mean the laser power is slightly wrong for the actual speed that is generated in the case where the slowest possible speed is planned.

[HoverClub]

Haven't had time to look at this again but I agree with the suggestions that it would be better handled "upstream" of the trapezoid calc. Looking at the code, MINIMUM_PLANNER_SPEED may result in next_entry_speed being slower than the calculated MINIMAL_STEP_RATE. These conflicting limits are possibly the underlying cause of this issue? MINIMAL_STEP_RATE is there to prevent timer variable overflow and MINIMUM_PLANNER_SPEED is a machine-adjustable parameter (not sure exactly why it would need adjusted?). A solution may be to NOLESS MINIMUM_PLANNER_SPEED using MINIMUM_STEP_RATE in next_entry_speed = _MAX(TERN0(HINTS_SAFE_EXIT_SPEED, SQRT(safe_exit_speed_sqr)), float(MINIMUM_PLANNER_SPEED));

On another issue to make behavior more consistent across platforms is there any reason why the ISR counter needs to be variable size (16 and 32/64 bit)? Why not make it 32bit on all platforms (or, some other width calculate from a consitent number of ISR timer periods)? There isn't any significant code or performance overhead in inc/dec with a wider counter on 16bit CPUs.

[mh-dm]

But if the planner ended up planning a move at the integer-truncated speed then there would be a slight difference between the planned speed and the speed that results from Stepper::calc_timer_interval(). This could mean the laser power is slightly wrong for the actual speed that is generated in the case where the slowest possible speed is planned.

@tombrazier This is a great point. Laser power being wrong is something I missed in my newest #27035 which just depends on calc_timer_interval() to ensure there's no timer overflow.

I think this PR is pretty much right at commit https://github.com/MarlinFirmware/Marlin/commit/3b887b468b3fd8a063f60c29e02b1d3dc2ef64f3. In that commit, the planner will not plan a move that is slower than the stepper module can step and block->nominal_rate will never be less than initial_rate or final_rate.

To summarize that commit, it's this:

// planner.h
#define MINIMAL_STEP_RATE _MAX((STEPPER_TIMER_RATE / HAL_TIMER_TYPE_MAX), 1)   // steps/s max. To prevent timer overflow, slowest is 1 step/s

// planner.cpp calculate_trapezoid_for_block()
NOLESS(initial_rate, uint32_t(MINIMAL_STEP_RATE));
NOLESS(final_rate, uint32_t(MINIMAL_STEP_RATE));
NOLESS(block->nominal_rate, (uint32_t)MINIMAL_STEP_RATE);

I have 2 issues with this:

• It should perfectly match calc_timer_interval() which is the same concern that @tombrazier points out.
• There are currently a few cases where block->entry_speed_sqr falls under minimum_planner_speed_sqr. Right now MINIMAL_STEP_RATE acts like a kludge for those cases and prevents the worst cases of acceleration spikes. Short explainer: The first step of a segment is always at intial_rate. If it's too slow, it will result in too much accumulated acceleration_time (see stepper.cpp) and that means a very high calculated second step rate, and the difference is the acceleration spike.

Looking at the code, MINIMUM_PLANNER_SPEED may result in next_entry_speed being slower than the calculated MINIMAL_STEP_RATE

@HoverClub I think you are looking at an old version of the code. MINIMUM_PLANNER_SPEED has been replaced with minimum_planner_speed_sqr.

For this change I suggest something like this:

// planner.h or stepper.h
#ifdef CPU_32_BIT
  constexpr uint32_t MIN_STEP_RATE = _MAX((STEPPER_TIMER_RATE / HAL_TIMER_TYPE_MAX), 1);
#else
  constexpr uint32_t MIN_STEP_RATE = (F_CPU) / 500000U;
#endif

// planner.cpp calculate_trapezoid_for_block()
NOLESS(initial_rate, MIN_STEP_RATE);
NOLESS(final_rate, MIN_STEP_RATE);
NOLESS(block->nominal_rate, MIN_STEP_RATE);
NOLESS(initial_rate, uint32_t(MINIMAL_STEP_RATE));
NOLESS(final_rate, uint32_t(MINIMAL_STEP_RATE));
NOMORE(initial_rate, block->nominal_rate);
NOMORE(final_rate, block->nominal_rate);

That is, apply block->nominal_rate fix from #27013 but also MIN_STEP_RATE to avoid the laser power issue. Then later #27035 can remove the legacy MINIMAL_STEP_RATE 120 limit.

[tombrazier]

@HoverClub

Looking at the code, MINIMUM_PLANNER_SPEED may result in next_entry_speed being slower than the calculated MINIMAL_STEP_RATE

MINIMUM_PLANNER_SPEED doesn't exist any more. I removed it in #26198. [Edit: I see @mh-dm has also pointed this out.]

is there any reason why the ISR counter needs to be variable size (16 and 32/64 bit)?

It depends a lot on what is available on the physical hardware. Marlin works on a wide range of MCUs.

[tombrazier]

@mh-dm I think rather than have both MIN_STEP_RATE and MINIMAL_STEP_RATE I would be inclined just to ensure the rounding error is not an issue with something like

#ifdef CPU_32_BIT
  constexpr uint32_t MINIMAL_STEP_RATE = _MAX((STEPPER_TIMER_RATE / HAL_TIMER_TYPE_MAX), 1);
#else
  constexpr uint32_t MINIMAL_STEP_RATE = (F_CPU) / 500000U;
#endif

// planner.cpp calculate_trapezoid_for_block()
NOLESS(initial_rate, MINIMAL_STEP_RATE + 1);
NOLESS(final_rate, MINIMAL_STEP_RATE + 1);
NOLESS(block->nominal_rate, MINIMAL_STEP_RATE + 1);

It's late here and my brain might not be operating at top speed so feel free to point out if I've missed something.

[mh-dm]

Only with #27035 submitted it is possible to safely remove the MINIMAL_STEP_RATE of 120 for initial/final_rate which is why removing it should be left as part of it.

[thinkyhead]

Only with https://github.com/MarlinFirmware/Marlin/pull/27035 submitted it is possible to safely remove the MINIMAL_STEP_RATE of 120 for initial/final_rate which is why removing it should be left as part of it.

@mh-dm — With your other PRs, we can probably just…

• merge this, then
• resolve conflicts in those PRs, and
• do a pass to make sure everything matches up properly to the modified implementation.

Does that sound like a good way to go? I'd like to get your other PRs merged, then over the coming weeks ahead of the next point release we'll see if we hit any unexpected edge cases.

[mh-dm]

Only with #27035 submitted it is possible to safely remove the MINIMAL_STEP_RATE of 120 for initial/final_rate which is why removing it should be left as part of it.

@mh-dm — With your other PRs, we can probably just…

• merge this, then

I'm trying to understand. This change, in its current state, just removes MINIMAL_STEP_RATE. Removing MINIMAL_STEP_RATE now/first would mean having the bugfix branch in a rather unsafe state.

[HoverClub]

// planner.h or stepper.h
#ifdef CPU_32_BIT
  constexpr uint32_t MIN_STEP_RATE = _MAX((STEPPER_TIMER_RATE / HAL_TIMER_TYPE_MAX), 1);
#else
  constexpr uint32_t MIN_STEP_RATE = (F_CPU) / 500000U;
#endif

// planner.cpp calculate_trapezoid_for_block()
NOLESS(initial_rate, MIN_STEP_RATE);
NOLESS(final_rate, MIN_STEP_RATE);
NOLESS(block->nominal_rate, MIN_STEP_RATE);
NOLESS(initial_rate, uint32_t(MINIMAL_STEP_RATE));
NOLESS(final_rate, uint32_t(MINIMAL_STEP_RATE));
NOMORE(initial_rate, block->nominal_rate);
NOMORE(final_rate, block->nominal_rate);

Unless I'm misunderstanding, it is perfectly OK for final_rate to be more than nominal_rate or initial_rate to be greater than nominal_rate. Using NOMORE assumes the trapezoid is accelerate->cruise->decelerate.

[tombrazier]

I'm trying to understand. This change, in its current state, just removes MINIMAL_STEP_RATE. Removing MINIMAL_STEP_RATE now/first would mean having the bugfix branch in a rather unsafe state.

Indeed. And that is not what we want. We persuaded @HoverClub to change his good PR into something less good. I still think what I said here.

[mh-dm]

From the other discussion:

Is there any reason to delay merging #26881?

No, there isn't. It could be improved further but it's better as it is than not merging it and I can do the extra improvements at some point.

There is. I'll rephrase a comment I've made previously.

Right now, the MINIMAL_STEP_RATE of 120 acts like a kludge to prevent the worst cases of acceleration spikes. Short explainer: The first step of a segment is always at intial_rate. If it's too slow, it will result in too much accumulated acceleration_time (see stepper.cpp) and that means a very high calculated second step rate, and the difference is the acceleration spike.

To see acceleration spikes (and maybe even lost steps) with this PR in this current state, just test with something that results in a low junction speed / a low initial_rate. For example set a low jerk of 0.1mm/s, high acceleration of 2000+mm/s^2 and a speed of 100+mm/s.

[thinkyhead]

I went ahead and added a commit applying suggestions from the discussion.

MINIMAL_STEP_RATE is an integer but STEPPER_TIMER_RATE / HAL_TIMER_TYPE_MAX generally won't be.

It will always be an integer. If STEPPER_TIMER_RATE > HAL_TIMER_TYPE_MAX it may be > 1, otherwise it will just be 1. Let's look at some samples: and see if we always get what we want:

HC32…

• STEPPER_TIMER_RATE = (HAL_TIMER_RATE / STEPPER_TIMER_PRESCALE) => (50000000 / 16) = 3125000
• HAL_TIMER_TYPE_MAX = 0xFFFF
• MINIMAL_STEP_RATE = (3125000 / 0xFFFF) = 47

Teensy 3.1/3.2

• STEPPER_TIMER_RATE = HAL_TIMER_RATE => 7500000
• HAL_TIMER_TYPE_MAX = 0xFFFFFFFF
• MINIMAL_STEP_RATE = (7500000 / 0xFFFFFFFF) = 0

SAMD21

• STEPPER_TIMER_RATE = HAL_TIMER_RATE => F_CPU => 48000000
• HAL_TIMER_TYPE_MAX = 0xFFFFFFFF
• MINIMAL_STEP_RATE = (48000000 / 0xFFFFFFFF) = 0

I presume that the MINIMAL_STEP_RATE really represents the smallest number of steps-per-second that we can handle given our timer size, and when it comes down to our architecture, the smallest number we permit is 1 step per second. Of course, that might just mean the smallest number of interrupts per second, whereas we could allow much lower feedrates, presuming we don't require a step event to occur on every stepper interrupt. Anyway, I've never personally tried it, so I don't know yet what happens when you request a feedrate that would result in under one step per second.

Is that all that the MINIMAL_STEP_RATE does?

[tombrazier]

Right now, the MINIMAL_STEP_RATE of 120 acts like a kludge to prevent the worst cases of acceleration spikes. Short explainer: The first step of a segment is always at intial_rate. If it's too slow, it will result in too much accumulated acceleration_time (see stepper.cpp) and that means a very high calculated second step rate, and the difference is the acceleration spike.

So we need #27035 before redefining MINIMAL_STEP_RATE as the speed the stepper driver will actually use.

[tombrazier]

@mh-dm Can you pull the min_entry_speed_sqr change out of #27035 and submit it as a standalone PR? I think that would work. Then it can be merged first, followed by this PR. And, as a bonus, it will make the changes in #27035 a bit simpler to read.

[tombrazier]

It will always be an integer. If STEPPER_TIMER_RATE > HAL_TIMER_TYPE_MAX it may be > 1, otherwise it will just be 1. Let's look at some samples: and see if we always get what we want:...

@thinkyhead I mean if you type the division into a calculator, it won't be generally be an integer. If we take just the first case, for HC32, the step rate that the stepper will generate is 3125000 / 0xFFFF, which is 47.68444342717632. Truncating to an integer takes that to 47, which is not achievable. However I'm splitting hairs. A 1.4% difference in the speed used to calculate laser power is probably not a great problem.

[mh-dm]

@mh-dm Can you pull the min_entry_speed_sqr change out of #27035 and submit it as a standalone PR? I think that would work. Then it can be merged first, followed by this PR. And, as a bonus, it will make the changes in #27035 a bit simpler to read.

@tombrazier Based on your request I started PR #27089 but I kinda hesitate to recommend this option. Adding min_entry_speed_sqr necessarily touches the planner forward/backward passes that will then be further changed by #27035. I don't yet have a good feel for the expected level of testing/stability each PR should get before being accepted into bugfix. If it's high then I'd rather spend it once on #27035 as a whole (esp. since I already did test it out). If it's not that high then #27089 is good to go.

[thinkyhead]

Of course, we shouldn't leave the branch in a known broken state, so although it's good to have 27089 as a separate PR that can be merged first (for clarity with the changes to follow) we should just decide when this PR and 27035 are "good enough" and I'll merge them all at the same time. Then we should do some testing soon after to make sure everything is stable and nothing is wonky, then continue repairs from there. If it ends up needing to be reverted for any reason, it will get reverted as a unit.