Browse Source

Enforce full-loop handling of aborted commands

After calling planner_abort_hard() no motion command can be
scheduled until we return to the main loop since the call can
potentially be scheduled inside a nested process_command call.

Despite previous fixes, bugs keep creeping in due to nested calls not
being obvious to detect at all.

Stop allowing motion _completely_ for the entire processing loop by
default. That is, instead of aborting the current plan_buffer_line call,
abort the entire command until we can actually schedule motion safely
again.

This benefits handling of pretty much all g/m-codes, since this flag
(now "planner_aborted" for clarity) becomes a general "command aborted"
call.

This also now ensures that the flag prevents _any_ new block (including
blocks partially planned while servicing an interrupt) are scheduled
after planner_abort_hard is called.

There are only two exceptions where it's safe to resume in this context:

- Within uvlo_, where we never return to the main processing loop
- When we're intentionally scheduling a new process_command loop for a
  MK3 filament recheck (which is *bad*)

Handle those two cases as exceptions.
Yuri D'Elia 2 years ago
parent
commit
5965572e88
4 changed files with 31 additions and 43 deletions
  1. 11 13
      Firmware/Marlin_main.cpp
  2. 1 1
      Firmware/motion_control.cpp
  3. 18 28
      Firmware/planner.cpp
  4. 1 1
      Firmware/planner.h

+ 11 - 13
Firmware/Marlin_main.cpp

@@ -1843,6 +1843,9 @@ void host_keepalive() {
 // Before loop(), the setup() function is called by the main() routine.
 void loop()
 {
+    // Reset a previously aborted command, we can now start processing motion again
+    planner_aborted = false;
+
     if(Stopped) {
         // Currently Stopped (possibly due to an error) and not accepting new serial commands.
         // Signal to the host that we're currently busy waiting for supervision.
@@ -2999,7 +3002,7 @@ static void gcode_G28(bool home_x_axis, bool home_y_axis, bool home_z_axis)
 static void gcode_G80()
 {
     st_synchronize();
-    if (waiting_inside_plan_buffer_line_print_aborted)
+    if (planner_aborted)
         return;
 
     mesh_bed_leveling_flag = true;
@@ -3093,7 +3096,7 @@ static void gcode_G80()
     plan_buffer_line_curposXYZE(XY_AXIS_FEEDRATE);
     // Wait until the move is finished.
     st_synchronize();
-    if (waiting_inside_plan_buffer_line_print_aborted)
+    if (planner_aborted)
     {
         custom_message_type = custom_message_type_old;
         custom_message_state = custom_message_state_old;
@@ -3168,7 +3171,7 @@ static void gcode_G80()
         //printf_P(PSTR("after clamping: [%f;%f]\n"), current_position[X_AXIS], current_position[Y_AXIS]);
         plan_buffer_line_curposXYZE(XY_AXIS_FEEDRATE);
         st_synchronize();
-        if (waiting_inside_plan_buffer_line_print_aborted)
+        if (planner_aborted)
         {
             custom_message_type = custom_message_type_old;
             custom_message_state = custom_message_state_old;
@@ -9546,7 +9549,7 @@ void mesh_plan_buffer_line(const float &x, const float &y, const float &z, const
                                  current_position[Z_AXIS] + t * dz,
                                  current_position[E_AXIS] + t * de,
                                  feed_rate, extruder, gcode_target);
-                if (waiting_inside_plan_buffer_line_print_aborted)
+                if (planner_aborted)
                     return;
             }
         }
@@ -10957,6 +10960,7 @@ void uvlo_()
 
     // Enable stepper driver interrupt to move Z axis. This should be fine as the planner and
     // command queues are empty, SD card printing is disabled, usb is inhibited.
+    planner_aborted = false;
     sei();
 
     // Retract
@@ -11089,6 +11093,7 @@ void uvlo_tiny()
 
         // Enable stepper driver interrupt to move Z axis. This should be fine as the planner and
         // command queues are empty, SD card printing is disabled, usb is inhibited.
+        planner_aborted = false;
         sei();
 
         // The axis was moved: adjust Z as done on a regular UVLO.
@@ -11568,7 +11573,7 @@ void stop_and_save_print_to_ram(float z_move, float e_move)
   st_reset_timer();
 	sei();
 	if ((z_move != 0) || (e_move != 0)) { // extruder or z move
-#if 1
+
     // Rather than calling plan_buffer_line directly, push the move into the command queue so that
     // the caller can continue processing. This is used during powerpanic to save the state as we
     // move away from the print.
@@ -11600,13 +11605,6 @@ void stop_and_save_print_to_ram(float z_move, float e_move)
     // If this call is invoked from the main Arduino loop() function, let the caller know that the command
     // in the command queue is not the original command, but a new one, so it should not be removed from the queue.
     repeatcommand_front();
-#else
-		plan_buffer_line(saved_pos[X_AXIS], saved_pos[Y_AXIS], saved_pos[Z_AXIS] + z_move, saved_pos[E_AXIS] + e_move, homing_feedrate[Z_AXIS], active_extruder);
-    st_synchronize(); //wait moving
-    memcpy(current_position, saved_pos, sizeof(saved_pos));
-    set_destination_to_current();
-#endif
-    waiting_inside_plan_buffer_line_print_aborted = true; //unroll the stack
   }
 }
 
@@ -11690,7 +11688,7 @@ void restore_print_from_ram_and_continue(float e_move)
 	lcd_setstatuspgm(MSG_WELCOME);
     saved_printing_type = PRINTING_TYPE_NONE;
 	saved_printing = false;
-    waiting_inside_plan_buffer_line_print_aborted = true; //unroll the stack
+    planner_aborted = true; // unroll the stack
 }
 
 // Cancel the state related to a currently saved print

+ 1 - 1
Firmware/motion_control.cpp

@@ -151,7 +151,7 @@ void mc_arc(float* position, float* target, float* offset, float feed_rate, floa
             // Insert the segment into the buffer
             plan_buffer_line(position[X_AXIS], position[Y_AXIS], position[Z_AXIS], position[E_AXIS], feed_rate, extruder, position);
             // Handle the situation where the planner is aborted hard.
-            if (waiting_inside_plan_buffer_line_print_aborted)
+            if (planner_aborted)
                 return;
         }
     }

+ 18 - 28
Firmware/planner.cpp

@@ -69,6 +69,9 @@
 #include "tmc2130.h"
 #endif //TMC2130
 
+#include <util/atomic.h>
+
+
 //===========================================================================
 //=============================public variables ============================
 //===========================================================================
@@ -593,17 +596,7 @@ void check_axes_activity()
 #endif
 }
 
