Eliminate skipped steps due to inaccurate pulse timing

[sjasonsmith]

Description

This is a rework of the way step pulses are timed, so that a minimum step can be guaranteed.

I am sure there will be some concerns and discussion surrounding this approach. While solving some problems, it introduces some complications, which I mention later in the PR. I will be away for a week, so wanted to post what I have so people can begin exploring the idea, or try it out on their printers if they find it promising.

I don't consider the code cleanup complete by any means (especially in stepper.h). I expect to need to put additional work into this when I get back, if we choose to use it.

How does it work before this change?

1. Timer is read at the beginning of the stepper loop.
2. Offsets of all step transitions are calculated from initial timer value using hard-coded cycle counts.
3. Interruptions such as systick of software serial can interfere, without changing the timer ticks at which steps transitions will occur.

Example of pulse which is shorter than MINIMUM_STEPPER_PULSE (DRV8825)
Configured 2us pulse is < 1us due to SysTick interference during timer access.

Example of short pulse (and missed step) during multi-stepping

How does it work after this change?

1. Each stepper pulse edge is timed relative to the previous edge.
2. The only constant cycle counts needed are the times to work with the timer.
3. Some logic from PULSE_START was extracted to PULSE_PREP.
   • Reduces "spread" from the rising edge of the first stepper to the rising edge of the last, allowing less error when timing after the last rising edge.
   • PULSE_PREP work is performed during the low stage of the prior pulse.
   • Allows logic to be "hidden" inside pulses when multi-stepping.
   • Allows timer interactions to be closer to pulse edges.
   • XYZ "spread" on AVR is reduced from ~7.7us to ~700ns
   • XYZ "spread" on LPC1768 is reduced from ~300ns to ~79ns

XYZ Rise-Time Spread before change, on LPC1768:

XYZ Rise-Time Spread after change, on LPC1768:

Benefits

• Guarantees minimum duration of step, as defined in MINIMUM_STEPPER_PULSE, resolving skipped steps due to inaccurate pulse timing.
• Reduces dependence of hard-to-maintain timing constants (also a concern)
• Improves pulse consistency, regardless of the location they come from (stepper ISR, Linear Advance, etc.)
• Allows sub-1us pulse periods by setting a maximum rate without a minimum period.

Concerns

• There is currently a very small timing degradation, on some combinations of hardware. This can likely be solved with a little more tuning I didn't have time for before posting this.
• Cycle-count constants are still used to calculate maximum step rates. This change doesn't update those, and they will still be hard to maintain as anything changes inside the stepper ISR. I'm really not sure how to deal with this right now.
• TMC step rates are not as fast as I would like. I foresee a future change on top of this to optimize for very short pulse durations, such as the 100ns acceptable to TMC drivers. This would bypass timer usage, as querying the timer is quite slow on all platforms.

Related Issues

#11205 - User tested an early experiment with a similar solution
#11047 - Probably will be fixed
#12403 - Fairly broad issue, likely resolves some of the issues, especially where sBase is mentioned

[sjasonsmith]

Here is a picture showing the behavior change with an actual print. These are the same gcode printed with the same printer configuration. The only difference is that the cube on the write was printed with firmware including this PR.

These were printed on an MKS sBase, which has built-in 8825 drivers. The printer is not very good or well-tuned, so there are some other artifacts in the print, but the main point is the straightness of the edges and consistency of extrusion.

[boelle]

its marked as draft but i took the artistic freedom to add the do not merge label, just to be safe

[ManuelMcLure]

How does this interact with SQUARE_WAVE_STEPPING?

[sjasonsmith]

How does this interact with SQUARE_WAVE_STEPPING?

This is a good question. I didn't explicitly test it (yet). Thanks for the reminder.

At one point I had it working so that specifying MINIMUM_STEPPER_PULSE=0 along with a MAXIMUM_STEPPER_RATE would essentially skip the first delay, but I think that behavior was lost when I reworked things to be more AVR-friendly. I will have to revisit that scenario.

