For loops broken...or more to the point not breaking

Projects Programming
1

Hello everyone,

Having a problem that I'm very confused about, and would appreciate some advice on how to proceed. Some context on the code below. I'm making a signal light that lights up Red, Yellow or Green (like a traffic light). This code is intended to automatically set the brightness of each aspect by use of three on-board digital potentiometers which are dialled in using SPI. Currently this program is only attempting to do this for the Red aspect as I've hit a massive roadblock.

The core of the process happens in the function setAspect(), which contains a for loop intended to step down from 7 to 0. The i value however seems to get stuck on...58? The complete code is included below as I'm fully stumped by why I can't seem to get the most simple thing working.

Fixes I've tried so far -
Replacing the for loop with a while loop - Same issue, which seriously casts doubt on my ability to assign a value to a variable

Serial.println() the value of i at various points throughout the loop - Initially an attempt to monitor whether the value of i is somehow being accidentally changed. However sometimes (and only sometimes) the very act of trying to monitor the value of the variable causes it to just become what I think it should be (very odd)

Replacing the arduino board - Suspecting a hardware issue I tried flashing the code to a fresh Arduino Mega 2560 Rev3 (exact same model as the original). Issue persists across units implying it's definitely a software issue rather than some internal fault on my board.

Suffice it to say I'm at a loss, would appreciate any advice you guys could contribute. Code is included below alongside a screenshot of the serial monitor.

#include <SPI.h>

//Set desired luminence targets in cd for each aspect
//const float target_R = 1100;
//const float target_Y = 1675;
//const float target_G = 800;

//Temporary values

const float target_R = 5.3;
const float target_Y = 3.6;
const float target_G = 4.2;

void setup() {
  Serial.begin(9600);     //Set up serial comms with pc
  Serial1.begin(115200);  //Set up serial comms with spectrometer
  
  //Pins 11, 12 and 13 - Digital control for Red Yellow and Green aspects
  pinMode(11, OUTPUT);
  pinMode(12, OUTPUT);
  pinMode(13, OUTPUT);

  //Pins 8 9 and 10 are the chip select pins for the digipots
  pinMode(8, OUTPUT);
  pinMode(9, OUTPUT);
  pinMode(10, OUTPUT);

  //Setup SPI comms with unit under test
  SPI.begin();
  SPI.beginTransaction(SPISettings(100000, MSBFIRST, SPI_MODE0));

  //Set CS pins high to lock out all digipots until required;
  digitalWrite(8, HIGH);
  digitalWrite(9, HIGH);
  digitalWrite(10, HIGH);
  digitalWrite(11, LOW);
  digitalWrite(12, HIGH);
  digitalWrite(13, HIGH);

  //testCycle();
}

void loop() {
  //To do - Add code to wait for button input
  //wiperTest(8);
  setAspect('R');
  //testCycle();
  //Cusick's pseudocode

  /*
    Switch on red aspect
    wiper=0

    for(i=7, i>=0, i--){

      wiper = wiper | 1<<i;  //Sets leftmost (not touched) bit of wiper value to 1
      Send wiper value by SPI to digipot volatile wiper register

      Send info request to LC800 via RS232
      Store result
      Pluck out the desired lux value and store -> lux_R
      if lux_R < target_R{    //If light is too dim
      //We need to make it brighter, this is done by setting the current bit we're working on back to 0 (as reducing the wiper value increases intensity)
      wiper = wiper & ~(1<<i); 
      } 
    }

    Store wiper value to non-volatile register
    Probably put all of the above into a function called setAspect();

    Repeat the above for yellow and green aspect

  */
}

