For loop works in loop() but not in setup()

Using Arduino Programming Questions
1

Hey guys!

As a class project, I'm trying to run through a for loop that changes all the lights on a model train track.

Now, when I run through a loop on the 'lights' array and call my 'changeLight()' method in the 'loop()' - it all works perfectly fine.

BUT when I do the exact same thing in my 'setup()', it doesn't work at all.
When doing 'Serial.print' I get all the correct values, it's like it just doesn't call the 'changeLight()' method.

Any ideas?

I hope it all makes sense, and please be gentle - I'm very new to all of this! :smiley:

Code:

#include "int.h"

#define controllerPin 7
#define triggerPin12 2
#define triggerPin13 3
#define triggerPin14 4
#define triggerPin15 5


unsigned char locomotiveAddress1;
unsigned char locomotiveAddress2;
unsigned char addr, data, addr2, data2;
unsigned int lights[] = {112, 152, 142, 102, 101, 141, 122, 121, 131, 132, 151, 111, 51, 21, 22, 62, 92, 52, 12, 32, 91, 61, 11, 41, 31, 81, 82, 42};
unsigned int switches[] = {252, 222, 250, 249, 221, 251, 224, 223, 234, 233, 231, 232, 241, 242, 243, 244};
unsigned int side = 249;

struct Train {
  int id;
  int data;
};

struct Light {
   int id;
   int col;
};

struct Switch {
  int id;
  int dir;
};


struct Train train1, train2;
struct Light light112, light152, light142, light102, light101, light141, light122, light121, light131, light132, light151, light111, light51, light21, light22, light62, light92, light52, light12, light32, light91, light61, light11, light41, light31, light81, light82, light42;
struct Light structLights[] = {light112, light152, light142, light102, light101, light141, light122, light121, light131, light132, light151, light111, light51, light21, light22, light62, light92, light52, light12, light32, light91, light61, light11, light41, light31, light81, light82, light42};
struct Switch switch252, switch222, switch250, switch249, switch221, switch251, switch224, switch223, switch234, switch233, switch231, switch232, xSwitch241, xSwitch242, xSwitch243, xSwitch244;
struct Switch structSwitches[] = {switch252, switch222, switch250, switch249, switch221, switch251, switch224, switch223, switch234, switch233, switch231, switch232, xSwitch241, xSwitch242, xSwitch243, xSwitch244};


void setup() {
  Serial.begin(9600); // monitor

  struct Train *train1Ptr = &train1;
  struct Train *train2Ptr = &train2;  
  struct Light* lightPtr = structLights;
  int *lightArrPtr = lights;
  struct Switch* switchPtr = structSwitches;
  int *switchArrPtr = switches;
  
  train1Ptr->id = 8;
  train2Ptr->id = 40;
  Serial.print("train id: ");
  Serial.println(train1Ptr->id);
  
  for(int i = 0; i < sizeof(structLights)/sizeof(structLights[0]); i++, lightPtr++, lightArrPtr++){
    lightPtr->id = *lightArrPtr;
    lightPtr->col = 1;
    if(lightPtr->id == 121 || lightPtr->id == 132 || lightPtr->id == 141 || lightPtr->id == 152){
      lightPtr->col = 0;
    }
    Serial.print("id: ");
    Serial.println(lightPtr->id);
    Serial.print("colour: ");
    Serial.println(lightPtr->col);
    changeLight(lightPtr->id, lightPtr->col);
  };

  for(int i = 0; i < sizeof(structSwitches)/sizeof(structSwitches[0]); i++, switchPtr++, switchArrPtr++){
    switchPtr->id = *switchArrPtr;
    switchPtr->dir = 1;
    if(switchPtr->id > 240 && switchPtr->id < 245){
      switchPtr->dir = 0;
    }
    Serial.print("id: ");
    Serial.println(switchPtr->id);
    Serial.print("direction: ");
    Serial.println(switchPtr->dir);

    changeSwitch(switchPtr->id, switchPtr->dir);
  };

  pinMode(controllerPin, OUTPUT);
  pinMode(triggerPin12, INPUT_PULLUP);
  pinMode(triggerPin13, INPUT_PULLUP);
  pinMode(triggerPin14, INPUT_PULLUP);
  pinMode(triggerPin15, INPUT_PULLUP);
  SetupTimer2();
  delay(50);


  delay(50);
  startTrain(train1Ptr->id, "fast"); // yderste
  delay(50);
  startTrain(train2Ptr->id, "fast"); // inderste
  delay(50);
  
}

