Code Review

I built myself a 10"x8"x3" cnc for milling pcb boards and other things and possibly in the future 3d printing. Plan is to use GRBL on an atmega328p board that I designed. Anyways even though I plan to use GRBL I want to learn and understand what is happening behind the scenes and in the end learn c and programming avrs. And maybe one day write my own code to control the cnc.

I wrote the following code just as a mechanical and electrical test of all 3 axis.

This is the first time I've worked with timers (I did do a simple blink sketch using timer CTC and had the LED on pin 13 blinking every 1 second), direct port manipulation and pointers.

/*Command all 3 axis, 1 at a time, to move until it hits its undertravel limit switch, then change direction and
move until it hits its overtravel limit switch, and then record the amount of pulses it took to go limit to limit,
divide that value in half and go to that position then stop.*/
  
// avr-libc library includes 
#include <avr/io.h> 
#include <avr/interrupt.h> 

#define RUN_ENABLE 0x80 //Axis "ENABLE" Control Bit, PIN 8

int X_DIR = 0x04; //X axis "DIRECTION" Control Bit, PIN 2
int X_STEP = 0x08; //X Axis "STEP" Control Bit, PIN 3
int Y_DIR = 0x10; //X axis "DIRECTION" Control Bit, PIN 2
int Y_STEP = 0x20; //X Axis "STEP" Control Bit, PIN 3
int Z_DIR = 0x40; //X axis "DIRECTION" Control Bit, PIN 2
int Z_STEP = 0x80; //X Axis "STEP" Control Bit, PIN 3
int X_UNDERTRAVELPROX = PINC5;  //X Axis over travel prox input, PIN A5
int X_OVERTRAVELPROX = PINC4;  //X Axis under travel prox input, PIN A4
int Y_UNDERTRAVELPROX = PINC3;  //Y Axis over travel prox input, PIN A3
int Y_OVERTRAVELPROX = PINC2;  //Y Axis under travel prox input, PIN A2
int Z_UNDERTRAVELPROX = PINC1;  //Z Axis over travel prox input, PIN A1
int Z_OVERTRAVELPROX = PINC0;  //Z Axis under travel prox input, PIN A0

//pointers
int *STEP;
int *DIR;
int *UNDERTRAVELPROX;
int *OVERTRAVELPROX;
int *ACTUALPOS;
int *COMMANDPOS;
int *MAXSTEPS;

char activeaxis = 0;
int motionenable = 0;
int sequence = 0;
/*
seq 0 = idle
seq 1 = move active axis until under travel prox is made
seq 2 = move active axis until over travel prox is made then record
        number of pulses from under travel to over travel prox
seq 3 = move active axis to center of travel
*/

int index = 0;

int xaxismaxsteps;
int xaxiscommandpos;
int xaxisactualpos;
int yaxismaxsteps;
int yaxiscommandpos;
int yaxisactualpos;
int zaxismaxsteps;
int zaxiscommandpos;
int zaxisactualpos;

int feedrate = 9999;    //5ms pulse interval, 0.0492in/sec feedrate
    
void setup() { 
  DDRD |= 0xFC;  // sets digital pins 2,3,4,5,6,7 as output, step and direction pins
  DDRB |= 0x01;  // sets digital pin 8 as output, stepper drivers enable pin
  PORTD |= 0xD8; // sets digital pins 2,3,4,5,6,7 HIGH (internal pull-up resistor)

  cli();          // disable global interrupts  
  // initialize Timer2 
  TCCR1A = 0;     // set entire TCCR1A register to 0 
  TCCR1B = 0;     // same for TCCR1B 
  // turn on CTC mode: 
  TCCR1B |= (1 << WGM12); 
  // Set CS11 bit for 8 prescaler: 
  TCCR1B |= (1 << CS11); 
} 
  
