Blink without delay Problem

I have written a program similar to blink without delay tutorial but I am having a hard time getting it to do what I want it to do. I would like it to simply receive an input from the serial monitor and if it is "1" turn on the pin 13 LED for 1 minute. After the 1 minute cycle I would like it to turn the LED off and wait to receive another command. I must have missed something because the LED just won't shut off.

What am I missing, why won't it shut off when the "diff" is greater than 1 minute?

Thank you for your help
Jake

PS all the serial print stuff what an attempt of me figuring out what is going wrong. It didn't work.

//Created by Jacob


int Led = 13;
unsigned long previousMillis = 0;

void setup() {
  pinMode(Led, OUTPUT);
  Serial.begin(9600);
}

void loop() {
  while (Serial.available() == 0);  //wait for an inputed value.
    int val = Serial.read()-'0';
  Serial.print(val);                //just to make sure send it back to me in the serial monitor
  Serial.println(" inputed value"); 

  unsigned long currentMillis = millis();  //my running timer I think
  
  Serial.print(currentMillis);            //tell me what it's current value is
  Serial.println(" currentMillis");
  Serial.print(previousMillis);           //tell me what previousMillis currently is
  Serial.println(" previousMillis");
  delay(1000);
  if (val == 1){                          //Okay if the value inputed happens to be one lets do something
    digitalWrite(Led, HIGH);              //
    unsigned long milsval=val*60.*1000.;  //convert value to minutes
    Serial.print(milsval);
    Serial.println(" inputed value in milsec");
    unsigned long diff = currentMillis - previousMillis;    //the prupose of these serial.prints is for me to keep track of when it should enter the next if statement
    Serial.print(diff);
    Serial.println(" difference");
    delay(1000);                        //so as to not recieve too much data
    if (currentMillis - previousMillis > milsval) {  
      digitalWrite(Led, LOW);          //once the time interval has passed shut off the LED
      previousMillis = currentMillis;  //set them equal to the whole time difference aplicable
      val = 0;                         //set the value to 0 to get me out of this whole if statement started on line 25
      
    }
  }   
}

You still have delays in that code, and you keep resetting previousMillis to equal currentMillis so it's never going to differ by a whole minute. You need to capture the time when the button was pressed but don't update that time unless the button gets another valid press.

jasmine2501:
You still have delays in that code, and you keep resetting previousMillis to equal currentMillis so it's never going to differ by a whole minute. You need to capture the time when the button was pressed but don't update that time unless the button gets another valid press.

Okay are mentioned I got rid of my delays and I moved where I put previousMillis = currentMillis in hope that it would only set themselves equal when there was an input into the serial port. I don't think that that is the right place for it because it still doesn't work.

//Created by Jacob


int Led = 13;
unsigned long previousMillis = 0;
unsigned long currentMillis = millis();

void setup() {
  pinMode(Led, OUTPUT);
  Serial.begin(9600);
}

void loop() {
  while (Serial.available() == 0);  //wait for an inputed value.
    previousMillis = currentMillis; 
    int val = Serial.read()-'0';
  Serial.print(val);                //just to make sure send it back to me in the serial monitor
  Serial.println(" inputed value"); 


  if (val == 1){                          //Okay if the value inputed happens to be one lets do something
    digitalWrite(Led, HIGH);              //
    unsigned long milsval=val*60.*1000.;  //convert value to minutes
    unsigned long diff = currentMillis - previousMillis;    //the prupose of these serial.prints is for me to keep track of when it should enter the next if statement

    if (currentMillis - previousMillis > milsval) {  
      digitalWrite(Led, LOW);          //once the time interval has passed shut off the LED
      val = 0;                         //set the value to 0 to get me out of this whole if statement started on line 25
      
    }
  }   
}

Well you have other issues in the code too. One I see right away is this:

while (Serial.available() == 0);  //wait for an inputed value.

Which does what it says. It waits for input from the serial port. Your code stops there until something is entered.

jasmine2501 is right, you have to look at it in another way.

Let the loop() function run over and over again. Check the serial input, if something is available, read it and process it. If nothing is available, just let loop() run over and over again. Check the timing in the loop().

There are more than one ways to do the timing:
You remember the previousMillis and compare it with the currentMillis.
You could also set a newMillis in the future, that would be the time that the led has to be turned off.
It is not better, but if you think that is easier to understand, you could make it like that.

...
currentMillis = millis();
...
newMillis = currentMillis + (60UL * 1000UL);
...
if( currentMillis >= newMillis )
...

This line could be written better, by making everything unsigned long:

// unsigned long milsval=val*60.*1000.;
unsigned long milsval = (unsigned long) val * 60UL * 1000UL;

Caltoa:
jasmine2501 is right, you have to look at it in another way.

Let the loop() function run over and over again. Check the serial input, if something is available, read it and process it. If nothing is available, just let loop() run over and over again. Check the timing in the loop().

There are more than one ways to do the timing:
You remember the previousMillis and compare it with the currentMillis.