void loop() {  
  /*
   if(side == 249){
    trigger(triggerPin12, train2->id, train1->id);
   }else if(side == 252){
    trigger(triggerPin15, train1->id, train2->id);
   }
   
   if(side == 249){
    triggerMid(triggerPin13);
   }else if(side == 252){
    triggerMid(triggerPin14);
   } 
   */
}

void triggerMid(int trigger) {
  while(true){
    if(digitalRead(trigger) == 0){
      if(side == 249){
        changeLight(light121.id, 1);
        delay(10000);
        changeLight(light121.id, 0);
        delay(2000);
        changeSwitch(side, 1);
        side = 252;
      }else if(side == 252){
        changeLight(light152.id, 1);
        delay(10000);
        changeLight(light152.id, 0);
        delay(2000);
        changeSwitch(side, 1);
        side = 249;
      }
      delay(3000);
      break;
    }
  }
}

void trigger(int trigger, int train1, int train2) {
  int counter = 0;
  while(true){
    if(digitalRead(trigger) == 0){
      counter++;
      if(counter == 1) {
        startTrain(train1, "slow");
        startTrain(train2, "slow");
        if(side == 249){
          changeLight(light141.id, 1);
        }else if(side == 252){
          changeLight(light132.id, 1);
        }
      } else if(counter == 2) {
        counter = 0;
        if(side == 249){
          startTrain(train2, "fast");
          changeLight(light121.id, 1);
          delay(3000);
          changeLight(light121.id, 0);
          changeSwitch(side, 0);
          changeLight(light141.id, 0);
        }else if(side == 252){
          startTrain(train1, "fast");
          changeLight(light152.id, 1);
          delay(3000);
          changeLight(light152.id, 0);
          changeSwitch(side, 0);
          changeLight(light132.id, 0);
        }
        break;
      }
      delay(3000);
    }
  }
}

void changeSwitch(unsigned int addrX, unsigned char valueX){
  calculateaddr(addrX, valueX);
  assemble_dcc_msg(addr, data);
  delay(50);
  assemble_dcc_msg(addr2, data2);
  delay(50);
}

void changeLight(unsigned int addrX, unsigned char valueX){
  calculateaddr(addrX, valueX);
  assemble_dcc_msg(addr, data);
  delay(50);
  assemble_dcc_msg(addr2, data2);
  delay(50);
}

void startTrain(unsigned char train, char s[]) {
  if(s == "fast"){
    assemble_dcc_msg(train,0x6a);
  }else if (s == "slow"){
    assemble_dcc_msg(train,0x62);
  }
}

void startTrain(unsigned char train) {
  assemble_dcc_msg(train,0x6a);
}

void stopTrain(unsigned char train) {
  assemble_dcc_msg(train,0x60);
}

void calculateaddr(unsigned int a, unsigned char b){
  unsigned int address, x, y;
  unsigned char reg, byte1, byte2;
  address = ((a / 4) + 1);
  reg = ((a % 4) - 1);
  if(reg == 255){
    address--;
    reg = 3;
  }
  
  byte1 = (address & 63) + 128;
  byte2 = 128;
  y = (address & 448) >> 6;
  x = 0;
  
  for(int i = 1;i < 8;i = i * 2)
    if((y & i) == 0)
      x = x + i;

  byte2 = byte2 + (x << 4);
  byte2 = byte2 + (reg << 1);
  
  if(b == 1){
    byte2 += 9;
  }
  else{
    byte2 += 8;
  }
  
  addr = byte1;
  data = byte2;
  addr2 = byte1;
  data2 = (byte2 - 8);
}

void sendbyte(unsigned char uch) {
 for(int a = 128; a > 0; a = a / 2) {
   if((uch & a) == 0)
    sendbit(0);
   else
    sendbit(1);
 }
}

void sendbit(int b) {
 if(b == 0) {
   digitalWrite(controllerPin, HIGH);
   delayMicroseconds(116);
   digitalWrite(controllerPin, LOW);
   delayMicroseconds(116);
 } else {
   digitalWrite(controllerPin, HIGH);
   delayMicroseconds(58);
   digitalWrite(controllerPin, LOW);
   delayMicroseconds(58);
 }
}
2

Your posted code has the for loops in setup(). Is this the example that works OK?

Can you show us a code example that doesn't work?

Also, let us know how it fails to work. Does it fail to compile? Or do something else that you don't want?

3

GypsumFantastic:
Your posted code has the for loops in setup(). Is this the example that works OK?

Can you show us a code example that doesn't work?

Also, let us know how it fails to work. Does it fail to compile? Or do something else that you don't want?

The above code is the code that doesn't work - adding the following code to the loop() would change every single light signal to 0 (in this case the color red):

for(int i = 0; i < 28; i++){
    changeLight(lights[i], 0);
};

Everything compiles as it should, it just doesn't change the light - there are no error messages of any kind.

