Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TMC Square Wave Stepping mode #14195

Merged

Conversation

Projects
None yet
9 participants
@teemuatlut
Copy link
Member

commented May 31, 2019

Stepper drivers can benefit from a step signal which is closer to 50/50 square wave.
In normal conditions this ensures adequate high/low times.

The changes to Marlin are very basic. In short, from the current ISR code we filter out the STOP calls and on START calls instead of write high we use toggle. This means the step signal "frequency" is halved but we still drive the correct number of steps because a step is executed on both edges of the signal.

The increased timing is especially helpful when using Linear Advance.

If the feature does not cause issues I'd expect it be turned on by default after a test period.

@teemuatlut

This comment has been minimized.

Copy link
Member Author

commented May 31, 2019

All except the last of the (oscilloscope) images are when using Linear Advance on SKRv1.3 (LPC1768).


LPC step pulse width

Upstream Marlin with LA.
This is the likely cause to issues people have had when using Linear Advance with TMC drivers.
A 12.4ns pulse is not enough to satisfy the IC specification.


LPC gcode ISR
LPC gcode step width min step 1

Setting MINIMUM_STEPPER_PULSE to 1 increases the pulse width enough for the driver.


LPC gcode dedge ISR

Enabling SQUARE_WAVE_STEPPING decreases the ISR duration by about 10% but this is solely because we no longer have to use MINIMUM_STEPPER_PULSE.
Outside of LA and when no pulse width forced there is virtually no difference in ISR execution time.
This is expected as we move from write high -> write low to read -> !write.


LPC dedge pulse width

With SQUARE_WAVE_STEPPING the pulse input high/low time is the same as the interval between two ISR calls that execute a step.


This image is with LA turned off and measures X axis when executing a fast move (96000steps/s) on X axis.
(E1STEP == ISR)

LPC dedge step delta

When multistepping inside a single ISR, the waveform starts to look more like current upstream code where the step pin is turned on and off within a single ISR call.
Pulse width requirement remains fulfilled.


image

Step counter to test if the correct number of edges are executed.

@p3p

This comment has been minimized.

Copy link
Member

commented May 31, 2019

Still don't think you should call the feature SQUARE_WAVE_STEPPING it's double edge stepping ;)

@ghost

This comment has been minimized.

Copy link

commented May 31, 2019

Edge (or double edge) stepping does sound a more accurate term than square wave stepping to me.

It's good though either way :)

Means half the number of interrupts needed to drive a stepper motor - if my thinking is right ?

@teemuatlut

This comment has been minimized.

Copy link
Member Author

commented May 31, 2019

No the number of interrupts is the same. The difference is that each ISR only executes one half of the waveform.

The feature is named more after the intent and benefit than actual implementation. Double Edge Stepping or Double Stepping also just sounds like trouble or as if the user would have to compensate in step/mm setting.
The condition where this does not produce a square wave is a high speed situation and is not the normal operating condition.
You can also look at this way; which is the bigger change, the one where we enable a certain mode in the driver or the one to the ISR to execute only half of the wave. The way I see it is the main change is in the ISR to get the benefit of longer pulses and following that we enable dedge to accommodate it. Not the other way around.

@ghost

This comment has been minimized.

Copy link

commented May 31, 2019

So it's to produce the cleanest square'ish step pulses, rather than short spikey step pulses ? .. that's cool if so !

@AnHardt

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

Have you tested double/quad-stepping-mode?

@thinkyhead

This comment has been minimized.

Copy link
Member

commented May 31, 2019

Configuration option naming is often a double-edged sword.

@teemuatlut

This comment has been minimized.

Copy link
Member Author

commented Jun 1, 2019

So it's to produce the cleanest square'ish step pulses, rather than short spikey step pulses ?

I suppose you can call it a cleaner approach.

Have you tested double/quad-stepping-mode?

Double stepping on LPC was shown in the second to last image. I'll see about getting the same measurements with AVR and to introduce quadstepping as well.

Configuration option naming is often a double-edged sword.

Pun intended.

@teemuatlut

This comment has been minimized.

Copy link
Member Author

commented Jun 1, 2019

Did some of the same tests with AVR.
LA enabled but X driver probed.
M92 X80, M203 X1000, M201 X10000, M569 S0 X.


AVR1 dedge ISR
AVR2 dedge step width

16000s/s (200mm/s), double stepping, Square wave enabled


AVR3 dedge ISR quadstep
AVR4 dedge step width quadstep

24000s/s (300mm/s), quadstepping, Square wave enabled


AVR5 upstream ISR quadstep
AVR6 quadstep step width
AVR7 quadstep step gap

24000s/s (300mm/s), quadstepping, Square wave disabled
Time between two active step edges remains about the same (14us vs 9.6us+4.0us, inaccuracies due to manually placed cursors)


AVR8 quadstep ISR interval

24000s/s (300mm/s), Time between ISR calls, Square wave disabled

@LinFor

This comment has been minimized.

Copy link

commented Jun 7, 2019

I'l tested it on lpc and it's works well.
But with this implementation, the entire second half of stepper_pulse_phase_isr() performs unnecessary work, since is busy waiting for execution * _STEP_WRITE (0), which in turn does nothing.
I hope we can throw off of this useless waiting in the future.

@teemuatlut

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

