Issues using structure/array timings?

A little while back I was very kindly advised on here to implement structures in my code to make my code a lot simpler.

The aim was to have two LED’s that pulsed at different intensities and frequencies to indicate the distance and direction of three separate locations (for now anyway, its just to test the mechanics of this before I start fiddling with an actual IMU). Which is a lot of different delays to be keeping track of at once (for a newbie like me anyway!).

So I wrote this to make the rest of the code leaner:

/* Solitaire Project

    Build Version:      0.11
    Build Status:       Unfinished, created structure for all variables
    Build Objectives:   Use structures to create a streamlined method for independent flashes of various duty-cycles


*/

const int locationNumber = 3;                               //the number of given locations we're tracking, in this case 3
int locationDistance[] = {712, 104, 1107};                  //the distance in metres to each location
float locationBearing[] = {202, 172, 5};                    //the bearing of the locations relative to north
int userBearing;                                            //currently unused/set to 0, but will eventually be derived from a compass IMU bearing in relation to the location bearings to give real-time bearing updates relative to the user
int pulseTime = 100;                                        //the length in millis for any given pulse

struct locations {                                          //the structure array defining the variables that define the strength and frequency of a given locations pulse signature
  int locationDelay[locationNumber];                        //the delay between pulses that's derived from the locationDistance
  unsigned long previousMillis[locationNumber];             //previous amount of time between actions for each location
  bool pulseON[locationNumber];                             //the status of the pin, whether its ON or OFF
  int bearingStrength[locationNumber];                      //the PWM value for each output, derived from how close the user bearing (currently north) is to the locations bearing
  int pinNumber[locationNumber];

  int bearingStrengthTotal;                                 //the aggregate total of all of these outputs, used to define the final PWM value to the pin

} locations[] = {                                           //define the two types, the RIGHT-hand pin, and the LEFT-hand pin
  {{}, {}, {}, {}, 0, 0}, //RIGHT
  {{}, {}, {}, {}, 0, 0}, //LEFT
  /*
     ^^^ INDEX VALUES FOR ^^^
    {{0,0,0}, {0,0,0}, {0,0,0}, {0,0,0}, 0 }, //RIGHT
    {{0,0,0}, {0,0,0}, {0,0,0}, {0,0,0}, 0 }, //LEFT

  */
};


void setup() {
  Serial.begin (9600);                                                                    //begin serial
  for (int  thisSide = 0; thisSide <= 2; thisSide++) {
    for (int  thisLocation = 0; thisLocation < locationNumber; thisLocation++) {          //turn off all outputs
      locations[thisSide].pulseON[thisLocation] = false;
    }
  }
}

void loop() {
  for (int thisLocation = 0; thisLocation < locationNumber; thisLocation++) {                                                               //begin by pushing each location's bearing through an equation that defines it's individual PWM value for each side
    locations[0].bearingStrength[thisLocation] = ((float(255) / 100) * (100 - (locationBearing[thisLocation] / 180) * 100));
    if (locations[0].bearingStrength[thisLocation] <= 0) {
      locations[0].bearingStrength[thisLocation] = 0;
      locations[0].pinNumber[thisLocation] = 10;                                                                                            //also sets the pins for every location
    }
    locations[1].bearingStrength[thisLocation] = ((float(255) / 100) * ((((locationBearing[thisLocation]) - 180) / 180) * 100));            //there's two equations; if the bearing is >180, the LEFT hand pin has a PWM of 0 for that location, and vice versa
    if (locations[1].bearingStrength[thisLocation] <= 0) {
      locations[1].bearingStrength[thisLocation] = 0;
      locations[1].pinNumber[thisLocation] = 9;
    }
  }
  for (int  thisSide = 0; thisSide <= 2; thisSide++) {
    unsigned long currentMillis = millis();
    for (int thisLocation = 0; thisLocation < locationNumber; thisLocation++) {                                                                           //for each location, set the delay in millis to equal the distance in metres
      locations[thisSide].locationDelay[thisLocation] = locationDistance[thisLocation];
      if ((currentMillis - locations[thisSide].previousMillis[thisLocation] >= pulseTime) && (locations[thisSide].pulseON[thisLocation] == true)) {       //check if the pulse has run long enough
        locations[thisSide].pulseON[thisLocation] = !locations[thisSide].pulseON[thisLocation];                                                           //set the status of the pulse to OFF
        locations[thisSide].bearingStrengthTotal = locations[thisSide].bearingStrengthTotal - locations[thisSide].bearingStrength[thisLocation];          //minus the current PWM value for that location off the aggregate total for that side, effectively turning that pulse OFF
        if (locations[thisSide].bearingStrengthTotal <= 0) {
          locations[thisSide].bearingStrengthTotal = 0;                                                                                                   //if the result ends up less than 0, bump it back up to zero
        }
        locations[thisSide].previousMillis[thisLocation] = currentMillis;                                                                                 //set the time for that location
        analogWrite(locations[thisSide].pinNumber[thisLocation], locations[thisSide].bearingStrengthTotal);                                               //UPDATE THE PIN WITH THE NEW AGGREGATE TOTAL PWM VALUE OF ALL LOCATIONS FOR THE GIVEN SIDE
      }
      else if ((currentMillis - locations[thisSide].previousMillis[thisLocation] >= locations[thisSide].locationDelay[thisLocation]) && (locations[thisSide].pulseON[thisLocation] == false)) {         //check if the delay has run long enough
        locations[thisSide].pulseON[thisLocation] = !locations[thisSide].pulseON[thisLocation];                                                                                                         //set the status of the pulse to ON
        locations[thisSide].bearingStrengthTotal = locations[thisSide].bearingStrengthTotal + locations[thisSide].bearingStrength[thisLocation];                                                        //add the current PWM value for that location to the aggregate total for that side, effectively turning that pulse ON
        if (locations[thisSide].bearingStrengthTotal >= 255) {
          locations[thisSide].bearingStrengthTotal = 255;                                                                                                                                               //if the total goes above 255, keep it from going any higher than 255
        }
        locations[thisSide].previousMillis[thisLocation] = currentMillis;                                                     //set the time for each location
        analogWrite(locations[thisSide].pinNumber[thisLocation], locations[thisSide].bearingStrengthTotal);                   //UPDATE THE PIN WITH THE NEW AGGREGATE TOTAL PWM VALUE OF ALL LOCATIONS FOR THE GIVEN SIDE
      }
    }
  }
}

