Could someone check this code?

This is my first attempt at a project (outside of messing with the examples). Not sure if my code/logic is out to lunch or if its the hardwired audio detection circuit. Any and all comments appreciated. I appologize for the messy code, its just my first draft, but since it does not work... lol... might be the last draft.

Please see this thread for details on the project

Edit: Updated code (last few lines would not fit on this post)

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

Ipod-Radio Switcher

Date: August 7, 2010

Hardware Setup: Audio Signal detector attached to analog pin 2 PushToTalk attached to digital pin 1 Headphones relay attached to digital pin 7 Mic relay attached to digital pin 8 RGB LED connectied to Pins 9, 10, 11 R = 9, G = 10, B = 11

Comments: Audio detection works great!

To Do List: 1. balance PNP (no idea how to do that one yet) 2. Auto Calibrate 'threshold' -this should solve the problem of changing volume settings -I think I could seperate HIGH and LOW readings and find the range of each group then auto pick a spot between them both 3. build prototype 4. print pcb board

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

/*************************************************************************************************** USER DEFINED Constants Set these values below \ / \ / \ / \ / \ / \ / V V V V V V ***************************************************************************************************/

const int timer = 10000; //wait 10 seconds after signal is detected before going back to iPod const int threshold = 10; //an arbitrary threshold level that's in the range of the analog input const boolean debugOn = true; //set to true to get debuging data sent on the serial connection

/*************************************************************************************************** ^ ^ ^ ^ ^ ^ ^ ^ / \ / \ / \ / \ / \ / \ / \ / \ DO NOT MESS WITH ANYTHING BELOW THIS LINE ***************************************************************************************************/ const int HeadphonePin = 2; //analog pin 2 listens for signal from TxRx const int MicPin = 1; //digital pin 1 listens for mic to be pressed const int RelayHeadphones = 7; const int RelayMic = 8;

//*** declare varables *** boolean iPod = true; //Set iPod to TRUE when we want iPod to work boolean TxRx = false; //Set TxRx to TRUE when we want to switch to the TxRx unsigned long DetectedUntil; //time in mills that we can turn iPod back on int val = 0; //used to store temp integers.

/**************************************** Setup runs once when first started initialize pins and variables **************************************/ void **setup(){

// initialize pins pinMode(RelayMic, INPUT); pinMode(13, OUTPUT);

// initialize serial communications: if (debugOn) { Serial.begin(9600); } //end: if (debugOn)

// initialize variables DetectedUntil = millis(); iPod = true; TxRx = false; SetRelays(); //initialize relays to the correct state

} //end: setup()

/**************************************** This is the main loop that will keep repeating **************************************/ void **loop(){ if (SignalFromMic() || SignalFromTxRx()) { SignalDetected(); } else { if (millis() > DetectedUntil) { if (!iPod) { iPod = true; TxRx = false; SetRelays(); } } } if (debugOn) { debugMsg(); } } //end: loop()

/**************************************** SignalFromTxRx() Checks if signal detected from TxRx uses threshold to determin result ****************************************/ boolean SignalFromTxRx(){ val = analogRead(HeadphonePin); if (val > threshold) { return true; } else { return false; } } //end: SignalFromTxRx()

/**************************************** SignalFromMic() checks if mic button is pressed (from TxRx) returns true if pin is low (grounded out) ****************************************/ boolean SignalFromMic(){ if (digitalRead(MicPin) == LOW) { return true; } else { return false; } } //end: SignalFromMic()

/**************************************** SignalDetected() handles event when signal is detected ****************************************/ void SignalDetected(){ DetectedUntil = millis() + timer; if (iPod == true) { iPod = false; TxRx = true; SetRelays(); } } //end: SignalDetected()

/**************************************** Turn Relays on or off (using PNP) HIGH = Relay off LOW = Relay on ****************************************/ void SetRelays(){ if (iPod) { digitalWrite(RelayHeadphones,HIGH); digitalWrite(RelayMic,HIGH); digitalWrite(13,LOW); } else { digitalWrite(RelayHeadphones,LOW); digitalWrite(RelayMic,LOW); digitalWrite(13,HIGH); } } //end: SetRelays()

/**************************************** debugMsg() this sends debuging info over the serial port *************************************/ // Send debuging data to the serial port void debugMsg(){ // Serial.print("Time: ");

void loop(){
  if (SignalFromMic) {
    SignalDetected();
  } else {
    if (SignalFromTxRx) {
      SignalDetected();
    } else {
      if (millis() > DetectedUntil) {
        if (!iPod) {
          iPod = true;
          TxRx = false;
          SetRelays();
        }
      }
    }
  }
  
  if (debugOn) {
    debugMsg();
  }
}  //end loop()

