From 9e534c19905b47868ec10cb9037e7ed8d970857e Mon Sep 17 00:00:00 2001 From: bubnikv Date: Sat, 20 Jan 2018 14:37:22 +0100 Subject: [PATCH 1/4] Minimize risk of stepper routine interrupt blocking by reorganizing the routine to move the G-code line length from the command queue to the planner queue. --- Firmware/Marlin_main.cpp | 36 +++++++++++++++++++++++++++--------- Firmware/cmdqueue.h | 4 ++++ 2 files changed, 31 insertions(+), 9 deletions(-) diff --git a/Firmware/Marlin_main.cpp b/Firmware/Marlin_main.cpp index c2a27fbb..91cabb90 100644 --- a/Firmware/Marlin_main.cpp +++ b/Firmware/Marlin_main.cpp @@ -1409,8 +1409,12 @@ void loop() #endif //SDSUPPORT if (! cmdbuffer_front_already_processed && buflen) - { - cli(); + { + // ptr points to the start of the block currently being processed. + // The first character in the block is the block type. + char *ptr = cmdbuffer + bufindr; + if (*ptr == CMDBUFFER_CURRENT_TYPE_SDCARD) { + // To support power panic, move the lenght of the command on the SD card to a planner buffer. union { struct { char lo; @@ -1419,14 +1423,28 @@ void loop() uint16_t value; } sdlen; sdlen.value = 0; - if (CMDBUFFER_CURRENT_TYPE == CMDBUFFER_CURRENT_TYPE_SDCARD) { - sdlen.lohi.lo = cmdbuffer[bufindr + 1]; - sdlen.lohi.hi = cmdbuffer[bufindr + 2]; + { + // This block locks the interrupts globally for 3.25 us, + // which corresponds to a maximum repeat frequency of 307.69 kHz. + // This blocking is safe in the context of a 10kHz stepper driver interrupt + // or a 115200 Bd serial line receive interrupt, which will not trigger faster than 12kHz. + cli(); + // Reset the command to something, which will be ignored by the power panic routine, + // so this buffer length will not be counted twice. + *ptr ++ = CMDBUFFER_CURRENT_TYPE_TO_BE_REMOVED; + // Extract the current buffer length. + sdlen.lohi.lo = *ptr ++; + sdlen.lohi.hi = *ptr; + // and pass it to the planner queue. + planner_add_sd_length(sdlen.value); + sei(); } - cmdqueue_pop_front(); - planner_add_sd_length(sdlen.value); - sei(); - } + } + // Now it is safe to release the already processed command block. If interrupted by the power panic now, + // this block's SD card length will not be counted twice as its command type has been replaced + // by CMDBUFFER_CURRENT_TYPE_TO_BE_REMOVED. + cmdqueue_pop_front(); + } host_keepalive(); } } diff --git a/Firmware/cmdqueue.h b/Firmware/cmdqueue.h index 2c6f93b7..dad81c94 100644 --- a/Firmware/cmdqueue.h +++ b/Firmware/cmdqueue.h @@ -18,6 +18,10 @@ #define CMDBUFFER_CURRENT_TYPE_UI 3 // Command in cmdbuffer was generated by another G-code. #define CMDBUFFER_CURRENT_TYPE_CHAINED 4 +// Command has been processed and its SD card length has been possibly pushed +// to the planner queue, but not yet removed from the cmdqueue. +// This is a temporary state to reduce stepper interrupt locking time. +#define CMDBUFFER_CURRENT_TYPE_TO_BE_REMOVED 5 // How much space to reserve for the chained commands // of type CMDBUFFER_CURRENT_TYPE_CHAINED, From 17a8e2db01a2a804c5dfa1850bbe8b040a35bfeb Mon Sep 17 00:00:00 2001 From: bubnikv Date: Sat, 20 Jan 2018 14:58:30 +0100 Subject: [PATCH 2/4] Documented the interrupt blocking by a main thread by its maximum time. Added a debug output to serial line on stepper timer overflow. --- Firmware/cmdqueue.cpp | 4 ++++ Firmware/planner.cpp | 4 ++++ Firmware/stepper.cpp | 11 +++++++++++ 3 files changed, 19 insertions(+) diff --git a/Firmware/cmdqueue.cpp b/Firmware/cmdqueue.cpp index 771daafe..bdf4c3ab 100644 --- a/Firmware/cmdqueue.cpp +++ b/Firmware/cmdqueue.cpp @@ -627,6 +627,10 @@ void get_command() sd_count.value = 0; cli(); + // This block locks the interrupts globally for 3.56 us, + // which corresponds to a maximum repeat frequency of 280.70 kHz. + // This blocking is safe in the context of a 10kHz stepper driver interrupt + // or a 115200 Bd serial line receive interrupt, which will not trigger faster than 12kHz. ++ buflen; bufindw += len; sdpos_atomic = card.get_sdpos()+1; diff --git a/Firmware/planner.cpp b/Firmware/planner.cpp index 7738d5cb..0aa60866 100644 --- a/Firmware/planner.cpp +++ b/Firmware/planner.cpp @@ -264,6 +264,10 @@ void calculate_trapezoid_for_block(block_t *block, float entry_speed, float exit } CRITICAL_SECTION_START; // Fill variables used by the stepper in a critical section + // This block locks the interrupts globally for 4.38 us, + // which corresponds to a maximum repeat frequency of 228.57 kHz. + // This blocking is safe in the context of a 10kHz stepper driver interrupt + // or a 115200 Bd serial line receive interrupt, which will not trigger faster than 12kHz. if (! block->busy) { // Don't update variables if block is busy. block->accelerate_until = accelerate_steps; block->decelerate_after = accelerate_steps+plateau_steps; diff --git a/Firmware/stepper.cpp b/Firmware/stepper.cpp index 38ece7ea..e915e7ea 100644 --- a/Firmware/stepper.cpp +++ b/Firmware/stepper.cpp @@ -854,6 +854,11 @@ void isr() { if (OCR1A < TCNT1) { stepper_timer_overflow_state = true; WRITE_NC(BEEPER, HIGH); + SERIAL_PROTOCOLPGM("Stepper timer overflow "); + SERIAL_PROTOCOL(OCR1A); + SERIAL_PROTOCOLPGM("<"); + SERIAL_PROTOCOL(TCNT1); + SERIAL_PROTOCOLLN("!"); } #endif } @@ -1137,6 +1142,7 @@ void st_init() // create_speed_lookuptable.py TCCR1B = (TCCR1B & ~(0x07< Date: Sat, 20 Jan 2018 15:39:21 +0100 Subject: [PATCH 3/4] Implemented a stepper timer reset after a long blocking cli() or DISABLE_STEPPER_DRIVER_INTERRUPT(). If this is not done, the stepper interrupt would likely overflow, leading to a maximum 32ms delay before the stepper interrupt wakes up. In addition, the stepper timer overflow error would be reported by the debug builds. --- Firmware/Marlin_main.cpp | 9 ++++++++- Firmware/planner.cpp | 12 ++++++++---- Firmware/stepper.cpp | 16 +--------------- Firmware/stepper.h | 18 ++++++++++++++---- Firmware/ultralcd.cpp | 5 +++++ 5 files changed, 36 insertions(+), 24 deletions(-) diff --git a/Firmware/Marlin_main.cpp b/Firmware/Marlin_main.cpp index 91cabb90..16581b81 100644 --- a/Firmware/Marlin_main.cpp +++ b/Firmware/Marlin_main.cpp @@ -628,6 +628,8 @@ void crashdet_stop_and_save_print2() cmdqueue_reset(); //empty cmdqueue card.sdprinting = false; card.closefile(); + // Reset and re-enable the stepper timer just before the global interrupts are enabled. + st_reset_timer(); sei(); } @@ -1993,11 +1995,14 @@ void force_high_power_mode(bool start_high_power_section) { if (silent == 1) { //we are in silent mode, set to normal mode to enable crash detection - + // Wait for the planner queue to drain and for the stepper timer routine to reach an idle state. st_synchronize(); cli(); tmc2130_mode = (start_high_power_section == true) ? TMC2130_MODE_NORMAL : TMC2130_MODE_SILENT; tmc2130_init(); + // We may have missed a stepper timer interrupt due to the time spent in the tmc2130_init() routine. + // Be safe than sorry, reset the stepper timer before re-enabling interrupts. + st_reset_timer(); sei(); digipot_init(); } @@ -7918,6 +7923,8 @@ void stop_and_save_print_to_ram(float z_move, float e_move) card.sdprinting = false; // card.closefile(); saved_printing = true; + // We may have missed a stepper timer interrupt. Be safe than sorry, reset the stepper timer before re-enabling interrupts. + st_reset_timer(); sei(); if ((z_move != 0) || (e_move != 0)) { // extruder or z move #if 1 diff --git a/Firmware/planner.cpp b/Firmware/planner.cpp index 0aa60866..d7f30d23 100644 --- a/Firmware/planner.cpp +++ b/Firmware/planner.cpp @@ -565,8 +565,7 @@ extern volatile uint32_t step_events_completed; // The number of step events exe void planner_abort_hard() { // Abort the stepper routine and flush the planner queue. - // DISABLE_STEPPER_DRIVER_INTERRUPT - TIMSK1 &= ~(1<decelerate_after after which it decelerates until the trapezoid generator is reset. // The slope of acceleration is calculated with the leib ramp alghorithm. -void st_wake_up() { - // TCNT1 = 0; - ENABLE_STEPPER_DRIVER_INTERRUPT(); -} - -void step_wait(){ - for(int8_t i=0; i < 6; i++){ - } -} - - FORCE_INLINE unsigned short calc_timer(unsigned short step_rate) { unsigned short timer; if(step_rate > MAX_STEP_FREQUENCY) step_rate = MAX_STEP_FREQUENCY; @@ -1241,6 +1226,7 @@ void quickStop() DISABLE_STEPPER_DRIVER_INTERRUPT(); while (blocks_queued()) plan_discard_current_block(); current_block = NULL; + st_reset_timer(); ENABLE_STEPPER_DRIVER_INTERRUPT(); } diff --git a/Firmware/stepper.h b/Firmware/stepper.h index 9915de90..2fdec2d6 100644 --- a/Firmware/stepper.h +++ b/Firmware/stepper.h @@ -23,6 +23,9 @@ #include "planner.h" +#define ENABLE_STEPPER_DRIVER_INTERRUPT() TIMSK1 |= (1< 2 #define WRITE_E_STEP(v) { if(current_block->active_extruder == 2) { WRITE(E2_STEP_PIN, v); } else { if(current_block->active_extruder == 1) { WRITE(E1_STEP_PIN, v); } else { WRITE(E0_STEP_PIN, v); }}} #define NORM_E_DIR() { if(current_block->active_extruder == 2) { WRITE(E2_DIR_PIN, !INVERT_E2_DIR); } else { if(current_block->active_extruder == 1) { WRITE(E1_DIR_PIN, !INVERT_E1_DIR); } else { WRITE(E0_DIR_PIN, !INVERT_E0_DIR); }}} @@ -71,11 +74,18 @@ void st_get_position_xy(long &x, long &y); float st_get_position_mm(uint8_t axis); -// The stepper subsystem goes to sleep when it runs out of things to execute. Call this -// to notify the subsystem that it is time to go to work. -void st_wake_up(); +// Call this function just before re-enabling the stepper driver interrupt and the global interrupts +// to avoid a stepper timer overflow. +FORCE_INLINE void st_reset_timer() +{ + // Clear a possible pending interrupt on OCR1A overflow. + TIFR1 |= 1 << OCF1A; + // Reset the counter. + TCNT1 = 0; + // Wake up after 1ms from now. + OCR1A = 2000; +} - void checkHitEndstops(); //call from somewhere to create an serial error message with the locations the endstops where hit, in case they were triggered bool endstops_hit_on_purpose(); //avoid creation of the message, i.e. after homing and before a routine call of checkHitEndstops(); bool endstop_z_hit_on_purpose(); diff --git a/Firmware/ultralcd.cpp b/Firmware/ultralcd.cpp index 5f5091b0..e5c81ff6 100644 --- a/Firmware/ultralcd.cpp +++ b/Firmware/ultralcd.cpp @@ -3359,6 +3359,8 @@ static void lcd_silent_mode_set() { SilentModeMenu = !SilentModeMenu; eeprom_update_byte((unsigned char *)EEPROM_SILENT, SilentModeMenu); #ifdef TMC2130 + // Wait until the planner queue is drained and the stepper routine achieves + // an idle state. st_synchronize(); if (tmc2130_wait_standstill_xy(1000)) {} // MYSERIAL.print("standstill OK"); @@ -3367,6 +3369,9 @@ static void lcd_silent_mode_set() { cli(); tmc2130_mode = SilentModeMenu?TMC2130_MODE_SILENT:TMC2130_MODE_NORMAL; tmc2130_init(); + // We may have missed a stepper timer interrupt due to the time spent in tmc2130_init. + // Be safe than sorry, reset the stepper timer before re-enabling interrupts. + st_reset_timer(); sei(); #endif //TMC2130 digipot_init(); From a94e266cf1763044fffaa5241bcb5d5a0bcad24a Mon Sep 17 00:00:00 2001 From: bubnikv Date: Sat, 20 Jan 2018 16:20:51 +0100 Subject: [PATCH 4/4] Documented CPU load and frequency of the following interrupts: 9. ISR(INT7_vect) - Fan signal interrupt 26. ISR(M_USARTx_RX_vect) - USB to serial RX 37. ISR(USART1_RX_vect) - R-PI serial RX --- Firmware/MarlinSerial.cpp | 12 +++++++----- Firmware/Marlin_main.cpp | 2 ++ 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/Firmware/MarlinSerial.cpp b/Firmware/MarlinSerial.cpp index 357de7c6..0464fe41 100644 --- a/Firmware/MarlinSerial.cpp +++ b/Firmware/MarlinSerial.cpp @@ -49,11 +49,13 @@ FORCE_INLINE void store_char(unsigned char c) } -//#elif defined(SIG_USART_RECV) #if defined(M_USARTx_RX_vect) - // fixed by Mark Sproul this is on the 644/644p - //SIGNAL(SIG_USART_RECV) -SIGNAL(M_USARTx_RX_vect) +// The serial line receive interrupt routine for a baud rate 115200 +// ticks at maximum 11.76 kHz and blocks for 2.688 us at each tick. +// If the serial line is fully utilized, this corresponds to 3.16% +// loading of the CPU (the interrupt invocation overhead not taken into account). +// As the serial line is not fully utilized, the CPU load is likely around 1%. +ISR(M_USARTx_RX_vect) { // Test for a framing error. if (M_UCSRxA & (1< 18 (approximately), which is sufficient because MIN_PRINT_FAN_SPEED is higher