In the example using the current locationDistance & locationBearing variables, I’d expect my two LED’s to simultaneously show:

Location 1: 512 meters at 202° degrees: medium frequency, fairly faint pulse in PIN 10
Location 2: 104 meters at 172° degrees: very high frequency, very faint pulse in PIN 9
Location 3: 1107 meters at 5 degrees: very low frequency, very strong pulse in PIN 9

This would all be occurring at once, so some pulses would overlap, but that was by design.

Currently it doesn’t act in this way; the blinks & serial-values are far more seemingly random than expected, and it seems that they don’t act independently. I suspect that there’s an issue with my logic and that somehow the timings of one variable are affecting others, and having never used structures before, I’m not entirely convinced the way of initializing by leaving the brackets blank is correct; but other than that I got nothin’ ¯_(ツ)_/¯

If anyone’s got the faintest idea what’s going wrong with this I’d be eternally grateful, I’ve tried to not make the thing an absolute nightmare to read but apologies if it’s got a bit of the ‘Ulysses-factor’…
Cheers!

I found your code extremely difficult to read because of the way long lines of code and comments are all jumbled up. To my mind this re-formatted version is much easier to read, but could probably be improved further

/* Solitaire Project

    Build Version:      0.11
    Build Status:       Unfinished, created structure for all variables
    Build Objectives:   Use structures to create a streamlined method for independent flashes of various duty-cycles


*/

const int locationNumber = 3;                              
    //the number of given locations we're tracking, in this case 3
int locationDistance[] = {712, 104, 1107};                  
    //the distance in metres to each location
float locationBearing[] = {202, 172, 5};                    
    //the bearing of the locations relative to north
int userBearing;                                            
    //currently unused/set to 0, but will eventually be derived from a compass IMU bearing in relation to the location bearings to give real-time bearing updates relative to the user
int pulseTime = 100;                                        
    //the length in millis for any given pulse

struct locations {                                          
        //the structure array defining the variables that define the strength and frequency of a given locations pulse signature
    int locationDelay[locationNumber];                        
            //the delay between pulses that's derived from the locationDistance
    unsigned long previousMillis[locationNumber];             
            //previous amount of time between actions for each location
    bool pulseON[locationNumber];                             
            //the status of the pin, whether its ON or OFF
    int bearingStrength[locationNumber];                      
            //the PWM value for each output, derived from how close the user bearing (currently north) is to the locations bearing
    int pinNumber[locationNumber];

    int bearingStrengthTotal;                                 
        //the aggregate total of all of these outputs, used to define the final PWM value to the pin

} locations[] = {                                           
        //define the two types, the RIGHT-hand pin, and the LEFT-hand pin
    {{}, {}, {}, {}, 0, 0}, //RIGHT
    {{}, {}, {}, {}, 0, 0}, //LEFT
        /*
         ^^^ INDEX VALUES FOR ^^^
        {{0,0,0}, {0,0,0}, {0,0,0}, {0,0,0}, 0 }, //RIGHT
        {{0,0,0}, {0,0,0}, {0,0,0}, {0,0,0}, 0 }, //LEFT

        */
};


void setup() {
    Serial.begin (9600);                                                                    
        //begin serial
    for (int  thisSide = 0; thisSide <= 2; thisSide++) {
        for (int  thisLocation = 0; thisLocation < locationNumber; thisLocation++) {          
                //turn off all outputs
            locations[thisSide].pulseON[thisLocation] = false;
        }
    }
}

