This is a general question about best practice with digital input from switches.
I have magnetically actuated reed switch which is connected to a Arduino UNO R3 utilizing the internal pull-up resistor. There is no hardware of software de-bounce on this input.
I want to exit the "while loop" when the switch goes LOW.
In your opinion, which is better practice? Why?
while (Switch == HIGH) {
...do stuff here....
}
or
while (Switch != LOW) {
...do stuff here....
}
Thanks in advance!
Don
It makes absolutely no difference whatsoever. They are functionally identical.
Regards,
Ray L.
RayLivingston:
They are functionally identical.
I think the OP knows that, and that's actually why he asked the question.
I don't recall seeing the not-LOW option used. But it makes a certain amount of sense: if active is low (which is what you have with pullups) then if it makes logical sense to you that not-LOW is the inactive state (as opposed to HIGH which as we know means the same functionally) then go for it.
I'd hazard a guess that folk trying to help though, if you post a problem, might do a double-take on the not-low and go "What?- ah, he means high, ok...."
Readability by others is important in this stuff, remember, so if I had to choose I'd go for high, not not-low.
Like RayL says, functionally, they are the same thing.
However, like JimboZA, I personally find it easier to read the code when it is written while (Switch == HIGH).
The purist may argue that we actually want to exit the loop when the switch goes LOW, so we should write the code that way.
I am curious if experienced programmers take the "purist" route or the "easier to read the code" route. Hopefully a few others will weigh in with their thoughts.
Don
Keep in mind that while(condition) evaluates the "condition" for true or false.
while(button) will be true when button is 1, high, +5V , true.
while(!button) will be true when button is 0,low, ground , false.
So if you use button mode as INPUT_PULLUP the active / pushed will be true when it is connected to ground, hence while(!button) is more descriptive as "active state true when grounded".
IMHO it logically "ties" software with hardware.
If the goal is readability, then:
#define PRESSED LOW
#define RELEASED HIGH
...
while (Switch == PRESSED)
{
...
}
It's most important for the code to be clear to the person working on it. One person will find one style more readable, another may well prefer a different style. Some, unfortunately, delight in writing code that nobody but they can understand, thinking it makes them look smarter (though it does exactly the opposite...).
Regards,
Ray L.
That's good thinking Ray
Ray +1
There is a small difference in performance.
Comparing to zero (LOW) is faster than comparing to some non zero value (HIGH) so when used in timing loops comparing with zero has a definite advantage.
definition HIGH "ambiguous"
Another point to note is that digitalWrite(int val) has an int as parameter and internally LOW writes 0 but all other values write 1. So in a sense HIGH is ambiguous.
Testing "switch != LOW" allows switch to have all non zero values, while sometimes one really wants to see only HIGH and LOW. Something similar occurs with booleans and sometimes you see the code construct:
val = !!b;
A double negation to enforce val to be 0 or 1 and nothing else.
RayLivingston:
It's most important for the code to be clear to the person working on it.
Readability is my primary goal, not for the benefit of others, but for myself! When I have to read the code in 6 months time and cannot remember what I was doing at the time, readability is a big time saver.
robtillaart:
There is a small difference in performance.
Comparing to zero (LOW) is faster than comparing to some non zero value (HIGH) so when used in timing loops comparing with zero has a definite advantage.
Interesting insight. I did not even consider performance.
robtillaart:
definition HIGH "ambiguous"
Another point to note is that digitalWrite(int val) has an int as parameter and internally LOW writes 0 but all other values write 1. So in a sense HIGH is ambiguous.
Testing "switch != LOW" allows switch to have all non zero values, while sometimes one really wants to see only HIGH and LOW.
This is really "splitting hairs", but is it possible that during the transition of a reed switch from HIGH to LOW that maybe for a few microseconds, the input is somewhere in between LOW and HIGH? If so, then while (Switch != LOW) is technically more correct. But again, this is splitting hairs. For what I am trying to it makes absolutely no difference.
What is wrong with treating digitalRead values as boolean?
while (Switch)
{
..
..
Switch = .... ;
}
Incidentally there's an issue here - presumably Switch is read from a pin - why not
read the pin in the while condition so that you don't have to remember to refresh it
in the loop body:
while (digitalRead (SwitchPin))
{
..
}
Of course if the pin is active low you'll need to do something like:
while (digitalRead (SwitchPin) == ACTIVE)
{
..
}
But back to the original question - which is better? Neither, you shouldn't be
using a while loop over a single input, since your entire sketch is locked up
in that loop while the relevant input is active - BlinkWithoutDelay approach is
the better approach!
void loop ()
{
..
if (digitalRead (SwitchPin) == ACTIVE)
handle_switch () ;
..
}
But then you probably don't want to do this at all, you probably want to
detect a change in state of the pin in the first place...
robtillaart:
There is a small difference in performance.
Comparing to zero (LOW) is faster than comparing to some non zero value (HIGH) so when used in timing loops comparing with zero has a definite advantage.
I'm really curious what you base that on. Both boil down to a single compare instruction, which will result in the Zero flag in the processor being either set or cleared. Execution time is exactly the same, regardless of operand values.
robtillaart:
definition HIGH "ambiguous"
Another point to note is that digitalWrite(int val) has an int as parameter and internally LOW writes 0 but all other values write 1. So in a sense HIGH is ambiguous.
The question had to do with testing the value of a switch, which would presumable be read using digitalRead, which will never return anything other than HIGH or LOW.
Regards,
Ray L.
dz63:
This is really "splitting hairs", but is it possible that during the transition of a reed switch from HIGH to LOW that maybe for a few microseconds, the input is somewhere in between LOW and HIGH? If so, then while (Switch != LOW) is technically more correct. But again, this is splitting hairs. For what I am trying to it makes absolutely no difference.
No, it's not possible. A digital input cannot read as anything other than logic o (LOW) or logic 1 (HIGH). If the pin is at some intermediate voltage level, the processor will still never see anything other than HIGH or LOW.
Regards,
Ray L.
Readability is my primary goal, not for the benefit of others, but for myself! When I have to read the code in 6 months time and cannot remember what I was doing at the time, readability is a big time saver.
I could not resist this OT comment.
Unfortunately this forum "gurus" on more than one occasion chastised the OP as "being cluttered with irrelevant HARD TO READ comments". Which to me does not encourage writing explanatory comments at all. This generally misguided opinion is clearly visible in many "for public benefit" software sources.
Off my soap box.
Cheers Vaclav
PS Very beneficial discussion and we did not have to read "format you code" and " what are you trying to do?".
Refreshing start of 2015. Thanks.
I worked with a guy once whose mantra was, if the code was difficult to write it should be difficult to read- no comments.
(He's a PM now.....)
RayLivingston:
I'm really curious what you base that on. Both boil down to a single compare instruction, which will result in the Zero flag in the processor being either set or cleared. Execution time is exactly the same, regardless of operand values.Regards,
Ray L.
@Ray,
please give this sketch a try,
uint32_t start;
uint32_t stop;
volatile int d = 100;
volatile int y = 0;
void setup()
{
Serial.begin(115200);
Serial.println("Start ");
start = micros();
for (int i = 0; i < 30000; i++)
{
if (d == 0)
{
y++;
}
};
stop = micros();
Serial.println(stop - start);
delay(1000); // to be sure printing is done, no IRQ's
start = micros();
for (int i = 0; i < 30000; i++)
{
if (d == 1)
{
y++;
}
};
stop = micros();
Serial.println(stop - start);
}
void loop()
{
}
Sorry for splitting hairs, sometimes every tick counts...
robtillaart:
@Ray,please give this sketch a try,
- timing of 30K compares with 0
- timing of 30K compares with 1
uint32_t start;
uint32_t stop;
volatile int d = 100;
volatile int y = 0;
void setup()
{
Serial.begin(115200);
Serial.println("Start ");
start = micros();
for (int i = 0; i < 30000; i++)
{
if (d == 0)
{
y++;
}
};
stop = micros();
Serial.println(stop - start);
delay(1000); // to be sure printing is done, no IRQ's
start = micros();
for (int i = 0; i < 30000; i++)
{
if (d == 1)
{
y++;
}
};
stop = micros();
Serial.println(stop - start);
}
void loop()
{
}
Sorry for splitting hairs, sometimes every tick counts...
Uh huh.... Give this one a try:
uint32_t start;
uint32_t stop;
volatile int d = 100;
volatile int y = 0;
void setup()
{
Serial.begin(115200);
Serial.println("Start ");
start = micros();
for (int i = 0; i < 30000; i++)
{
if (d == 0)
{
y++;
}
};
stop = micros();
Serial.println(stop - start);
delay(1000); // to be sure printing is done, no IRQ's
start = micros();
for (int i = 0; i < 30000; i++)
{
if (d == 0)
{
y++;
}
};
stop = micros();
Serial.println(stop - start);
}
void loop()
{
}
You'll find this results in exactly the same output as your example.... The compare value makes no difference whatsoever. There is something else going on there, probably interrupt overhead from the timer, or something similar.
Regards,
Ray L.
if I compare both with zero
Start
20796
20752
first compare with one then with zero
Start
22684
20752
so on my PC it behaves differently?
possible explanation
if (x== 1) compiles to (pseudo assembly)
load x
sub 1
jumpNonZero -> end of if
if (x == 0) compiles to
load x
jumpNonZero -> end of if
the difference is one instruction.
if we look at the delta of the loops that is 1932 useconds * 16 ticks / usec = 30912 ticks
==> that is 1 tick per loop (leaving 912 ticks unexplained) that could just be the subtraction of 1 .
(No I did not check with objdump.exe)
Which version of the compiler/platform do you use? I used win7 IDE158 UNO3
MarkT:
What is wrong with treating digitalRead values as boolean?
I would be curious to the answer to this question because that is exactly what I am doing - treating my digitalRead values as either HIGH or LOW. Seems to be working for me. I hope it is OK?
MarkT:
Neither, you shouldn't be
using a while loop over a single input, since your entire sketch is locked up
in that loop while the relevant input is active - BlinkWithoutDelay approach is
the better approach!void loop ()
{
..
if (digitalRead (SwitchPin) == ACTIVE)
handle_switch () ;
..
}
But then you probably don't want to do this at all, you probably want to detect a change in state of the pin in the first place...
Actually, the while loop is in my void setup(), so I want to and can afford to "lock up" the sketch. In the void loop(), there are no "while" loops - strictly "IF" loops and I am using millis() and unsigned long global variables for four timers (ie. the blink without delay approach). Initially, it was all a bit complicated to figure it out, but now I have got the hang of it. Works pretty slick too.
Thanks for your suggestions.
dz63:
I would be curious to the answer to this question because that is exactly what I am doing - treating my digitalRead values as either HIGH or LOW. Seems to be working for me. I hope it is OK?
Nothing at all wrong with that.
Regards,
Ray L.
The thing that can make the reading more confusing is when you missname your variables. For instance if you have a float switch that is active LOW and is triggered by the water level being high. Then you can end up with this counter intuitive logic
if ( digitalRead (waterLevel) == HIGH )
openInletValve();
Whereas if you change the name "waterLevel" to "lowWater" you then get
if ( digitalRead (lowWater))
openInletValve();
and it all makes sense.
Rather than being vague about what your inputs are, think about what they tell you when they are true (ie HIGH)