Fix i2s probing hang (bdring#608)

odaki · web-flow · commit d9dba2c9081f · 2020-10-09T16:32:32.000-05:00
* Fix I2S stepper hung just after the completion of motor moving * Fix recompile issue Fixed a problem with the recompile not being recompiled even if the files under the Custom folder are changed. * More comment for macOS in debug.ini * Fix the timing of calling I2S out's exclusion function and reset sequence The reset sequence did not seem to be correct, so I changed it. According to the ESP-IDF PR, the correct sequence is as follows: 1)TX module 2)DMA 3)FIFO espressif/esp-idf@c7f3352#diff-27688c6b3c29373d2a2b142b8471981c * Changed the message level for I2S swtiching from warning to debug * Add some comments
diff --git a/Grbl_Esp32/src/GCode.cpp b/Grbl_Esp32/src/GCode.cpp
@@ -1502,9 +1502,6 @@ Error gc_execute_line(char* line, uint8_t client) {
     if (gc_state.modal.motion != Motion::None) {
         if (axis_command == AxisCommand::MotionMode) {
             GCUpdatePos gc_update_pos = GCUpdatePos::Target;
-#ifdef USE_I2S_STEPS
-            stepper_id_t save_stepper = current_stepper;
-#endif
             if (gc_state.modal.motion == Motion::Linear) {
                 //mc_line(gc_block.values.xyz, pl_data);
                 mc_line_kins(gc_block.values.xyz, pl_data, gc_state.position);
@@ -1527,12 +1524,6 @@ Error gc_execute_line(char* line, uint8_t client) {
                 // upon a successful probing cycle, the machine position and the returned value should be the same.
 #ifndef ALLOW_FEED_OVERRIDE_DURING_PROBE_CYCLES
                 pl_data->motion.noFeedOverride = 1;
-#endif
-#ifdef USE_I2S_STEPS
-                save_stepper = current_stepper;  // remember the stepper
-                if (save_stepper == ST_I2S_STREAM) {
-                    stepper_switch(ST_I2S_STATIC);  // Change the stepper to reduce the delay for accurate probing.
-                }
 #endif
                 gc_update_pos = mc_probe_cycle(gc_block.values.xyz, pl_data, gc_parser_flags);
             }
@@ -1544,11 +1535,6 @@ Error gc_execute_line(char* line, uint8_t client) {
             } else if (gc_update_pos == GCUpdatePos::System) {
                 gc_sync_position();  // gc_state.position[] = sys_position
             }                        // == GCUpdatePos::None
-#ifdef USE_I2S_STEPS
-            if (save_stepper == ST_I2S_STREAM && current_stepper != ST_I2S_STREAM) {
-                stepper_switch(ST_I2S_STREAM);  // Put the stepper back on.
-            }
-#endif
         }
     }
     // [21. Program flow ]:
diff --git a/Grbl_Esp32/src/I2SOut.cpp b/Grbl_Esp32/src/I2SOut.cpp
@@ -291,21 +291,15 @@ static int IRAM_ATTR i2s_out_start() {
 
     // Attach I2S to specified GPIO pin
     i2s_out_gpio_attach(i2s_out_ws_pin, i2s_out_bck_pin, i2s_out_data_pin);
-    //start DMA link
-    i2s_out_reset_fifo_without_lock();
 
-#    ifdef USE_I2S_OUT_STREAM_IMPL
-    if (i2s_out_pulser_status == PASSTHROUGH) {
-        I2S0.conf_chan.tx_chan_mod = 3;  // 3:right+constant 4:left+constant (when tx_msb_right = 1)
-        I2S0.conf_single_data      = port_data;
-    } else {
-        I2S0.conf_chan.tx_chan_mod = 4;  // 3:right+constant 4:left+constant (when tx_msb_right = 1)
-        I2S0.conf_single_data      = 0;
-    }
-#    endif
+    // reest TX/RX module
+    I2S0.conf.tx_reset = 1;
+    I2S0.conf.tx_reset = 0;
+    I2S0.conf.rx_reset = 1;
+    I2S0.conf.rx_reset = 0;
 
 #    ifdef USE_I2S_OUT_STREAM_IMPL
-    //reset DMA
+    // reset DMA
     I2S0.lc_conf.in_rst  = 1;
     I2S0.lc_conf.in_rst  = 0;
     I2S0.lc_conf.out_rst = 1;
@@ -314,10 +308,19 @@ static int IRAM_ATTR i2s_out_start() {
     I2S0.out_link.addr = (uint32_t)o_dma.desc[0];
 #    endif
 
-    I2S0.conf.tx_reset = 1;
-    I2S0.conf.tx_reset = 0;
-    I2S0.conf.rx_reset = 1;
-    I2S0.conf.rx_reset = 0;
+    // reset FIFO
+    i2s_out_reset_fifo_without_lock();
+
+    // start DMA link
+#    ifdef USE_I2S_OUT_STREAM_IMPL
+    if (i2s_out_pulser_status == PASSTHROUGH) {
+        I2S0.conf_chan.tx_chan_mod = 3;  // 3:right+constant 4:left+constant (when tx_msb_right = 1)
+        I2S0.conf_single_data      = port_data;
+    } else {
+        I2S0.conf_chan.tx_chan_mod = 4;  // 3:right+constant 4:left+constant (when tx_msb_right = 1)
+        I2S0.conf_single_data      = 0;
+    }
+#    endif
 
     I2S0.conf1.tx_stop_en = 1;  // BCK and WCK are suppressed while FIFO is empty
 
@@ -339,13 +342,14 @@ static int IRAM_ATTR i2s_out_start() {
 }
 
 #    ifdef USE_I2S_OUT_STREAM_IMPL
-
+// Fill out one DMA buffer
+// Call with the I2S_OUT_PULSER lock acquired.
+// Note that the lock is temporarily released while calling the callback function.
 static int IRAM_ATTR i2s_fillout_dma_buffer(lldesc_t* dma_desc) {
     uint32_t* buf = (uint32_t*)dma_desc->buf;
     o_dma.rw_pos  = 0;
     // It reuses the oldest (just transferred) buffer with the name "current"
     // and fills the buffer for later DMA.
-    I2S_OUT_PULSER_ENTER_CRITICAL();  // Lock pulser status
     if (i2s_out_pulser_status == STEPPING) {
         //
         // Fillout the buffer for pulse
@@ -408,7 +412,6 @@ static int IRAM_ATTR i2s_fillout_dma_buffer(lldesc_t* dma_desc) {
         o_dma.rw_pos                         = 0;  // If someone calls i2s_out_push_sample, make sure there is no buffer overflow
         i2s_out_remain_time_until_next_pulse = 0;
     }
-    I2S_OUT_PULSER_EXIT_CRITICAL();  // Unlock pulser status
 
     return 0;
 }
@@ -591,10 +594,18 @@ i2s_out_pulser_status_t IRAM_ATTR i2s_out_get_pulser_status() {
 int IRAM_ATTR i2s_out_set_passthrough() {
     I2S_OUT_PULSER_ENTER_CRITICAL();
 #    ifdef USE_I2S_OUT_STREAM_IMPL
+    // Triggers a change of mode if it is compiled to use I2S stream.
+    // The mode is not changed directly by this function.
+    // Pull the trigger
     if (i2s_out_pulser_status == STEPPING) {
-        i2s_out_pulser_status = WAITING;  // Start stopping the pulser
+        i2s_out_pulser_status = WAITING;  // Start stopping the pulser (trigger)
     }
+    // It is a function that may be called via i2sOutTask().
+    // (i2sOutTask() -> stepper_pulse_func() -> st_go_idle() -> Stepper_Timer_Stop() -> this function)
+    // And only i2sOutTask() can change the state to PASSTHROUGH.
+    // So, to change the state, you need to return to i2sOutTask() as soon as possible.
 #    else
+    // If it is compiled to not use I2S streams, change the mode directly.
     i2s_out_pulser_status = PASSTHROUGH;
 #    endif
     I2S_OUT_PULSER_EXIT_CRITICAL();
@@ -627,6 +638,7 @@ int IRAM_ATTR i2s_out_set_stepping() {
             I2S_OUT_PULSER_EXIT_CRITICAL();
             return 0;
         }
+        // Now, DMA completed. Fallthrough.
     }
 
     // Change I2S state from PASSTHROUGH to STEPPING
@@ -772,7 +784,6 @@ int IRAM_ATTR i2s_out_init(i2s_out_init_t& init_param) {
     //
 
     // configure I2S data port interface.
-    i2s_out_reset_fifo();
 
     //reset i2s
     I2S0.conf.tx_reset = 1;
@@ -786,6 +797,8 @@ int IRAM_ATTR i2s_out_init(i2s_out_init_t& init_param) {
     I2S0.lc_conf.out_rst = 1;  // Set this bit to reset out DMA FSM. (R/W)
     I2S0.lc_conf.out_rst = 0;
 
+    i2s_out_reset_fifo_without_lock();
+
     //Enable and configure DMA
     I2S0.lc_conf.check_owner        = 0;
     I2S0.lc_conf.out_loop_test      = 0;
diff --git a/Grbl_Esp32/src/MotionControl.cpp b/Grbl_Esp32/src/MotionControl.cpp
@@ -260,6 +260,25 @@ static bool axis_is_squared(uint8_t axis_mask) {
     return mask_is_single_axis(axis_mask) && mask_has_squared_axis(axis_mask);
 }
 
+#ifdef USE_I2S_STEPS
+#    define BACKUP_STEPPER(save_stepper)                                                                                                   \
+        do {                                                                                                                               \
+            if (save_stepper == ST_I2S_STREAM) {                                                                                           \
+                stepper_switch(ST_I2S_STATIC); /* Change the stepper to reduce the delay for accurate probing. */                          \
+            }                                                                                                                              \
+        } while (0)
+
+#    define RESTORE_STEPPER(save_stepper)                                                                                                  \
+        do {                                                                                                                               \
+            if (save_stepper == ST_I2S_STREAM && current_stepper != ST_I2S_STREAM) {                                                       \
+                stepper_switch(ST_I2S_STREAM); /* Put the stepper back on. */                                                              \
+            }                                                                                                                              \
+        } while (0)
+#else
+#    define BACKUP_STEPPER(save_stepper)
+#    define RESTORE_STEPPER(save_stepper)
+#endif
+
 // Perform homing cycle to locate and set machine zero. Only '$H' executes this command.
 // NOTE: There should be no motions in the buffer and Grbl must be in an idle state before
 // executing the homing cycle. This prevents incorrect buffered plans after homing.
@@ -371,6 +390,13 @@ GCUpdatePos mc_probe_cycle(float* target, plan_line_data_t* pl_data, uint8_t par
     if (sys.abort) {
         return GCUpdatePos::None;  // Return if system reset has been issued.
     }
+
+#ifdef USE_I2S_STEPS
+    stepper_id_t save_stepper = current_stepper; /* remember the stepper */
+#endif
+    // Switch stepper mode to the I2S static (realtime mode)
+    BACKUP_STEPPER(save_stepper);
+
     // Initialize probing control variables
     uint8_t is_probe_away = bit_istrue(parser_flags, GCParserProbeIsAway);
     uint8_t is_no_error   = bit_istrue(parser_flags, GCParserProbeIsNoError);
@@ -381,7 +407,8 @@ GCUpdatePos mc_probe_cycle(float* target, plan_line_data_t* pl_data, uint8_t par
     if (probe_get_state() ^ is_probe_away) {  // Check probe pin state.
         sys_rt_exec_alarm = ExecAlarm::ProbeFailInitial;
         protocol_execute_realtime();
-        return GCUpdatePos::None;  // Nothing else to do but bail.
+        RESTORE_STEPPER(save_stepper);  // Switch the stepper mode to the previous mode
+        return GCUpdatePos::None;       // Nothing else to do but bail.
     }
     // Setup and queue probing motion. Auto cycle-start should not start the cycle.
     mc_line(target, pl_data);
@@ -392,9 +419,14 @@ GCUpdatePos mc_probe_cycle(float* target, plan_line_data_t* pl_data, uint8_t par
     do {
         protocol_execute_realtime();
         if (sys.abort) {
-            return GCUpdatePos::None;  // Check for system abort
+            RESTORE_STEPPER(save_stepper);  // Switch the stepper mode to the previous mode
+            return GCUpdatePos::None;       // Check for system abort
         }
     } while (sys.state != State::Idle);
+
+    // Switch the stepper mode to the previous mode
+    RESTORE_STEPPER(save_stepper);
+
     // Probing cycle complete!
     // Set state variables and error out, if the probe failed and cycle with error is enabled.
     if (sys_probe_state == Probe::Active) {
diff --git a/Grbl_Esp32/src/Stepper.cpp b/Grbl_Esp32/src/Stepper.cpp
@@ -457,11 +457,10 @@ void stepper_switch(stepper_id_t new_stepper) {
 #ifdef USE_I2S_STEPS
     if (current_stepper == ST_I2S_STREAM) {
         if (i2s_out_get_pulser_status() != PASSTHROUGH) {
-            // This switching function should not be called during streaming.
-            // However, if it is called, it will stop streaming.
-            grbl_msg_sendf(CLIENT_SERIAL, MsgLevel::Warning, "Stop the I2S streaming and switch to the passthrough mode.");
+            // Called during streaming. Stop streaming.
+            grbl_msg_sendf(CLIENT_SERIAL, MsgLevel::Debug, "Stop the I2S streaming and switch to the passthrough mode.");
             i2s_out_set_passthrough();
-            i2s_out_delay();
+            i2s_out_delay();  // Wait for a change in mode.
         }
     }
 #endif
diff --git a/debug.ini b/debug.ini
@@ -6,14 +6,18 @@
 upload_port = COM3
 monitor_port = COM3
 ; macOS
-;upload_port = /dev/cu.usbserial-*
-;monitor_port = /dev/cu.usbserial-*
+;upload_port = /dev/cu.SLAB_USBtoUART
+;monitor_port = /dev/cu.SLAB_USBtoUART
+; macOS ESP-Prog
+;upload_port = /dev/cu.usbserial-14200
+;monitor_port = /dev/cu.usbserial-14201
+;upload_protocol = esp-prog
 ; Linux
 ;upload_port = /dev/ttyUSB*
 ;monitor_port = /dev/ttyUSB*
 build_flags =
     ${common.build_flags}
-	-DMACHINE_FILENAME=test_drive.h 
+	-DMACHINE_FILENAME=test_drive.h
 
 [env:debug]
 ; You can switch between debugging tools using debug_tool option
diff --git a/platformio.ini b/platformio.ini
@@ -49,7 +49,7 @@ board_build.flash_mode = qio
 build_flags = ${common.build_flags}
 src_filter =
     +<*.h> +<*.s> +<*.S> +<*.cpp> +<*.c> +<*.ino> +<src/>
-    -<.git/> -<data/> -<test/> -<tests/> -<Custom/>
+    -<.git/> -<data/> -<test/> -<tests/>
 
 [env:release]
 