void loop() {
    for (int thisLocation = 0; thisLocation < locationNumber; thisLocation++) {
            //begin by pushing each location's bearing through an equation that defines it's individual PWM value for each side
        locations[0].bearingStrength[thisLocation] = ((float(255) / 100) * (100 - (locationBearing[thisLocation] / 180) * 100));
        
        if (locations[0].bearingStrength[thisLocation] <= 0) {
            locations[0].bearingStrength[thisLocation] = 0;
            locations[0].pinNumber[thisLocation] = 10;
                //also sets the pins for every location
        }
        locations[1].bearingStrength[thisLocation] = ((float(255) / 100) * ((((locationBearing[thisLocation]) - 180) / 180) * 100));  
                  
            //there's two equations; if the bearing is >180, the LEFT hand pin has a PWM of 0 for that location, and vice versa
            
        if (locations[1].bearingStrength[thisLocation] <= 0) {
            locations[1].bearingStrength[thisLocation] = 0;
            locations[1].pinNumber[thisLocation] = 9;
        }
    }
    
    for (int  thisSide = 0; thisSide <= 2; thisSide++) {
        unsigned long currentMillis = millis();
        for (int thisLocation = 0; thisLocation < locationNumber; thisLocation++) {
                //for each location, set the delay in millis to equal the distance in metres
            locations[thisSide].locationDelay[thisLocation] = locationDistance[thisLocation];
            
            if ((currentMillis - locations[thisSide].previousMillis[thisLocation] >= pulseTime) && (locations[thisSide].pulseON[thisLocation] == true)) {
                
                    //check if the pulse has run long enough
                locations[thisSide].pulseON[thisLocation] = !locations[thisSide].pulseON[thisLocation];
                    //set the status of the pulse to OFF
                locations[thisSide].bearingStrengthTotal = locations[thisSide].bearingStrengthTotal - locations[thisSide].bearingStrength[thisLocation];  
                        
                    //minus the current PWM value for that location off the aggregate total for that side, effectively turning that pulse OFF
                    if (locations[thisSide].bearingStrengthTotal <= 0) {
                        locations[thisSide].bearingStrengthTotal = 0;                                                                                                   //if the result ends up less than 0, bump it back up to zero
                    }
                locations[thisSide].previousMillis[thisLocation] = currentMillis;       
                    //set the time for that location
                analogWrite(locations[thisSide].pinNumber[thisLocation], locations[thisSide].bearingStrengthTotal); 
                    //UPDATE THE PIN WITH THE NEW AGGREGATE TOTAL PWM VALUE OF ALL LOCATIONS FOR THE GIVEN SIDE
            }
            else if ((currentMillis - locations[thisSide].previousMillis[thisLocation] >= locations[thisSide].locationDelay[thisLocation]) && (locations[thisSide].pulseON[thisLocation] == false)) { 
                        //check if the delay has run long enough
                locations[thisSide].pulseON[thisLocation] = !locations[thisSide].pulseON[thisLocation];
                
                    //set the status of the pulse to ON
                locations[thisSide].bearingStrengthTotal = locations[thisSide].bearingStrengthTotal + locations[thisSide].bearingStrength[thisLocation];
                    
                    //add the current PWM value for that location to the aggregate total for that side, effectively turning that pulse ON
                if (locations[thisSide].bearingStrengthTotal >= 255) {
                    locations[thisSide].bearingStrengthTotal = 255;
                      
                        //if the total goes above 255, keep it from going any higher than 255
                }
                locations[thisSide].previousMillis[thisLocation] = currentMillis;
                    //set the time for each location
                analogWrite(locations[thisSide].pinNumber[thisLocation], locations[thisSide].bearingStrengthTotal); 
                    //UPDATE THE PIN WITH THE NEW AGGREGATE TOTAL PWM VALUE OF ALL LOCATIONS FOR THE GIVEN SIDE
            }
        }
    }
}

It seems to me that the first FOR in loop() only deals with things that never change and should probably be in setup().

This line (in the second FOR)

unsigned long currentMillis = millis();

is almost certainly in the wrong place. I suspect it should be the first line in loop() so that the same value of currentMillis is used in all the tests.

It would be a big help if you could provide a description in English of how the code is intended to work

…R

I think that whoever advised on the use of a struct had more something like the below in mind

//the structure array defining the variables that define the strength and frequency of a given locations pulse signature
struct LOCATION
{
  int locationDelay;
  unsigned long previousMillis;
  bool pulseON;
  int bearingStrength;
  int pinNumber;
};

// array of locations
LOCATION locations[]
{
  {0, 0, 0, 0, 0},
  {0, 0, 0, 0, 0},
};

int bearingStrengthTotal;                                 //the aggregate total of all of these outputs, used to define the final PWM value to the pin

Many thanks Robin, I've taken that on board and am trying to use that style of formatting from now on!
Also moved that line, not sure how much it changed but at least now any issues are easier to identify.

And sterretje, massive thanks too, that way makes a LOT more sense, I wrote a new script using that and added an array for the bearingStrengthTotal to serve as a method of indexing so it could tell which pin to write to.

And it all worked! There's a couple of weird issues regarding the PWM value but hopefully it's a fairly small tweak needed and the rest of the script runs as intended!

Many thanks, learned a lot! :smiley: