Why won't this code toggle an output and hold?

system · May 21, 2009, 6:03pm

So, I'm new to programming (but having fun). I have written the following code but when I run it and send the ASCII characters to toggle an output (which is used to turn a relay on or off), it switches the output on for approx. one second and then it always clicks back to off. I want the relay, when the ASCII is received, to switch states and hold it's position there until the same ASCII message is received. I don't know that it matters but, I'm using a Fundamental Logic MaxSerial board (Diecimila).

int Pin0 = 0; // Relay Pins and LED test on 13 int Pin1 = 1; int Pin2 = 2; int Pin3 = 3; int Pin4 = 4; int Pin5 = 5; int Pin6 = 6; int Pin7 = 7; int Pin8 = 8; int Pin9 = 9; int Pin10 = 10; int Pin11 = 11; int Pin12 = 12; int ledPin = 13; int Pin14 = 14; int Pin15 = 15; int Pin16 = 16; int incomingCommand = 0; //holding variable for strings

void setup() // run once, when the sketch starts { Serial.begin(9600); pinMode(Pin0, OUTPUT); // sets the digital pins as outputs pinMode(Pin1, OUTPUT); pinMode(Pin2, OUTPUT); pinMode(Pin3, OUTPUT); pinMode(Pin4, OUTPUT); pinMode(Pin5, OUTPUT); pinMode(Pin6, OUTPUT); pinMode(Pin7, OUTPUT); pinMode(Pin8, OUTPUT); pinMode(Pin9, OUTPUT); pinMode(Pin10, OUTPUT); pinMode(Pin11, OUTPUT); pinMode(Pin12, OUTPUT); pinMode(ledPin, OUTPUT); pinMode(Pin14, OUTPUT); pinMode(Pin15, OUTPUT); pinMode(Pin16, OUTPUT); }