-bool waiting_inside_plan_buffer_line_print_aborted = false;
-/*
-void planner_abort_soft()
-{
-    // Empty the queue.
-    while (blocks_queued()) plan_discard_current_block();
-    // Relay to planner wait routine, that the current line shall be canceled.
-    waiting_inside_plan_buffer_line_print_aborted = true;
-    //current_position[i]
-}
-*/
+bool planner_aborted = false;
 
 #ifdef PLANNER_DIAGNOSTICS
 static inline void planner_update_queue_min_counter()
@@ -665,6 +658,9 @@ void planner_abort_hard()
 #endif
     }
 #endif
+    // Relay to planner wait routine, that the current line shall be canceled.
+    planner_aborted = true;
+
     // Clear the planner queue, reset and re-enable the stepper timer.
     quickStop();
 
@@ -680,9 +676,6 @@ void planner_abort_hard()
 
     plan_reset_next_e_queue = false;
     plan_reset_next_e_sched = false;
-
-    // Relay to planner wait routine, that the current line shall be canceled.
-    waiting_inside_plan_buffer_line_print_aborted = true;
 }
 
 void plan_buffer_line_curposXYZE(float feed_rate) {
@@ -706,10 +699,7 @@ void plan_buffer_line(float x, float y, float z, const float &e, float feed_rate
   // Calculate the buffer head after we push this byte
   uint8_t next_buffer_head = next_block_index(block_buffer_head);
 
-  // TODO: shouldn't this be reset only in the outer marlin loop?
-  waiting_inside_plan_buffer_line_print_aborted = false;
-
-  // If the buffer is full: good! That means we are well ahead of the robot. 
+  // If the buffer is full: good! That means we are well ahead of the robot.
   // Rest here until there is room in the buffer.
   if (block_buffer_tail == next_buffer_head) {
       do {
@@ -718,18 +708,14 @@ void plan_buffer_line(float x, float y, float z, const float &e, float feed_rate
           manage_inactivity(false); 
           lcd_update(0);
       } while (block_buffer_tail == next_buffer_head);
-      if (waiting_inside_plan_buffer_line_print_aborted) {
-          // Inside the lcd_update(0) routine the print has been aborted.
-          // Cancel the print, do not plan the current line this routine is waiting on.
-#ifdef PLANNER_DIAGNOSTICS
-          planner_update_queue_min_counter();
-#endif /* PLANNER_DIAGNOSTICS */
-          return;
-      }
   }
 #ifdef PLANNER_DIAGNOSTICS
   planner_update_queue_min_counter();
 #endif /* PLANNER_DIAGNOSTICS */
+  if(planner_aborted) {
+      // avoid planning the block early if aborted
+      return;
+  }
 
   // Prepare to set up new block
   block_t *block = &block_buffer[block_buffer_head];
@@ -1334,8 +1320,12 @@ Having the real displacement of the head, we can calculate the total movement le
   if (block->step_event_count.wide <= 32767)
     block->flag |= BLOCK_FLAG_DDA_LOWRES;
 
-  // Move the buffer head. From now the block may be picked up by the stepper interrupt controller.
-  block_buffer_head = next_buffer_head;
+  ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
+      // Move the buffer head ensuring the current block hasn't been cancelled from an isr context
+      // (this is possible both during crash detection *and* uvlo, thus needing a global cli)
+      if(planner_aborted) return;
+      block_buffer_head = next_buffer_head;
+  }
 
   // Update position
   memcpy(position, target, sizeof(target)); // position[] = target[]

+ 1 - 1
Firmware/planner.h

@@ -259,7 +259,7 @@ FORCE_INLINE bool planner_queue_full() {
 // wait for the steppers to stop,
 // update planner's current position and the current_position of the front end.
 extern void planner_abort_hard();
-extern bool waiting_inside_plan_buffer_line_print_aborted;
+extern bool planner_aborted;
 
 #ifdef PREVENT_DANGEROUS_EXTRUDE
 extern int extrude_min_temp;