void loop() { 
  // set compare match register equal to feedrate
  OCR1A = feedrate; 
  // enable global interrupts: 
  sei(); 
  
  if ((sequence == 0) && (index < 3)) {
    if (activeaxis == 0) { //first run
      activeaxis = 'X';
      //assign pointers
      DIR = &X_DIR;  //sets DIR to the memory address location of X_DIR
      STEP = &X_STEP;  //sets STEP to the memory address location of X_STEP
      UNDERTRAVELPROX = &X_UNDERTRAVELPROX;
      OVERTRAVELPROX = &X_OVERTRAVELPROX;
      ACTUALPOS = &xaxisactualpos;
      COMMANDPOS = &xaxiscommandpos;
      MAXSTEPS = &xaxismaxsteps;
    }
    else if (activeaxis == 'X') {
      activeaxis = 'Y';
      //assign pointers
      DIR = &Y_DIR;
      STEP = &Y_STEP;
      UNDERTRAVELPROX = &Y_UNDERTRAVELPROX;
      OVERTRAVELPROX = &Y_OVERTRAVELPROX;
      ACTUALPOS = &yaxisactualpos;
      COMMANDPOS = &yaxiscommandpos;
      MAXSTEPS = &yaxismaxsteps;
    }
    else if (activeaxis == 'Y') {
      activeaxis = 'Z';
      //assign pointers
      DIR = &Z_DIR;
      STEP = &Z_STEP;
      UNDERTRAVELPROX = &Z_UNDERTRAVELPROX;
      OVERTRAVELPROX = &Z_OVERTRAVELPROX;
      ACTUALPOS = &zaxisactualpos;
      COMMANDPOS = &zaxiscommandpos;
      MAXSTEPS = &zaxismaxsteps;
    }
    else {
      activeaxis = 0;
      //assign pointers
      DIR = NULL;
      STEP = NULL;
      UNDERTRAVELPROX = NULL;
      OVERTRAVELPROX = NULL;
      ACTUALPOS = NULL;
      COMMANDPOS = NULL;
      MAXSTEPS = NULL;
    }
    //set direction of active axis to reverse
    PORTD &= ~*DIR;
    //zero active axis position
    *ACTUALPOS = 0;    // zero actual steps
    //command active axis to maximum negative position
    *COMMANDPOS = -32768;
    sequence = 1;  //advance to the next sequence
    motionenable = 1;
    // enable timer 1 compare interrupt used for pulsing stepper driver output: 
    TIMSK1 |= (1 << OCIE1A);
  }

  if ((sequence == 1) && (*UNDERTRAVELPROX == 1)) {  //under travel prox reached
    motionenable = 0; // disable motion
    *ACTUALPOS = 0;    // zero actual steps
    *COMMANDPOS = 32768;    //command axis to maximum positive position
    // change direction of X axis
    PORTD |= *DIR; // set pin HIGH     
    sequence = 2;  //advance to the next sequence
    motionenable = 1; // enable motion
  }
  if ((sequence == 2) && (*OVERTRAVELPROX == 1)) {
    motionenable = 0; // disable motion
    *MAXSTEPS = *ACTUALPOS;
    *COMMANDPOS = (*MAXSTEPS / 2);
    // change direction of X axis
    PORTD &= ~*DIR; // set pin LOW     
    sequence = 3;  //advance to the next sequence
    motionenable = 1; // enable motion
  }
  if ((sequence == 3) && (*ACTUALPOS == *COMMANDPOS)) {
    motionenable = 0; // disable motion
    sequence = 0;  //loop back to sequence 0
    index ++;
  }
}

ISR (TIMER1_COMPA_vect) {
  int t =0;
  
  if (motionenable == 1) {
    if((PORTD & *DIR) != ~*DIR) //If going forward, increment active axis position counter
      *ACTUALPOS++;
    else //If going reverse, decrement axis position counter
      *ACTUALPOS--;
    if (*ACTUALPOS != *COMMANDPOS) { //If we are not at our commanded position     
      PORTD |= *STEP; //Toggle "Step" output with each entry
      while(t++ != 10); 
        PORTD &= ~*STEP;
    }
  } 
}

My next code challenge will be linear interpolation, making 2 axis start and stop at the same time no matter the difference in distance that each axis has to travel.

