Why are these additions stopping the loop?

@tom321 has been helping me troublshoot some code and we're at a section that we can't seem to resolve and could use another set of eyes.

Why are these additions stopping the loop?
additions are in lines 79, 123, 248 -259

Code below:

/*
  Christmas Light Controller & Real Time Frequency Analyzer based on FHT code by Open Music Labs at http://openmusiclabs.com

  Then modified by PK from : https://dqydj.com/build-your-own-real-time-frequency-analyzer-and-christmas-light-controller/

  Modified by dimecoin: https://github.com/dimecoin/XmasFHT

  Requires FHT library, from here:
    http://wiki.openmusiclabs.com/wiki/ArduinoFHT
*/

/////////////////////////////////////////////////////////////////////
// additions line 79, 132, 248 - 259
/////////////////////////////////////////////////////////////////////

// Adjust the Treshold - what volume should make it light up?
#define THRESHOLD 35

// Old way if you want to statically set this.
// Attempt to 'zero out' noise when line in is 'quiet'.  You can change this to make some segments more sensitive.
// defaults:
// { 100, 81, 54, 47, 56, 58, 60, 67 };
//int oct_bias[] = { 136, 107, 44, 47, 56, 58, 60, 77 };

// New Auto calibration.
uint8_t oct_bias[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
uint16_t cal_bias[] = { 0, 0, 0, 0, 0, 0, 0, 0 };

/* Number of times to sample the "natural noise" on wires to get average.
   This average is used to cancel out noise while running.
   Don't call to many times or will be slow to startup.
   Dont' call over 16777215 or so times or it might overflow (plus would take forever to startup).
  ie. 256 (max reading) * CAL_TIME needs to be <= (2^32)-1 (size in bits of unint16_t)
*/

#define CAL_TIME 100

// Divide Threshold by 2 for top octave? 1 - yes 2 - no.  Makes highest frequency blink more.
#define TOP_OCTAVE_DIVIDE false

// This is for ACTIVE HIGH relays (works with LEDS for testing), switch values if you have ACTIVE LOW relays.
#define ACTIVE_ON LOW
#define ACTIVE_OFF HIGH

// enable for serial mode output, comment out to speed up lights
#define DEBUG

// Timer/delay for self test on startup.
#define SELFTESTTIME 100

/////////////////////////////////////////////////////////////////////
// Hard Customizations - know what you are doing, please.
/////////////////////////////////////////////////////////////////////
// FHT defaults - don't change without reading the Open Music Labs documentation at openmusiclabs.com
#define LOG_OUT 1   // use the log output function
#define FHT_N 256   // set to 256 point fht
#define OCTAVE 1
#define OCT_NORM 0

// include the library, must be done after some of the aboves are defined.. (required by FHT, won't work if included in wrong order)
#include <FHT.h>

// Delay - defines how many cycles before the lights will update.  OML's algorithm at 256 samples (needed for our 8 octaves) takes
// 3.18 ms per cycle, so we essentially throw out 14 cycles (I used mechanical relays, you can lower this for solid state relays).
// 15 cycles = 47.7 ms update rate.  Be careful here and don't change it too quickly!  I warned you!
// Default is 15
#define DELAY 15

// Don't change NUM_PINS.  FHT outputs 8 octs.
#define NUM_PINS 8
// Pin configuration, there is only 8 channels here.  Add duplicate entries if you don't have 8 lights, must be 8!
int relayPins[] = { 2, 3, 4, 5, 6, 7, 8, 9 };

uint8_t x[NUM_PINS];


void frequencyGraph(uint8_t x[], int size);

//++++++++++++++++++++++++++++++++ addition  #1 +++++++++++++++++++++++++
float Val;
//+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++




//================================================================
void setup() {

  // pin setup
  for (int i = 0; i < NUM_PINS; i++) {
    pinMode(relayPins[i], OUTPUT);
    digitalWrite(relayPins[i], ACTIVE_OFF);
  }

  // quick self test
  for (int i = 0; i < 2; i++) {
    for (int j = 0; j < NUM_PINS; j++) {
      digitalWrite(relayPins[j], ACTIVE_ON);
      delay(SELFTESTTIME);
      digitalWrite(relayPins[j], ACTIVE_OFF);
    }
  }

#ifdef DEBUG
  Serial.begin(115200);
  while (!Serial) {
  };
#endif

  TIMSK0 = 0;   // turn off timer0 for lower jitter
  ADCSRA = 0xe5;    // set the adc to free running mode

  // This is setting up A0 - dime
  ADMUX = 0x40;   // use adc0
  DIDR0 = 0x01;   // turn off the digital input for adc0

}
//==============================  setup end  =========================================
/**********************************************************************************
  Loop - includes initialization function and the full loop

**********************************************************************************/

void loop() {

  // True full loop
  int q = 0;
  int cal = 0;


  //+++++++++++++++++++++++++++++  addition #2  +++++++++++++++++++++++++++++++++++++++++++++++++++
  Val = getVal(); //
  //++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++


  while (1) {   // reduces jitter

    cli();    // UDRE interrupt slows this way down on arduino1.0

    for (int i = 0; i < FHT_N; i++) { // save 256 samples
      while (!(ADCSRA & 0x10)) ;  // wait for adc to be ready
      ADCSRA = 0xf5;  // restart adc

      // This is his way of reading Analog 0 (A0).  It pulls in L[ow] and H[igh] bit. - dimecoin
      byte m = ADCL;  // fetch adc data
      byte j = ADCH;

      int k = (j << 8) | m; // form into an int
      k -= 0x0200;  // form into a signed int
      k <<= 6;  // form into a 16b signed int
      fht_input[i] = k; // put real data into bins
    }

    fht_window(); // window the data for better frequency response
    fht_reorder();  // reorder the data before doing the fht
    fht_run();  // process the data in the fht
    fht_mag_octave(); // take the output of the fht

    sei();

    // We are in calibration mode.
    if (cal < CAL_TIME) {

      for (int i = 0; i < NUM_PINS; ++i) {
        cal_bias[i] += fht_oct_out[i];
      }

#ifdef DEBUG
      Serial.print(F("Calibrating "));
      Serial.print(cal);
      Serial.print(F("/"));
      Serial.println(CAL_TIME);
#endif

      cal++;
      continue;
    }
    // Calibration mode has just ended, crunch data collected.
    if (cal == CAL_TIME) {

      for (int i = 0; i < NUM_PINS; ++i) {
        oct_bias[i] = (uint8_t) (cal_bias[i] / CAL_TIME);
      }

#ifdef DEBUG
      Serial.println(F("--------------------------------------"));
      Serial.println(F("Done with Cal"));

      for (int i = 0; i < NUM_PINS; ++i) {
        Serial.print(oct_bias[i]);
        Serial.print(" ");
      }

      Serial.println(F(""));
      Serial.println(F("--------------------------------------"));

      for (int i = 0; i < NUM_PINS; ++i) {
        Serial.print(fht_oct_out[i] - oct_bias[i]);
        Serial.print(F(" "));
      }
      Serial.println(F(""));
      Serial.println(F("--------------------------------------"));

      Serial.flush();

#endif

      // Ready signal.
      for (int i = 0; i < NUM_PINS; i++) {
        digitalWrite(relayPins[i], ACTIVE_ON);
      }
      for (int i = 0; i < NUM_PINS; i++) {
        digitalWrite(relayPins[i], ACTIVE_OFF);
      }

      cal++;
      continue;
    }
    // Normal play mode

    if (q % DELAY == 0) {

      for (int i = 0; i < NUM_PINS; i++) {
        x[i] = fht_oct_out[i] - oct_bias[i];
      }

      frequencyGraph(x, NUM_PINS);

#ifdef DEBUG
      for (int i = 0; i < NUM_PINS; ++i) {
        Serial.print(x[i]);
        Serial.print(F(" "));
      }
      Serial.println(F(""));
#endif

    }

    ++q;
  }

}
//================================= loop end =====================================




//++++++++++++++++++++++++++++++++++++++ addition #3 ++++++++++++++++++++++++++++++++++++++


float getVal()
{
  int Val = analogRead(A2);
  Val = map(Val, 0, 1023, 1, 255);
  //  Serial.print( " Val  ");
  // Serial.println(Val);
}

//+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++



void frequencyGraph(uint8_t x[], int size) {

  int top_threshold = THRESHOLD; 
  //int top_threshold = Val;
  for (int i = 0; i < size - 1; i++) {
    x[i] = max(x[i], 0);

    // Special logic for last pin
    if (TOP_OCTAVE_DIVIDE && i == (size - 1)) {
      top_threshold /= 2;
    }

    if (x[i] >= top_threshold) {
      digitalWrite(relayPins[i], ACTIVE_ON);
    } else if (x[i] < top_threshold) {
      // && digitalRead(relayPins[i]) == ACTIVE_ON ) {
      digitalWrite(relayPins[i], ACTIVE_OFF);
    }

  }

}

What evidence do you have that the loop is stopping?

This function does not return any value. It also creates a local variable Val which has nothing to do with your global variable Val

they are definitely not stopping the loop

79 is just declaring a variable
123 is empty may be you meant 132 but that's OK to call a function

248 -259 is just a function definition... that does not honour the promise to return a float... so that's UB (undefined behavior)

try

float getVal() {
  return map(analogRead(A2), 0, 1023, 1, 255); 
}

or better (as it's not a float)

int getVal() {
  return analogRead(A2) >> 2;
}

// True full loop
int q = 0;
int cal = 0;

Try making these Global variables .

This

while (1) {   // reduces jitter

and no sign of any break means you aren't looping using loop().

Which is what I can see through the tiny window. Does it matter? Can you not use loop() to do the looping, as it is designed to do?

Again through the tiny window it looks like a few changes would eliminate the need to while yourself in the loop function.

And elaborate on the comment "reduces jitter" please. All you saving is a tiny bit of overhead the build in loop mechanism takes to get around to calling loop)( again after you return from it. If you did.

a7

You're spending one helluva LONG time with interrupts disabled...

The serial Monitor doesn't post what it should and the LED's on the relay don't respond to audio - suggesting the program is stalling some where.

sorry, I'm still a novice, what are you suggesting?

Ok, I tried the following, but only get a blank serial monitor, nothing posts.

//++++++++++++++++++++++++++++++++++++++ addition #3 ++++++++++++++++++++++++++++++++++++++


float getVal() {
  return map(analogRead(A2), 0, 1023, 1, 255); 
  Serial.print( " Val  ");
  Serial.println(Val);}


//+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

and tried the following but also got an empty serial monitor.

//++++++++++++++++++++++++++++++++++++++ addition #3 ++++++++++++++++++++++++++++++++++++++


int getVal() {
  return analogRead(A2) >> 2;
  Serial.print( " Val  ");
  Serial.println(Val);}


//+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

I made int q = 0; int cal = 0; global, but the serial monitor remains blank - program is stalled.

I don't know why the original programmer opted to use "while" nor do I know what "jitter" he's speaking of. I've just been trying to preserve as much of the original working code as I can. I've never used the while function before, so I'm open to suggestions on changing it. What overhead is it saving? I tried commenting it out, but received a handful of errors.

What's an interrupt? is this an interrupt #ifdef DEBUG?
I'm open to suggestions.

did you keep the debug mode ? are you seeing anything else in the monitor?

Yes, I kept the debug mode on #define DEBUG. Nothing appears in the serial monitor.

Inside your getVal() function, you are calling AnalogRead(A2) which changes the values of several of the registers you manually set up such as ADMUX so you won't be reading A0 after that. That is probably why things are not working.

ohhh, interesting.
how can I get around that then? I'd like to prove this out.

I guess that you have taken this code from somewhere without knowing what the code is doing.

This code uses a lot of advanced programming techniques and re-configures internal hardware of the microcontroller. Without knowing how this all works you will have a hard time.

So my suggestion is post a link to a video that shows what the code is doing with the lights.
There is a chance that the same - or at least a similar effect can be done with a different code that is easier to work with

best regards Stefan

1 Like

Let start over
this are two codes to be combined
A

/* 
* Christmas Light Controller & Real Time Frequency Analyzer based on FHT code by Open Music Labs at http://openmusiclabs.com
* 
* Then modified by PK from : https://dqydj.com/build-your-own-real-time-frequency-analyzer-and-christmas-light-controller/
*
* Modified by dimecoin: https://github.com/dimecoin/XmasFHT
*
* Requires FHT library, from here: 
*   http://wiki.openmusiclabs.com/wiki/ArduinoFHT
*/

/////////////////////////////////////////////////////////////////////
// Easy Customizations
/////////////////////////////////////////////////////////////////////

// Adjust the Treshold - what volume should make it light up?
#define THRESHOLD 35

// Old way if you want to statically set this.
// Attempt to 'zero out' noise when line in is 'quiet'.  You can change this to make some segments more sensitive.
// defaults: 
// { 100, 81, 54, 47, 56, 58, 60, 67 };
//int oct_bias[] = { 136, 107, 44, 47, 56, 58, 60, 77 };

// New Auto calibration.
uint8_t oct_bias[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
uint16_t cal_bias[] = { 0, 0, 0, 0, 0, 0, 0, 0 };

/* Number of times to sample the "natural noise" on wires to get average.
 * This average is used to cancel out noise while running.
 * Don't call to many times or will be slow to startup.  
 * Dont' call over 16777215 or so times or it might overflow (plus would take forever to startup).
  ie. 256 (max reading) * CAL_TIME needs to be <= (2^32)-1 (size in bits of unint16_t)
*/

#define CAL_TIME 100

// Divide Threshold by 2 for top octave? 1 - yes 2 - no.  Makes highest frequency blink more.
#define TOP_OCTAVE_DIVIDE false

// This is for ACTIVE HIGH relays (works with LEDS for testing), switch values if you have ACTIVE LOW relays.
#define ACTIVE_ON LOW
#define ACTIVE_OFF HIGH

// enable for serial mode output, comment out to speed up lights
#define DEBUG

// Timer/delay for self test on startup.
#define SELFTESTTIME 100

/////////////////////////////////////////////////////////////////////
// Hard Customizations - know what you are doing, please.
/////////////////////////////////////////////////////////////////////
// FHT defaults - don't change without reading the Open Music Labs documentation at openmusiclabs.com
#define LOG_OUT 1   // use the log output function
#define FHT_N 256   // set to 256 point fht
#define OCTAVE 1
#define OCT_NORM 0

// include the library, must be done after some of the aboves are defined.. (required by FHT, won't work if included in wrong order)
#include <FHT.h>

// Delay - defines how many cycles before the lights will update.  OML's algorithm at 256 samples (needed for our 8 octaves) takes
// 3.18 ms per cycle, so we essentially throw out 14 cycles (I used mechanical relays, you can lower this for solid state relays).
// 15 cycles = 47.7 ms update rate.  Be careful here and don't change it too quickly!  I warned you!
// Default is 15
#define DELAY 15

// Don't change NUM_PINS.  FHT outputs 8 octs.
#define NUM_PINS 8
// Pin configuration, there is only 8 channels here.  Add duplicate entries if you don't have 8 lights, must be 8!
int relayPins[] = { 2, 3, 4, 5, 6, 7, 8, 9 };

uint8_t x[NUM_PINS];


void frequencyGraph(uint8_t x[], int size);

void setup() {

  // pin setup
  for (int i = 0; i < NUM_PINS; i++) {
    pinMode(relayPins[i], OUTPUT);
    digitalWrite(relayPins[i], ACTIVE_OFF);
  }

  // quick self test
  for (int i = 0; i < 2; i++) {
    for (int j = 0; j < NUM_PINS; j++) {
      digitalWrite(relayPins[j], ACTIVE_ON);
      delay(SELFTESTTIME);
      digitalWrite(relayPins[j], ACTIVE_OFF);
    }
  }

#ifdef DEBUG
  Serial.begin(115200);
  while (!Serial) {
  };
#endif

  TIMSK0 = 0;   // turn off timer0 for lower jitter
  ADCSRA = 0xe5;    // set the adc to free running mode

  // This is setting up A0 - dime
  ADMUX = 0x40;   // use adc0
  DIDR0 = 0x01;   // turn off the digital input for adc0

}

/**********************************************************************************
  Loop - includes initialization function and the full loop
  
**********************************************************************************/

void loop() {

  // True full loop
  int q = 0;
  int cal = 0;

  while (1) {   // reduces jitter

    cli();    // UDRE interrupt slows this way down on arduino1.0

    for (int i = 0; i < FHT_N; i++) { // save 256 samples
      while (!(ADCSRA & 0x10)) ;  // wait for adc to be ready
      ADCSRA = 0xf5;  // restart adc

      // This is his way of reading Analog 0 (A0).  It pulls in L[ow] and H[igh] bit. - dimecoin
      byte m = ADCL;  // fetch adc data
      byte j = ADCH;

      int k = (j << 8) | m; // form into an int
      k -= 0x0200;  // form into a signed int
      k <<= 6;  // form into a 16b signed int
      fht_input[i] = k; // put real data into bins
    }

    fht_window(); // window the data for better frequency response
    fht_reorder();  // reorder the data before doing the fht
    fht_run();  // process the data in the fht
    fht_mag_octave(); // take the output of the fht

    sei();

    // We are in calibration mode.
    if (cal < CAL_TIME) {

      for (int i = 0; i < NUM_PINS; ++i) {
        cal_bias[i] += fht_oct_out[i];
      }

#ifdef DEBUG
      Serial.print(F("Calibrating "));
      Serial.print(cal);
      Serial.print(F("/"));
      Serial.println(CAL_TIME);
#endif

      cal++;
      continue;
    }
    // Calibration mode has just ended, crunch data collected.
    if (cal == CAL_TIME) {

      for (int i = 0; i < NUM_PINS; ++i) {
        oct_bias[i] = (uint8_t) (cal_bias[i] / CAL_TIME);
      }

#ifdef DEBUG
      Serial.println(F("--------------------------------------"));
      Serial.println(F("Done with Cal"));

      for (int i = 0; i < NUM_PINS; ++i) {
        Serial.print(oct_bias[i]);
        Serial.print(" ");
      }

      Serial.println(F(""));
      Serial.println(F("--------------------------------------"));

      for (int i = 0; i < NUM_PINS; ++i) {
        Serial.print(fht_oct_out[i] - oct_bias[i]);
        Serial.print(F(" "));
      }
      Serial.println(F(""));
      Serial.println(F("--------------------------------------"));

      Serial.flush();

#endif

      // Ready signal.
      for (int i = 0; i < NUM_PINS; i++) {
        digitalWrite(relayPins[i], ACTIVE_ON);
      }
      for (int i = 0; i < NUM_PINS; i++) {
        digitalWrite(relayPins[i], ACTIVE_OFF);
      }

      cal++;
      continue;
    }
    // Normal play mode

    if (q % DELAY == 0) {

      for (int i = 0; i < NUM_PINS; i++) {
        x[i] = fht_oct_out[i] - oct_bias[i];
      }

      frequencyGraph(x, NUM_PINS);

#ifdef DEBUG
      for (int i = 0; i < NUM_PINS; ++i) {
        Serial.print(x[i]);
        Serial.print(F(" "));
      }
      Serial.println(F(""));
#endif

    }

    ++q;
  }

}

void frequencyGraph(uint8_t x[], int size) {

  int top_threshold = THRESHOLD;

  for (int i = 0; i < size - 1; i++) {
    x[i] = max(x[i], 0);

    // Special logic for last pin
    if (TOP_OCTAVE_DIVIDE && i == (size - 1)) {
      top_threshold /= 2;
    }

    if (x[i] >= top_threshold) {
      digitalWrite(relayPins[i], ACTIVE_ON);
    } else if (x[i] < top_threshold) {
      // && digitalRead(relayPins[i]) == ACTIVE_ON ) {
      digitalWrite(relayPins[i], ACTIVE_OFF);
    }

  }

}

B


double Val = 0;

void setup() {
  Serial.begin(115200);
}

void loop() {

  Val = getVAL();
  Val = map(Val, 0, 1023, 1, 255);
  Serial.print( " Val = " );
  Serial.println(Val);
}

float getVAL()
{
  float result = analogRead(A2);
  return result;
}

Since your first sketch has a while(1) loop inside of loop(), how would you combine the second sketch? loop() would only be called 1 time.

Maybe explain what it is you are trying to do? display the value of A2 every time through loop()? Normally, that would be very fast and eventually the Serial buffer will get full and the code will slow down waiting for it to empty. That seems to be a contradiction to the first sketch which goes to great lengths to program the hardware directly to achieve maximum speed.

I was trying to help jalbes , basically he wants to regulate THRESHOLD using potentiometer

I seriously thought this was going to be a lot easier than this...
but the goal was to modify the the original code, that I copied from another working project, which had a hard coded threshold value that triggers when the relays fire. I just wanted to add a potentiometer to control this Threshold value instead. Unfortunately, I'm realizing this code is far beyond my skillset so I'm asking for help to troubleshoot this.

void setup() {
  // put your setup code here, to run once:

}
//=================== main loop  ==============
void loop()
{

//++++++++++++++++++++++++ while loop +++++++++++++
 while (1)
 { 
  
 }
//++++++++++++++++++++++++++ while loop end ++++++++

/////////////////////////////////////////
 //Val = getVAL();
 // Val = map(Val, 0, 1023, 1, 255);
  Serial.print( " Val = " );
  Serial.println(Val);
///////////////////////////////////////
}
//========================== main loop end =========


////////////////////////////
float getVAL()
{
  float result = analogRead(A2);
  return result;
}
///////////////////////////////

The easiest fix for this would be to read A2 inside setup() and assign the threshold and then do all the register setup and don't touch the existing loop that reads A0. Of course, this means if you want to adjust the threshold, you would have to restart the sketch since it only gets read at startup.

I want to be able to adjust the threshold without having to restart the sketch - so as I understand, it needs to remain in the loop, correct?

Learn or review the purpose and use of the return statement.

In the quoted code, it causes the function to exit, therefore the following two lines are never execute.

analogRead and map deal with integers. If you actually need a floating point number, one way to be sure you are returning one is to force it to be. It looks like you could just use an int, so

int getVal() {
  int val;

  val = analogRead(A2);
  val = map(val, 0, 1023, 1, 255);
 
  Serial.print( " Val  ");
  Serial.println(val);

  return val;
}

would be plausible.


Yes. If it is to be adjustable, you have to analogRead() the analog value again, and do the map() again.

a7