Use/enable TMC2130 DEDGE support in MK3/MK3S #2789
Conversation
Introduce new wrapper macros to tick the stepper pins. Default to the original raising-edge stepping mode. When using the TMC double-edge stepping mode (aka half-wave or square-wave mode) the _LO macros become no-ops.
Add constants for the various required delays in tmc2130.h, which will come in handy for stepper.cpp as well. Move the delays in the _set functions and remove the pauses from the various calling points and macros. Note that the hard-coded pause wouldn't cut it for the stepper ISR, but it's fine for other use cases.
- Avoid all delays when using DEDGE stepping - Correctly account for direction change delays
|
@wavexx @vintagepc is this ready to get merged into MK3? |
Firmware/tmc2130.cpp
Outdated
| #define _DO_STEP_Z TOGGLE(Z_STEP_PIN) | ||
| #define _DO_STEP_E TOGGLE(E0_STEP_PIN) | ||
| #else | ||
| #define _DO_STEP_X { WRITE(X_STEP_PIN, !INVERT_X_STEP_PIN); delayMicroseconds(TMC2130_MINIMUM_PULSE); WRITE(X_STEP_PIN, INVERT_X_STEP_PIN); } |
There was a problem hiding this comment.
calling/inlining delayMicroseconds will definitely take more time than the original "nop" instruction, why was this changed?
There was a problem hiding this comment.
Looks like it's to support variable minimum pulse times. Could probably special-case it in a #define so that if the pulse time is 0 it just nops but otherwise adds a delay call
There was a problem hiding this comment.
Do we really need that nop? It’s not like it makes any real difference
There was a problem hiding this comment.
I can make an exception for TMC2130 to use the nop as before, much nicer.
_DO_STEP* is not used by the stepping code, this is essentially just for XYZCAL. But removing the the call is still nice ;)
There was a problem hiding this comment.
Do we really need that nop? It’s not like it makes any real difference
Preventing Compiler optimizations that could eliminate the actual change, perhaps?
There was a problem hiding this comment.
(on an external 2130)
There was a problem hiding this comment.
Ok, I removed all delays entirely. @leptun could you try a build without DEDGE and see if everything is still a-ok?
There are still a couple of "nops" in sm4.c for XYZCAL.
We could go the extra mile and remove those as well.
There was a problem hiding this comment.
I wanted do say that there should be no NOP when DEDGE is used. With regular stepping it is unfortunately needed as you've said. I thought you meant to keep the delay with DEDGE, which didn't make sense to me. There should only be delays between the HIGH->LOW transition of the nonDEDGE mode
There was a problem hiding this comment.
I'll refine this further in a few minutes
There was a problem hiding this comment.
Ok, the regular variant now restores the nop instruction, but still allows to use delayMicroseconds when not using TMC2130. DEDGE removes all details (as it was the case before).
Introduce new macros TMC2130_MINIMUM_DELAY/STEPPER_MINIMUM_DELAY for blocking pauses. If MINIMUM_PULSE has defined to be zero, avoid the delay call entirely.
With the new STEPPER_MINIMUM_DELAY being automatically removed for TMC2130 we no longer need to add specialized #ifdefs for DEDGE in babystep.
This ensures delays are always properly elided without having to check for DEDGE all over the place.
When TMC2130_MINIMUM_PULSE is 0 a minimum delay is implied. In this case, use a single "nop" instruction.
Use the same technique used in fastio to toggle pins efficiently in sm4 when DEDGE is used.
|
Hopefully I've ironed out all concerns. The regular (non-DEDGE) variant now uses a single nop instruction as it used to, but still allows variable delays if needed. Tweaked sm4.cpp to use the same efficient toggle mechanism as used by stepper.cpp. I've tested both variants merged on the current master and both work as expected, including XYZcal. |
The TMC2130 support Double Edge stepping aka
DEDGEwhere both the rising and lowering edge of the stepping pin count as a step instead of requiring a full cycle.DEDGE is advantageous as it removes one extra toggle for each stepper pulsed (plus a delay, in some circumstances), saving a few µs here and there.
We introduce 2 macros to step: STEP_NC_HI and STEP_NC_LO.
In normal mode, both perform as their name implies. In DEDGE mode instead STEP_NC_HI inverts the line, while STEP_NC_LO becomes a no-op. Both modes are still fully supported. The macros are specialized for each axis, and still respect INVERT_?_AXIS as defined in the variant file.
We also introduce a few constant definitions the delays required by the TMC2130, and use them when appropriate instead of hard-coding delays:
Existing code that was manipulating the pins directly has been changed to use either the macros or the helper functions defined in tmc2130.h. We also fix the behavior of TMC2130_INTPOL_* which was previously ignored in the variant files.
PFW-1192