millis in a function

Hi,

I am trying to move a motor using millis() and have come to a dead end. I have had it working in just the loop function but I am now trying to increase my program to control movement in different directions and speeds. This has calkled for me to place it in a function outside of the main loop. I have added my code for the section using millis below to see if you can spot why it is not working.

I feel like I have done everything I did in the main loop, just know I have it running in a for loop and a switch case, could it be that the millis does not work in a for loop or do ~I have to start the millis somewhere else?

Thanks

void timelapse_go() {
  serial_lcd_clear();
  serial_lcd_showline(1);
  serial_lcd_writeline(1);
  serial_lcd.println("Timelapse Running");
    int i = 0;
    unsigned long Previousmillis = 0;
      unsigned long Currentmillis = millis();
  switch (num_steps) {
    case 1: {
      for (i=0;i==num_pics/4;i++) {
        if (Currentmillis - Previousmillis == pic_1 || pic_2 || pic_3 || pic_4) {
          digitalWrite(shutterPin, LOW);
          delay(25);
          digitalWrite(shutterPin, HIGH);
        }
          else if (Currentmillis - Previousmillis == step_1) {
            mmm_1();
          }
            else if (Currentmillis - Previousmillis == step_2) {
              mmm_2();
            }
              else if (Currentmillis - Previousmillis == step_3) {
                mmm_3();
              }
                else if (Currentmillis - Previousmillis == step_4) {
                  mmm_4();
                }
                  else if(currentMillis - previousMillis == lapse_end){
                    previousMillis = currentMillis;
                  }
      }
          battery_check();
    }
  }
}

There are a number of programming errors. Do you want me to name them all ?

could it be that the millis does not work in a for loop or do ~I have to start the millis somewhere else?

