loop() isn't looping

Using Arduino Programming Questions
1

I'm confused. I'm building an intervalometer for my camera but the loop() function isn't looping. I added in the first and last lines for debugging. It prints "the end" signifying the first loop has completed but never prints "the beginning" meaning that loop() does not restart. loop() works properly on a smaller sample sketch so I don't think the problem is hardware or setup related. Using an Arduino Uno R3. Anyone know what might be going on?

void loop() {	
	lcd.setCursor(0,1); lcd.print("the beginning");

	// Get user input
	int exposureCount = exposureCountSelect(); 	// exposure count
	int exposureTime = exposureSelect();		// exposure time in seconds
	int intervalTime = intervalSelect(exposureTime);	// delay time in seconds
	if( userWait(exposureTime, exposureCount, intervalTime, "Press to start. ") != 0 ) { return; } // wait for user to start
		
	// Time to take photos
	unsigned long startTime = millis();
	unsigned long endTime = ((exposureCount*intervalTime)+startTime);	
	for( int i=1; i <= exposureCount; i=i+1 ) {
		if( takePhoto(exposureCount, exposureTime, intervalTime, i, startTime, endTime) != 0 ) { return; }
	}
	userWait(exposureTime, exposureCount, intervalTime, "Frames complete!"); // Wait for user to acknowledge completion

	lcd.clear(); lcd.print("the end");
	lcd.setCursor(0,1); lcd.print(freeMemory()); delay(3000);
}

I was able to narrow it down somewhat. When I commented takePhoto() out of loop() then it looped properly. Probably still some bugs in takePhoto() but I don't see anything that would cause loop() to break. I tried commenting out parts of takePhoto() to narrow it down. It doesn't seem like there's any single line causing a problem, but when I add any 2 or 3 of the lines in together then loop() stops working. freeMemory() says I have over ~1500 bytes available both inside takePhoto() and at the end. The cameraSnap() subfunction works properly in another test sketch.

int takePhoto(int exposureCount, int exposureTime, int intervalTime, int currentCount, unsigned long startTime, unsigned long endTime) {
	char string1[3];
	
	// Write to LCD
	lcd.clear();
	lcd.setCursor(0,0); lcd.print("Frame ");
	lcd.setCursor(6,0); sprintf(string1,"%3d",currentCount); lcd.print(string1);
	lcd.setCursor(9,0); lcd.print("/");
	lcd.setCursor(10,0); sprintf(string1,"%3d",exposureCount); lcd.print(string1);
	lcd.setCursor(4,1); lcd.print("minutes left.");

	// If we're in bulb mode, two IR clicks are required. Otherwise just one.
	analogWrite(redLed, backlightLevel);
	unsigned long initialTime = millis();
	cameraSnap();
	while( millis() < ((exposureTime*1000ul)+initialTime) ) { 
		lcd.setCursor(0,1); sprintf(string1,"%3d",(((endTime-millis())/60000)+1)); lcd.print(string1);
		if( readButton() == 2 ) { if( cancelSelect() != 0 ) { break; } }
	}
	if ( exposureTime != 0 && exposureTime != 1800 ) { cameraSnap(); }
	digitalWrite(redLed, LOW);
	while( millis() < ((intervalTime*1000ul)+initialTime) ) {
		if( currentCount == exposureCount ) { break; };
		lcd.setCursor(0,1); sprintf(string1,"%3d",(((endTime-millis())/60000)+1)); lcd.print(string1);
		if( readButton() == 2 ) { if( cancelSelect() != 0 ) { return 1; } }
	}
	lcd.clear(); lcd.print("inside takePhoto");
	lcd.setCursor(0,1); lcd.print(freeMemory()); delay(3000);
	return 0;
}
2

In that function you have string1 defined as 3 elements wide. But then you sprintf into it with %3d so you're asking to put in 3 characters. The string needs a terminating null at the end, so it has to be 4 characters wide to hold a 3 digit number like that.

3

And please, one statement per line. When you bunch them together like that it makes the code hard to read which can make it hard to help you. Lines of text don't cost any money, so when you make a semicolon, but enter and put the next statement on the next line.

4

Delta_G:
And please, one statement per line. When you bunch them together like that it makes the code hard to read which can make it hard to help you. Lines of text don't cost any money, so when you make a semicolon, but enter and put the next statement on the next line.

Especially since it makes no difference to the final result inside your Arduino.
In the context of an Arduino program, line breaks are for people: the computer doesn't need them.

5

You have a while loop in takePhoto() - that's basically doing the same thing as delay(),
so you need to do things differently if you want loop() to be responsive to other things.

blinkWithoutDelay example is the first place to go for this if you've not seen it already,
use tests on the value of millis() within loop(), not delay() or while loops() in other
functions.

6

I think it's appropriate to busywait while holding the shutter open for a picture - I reckon that's a rather time-sensitive operation?

In any case, I think Delta_G hit the nail on the head - you need to leave space one extra character on the end of a char array (c string) for the null to terminate the string.

And putting multiple statements on one line is terrible, terrible practice, as it serves only to make your code harder to read, and bugs harder to spot.

7

Thanks everyone. I changed the char definition as suggested and now everything is working. Still a little confused as to why... takePhoto() (as well as several other functions with the wrong char definition) seemed to work properly despite the wrong definition and the sketch did not get stuck until the end of loop() was reached. But at least it's working. Sorry I did not think about leaving multiple commands on the same line when I posted earlier. I actually find it easier to read when similar commands are grouped.

8

Because sprintf was putting the null character there even though there wasn't room for it. That means it was writing a 0 to some memory location that some other thing may have been using. When you write to memory you don't own, the results are undefined. That means anything can happen and there is no use trying to explain what actually did.

9

Well, it's not mysterious - once memory has been scribbled over, it would go unnoticed until the code got to somewhere where it tried to read that piece of memory, and found a value there that caused to to behave badly. How long that would take varies...

10

What makes this particular bug so nasty is that if you come in and change some other part of the code then it may change the way stuff is layed out in memory and cause the program to hang in a different place or even some other unrelated symptom.

11

Correct way to do things like this

…
	lcd.setCursor(0,0); lcd.print("Frame ");
	lcd.setCursor(6,0); sprintf(string1,"%3d",currentCount); lcd.print(string1);
	lcd.setCursor(9,0); lcd.print("/");
	lcd.setCursor(10,0); sprintf(string1,"%3d",exposureCount); lcd.print(string1);
	lcd.setCursor(4,1); lcd.print("minutes left.");
…

is like this:

  void printAt(int x, int y, char *s) {
    lcd.setCursor(x, y);
    lcd.print(s);
  }

  void printAt(int x, int y, int n) {
    sprintf(string1,"%3d", n);
    printAt(x, y, string1);
  }

…
	printAt(0, 0 ,"Frame ");
	printAt(6, 0, currentCount);
	printAt(9, 0, "/");
	printAt(10, 0, exposureCount); 
	printAt(4,1, "minutes left.");
        // etc
…

Also, consider using itoa rather than sprintf. See point 13 in the sticky post Read this before posting a programming question ... - Programming Questions - Arduino Forum .

Oh and yeah - you need to make room for a nul terminator.

Alternatively, you can re-write printAt to work more directly:

  // print a three-digit non-negative number with leading zeros.
  void printAt(int x, int y, int n) {
    lcd.setCursor(x, y);
    lcd.print((char)('0' + (n/100)%10));
    lcd.print((char)('0' + (n/10)%10));
    lcd.print((char)('0' + (n/1)%10));
  }

Because you have pulled the common code out into a function, making this kind of change becomes easy.

12