4 
unsigned int lights[] = {112, 152, 142, 102, 101, 141, 122, 121, 131, 132, 151, 111, 51, 21, 22, 62, 92, 52, 12, 32, 91, 61, 11, 41, 31, 81, 82, 42};
unsigned int switches[] = {252, 222, 250, 249, 221, 251, 224, 223, 234, 233, 231, 232, 241, 242, 243, 244};

None of those value require that the array type be int.

struct Light light112, light152, light142, light102, light101, light141, light122, light121, light131, light132, light151, light111, light51, light21, light22, light62, light92, light52, light12, light32, light91, light61, light11, light41, light31, light81, light82, light42;
struct Light structLights[] = {light112, light152, light142, light102, light101, light141, light122, light121, light131, light132, light151, light111, light51, light21, light22, light62, light92, light52, light12, light32, light91, light61, light11, light41, light31, light81, light82, light42};

How does that fundamentally differ from

struct Light structLights[28];

You created 28 uninitialized instances of struct Light and then made the array contain all 28 of them. An array, sized properly, would contain n uninitialized instances of struct Light.

What, EXACTLY, does "doesn't work" mean? Smoke pouring out of the Arduino would be one definition, although I'd challenge that definition.

5

PaulS:

unsigned int lights[] = {112, 152, 142, 102, 101, 141, 122, 121, 131, 132, 151, 111, 51, 21, 22, 62, 92, 52, 12, 32, 91, 61, 11, 41, 31, 81, 82, 42};

unsigned int switches[] = {252, 222, 250, 249, 221, 251, 224, 223, 234, 233, 231, 232, 241, 242, 243, 244};



None of those value require that the array type be int.

struct Light light112, light152, light142, light102, light101, light141, light122, light121, light131, light132, light151, light111, light51, light21, light22, light62, light92, light52, light12, light32, light91, light61, light11, light41, light31, light81, light82, light42;
struct Light structLights[] = {light112, light152, light142, light102, light101, light141, light122, light121, light131, light132, light151, light111, light51, light21, light22, light62, light92, light52, light12, light32, light91, light61, light11, light41, light31, light81, light82, light42};



How does that fundamentally differ from

struct Light structLights[28];




You created 28 uninitialized instances of struct Light and then made the array contain all 28 of them. An array, sized properly, would contain n uninitialized instances of struct Light.

What, EXACTLY, does "doesn't work" mean? Smoke pouring out of the Arduino would be one definition, although I'd challenge that definition.