void loop(){ if (Serial.available() > 0) { digitalWrite (ledPin, HIGH); //turn LED on when serial incoming incomingCommand = Serial.read(); } else { digitalWrite (ledPin, LOW); //turn it off when no serial available }

//Serial.flush();

if (incomingCommand == 00) while(Pin0 == true){ digitalWrite(Pin0, LOW); } else{ digitalWrite(Pin0, HIGH); }

//Serial.flush();

if (incomingCommand == 01) while(Pin1 = true){ digitalWrite(Pin1, LOW); } else{ digitalWrite(Pin1, HIGH); }

//Serial.flush();

if (incomingCommand == 02) while(Pin2 = true){ digitalWrite(Pin2, LOW); } else{ digitalWrite(Pin2, HIGH); }

//Serial.flush();

if (incomingCommand == 03) while(Pin3 = true){ digitalWrite(Pin3, LOW); } else{ digitalWrite(Pin3, HIGH); }

//Serial.flush();

if (incomingCommand == 04) while(Pin4 = true){ digitalWrite(Pin4, LOW); } else{ digitalWrite(Pin4, HIGH); }

//Serial.flush();

if (incomingCommand == 05) while(Pin5 = true){ digitalWrite(Pin5, LOW); } else{ digitalWrite(Pin5, HIGH); }

//Serial.flush();

if (incomingCommand == 06) while(Pin6 = true){ digitalWrite(Pin6, LOW); } else{ digitalWrite(Pin6, HIGH); }

//Serial.flush();

if (incomingCommand == 07) while(Pin7 = true){ digitalWrite(Pin7, LOW); } else{ digitalWrite(Pin7, HIGH); }

//Serial.flush();

if (incomingCommand == 18) while(Pin8 = true){ digitalWrite(Pin8, LOW); } else{ digitalWrite(Pin8, HIGH); }

//Serial.flush();

if (incomingCommand == 19) while(Pin9 = true){ digitalWrite(Pin9, LOW); } else{ digitalWrite(Pin9, HIGH); }

//Serial.flush();

if (incomingCommand == 10) while(Pin10 = true){ digitalWrite(Pin10, LOW); } else{ digitalWrite(Pin10, HIGH); }

//Serial.flush();

if (incomingCommand == 11) while(Pin11 = true){ digitalWrite(Pin11, LOW); } else{ digitalWrite(Pin11, HIGH); }

//Serial.flush();

if (incomingCommand == 12) while(Pin12 = true){ digitalWrite(Pin12, LOW); } else{ digitalWrite(Pin12, HIGH); }

//Serial.flush();

if (incomingCommand = 14) while(Pin14 = true){ digitalWrite(Pin14, LOW); } else{ digitalWrite(Pin14, HIGH); }

//Serial.flush();

if (incomingCommand == 15) while(Pin15 = true){ digitalWrite(Pin15, LOW); } else{ digitalWrite(Pin15, HIGH); }

//Serial.flush();

if (incomingCommand == 16) while(Pin16 = true){ digitalWrite(Pin16, LOW); } else{ digitalWrite(Pin16, HIGH); }

//Serial.flush();

}

system · May 21, 2009, 7:40pm

Right off the bat you have a problem. You are using pins 0 & 1 for digital outputs. You are also using Serial. Pins 0 and 1 are used for serial received data and transmit data. You can't have that and also use them for I/O pins or really crazy stuff will happen -- they'll go off and on at high speed as data is sent and received.

So pick different pins.

Lets look at one piece of your code and see what it really does. I moved the brackets and indention to make it more clear

if (incomingCommand == 03) // if the value of incomingCommand is 3 - good while(Pin3 = true) // I think you wanted == here - what you did was { // set the value of Pin3 to 1 (true = 1) then check the // while. While(1) is always satisfied so the next // statement will execute forever. digitalWrite(Pin3, LOW); // and since the value of Pin3 is now 1 you are setting // Arduino pin 1 low. Using = where you mean == will get ya every time } else { // the else pairs with the if so if the incoming command isn't 03 we set Arduino pin 3 HIGH // unless the code above executed first in which case it sets pin 1 high! digitalWrite(Pin3, HIGH); // for example if the command is 04, then this code will turn Arduino pin 3 (or 1) ON! }

So lets change (Pin3 = true) to (Pin3 == true) like I think you meant and see what we have

if (incomingCommand == 03) // if the value of incomingCommand is 3 - good while(Pin3 == true) // Lets see = Pin3 was set to 3 above,right? // so the statement says while(3==true) // any non-zero value is true so again the while // will always be satisfied and we'll hang here. { digitalWrite(Pin3, LOW); // we'll always write LOW to pin 3. // if incomingCommand is 03 } else { // again, the else pairs with the closest 'if' - NOT the while() so digitalWrite(Pin3, HIGH); // if the incomingCommand is not 03, write a HIGH to Pin3 // which is still not what I think you want. }

but if you change the while(Pin3 == true) to if(Pin3 == true) then the else would pair with that if:

if (incomingCommand == 03) // if the value of incomingCommand is 3 - good if(Pin3 == true) // again Pin3 = 3. 3 is true so we still have a problem { digitalWrite(Pin3, LOW); } else { // now the else pairs with if(Pin3 == true) digitalWrite(Pin3, HIGH); }

You could add more variables to keep track of whether Pin3 was last set to HIGH or LOW OR simply read it and see what it is now. Yes! You can read an output pin so lets change it to:

if (incomingCommand == 03) // if the value of incomingCommand is 3 - good if(digitalRead(Pin3) == true) { digitalWrite(Pin3, LOW); } else { digitalWrite(Pin3, HIGH); // so if Pin3 was LOW this will set it high }

Or you could just do something like this:

if(incomingCommand == 03) digitalWrite( Pin3, !digitalRead(Pin3)); // Read Pin3, invert it and write it back to Pin3 // YES you can read an output pin! // ! is the 'not' operator // !HIGH = LOW, !LOW = HIGH, !true = false, etc.

system · May 21, 2009, 7:52pm

Wow, RoyK, thanks. It's like you know how I learn and this is so well done. Thank you for putting so much into the answer!

system · May 21, 2009, 7:53pm

Hey - been there - made those mistakes. We all have. That's how you learn.

One more piece of advice - before you write a sketch like the above handling a bunch of pins - get it working with just one or two. Once you have that working and tested then expand it to handle more.

Keep truckin.

system · May 21, 2009, 8:20pm

while(Pin7 = true){

OK, maybe you wanted "==" instead of "=", BUT even if you put "==" the only "Pinx" that isn't already "true" is "Pin0"

system · May 21, 2009, 8:31pm

Oh, and one more thing - your code:

incomingCommand = Serial.read();

will get one byte from the read buffer. I think you intend for your program to toggle Pin3 if you send it a '3' from the keyboard - do you not?

If you type '3' it will send the ASCII code for 3 which is 51 decimal (33 hex).

So your code would need to read if (incomingCommand == 51) or if(incomingCommand == '3') ...

Note that you declared incomingCommand as int. It will hold exactly one character.

For numbers greater than 9 (like typing ''10" to control Pin10) you'll have to do more logic because you'll have two characters to deal with...

If you have something that is actually sending 03 & 10 then your code is OK.

system · May 21, 2009, 10:27pm

I am a very happy man! Thanks all for the help! It is now working a charm. I thought I’d post my updated code (it’s a bit cleaner now) for others who may want to do similar…

int Pin1 = 2; // Relay Pins
int Pin2 = 3;
int Pin3 = 4;
int Pin4 = 5;
int Pin5 = 6;
int Pin6 = 7;
int Pin7 = 8;
int Pin8 = 9;
int Pin9 = 10;
int Pin10 = 11;
int Pin11 = 12;
int Pin12 = 13;
int Pin13 = 14;
int Pin14 = 15;
int Pin15 = 16;
int Pin16 = 17;
char incomingCommand = 0; //holding variable for commands

void setup() // run once, when the sketch starts
{
Serial.begin(9600);
pinMode(Pin1, OUTPUT); // sets the digital pins as outputs
pinMode(Pin2, OUTPUT);
pinMode(Pin3, OUTPUT);
pinMode(Pin4, OUTPUT);
pinMode(Pin5, OUTPUT);
pinMode(Pin6, OUTPUT);
pinMode(Pin7, OUTPUT);
pinMode(Pin8, OUTPUT);
pinMode(Pin9, OUTPUT);
pinMode(Pin10, OUTPUT);
pinMode(Pin11, OUTPUT);
pinMode(Pin12, OUTPUT);
pinMode(Pin13, OUTPUT);
pinMode(Pin14, OUTPUT);
pinMode(Pin15, OUTPUT);
pinMode(Pin16, OUTPUT);
}

void loop(){
{
incomingCommand = Serial.read();
}

if(incomingCommand == ‘A’)
if(digitalRead(Pin1) == true)
{
digitalWrite(Pin1, LOW);
}
else
{
digitalWrite(Pin1, HIGH);
}

if (incomingCommand == ‘B’)
if(digitalRead(Pin2) == true)
{
digitalWrite(Pin2, LOW);
}
else
{
digitalWrite(Pin2, HIGH);
}

if (incomingCommand == ‘C’)
if(digitalRead(Pin3) == true)
{
digitalWrite(Pin3, LOW);
}
else
{
digitalWrite(Pin3, HIGH);
}

if (incomingCommand == ‘D’)
if(digitalRead(Pin4) == true)
{
digitalWrite(Pin4, LOW);
}
else
{
digitalWrite(Pin4, HIGH);
}

if (incomingCommand == ‘E’)
if(digitalRead(Pin5) == true)
{
digitalWrite(Pin5, LOW);
}
else
{
digitalWrite(Pin5, HIGH);
}

if (incomingCommand == ‘F’)
if(digitalRead(Pin6) == true)
{
digitalWrite(Pin6, LOW);
}
else
{
digitalWrite(Pin6, HIGH);
}

if (incomingCommand == ‘G’)
if(digitalRead(Pin7) == true)
{
digitalWrite(Pin7, LOW);
}
else
{
digitalWrite(Pin7, HIGH);
}

if (incomingCommand == ‘H’)
if(digitalRead(Pin8) == true)
{
digitalWrite(Pin8, LOW);
}
else
{
digitalWrite(Pin8, HIGH);
}

if (incomingCommand == ‘I’)
if(digitalRead(Pin9) == true)
{
digitalWrite(Pin9, LOW);
}
else
{
digitalWrite(Pin9, HIGH);
}

if (incomingCommand == ‘J’)
if(digitalRead(Pin10) == true)
{
digitalWrite(Pin10, LOW);
}
else
{
digitalWrite(Pin10, HIGH);
}

if (incomingCommand == ‘K’)
if(digitalRead(Pin11) == true)
{
digitalWrite(Pin11, LOW);
}
else
{
digitalWrite(Pin11, HIGH);
}

if (incomingCommand == ‘L’)
if(digitalRead(Pin12) == true)
{
digitalWrite(Pin12, LOW);
}
else
{
digitalWrite(Pin12, HIGH);
}

if (incomingCommand == ‘M’)
if(digitalRead(Pin13) == true)
{
digitalWrite(Pin13, LOW);
}
else
{
digitalWrite(Pin13, HIGH);
}

if (incomingCommand == ‘N’)
if(digitalRead(Pin14) == true)
{
digitalWrite(Pin14, LOW);
}
else
{
digitalWrite(Pin14, HIGH);
}

if (incomingCommand == ‘O’)
if(digitalRead(Pin15) == true)
{
digitalWrite(Pin15, LOW);
}
else
{
digitalWrite(Pin15, HIGH);
}

if (incomingCommand == ‘P’)
if(digitalRead(Pin16) == true)
{
digitalWrite(Pin16, LOW);
}
else
{
digitalWrite(Pin16, HIGH);
}
}

mem · May 21, 2009, 10:41pm

Good job.

Here is a simplification of your sketch (untested) that you may find useful.

int firstPin = 2;       // Relay Pins are sequential starting from pin 2
int pinCount = 16;      // the number of pins connected to relays  

char incomingCommand = 0;              //holding variable for strings

void setup()                    // run once, when the sketch starts
{
  Serial.begin(9600);
  for( int i=0; i < pinCount; i++) 
  {
    pinMode(firstPin + i, OUTPUT);        // sets the digital pins as outputs
  }
}

void loop(){
 if(Serial.available()
{
    incomingCommand = Serial.read();
    // check if the command is within the range of characters expected
   if(incomingCommand >= 'A' && incomingCommand <= 'L')
   {
     // subtract the ascii value from the command
     // and add firstPin to to get the actual pin number 
     int Pin = incomingCommand - 'A' + firstPin; 
     if(digitalRead(Pin) == true)  
     {                 
      digitalWrite(Pin, LOW);  
     }
     else
     {
      digitalWrite(Pin, HIGH); // so if Pin was LOW this will set it high
     }
   }
 }
}

this code relies on the fact that your pins are sequential and so are your commands. So it converts the commands to sequential values :
int Pin = incomingCommand - ‘A’;
sets Pin to 0 for command A, 1 for B etc.

you then add the offset to the first pin to get the actual pin number:
int Pin = incomingCommand - ‘A’+ firstPin;
sets pin to 2 when command A is received, its set to 2 with command B etc.

I have added the Serial.available() test to see if a character is available. the code would work without this because Serial.read() returns -1 if there is no character available - but someday you may want to receive this character value so its more robust and clearer to use Serial.available()

I hope that helps

system · May 21, 2009, 11:09pm

Yup!
And even a bit more compact:

int firstPin = 2; // Relay Pins are sequential starting from pin 2
int lastpin = 17; //

char incomingCommand; //holding variable for received character

void setup() // run once, when the sketch starts
{
Serial.begin(9600);
for( int i=firstPin; i <= lastpin; i++)
pinMode(i, OUTPUT); // sets the digital pins as outputs
}

void loop(){
if(Serial.available()
{
incomingCommand = Serial.read();
if(incomingCommand >= ‘A’ && incomingCommand <= ‘L’)
{
// subtract the ascii valuei of the first char to respond tofrom the command
// and add firstPin to to get the actual pin number
int Pin = incomingCommand - ‘A’ + firstPin;
digitalWrite(Pin, !digitalRead(Pin));
}
}

system · May 21, 2009, 11:28pm

RoyK and mem, thanks. Those are great examples of taking "working" code and turning it into "elegant" code. I think I understand the bulk of what's happening but, it just shows that I have a ways to go. In other words, "elegant" is the goal. Thanks for being so patient and putting so much into your responses and in such a way that a rookie can understand.

system · May 21, 2009, 11:35pm

elegant comes with time & experience & learning from others' code.

The important thing is that the code does what you want it to do every time. Elegant is less important.

You've learned a lot today. Pat yourself on the back.

mem · May 21, 2009, 11:42pm

The important thing is that the code does what you want it to do every time

I completely agree with this sentiment.

Indeed, the last example code is IMO overly terse .

There is nothing to be ashamed about using: if(digitalRead(Pin) == true) { digitalWrite(Pin, LOW); } else { digitalWrite(Pin, HIGH); // so if Pin was LOW this will set it high }

system · May 21, 2009, 11:55pm

well it could have been

digitalWrite(Pin, digitalRead(pin) ? LOW : HIGH);

if I really wanted to be confusing to an inexperienced programmer.

Toggling a pin is such a common thing to do that I find the
digitalWrite(Pin, !digitalRead(Pin)) a very handy expression because it is terse.

There are always multiple ways of coding things - which one to use is often a matter of habit and opinion. ;>)

retrolefty · May 22, 2009, 12:43am

There are always multiple ways of coding things - which one to use is often a matter of habit and opinion. ;>)

Very true and also varies depending on background and experience. Being from a hardware background my instinct is to use direct port read, then use an exclusive OR mask with mask bits set to one for pins wishing to be toggled, and then write back to the port. Saves quite a lot of code and time, but probably no clear for beginners.

Lefty

system · May 22, 2009, 1:30am

retrolefty, would you mind posting an example of your version? RoyK is right. I have learned a lot today but, more is better.

retrolefty · May 22, 2009, 2:09am

OK, to toggle say Arduino pin2 one could use:

PORTD = PIND ^ 0x40;

This should result in the fastest shortest code, but it requires you to figure out what port and bit position relate to which Arduino pin number. The Arduino digital read and write functions do this mapping for you automatically.

Lefty

system · May 22, 2009, 9:18am

I too am from a hardware background and indeed as retroLefty says that's the way I also learned to toggle a pin. However for recreational programming with Arduinos I'd avoid that one because, for one thing, it would make troubleshooting your project tougher.

If you use the digitalRead(Pin) and digitalWrite(Pin) methods you can much more easily relate your code to the pin numbers marked on your board and, when one of the LEDs isn't coming on, find that LED that you put in backwards or the jumper wire that's plugged in the wrong hole or that has popped out.

One piece of advice for you based on long experience - comment your code! Over comment your code! Not with this kind of comment:

digitalWrite(Pin3, HIGH); // Write a high to pin 3

That is obviously what the code does - it doesn't need a comment to say so.

But use this kind of comment:

// Check the received character. If it is the 'A' key then toggle the // Alarm LED (assuming that's what the LED connected to Pin2 is)

That explains what you are trying to achieve with the next few lines of code.

You will be absolutely amazed a few months from now when you look back at your own code -- let alone someone else's code -- how difficult it will be to figure out what's going on if it isn't well commented.

Also clearly stating what you want the code to do as you go along greatly helps you figure out how to make the code do it.

By the way - good for you for setting the pin numbers to variable names (Pin2 = 2; Pin3 = 3; etc) at the top of your code and using the variable names from then on. That makes it much easier to change what pin does what if you have to, doesn't it? But wait - you can simply change the pin numbers but then the code would really get confusing if you used Pin2 = 7; Pin3 = 5; etc.

Even better -- if appropriate -- would be to name the pins based on what they do -- AlarmLEDPin = 2; AlarmRelayPin = 3; BeeperPin = 4; etc.

Then your code would read say

// Reset the alarm if we get an 'R'

if(incomingCharacter == 'R') { digitalWrite(AlarmRelayPin, LOW); digitalWrite(BeeperPin, LOW); digitalWrite(AlarmLEDPin , LOW); }

Which makes it pretty darned clear what the code does. And if you needed to change the pin connected to the alarm relay to , say 12, simply change AlarmRelayPin = 3; to AlarmRelayPin = 12; at the top of your code and everything still makes sense.

system · May 6, 2021, 8:04pm