There is a lot of room for TMC improvement, in general. The timer interactions are quite slow, and I can get higher performance by simply delaying 100ns, rather than reading a timer, which takes about 230ns on LPC1768. I went back to the timer approach to avoid exploding AVR, where it is extremely important to perform as much logic as possible while waiting for timers.

If this goes forward, I expect a follow-up to improve speed on 32-bit. It just would've added a lot of noise to this change trying to do everything at once.

[sjasonsmith]

This is a reminder to myself to check the babystepping implementation as well, to see if it will need updating.

[sjasonsmith]

I was surprised by the error on STM32 about F_CPU not being constant, but I see similar issues have been encountered before. That makes sense when considering the chip can probably be slowed down to save power.

[thinkyhead]

Thank you for taking a close look at this. Marlin's current approach is definitely imperfect, and I'm not surprised to see missed (micro) steps are happening.

In an ideal world MCUs would give you a pin mode where the output turns itself LOW after some duration whenever it gets turned HIGH (or vice-versa)…. Maybe in the future.

I'll review your approach more closely and look at the trade-offs. Pulse duration is a fundamental issue, so it will be good to get our algorithms working solidly.

[sjasonsmith]

I'll be down in Austin all of next week so won't be able to try out any new ideas right away. I should be able to get on here semi-regularly to discuss ideas and concerns.

I fully expect some controversy, as this is a significant paradigm shift to the timing mechanisms in this ISR, and I would be suspicious if there were not concerns.

Right now my largest concern is the maximum stepper rate calculations. I would love to eliminate all those hard-coded cycle counts, and the maintenance that goes with them. Maintaining those cycle counts create a huge barrier of entry to anyone wanting to improve this code, and are likely not consistently perfect as compilers and processor architectures change.

[ejtagle]

In an ideal world MCUs would give you a pin mode where the output turns itself LOW after some duration whenever it gets turned HIGH (or vice-versa)…. Maybe in the future.

Most MCUs have that exact ability , but just for selected pins....

[thinkyhead]

I'll be down in Austin all of next week so won't be able to try out any new ideas right away. I should be able to get on here semi-regularly to discuss ideas and concerns.

I'm in Austin all week too. All the time, really….

[thinkyhead]

Most MCUs have that exact ability , but just for selected pins....