I'm creating the 28 uninitialized in order to get the correct name of every single light with it's id, in order to make it easier to find the correct light (it's on a 6x3m train track with ALOT of lights and switches).

The only issue I have is that the 'changeLight()' method doesn't change the color of the light (being the 2nd. parameter where 1 = green and 0 = red).

6

Wow, that's confusing code. What's the whole idea of using that many pointer stuff? Instead of sticking to more common array indexing?

And general rule of thumb, if you start numbering you're better of using arrays. Which you kind of do, but why define all the intermediate structs as well? You can just define 'structLughts' and 'structSwitches' directly.

And now the big part, that code isn't complete! 'assemble_dcc_msg' isn't defined so I can't check it. But the name implies it only assembles a message, not send it.

7

I completely forgot to mention - the only reason I'm using it, is because we HAVE to include structs and pointers... even though it would be a lot easier without :slight_smile:

Also, it's missing these two files - my bad!

#include "int.h"
#include <stdio.h>

#define TIMER_SHORT 0x8D                             // 58usec pulse length 141 255-141=114
#define TIMER_LONG  0x1B                             // 116usec pulse length 27 255-27 =228
#define DCC_PIN     7                                // DCC out
#define PREAMBLE    0                                // definitions for state machine
#define SEPERATOR   1                                // definitions for state machine
#define SENDBYTE    2                                // definitions for state machine
#define MAXMSG      2

bool second_isr              = false;                // pulse up or down
unsigned char last_timer     = TIMER_SHORT;          // store last timer value
unsigned char state          = PREAMBLE;
unsigned char flag           = 0;                    // used for short or long pulse
unsigned char preamble_count = 16;
int msgIndex                 = 0;  
int byteIndex                = 0;
unsigned char outbyte        = 0;
unsigned char cbit           = 0x80;

struct Message                                       // buffer for command
{
    unsigned char data[7];
    unsigned char len;
};

struct Message msg[MAXMSG] = 
{ 
    { { 0xFF,     0, 0xFF, 0, 0, 0, 0}, 3},          // idle msg
    { { 0xFF,     0, 0xFF, 0, 0, 0, 0}, 3}           // message 
}; 

void SetupTimer2(void)
{
    TCCR2A = 0; //page 203 - 206 ATmega328/P
    TCCR2B = 2; //Page 206
    TIMSK2 = 1<<TOIE2;   //Timer2 Overflow Interrupt Enable - page 211 ATmega328/P   
    TCNT2=TIMER_SHORT;   //load the timer for its first cycle
}

ISR(TIMER2_OVF_vect) //Timer2 overflow interrupt vector handler
{
    unsigned char latency;
  
    if (second_isr) 
    {  // for every second interupt just toggle signal
        digitalWrite(DCC_PIN,1);
        second_isr = false;    
        latency=TCNT2;    // set timer to last value
        TCNT2=latency+last_timer; 
    }
    else  
    {  // != every second interrupt, advance bit or state
        digitalWrite(DCC_PIN,0);
        second_isr = true;
        switch(state)  
        {
        case PREAMBLE:
            flag=1; // short pulse
            preamble_count--;
            if (preamble_count == 0)  
            {  
                state = SEPERATOR; // advance to next state
                msgIndex++; // get next message
                if (msgIndex >= MAXMSG)  
                {  
                    msgIndex = 0; 
                }  
                byteIndex = 0; //start msg with byte 0
            }
            break;
        case SEPERATOR:
            flag=0; // long pulse and then advance to next state
            state = SENDBYTE; // goto next byte ...
            outbyte = msg[msgIndex].data[byteIndex];
            cbit = 0x80;  // send this bit next time first
            break;
        case SENDBYTE:
            if ((outbyte & cbit)!=0)  
            { 
               flag = 1;  // send short pulse
            }
            else  
            {
                flag = 0;  // send long pulse
            }
            cbit = cbit >> 1;
            if (cbit == 0)  
            {  // last bit sent 
                byteIndex++;
                if (byteIndex >= msg[msgIndex].len) // is there a next byte?  
                {                                   // this was already the XOR byte then advance to preamble
                    state = PREAMBLE;
                    preamble_count = 16;
                }
                else  
                {  // send separtor and advance to next byte
                    state = SEPERATOR ;
                }
            }
            break;
        }   
 
        if (flag)  
        {  // data = 1 short pulse
            latency=TCNT2;
            TCNT2=latency+TIMER_SHORT;
            last_timer=TIMER_SHORT;
        }  
        else  
        {   // data = 0 long pulse
            latency=TCNT2;
            TCNT2=latency+TIMER_LONG; 
            last_timer=TIMER_LONG;
        } 
    }
}

void assemble_dcc_msg(unsigned char a, unsigned char b) 
{
   noInterrupts();  // make sure that only "matching" parts of the message are used in ISR
       msg[1].data[0] = a;
       msg[1].data[1] = b;
       msg[1].data[2] = (msg[1].data[0]^msg[1].data[1]);
       Serial.print("Byte1 : ");
       Serial.println(msg[1].data[0],HEX); //BIN
       Serial.print("Byte2 : ");
       Serial.println(msg[1].data[1],HEX); //BIN
 
   interrupts();
}

and

#ifndef int_h
#define int_h

#if ARDUINO >= 100
  #include "Arduino.h"
#else
  #include "WProgram.h"
  #include "pins_arduino.h"
  #include "WConstants.h"
#endif


extern void SetupTimer2(void);
ISR(TIMER2_OVF_vect);
extern void assemble_dcc_msg(unsigned char a, unsigned char b);  
#endif
8

the only reason I'm using it, is because we HAVE to include structs and pointers

Why? Is this some kind of school assignment?

9

PaulS:
Why? Is this some kind of school assignment?

Correct!
Both of my teachers have tried to solve this issue, and they are the ones who suggested I asked you guys :slight_smile:

10

Well, in setup() you have your for loop before your pinMode() calls...

11

westfw:
Well, in setup() you have your for loop before your pinMode() calls...

You. You just fixed everything.

Thanks a lot!

12

Tjofoed:
I completely forgot to mention - the only reason I'm using it, is because we HAVE to include structs and pointers... even though it would be a lot easier without :slight_smile:

Fair enough. But still think you overdo it a bit.

Also, does not correct the point Paul made.

Tjofoed:
I'm creating the 28 uninitialized in order to get the correct name of every single light with it's id, in order to make it easier to find the correct light (it's on a 6x3m train track with ALOT of lights and switches).

But you never use it in software... So makes it useless... And because you make a new copy you even can't use them. Just have that list for yourself. And if you really want it, an enum would be easier :slight_smile: Think a simplification like that should grand you a higher grade :wink:

13

You just fixed everything.

Cool. I have an intimate and ages-old familiarity with staring at the wrong part of non-working code. :slight_smile:

14