Very good to hear validation from someone else as well =)

I understand that you're mainly talking about all the lines that relate to pulse_end variable?
It sure looks like it could be conditioned away if all set drivers are TMCs, but I'll have to do the multistepping probes again. With that we might actually see improvements in the ISR execution time.

@thinkyhead thinkyhead force-pushed the teemuatlut:square_step_signal branch from 27ed2a8 to b1b0005 Jun 9, 2019

@thinkyhead thinkyhead force-pushed the teemuatlut:square_step_signal branch from b1b0005 to 7f17de6 Jun 9, 2019

@thinkyhead thinkyhead merged commit cccc51e into MarlinFirmware:bugfix-2.0.x Jun 9, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@thinkyhead thinkyhead changed the title Implement TMC SQUARE_WAVE_STEPPING mode TMC Square Wave Stepping mode Jun 9, 2019

@teemuatlut teemuatlut deleted the teemuatlut:square_step_signal branch Jun 9, 2019

Minims added a commit to Minims/Marlin that referenced this pull request Jun 9, 2019

@Bergerac56

This comment has been minimized.

Copy link

commented Jun 12, 2019

Made some tests with original pulse parameter (2 for DRV8825), square wave stepping and lin_advance. In this configuration, the issue for lin_advance is not solved

@teemuatlut

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

I need more information to do anything.

@AnHardt

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

Who wonders?
DRV8825 is not a TMC. So an option specifically made for TMCs can't apply.

@Bergerac56

This comment has been minimized.

Copy link

commented Jun 12, 2019

Sorry. As some people asked explicitely if that solved the issue for linear_advance, I tested it. Moreover, I did not know that it was only for TMC!

nforceroh added a commit to nforceroh/Marlin that referenced this pull request Jun 13, 2019

@drmulligan

This comment has been minimized.

Copy link

commented Jun 14, 2019

Could square wave stepping be applied to other driver types? Wouldn't they also benefit from interrupt based timing on both edges?

@teemuatlut

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

Yes but the implementation would be different.
Here we take advantage the double edge triggering which is unique to TMCs and does not increase the ISR call frequency.
p3p was working on a feature that would bring similar functionality to other driver types by adding a lighter ISR that would just toggle back the step pins.

@p3p

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

This PR is specifically TMC double edge triggering support, general double edge triggering support would use the same method. What drivers support that? I was working on 50% duty cycle pulse "square wave" support, your naming convention coming back to haunt us already @teemuatlut ;)

@drmulligan

This comment has been minimized.

Copy link

commented Jun 17, 2019

@p3p thank you for clarifying. Somehow I missed the distinction between this PR and achieving a 50% duty cycle using a single timer to trigger both the rising and falling edges. I'm sorry for the confusion I caused.

Also thank you for working on that idea,

oechslein pushed a commit to oechslein/Marlin that referenced this pull request Jun 19, 2019

@KingTomaHawk

This comment has been minimized.

Copy link

commented Jul 13, 2019

Tried Linear Advance + Square Wave Stepping on TMC2208 with SKR 1.3 and the Extruder Stops on K-Factor pattern :(
Did I miss something?

@LastDragon-ru

This comment has been minimized.

Copy link

commented Jul 13, 2019

Did I miss something?

You can try

  1. Set MINIMUM_STEPPER_PULSE to 1 or 2
  2. Enable HYBRID_THRESHOLD and set threshold
  3. Disable STEALTHCHOP_E
@KingTomaHawk

This comment has been minimized.

Copy link

commented Jul 13, 2019

Did I miss something?

You can try

  1. Set MINIMUM_STEPPER_PULSE to 1 or 2

Tested with 1 without success, but resulted in wired behavior ( X,Y movement speed slowed more and more down while printing the pattern, E still stops completely )

  1. Enable HYBRID_THRESHOLD and set threshold

Was enabled and set

  1. Disable STEALTHCHOP_E

Didn't tried this. Is it a required change to get it work or just for testing purposes?

@LastDragon-ru

This comment has been minimized.

Copy link

commented Jul 13, 2019

Tested with 1 without success, but resulted in wired behavior

What min/max k-value are you use for pattern?

Is it a required change to get it work or just for testing purposes?

It may help, MONITOR_DRIVER_STATUS may be helpfull too.

Maybe related to #11024 (and other issues about lin_advance + tmc)

@KingTomaHawk

This comment has been minimized.

Copy link

commented Jul 13, 2019

Tested with 1 without success, but resulted in wired behavior

What min/max k-value are you use for pattern?

Is it a required change to get it work or just for testing purposes?

It may help, MONITOR_DRIVER_STATUS may be helpfull too.

Maybe related to #11024 (and other issues about lin_advance + tmc)

I think my pattern was from 0 to 2. But thank you for that link, I have a lot to read there. Do you want me to continue here reporting or at your linked issue?

@LastDragon-ru

This comment has been minimized.

Copy link

commented Jul 13, 2019

Do you want me to continue here reporting or at your linked issue?

I'm a user like you ... just had the similar problem ... 🤷‍♂

X,Y movement speed slowed more and more down while printing the pattern, E still stops completely

One thing. I'm may be wrong but afaik marlin decreases speed if excturer cannot extrude required amount of filament => so you can try increase feedrate/jerk for extruder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.