SignalFromMic is the name of a function. It acts, the way you are using it as a pointer to that function. So, of course that pointer has a value, so the if statement is true.

You are [u]not[/u], however, calling that function.

The same holds true for SignalFromTxRx.

As a result, the else clause is always skipped, and SignalDetected is the only function ever called.

Adding some () after SignalFromMic and SignalFromTxRx might significantly change the behavior of the program.

Digital pins 0 and 1 are your serial communication pins. They are the ones that you use when you use "Serial.print()": It might be a good idea to not use them in your project (as a digital output anyway).

You start off with good documentation, but it seems to die down as the code goes on.

You can also simplify your code a bit by changing:

  if (SignalFromMic[glow]()[/glow]) {
    SignalDetected();
  } else {
    if (SignalFromTxRx[glow]()[/glow]) {
      SignalDetected();
    } else {...

To

  if (SignalFromMic[glow]()[/glow] || SignalFromTxRx[glow]()[/glow]) {
    SignalDetected();
  }
 else { ...

    }

I'd try and take a few measurements of any analogue read and average it, as you may find that analogue measurements aren't as reliable as you may think.

Thank you InvalidApple and PaulS for your reply. It's clearly been too many years since I programed anything. I'm glad its just syntax and not a hardware issue. One of the biggest discoveries I made last night is this programing language is case sensitive!!! that took me a few hours to figure out :-[

Thanks again, I will post results

Don't mention it.

And don't worry if you make mistakes discovering things.

I have not failed, I've just found 10000 ways that won't work

If you have code that doesn't work, just asked people on here. If you want to improve, read other problems and see if you can solve their problems!

Have it working like a top tonight. Thanks for the help guys!!! I updated the code in the first post to reflect the changes suggested.

if (SignalFromMic() || SignalFromTxRx()) {
    SignalDetected();
  } else {
    if (millis() > DetectedUntil) {
      if (!iPod) {
        iPod = true;
        TxRx = false;
        SetRelays();
      }
    }
  }
  if (debugOn) {
    debugMsg();
  }
}
if (SignalFromMic() || SignalFromTxRx()) {
    SignalDetected();
  } else if ((millis() > DetectedUntil) && (!iPod)) {
        iPod = true;
        TxRx = false;
        SetRelays();

        ...

It might help if you look for ways of combining if statements into one. I still don't like you using one reading of an analogue input, but if it works, it works.

:)

InvalidApple, I’m not following you on both counts.

  1. I thought I did combine if statements by using an OR <||>. Is there more that I can combine to make it slicker?

  2. Use of analogue read. How else could I do this. given the nature of a signal, its not surprising that I do not get a positive detection every time, as a mater of fact I only get a positive about once every 50 samplings. To mitigate this I tried to stream line my program to keep sampling uninterrupted as much as possible, and each subsequent detection results in the overhead of only resetting the clock to a 10 second countdown and then returns to sampling the input.

Is there a better way of doing this with a digital pin? I was under the impression that the digital pins were either HIGH or LOW as found on a logic circuit

Is there more that I can combine to make it slicker?

else {
    if (millis() > DetectedUntil) {
      if (!iPod) {
        iPod = true;
        TxRx = false;
        SetRelays();
      }
    }
}

Can be rewritten as:

else if ((millis() > DetectedUntil) && (!iPod)) {
        iPod = true;
        TxRx = false;
        SetRelays();
}

You can start using shortcuts to shorten your code even more.

Consider this:

if (val > threshold) {
return true;
} else {
return false;
}

This can be written as

return(val > threshold);

As for the analogue read, everyone has their own way. I like to take an average of a few readings:

#define SAMPLES 10 // number of samples on Analogue read
...

//Get 10 samples and find average input

val = 0;
for(uint16_t i = 1; i <= SAMPLES; i++) {
  val += analogRead(_Pin);
  delay(20);
}

val = val / SAMPLES;
//(Here you could just make your threshold 10 times larger)
        ...

The value in “val” will be a lot more accurate, as it has taken in consideration what the analogue pin has been over some time. This is an effective solution to impulse noise, which is usually more significant than other types of interference.

The more samples you take, the more stable the reading is going to be. However, the more you take, the more of a burden the sampling is going to be on you program.

Another think that I notices is your:

  if (debugOn) {
    debugMsg();
  }

What you can do is use pre-processor commands.

debugOn would be defined at the top:

#define DEBUGGING 0 //0 = off, 1 = on

… and in your code have something like this:

#if DEBUGGING

Serial.begin(9600);

#endif

#if DEBUGGING

Serial.print("");

#endif

And when your code is compiled, these parts are only included in you sketch if DEBUGGING = 1.

This means that you code is smaller when you don’t need to use the debugging code.