int X_DIR = 0x04; //X axis "DIRECTION" Control Bit, PIN 2
int X_STEP = 0x08; //X Axis "STEP" Control Bit, PIN 3
int Y_DIR = 0x10; //X axis "DIRECTION" Control Bit, PIN 2
int Y_STEP = 0x20; //X Axis "STEP" Control Bit, PIN 3
int Z_DIR = 0x40; //X axis "DIRECTION" Control Bit, PIN 2
int Z_STEP = 0x80; //X Axis "STEP" Control Bit, PIN 3
int X_UNDERTRAVELPROX = PINC5;  //X Axis over travel prox input, PIN A5
int X_OVERTRAVELPROX = PINC4;  //X Axis under travel prox input, PIN A4
int Y_UNDERTRAVELPROX = PINC3;  //Y Axis over travel prox input, PIN A3
int Y_OVERTRAVELPROX = PINC2;  //Y Axis under travel prox input, PIN A2
int Z_UNDERTRAVELPROX = PINC1;  //Z Axis over travel prox input, PIN A1
int Z_OVERTRAVELPROX = PINC0;  //Z Axis under travel prox input, PIN A0

Are you planning on changing (hint) these small (hint) numbers during the course of your program?

No, I tried setting them to const int but then I get compling error: invalid conversion from 'const int*' to 'int*'

And there's another hint.

hints received (or atleast I hope so)

/*Command all 3 axis, 1 at a time, to move until it hits its undertravel limit switch, then change direction and
move until it hits its overtravel limit switch, and then record the amount of pulses it took to go limit to limit,
divide that value in half and go to that position then stop.*/
  
// avr-libc library includes 
#include <avr/io.h> 
#include <avr/interrupt.h> 

#define RUN_ENABLE 0x80 //Axis "ENABLE" Control Bit, PIN 8

const byte X_DIR = 0x04; //X axis "DIRECTION" Control Bit, PIN 2
const byte X_STEP = 0x08; //X Axis "STEP" Control Bit, PIN 3
const byte Y_DIR = 0x10; //X axis "DIRECTION" Control Bit, PIN 2
const byte Y_STEP = 0x20; //X Axis "STEP" Control Bit, PIN 3
const byte Z_DIR = 0x40; //X axis "DIRECTION" Control Bit, PIN 2
const byte Z_STEP = 0x80; //X Axis "STEP" Control Bit, PIN 3
const byte X_UNDERTRAVELPROX = PINC5;  //X Axis over travel prox input, PIN A5
const byte X_OVERTRAVELPROX = PINC4;  //X Axis under travel prox input, PIN A4
const byte Y_UNDERTRAVELPROX = PINC3;  //Y Axis over travel prox input, PIN A3
const byte Y_OVERTRAVELPROX = PINC2;  //Y Axis under travel prox input, PIN A2
const byte Z_UNDERTRAVELPROX = PINC1;  //Z Axis over travel prox input, PIN A1
const byte Z_OVERTRAVELPROX = PINC0;  //Z Axis under travel prox input, PIN A0

//pointers
const byte *STEP;
const byte *DIR;
const byte *UNDERTRAVELPROX;
const byte *OVERTRAVELPROX;
int *ACTUALPOS;
int *COMMANDPOS;
int *MAXSTEPS;

char activeaxis = 0;
int motionenable = 0;
int sequence = 0;
/*
seq 0 = idle
seq 1 = move active axis until under travel prox is made
seq 2 = move active axis until over travel prox is made then record
        number of pulses from under travel to over travel prox
seq 3 = move active axis to center of travel
*/

int index = 0;

int xaxismaxsteps;
int xaxiscommandpos;
int xaxisactualpos;
int yaxismaxsteps;
int yaxiscommandpos;
int yaxisactualpos;
int zaxismaxsteps;
int zaxiscommandpos;
int zaxisactualpos;

int feedrate = 9999;    //5ms pulse interval, 0.0492in/sec feedrate
    
void setup() { 
  DDRD |= 0xFC;  // sets digital pins 2,3,4,5,6,7 as output, step and direction pins
  DDRB |= 0x01;  // sets digital pin 8 as output, stepper drivers enable pin
  PORTD |= 0xD8; // sets digital pins 2,3,4,5,6,7 HIGH (internal pull-up resistor)

  cli();          // disable global interrupts  
  // initialize Timer2 
  TCCR1A = 0;     // set entire TCCR1A register to 0 
  TCCR1B = 0;     // same for TCCR1B 
  // turn on CTC mode: 
  TCCR1B |= (1 << WGM12); 
  // Set CS11 bit for 8 prescaler: 
  TCCR1B |= (1 << CS11); 
} 
  
