[2.0.x] Always honor minimum period on stepper pulse generation, and fix timing calculations

[ejtagle]

If MINIMUM_STEPPER_PULSE_TIME was 0, then the minimum period was ignored. Enforce respecting the maximum stepper rate always.

Also fix the improper calculations of timer counts to enforce that maximum stepper rate: Timer counts are not the same as instruction cycle count, so we must convert it using the pulse timer prescaler define

[thinkyhead]

Likewise. If MINIMUM_STEPPER_PULSE isn't defined, this won't compile either.

[thinkyhead]

If MINIMUM_STEPPER_PULSE_TIME was 0, then the minimum period was ignored. Enforce respecting the maximum stepper rate always.

I assumed we would ensure a maximum stepper rate by limiting the frequency of the ISR (giving time back to the main loop) not by adding a pulse delay even when MINIMUM_STEPPER_PULSE_TIME is 0 or undefined. On AVR I assume most of the time there won't be any added delay, but on 32-bit platforms we probably do have the luxury of just limiting the ISR rate.

Anyway, as I've pointed out above, if MINIMUM_STEPPER_PULSE_TIME is undefined then some of this code won't compile. So I've added a conditional to set it to 0 if not defined.

[ejtagle]

Yes, @thinkyhead : The problem happens when multistepping (DISABLE_MULTI_STEPPING disabled, and that is the default) with multistepping, when on the same stepper ISR we send more than one step pulse. When that happens, and MINIMUM_STEPPER_PULSE_TIME == 0, previously the multistepping was done at the maximum rate possible, with no delays at all!

Very dangerous, as there are stepper drivers that will simply ignore the stepping pulses ... 👎

[thinkyhead]

The problem happens with multistepping

Yeah, I just noticed that. So, most of the time for 32-bit multi-stepping probably won't be invoked, so not a big deal.

[ejtagle]

Initially i thought that, but it´s not the case. Z axis usually has a high number of pulses per mm (in my case, 4000) so it happens to be invoked more often than thought initially ;)

[ejtagle]

And on AVR it is even worse, and manifests itself as losing steps when going too fast ... Does it sound familiar to you ? ;)

[ejtagle]

Even if the drivers support the associated stepping rate, the mechanical hardware could lose steps. The situation worsens as soon as you start increasing microstepping.
Better play safe, and always enforce it! 👍

[thinkyhead]

And on AVR it is even worse, and manifests itself as losing steps when going too fast ... Does it sound familiar to you ? ;)

Yes, but in real-world terms it seems to be absent until sometime after 1.1.8…

[ejtagle]

I´d say... Luckily it is absent... 👍 ... Something i have noticed lately is that everything seems to be settling down nicely. Less and less reports of layer shifts, people that say latest bugfix seems to be solving their issues... Very nice! 👍

[ejtagle]

Theoretically ADDED_STEP_DELAY will always be > 0, as the pulse width is always <= 0.5* pulse period, and pulse period is always > 0

[thinkyhead]

Luckily…

By rubber-bands and duct tape.

…everything seems to be settling down nicely…

I'm sure we're getting much closer to the ideal!

[thinkyhead]

Theoretically ADDED_STEP_DELAY will always be > 0…

Makes sense. But someone might decide to set MINIMUM_STEPPER_PULSE_TIME to some large value, and in that case it'll be good to leave out the other delay code.

[ejtagle]

If it tries, then an error should be printed. Again, no driver allows that. Pulse is always recommended to be 50% of the period or less. An incorrect value will most of the time mean badly configured Marlin and bug reports you probably don´t want (I call it: Defensive programming!)

Of course, i am the culprit of some of the bugs that we are fixing... But not all of them... For example, the serial errors were my fault, as some of the stepper and endstops...

[ejtagle]

Now, rechecking this, ADDED_STEP_DELAY could become negative if the MINIMUM__STEP_PULSE > 1/MAXIMUM_STEPPER_RATE ..

Trust me, i`d cause a compilation error on that case ;)

[ejtagle]

The only case where i would leave out the timing code is if the user did not define any limits at all... And in such case, i'd personally just disallow compilation - That is asking for trouble!

[thinkyhead]

Some HALs use a static function to define HAL_TIMER_RATE so unfortunately we can't catch the error at compile time.

[ejtagle]

Yes, that is right