Why use the variable currentMillis when millis() is almost the same thing, but more accurate?

Using millis() with every calculation and with every compare, could result in something unpredictable.

When the current value of millis() is copied into variable 'currentMillis', that value stays the same.
When only millis() is used, it could change during the code. You have to check every line of code to see if that is not a problem.

Caltoa:
jasmine2501 is right, you have to look at it in another way.

Let the loop() function run over and over again. Check the serial input, if something is available, read it and process it. If nothing is available, just let loop() run over and over again. Check the timing in the loop().

There are more than one ways to do the timing:
You remember the previousMillis and compare it with the currentMillis.
You could also set a newMillis in the future, that would be the time that the led has to be turned off.
It is not better, but if you think that is easier to understand, you could make it like that.

Okay I like the idea of the whole newMillis thing. It seams to make more sense to me. What I think I don't understand is exactly how the loops are running. Once the val == 1 doesn't it stay inside that if statement. Thanks for your help. Here is my latest code.

//Created by Jacob


int Led = 13;
unsigned long previousMillis = 0;
unsigned long currentMillis = millis();

void setup() {
  pinMode(Led, OUTPUT);
  Serial.begin(9600);
}

void loop() {
  while (Serial.available() == 0);  //wait for an inputed value.
    previousMillis = currentMillis; 
    int val = Serial.read()-'0';
  Serial.print(val);                //just to make sure send it back to me in the serial monitor
  Serial.println(" inputed value"); 


  if (val == 1){                          //Okay if the value inputed happens to be one lets do something
    digitalWrite(Led, HIGH);              //
    unsigned long milsval=val*60.*1000.;  //convert value to minutes
    unsigned long newMillis = currentMillis + milsval;    //the prupose of these serial.prints is for me to keep track of when it should enter the next if statement

    if (newMills >= currentMillis) {  
      digitalWrite(Led, LOW);          //once the time interval has passed shut off the LED
      val = 0;                         //set the value to 0 to get me out of this whole if statement started on line 25
      
    }
  }   
}

Once the val == 1 doesn't it stay inside that if statement.

It executes the stuff between the { } for that "if" then carries on . . .

Not staying inside an if is what you want. Let the loop() run over and over again. Process the data while the loop() keeps running over and over again. If it was only for a single task, you could wait before doing the next action. But you have already two things going, the serial data and the leds with timing. So it is best to let the loop() run over and over again, and check every time if something needs to be done.

You missed the comment about the while that is waiting.

while (Serial.available() == 0);

The ';' actually is an empty statement and closes the 'while'. If nothing is available, the 'while' is run repeatedly and the next lines are never executed.

// this is your code
while (Serial.available() == 0);

// this is the same
while (Serial.available() == 0)
  ;

// this is also the same
while (Serial.available() == 0)
  // do something or nothing here.
  ;

It should be something like this:

if( Serial.available() == 0)
{
  // nothing available
}
else
{
  // availabe was not zero, something available.
}

I should have posted this to begin with... make some functions so your program is easier to follow...

You need to make everything in the loop finish quickly so you can run the loop again.

I think I finally am starting to understand a little bit! I input a number between 0 and 9 and the led stays on for that amount of time! "as desired!" You all are great that video helped a lot. I will deffiently save that one for if I ever find someone like me with my same problem.

