Making Functions behave

Hi,

im working through the ‘planning code’ using functions article and trying to customise it as i go. so far so good!

I now want to :

  1. run the moveServo() function 3 times before moving onto the next function (flashLEDB)

  2. While running the move servo() function, run the flashLedA() function at the same time then stop both after 3 cycles and move onto the next function.

You can see in the void loop() function i tried

for (i=1; i==3; i++){
moveServo();
};

Which should have worked?

What am i doing wrong please?

// LessonD.ino
// Program with servo, potentiometer and led flashes


//======for the servo==========
#include <Servo.h>
Servo myServo;
const byte servoPin = 20;
const int servoMinDegrees = 20; // the limits to servo movement
const int servoMaxDegrees = 150;
Servo myservo;  // create servo object to control a servo 

int servoPosition = 90;     // the current angle of the servo - starting at 90.
int servoSlowInterval = 80; // millisecs between servo moves
int servoFastInterval = 10;
int servoInterval = servoSlowInterval; // initial millisecs between servo moves
int servoDegrees = 2;       // amount servo moves at each step 
                            //    will be changed to negative value for movement in the other directio
unsigned long previousServoMillis = 0; // the time when the servo was last moved

 int counter = 0;

//======for the potentiometer===
const byte potPin = A0;
int potValue;

//======for the LEDs============
const unsigned long ledOnMillis = 300;
const unsigned long ledAbaseInterval = 500;
const unsigned long ledBbaseInterval = 800;
const unsigned long ledCbaseInterval = 200;
unsigned long ledAInterval = ledAbaseInterval;
unsigned long ledBInterval = ledBbaseInterval;
unsigned long ledCInterval = ledBbaseInterval;

byte ledAstate = HIGH;
byte ledBstate = HIGH;
byte ledCstate = HIGH;

unsigned long prevLedAMillis;
unsigned long prevLedBMillis;
unsigned long prevLedCMillis;
unsigned long currentMillis;
const byte ledApin = 3;
const byte ledBpin = 4;
const byte ledCpin = 6;


void setup() {
 myServo.attach(servoPin);
 myServo.write(servoPosition);
 
 pinMode(ledApin, OUTPUT);
 pinMode(ledBpin, OUTPUT);
 pinMode(ledCpin, OUTPUT);
 digitalWrite(ledApin, HIGH);
 digitalWrite(ledBpin, HIGH);
 digitalWrite(ledCpin, HIGH);
 
 delay(2000);
}
int i=0;

void loop() {

   currentMillis = millis();

   checkButtons();
   setFlashPeriod();
  
  for (i=1; i==3; i++){
        moveServo();
    };

   flashLedA();
   flashLedB();
   flashLedC1(); 
   
   
    
   
  
   
   askForUserInput();
   getUserResponse();
   //moveServo();
}

void flashLedA() {
 if (currentMillis - prevLedAMillis >= ledAInterval) {
   prevLedAMillis += ledAInterval;
   ledAstate = ! ledAstate;
   if (ledAstate == HIGH) {
     ledAInterval = ledOnMillis;
   }
   else {
     ledAInterval = ledAbaseInterval;
   }
   digitalWrite(ledApin, ledAstate);
 }

}


void flashLedB() {
 if (currentMillis - prevLedBMillis >= ledBInterval) {
   prevLedBMillis += ledBInterval;
   ledBstate = ! ledBstate;
   if (ledBstate == HIGH) {
     ledBInterval = ledOnMillis;
   }
   else {
     ledBInterval = ledBbaseInterval;
   }
   digitalWrite(ledBpin, ledBstate);
 }
}
void flashLedC1() {
 if (currentMillis - prevLedCMillis >= ledCInterval) {
   prevLedCMillis += ledCInterval;
   ledCstate = ! ledCstate;
   if (ledCstate == HIGH) {
     ledCInterval = ledOnMillis;
   }
   else {
     ledCInterval = ledCbaseInterval;
      
   }
   digitalWrite(ledCpin, ledCstate);

}
}





void checkButtons() {

}

void setFlashPeriod() {

}

void askForUserInput() {

}

void getUserResponse() {

}

void moveServo() {

      // this is similar to the servo sweep example except that it uses millis() rather than delay()
 
        // nothing happens unless the interval has expired
        // the value of currentMillis was set in loop()
    
    if (currentMillis - previousServoMillis >= servoInterval) {
          // its time for another move
      previousServoMillis += servoInterval;
      
      servoPosition = servoPosition + servoDegrees; // servoDegrees might be negative
  
      if (servoPosition <= servoMinDegrees) {
            // when the servo gets to its minimum position change the interval to change the speed
         if (servoInterval == servoSlowInterval) {
           servoInterval = servoFastInterval;
         }
         else {
          servoInterval = servoSlowInterval;
         }
      }
  
      if ((servoPosition >= servoMaxDegrees) || (servoPosition <= servoMinDegrees))  {
            // if the servo is at either extreme change the sign of the degrees to make it move the other way
        servoDegrees = - servoDegrees; // reverse direction
            // and update the position to ensure it is within range
        servoPosition = servoPosition + servoDegrees; 
      }
      
          // make the servo move to the next position
      myServo.write(servoPosition);
          // and record the time when the move happened
  
    }
}
for (i=1; i==3; i++){
        moveServo();
    };

