What have I done right/wrong?

Hello everyone. Let me start by saying I am very new to programming. I bought the uno for an electrical engineering class and had some assignments that were basically just copying the blink and other simple programs. For my final project I am using the uno to control a servo from an rc plane that will press the buttons on a camera as a kind of poor man's time lapse camera.

I wanted to figure it out for myself before I asked for help so I already made the code do what I want it to do. I am sure that I did everything the hard way though and I wanted some more experienced opinions.

What it does: moves the servo in the "power" direction for 0.5 s, then in the "center" direction for 1 s, then it oscillates between "shutter" and "center" indefinitely.

int signal = 2;  

int c = 1300;

int p = 1160;

int s = 1450;

int interval = 3000;

int shutterTime = 0500;

int powerTime = 0500;

unsigned long previousTime = 0; 

void setup()                           

{                                      

  pinMode(signal, OUTPUT);

}

void loop()

{

  if (millis() < powerTime)

  {

    power();

  }

  else

  {

    if (millis() < 1500)

    {

      center();
      
      previousTime = millis();

    }

    else

    {

      if (millis() - previousTime < interval)

      {

        if (millis() - previousTime < shutterTime)

        {

          shutter();

        }

        else

        {

          center();

        }

      }

      else

      {

        reset();

      }

    }

  }

}

void shutter()                         

{

  digitalWrite(signal, HIGH);

  delayMicroseconds(s);

  digitalWrite(signal, LOW);

  delay(20);

}

void center()                         

{

  digitalWrite(signal, HIGH);

  delayMicroseconds(c);

  digitalWrite(signal, LOW);

  delay(20);

}

void reset()

{

  previousTime = millis();

}

void power()                         

{

  digitalWrite(signal, HIGH);

  delayMicroseconds(p);

  digitalWrite(signal, LOW);

  delay(20);

}

I think the behaviour you describe is because "loop()", well, loops.

I understand that. That's what I want it to do. It should only do the "power" function once, then oscillate between "shutter" and "center." I'm talking about the way I did it with the ifs inside ifs inside ifs. I believe there has to be a better way to make an interval timer.

If you want to do something once, and only once, and only at the beginning, you can put it in setup()

cholz:
I wanted to figure it out for myself before I asked for help

Bravo!!!

So many come here stating that they want to make a space shuttle, can we show them how.
You have read up ahead of time, and actually TRIED something before posting. Good on you!

Huh, reading further, I see you have the device behaving the way you want it to. Another Bravo for you!

So now you are looking for a critique of you coding methods.
My first comment would be - Why aren't you using the servo library? It does a lot of the work for you. It sends out signals to the servo about 50 times per second, which is what the typical RC servo expects.

With your direct handling of the servo signal, you are not sending signals in the frame rate the servo expects. It likely works for your servo, but others might not be as happy.

You might moving the call to the power() function in to the setup function. Since you only want it to happen once, at the start of the execution, that is a good place for it. It will clean up your loop a bit.

Something to keep in mind - the Millis function overflows back to zero after about 50 days. This might be relevant to you if you are running a long series.

A popular method to handle timing issues and different actions is a state machine. Something like this:

#define START   0
#define POWER   1
#define CENTRE  2
#define SHUTTER 3

int state=START;
unsigned long NextAction=0L;

void loop()
{
static int PulseLength=0;
if(millis()>NextAction)
  PulseLength=ChangeState();
SendServoPulse(PulseLength);
}

int ChangeState()
{
int PulseLength=0;
switch (state)
  {
  case START:
    NextAction+=500L;
    PulseLength=p;
    state=POWER;
    break;
  case POWER:
    NextAction+=1500L;
    PulseLength=c;
    state=CENTRE;
    break;
  case CENTRE:
    NextAction+=500L;
    PulseLength=s;
    state=SHUTTER;
    break;
  case SHUTTER:
    NextAction+=1500L;
    PulseLength=c;
    state=CENTRE;
    break;
  }
return PulseLength;
}

void SendServoPulse(int PulseLength)
{
digitalWrite(signal, HIGH);
delayMicroseconds(PulseLength);
digitalWrite(signal, LOW);
delay(20);
}

Note also that I combined your separate Shutter, Power and Center routines as they are so similar. Also, I can't compile it on this machine, so it likely has a syntax error or three.

@tempmj: I can't put the power() in the setup. It has to be looped in order for it to generate a signal.

@vinceherman: I checked out the servo library. I didn't really understand what was going on though. I'm not sure I understand what a library is even. I did use it to test my servo when I wasn't sure if it was my code or my circuit that was giving me trouble. I like my method because at least I know exactly what is happening and why, but I see why it is limited and in the future I will make an attempt to understand the servo library.

@wildbill: Thanks! This is what I was looking for. I was sure there had to be simpler way. Although I don't really understand what is going on with what you wrote, I will definitely try to figure it out.

I can't put the power() in the setup. It has to be looped in order for it to generate a signal.

But power only runs in the first 320 (why octal?) milliseconds after reset (or timer wrap).
No need for it to be in "loop".

cholz:
Thanks! This is what I was looking for. I was sure there had to be simpler way. Although I don't really understand what is going on with what you wrote, I will definitely try to figure it out.

Here's what it's doing:
Your system basically has to worry about three things: what are we doing, how long will we be doing it and what length of pulse we need to send to the servo to achieve it.

The state variable keeps track of what we’re doing. It will be START when the arduino is powered up, then it needs to change to POWER for a while, then alternate between CENTER and SHUTTER with appropriate delays.

NextAction is the time in millis when the next state change will occur. All your state changes are time based, so we just check on every iteration of loop to see whether that time has been passed. If so, we call ChangeState to set up what to do next.

Your state changes are simple, there are no choices to be made, if we are in one state and it’s time to change, the next state is always the same. The switch statement implements this and changes START to POWER, POWER to CENTRE, CENTRE to SHUTTER and SHUTTER to CENTRE. It also increases NextAction to the due time of the next state change so that we can stay in our new state for the desired duration. Finally, it sets the PulseWidth to whatever is appropriate for the new state and a pulse of that size will be sent to the server on each iteration of loop.

In this example, there are few states and only one event (elapsed time) to trigger state change, so the advantage of the method is somewhat limited. With more inputs & outputs and more states with a more complicated flow between them, it can be a nice way to model your system and encapsulate your change logic.

Edit: typo