My first program - looking for feedback

Hello,
So I started messing around with Auduino’s today as I needed to control a stepper motor. This is the first program I have ever written in anything but Q basic when I was at school. It has taken me all day but I got it working, then improved it, then fixed a bug! I learnt as I went from the reference material here and from forum posts.

It would be really good if someone could give feedback on what I have done. Are there better ways of doing what I have done? (without getting too far beyond the basics at this point :D)

It is a controller for a stepper motor that I want to use to raise and lower a table on my laser cutter.

  • There are two buttons, one for clockwise rotation and for for CC.
  • The speed of the stepper is controlled by a potentiometer.
  • There are two limit switches.
  • The max speed of the stepper is limited.
  • The Easy Driver goes in to sleep mode when no buttons are pressed.
  • The Easy Driver goes in to sleep mode when both buttons are pressed at once, to stop the high pitch whine from the stepper being confused.
//Hububalli's Z axis control Sketch
//Controls a stepper motor via two buttons and a speed control.
//Uses a limit switch on each button. The EasyDriver also sleeps
//when nothing is pressed to save power.

#include <AccelStepper.h>        // import the AccelSteer lib

AccelStepper stepper(1,9,8);     // define the stepper called "stepper" pin 9 is Direction, pin 8 is Step

int upPin = 2;       // UP Button
int downPin = 3;     // DOWN Button
int upLimit = 4;     // Upper limit
int downLimit = 5;   // Lower limit
int speedPot = A0;   // Speed pot
int edSleep = 6;     // easy driver sleep mode (HIGH - Awake, LOW - sleep)


void setup() {
  pinMode(upPin, INPUT);         // declare upPin as an input
  pinMode(downPin, INPUT);       // declare downPin as an input
  pinMode(upLimit, INPUT);       // declare upLimit as an input
  pinMode(downLimit, INPUT);     // declare downLimit as an input
  pinMode(speedPot, INPUT);      // declare speedPot as an input
  pinMode(edSleep, OUTPUT);      // declare edEnable as an output
  
  stepper.setMaxSpeed(1500);     // set stepper max speed
  stepper.setSpeed(100);         // Set stepper speed
  
}

void loop(){
  if (digitalRead(upPin) == HIGH && digitalRead(downPin) == HIGH) {    // If both buttons are released
     digitalWrite(edSleep, LOW);                               // Set edSleep to LOW(Sleep)
  }
  
  if (digitalRead(upPin) == LOW && digitalRead(downPin) == LOW) {      // If both buttons are pressed (error)
     digitalWrite(edSleep, LOW);                               // Set edSleep to LOW(Sleep)
     } else
      {  
  
        if (digitalRead(upLimit) == HIGH) {                    // Check if the upper limit switch is open
    
        if (digitalRead(upPin) == LOW) {                       // Check if the up button is pressed
          digitalWrite(edSleep, HIGH);                         // Set edSleep to HIGH(Awake)
          stepper.runSpeed();                                  // Run the Stepper
          int potValue = analogRead(speedPot);                 // define potValue by reading pot
          int stepSpeed = map(potValue, 0, 1023, 100, 1500);   // define stepSpeed by mapping potValue
          stepper.setSpeed(stepSpeed);                         // Set the stepper speed with stepSpeed
          } 
        }
    
      if (digitalRead(downLimit) == HIGH) {                    // Check if the lower limit switch is open
    
        if (digitalRead(downPin) == LOW) {                     // check if the down button is pressed
          digitalWrite(edSleep, HIGH);                         // Set edSleep to HIGH(Awake)
          stepper.runSpeed();                                  // Run the Stepper
          int potValue = analogRead(speedPot);                 // define potValue by reading pot
          int stepSpeed = map(potValue, 0, 1023, 100, 1500);   // define stepSpeed by mapping potValue
          int stepSpeedNeg = (stepSpeed)*-1;                   // Define stepStepNeg (convert + to - CCW)
          stepper.setSpeed(stepSpeedNeg);                      // Set the stepper speed with stepSpeed
          }  
        }
      }  
}

Hi,

Congratulatons Hububalli, good to see persistence and your hard work on research, has got you a working sketch.

I see you have used lots of comment lines to keep track of your progress, you will learn in time that a lot of them are not necessary as the line of code is self explanatory.

I'm at work during lunch so son't have time to look deeply at your code but from what I see layout wise it is very good, it looks like you have used AutoFormat which makes all the indents nice and even and easy to follow.

To make you presentation complete so someone else looking at accomplishing what you have, a circuit diagram of your project would also help.

Well done...Tom.... :)

Kudos for making it really easy to read. Also well written.

After you declare a variable, you no longer need to redeclare it in the middle of your program, for example:

int dusty;
int dusty = <whatever>;

You should have one declaration in setup() with an optional initialization:

int dusty = <whatever>;

or just

int dusty;

then in your code:

dusty = <whatever>;

The comments should talk more to the application than the code. I’m just using an example here:

temp = getTemp(ROOF); // assign the result of passing ROOF enumerated constant to function getTemp() to variable temp

Is useless because a C reference will tell you all that. It’s redundant information. The comment should elucidate the code, for example:

temp = getTemp(ROOF); // record the temperature from the roof sensor

I see a lot of this:

// xDistance is a constant
const int xDistance;
 // x, y and z are variables
int x,y,z;

Well, yeah, I guess a constant is a constant and a variable is a variable! It’s appropriate for examples that introduce the language features, but not in a real application, no matter how small.

One more thing, I see several consecutive lines in your if statements that are nearly identical. That is usually a sign that you can “factor” them into a single function and then call the function twice with different parameters.

Not bad for one day.

I usually use INPUT_PULLUP in pinMode so the pull-up resistors are enabled.

For variables which won't change, I use const Ex: int downPin = 3; // DOWN Button const int downPin = 3; // DOWN Button This will save 2 bytes of RAM

I like putting the opening brace on a new line. if(a == b) { . . . }

hi

also

pinMode(upPin, INPUT);         // declare upPin as an input
  pinMode(downPin, INPUT);       // declare downPin as an input
  pinMode(upLimit, INPUT);       // declare upLimit as an input
  pinMode(downLimit, INPUT);     // declare downLimit as an input
  pinMode(speedPot, INPUT);      // declare speedPot as an input

if you have to set a lots of pins as inputs

you may consider using an array for example from the guy Paul something in this forum

int ledPins[] = {9, 3, 2, 12, 15, 11, 7, 6}; // LED pins
int ledCnt = 8;

void setup()
{
   for(int p=0; p<lenCnt; p++)
   {
       pinMode(ledPins[p], OUTPUT); // Set the mode to OUTPUT
   }
}

Thanks for all the suggestions. I have been working on a second version taking into consideration what you all have said, ive just been a bit busy to get it finish the last week. I agree the comments are quite over the top, at that stage it was more for me to keep track of what was going on as I wrote it.

I also tried making a circuit diagram in a Fritzing which I have seen mentioned here quite a lot. Trouble is I have no idea what im doing so its just a mess of lines and symbols. I have the breadboard layout done so must just have to post that instead.

Once I get it done is there a place where you can put completed Sketches? Like some kind of repository?

Cheers,

You may find something useful in stepper motor basics, or you may already be past that stage.

Usually the most informative circuit diagrams are those drawn with a pencil - just post a photo of the drawing. Most people who use Fritzing for diagrams on this website create images that look like photos of breadboards etc which are very difficult to follow.

If you want others to have access to your program just post it in this Thread when it is ready. DO NOT treat that as an alternative to having your own back-up copy.

...R