diff --git a/dda.c b/dda.c index f667ade..28e8851 100644 --- a/dda.c +++ b/dda.c @@ -482,8 +482,6 @@ void dda_start(DDA *dda) { Next, we work out how long until our next step using the selected acceleration algorithm and set the timer. Then we decide if this was the last step for this move, and if so mark this dda as dead so next timer interrupt we can start a new one. Finally we de-assert any asserted step pins. - - \todo take into account the time that interrupt takes to run */ void dda_step(DDA *dda) { uint8_t endstop_stop; ///< Stop due to endstop trigger @@ -598,10 +596,11 @@ void dda_step(DDA *dda) { } #if STEP_INTERRUPT_INTERRUPTIBLE - // since we have sent steps to all the motors that will be stepping and the rest of this function isn't so time critical, - // this interrupt can now be interruptible - // however we must ensure that we don't step again while computing the below, so disable *this* interrupt but allow others to fire -// disableTimerInterrupt(); + // Since we have sent steps to all the motors that will be stepping + // and the rest of this function isn't so time critical, this interrupt + // can now be interruptible by other interrupts. + // The step interrupt is disabled before entering dda_step() to ensure + // that we don't step again while computing the below. sei(); #endif @@ -695,8 +694,6 @@ void dda_step(DDA *dda) { else steptimeout = 0; - cli(); - #ifdef ACCELERATION_RAMPING // we don't hit maximum speed exactly with acceleration calculation, so limit it here // the nice thing about _not_ setting dda->c to dda->c_min is, the move stops at the exact same c as it started, so we have to calculate c only once for the time being diff --git a/dda_queue.c b/dda_queue.c index 0d0113f..854ee5e 100644 --- a/dda_queue.c +++ b/dda_queue.c @@ -74,13 +74,10 @@ void queue_step() { current_movebuffer->live = current_movebuffer->waitfor_temp = 0; serial_writestr_P(PSTR("Temp achieved\n")); } - - #if STEP_INTERRUPT_INTERRUPTIBLE - sei(); - #endif } else { - // NOTE: dda_step makes this interrupt interruptible after steps have been sent but before new speed is calculated. + // NOTE: dda_step makes this interrupt interruptible for some time, + // see STEP_INTERRUPT_INTERRUPTIBLE. dda_step(current_movebuffer); } } @@ -135,18 +132,9 @@ void enqueue_home(TARGET *t, uint8_t endstop_check, uint8_t endstop_stop_cond) { SREG = save_reg; if (isdead) { - timer1_compa_deferred_enable = 0; next_move(); - if (timer1_compa_deferred_enable) { - uint8_t save_reg = SREG; - cli(); - CLI_SEI_BUG_MEMORY_BARRIER(); - - TIMSK1 |= MASK(OCIE1A); - - MEMORY_BARRIER(); - SREG = save_reg; - } + // Compensate for the cli() in setTimer(). + sei(); } } diff --git a/timer.c b/timer.c index 2a555d7..7c9b991 100644 --- a/timer.c +++ b/timer.c @@ -41,8 +41,6 @@ volatile uint8_t clock_flag_10ms = 0; volatile uint8_t clock_flag_250ms = 0; volatile uint8_t clock_flag_1s = 0; -volatile uint8_t timer1_compa_deferred_enable = 0; - /// comparator B is the system clock, happens every TICK_TIME ISR(TIMER1_COMPB_vect) { // set output compare register to the next clock tick @@ -83,7 +81,6 @@ ISR(TIMER1_COMPA_vect) { // disable this interrupt. if we set a new timeout, it will be re-enabled when appropriate TIMSK1 &= ~MASK(OCIE1A); - timer1_compa_deferred_enable = 0; // stepper tick queue_step(); @@ -92,19 +89,7 @@ ISR(TIMER1_COMPA_vect) { #ifdef DEBUG_LED_PIN WRITE(DEBUG_LED_PIN, 0); #endif - - // Enable the timer1_compa interrupt, if needed, - // but only do it after disabling global interrupts. - // This will cause push any possible timer1a interrupt - // to the far side of the return, protecting the - // stack from recursively clobbering memory. - - cli(); - CLI_SEI_BUG_MEMORY_BARRIER(); - - if (timer1_compa_deferred_enable) { - TIMSK1 |= MASK(OCIE1A); - } + return; } @@ -136,17 +121,20 @@ void timer_init() } #ifdef HOST -/// specify how long until the step timer should fire +/*! Specify how long until the step timer should fire. + \param delay in CPU ticks + + This enables the step interrupt, but also disables interrupts globally. + So, if you use it from inside the step interrupt, make sure to do so + as late as possible. If you use it from outside the step interrupt, + do a sei() after it to make the interrupt actually fire. +*/ void setTimer(uint32_t delay) { // save interrupt flag uint8_t sreg = SREG; uint16_t step_start = 0; - - // disable interrupts - cli(); - CLI_SEI_BUG_MEMORY_BARRIER(); - + // re-enable clock interrupt in case we're recovering from emergency stop TIMSK1 |= MASK(OCIE1B); @@ -184,14 +172,20 @@ void setTimer(uint32_t delay) OCR1A = step_start; } - // Defer the enabling of the timer1_CompA interrupts. - - timer1_compa_deferred_enable = 1; + // Enable this interrupt, but only do it after disabling + // global interrupts. This will cause push any possible + // timer1a interrupt to the far side of the return, protecting the + // stack from recursively clobbering memory. + cli(); + CLI_SEI_BUG_MEMORY_BARRIER(); + TIMSK1 |= MASK(OCIE1A); + } else { + // TODO: as the interrupt is designed to fire only once, + // doing a setTimer(0) should be obsolete. // flag: move has ended next_step_time = 0; TIMSK1 &= ~MASK(OCIE1A); - timer1_compa_deferred_enable = 0; } // restore interrupt flag diff --git a/timer.h b/timer.h index 702a20a..9edc71b 100644 --- a/timer.h +++ b/timer.h @@ -15,8 +15,6 @@ extern volatile uint8_t clock_flag_10ms; extern volatile uint8_t clock_flag_250ms; extern volatile uint8_t clock_flag_1s; -extern volatile uint8_t timer1_compa_deferred_enable; - // If the specific bit is set, execute the following block exactly once // and then clear the flag. #define ifclock(F) for (;F;F=0 )