I wish we only had to support a single MCU and board, already organized with all these factors in mind. But of course we have to maintain some time-checking method for AVR (where it's adequate) and then we need to provide some time-based method for any STEP pins that don't have this special mode. So it's another thing we could check for as we now do with PWM, then use whenever possible….

Looking to the future, modern boards will benefit from a 32-bit-only firmware built from the ground up to:

• allocate pins and their modes to features / functions, then
• abstract the needed protocols on top of that layer, and then
• add the needed abstract interfaces on top of that layer.

…where we have our own methods to allocate SW SPI and SW Serial as needed.

Two approaches rise to the top:

• Re-architect Marlin around an RTOS that has broad platform support
• Add a HAL to RepRapFirmware, then port over our hardware support and features.

So far my preference is to build on top of RepRapFirmware because it has already solved some low level problems and provides a framework for adding new features. However, it doesn't have a full Hardware Abstraction Layer at the level of Marlin, but it supports a couple of ATSAM boards.

[ejtagle]

Most MCUs have that exact ability , but just for selected pins....

I wish we only had to support a single MCU and board, already organized with all these factors in mind. But of course we have to maintain some time-checking method for AVR (where it's adequate) and then we need to provide some time-based method for any STEP pins that don't have this special mode. So it's another thing we could check for as we now do with PWM, then use whenever possible….

Looking to the future, modern boards will benefit from a 32-bit-only firmware built from the ground up to:

• allocate pins and their modes to features / functions, then
• abstract the needed protocols on top of that layer, and then
• add the needed abstract interfaces on top of that layer.

…where we have our own methods to allocate SW SPI and SW Serial as needed.

Two approaches rise to the top:

• Re-architect Marlin around an RTOS that has broad platform support
• Add a HAL to RepRapFirmware, then port over our hardware support and features.

So far my preference is to build on top of RepRapFirmware because it has already solved some low level problems and provides a framework for adding new features. However, it doesn't have a full Hardware Abstraction Layer at the level of Marlin, but it supports a couple of ATSAM boards.

I do not know if it call my experience fortunate or infortunate, but right now, there is no vendor supplied HAL that targets all the MPUs that are being brought into the market.

RTOS support for new MCUs is lagging quite a bit, so moving to an RTOS would not solve anything for newer MCUs, as we would have to add support for them ourselves.

So, i think your idea of building over Reprap firmware, it probably the most sane approach here. Marlin has to support a ton of different hardware platforms, thus, most of the time, there is simply no way to ensure specific hw features (such as PWM on STEP pins) is available.

So, most of the time a software fallback must exist (as Marlin already does with mostly everything)

The only thing you usually can count on are at least having 1 o 2 timers to create periodic interrupts... This pull request tries to improve the pulse width without using extra hardware resources, so, it is a good step in the right direction

[thinkyhead]

How high is our confidence in this change? If we merge this for the wider test audience, should we expect a small flood of support followup?

[sjasonsmith]

How high is our confidence in this change? If we merge this for the wider test audience, should we expect a small flood of support followup?

I think a casual user won't notice anything changed. An MKS sBase user will sing it praises. A power user saturating their 32-bit CPU with LV8729 pulse interrupts might report a minor drop in max speed. I don't know whether anyone actually falls into that last category.

I'm getting ready to run a couple sanity checks on AVR followed by STM32. I'll post another update later tonight.

[sjasonsmith]

I have played with several different optimizations, in particular for drivers with very fast timing (TMC and LV8729). Some minimal improvement is possible, but at the cost of added churn and complexity. I decided not to push forward with those changes for now.

[BarsMonster]

I've tested this change with SQUARE_STEPPING on MKS GEN-L (AVR) in the context of the issue I am having ( #15473 ), and found no changes to better of worse.

[Lord-Quake]

@sjasonsmith Yesterday afternoon and this morning I worked on the issue. After numerous tests with different bug fix versions. I have come to the conclusion that the your modifications were not related to my waves seen in the test objects. I often like to use threaded objects to test precision. It seems this particular filament is sensitiv to shrinkage when the walls have a certain minimum thickness, enough to be felt and seen. I did a slight change to the object and the issue was resolved. Subsequent runs on two different machines were all successful.
Here one test print from this morning of the threaded ring for reference (last used commit f339a39)

[sjasonsmith]

@Lord-Quake, thanks for the update!

[ruggb]

@sjasonsmith OMG - I thought my printer was printing pretty good b4 this bug fix. This is the 1st fix that I have actually SEEN results from. I have not printed a lot since, but my printer is definitely much happier. GREAT JOB Jason. Mine is a CoreXY, home built from Carl Feneck's plans and upgraded with a piezo nozzle probe/endstop. My steppers do 1/32 microstepping and the roughness I saw previously was not signifficant, but was there and it is now gone. obviously, it was missing a few steps but at 1/32 it wasn't terribly obvious. THANK YOU.

[sjasonsmith]

@ruggb, what controller and drivers do you use? I expect that most improvement will be seen by 32-bit DRV8825 users, but if it has helped other configurations I would like to know as well!

[ruggb]

I have an Arduino Mega 2560 with Ramps and 8825 drivers.
I need to print something big - last prints were small, but beautiful.

[italocjs]

I'm having the exact issue shown here, but using a arduino mega + a4988, i'm trying to seek the cause, but so far nothing mechanical seems wrong. i'll try burning the latest bugfix with this PR to see if it has any effect, the only "different" thing i have on my machine is that it uses two steppers for the Y (two drivers) to ensure no skew, its a simple cartesian.

Here is a picture showing the behavior change with an actual print. These are the same gcode printed with the same printer configuration. The only difference is that the cube on the write was printed with firmware including this PR.

These were printed on an MKS sBase, which has built-in 8825 drivers. The printer is not very good or well-tuned, so there are some other artifacts in the print, but the main point is the straightness of the edges and consistency of extrusion.

[ruggb]

So what are you waiting for? It is missing steps. Flyback diodes on the steppers may also help, but the fix in firmware is real.

[richfelker]

I'm also wondering why it's not upstream. Are there concerns about problems with the change?

[InsanityAutomation]

See where it says merged up above? It is upstream.

[richfelker]

OK, GitHub is just awful and lies that it was merged when it was actually committed as a squash (and doesn't provide any link to the squash commit, arrg). For others wondering, it's 1bad8f1.

[vivian-ng]

@sjasonsmith Sorry for digging up an old PR, but I am trying to figure out what was changed to Marlin to cause some issues with stepper control on the ESP32 when I came across this. Not really related to my issue, but I was looking through the code and have a few questions:

1. In Configuration_adv.h,

 * Maximum stepping rate (in Hz) the stepper driver allows
 *  If undefined, defaults to 1MHz / (2 * MINIMUM_STEPPER_PULSE)

This means that if MAXIMUM_STEPPER_RATE is not defined, it should be
1000000 / (2 * MINIMUM_STEPPER_PULSE)
but in Conditionals_post.h

#ifndef MAXIMUM_STEPPER_RATE

automatically assigns the value based on the driver type, regardless of what has been defined for MINIMUM_STEPPER_PULSE. Any reason why?

2. In stepper.cpp, there are many checks along the line of

#if (MINIMUM_STEPPER_PULSE || MAXIMUM_STEPPER_RATE)

but this will always be true, since MAXIMUM_STEPPER_RATE is not a zero value (unless explicitly defined as 0 in Configuration_adv.h). Can I ask what is the intended effect of this check so that I can understand the logic better?

[sjasonsmith]

1. In stepper.cpp, there are many checks along the line of

#if (MINIMUM_STEPPER_PULSE || MAXIMUM_STEPPER_RATE)

but this will always be true, since MAXIMUM_STEPPER_RATE is not a zero value (unless explicitly defined as 0 in Configuration_adv.h). Can I ask what is the intended effect of this check so that I can understand the logic better?

Can you point me to a specific instance you are curious about. It's been a while since I have thought about that. Certainly come of the comments int he configuration files aren't quite right, even before I changed anything.

[vivian-ng]

@sjasonsmith

Can you point me to a specific instance you are curious about. It's been a while since I have thought about that. Certainly come of the comments int he configuration files aren't quite right, even before I changed anything.

It is just a general observation. For example, in Stepper::stepper_block_phase_isr() and Stepper::stepper_block_phase_isr(), what used to be just #if MINIMUM_STEPPER_PULSE had been replaced by #if (MINIMUM_STEPPER_PULSE || MAXIMUM_STEPPER_RATE) so I was wondering why, since this is likely to evaluate to True unless MAXIMUM_STEPPER_RATE is explicitly defined as 0 in Configuration_adv.h.

[sjasonsmith]

what used to be just #if MINIMUM_STEPPER_PULSE had been replaced by #if (MINIMUM_STEPPER_PULSE || MAXIMUM_STEPPER_RATE) so I was wondering why

@vivian-ng if either of those is defined there is a need to time pulses. There are probably plenty of opportunities to clean up and improve that code further. You are probably right that it would be difficult for that to ever be false.

[Roxy-3D]

The fact you caught a 'short pulse' on the oscilloscope (in the opening post) suggests to me we need to fully understand what is happening. We need to make sure all the setup and hold times are being met. It would be great if we could tie this to the layer shift problem we have been chasing for the last year.

[sjasonsmith]

The fact you caught a 'short pulse' on the oscilloscope (in the opening post) suggests to me we need to fully understand what is happening. We need to make sure all the setup and hold times are being met.

@Roxy-3D, the poblem was that the stepper ISR assumed that it would never be interrupted, and based all its timing decisions on offsets added to the initial value of the step timer. This isn't true, since systick and serial ISRs run at higher priority that the stepper. This created race conditions where interrupt at the wrong time would mess up its pulse generation.

I haven't looked in detail, but that is probably not the case for the other setup times. I believe they are just simple delays, which could get longer if interrupted, but should never get shorter.

[Roxy-3D]

OK... So with this understanding about the serial interrupts... Can we make a small tweak to the code to guarantee we never short change the length of that step pulse? If so, it would probably be worth while to do that and see if it helps with the layer shift problem. I certainly would be willing to run some tests with a change that did that!

[sjasonsmith]

@Roxy-3D, that is what this PR was all about, it went in months ago. This thread just got re-awoken by someone asking a question about it.

[Roxy-3D]

Thank You for the help understanding that! Good!

[thinkyhead]

I'm currently looking at an issue related to "noisy steppers with input shaping enabled" which seems to go away when OLD_ADAPTIVE_MULTISTEPPING is enabled. Always risky to mess around in this area of the code….

Anyway, as I was poking around I noticed that this PR made an interesting change:

-   // Decrement the count of pending pulses to do
-   --events_to_do;
-   // For minimum pulse time wait after stopping pulses also
-   if (events_to_do) {
-     // Just wait for the requested pulse duration
-     while (HAL_timer_get_count(PULSE_TIMER_NUM) < pulse_end) { /* nada */ }
-     #if MINIMUM_STEPPER_PULSE
-       // Add to the value, the time that the pulse must be active (to be used on the next loop)
-       pulse_end += hal_timer_t(MIN_PULSE_TICKS);
-     #endif
-   }
- } while (events_to_do);
+   #if (MINIMUM_STEPPER_PULSE || MAXIMUM_STEPPER_RATE) && DISABLED(I2S_STEPPER_STREAM)
+     if (events_to_do) START_LOW_PULSE();
+   #endif
+ } while (--events_to_do);

I noticed that events_to_do is always going to be non-zero when we get to this line:

if (events_to_do) START_TIMED_PULSE(); // formerly START_LOW_PULSE

…so it could just be…

START_TIMED_PULSE();

Since the condition was left in place, it's not clear whether there was an intention to skip START_TIMED_PULSE on the last iteration or not.

Can anyone verify that START_TIMED_PULSE should always be called here? If so then we can remove the check for events_to_do from that line and save a few cycles.

[sjasonsmith]

@thinkyhead in the current rendition of the code it looks like this:

  #if ISR_MULTI_STEPS
    if (events_to_do) START_TIMED_PULSE();
  #endif

} while (--events_to_do);

I think the intent is that it will use the timer to enforce a delay between steps when using ISR_MULTI_STEPS. The condition should most likely change to this, so that it only starts the timer if it is going to execute another pulse in the same ISR.

    #if ISR_MULTI_STEPS
      if (events_to_do > 1) START_TIMED_PULSE();
    #endif

In general, multi-stepping leads to terrible step spacing, and hopefully doesn't play much of a role on modern 32-bit MCUs.

[sjasonsmith]

I'm currently looking at an issue related to "noisy steppers with input shaping enabled" which seems to go away when OLD_ADAPTIVE_MULTISTEPPING is enabled.

If this can be reproduced with a simple movement, then I would recommend replicating the configuration as much as possible in the simulator and capturing the simulated step waveform. Something may stand out that looks very different when enabling OLD_ADAPTIVE_MULTISTEPPING.