Next task at hand, if you all are up for it. Inputting 1-9 was a baby step to being able to input something like "ledon12.50" basically turn on the LED for 12.50 seconds. I think I could do it by with a ton of if statements like if l..... if e..... if d...... if o..... but that seams crazy long. If I wanted to cover every time between 00.01-99.99 that would take like 9999 if statements. There must be a better way. (FYI eventually my Serial input will be a text message from a gsm shield, I don't know if that changes anything) Please point me in the right direction I will do my best to figure it out.

Also thanks for being so willing to help. It amazes me that some poor beginner like me now days can ask a question and have so many nice people be willing to help.

This is what I got from before.

//Created by Jacob


int Led = 13;
unsigned long newMillis =0;


void setup() {
  pinMode(Led, OUTPUT);
  Serial.begin(9600);
}

void loop() {
  unsigned long currentMillis = millis();
  if (Serial.available() == 0){
     if (currentMillis >= newMillis ){                         
      digitalWrite(Led, LOW);       
    }
    
  }
  else {
    int val = Serial.read()-'0';
    Serial.print(val);                
    Serial.println(" inputed value"); 
    newMillis = currentMillis + val*1UL*1000UL;
    digitalWrite(Led, HIGH); 
   
  }   
}

This will show you how to receive and buffer a string of serial input. http://gammon.com.au/serial
Once you have the complete string you can parse it to extract commands and respond to them.

Hey thanks for your input. I stared at your example for a long time and it was very useful. Seeing how we are now off topic if i run into unsurpassable problems i will start a new topic.

Thanks again

jacob84401:
Hey thanks for your input. I stared at your example for a long time and it was very useful. Seeing how we are now off topic if i run into unsurpassable problems i will start a new topic.

Thanks again

It's ok to continue here, you have some people watching this. You lose track of us, and we lose track of what you're doing, when you create a new thread. Keep that in mind - in a new thread, I won't remember you. BTW, one big reason I made that video is because it helps me learn it too - I've actually watched it 3 or 4 times. Teaching and reviewing is the best way to learn something.

I am having issues and I need more help. I would like to input on the serial monitor a number (greater than one digit) and have the LED on pin 13 light up for that number of seconds.

I can't get my loops to work together. I expect that I should be able to enter "30" and have it output "30 imputed value" and turn on the LED for 30 seconds. Sadly when I put in 30 I don't even get an output.

This is what I think my code should be doing

void loop
stick in here most the time and keep on updating the currentMillis

check to see if it should turn of LED (assuming it was on in the first place)

If there is Serial available it should jump to the processIncomingByte loop and place the Serial.read() value in for the inByte
varriable (this is how I believe that this should work but I really don't know)

processIncomingByte loop
set input_line to a certain character length=[MAX_INPUT] I think that this is how you declare an array with that many spots. (also not too sure if that is what this line is)

declare a variable to track where to keep the information in the array.

create a switch case who is a function of inByte

if \n it is an end of a line so jump to process_data loop but why are we setting the input_line array to 0 before doing that? and why do we input_pos = 0 after that if it will jump before that. (Am I right about that whole jump thing?)

if '\r' it is a return ignore it

default if there is room left in the array write the byte into the array and see what (I guess input_pos++ will increase by one before writing each time.

void process_data (const char * data)

take data and print it

take data and try and convert to seconds but it DOESN'T work because it is a character and I don't know how to convert it back to a number. Hence why it is commented out of not I get this error Arduino: 1.5.5 (Windows 7), Board: "Arduino Uno"

REV_E_TIMER_ino.ino: In function 'void process_data(const char*)':
REV_E_TIMER_ino:27: error: invalid operands of types 'const char*' and 'long unsigned int' to binary 'operator*'

Turn on the LED

Please help I am sorry that it is so messed up.

I am trying to base what I am doing of the example mentioned a few posts above.

//Created by Jacob


int Led = 13;
unsigned long newMillis =0;
const unsigned int MAX_INPUT = 50;
unsigned long currentMillis;


void setup() {
  pinMode(Led, OUTPUT);
  Serial.begin(9600);
}
void loop(){
  currentMillis = millis();
  if (Serial.available() > 0){
    processIncomingByte (Serial.read ());
  }
  if (currentMillis>=newMillis){
    digitalWrite(Led, LOW);
  }
}
void process_data (const char * data)
  {
  Serial.print(data);                
  Serial.println(" imputed value");
  newMillis = currentMillis + data*1000UL;
  digitalWrite(Led, HIGH);
  }
void processIncomingByte (const byte inByte) {
  static char input_line [MAX_INPUT];
  static unsigned int input_pos;
   
    switch (inByte)
      {
        case '\n':
          input_line [input_pos] = 0;
          process_data (input_line);
          input_pos = 0;
          break;
        case '\r':
          break;
        default:
          if (input_pos < (MAX_INPUT - 1))
            input_line [input_pos++] = inByte;
          break;
      }
}

Suppose the const char data pointer, passed to process_data, points to "elephant". Please explain how you can multiply "elephant" by 1000. Now, suppose that it points to "1000". Again, please explain how you can multiply a string by 1000.

PaulS:
Suppose the const char data pointer, passed to process_data, points to "elephant". Please explain how you can multiply "elephant" by 1000. Now, suppose that it points to "1000". Again, please explain how you can multiply a string by 1000.

easy elephant*1000= one thousand elephants! yes I realize this and I have tried to convert the character "data" to an integer but I must be missing something here. Here is what I tried and the error I got.

void process_data (const char * data)
  {
  Serial.print(data);                
  Serial.println(" imputed value");
  int long val=data-'0';
  newMillis = currentMillis + val*1000UL;
  digitalWrite(Led, HIGH);
  }

Arduino: 1.5.5 (Windows 7), Board: "Arduino Uno"

REV_E_TIMER_ino.ino: In function 'void process_data(const char*)':
REV_E_TIMER_ino:27: error: invalid conversion from 'const char*' to 'long int'

The other problem at hand is that the code won't even reach this point because if it did it should print something on the serial monitor Serial.print(data)

data is not a character. You can't subtract a character from an array.

Look at the atoi() function.

PaulS:
data is not a character. You can't subtract a character from an array.

Look at the atoi() function.

Okay if I understand atoi() converts an char* to a int. In my case atol() might be more applicable because I am using val in arithmetic with unsigned long values. Either way I do it I can't see if it works because it would appear that my code never even makes it this far. Thank you for your help. I figured that -'0' wouldn't work I just didn't know any better.

This what I put in to fix that.

void process_data (const char * data)
  {
  Serial.print(data);                
  Serial.println(" imputed value");
  int val=atol(data);
  newMillis = currentMillis + val*1000UL;
  digitalWrite(Led, HIGH);
  }