Introduced delays for enable and direction pins (#720)

* Added some timing changes to steppers. Basically this should introduce a small (configurable) delay for enable and direction pins.

* Updates after testing

* Fixed small casting bug

* Fixed subtractions as per @Mitch's comments

* Added STEP_PULSE_DELAY handling for compatibility with RMT_STEPS. Functionality should be unchanged.

* Updates to help merge

Co-authored-by: Stefan de Bruijn <stefan@nubilosoft.com>
Co-authored-by: bdring <barton.dring@gmail.com>

.gitignore

@@ -20,3 +20,4 @@ __vm/
 *.vcxproj.filters
 *.suo
 Grbl_Esp32.ino.cpp
+/packages

Grbl_Esp32/src/Config.h

@@ -446,17 +446,7 @@ const int DWELL_TIME_STEP = 50;  // Integer (1-255) (milliseconds)
 // While this is experimental, it is intended to be the future default method after testing
 //#define USE_RMT_STEPS
 
-// Creates a delay between the direction pin setting and corresponding step pulse by creating
+// STEP_PULSE_DELAY is now a setting...$Stepper/Direction/Delay
-// another interrupt (Timer2 compare) to manage it. The main Grbl interrupt (Timer1 compare)
-// sets the direction pins, and does not immediately set the stepper pins, as it would in
-// normal operation. The Timer2 compare fires next to set the stepper pins after the step
-// pulse delay time, and Timer2 overflow will complete the step pulse, except now delayed
-// by the step pulse time plus the step pulse delay. (Thanks langwadt for the idea!)
-// NOTE: Uncomment to enable. The recommended delay must be > 3us, and, when added with the
-// user-supplied step pulse time, the total time must not exceed 127us. Reported successful
-// values for certain setups have ranged from 5 to 20us.
-// must use #define USE_RMT_STEPS for this to work
-//#define STEP_PULSE_DELAY 10 // Step pulse delay in microseconds. Default disabled.
 
 // The number of linear motions in the planner buffer to be planned at any give time. The vast
 // majority of RAM that Grbl uses is based on this buffer size. Only increase if there is extra

Grbl_Esp32/src/Defaults.h

@@ -46,6 +46,10 @@
 #    define DEFAULT_STEP_ENABLE_DELAY 0
 #endif
 
+#ifndef STEP_PULSE_DELAY
+#    define STEP_PULSE_DELAY 0
+#endif
+
 #ifndef DEFAULT_STEPPER_IDLE_LOCK_TIME
 #    define DEFAULT_STEPPER_IDLE_LOCK_TIME 250  // $1 msec (0-254, 255 keeps steppers enabled)
 #endif
@@ -674,4 +678,4 @@
 
 #ifndef DEFAULT_USER_MACRO3
 #    define DEFAULT_USER_MACRO3 ""
-#endif
+#endif

Grbl_Esp32/src/Grbl.h

@@ -22,7 +22,7 @@
 
 // Grbl versioning system
 const char* const GRBL_VERSION       = "1.3a";
-const char* const GRBL_VERSION_BUILD = "20210226";
+const char* const GRBL_VERSION_BUILD = "20210228";
 
 //#include <sdkconfig.h>
 #include <Arduino.h>

Grbl_Esp32/src/Motors/Motors.cpp

@@ -478,7 +478,7 @@ uint8_t motors_set_homing_mode(uint8_t homing_mask, bool isHoming) {
     return can_home;
 }
 
-void motors_step(uint8_t step_mask, uint8_t dir_mask) {
+bool motors_direction(uint8_t dir_mask) {
     auto n_axis = number_axis->get();
     //grbl_msg_sendf(CLIENT_SERIAL, MsgLevel::Info, "motors_set_direction_pins:0x%02X", onMask);
 
@@ -493,7 +493,17 @@ void motors_step(uint8_t step_mask, uint8_t dir_mask) {
             myMotor[axis][0]->set_direction(thisDir);
             myMotor[axis][1]->set_direction(thisDir);
         }
+
+        return true;
+    } else {
+        return false;
     }
+}
+
+void motors_step(uint8_t step_mask) {
+    auto n_axis = number_axis->get();
+    //grbl_msg_sendf(CLIENT_SERIAL, MsgLevel::Info, "motors_set_direction_pins:0x%02X", onMask);
+
     // Turn on step pulses for motors that are supposed to step now
     for (uint8_t axis = X_AXIS; axis < n_axis; axis++) {
         if (bitnum_istrue(step_mask, axis)) {
@@ -513,4 +523,4 @@ void motors_unstep() {
         myMotor[axis][0]->unstep();
         myMotor[axis][1]->unstep();
     }
-}
+}

Grbl_Esp32/src/Motors/Motors.h

@@ -40,7 +40,8 @@ void    motors_read_settings();
 // The return value is a bitmask of axes that can home
 uint8_t motors_set_homing_mode(uint8_t homing_mask, bool isHoming);
 void    motors_set_disable(bool disable, uint8_t mask = B11111111);  // default is all axes
-void    motors_step(uint8_t step_mask, uint8_t dir_mask);
+bool    motors_direction(uint8_t dir_mask);
+void    motors_step(uint8_t step_mask);
 void    motors_unstep();
 
 void servoUpdateTask(void* pvParameters);

Grbl_Esp32/src/Motors/StandardStepper.cpp

@@ -40,17 +40,14 @@ namespace Motors {
     }
 
     StandardStepper::StandardStepper(uint8_t axis_index, uint8_t step_pin, uint8_t dir_pin, uint8_t disable_pin) :
-        Motor(axis_index), _step_pin(step_pin), _dir_pin(dir_pin), _disable_pin(disable_pin) {
+        Motor(axis_index), _step_pin(step_pin), _dir_pin(dir_pin), _disable_pin(disable_pin) {}
-    }
 
     void StandardStepper::init() {
         read_settings();
         config_message();
     }
 
-    void StandardStepper::read_settings() { 
+    void StandardStepper::read_settings() { init_step_dir_pins(); }
-        init_step_dir_pins(); 
-    }
 
     void StandardStepper::init_step_dir_pins() {
         _invert_step_pin = bitnum_istrue(step_invert_mask->get(), _axis_index);
@@ -68,11 +65,8 @@ namespace Motors {
         rmtConfig.tx_config.carrier_level        = RMT_CARRIER_LEVEL_LOW;
         rmtConfig.tx_config.idle_output_en       = true;
 
-#    ifdef STEP_PULSE_DELAY
+        auto stepPulseDelay  = direction_delay_microseconds->get();
-        rmtItem[0].duration0 = STEP_PULSE_DELAY * 4;
+        rmtItem[0].duration0 = stepPulseDelay < 1 ? 1 : stepPulseDelay * 4;
-#    else
-        rmtItem[0].duration0 = 1;
-#    endif
 
         rmtItem[0].duration1 = 4 * pulse_microseconds->get();
         rmtItem[1].duration0 = 0;
@@ -126,5 +120,8 @@ namespace Motors {
 
     void StandardStepper::set_direction(bool dir) { digitalWrite(_dir_pin, dir ^ _invert_dir_pin); }
 
-    void StandardStepper::set_disable(bool disable) { digitalWrite(_disable_pin, disable); }
+    void StandardStepper::set_disable(bool disable) {
+        disable ^= step_enable_invert->get();
+        digitalWrite(_disable_pin, disable);
+    }
 }

Grbl_Esp32/src/Protocol.cpp

@@ -193,7 +193,7 @@ void protocol_main_loop() {
             return;  // Bail to main() program loop to reset system.
         }
         // check to see if we should disable the stepper drivers ... esp32 work around for disable in main loop.
-        if (stepper_idle) {
+        if (stepper_idle && stepper_idle_lock_time->get() != 0xff) {
             if (esp_timer_get_time() > stepper_idle_counter) {
                 motors_set_disable(true);
             }

Grbl_Esp32/src/SettingsDefinitions.cpp

@@ -10,6 +10,7 @@ StringSetting* build_info;
 
 IntSetting* pulse_microseconds;
 IntSetting* stepper_idle_lock_time;
+IntSetting* direction_delay_microseconds;
 IntSetting* enable_delay_microseconds;
 
 AxisMaskSetting* step_invert_mask;
@@ -402,6 +403,9 @@ void make_settings() {
     pulse_microseconds     = new IntSetting(GRBL, WG, "0", "Stepper/Pulse", DEFAULT_STEP_PULSE_MICROSECONDS, 3, 1000);
     enable_delay_microseconds = new IntSetting(EXTENDED, WG, NULL, "Stepper/Enable/Delay", DEFAULT_STEP_ENABLE_DELAY, 0, 1000);  // microseconds
 
+    direction_delay_microseconds = new IntSetting(EXTENDED, WG, NULL, "Stepper/Direction/Delay", STEP_PULSE_DELAY, 0, 1000);
+    enable_delay_microseconds = new IntSetting(EXTENDED, WG, NULL, "Stepper/Enable/Delay", DEFAULT_STEP_ENABLE_DELAY, 0, 1000);  // microseconds
+
     stallguard_debug_mask = new AxisMaskSetting(EXTENDED, WG, NULL, "Report/StallGuard", 0, postMotorSetting);
 
     homing_cycle[5] = new AxisMaskSetting(EXTENDED, WG, NULL, "Homing/Cycle5", DEFAULT_HOMING_CYCLE_5);

Grbl_Esp32/src/SettingsDefinitions.h

@@ -18,6 +18,7 @@ extern StringSetting* build_info;
 
 extern IntSetting* pulse_microseconds;
 extern IntSetting* stepper_idle_lock_time;
+extern IntSetting* direction_delay_microseconds;
 extern IntSetting* enable_delay_microseconds;
 
 extern AxisMaskSetting* step_invert_mask;

Grbl_Esp32/src/Spindles/Spindle.cpp

@@ -104,4 +104,4 @@ namespace Spindles {
     void Spindle::deinit() { stop(); }
 }
 
-Spindles::Spindle* spindle;
+ Spindles::Spindle* spindle;

Grbl_Esp32/src/Stepper.cpp

@@ -58,10 +58,7 @@ typedef struct {
 
     uint32_t counter[MAX_N_AXIS];  // Counter variables for the bresenham line tracer
 
-#ifdef STEP_PULSE_DELAY
+    uint8_t  step_bits;        // Stores out_bits output to complete the step pulse delay
-    uint8_t step_bits;  // Stores out_bits output to complete the step pulse delay
-#endif
-
     uint8_t  execute_step;     // Flags step execution for each interrupt.
     uint8_t  step_pulse_time;  // Step pulse reset time after step rise
     uint8_t  step_outbits;     // The next stepping-bits to be output
@@ -182,7 +179,7 @@ stepper_id_t current_stepper = DEFAULT_STEPPER;
 
 	 The complete step timing should look this...
 		Direction pin is set
-		An optional (via STEP_PULSE_DELAY in config.h) is put after this
+		An optional delay (direction_delay_microseconds) is put after this
 		The step pin is started
 		A pulse length is determine (via option $0 ... pulse_microseconds)
 		The pulse is ended
@@ -196,8 +193,8 @@ static void stepper_pulse_func();
 // TODO: Replace direct updating of the int32 position counters in the ISR somehow. Perhaps use smaller
 // int8 variables and update position counters only when a segment completes. This can get complicated
 // with probing and homing cycles that require true real-time positions.
-void IRAM_ATTR onStepperDriverTimer(
+void IRAM_ATTR onStepperDriverTimer(void* para) {
-    void* para) {  // ISR It is time to take a step =======================================================================================
+    // ISR It is time to take a step =======================================================================================
     //const int timer_idx = (int)para;  // get the timer index
     TIMERG0.int_clr_timers.t0 = 1;
     if (busy) {
@@ -221,14 +218,42 @@ void IRAM_ATTR onStepperDriverTimer(
 static void stepper_pulse_func() {
     auto n_axis = number_axis->get();
 
-    motors_step(st.step_outbits, st.dir_outbits);
+    if (motors_direction(st.dir_outbits)) {
+        auto wait_direction = direction_delay_microseconds->get();
+        if (wait_direction > 0) {
+            // Stepper drivers need some time between changing direction and doing a pulse.
+            switch (current_stepper) {
+                case ST_I2S_STREAM:
+                    i2s_out_push_sample(wait_direction);
+                    break;
+                case ST_I2S_STATIC:
+                case ST_TIMED: {
+                    // wait for step pulse time to complete...some time expired during code above
+                    //
+                    // If we are using GPIO stepping as opposed to RMT, record the
+                    // time that we turned on the direction pins so we can delay a bit.
+                    // If we are using RMT, we can't delay here.
+                    auto direction_pulse_start_time = esp_timer_get_time() + wait_direction;
+                    while ((esp_timer_get_time() - direction_pulse_start_time) < 0) {
+                        NOP();  // spin here until time to turn off step
+                    }
+                    break;
+                }
+                case ST_RMT:
+                    break;
+            }
+        }
+    }
 
     // If we are using GPIO stepping as opposed to RMT, record the
     // time that we turned on the step pins so we can turn them off
     // at the end of this routine without incurring another interrupt.
     // This is unnecessary with RMT and I2S stepping since both of
     // those methods time the turn off automatically.
+    //
+    // NOTE: We could use direction_pulse_start_time + wait_direction, but let's play it safe
     uint64_t step_pulse_start_time = esp_timer_get_time();
+    motors_step(st.step_outbits);
 
     // If there is no step segment, attempt to pop one from the stepper buffer
     if (st.exec_segment == NULL) {
@@ -359,13 +384,20 @@ void st_wake_up() {
     // Enable stepper drivers.
     motors_set_disable(false);
     stepper_idle = false;
+
     // Initialize step pulse timing from settings. Here to ensure updating after re-writing.
-#ifdef STEP_PULSE_DELAY
+#ifdef USE_RMT_STEPS
     // Step pulse delay handling is not require with ESP32...the RMT function does it.
+    if (direction_delay_microseconds->get() < 1)
+    {
+        // Set step pulse time. Ad hoc computation from oscilloscope. Uses two's complement.
+        st.step_pulse_time = -(((pulse_microseconds->get() - 2) * ticksPerMicrosecond) >> 3);
+    }
 #else  // Normal operation
     // Set step pulse time. Ad hoc computation from oscilloscope. Uses two's complement.
     st.step_pulse_time = -(((pulse_microseconds->get() - 2) * ticksPerMicrosecond) >> 3);
 #endif
+
     // Enable Stepper Driver Interrupt
     Stepper_Timer_Start();
 }