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)?
#20150
#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 65934ee, 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 65934ee 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]

@tombrazier in the latest diff MINMAL_STEP_RATE and these limits are removed entirely.

Are you confident that this is a correct thing to do, and that Otherwise the timer will overflow is just an obsolete relic?

[sjasonsmith]

I imagine we should have some testing on this in its final form before merging it.

[GelidusResearch]

I believe there is likely a need to limit the minimum with AVR HAL hardware timers e.g. ATM2560 etc.
see https://github.com/MarlinFirmware/Marlin/blob/bugfix-2.1.x/Marlin/src/HAL/AVR/timers.h
Don't think you can remove them entirely.
regards @descipher

#define TEMP_TIMER_FREQUENCY    (((F_CPU) + 0x2000) / 0x4000)

#define STEPPER_TIMER_RATE      HAL_TIMER_RATE
#define STEPPER_TIMER_PRESCALE  8
#define STEPPER_TIMER_TICKS_PER_US ((STEPPER_TIMER_RATE) / 1000000)

#define PULSE_TIMER_RATE         STEPPER_TIMER_RATE
#define PULSE_TIMER_PRESCALE     STEPPER_TIMER_PRESCALE
#define PULSE_TIMER_TICKS_PER_US STEPPER_TIMER_TICKS_PER_US

#define ENABLE_STEPPER_DRIVER_INTERRUPT()  SBI(TIMSK1, OCIE1A)
#define DISABLE_STEPPER_DRIVER_INTERRUPT() CBI(TIMSK1, OCIE1A)
#define STEPPER_ISR_ENABLED()             TEST(TIMSK1, OCIE1A)

#define ENABLE_TEMPERATURE_INTERRUPT()     SBI(TIMSK0, OCIE0A)
#define DISABLE_TEMPERATURE_INTERRUPT()    CBI(TIMSK0, OCIE0A)
#define TEMPERATURE_ISR_ENABLED()         TEST(TIMSK0, OCIE0A)

FORCE_INLINE void HAL_timer_start(const uint8_t timer_num, const uint32_t) {
  switch (timer_num) {
    case MF_TIMER_STEP:
      // waveform generation = 0100 = CTC
      SET_WGM(1, CTC_OCRnA);

      // output mode = 00 (disconnected)
      SET_COMA(1, NORMAL);

      // Set the timer pre-scaler
      // Generally we use a divider of 8, resulting in a 2MHz timer
      // frequency on a 16MHz MCU. If you are going to change this, be
      // sure to regenerate speed_lookuptable.h with
      // create_speed_lookuptable.py
      SET_CS(1, PRESCALER_8);  //  CS 2 = 1/8 prescaler

      // Init Stepper ISR to 122 Hz for quick starting
      // (F_CPU) / (STEPPER_TIMER_PRESCALE) / frequency
      OCR1A = 0x4000;
      TCNT1 = 0;
      break;

    case MF_TIMER_TEMP:
      // Use timer0 for temperature measurement
      // Interleave temperature interrupt with millies interrupt
      OCR0A = 128;
      break;
  }
}

[tombrazier]

@tombrazier in the latest diff MINMAL_STEP_RATE and these limits are removed entirely.

Are you confident that this is a correct thing to do, and that Otherwise the timer will overflow is just an obsolete relic?

Not confident enough to merge it without testing. I am fairly sure it is redundant but it needs to be tried.

[thinkyhead]

It would be good to add some code to the test suite that would run a set of values from very small to very large through routines like this to ensure they don't overflow. Meanwhile, those with questions about a math routine can use the Simulator or make a set of test files — e.g., test.h, test.cpp — containing only the Marlin routines that need checking, plus the supporting types, etc. I have just such a set of files that I've occasionally used to figure out macro behavior and things like that.

With that sage advice out of the way, has anyone explored and tested for overflow/underflow yet?

[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.

• [BUG] 120Hz pulse at the beginning of each move/Classic Jerk not functional (?) #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.

[thinkyhead]

Please double-check the results of my merge. It was unclear whether to keep most of the stuff from #27013 or make it look more like the prior state of this PR. Maybe a hybrid is needed.

[mh-dm]

I took another look and the step_rate <= minimal_step_rate case is not correct, although it wasn't correct before either.

STEPPER_TIMER_RATE is at 100-500Mhz, divided by some 4/8 prescale. So somewhere around 10-200 million.
However, HAL_TIMER_TYPE_MAX is usually 0xFFFFFFFF or ~4 billion. So if we're at the minimal_step_rate of 1 (usually) we'll be waiting for HAL_TIMER_TYPE_MAX ticks which at 10-200 million rate will take in the range of 400-20 seconds.

Maybe switch to:
return step_rate > minimal_step_rate ? uint32_t(STEPPER_TIMER_RATE) / step_rate : MIN(STEPPER_TIMER_RATE, HAL_TIMER_TYPE_MAX);

[thinkyhead]

The choice to use HAL_TIMER_TYPE_MAX to prevent a divide by zero in calc_timer_interval originally comes from #25557. The aim was to use the value closest to infinity when the step_rate was equal to zero.

return step_rate ? uint32_t(STEPPER_TIMER_RATE) / step_rate : HAL_TIMER_TYPE_MAX;

This was updated in #25696 to not just apply to a step_rate of zero, but to any step rate below a minimum threshold.

constexpr uint32_t min_step_rate = uint32_t(STEPPER_TIMER_RATE) / HAL_TIMER_TYPE_MAX;
return step_rate > min_step_rate ? uint32_t(STEPPER_TIMER_RATE) / step_rate : HAL_TIMER_TYPE_MAX;

If the value returned from calc_timer_interval is later used in a divide by STEPPER_TIMER_RATE operation that expects a result >= 1, it makes sense to make that our substitute for infinity, but with this value being 1/8 of HAL_TIMER_TYPE_MAX, it means more likelihood of a divide result of 1 or greater.

There is some code that expects HAL_TIMER_TYPE_MAX to be returned. Specifically, code that looks for LA_ADV_NEVER expects it to be equal to HAL_TIMER_TYPE_MAX, and when this value is returned it essentially represents "never."

So, can this really be done? You and I and @tombrazier will need to look closer at this question.