No and no.

      for (i=0;i==num_pics/4;i++) {

Starting with i eqaul 0, while i is equal to num_pics divided by 4, do something and then increment i by 1. Does that make sense? Unless num_pics is 0, this loop will never do anything. If num_pics IS 0, does it make sense for it to do anything?

        if (Currentmillis - Previousmillis == pic_1 || pic_2 || pic_3 || pic_4) {

This is interpreted as if now minus then is equal to pic_1 or pic_2 is true or pic_3 is true or pic_4 is true... Is that really your intent?

        if (Currentmillis - Previousmillis == pic_1 || pic_2 || pic_3 || pic_4) {
          digitalWrite(shutterPin, LOW);
          delay(25);
          digitalWrite(shutterPin, HIGH);
        }
          else if (Currentmillis - Previousmillis == step_1) {
            mmm_1();
          }
            else if (Currentmillis - Previousmillis == step_2) {
              mmm_2();
            }
              else if (Currentmillis - Previousmillis == step_3) {
                mmm_3();
              }
                else if (Currentmillis - Previousmillis == step_4) {
                  mmm_4();
                }
                  else if(currentMillis - previousMillis == lapse_end){
                    previousMillis = currentMillis;
                  }

An else if statement does NOT get incremented further.

I seriously doubt that this line is doing what you think it is. This isn't how you compare to more than one thing.

if (Currentmillis - Previousmillis == pic_1 || pic_2 || pic_3 || pic_4)

I suspect you meant:

if (Currentmillis - Previousmillis == pic_1 || Currentmillis - Previousmillis ==pic_2 || Currentmillis - Previousmillis == pic_3 || Currentmillis - Previousmillis == pic_4)

The other problem you are probably having is scope. It's hard to tell without seeing the rest of your code and how this function is called, but every time it exits Previousmillis goes away and every time it gets called Previousmillis is set back to zero. You probably want Previousmillis to be either global or static.

Several things: First, you should show all your code so we can see if it compiles. Second, assuming it does compile, use Ctrl-T to auto-format your code. It makes it easier for us to read.

In your code, you have a statement:

       if (Currentmillis - Previousmillis == pic_1 || pic_2 || pic_3 || pic_4) {

which checks to see if the elapsed time is equal to pic_1. Keep in mind that calling millis() takes a little time itself so the chances of the elapsed time being exactly equal to pic_1 is pretty small. A better operator is probably ">=".

In the same statement, you cannot test elapsed time for pic_2 - pic_4 using your syntax. If any of those variables are non-zero, they are logic True and the statement appears True, which I don't think is what you want.

Next, the statements:

          else if (Currentmillis - Previousmillis == step_1) {
            mmm_1();
          }
            else if (Currentmillis - Previousmillis == step_2) {
              mmm_2();
            }
              else if (Currentmillis - Previousmillis == step_3) {
                mmm_3();
              }
                else if (Currentmillis - Previousmillis == step_4) {
                  mmm_4();
                }
                  else if(currentMillis - previousMillis == lapse_end){
                    previousMillis = currentMillis;
                  }

are often referred to as a cascading if statement. Given that your switch/case only has one case to it, it might make more sense to use the switch/case for the cascading if and use a plain if statement in place of the switch/case, as in:

  long elapsedTime; 

  if (num_steps == 1) {
      for (i=0;i==num_pics/4;i++) {
        elapsedTime = Currentmillis = Previousmillis;
        if (elapsedTime  == pic_1 || elapsedTime  == pic_2 || elapsedTime  == pic_3 || elapsedTime == pic_4) {
          digitalWrite(shutterPin, LOW);
          delay(25);
          digitalWrite(shutterPin, HIGH);
        } else {
           switch(elapsedTime) {
             case step_1:
                mmm_1();
                break;
            case step_2:
               mmm_2();
               break;
           case step_3:
              mmm_3();
              break;
           case step_4
                mmm_4();
                break;
           case  lapse_end:
                previousMillis = currentMillis;
                break;
            default:
               Serial.println("I should never get here...");
               break;
      }
      battery_check();
  }

I find it easier to read switch/case statements than cascading if's.

Thanks for all your help guys, I knew therewould be quite a few mistakes probably in here.

First of all with the for loop I thought it read as starting at i=0 until i == num_pics divided by 4 incrementing i, is this not the case?

The or statement you have suggested makes much more sense so I have implemented that.

I am using cascading if’s due to the fact that I will be using many switch cases depending on there being 1 step per picture, 2 steps or more etc etc.

Here is all of my code, there are probably loads of mistakes but everything so far seems to be working (I have left out some parts as the maximum characters per post was met).

#include <SoftwareSerial.h>
// Displays a welcome message and sets up the serial links to the monitor and the lcd display

void setup() {
  Serial.begin(9600);

  serial_lcd.begin(9600);

  serial_lcd_init();
  serial_lcd.println(" CameraDolleyv1");
  serial_lcd_writeline(2);
  serial_lcd.println("     By MASH");
  serial_lcd_showline(1);

  delay(3000);

  pinMode(motorPin1, OUTPUT);
  pinMode(motorPin2, OUTPUT);
  pinMode(motorPin3, OUTPUT);
  pinMode(motorPin4, OUTPUT);
  pinMode(shutterPin, OUTPUT);

  main_menu();

}

// Looks for buttons pressed and reacts accordingly

void loop() {

  if (serial_lcd.available()) {
    button = serial_lcd.read();
    Serial.write(button);
    if (bc <= 3) {
      if (button == 65) {
        serial_lcd_moveup();
      }
      else if (button == 66) {
        serial_lcd_movedown();
      }
      else if (button == 67) {
        switch (bc) {
        case 1:
          move_lapse_start();
          break;
        case 2:
          move_video_start();
          break;
        case 4:
          cfig();
          break;
        }
      }
    }

    else if (bc == 4) {
      if (button == 65){ //move motor left
        while  (button != 97){
          button = serial_lcd.read();
          mmm_1();
          steps--;
          mmm_2();
          steps--;
          mmm_3();
          steps--;
          mmm_4();
          steps--;
        }
      }
      if (button == 66) { //Move motor right
        while (button != 98) {
          button = serial_lcd.read();
          mmm_4();
          steps++;
          mmm_3();
          steps++;
          mmm_2();
          steps++;
          mmm_1();
          steps++;
        }
      }
      if (button == 67) {
        steps = 0;
        move_lapse_finish();
      }
    }

    else if (bc == 5) {
      if (button == 65){
        while  (button != 97){
          button = serial_lcd.read();
          mmm_1();
          steps--;
          mmm_2();
          steps--;
          mmm_3();
          steps--;
          mmm_4();
          steps--;
        }
      }
      else if (button == 66) {
        while (button != 98) { //move motor right
          button = serial_lcd.read();
          mmm_4();
          steps++;
          mmm_3();
          steps++;
          mmm_2();
          steps++;
          mmm_1();
          steps++;
        }
      }
      if (button == 67) {
        interval();
      }
    }

    else if (bc == 6) {
      if (button == 65) {
        intval++;
        interval();
      }
      else if (button == 66) {
        intval--;
        interval();
      }
      else if (button == 67) {
        number_of_pics();
      }
    }

    else if (bc == 7) {
      if (button == 65) {
        num_steps++;
        number_of_pics();
      }
      else if (button == 66) {
        num_steps--;
        number_of_pics();
      }
      else if (button == 67) {
        lapse_steps();
        timelapse_go();
      }
    }

    if (button == 68) {
      main_menu();
    }
  }
}

void serial_lcd_init() {
  delay(2500);

  serial_lcd.write(12);

  serial_lcd.write((byte)254);
  serial_lcd.write((byte)67);
  serial_lcd.write((byte)3);
}

void serial_lcd_writeline(int linenum) {
  serial_lcd.write((byte)254);
  serial_lcd.write((byte)76);
  serial_lcd.write((byte)linenum);
}

void serial_lcd_showline(int linenum) {
  serial_lcd.write((byte)254);
  serial_lcd.write((byte)71);
  serial_lcd.write((byte)linenum);
}

void serial_lcd_positioncursor(int linenum, int colnum) {
  serial_lcd.write((byte)254);
  serial_lcd.write((byte)80);
  serial_lcd.write((byte)linenum);
  serial_lcd.write((byte)colnum);
}

void serial_lcd_clear() {
  serial_lcd.write((byte)12);
}

int serial_lcd_moveup() {
  if (bc > 1) {
    bc--;
  }
  else {
    bc = 3;
  }
  serial_lcd.write((byte)11);
  serial_lcd.write((byte)254);
  serial_lcd.write((byte)71);
  serial_lcd.write((byte)bc);
  Serial.print(bc);
  return bc;
}

int serial_lcd_movedown() {
  if (bc < 3) {
    bc++;
  }
  else {
    bc = 1;
  }
  Serial.print(bc);
  serial_lcd.write((byte)10);
  serial_lcd.write((byte)254);
  serial_lcd.write((byte)71);
  serial_lcd.write((byte)bc);
  return bc;
}

void main_menu() {

  bc = 1;
  serial_lcd_clear();
  serial_lcd_showline(1);
  serial_lcd.println("Setup Timelapse");
  serial_lcd_showline(2);
  serial_lcd.println("Video");
  serial_lcd_showline(3);
  serial_lcd.println("Config");
  serial_lcd_showline(1);

}

void move_lapse_start() {
  bc = 4;
  serial_lcd_clear();
  serial_lcd_showline(1);
  serial_lcd.write((byte)152);
  serial_lcd.println(" Move to     OK");
  serial_lcd_showline(2);
  serial_lcd.write((byte)126);
  serial_lcd.println(" Start     Back");
  serial_lcd_showline(1);
  Serial.print(bc);
}

void move_lapse_finish() {
  bc = 5;
  serial_lcd_clear();
  serial_lcd_showline(1);
  serial_lcd.write((byte)152);
  serial_lcd.println(" Move to     OK");
  serial_lcd_showline(2);
  serial_lcd.write((byte)126);
  serial_lcd.println(" Finish    Back");
  serial_lcd_showline(1);
  Serial.print(bc);
}


void interval() {
  Serial.println(steps);
  bc = 6;
  serial_lcd_clear();
  serial_lcd_showline(1);
  serial_lcd.println("> Shot int    OK");
  serial_lcd_showline(2);
  serial_lcd.println("< ");
  serial_lcd_positioncursor(2,3);
  serial_lcd.println((int)intval);
  serial_lcd_positioncursor(2,5);
  serial_lcd.println("secs");
  serial_lcd_showline(1);
  Serial.print(bc);
}

void number_of_pics() {
  bc = 7;
  serial_lcd_clear();
  serial_lcd_showline(1);
  serial_lcd.println("> No. of Pics OK");
  serial_lcd_showline(2);
  serial_lcd.println("< ");
  serial_lcd_positioncursor(2,3);
  serial_lcd.println((int)num_steps);
  pic_amount();
  serial_lcd_positioncursor(2,5);
  serial_lcd.println((int)num_pics);
  serial_lcd_showline(1);
  Serial.print(bc);
}

float lapse_steps() {
  if (num_steps == 1) {
    pic_1 = 0;
    step_1 = (intval * 0.15) * 100;
    pic_2 = intval * 100;
    step_2 = ((intval * 0.15) + pic_2) * 100;
    pic_3 = (2 * intval) * 100;
    step_3 = ((intval * 0.15) + pic_3) * 100;
    pic_4 = (3 * intval) * 100;
    step_4 = ((intval * 0.15) + pic_4) * 100;
    lapse_end = (4 * intval) * 100;
  }
  return step_1, step_2, step_3, step_4, pic_1, pic_2, pic_3, pic_4, lapse_end;
}

void timelapse_go() {
  float Currentmillis = millis();
  serial_lcd_clear();
  serial_lcd_showline(1);
  serial_lcd_writeline(1);
  serial_lcd.println("Timelapse Running");
  int i = 0;
  switch (num_steps) {
  case 1: 
    {
      for (i=0;i==num_pics/4;i++) {
        elapsedTime = Currentmillis - Previousmillis;
        if (elapsedTime  == pic_1 || elapsedTime  == pic_2 || elapsedTime  == pic_3 || elapsedTime == pic_4) {
          digitalWrite(shutterPin, LOW);
          delay(25);
          digitalWrite(shutterPin, HIGH);
        }
        else if (elapsedTime  == step_1) {
          mmm_1();
        }
        else if (elapsedTime  == step_2) {
          mmm_2();
        }
        else if (elapsedTime  == step_2) {
          mmm_3();
        }
        else if (elapsedTime  == step_4) {
          mmm_4();
        }
        else if(elapsedTime  == lapse_end){
          Previousmillis = Currentmillis;
        }
      }
//      battery_check();
    }
  }
}

void battery_check() {

}

void move_video_start() {

  serial_lcd_clear();
  serial_lcd_showline(1);
  serial_lcd.println("Move to vid start point");
}

void mmm_1() {
  m_pos = 1;
  digitalWrite(motorPin1, HIGH);
  digitalWrite(motorPin2, LOW);
  digitalWrite(motorPin3, HIGH);
  digitalWrite(motorPin4, LOW);
  delay(25);
}

void mmm_2() {
  m_pos = 2;
  digitalWrite(motorPin1, LOW);
  digitalWrite(motorPin2, HIGH);
  digitalWrite(motorPin3, HIGH);
  digitalWrite(motorPin4, LOW);
  delay(25);
}

void mmm_3() {
  m_pos = 3;
  digitalWrite(motorPin1, LOW);
  digitalWrite(motorPin2, HIGH);
  digitalWrite(motorPin3, LOW);
  digitalWrite(motorPin4, HIGH);
  delay(25);
}

void mmm_4() {
  m_pos = 4;
  digitalWrite(motorPin1, HIGH);
  digitalWrite(motorPin2, LOW);
  digitalWrite(motorPin3, LOW);
  digitalWrite(motorPin4, HIGH);
  delay(25);
}

First of all with the for loop I thought it read as starting at i=0 until i == num_pics divided by 4 incrementing i, is this not the case?

No. The middle clause is a while clause, not an until clause.

Ah yeah I remember now, thanks for that.

I have changed it to while i is less than num_pics/4 (i<num_pics/4), but still no joy.

Mashley: I am using cascading if's due to the fact that I will be using many switch cases depending on there being 1 step per picture, 2 steps or more etc etc.

You need to find a much much simpler way of organizing your code. In a few days time (never mind in 6 months) you will have forgotten how all those IF and CASE statements are supposed to work.

Break your code into several short functions with meaningful names each of which does one piece of the action.

...R

Thanks for your comment Robin2, but it is a complex menu structure running quite a complex process with lots of options when I will have it all finished. I have been trying to write the least ampount of functions that I can but this is the only way I know to do what I need. If you can see any way of doing bits better and simpler please let me know.

Part of the problem comes from the lcd screen I am using (the only one readily available to me 'Cat's Whiskers Textstar'). This only has four buttons and two lines of 16 characters. This has made me do each section of set up then move to the next one causing five or six screens before the for loop runs.

Still have not sorted this by the way so if anyone can help I would be very happy.

Thanks,

Mashley: (I have left out some parts as the maximum characters per post was met).

You can attach files, such as your code.

How to use this forum

Without looking through your code or this entire thread, the first thing that comes to mind is that millis don't get updated within an interrupt routine (or any of the functions called from that interrupt routine).

Since you are using millis to control timing of motors, I wonder if this is the trap you have fallen into.

      if (button == 65) {
        serial_lcd_moveup();
      }
      else if (button == 66) {
        serial_lcd_movedown();
      }
      else if (button == 67) {

...

      else if (button == 66) {
        while (button != 98) { //move motor right

I presume these are ASCII letters? How about making it easier on yourself?

      if (button == 'A') {
        serial_lcd_moveup();
      }
      else if (button == 'B') {
        serial_lcd_movedown();
      }
      else if (button == 'C') {

...

      else if (button == 'B') {
        while (button != 'b') { //move motor right

That's a lot easier to follow isn't it?

Yep I think I will change that as you are right A is easier to remember than what 66 does.

Ken F, can you elaborate a little? Do you mean that the for loop is an interrupt routine that will not look at millis while it is running as it is self contained. Does this mean I would have to put all my movement sections in the main loop so the millie will work?

Thanks

Mash

Mash
NO a for loop is NOT an interrupt routine.

If you don’t use attachInterrupt() in your code then the issue I mentioned is irrelevant. Pay it no attention.

KenF: Mash NO a for loop is NOT an interrupt routine.

If you don't use attachInterrupt() in your code then the issue I mentioned is irrelevant. Pay it no attention.

Another good reason to post all of your code. So people don't have to try to guess how the rest of it works.

Mashley: I have been trying to write the least ampount of functions that I can

I would be trying to use functions wherever they help to simplify the code regardless of how many that might be.

Have a look at Planning and Implementing a Program.

...R

Thanks for the link Robin2.

Can we get back on topic though and try and solve why the millis isn’t working, Thanks.

I have attached all my code to this post, all that was missing from the last one was my global variables.

Thanks.

lcddisplay1_1.ino (10.1 KB)

Mashley: Thanks for the link Robin2.

Can we get back on topic though and try and solve why the millis isn't working, Thanks.

I have attached all my code to this post, all that was missing from the last one was my global variables.

Thanks.

millis() returns an unsigned long integer, not a float, FYI

void timelapse_go() {
  float Currentmillis = millis();
  serial_lcd_clear();
  serial_lcd_showline(1);
  serial_lcd_writeline(1);
  serial_lcd.println("Timelapse Running");
  int i = 0;

Yep cheers for that, I had it like that before but was trying something out as the motor should move at 1.5 seconds (for some setting) but as its obviously in millis that doesn't matter. I have changed it back but still no joy.

I think I am going to restructure the program trying to put everything in the main loop in functions other than the millis section.

Thanks,

Matt