float readIntensity(){ // This code requests data from the LC800, processes it and returns a value of the detected lux value (or candela or whatever)
  bool valid = false;
  char Incoming[21];
  char Mantissa[4];
  char Exponent[2];
  float numb = 0.0;
  int ex = 0;
  
  while(valid==false){
    Serial1.write("MEA");  //Sends command - Request data

    delay(100);

    for (int x = 0; x < 21; x++) {
      Incoming[x] = Serial1.read(); //Stores the whole serial buffer in Incoming
    }

    //Checks the delimiting characters to ensure incoming data was received correctly
    if ((Incoming[1] == '.') && (Incoming[5] == 'E') && (Incoming[9] == ';') && (Incoming[11] == ';')) {
      valid = true;
    }
    else {
      valid = false;
    }
  }
  //When the loop breaks, data should have been received correctly. We can now begin processing the numbers
  
  //The below extracts the mantissa digits from the string, and saves them to a new string
  Mantissa[0] = Incoming[0];
  Mantissa[1] = Incoming[2];
  Mantissa[2] = Incoming[3];
  Mantissa[3] = Incoming[4];
  Mantissa[4] = ';'; //For some reason the program doesn't respect the length defined when the variable is declared, appending a bunch of random junk to the end. As such I make it deliberately too long and add a delimiting character ';' to the end to signify the end of the string

  //Said string is then converted to int datatype? also divided by 1000 to reintroduce the decimal point in the correct place
  numb = atof(Mantissa)/1000;

  Exponent[0] = Incoming[7];
  Exponent[1] = Incoming[8];
  Exponent[2] = ';';

  ex = atoi(Exponent);

  if(ex == 0){return numb;}
  else{
    if (Incoming[6] == ('-')) {  // checks the data in the Incoming for a negative power
      for(int i=0; i<atoi(Exponent); i++){  //Assign appropriate radix
        numb/=10;
      } 
    } 

    else if(Incoming[6] == "+"){
      for(int i=0; i<atoi(Exponent); i++){  //Assign appropriate radix
        numb/=10;
      }
    }
    return numb;
  }
}

void setWiper(uint8_t value, char aspect){
  int cspin;             //To do - Set cs pin dependant on aspect
  uint16_t command = value | 0x0000;   //Set command to write value to nv wiper register

  switch(aspect){
    case 'R':
    cspin = 8;
    break;

    case 'Y':
    cspin = 9;
    break;

    case 'G':
    cspin = 10;
  }

  //Transfer command to digipot via SPI
  digitalWrite(cspin, LOW);
  delay(100);
  SPI.transfer16(command);
  delay(100);
  digitalWrite(cspin, HIGH);
}

int readWiper(char aspect){
  int cspin;
  uint16_t command = 0x2FFF;   //Set command to read value from nv wiper register
  int value;

  switch(aspect){
    case 'R':
    cspin = 8;
    break;

    case 'Y':
    cspin = 9;
    break;

    case 'G':
    cspin = 10;
  }

  //Transfer command to digipot via SPI
  digitalWrite(cspin, LOW);
  delay(100);
  value = SPI.transfer16(command);
  delay(100);
  digitalWrite(cspin, HIGH);
  return value & 0xFF;
}

void setAspect(char aspect){
  float lux;
  float target;
  int cspin;
  int i;
    
    switch(aspect){
      case 'R':
        digitalWrite(11, LOW);
        digitalWrite(12, HIGH);
        digitalWrite(13, HIGH);
        target = target_R;
        cspin = 8;
        break;
      
      case 'Y':
        digitalWrite(11, HIGH);
        digitalWrite(12, LOW);
        digitalWrite(13, HIGH);
        target = target_Y;
        cspin = 9;
        break;

      case 'G':
        digitalWrite(11, LOW);
        digitalWrite(12, HIGH);
        digitalWrite(13, HIGH);
        target = target_G;
        cspin = 10;
    }

    uint8_t wiper=0;
    uint8_t bitmask=0;
    //uint16_t test = 0;
    
    for(int i=7; i>=0; i--){
      Serial.print("Now working on iteration ");
      Serial.println(i);

      bitmask = 1<<i;
      wiper |= bitmask;

      Serial.print("Testing wiper value ");
      Serial.println(wiper);

      //Write the wiper value to the digipot, so it can be tested
      setWiper(wiper, aspect);
      delay(1000);

      lux = readIntensity();
      Serial.print("Incident light: ");
      Serial.println(lux, 4);
      if (lux < target){    //If light is too dim
        //Reverse the change made to the wiper value
        wiper &= ~bitmask;

        Serial.print("Light too dim, setting wiper back to ");
        Serial.println(wiper);
      }
      Serial.println(); 
    }
  
    Serial.println("Value dialled in, writing to NV register: ");
    setWiper(wiper, aspect);
    Serial.println("Wiper value is now ");
    Serial.println(readWiper(aspect));
}

void wiperTest(int csPin){
  uint16_t DataRead;
  uint16_t command;

  for(uint16_t wiper=0;wiper<255;wiper+=100){
    command = wiper | 0x0000;
    Serial.print("About to send value: ");
    Serial.println(wiper);
    Serial.print("Command string looks like this: ");
    Serial.println(command);

    digitalWrite(csPin, LOW);
    delay(100);
    SPI.transfer16(command);
    delay(100);
    digitalWrite(csPin, HIGH);

    Serial.print("Okay, now reading that back, command string looks like this:");
    command = 0x0FFF;
    DataRead = 0xDEAD;
    Serial.println(command);

    digitalWrite(csPin, LOW);
    delay(100);
    DataRead = SPI.transfer16(command);
    delay(100);
    digitalWrite(csPin, HIGH);

    delay(300);
    Serial.println("And the result:");
    Serial.println(DataRead & 0XFF);
    delay(2000);
    Serial.println();
    Serial.println();
    Serial.println();
  }
}

void testCycle(){
  float intensity = 0.0;
  digitalWrite(11, LOW);
  digitalWrite(12, HIGH);
  digitalWrite(13, HIGH);
  Serial.println("Red");
  for(int j=0; j<10; j++){
    if(j%2 == 0){
      Serial.println("Even");
      setWiper(0, 'R');
    }else {setWiper(255, 'R');}
    intensity = readIntensity();
    Serial.print("Current detected luminance value is ");
    Serial.println(intensity, 3);
    delay(500);
  }
  Serial.println();
  Serial.println();
  delay(1800);

  digitalWrite(11, HIGH);
  digitalWrite(12, LOW);
  digitalWrite(13, HIGH);
  Serial.println("Yellow");
  intensity = readIntensity();
  Serial.print("Current detected luminance value is ");
  Serial.println(intensity, 3);
  Serial.println();
  Serial.println();
  delay(1800);

  digitalWrite(11, HIGH);
  digitalWrite(12, HIGH);
  digitalWrite(13, LOW);
  Serial.println("Green");
  intensity = readIntensity();
  Serial.print("Current detected luminance value is ");
  Serial.println(intensity, 3);
  Serial.println();
  Serial.println();
  delay(1800);
}

image
image338×620 2.59 KB

2

Welcome to the forum

As your topic does not relate directly to the installation or operation of the IDE it has been moved to the Programming category of the forum

3

Thanks Bob! And apologies for putting it in the wrong category.

Didn't spot the programming category when I was looking through them, thanks for moving it for me

4

The first thing that I would do is to remove any possible confusion regarding which of the i variables that your code uses in the setAspect() function

You have a local version of i, to which you do not assign a value

    int i;

and a second variable of the same name in the for loop

    for (int i = 7; i >= 0; i--)

The first one does not seem to be used in the function so delete it

5

You're quite correct

That local version of i is a hangover from when I replaced the for loop with a while loop in an attempt to fix. Just removed the unneeded declaration, but it didn't fix the issue

6

What values do you see when the for loop starts ?

7

in readIntensity Mantissa is declared as a char array of 4 elements which means valid indices are 0 through 3. The code writes to index 4 which is one byte past the end of the array. That extra write lands on whatever memory happens to sit right after Mantissa on the stack.

The same thing happens with Exponent which is a 2 elements array and you write ay index 2 which is beyond the array’s end.

This likely corrupts other parts of the code in harmful ways…

(If you want a char array to be a cString you need a trailing null char and the memory for that null char)

8

The first iteration is 7, as it should be. All subsequent iterations are 58

9

In addition to what others have observed,

   for (int x = 0; x < 21; x++) {
      Incoming[x] = Serial1.read(); //Stores the whole serial buffer in Incoming
    }

This is not gathering your incoming string correctly. Serial.read() does not wait for characters, instead it returns -1. You'll have to check Serial.available() before doing your read.

10

And ASCII code for the ';' semicolon, which you write outside the array is 59 - which after the decrementation in

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

is exactly the "strange value" 58

11

Also gcc compiler is allowed to optimise your code, so it may inline functions where it see it to fit and reorder variables in memory (and also keep variable in registers only, so it does not hit memory at all), which in correct program does not change the result.

But writing out of bound index (like Mantissa[4]) is UB=Undefined Behavior and it is Big Bad, because compiler is allowed (by the ANSI C++ definition) to do anything, even nasal demons

So having the loop index i just after end of Mantissa or Exponent is totally normal and sane behavior. Writing semicolon inside than have interesting result. (still beter, than demons, needed to say :slight_smile: )

12

This is a good suggestion! I had suspected some sort of memory corruption but couldn't fathom where it might be coming from. I've said it before, zero indexing will be the death of me.

Increased the bounds of both arrays and now the whole thing works just fine!

13

I guess you mean '+'.

14

Second guessing how long it takes for the data to be in the Serial buffer is not the proper way to read the serial answer

You should remove the delay and just have a waiting loop checking available() and reading bytes as they come.

I would suggest to study Serial Input Basics to handle this