You're not following the State Machine model outlined by @PaulMurrayCbr in Reply #14.
while(!S1); // wait for S1 to close
S1 contains the number of the pin, NOT the state of the pin. The NUMBER is never going to be false, because it never changes.
You need to actually read the state of the pin. There is this useful function for reading a digital pin...
Further to what PaulS has pointed out, if you want to test S1 you need to test the value of it which can be done in a few different ways. The Q&D is with bitRead(x,bitn) understanding that you need to reference the PINx register and not the PORTx register (which is output).
okasional:
And to DKWatson, I still don't understand the "adding to millis()" problem. Suppose I start the 100 second timer with a variable Endtime = millis() + 100000, and every iteration of the loop includes a test to see if millis() > Endtime. There is no rollover to worry about. Sooner or later millis() will be greater than Endtime and I will see that the next time through the loop. I don't have to spot the exact moment Endtime is exceeded.
No dude.
Let's say long integers roll over at one million.
Lets say you need a delay of one thousand.
Let's say the event timer happens at tick 999500.
When you add 1000 to 999500, because of the long integer rollover this will wind up having a value of 500. The timer at 999500 is already greater than 500, and so the code will emmediately conclude that the delay has already elapsed.
Now, this happens at 49 days into the runtime. Your code will run perfectly fine until then. At 49 days, well - it won't. How catastrophic this is depends on exactly what you have hooked up to your arduino and the logic of the rest of your code.
okasional:
It compiles with no errors, but nothing happens when when I close switch S1.while (millis() - startTime) >= (interval)&&(digitalRead(S1) = HIGH); // wait for S1 closure
delay(1); // but not longer than 5 seconds
Well, what you have here won't compile without errors, so there's something haywire with your saying "this compiles without errors".
Problem 1 - the syntax is screwed up. A while statement takes a single expression in parenthesis. This means that the compiler will assume that a statement begins at your >=, which is just plain unparseable.
Problem 2 - you want to wait while millis()-startTime (the elapsed time) is LESS THAN the interval.
Problem 3 - You use an assignment = rather than a comparison == . Again, this won't compile unless digtialread() compiles down to an lvalue, which I'm pretty sure it doesn't. If it did, the consequence would be that the code would conclude that the switch is always HIGH (because that would be the result of the assignment).
Problem 4 - you have a semicolon at the end of the while. This will be treated as a "do nothing" statement, and the delay won't get executed. This won't actually affect the operation of this loop, but it's still not what you intended. The wile won't delay - it will just loop around itself until the condition is met, and then there will be a 1ms delay after it exits.
Try
// wait for S1 closure but not longer than 5 seconds
while ((millis() - startTime < interval) && (digitalRead(S1) == HIGH))
delay(1);
Another thing you need to be wary of:
while(!S1); // wait for S1 to close
delay(1); //
forget the test, others have already commented wisely, but because of the semi-colon after the while, you'll never get out of this loop. The delay(1) needs to be an executable instruction inside the while loop. As it sits its just another statement that the process will fall through. So, try
while(!(valueof)S1) delay(1);
Remember, when you test the value of a register bit, it takes about 125 nanoseconds and the processor doesn't wait around for valid data. Be aware of the time these things take. Sometimes we must wait.
If your interested, I used a downloaded sketch to do some timing, copied and pasted the output into a picture box in visual basic and created a little pop-up I can refer to any time I need to remember the timing bits. Attached a copy.
UnoTiming.zip (105 KB)
The point by DKWatson about delay(1) being outside of the while loop is valid, but...
What is (valueof) ?
Shouldn't (valueof)S1 be digitalRead(S1) ?
Furthermore, I do not agree about the need for the delay(1). This is attempting to save nanoseconds (or perhaps costing a millisecond) recognizing that a human pushed a switch, but costing precious bytes.
I think it was agreed that testing S1 was not going to work as S1=0, the pin number. So he wants to test the 'value of' S1, and sure it should be as you suggested, but in the spirit of the forum, I was offering ideas as opposed to code.
As for using the delay, your choice. I prefer a microsecond as I suggested early on. I have code that won't work without it.
I'm completely hung up on doing timing stuff using millis instead of delay. I've tried multiple times to write a sketch following Gammon's recommended example of subtracting. Here's an example:
// Test Sketch 1.1 19 Sept 2017 okasional
const byte S1 = 0; // control switch connected from pin #PB0 to ground
const byte LED1 = 1; // blue LED connected via resistor to pin #PB1
const byte LED2 = 2; // red LED connected via resistor to pin #PB2
void setup()
{
pinMode(S1, INPUT_PULLUP);
pinMode(LED1, OUTPUT);
pinMode(LED2, OUTPUT);
}
void loop()
{
unsigned long startTime = millis ();
unsigned long interval = 5000;
if (millis () - startTime >= interval)
digitalWrite (LED1, HIGH); // do something
delay(500); // do something
digitalWrite (LED1, LOW); // do something
}
This sketch compiles and uploads with both LEDs off, and then doesn't do ANYTHING. Clearly this is an attempt to blink an LED one (or more?) times while a 5-second timer is running. I used an "if" rather than a "while" because that's the way Gammon did it in his millis tutorial. I understand the difference between the two, but not the pro's and con's. To add to the confusion, if I change the code to wait while millis is LESS THAN the interval, as suggested by PaulMurrayCbr, at the end of upload LED1 turns on and stays on forever. Can someone tell me what's going on, or just show me a workable loop.
okasional
There are at least three problems:
(1)
This line
unsigned long startTime = millis ();
gets executed every time that loop() gets called, which can be thousands of times per second. You probably do not want startTime reinitialized quite that frequently. Remove that line.
I suggest that
unsigned long startTime ;
should be in the global area above the setup() function, and that
startTime = millis ();
should be in the setup() function.
(2)
This code
if (millis () - startTime >= interval)
digitalWrite (LED1, HIGH); // do something
delay(500); // do something
digitalWrite (LED1, LOW); // do something
does not act the way that you might expect. The compiler ignores white space and indentations. You have to use curly brackets like this:
if (millis () - startTime >= interval)
{
digitalWrite (LED1, HIGH); // do something
delay(500); // do something
digitalWrite (LED1, LOW); // do something
}
to avoid having just the first digitalWrite(...) controlled by the 'if' statement.
There are different camps on exactly where (on what line) to place the brackets, but it is the sequence that is important.
(3)
startTime never gets updated. You need something like this:
if (millis () - startTime >= interval)
{
digitalWrite (LED1, HIGH); // do something
delay(500); // do something
digitalWrite (LED1, LOW); // do something
startTime = millis() ;
}
Some would say that the line should be
startTime += interval ;
For this application, there is little practical difference.
Also, in the future, to post code and/or error messages:
- Use CTRL-T in the Arduino IDE to autoformat your complete code.
- Paste the complete autoformatted code between code tags (the </> button)
so that we can easily see and deal with your code. - Paste the complete error message between code tags (the </> button)
so that we can easily see and deal with your messages. - If you already posted without code tags, you may add the code tags by
editing your post. Do not change your existing posts in any other way.
You may make additional posts as needed. - Please provide links to any libraries that are used
(look for statements in your code that look like #include ). Many libraries
are named the same but have different contents.
Before posting again, you should read the three locked topics at the top of the Programming Questions forum, and any links to which these posts point.
If your project involves wiring, please provide a schematic and/or a wiring diagram and/or a clear photograph of the wiring.
Good Luck!
Agree with all of the above (except for using delay() to time the blink period). AND: Go into the IDE and click on File --> Examples --> 02.Digital --> BlinkWithoutDelay
That example program does exactly what you're attempting to do.
Many thanks to vaj4088. I incorporated your suggestions. The only change I had to make was to add another closing bracket. One compile error message I really like is the one that guides you when you don't have the brackets balanced. I tested it for both 5 second and 10 second timing and it works exactly as expected.
// Test Sketch 1.2 20 Sept 2017 okasional
const byte S1 = 0; // control switch connected from pin #PB0 to ground
const byte LED1 = 1; // blue LED connected via resistor to pin #PB1
const byte LED2 = 2; // red LED connected via resistor to pin #PB2
unsigned long startTime;
void setup()
{
pinMode(S1, INPUT_PULLUP);
pinMode(LED1, OUTPUT);
pinMode(LED2, OUTPUT);
startTime = millis ();
}
void loop()
{
unsigned long interval = 5000;
if (millis () - startTime >= interval)
{
digitalWrite (LED1, HIGH); // do something
delay(500); // do something
digitalWrite (LED1, LOW); // do something
//delay(500); // do something
startTime = millis ();
}
}
I read the instructions on all about posting, and spent two hours trying to insert a small schematic into the text so I could see it in the preview. I can choose the file but no way to insert it. The only image insertion seems to be a URL. So I'll go ahead and post this. Glad to attach a schematic if someone will tell me how.
Now to digest into gfvalvo's comment and try to do the blink without delay.
okasional
I am glad that you got it working, although I would prefer (as others have commented) that you implement the state machine and get rid of the delay(...).
Parentheses must match. Square brackets must match. Curly brackets must match. If the "...only change I had to make was to add another closing bracket" then (because the brackets matched in the posted code) either an opening bracket got dropped in the posting, or you forgot that you added an opening bracket.
If you replace this
{
digitalWrite (LED1, HIGH); // do something
delay(500); // do something
digitalWrite (LED1, LOW); // do something
//delay(500); // do something
startTime = millis ();
}
with this
{
digitalWrite (LED1, (digitalRead(LED1)==HIGH?LOW:HIGH)); // Put the LED in the opposite state.
}
you will have eliminated the delay(...), although this does not blink the same and does not implement the state machine. Perhaps you want to predict its behavior?
Explanation: a?b:c evaluates 'a' and if it is true, then results in 'b', otherwise results in 'c'. Thus
(digitalRead(LED1)==HIGH?LOW:HIGH) results in LOW if digitalRead(LED1) is HIGH, otherwise
digitalRead(LED1) is LOW and the result is HIGH.
Note: Some people would simplify (digitalRead(LED1)==HIGH?LOW:HIGH) to !digitalRead(LED1) and it would work currently, but it is not documented this way and might not work in the future.
I'm working on the state machine. What's the difference between establishing the three state cases and having three functions and move from one function to the next at the appropriate time?
I wrote a simple loop to play with a 1/2 second blink to an LED if a switch closed.
void loop() {
if (digitalRead (S1) == LOW) // is switch S1 closed?
{ digitalWrite (LED1, HIGH); // yes, turn on LED2
delay(1000);
digitalWrite (LED1, LOW); // turn off LED2
}
}
Obviously, if the switch is still closed at the end of the blink time the loop whips around so fast that the LED never goes off until the switch opens. That brings up the question, is there a technique for detecting the falling edge of a switch input so that you get one, and only one, short "blink" when the switch closes, regardless of how long the closure lasts? I'm still looking for a source that shows all 200+ Arduino statements. Is there any "pulse (X)" statement that gives a brief output of X time?
okasional
Yes, there are multiple ways to do that.
I will suggest that you save yourself all the grief of getting that information, and, as a bonus a way to debounce your switch.
First, download this Library. and install it in the IDE.
Look at the click.ino example. If you want to know the instant when the switch was closed, use
if(button.rose()) instead of if(button.fell())
Jacques
jbellavance:
Yes, there are multiple ways to do that.
But, if you want to learn the very basics before moving on to new libraries and such, look in the built-in examples:
File --> Examples --> 02.Digital --> StateChangeDetection
And, AGAIN, look at BlinkWithoutDelay. Get those delays out of your blinking attempts for God's Sake !!!!!!!!!!
This sketch is supposed to show that I can REPEAT a non-delay 5-second timer that's started by a switch closure. I expected to close the switch momentarily to exit the first 'while', and then 5 seconds later the timer would time out and exit the second 'while' to set off the fireworks. Then close the switch momentarily again with the same results. Ad infinitum. As with most of my creations, it compiles and uploads without error, then just sits there on its butt and does nothing when the switch is closed.
// Restarting Millis Timer 09/21/17
const int S1 = 0; // control switch connected from pin #PB0 to ground
const int LED1 = 1; // blue LED connected via resistor to pin #PB1
const int LED2 = 2; // red LED connected via resistor to pin #PB2
unsigned long startTime = 0;
unsigned long interval = 5000;
void setup() {
pinMode(S1, INPUT_PULLUP);
pinMode(LED1, OUTPUT);
pinMode(LED2, OUTPUT);
}
void loop() {
while (digitalRead(S1) == HIGH)
{ delay(1);
}
startTime = millis();
while (millis() - startTime >= interval)
{ digitalWrite (LED1, HIGH); // |
delay(1000); // |
// > just to celebrate timer completion
digitalWrite (LED1, LOW); // |
delay (1000); // |
}
}
UPDATE
When I add a 1millisecond delay after the second 'while' I get the flashing LED, but there's no 5 second timer, it just blinks instantly 1-second on, 1-second off, repeated until S! opens.
UPDATE to the UPDATE
Just on a hunch I changed the second 'while' statement to LESS THAN instead of GREATER THAN, and the sketch seems to work perfectly. That GREATER THAN version is straight out of Gammon's paper. What goes?
You still have those delay().
What your sketch does (with the less than) is:
loop() {
- wait until the switch is pressed,
- during 5 seconds: blink with delay at 1 second interval.
}