ivalue required with IF statement

Using Arduino Programming Questions
1

Hello everyone, I have some code below which i got great help with last week.
now i have an extra IF statement and am getting a (Ivaluve required as left operand of assignment) at my second if statement.

the idea behind the code (and its just for fun) is that an output (ledPin) will turn on and off with the millis code.
While this is happening, another pin (sensorPin) will listen for a high input.
If both ledPin and sensorPin are high, i would like (alarmPin) to go high.

it seemed like it was going to be very simple, i was wrong.

unsigned long startMillis; //some global variables anywhere in the program
unsigned long currentMillis; 
const unsigned long period = 1000; //the value is a number in milliseconds
const byte ledPin = 13; // using the built in led
const int alarmPin =  12; // output for alarm
const int sensorPin = 11;  // input for ir sensor  



void setup() {
  // put your setup code here, to run once:
Serial.begin (115200); //start serial in case we need to print debugging info
pinMode (ledPin,OUTPUT);
pinMode (alarmPin,OUTPUT);
pinMode (sensorPin, INPUT);
startMillis = millis (); //initial start time
}

void loop() {
  // put your main code here, to run repeatedly:
currentMillis = millis (); //get the current ''time'' (actually the number of milliseconds since the program started
if (currentMillis - startMillis >= period)         // test whether the period has elapsed
{digitalWrite (ledPin, !digitalRead(ledPin));     //if so, change the state of the led
startMillis = currentMillis;                    // IMPORTANT to save the start time of the current led state

if (ledPin == HIGH && sensorPin = HIGH) 
{ digitalWrite (alarmPin, HIGH);}




}
2 
...
if (currentMillis - startMillis >= period)         // test whether the period has elapsed
{digitalWrite (ledPin, !digitalRead(ledPin));     //if so, change the state of the led
...

Where is the closing brace for that if?

Small changes to your formatting would dramatically increase the legibility.

3

sensorPin = HIGH is an assignment, sensorPin == HIGH is a test.

4

DKWatson:
sensorPin = HIGH is an assignment, sensorPin == HIGH is a test.

...and a pointless one at that.

5 
if (ledPin == HIGH && sensorPin = HIGH)

should probably be

if (digitalRead(ledPin) == HIGH && digitalRead(sensorPin) == HIGH)
6

Howabout the much clearer:

void loop()
{
  if (millis() - startMillis >= period)         // test whether the period has elapsed
  {
    startMillis += period ;
    
    digitalWrite (ledPin, !digitalRead(ledPin));     //if so, change the state of the led
  }

  digitalWrite (alarmPin, digitalRead(ledPin) && digitalRead(sensorPin)) ;
}
7

johnwasser:

if (ledPin == HIGH && sensorPin = HIGH)

should probably be

if (digitalRead(ledPin) == HIGH && digitalRead(sensorPin) == HIGH)

Hey John thanks for the reply.
sorry this is so late i only get 3 hours between end of shift and bed time each day, madness.
i replaced my line of code with yours (if (digitalRead(ledPin) == HIGH && digitalRead(sensorPin) == HIGH))
and the circuit almost worked.
with your line of code the circuit almost worked, just now my output was a latching one.
i fiddled with it and got to unlatch using what you showed me and added an alarmAck input to delatch the alarm output. thank you so much for clearing up the IF stuff for me.

unsigned long startMillis; //some global variables anywhere in the program
unsigned long currentMillis; 
const unsigned long period = 1000; //the value is a number in milliseconds
const byte ledPin = 13; // using the built in led
const int alarmPin =  12; // output for alarm
const int sensorPin = 11;  // input for ir sensor  
const int alarmAck = 10; //reset for alarm


void setup() {
  // put your setup code here, to run once:
Serial.begin (115200); //start serial in case we need to print debugging info
pinMode (ledPin,OUTPUT);
pinMode (alarmPin,OUTPUT);
pinMode (sensorPin, INPUT);
pinMode (alarmAck, INPUT);
startMillis = millis (); //initial start time
}

void loop() {
  // put your main code here, to run repeatedly:
currentMillis = millis (); //get the current ''time'' (actually the number of milliseconds since the program started
if (currentMillis - startMillis >= period)         // test whether the period has elapsed
{digitalWrite (ledPin, !digitalRead(ledPin));     //if so, change the state of the led
startMillis = currentMillis;                    // IMPORTANT to save the start time of the current led state

if (digitalRead(ledPin) == LOW && digitalRead(sensorPin) == HIGH)

{ digitalWrite (alarmPin, HIGH);}

if (digitalRead (alarmAck) == HIGH && digitalRead (alarmPin) ==HIGH)
 {digitalWrite (alarmPin, LOW);}}



}

this is how it looks now, the idea for the exercise was to have an output to something which pulsed on and off.
then there was to be a sensor input which would only be read while the output was off.
if the sensor went high while the output was off it would trigger another output for an alarm.
this alarm could then be cleared via an acknowledge input.
i really am only at the very very basic level with this stuff and keep thinking ''i could just do this with CMOS'' but i must push on and learn ow to use these damn microcontrollers.
how would you have written this sort of thing? much shorter?

thank you for your help.

8

Looks good. Remember to run Tools->Auto Format before posting your code.

I suggest a few minor changes:

Make "currentMillis" a local variable in loop() since it is not needed elsewhere. In general it is good to make variables as local as possible so you don't have to search the entire program to find out how they are used.

Make all your pin numbers 'const byte'. It probably produces the same code as 'const int' in almost all cases but 'byte' might be slightly more space efficient in some cases.

Add "Pin" to the name "alarmAck" like you did with the other pins. It makes it clearer what the variable is used for.

Make the first letter of each of the global variable names a capital letter. This makes it easier to separate globals and locals. If you declare a local variable with the same name as a global variable, the local will be used in that one function. Having globals start with capitals and locals start with lowercase makes confusion less likely.

When your millis() timer goes off, it's slightly better to use "StartMillis += Period;" than "StartMillis = currentMillis;". If there is something in your code that takes more than a millisecond you could miss the exact time the timer expires. Using "StartMillis += Period;" will get you back on track. For example if StartMillis is 500 and your Period is 1000 you should trigger when currentMillis gets to 1500. If you get delayed and can't check the timer until currentMillis == 1508 you would get StartMillis set to 1508 and the next period would end at 2508, not 2500. By using "StartMillis += Period;" you would set StartMillis to 1500 (500 + 1000), not 1508, and the next period would end at 2500, right on time.

Those are the only suggestions I have, many of them are just style and naming conventions to make life easier for you and anyone trying to understand your sketch.

unsigned long StartMillis; //some global variables anywhere in the program
const unsigned long Period = 1000; //the value is a number in milliseconds

const byte LEDPin = 13; // using the built in led
const byte AlarmPin =  12; // output for alarm
const byte SensorPin = 11;  // input for ir sensor
const byte AlarmAckPin = 10; //reset for alarm

void setup()
{
  // put your setup code here, to run once:
  Serial.begin (115200); //start serial in case we need to print debugging info
  pinMode (LEDPin, OUTPUT);
  pinMode (AlarmPin, OUTPUT);
  pinMode (SensorPin, INPUT);
  pinMode (AlarmAckPin, INPUT);
  StartMillis = millis (); //initial start time
}

void loop()
{
  // put your main code here, to run repeatedly:
  unsigned long currentMillis = millis (); //get the current ''time'' (actually the number of milliseconds since the program started
  if (currentMillis - StartMillis >= Period)         // test whether the period has elapsed
  {
    digitalWrite (LEDPin, !digitalRead(LEDPin));     //if so, change the state of the led
    StartMillis += Period;                    // IMPORTANT to save the start time of the current led state

    if (digitalRead(LEDPin) == LOW && digitalRead(SensorPin) == HIGH)
    {
      digitalWrite (AlarmPin, HIGH);
    }

    if (digitalRead (AlarmAckPin) == HIGH && digitalRead (AlarmPin) == HIGH)
    {
      digitalWrite (AlarmPin, LOW);
    }
  }
}
9

thanks for that John, its great to get some straight forward advice on this stuff.