for loop running infinitely

Question

void writereg() {
 digitalWrite(latchPin, LOW);
 for (int i = 7; i >= 0; i--) {
 digitalWrite(clockPin, LOW);
 Serial.print("wswitch&&&&&&");
 Serial.print(i);
 Serial.print("----------------------");
 digitalWrite(dataPin, switches[i].stat);
 Serial.println(switches[i].stat);
 digitalWrite(clockPin, HIGH);
 }
 digitalWrite(latchPin, HIGH);
 touched = false;
}

I don't understand what's wrong with what I'm doing here, but this for loop runs into an almost infinite loop and prints crazily. I'm a newbie to electronics and Arduino and I don't understand what's wrong here. Please help.

This is my switch class:

struct Swtch
{
 int num;
 bool stat;
 bool touched;
};
Swtch switches[8] = {{1, 0, 0}, {2, 0, 0}, {3, 0, 0}, {4, 0, 0}, {5, 0, 0}, {6, 0, 0}, {7, 0, 0}, {8, 0, 0}};

enter image description here

• jfpoilpret: From the output, it looks like i is not an int but a byte (or unsigned char or uint8_t). In this case, what you observe is normal because an unsigned is always positive, hence the loop never ends. Are you sure the output posted here matches the code in your question?
• bukke hari prasad: Yes. Its the same code that produces this output. i is an int only and not a byte.
• jfpoilpret: Could you show how switches was declared (and details of the underlying type)?
• bukke hari prasad: i have edited the question. Please see and let me know if it can solve the issue im facing.
• jfpoilpret: Thanks, unfortunately, that did not show any potential issue of memory overwriting (which is what I thought of when I asked you to add this code). Sorry to insist, but I am really surprised that i is an int. Just in case, do you have special compilation options?

Answer 1

After a chat with OP, it turned out this was a more complex issue, probably a memory overrun.

This cannot be seen in this small snippet, but the whole program is using memory extensively (global variables and dynamic memory heap allocation).

Heavy use of String variables is a likely explanation for the observed behavior. Also a lot of string literals are putting more pressure on memory in the OP program.

Here are my suggestions:

1. Replace all "..." string literals with flash-stored strings with F("..."). That will reduce the initial SRAM size used.
2. Replace all String variables with pure C-string (char* and char arrays) and use standard C string functions such as strcpy, strcat, strlen, strcmp, strchr... This forces to pre-allocate string to their maximum size, but this can be controlled at compile-time (for string variables defined as global). For this point, I also suggest to read a good tutorial on strings in C language.
3. Use const whenever possible (instead of variables that waste memory)
4. Use the smallest type needed for each variable, e.g. avoid int (2 bytes) when you need to store a boolean value, prefer bool type (only one byte) or even use 1 byte to store 8 boolean values and use boolean logic to manipulate each value.

• bukke hari prasad: Thanks and i would make all the changes that you suggested. Thanks a lot!

Answer 2

Like jfpoilpret said, it looks like i is being treated as an unsigned char or uint8_t causing it to never go below zero. Instead it wraps around to 255, the max value of an 8-bit unsigned integer, as you can see in line 9 of your output. The condition to remain in the loop is i >= 0 which means the loop will never exit. A solution is to add a case for if i is above the starting value:

for (int i = 7; i >= 0 && i <= 7; i--) {

I would recommend finding out why int is being treated as uint8_t for any future projects

• bukke hari prasad: I tried this too. But it didnt work.

Answer 3

I have noticed that you have incorrectly declared the switches array.

Your code:

struct Swtch
{
 int num;
 bool stat;
 bool touched;
};
Swtch switches[8] = {{1, 0, 0}, {2, 0, 0}, {3, 0, 0}, {4, 0, 0}, {5, 0, 0}, {6, 0, 0}, {7, 0, 0}, {8, 0, 0}};

It should be:

struct Swtch
{
 int num;
 bool stat;
 bool touched;
};
struct Swtch switches[8] = {{1, 0, 0}, {2, 0, 0}, {3, 0, 0}, {4, 0, 0}, {5, 0, 0}, {6, 0, 0}, {7, 0, 0}, {8, 0, 0}};

Alternatively, you could make a typedef struct:

typedef struct
{
 int num;
 bool stat;
 bool touched;
}Swtch;
Swtch switches[8] = {{1, 0, 0}, {2, 0, 0}, {3, 0, 0}, {4, 0, 0}, {5, 0, 0}, {6, 0, 0}, {7, 0, 0}, {8, 0, 0}};

References:
https://stackoverflow.com/a/32185804/6075112
https://overiq.com/c-programming/101/array-of-structures-in-c/#initializing-array-of-structures https://stackoverflow.com/a/3275389/6075112

• Carsten S: This would be true for C, not for C++, and I do not know in what language the code in the question is.
• vurp0: The Arduino development tools by default use C++.
• sa_leinad: Isn't the typedef the best practice?
• cHao: @sa_leinad: No. In C++, it's generally considered quite ugly, as there's no good reason for it. (C++ lets you treat user-defined type names like built-in type names in nearly all cases.)
• sa_leinad: Thanks all for your comments. I have learnt something today!