[2.0.x] Fix stepper/planner race condition, Stepper pulse timer

[ejtagle]

As pointed out by @AnHardt in #11047 , and measured by @p3p , there seems to be something wrong with the Planner, that somehow , when jogging the Pronterface controls, outputs a first movement block that properly accelerates and decelerates, and then a 2nd block that starts at maximum feed rate and then decelerates.

There is an strong suspicion that this is caused by a race condition between the Planner and the Stepper ISR, that goes like this: When there are few blocks queued, the planner starts the forward and backward passes. And, at some point, there is a chance the block entry and exit speed will be modified before tagging it as "RECALCULATE".

That allows the Stepper to "kidnap" the block and mark it as busy after the entry and exit speed was updated by the planner, but before the new stepper parameters of the block are updated in the calc_trapezoids() function.

As the block is marked as BUSY, the planner tries to calculate the new trapezoid parameters, but is unable to update the block.

But, as the block entry and exit speed IS updated, then the planner will use those updated entry and exit speeds to plan the rest of the blocks: The result is a 1st block that decelerates to 0 (but the planner has replanned it and it should NOT decelerate!) and the 2nd block uses that assumption and starts accelerated.

The fix is to mark the block as RECALCULATE before updating its entry and exit speeds, and not after as it was done previously. Marking the block as RECALCULATE inhibits the Stepper ISR using and "kidnappiing" the block, allowing proper update of the trapezoidal form.

@AnHardt pointed out 2 places, and I found out 2 other places where this race condition exists. So, hopefully this PR will fix the "Proterface jog losing steps issue"

The other thing found by @p3p doing actual measurements on hw is that the width of the Stepper pulses is a little bit short compared with the configured value. This was caused by the fact the program takes some time to toggle the STEP line, but the pulse timing is measured from the point the first STEP line is set, to the first STEP line that is reset.

That makes the last (typically the extruder stepper) to have a pulse shorter than the specified one.

I did disassemble the code to count the cycles that takes to SET the STEPPER line for both ARM and AVR targets, and now that time is also taken into account in the timing calculations, hopefully will ensure all pulse widths are at least the specified time long.

[thinkyhead]

Great analysis. Let's hope this does the trick and fixes some E axis anomalies too!

[p3p]

@ejtagle update looks good, appears to fix minimum pulse length on all axes

Unfortunately it doesn't seem to fix the odd planner behaviour

[ejtagle]

@p3p : Unfortunately, there is still a race condition there, I knew it was, but there is no easy fix for it. The reasoning is simple:

-The Planner does not take into consideration if a block is BUSY or not. That means that it could be using the executing block as reference (-- that is perfectly fine--), and then, at some point, that block is discarded (it just completed execution) and the next block becomes busy (because the planner has accessed it to use it as reference, but not modified it yet)
But, eventually, the planner decides to modify that block (that has become busy without the planner knowing that) and marks the block that is now busy as "RECALCULATE", but it is just too late, the block will be recalculated but its parameters can´t be changed, so the calc_trapezoid() function will not update its values.

The result is exactly the same as described above: Planner trying to join blocks, but not able to update the executing block...

This problem is caused by the inherent race condition between the planner and the stepper. The planner just takes time to execute and it is not an option to disable the stepper ISR, as that would cause severe stutter.

Excluding the last(s) blocks is also not the answer - It´s a game that does not always work, because there is no easy way to predict block consumption speed.

There are 2 possibilities here:

1. Update a running block
2. If we detect the situation (busy block that is tried to be updated or marked as "RECALCULATE" (that is the easy part!) undo the speed changes and backpropagate them. Not easy at all...

If you want to detect the situation, just add at the end of Planner::calculate_trapezoid_for_block()
to the last if of the function, an else clause that

SERIAL_ECHOPGM("ERROR: Trying to update busy block");

I must admit this race condition is mostly diabolically hard to reproduce if you try to do it by hand, as it requires specific and perfect timing to trigger it, but Pronterface seems to be the "specialist"...

[skarcha]

Hi!

After merging this PR I got this warning:

In file included from sketch/src/module/stepper.cpp:80:0:
sketch/src/module/stepper.cpp: In static member function 'static void Stepper::stepper_pulse_phase_isr()':
sketch/src/module/stepper.h:182:100: warning: large integer implicitly truncated to unsigned type [-Woverflow]
 #define ADDED_STEP_TICKS (((MIN_STEPPER_PULSE_CYCLES) / (PULSE_TIMER_PRESCALE)) - (MIN_PULSE_TICKS))
                                                                                                    ^
sketch/src/module/stepper.cpp:1322:40: note: in expansion of macro 'ADDED_STEP_TICKS'
   const hal_timer_t added_step_ticks = ADDED_STEP_TICKS;

[ejtagle]

@skarcha : Yes, i know there is a warning. I have tried nearly everything to get rid of it, didn´t found the cause yet. It only happens in AVR...

[thinkyhead]

Some of MIN_STEPPER_PULSE_CYCLES, PULSE_TIMER_PRESCALE, and MIN_PULSE_TICKS are forced to be unsigned long, some are default type, which is signed. To get rid of the warning, coerce them all to the same type.

[ejtagle]

Trust me @thinkyhead . The first thing i tried is exactly that. And the warning was still there. I suspect that, as we are using F_CPU, this is right now unsolvable (and the warning only happens in AVR GCC

[skarcha]

@ejtagle @thinkyhead. Ok, after some tests, I think the problem is "the minus sign", I mean: MIN_PULSE_TICKS could be greater than ((MIN_STEPPER_PULSE_CYCLES) / (PULSE_TIMER_PRESCALE)) so the compiler is complaining about it. You can test it changing the minus sign by a plus sign, and the warning disapear.

Unfortunately I don't know how to solve these situations, but I hope this helps... ;)

[thinkyhead]

Trust me @thinkyhead . The first thing i tried is exactly that.

It worked! aa4cd2e

[thinkyhead]

I see that this PR originally defined ISR_START_STEPPER_CYCLES along with ISR_START_Y_STEPPER_CYCLES and others for each axis. Most of those counts are defined as zero, and they don't seem to be used anyplace at this time. I'm trying to get finishing touches on more configurable linear axes at #19112 and if these extra cycles could ever add up then it would be good to account for those, especially as we get the point of driving 8 or 9 coordinated linear axes.