for loops run while the middle statement returns true. For what you wrote, that is never. If you want to run the loop 3 times:

for (int i=0; i<3; i++){
        moveServo();
    };

check the sticky on this board for an example of doing several things at once.[/code]

Which should have worked?

No, the centre expression is effectively the condition for a “while” loop.

So, what you have is equivalent to

int i = 1;
while (i == 3) {
  ...do something
  i++;
}

now do you see the problem?

roweng:
You can see in the void loop() function i tried

for (i=1; i==3; i++){
moveServo();
};

Using a FOR loop is completely against the spirit of my code because it blocks the Arduino from doing other things.

A better way to approach your requirement is to have a variable that keeps track of whether the servos are finished - let's call it servosMoving. You also need a variable to count the number of moves.

However ... the way my program is written all the the functions (including the moveServo() function) need to be called many times before their action is complete so determining when all the actions are done and the sequence should repeat depends on whichever function takes the longest to complete.

Also, because each function needs to be called many time (dozens or hundreds of times) calling moveServo() 3 times won't achieve anything. You need to increment the move counter after the move is complete, not when moveServo() is called.

It will be easier to help if you describe the project you want to implement. The program in my Tutorial may not be the ideal starting point even though the individual techniques are probably relevant.

...R

OK, I get it TimMJN and heMemberFormerlyKnownAsAWOL I see the error in == instead of <. Thanks. Now Robin2 has alluded to a bigger problem which he might be able to help with in my real program once i get the structure sorted.

Robin2,

Firstly, thanks for your tute code- its been fun learning it and modifying!

My ultimate project architecture would look something like this for a model rocket controller with thrust vectoring:

  1. ON. Flash 3x LEDs to show started, read MPU 9250 and Barometer Initialize SD writer and start recording

  2. Cycle servos through a test pattern, flash a LED during this process

  3. Wait for a button press

  4. Start IMU loop (will be PID controlled eventually, but for now, just straight 2x servo control- based on Kris Winer code)

  5. While in IMU loop, :

  6. check for x, y, z accelerations and Pressure readings. If delta accel_x reaches a minimum level, trigger Relay to deploy parachute,

  7. When pressure reading= initial pressure reading and, X_accel =0, (landed), stop IMU Loop, recenter servos
    then, flash lights, record for another x seconds, stop recording.

So i want to get the basic structure right, then add functionalty fia individual functions that i will learn and research over time. But i have to get the architecture right! I think the millis() approach is right, but the rest im not to sure! WOuld love help to get it right. Functions make sense to me, so i like that approach, or somethign like it.

I already have sub elements like the IMU code running, now need to stitch it all togerther!

roweng:
My ultimate project architecture would look something like this for a model rocket controller with thrust vectoring:

  1. ON. Flash 3x LEDs to show started, read MPU 9250 and Barometer Initialize SD writer and start recording

  2. Cycle servos through a test pattern, flash a LED during this process

  3. Wait for a button press

  4. Start IMU loop (will be PID controlled eventually, but for now, just straight 2x servo control- based on Kris Winer code)

  5. While in IMU loop, :

  6. check for x, y, z accelerations and Pressure readings. If delta accel_x reaches a minimum level, trigger Relay to deploy parachute,

  7. When pressure reading= initial pressure reading and, X_accel =0, (landed), stop IMU Loop, recenter servos
    then, flash lights, record for another x seconds, stop recording.

My Tutorial program is not a good starting point for that.

Parts 1,2 and 3 should probably go in setup() and what you refer to as the IMU loop should just be the loop() function

For the initial test patterns in setup() it would be perfectly OK to write blocking code as nothing else needs to happen and when the tests are complete they won't be needed again.

...R

Sounds fair then. So i can pretty much do everthing in loop() using normal if then type approach? What about timings? Should i stick to millis() or is DELAY ok?

roweng:
Should i stick to millis() or is DELAY ok?

Best to use millis() because if you later find that delay() is causing a problem it will be a lot of work to change from delay() to millis() - almost a complete re-write.

The code that you put in loop() should almost certainly follow the concepts in the functions in my Tutorial - i.e. no blocking code.

...R

OK Thanks. Ill plan out my code structure and post it up later. If i can get the structure right, then its just a case of adding functionality as i get the previous bit working.

