Problem calling a function from another function

I am trying to call a function to shift bits into a 74hc595 from another function outside of "loop" and it fails to shift shift the bits into the 74hc595. When I call the same function from inside loop it works. Any help is appreciated.

/*
Halloween 2012 Coffin Prop Controller
 */
//Set up 74hc595
//Pin connected to latch pin (ST_CP) of 74HC595
const int latchPin = 8;
//Pin connected to clock pin (SH_CP) of 74HC595
const int clockPin = 12;
////Pin connected to Data in (DS) of 74HC595
const int dataPin = 11;

void setup() {
  //set pins to output because they are addressed in the main loop
  //This is the code for the 74hc595
  pinMode(latchPin, OUTPUT);
  pinMode(dataPin, OUTPUT);  
  pinMode(clockPin, OUTPUT);
  //Reset the Chip
  digitalWrite(latchPin, LOW);
  digitalWrite(dataPin, LOW);
  digitalWrite(clockPin, LOW);
  
  //Setup the Serial Connection
  Serial.begin(9600);
  Serial.println("reset");

  //Print a pretty message on the lcd that we are ready
  lcd.println("Program Running");
}

void loop() {
       //There is PIR code here that will actually trigger the animation.
       runAnimation();  //call the function to start the animation
       delay(5000);

       // This works from here.
       for ( int bitToSet = 0; bitToSet <=8; bitToSet++ ) {
         registerWrite(bitToSet, HIGH);
         Serial.println(bitToSet);
         delay(1000);
       }
    }


} // End Loop

// This method sends bits to the shift register:

}

void runAnimation() {
  //Start the fogger
  Serial.println("fire the fogger");
  registerWrite(bitToSet, HIGH); // This fails here - the serial shows that the bits are right, but nothing happens
  delay(1000);
  
  //Start strobing the light
  Serial.println("Strobe the light");
  registerWrite(6, HIGH);
  delay(1000);
  registerWrite(6, LOW);
  delay(500);
  registerWrite(6, HIGH);
  delay(1000);
  registerWrite(6, LOW);
  delay(500);
  registerWrite(6, HIGH);
  
  //Pop open the coffin
  Serial.println("Open the Coffin");
  registerWrite(1, HIGH);
  delay(1000);
  registerWrite(1, LOW);
  delay(500);
  registerWrite(1, HIGH);
  delay(1000);
  registerWrite(1, LOW);
  delay(500);
  registerWrite(1, HIGH);
  delay(1000);
  
  //Pop up the skeleton
  Serial.println("Pop the Skeleton");
  registerWrite(2, HIGH);
  delay(1000);
  
  //Stop the fogger so it has time to warm up again
  Serial.println("Stop the Fogger");
  registerWrite(5, LOW);
  delay(1000);
  
  //Reset the prop
  Serial.println("Reset the Prop");
  //lower the skeleton
  Serial.println("Lower the Skeleton");
  registerWrite(2, LOW);
  delay(1000);
  //Lower the lid
  Serial.println("Lower the Lid");
    registerWrite(1, LOW);
  delay(1000);
  //Turn off the light
  Serial.println("Turn of the light");
  registerWrite(6, LOW);
  
  Serial.println("Animation Finished");
}

void registerWrite(int whichPin, int whichState) {
  Serial.print("called registerWrite PIN - ");
  Serial.println(whichPin);
  Serial.print("whichState - ");
  Serial.println(whichState);
// the bits you want to send
  byte bitsToSend = 0;

  // turn off the output so the pins don't light up
  // while you're shifting bits:
  digitalWrite(latchPin, LOW);

  // turn on the next highest bit in bitsToSend:
  bitWrite(bitsToSend, whichPin, whichState);
  Serial.print("bitsToSend - ");
  Serial.println(bitsToSend);

  // shift the bits out:
  shiftOut(dataPin, clockPin, MSBFIRST, bitsToSend);

  // turn on the output so the LEDs can light up:
  digitalWrite(latchPin, HIGH);

}

Again, calling registerWrite from inside the "loop" works great. When calling registerWrite from startAnimation it fails to active the 74hc595. The Serial output shows the correct bits are set, but I cannot figure out why the digitalWrite or shiftOut fails.

Thanks, Josh

Looks to me like the code doesn't compile.

It doesn't compile, you have too many braces, and I don't get this bit:

void runAnimation() {
  //Start the fogger
  Serial.println("fire the fogger");
  registerWrite(bitToSet, HIGH); // This fails here - the serial shows that the bits are right, but nothing happens

bitToSet is not in scope at that point.

The Serial output shows the correct bits are set, but I cannot figure out why the digitalWrite or shiftOut fails.

Better post code that compiles if the complaint is about runtime behaviour.

Hint: hitting CTRL+T gives "too many closed curly braces".

I'm sorry, in my haste to clean up the code so it was more readable in the forum I missed a few things. I've checked the code below and it now compiles properly as did the original unseen code. Unfortunatly there is still no change. When calling the registerWrite function from loop it works great. When registerWrite is called from startAnimation is is not working.

/*
Halloween 2012 Coffin Prop Controller
 */
//Set up 74hc595
//Pin connected to latch pin (ST_CP) of 74HC595
const int latchPin = 8;
//Pin connected to clock pin (SH_CP) of 74HC595
const int clockPin = 12;
////Pin connected to Data in (DS) of 74HC595
const int dataPin = 11;

void setup() {
  //set pins to output because they are addressed in the main loop
  //This is the code for the 74hc595
  pinMode(latchPin, OUTPUT);
  pinMode(dataPin, OUTPUT);  
  pinMode(clockPin, OUTPUT);
  //Reset the Chip
  digitalWrite(latchPin, LOW);
  digitalWrite(dataPin, LOW);
  digitalWrite(clockPin, LOW);
  
  //Setup the Serial Connection
  Serial.begin(9600);
  Serial.println("reset");

  //Print a pretty message on the lcd that we are ready

}

void loop() {
       //There is PIR code here that will actually trigger the animation.
       runAnimation();  //call the function to start the animation
       delay(5000);

       // This works from here.
       for ( int bitToSet = 0; bitToSet <=8; bitToSet++ ) {
         registerWrite(bitToSet, HIGH);
         Serial.println(bitToSet);
         delay(1000);
       }
    


} // End Loop

void runAnimation() {
  //Start the fogger
  Serial.println("fire the fogger");
  registerWrite(5, HIGH); // This fails here - the serial shows that the bits are right, but nothing happens
  delay(1000);
  
  //Start strobing the light
  Serial.println("Strobe the light");
  registerWrite(6, HIGH);
  delay(1000);
  registerWrite(6, LOW);
  delay(500);
  registerWrite(6, HIGH);
  delay(1000);
  registerWrite(6, LOW);
  delay(500);
  registerWrite(6, HIGH);
  
  //Pop open the coffin
  Serial.println("Open the Coffin");
  registerWrite(1, HIGH);
  delay(1000);
  registerWrite(1, LOW);
  delay(500);
  registerWrite(1, HIGH);
  delay(1000);
  registerWrite(1, LOW);
  delay(500);
  registerWrite(1, HIGH);
  delay(1000);
  
  //Pop up the skeleton
  Serial.println("Pop the Skeleton");
  registerWrite(2, HIGH);
  delay(1000);
  
  //Stop the fogger so it has time to warm up again
  Serial.println("Stop the Fogger");
  registerWrite(5, LOW);
  delay(1000);
  
  //Reset the prop
  Serial.println("Reset the Prop");
  //lower the skeleton
  Serial.println("Lower the Skeleton");
  registerWrite(2, LOW);
  delay(1000);
  //Lower the lid
  Serial.println("Lower the Lid");
    registerWrite(1, LOW);
  delay(1000);
  //Turn off the light
  Serial.println("Turn of the light");
  registerWrite(6, LOW);
  
  Serial.println("Animation Finished");
}

void registerWrite(int whichPin, int whichState) {
  Serial.print("called registerWrite PIN - ");
  Serial.println(whichPin);
  Serial.print("whichState - ");
  Serial.println(whichState);
// the bits you want to send
  byte bitsToSend = 0;

  // turn off the output so the pins don't light up
  // while you're shifting bits:
  digitalWrite(latchPin, LOW);

  // turn on the next highest bit in bitsToSend:
  bitWrite(bitsToSend, whichPin, whichState);
  Serial.print("bitsToSend - ");
  Serial.println(bitsToSend);

  // shift the bits out:
  shiftOut(dataPin, clockPin, MSBFIRST, bitsToSend);

  // turn on the output so the LEDs can light up:
  digitalWrite(latchPin, HIGH);

}

Could it be because you turn the latch pin HIGH inside the loop ?

    for ( int bitToSet = 0; bitToSet <= 8; bitToSet++ ) {
                                    ^^^^

The way I read your code:

void registerWrite(int whichPin, int whichState) {
  byte bitsToSend = 0;
  bitWrite(bitsToSend, whichPin, whichState);  
  shiftOut(dataPin, clockPin, MSBFIRST, bitsToSend);

The routine ALWAYS starts with bitsToSend at zero. If you called it with
  registerWrite(5, HIGH);it will correctly set and transfer bit 5 to the shift register.
A second later your code in runAnimation reaches

  registerWrite(6, HIGH);

Because the bitsToSend starts with zero, bit 5 is now clear(low) again, as we set bit 6.

This is "simply" fixed by putting the word static in front. bitsToSend will now keep the value of 5 to what you set it to, until you explicitly reset it.

 static byte bitsToSend = 0;

This does not explain why the test loop "seems" to work, but I suspect you expected each port to turn on in sequence and missed that the only one port on at a time was turned on when your expectation (from the way I read your code) shoudl have been that more and more turned on.

I am at a loss and I must be doing something simple wrong. If we throw out the startAnimation function and just try to call the registerWrite function from the loop I still can't get it to work

This code still works:

for ( int bitToSet = 0; bitToSet <=8; bitToSet++) {
  registerWrite(bitToSet, HIGH);
  Serial/println(bitToSet);
  delay(1000);
}

If I try to call registerWrite outside of the for loop I imagine it should work the same. However it never triggers the relay that is attached.

registerWrite(5, High);
delay(1000);

If I write it and define an int first it still fails.

int z = 5;
registerWrite(5, High);
delay(1000);

Any thought on why that would be?

  • Josh

There are a number of typos in the code snippets in your last reply. It's OK, I hit he wrong key too sometimes. But in this case, as we're trying to help you with a coding issue, you must cut-N-paste the actual code you use, not retype it in the forum. Otherwise we would suggest fixes to something not present in your code, and we can't comment on that which you have not typed in.

So start a new tiny sketch, where you only use, say, pin 5 and your registerWrite routine. with/without a for loop or an oneline extra function.

Thank you for the feedback. Here is the compiled code as it is running on the Arduino. I hope there are enough comments in there to answer any questions.

Again, calling the function from the for loop works great, when calling from any other place i am unable to get the LEDs to light.

//Set up 74hc595
//Pin connected to latch pin (ST_CP) of 74HC595
const int latchPin = 8;
//Pin connected to clock pin (SH_CP) of 74HC595
const int clockPin = 12;
////Pin connected to Data in (DS) of 74HC595
const int dataPin = 11;

void setup() {
  //set pins to output because they are addressed in the main loop
  //This is the code for the 74hc595
  pinMode(latchPin, OUTPUT);
  pinMode(dataPin, OUTPUT);  
  pinMode(clockPin, OUTPUT);
  
  //Setup the Serial Connection
  Serial.begin(9600);
  Serial.println("reset");
}

void loop() {
     // This works in lighting up the LEDs in sequence, one at a time
     for ( int bitToSet = 0; bitToSet <=8; bitToSet++ ) {
         registerWrite(bitToSet, HIGH);
         Serial.println(bitToSet);
         delay(1000);
       }
     
     //Light up LED "3"
     // This does not work
     registerWrite(3, HIGH);
     Serial.println("set bit 3 ON");
     delay(1000);
     
     //Light up LED 5
     // This should turn off the non-lit 3 and turn on 5
     // This also does nothing
     registerWrite(5, HIGH);
     Serial.println("set bit 5 ON");
     delay(1000);
     
} // End Loop

void registerWrite(int whichPin, int whichState) {
  Serial.print("called registerWrite PIN - ");
  Serial.println(whichPin);
  byte bitsToSend = 0;

  // turn off the output so the pins don't light up
  digitalWrite(latchPin, LOW);

  // turn on the next highest bit in bitsToSend:
  bitWrite(bitsToSend, whichPin, whichState);
  Serial.print("bitsToSend - ");
  Serial.println(bitsToSend);

  // shift the bits out:
  shiftOut(dataPin, clockPin, MSBFIRST, bitsToSend);

  // turn on the output so the LEDs can light up:
  digitalWrite(latchPin, HIGH);
}

Once this understand why this is happening, I am hoping that I can re-write the function to turn on multiple bits at the same time. But, that is something for another post or day.

Thanks again, Josh

I'm confused by your rather complex code to address the 595 chip. Why not just use SPI? A single SPI transfer will do it. Check out my page here:

Example code to ripple out bits:

#include <SPI.h>
const byte LATCH = 10;
void setup ()
{
  SPI.begin ();
}  // end of setup

byte c;
void loop ()
{
  c++;
  digitalWrite (LATCH, LOW);
  SPI.transfer (c);
  digitalWrite (LATCH, HIGH);
  delay (20);
}  // end of loop

As others have pointed out, the 595 has 8 bits not 9:

     for ( int bitToSet = 0; bitToSet <=8; bitToSet++ ) {
                                    ^^^^^^^^