Fix bug with premature step end. Refactored _delay_ms() and square() for better portability.

- Fixed a premature step end bug dating back to Simen's 0.7b edge
version is fixed, from which this code is forked from. Caused by Timer2
constantly overflowing calling the Step Reset Interrupt every 128usec.
Now Timer2 is always disabled after a step end and should free up some
cycles for the main program. Could be more than one way to fix this
problem. I'm open to suggestions.

- _delay_ms() refactored to accept only constants to comply with
current compilers. square() removed since not available with some
compilers.

config.h

@@ -108,7 +108,7 @@
 // this delay will increase the maximum dwell time linearly, but also reduces the responsiveness of 
 // run-time command executions, like status reports, since these are performed between each dwell 
 // time step. Also, keep in mind that the Arduino delay timer is not very accurate for long delays.
-#define DWELL_TIME_STEP 50 // Integer (milliseconds)
+#define DWELL_TIME_STEP 50 // Integer (1-255) (milliseconds)
 
 
 // -----------------------------------------------

main.c

@@ -39,9 +39,9 @@ system_t sys;
 int main(void)
 {
   // Initialize system
-  sei(); // Enable interrupts
   serial_init(BAUD_RATE); // Setup serial baud rate and interrupts
   st_init(); // Setup stepper pins and interrupt timers
+  sei(); // Enable interrupts
 
   memset(&sys, 0, sizeof(sys));  // Clear all system variables
   sys.abort = true;   // Set abort to complete initialization

motion_control.c

@@ -182,7 +182,7 @@ void mc_dwell(double seconds)
 {
    uint16_t i = floor(1000/DWELL_TIME_STEP*seconds);
    plan_synchronize();
-   _delay_ms(floor(1000*seconds-i*DWELL_TIME_STEP)); // Delay millisecond remainder
+   delay_ms(floor(1000*seconds-i*DWELL_TIME_STEP)); // Delay millisecond remainder
    while (i > 0) {
      // NOTE: Check and execute runtime commands during dwell every <= DWELL_TIME_STEP milliseconds.
      protocol_execute_runtime();

nuts_bolts.c

@@ -21,6 +21,7 @@
 #include "nuts_bolts.h"
 #include <stdint.h>
 #include <stdlib.h>
+#include <util/delay.h>
 
 int read_double(char *line, uint8_t *char_counter, double *double_ptr)                  
 {
@@ -36,3 +37,10 @@ int read_double(char *line, uint8_t *char_counter, double *double_ptr)
   return(true);
 }
 
+
+// Delays variable defined milliseconds. Compiler compatibility fix for _delay_ms(),
+// which only accepts constants in future compiler releases.
+void delay_ms(uint16_t ms) 
+{
+  while ( ms-- ) { _delay_ms(1); }
+}

nuts_bolts.h

@@ -83,4 +83,7 @@ extern system_t sys;
 // a pointer to the result variable. Returns true when it succeeds
 int read_double(char *line, uint8_t *char_counter, double *double_ptr);
 
+// Delays variable-defined milliseconds. Compiler compatibility fix for _delay_ms().
+void delay_ms(uint16_t ms);
+
 #endif

planner.c

@@ -376,8 +376,8 @@ void plan_buffer_line(double x, double y, double z, double feed_rate, uint8_t in
   delta_mm[X_AXIS] = (target[X_AXIS]-pl.position[X_AXIS])/settings.steps_per_mm[X_AXIS];
   delta_mm[Y_AXIS] = (target[Y_AXIS]-pl.position[Y_AXIS])/settings.steps_per_mm[Y_AXIS];
   delta_mm[Z_AXIS] = (target[Z_AXIS]-pl.position[Z_AXIS])/settings.steps_per_mm[Z_AXIS];
-  block->millimeters = sqrt(square(delta_mm[X_AXIS]) + square(delta_mm[Y_AXIS]) + 
+  block->millimeters = sqrt(delta_mm[X_AXIS]*delta_mm[X_AXIS] + delta_mm[Y_AXIS]*delta_mm[Y_AXIS] + 
-                            square(delta_mm[Z_AXIS]));
+                            delta_mm[Z_AXIS]*delta_mm[Z_AXIS]);
   double inverse_millimeters = 1.0/block->millimeters;  // Inverse millimeters to remove multiple divides	
   
   // Calculate speed in mm/minute for each axis. No divide by zero due to previous checks.
@@ -476,9 +476,10 @@ void plan_set_current_position(double x, double y, double z)
 {
   // To correlate status reporting work position correctly, the planner must force the steppers to
   // empty the block buffer and synchronize with the planner, as the real-time machine position and 
-  // the planner position at the end of the buffer are different. This will only be called with a 
+  // the planner position at the end of the buffer can be and are usually different. This function is
-  // G92 is executed, which typically is used only at the beginning of a g-code program.
+  // only called with a G92, which typically is used only at the beginning of a g-code program or 
-  // TODO: Find a robust way to avoid a planner synchronize, but may require a bit of ingenuity.
+  // between different operations.
+  // TODO: Find a robust way to avoid a planner synchronize, but this may require a bit of ingenuity.
   plan_synchronize();
   
   // Update the system coordinate offsets from machine zero

script/stream.py

@@ -19,6 +19,7 @@ import re
 import time
 import sys
 import argparse
+# import threading
 
 RX_BUFFER_SIZE = 128
 

stepper.c

@@ -130,29 +130,24 @@ static uint8_t iterate_trapezoid_cycle_counter()
 // config_step_timer. It pops blocks from the block_buffer and executes them by pulsing the stepper pins appropriately. 
 // It is supported by The Stepper Port Reset Interrupt which it uses to reset the stepper port after each pulse. 
 // The bresenham line tracer algorithm controls all three stepper outputs simultaneously with these two interrupts.
-// NOTE: ISR_NOBLOCK allows SIG_OVERFLOW2 to trigger on-time regardless of time in this handler.
+ISR(TIMER1_COMPA_vect)
-
-// TODO: ISR_NOBLOCK is the same as the old SIGNAL with sei() method, but is optimizable by the compiler. On
-// an oscilloscope there is a weird hitch in the step pulse during high load operation. Very infrequent, but
-// when this does happen most of the time the pulse falling edge is randomly delayed by 20%-50% of the total 
-// intended pulse time, but sometimes it pulses less than 3usec. The former likely caused by the serial 
-// interrupt doing its thing, not that big of a deal, but the latter cause is unknown and worrisome. Need
-// to track down what is causing this problem. Functionally, this shouldn't cause any noticeable issues
-// as long as stepper drivers have a pulse minimum of 1usec or so (Pololu and any Allegro IC are ok).
-// ** This seems to be an inherent issue that dates all the way back to Simen's v0.6b or earlier. **
-
-ISR(TIMER1_COMPA_vect,ISR_NOBLOCK)
 {        
   if (busy) { return; } // The busy-flag is used to avoid reentering this interrupt
-  busy = true;
   
   // Set the direction pins a couple of nanoseconds before we step the steppers
   STEPPING_PORT = (STEPPING_PORT & ~DIRECTION_MASK) | (out_bits & DIRECTION_MASK);
   // Then pulse the stepping pins
   STEPPING_PORT = (STEPPING_PORT & ~STEP_MASK) | out_bits;
-  // Reset step pulse reset timer so that The Stepper Port Reset Interrupt can reset the signal after
+  // Enable step pulse reset timer so that The Stepper Port Reset Interrupt can reset the signal after
-  // exactly settings.pulse_microseconds microseconds.
+  // exactly settings.pulse_microseconds microseconds, independent of the main Timer1 prescaler.
-  TCNT2 = step_pulse_time;
+  TCNT2 = step_pulse_time; // Reload timer counter
+  TCCR2B = (1<<CS21); // Begin timer2. Full speed, 1/8 prescaler
+
+  busy = true;
+  // Re-enable interrupts to allow ISR_TIMER2_OVERFLOW to trigger on-time and allow serial communications
+  // regardless of time in this handler. The following code prepares the stepper driver for the next
+  // step interrupt compare and will always finish before returning to the main program.
+  sei();
   
   // If there is no current block, attempt to pop one from the buffer
   if (current_block == NULL) {
@@ -296,12 +291,15 @@ ISR(TIMER1_COMPA_vect,ISR_NOBLOCK)
   busy = false;
 }
 
-// This interrupt is set up by SIG_OUTPUT_COMPARE1A when it sets the motor port bits. It resets
+// This interrupt is set up by ISR_TIMER1_COMPAREA when it sets the motor port bits. It resets
 // the motor port after a short period (settings.pulse_microseconds) completing one step cycle.
+// TODO: It is possible for the serial interrupts to delay this interrupt by a few microseconds, if
+// they execute right before this interrupt. Not a big deal, but could use some TLC at some point.
 ISR(TIMER2_OVF_vect)
 {
   // Reset stepping pins (leave the direction pins)
   STEPPING_PORT = (STEPPING_PORT & ~STEP_MASK) | (settings.invert_mask & STEP_MASK); 
+  TCCR2B = 0; // Disable Timer2 to prevent re-entering this interrupt when it's not needed. 
 }
 
 // Reset and clear stepper subsystem variables
@@ -333,7 +331,7 @@ void st_init()
 	
   // Configure Timer 2
   TCCR2A = 0; // Normal operation
-  TCCR2B = (1<<CS21); // Full speed, 1/8 prescaler
+  TCCR2B = 0; // Disable timer until needed.
   TIMSK2 |= (1<<TOIE2);      
 
   // Start in the idle state