Robin2, all,

Please see my revised structure below. How does that look? I have a couple of questions for the loop functions, and how i only trigger the others when a condition is met? see comments in code. Any help greatly appreciated.

excerpt:

void loop() {

  
  currentMillis = millis();
  armforflight(); // check that the switch is closed, then allow other functions to run. how to do this?
  armTVC_Parachutecct(); // arm parachute eject above 10m alt
  IMU_servocontrol(); // my modified Winer code with 2 servos. Eventually replace with PID closed loop
  SDCard_write(); // write time, alt, delta_alt, ax, ay, az, gx, gy, gz, etc to card)
  Landed_shutdown();  // turn off tvc ( imu servocontrol cct), wait 20 sec, close SD card write

}

also, opening the sd card in setup(). will it still run in loop()?

// TVC for Model Rocket V1
// Put acknowledgements in here!
//=======Include Libraries===============
//libraries here

//=============Servo setup===============
declare servo pins, initial state
Servo servox; //xaxis servo
Servo servoy;  //y axis servo

//=============LED Setup=================
const byte PowerLED = 9;
const byte SDcardLED = 10;
const byte BarometerLED = 11;
const byte ServoLED = 12;


//=============SD CARD Setup=============
data (time, alt, pressure, delta_alt, ax, ay, az, gx, gy, gz, arm pin,  parachute pin); // check this, but define objects to write to sd card)
  

//=============Pressure sensor Setup=====

//from pressure sensor used

//============= MPU Setup===============

//from modified winer code


void setup() {
  
  // ===Initialise LED's====
  digitalWrite(PowerLED, HIGH);
  digitalWrite(SDcardLED, HIGH);
  digitalWrite(BarometerLED, HIGH);
  digitalWrite(ServoLED, HIGH);


  //====Start SD CArd and open new file====
    // initialise and open sd card file
    //Write(time, altitude, ax,ay,az gx,gy,gz) to card
     //*** will this stay open when prog switched to loop()?***
     //Flash SDCARD LED
     
  // ===Initialise Barometer====
    //start flashing baro LED
    //Initialise BARO
     // Record Start pressure ( average over 20 secs)
    
  //====Servo Sweep====
    //Start flashing Servo LED
    //Set sweep limits
    //Sweep servos to all extents
      //return to centre position
    //Stop flashing servo LED

  
    

}

void loop() {

  
  currentMillis = millis();
  armforflight(); // check that the switch is closed, then allow other functions to run. how to do this?
  armTVC_Parachutecct(); // arm parachute eject above 10m alt
  IMU_servocontrol(); // my modified Winer code with 2 servos. Eventually replace with PID closed loop
  SDCard_write(); // write time, alt, delta_alt, ax, ay, az, gx, gy, gz, etc to card)
  Landed_shutdown();  // turn off tvc ( imu servocontrol cct), wait 20 sec, close SD card write

}

void armforflight(){
  //====Arm TVC and Parachute cct====

// insert code for latching switch
    if (pressureswitch = HIGH){
      //blink servo and baro LEDs
      // allow armTVC_Parachutecct(), IMU_servocontrol(), Landed_shutdown()
      }
    else (flash power led?) // some command to a LED to show not armed

      // dont let other loops run.  How do i do this?

}
    
void armTVC_Parachutecct{
  // arm parachute eject above 10m alt, eject when delta altitude <=0
  first, calculate altitude from pressure card (existing code)

  
  if (altitude >= 10) {    
    delta_alt = (currentalt-previous alt);
    if (delta_alt <= 0) {
      digitalwrite(parachutepin, HIGH);
    }
    else digitalwrite(parachutepin, LOW);
  }
}; 
  
void IMU_servocontrol{
  // my modified Winer code with 2 servos. Eventually replace with PID closed loop control
}; 

void  SDCard_write{
   calculate pressure using pressure card code
    // write time, alt, pressure, delta_alt, ax, ay, az, gx, gy, gz, arm pin,  parachute pin, etc to SD card)
    dataFile.write(data );  // need to define 'data'!
    
}; 
  
  Landed_shutdown{    
  // turn off tvc ( imu servocontrol cct), wait 20 sec, close SD card file

  if (delta_aly <=0) and (ax <=0){
    // code to recenter servo
    digitalWrite(servoxpin, LOW);
    digitalWrite(servoypin, LOW);
    // use millis code to pause 20 seconds
    SD.close("data.txt"); // using sd card code   
    }
  
};

roweng:
Please see my revised structure below. How does that look?

I think that approach should work.

If it was my project I think I would prefer to have a function called (say) updateSystemState() which would check for

  • armforflight;
  • armTVC_Parachutecct;
  • Landed_shutdown;