void loop() { 
  // set compare match register equal to feedrate
  OCR1A = feedrate; 
  // enable global interrupts: 
  sei(); 
  
  if ((sequence == 0) && (index < 3)) {
    if (activeaxis == 0) { //first run
      activeaxis = 'X';
      //assign pointers
      DIR = &X_DIR;  //sets DIR to the memory address location of X_DIR
      STEP = &X_STEP;  //sets STEP to the memory address location of X_STEP
      UNDERTRAVELPROX = &X_UNDERTRAVELPROX;
      OVERTRAVELPROX = &X_OVERTRAVELPROX;
      ACTUALPOS = &xaxisactualpos;
      COMMANDPOS = &xaxiscommandpos;
      MAXSTEPS = &xaxismaxsteps;
    }
    else if (activeaxis == 'X') {
      activeaxis = 'Y';
      //assign pointers
      DIR = &Y_DIR;
      STEP = &Y_STEP;
      UNDERTRAVELPROX = &Y_UNDERTRAVELPROX;
      OVERTRAVELPROX = &Y_OVERTRAVELPROX;
      ACTUALPOS = &yaxisactualpos;
      COMMANDPOS = &yaxiscommandpos;
      MAXSTEPS = &yaxismaxsteps;
    }
    else if (activeaxis == 'Y') {
      activeaxis = 'Z';
      //assign pointers
      DIR = &Z_DIR;
      STEP = &Z_STEP;
      UNDERTRAVELPROX = &Z_UNDERTRAVELPROX;
      OVERTRAVELPROX = &Z_OVERTRAVELPROX;
      ACTUALPOS = &zaxisactualpos;
      COMMANDPOS = &zaxiscommandpos;
      MAXSTEPS = &zaxismaxsteps;
    }
    //set direction of active axis to reverse
    PORTD &= ~*DIR;
    //zero active axis position
    *ACTUALPOS = 0;    // zero actual steps
    //command active axis to maximum negative position
    *COMMANDPOS = -32768;
    sequence = 1;  //advance to the next sequence
    motionenable = 1;
    // enable timer 1 compare interrupt used for pulsing stepper driver output: 
    TIMSK1 |= (1 << OCIE1A);
  }

  if ((sequence == 1) && (*UNDERTRAVELPROX != 1)) {  //under travel prox reached
    motionenable = 0; // disable motion
    *ACTUALPOS = 0;    // zero actual steps
    *COMMANDPOS = 32768;    //command axis to maximum positive position
    // change direction of X axis
    PORTD |= *DIR; // set pin HIGH     
    sequence = 2;  //advance to the next sequence
    motionenable = 1; // enable motion
  }
  if ((sequence == 2) && (*OVERTRAVELPROX != 1)) {
    motionenable = 0; // disable motion
    *MAXSTEPS = *ACTUALPOS;
    *COMMANDPOS = (*MAXSTEPS / 2);
    // change direction of X axis
    PORTD &= ~*DIR; // set pin LOW     
    sequence = 3;  //advance to the next sequence
    motionenable = 1; // enable motion
  }
  if ((sequence == 3) && (*ACTUALPOS == *COMMANDPOS)) {
    motionenable = 0; // disable motion
    sequence = 0;  //loop back to sequence 0
    index ++;
  }
}

ISR (TIMER1_COMPA_vect) {
  int t =0;
  
  if (motionenable == 1) {
    if((PORTD & *DIR) != ~*DIR) //If going forward, increment active axis position counter
      *ACTUALPOS++;
    else //If going reverse, decrement axis position counter
      *ACTUALPOS--;
    if (*ACTUALPOS != *COMMANDPOS) { //If we are not at our commanded position     
      PORTD &= ~*STEP; //Toggle "Step" output with each entry
      while(t++ != 10); 
        PORTD |= *STEP;
    }
  } 
}