and update a variable perhaps called systemState. For example systemState could be 'F' for flight. 'P' for parachute and 'L' for landed. And then the functions to implement those actions could check systemState to decide whether it is their turn to act.

also, opening the sd card in setup(). will it still run in loop()?

That should be fine provided the SD Card instance is a global variable

...R

Brilliant-Thanks! I like the system state function idea. I might try that as well. So if i understand right the loop() and functions would look something like:

( am i calling up the functions correctly?

void loop() {

  
  currentMillis = millis();

  Updatesystemstate(); // determine which flight segment is active
  armforflight(); // check that the switch is closed
  armTVC_Parachutecct(); // arm parachute eject above 10m alt
  Landed_shutdown();  // turn off tvc ( imu servocontrol cct), wait 20 sec, close SD card write

}

void  Updatesystemstate{
  for (pressureswitch = HIGH){   // this is the latching arming switch
      systemstate = Flight;
      }
  for (Altitude >= 10) and (delta_ax <= 0 ) {   // reach min altitude and have acceleration in x direction
      systemstate = Parachutearmed;
      }
  for (altitude <= initialaltitude) and (deltaax = 0){   // detecting that rocket has landed
      systemstate = landed;
      }
  };

void armforflight{
  if (sytstemstate = flight){
      //blink servo and baro LEDs
      // allow armTVC_Parachutecct(), IMU_servocontrol()
     } 
  };

void armTVC_Parachutecct{
  if (syttemstate = Parachutearmed) {
    // arm parachute eject above 10m alt, eject when delta altitude <=0
    first, calculate altitude from pressure card (existing code)
    if (altitude >= 10) {    
      delta_alt = (currentalt-previous alt);
      
       if (delta_alt <= 0) {
          digitalwrite(parachutepin, HIGH);
    }
    else digitalwrite(parachutepin, LOW);  // dont really need this- more a safety thing
  }
};

etc etc etc...

If this is right, then im off to do a lot of coding! Thanks for your inputs

I think you mean "if" not "for", and the ='s in there should then be =='s.

[color=red]for [/color](pressureswitch [color=red]= [/color]HIGH){
[color=limegreen]if[/color] (pressureswitch [color=limegreen]==[/color] HIGH){

roweng:
If this is right, then im off to do a lot of coding! Thanks for your inputs

As well as what @finola_marsaili has said you need to post the whole program so we can see how the variables are defined.

It looks as if you are using the String class for sytstemstate and it is not a good idea to use the String (capital S) class on an Arduino as it can cause memory corruption in the small memory on an Arduino. This can happen after the program has been running perfectly for some time. Just use cstrings - char arrays terminated with '\0' (NULL).

In the example I gave earlier I was just using a single char for systemState - which will be a lot faster as well as avoiding other problems. If you prefer spell out the state names then I suggest you use an ENUM

Get into the habit of {A} developing programs in very small steps and {B} testing the code before asking a follow-on question. I tend to test my programs after every 5 or 6 lines of new code.

...R

Get into the habit of {A} developing programs in very small steps and {B} testing the code before asking a follow-on question. I tend to test my programs after every 5 or 6 lines of new code.

Great advice. I have been playing around with the sub elements of the program in small sketches to understand how the elements work ( MPU9250 controlling servos, flashing leds etc) and am now ready to tackle the big code. I want to get the architecture right so i dont have massive rewrites ( which was the origional post). I am now ready to get this structure working, which i will do by printing messages to serial monitor and flashing LEDs at the appropriate times. Then my plan is to populate with the project specific code, sort out all the names, variables etc and tackle the next problems! It will take some time, so ill post again when i’ve made more progress.

Cheers!

Alright, great progress!!
All my setup stuff works beautifully.
Quite proud of the time loop to get milis loo to work in the setup area. Im sure you could use millis to do what i did using unit32_t loop, but i dont know how…

Anyway, moving forward to Loop().
Everything is working and the first function updatesystemstate() also works correctly for the button pressing. The correct test LED’s blink when the button is pushed.

However, now the system state for armforflight() function is not.

I’m expecting armforflight() to read systemstate == F (flight) when the button is pushed, and the barometerLED to go LOW, which it does. However when the systemstate is low or not equal to F, armforflight should read nothing and therefore the barometer LED should go HIGH. What am i missing?

I even tried to create a state called A. I think is not overwriting the 'F" state in memory?

All help appreciated!

TVC_V1_Public.ino (7.77 KB)

Please include your program in a Post we don’t have to download it. You were doing that correctly earlier.

…R

All your mode variables are the same:

int A = 0; // Empty mode
int F; // Flight mode
int P; // Armed Parachute mode
int L; // LAnded shutdown mode

Every one of them is zero.

Sorry Robin, it kept saying i had exceeded 9000 characters and wouldnt post, even though the post was only 7400 or so. Had to revert to file upload to get round it. no idea why