My first sketch, teething troubles...

Hi all,

Here is my first Arduino sketch.

It is an input selector for a hifi preamp. I would like to press a button to cycle through the various inputs. The arduino is controlling 7 relays, and various combinations of each relay will result in different audio sources playing through the speaker.

I hoped to implement this using a counter to record button presses, and with a set of options of what to do at each count. when it gets the the end of the count (6) it goes back to the beginning.

At this stage I am just trying to get the relevant output pins to change state according to the count but I guess that there are some problems with my programming syntax.

 int sourcecounter = 1;

 int sourcebuttonState = 1;
 int prevsourcebuttonState = 1;

 const int sourcePin = 14; // Source select button input

 const int relay1 = 1; // Turntable power on
 const int relay2 = 2; // digital / analog on stereo coffe volume board
 const int relay3 = 3; // i2s/spdif dddac
 const int relay4 = 4; // spdif / optical on module
 const int relay5 = 5; // int / ext usb on waveio
 const int relay6 = 6; // turntable / aux on my balanced ldr bord

 const int led1 = 7; // Streamer
 const int led2 = 8; // Turntable
 const int led3 = 9; // Optical
 const int led4 = 10; // SPDIF
 const int led5 = 11; // Ext. USB
 const int led6 = 12; // Balanced AUX


 const int led7 = 15; // Hypex amps
 const int led8 = 16; // Valve amps

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

 pinMode (relay1, OUTPUT); 
 pinMode (relay2, OUTPUT); 
 pinMode (relay3, OUTPUT); 
 pinMode (relay4, OUTPUT); 
 pinMode (relay5, OUTPUT); 
 pinMode (relay6, OUTPUT); 
 pinMode (relay7, OUTPUT); 
 pinMode (buttonPin, INPUT); // source selector button
}

void loop() {
  // put your main code here, to run repeatedly:

  buttonState = digitalRead(sourcePin);

  if (buttonState != prevbuttonState) {
    if (buttonState == LOW) {
      sourcecounter = sourcecounter + 1;
    }
    prevbuttonState = buttonState;
  }
  
if counter == 1;{ // Streamer relay settings
digitalWrite(relay1, HIGH);
digitalWrite(relay3, HIGH);
digitalWrite(relay6, HIGH);
digitalWrite(led1, HIGH);
}

else if counter == 2;{ // Turntable relay settings
digitalWrite(relay1, HIGH);
digitalWrite(relay4, HIGH);
digitalWrite(relay7, HIGH);
digitalWrite(led2, HIGH);
}

else if counter == 3;{ // Optical relay settings
digitalWrite(relay2, HIGH);
digitalWrite(relay3, HIGH);
digitalWrite(relay7, HIGH);
digitalWrite(led3, HIGH);
}

else if counter == 4;{ // SPDIF relay settings
  digitalWrite(relay1, HIGH);
  digitalWrite(relay3, HIGH);
  digitalWrite(relay4, HIGH);
  digitalWrite(led4, HIGH);
  }
else if counter == 5;{ // External USB relay settings
digitalWrite(relay1, HIGH);
digitalWrite(relay5, HIGH);
digitalWrite(relay6, HIGH);
digitalWrite(led5, HIGH);
}

else if counter == 6;{  // Aux Balanced relay settings
  digitalWrite(relay2, HIGH);
  digitalWrite(relay5, HIGH);
  digitalWrite(relay6, HIGH);
  digitalWrite(led6, HIGH);
}

else counter = 1; // Set the count back to 1 if it goes above 6 *is it this simple?
}

This isn't going to work for an if expression:

if counter == 1;

The syntax for an if expression is:

if (expression1) { expression2; }

where expression1 is usually a relational test that evaluates to either logic true or false. If expression1 evaluates to logic true, expression2 is evaluated. If expression1 is logic false, expression2 is bypassed. So, your code needs to be written:

if (counter == 1){ // Streamer relay settings
digitalWrite(relay1, HIGH);
digitalWrite(relay3, HIGH);
digitalWrite(relay6, HIGH);
digitalWrite(led1, HIGH);
}

Note there is no semicolon until the end of expression2 inside the if statement block

EDIT: That long chain of nested if statements might be easier to read if you used a *switch * statement.

Thanks. Does this look better?

int sourcecounter = 1;

 int sourcebuttonState = 1;
 int prevsourcebuttonState = 1;

 const int sourcePin = 14; // Source select button input

 const int relay1 = 1; // Turntable power on
 const int relay2 = 2; // digital / analog on stereo coffe volume board
 const int relay3 = 3; // i2s/spdif dddac
 const int relay4 = 4; // spdif / optical on module
 const int relay5 = 5; // int / ext usb on waveio
 const int relay6 = 6; // turntable / aux on my balanced ldr bord

 const int led1 = 7; // Streamer
 const int led2 = 8; // Turntable
 const int led3 = 9; // Optical
 const int led4 = 10; // SPDIF
 const int led5 = 11; // Ext. USB
 const int led6 = 12; // Balanced AUX

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

 pinMode (relay1, OUTPUT); 
 pinMode (relay2, OUTPUT); 
 pinMode (relay3, OUTPUT); 
 pinMode (relay4, OUTPUT); 
 pinMode (relay5, OUTPUT); 
 pinMode (relay6, OUTPUT); 
 pinMode (buttonPin, INPUT); // source selector button
}

void loop() {
  // put your main code here, to run repeatedly:

  buttonState = digitalRead(sourcePin);

  if (buttonState != prevbuttonState) {
    if (buttonState == LOW) {
      sourcecounter = sourcecounter + 1;
    }
    prevbuttonState = buttonState;
  }

switch (sourcecounter) {
  case 1:  // Streamer relay settings
  
digitalWrite(relay1, HIGH);
digitalWrite(relay3, HIGH);
digitalWrite(relay6, HIGH);
digitalWrite(led1, HIGH);
break;
case 2: // Turntable relay settings
digitalWrite(relay1, HIGH);
digitalWrite(relay4, HIGH);
digitalWrite(relay7, HIGH);
digitalWrite(led2, HIGH);
break;
case 3: // Optical relay settings
digitalWrite(relay2, HIGH);
digitalWrite(relay3, HIGH);
digitalWrite(relay7, HIGH);
digitalWrite(led3, HIGH);
break;
case 4: // SPDIF relay settings
  digitalWrite(relay1, HIGH);
  digitalWrite(relay3, HIGH);
  digitalWrite(relay4, HIGH);
  digitalWrite(led4, HIGH);
break;
case 5: // External USB relay settings
digitalWrite(relay1, HIGH);
digitalWrite(relay5, HIGH);
digitalWrite(relay6, HIGH);
digitalWrite(led5, HIGH);
break;
case 6:  // Aux Balanced relay settings
  digitalWrite(relay2, HIGH);
  digitalWrite(relay5, HIGH);
  digitalWrite(relay6, HIGH);
  digitalWrite(led6, HIGH);
break;
default:
sourcecounter = 1// Set the count back to 1 if it goes above 6 
}

It looks much cleaner and readable to me.

One thing that I would do is to change as many int variables to byte as you can to save memory. It is good practice to use variable types suitable for the range of value that need to be held. The pin numbers, for instance, do not need to be ints.

You could potentially reduce the size of the code by using an array to hold the details of which relays to activate for each case and use the case number as the index to the arrays. This, however, would make the program less readable,

you switch relays on in the switch statement, but when do you ever switch them off? Perhaps you should extend your switch statements to switch the right relays on, and switch the rest off.

Also, you don’t have any debouncing on your button, which means it would sometimes increase very quickly even though you think you only pushed it once. a quick but VERY DIRTY fix is to put a delay(20) in your loop, otherwise look at button debounce modules.

Thankyou chaps.

I do not know much about memory limits at this stage, but clearly see the point of using the correct type of variable in any given application.

If using 'byte' rather than 'int' does it need to specify the values in binary?

I'm also unsure of the behaviour of the digital pins. They may need to be LOW in order to operate the relay, I have yet to find that out, but now I know that I need to specify the state of all relevant pins for each combination of relay settings.

Lastly the button I have is a high quality stainless momentary push switch with a very positive click. Is this type of switch still likely to cause bouncing?

f using 'byte' rather than 'int' does it need to specify the values in binary?

No Instead of

const int relay1 = 1; // Turntable power on

just use

const byte relay1 = 1; // Turntable power on

I'm also unsure of the behaviour of the digital pins. They may need to be LOW in order to operate the relay,

Whatever way the relays need to be driven to make them operate it would probably be a good idea to set them HIGH or LOW in setup() so that the system starts in a known state.

You have comments to indicate what each relay and LED does. It would be much better if you moved that information into the variable name, instead of just numbering them. It would greatly enhance the understanding of what is going on, as you read it.

 const int relayTurntable = 6; // turntable / aux on my balanced ldr bord
 const int ledStreamer = 7; // Streamer

instead of:

 const int relay6 = 6; // turntable / aux on my balanced ldr bord
 const int led1 = 7; // Streamer

@ HeliBob

Something like this just after the pin names are declared?

digitalWrite(relay1, LOW); digitalWrite(relay2, LOW); digitalWrite(relay3, LOW); digitalWrite(relay4, LOW); digitalWrite(relay5, LOW); digitalWrite(relay6, LOW);

@aarg

It doesn't work like that for the relays. 6 isn't the turntable relay. It's just relay 6, and the turntable will require 3 relays to close in order to operate. I am not routing audio signals with the relays so there isn't a relay per source, instead it is switching power to different parts of the system and combinations thereof result in different sources being heard.

I could do that with the LED names however.

 int sourcecounter = 1;

 int sourcebuttonState = 1;
 int prevsourcebuttonState = 1;

 const byte sourcePin = 14; // Source select button input

 const byte relay1 = 1; // Turntable power on
 const byte relay2 = 2; // digital / analog on stereo coffe volume board
 const byte relay3 = 3; // i2s/spdif dddac
 const byte relay4 = 4; // spdif / optical on module
 const byte relay5 = 5; // int / ext usb on waveio
 const byte relay6 = 6; // turntable / aux on my balanced ldr bord

 const byte ledStreamer = 7; // Streamer
 const byte ledTurntable = 8; // Turntable
 const byte ledOptical = 9; // Optical
 const byte ledSPDIF = 10; // SPDIF
 const byte ledUSB = 11; // Ext. USB
 const byte ledAUX = 12; // Balanced AUX

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

 pinMode (relay1, OUTPUT); 
 pinMode (relay2, OUTPUT); 
 pinMode (relay3, OUTPUT); 
 pinMode (relay4, OUTPUT); 
 pinMode (relay5, OUTPUT); 
 pinMode (relay6, OUTPUT); 
 pinMode (relay7, OUTPUT); 
 pinMode (buttonPin, INPUT); // source selector button

digitalWrite(relay1, LOW);
digitalWrite(relay2, LOW);
digitalWrite(relay3, LOW);
digitalWrite(relay4, LOW);
digitalWrite(relay5, LOW);
digitalWrite(relay6, LOW);
}

void loop() {
  // put your main code here, to run repeatedly:

  buttonState = digitalRead(sourcePin);

  if (buttonState != prevbuttonState) {
    if (buttonState == LOW) {
      sourcecounter = sourcecounter + 1;
    }
    prevbuttonState = buttonState;
  }

switch (sourcecounter) {
  case 1:  // Streamer relay settings
  
digitalWrite(relay1, HIGH);
digitalWrite(relay3, HIGH);
digitalWrite(relay6, HIGH);
digitalWrite(ledStreamer, HIGH);
break;
case 2: // Turntable relay settings
digitalWrite(relay1, HIGH);
digitalWrite(relay4, HIGH);
digitalWrite(relay7, HIGH);
digitalWrite(ledTurntable, HIGH);
break;
case 3: // Optical relay settings
digitalWrite(relay2, HIGH);
digitalWrite(relay3, HIGH);
digitalWrite(relay7, HIGH);
digitalWrite(ledOptical, HIGH);
break;
case 4: // SPDIF relay settings
  digitalWrite(relay1, HIGH);
  digitalWrite(relay3, HIGH);
  digitalWrite(relay4, HIGH);
  digitalWrite(ledSPDIF, HIGH);
break;
case 5: // External USB relay settings
digitalWrite(relay1, HIGH);
digitalWrite(relay5, HIGH);
digitalWrite(relay6, HIGH);
digitalWrite(ledUSB, HIGH);
break;
case 6:  // Aux Balanced relay settings
  digitalWrite(relay2, HIGH);
  digitalWrite(relay5, HIGH);
  digitalWrite(relay6, HIGH);
  digitalWrite(ledAUX, HIGH);
break;
default:
sourcecounter = 1// Set the count back to 1 if it goes above 6 
}

Thanks for the fantastic support BTW, I only started coding Arduino yesterday lunchtime and 24 hour later it looks like it's almost there.

You can use Ctrl-T within the IDE to reformat your code in a more standard format. If you do that before posting, it makes it easier for us to read.

Ah thank you, that saves a lot of tabbing!

int sourcecounter = 1;

int sourcebuttonState = 1;
int prevsourcebuttonState = 1;

const byte sourcePin = 14; // Source select button input

const byte relay1 = 1; // Turntable power on
const byte relay2 = 2; // digital / analog on stereo coffe volume board
const byte relay3 = 3; // i2s/spdif dddac
const byte relay4 = 4; // spdif / optical on module
const byte relay5 = 5; // int / ext usb on waveio
const byte relay6 = 6; // turntable / aux on my balanced ldr bord

const byte ledStreamer = 7; // Streamer
const byte ledTurntable = 8; // Turntable
const byte ledOptical = 9; // Optical
const byte ledSPDIF = 10; // SPDIF
const byte ledUSB = 11; // Ext. USB
const byte ledAUX = 12; // Balanced AUX

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

  pinMode (relay1, OUTPUT);
  pinMode (relay2, OUTPUT);
  pinMode (relay3, OUTPUT);
  pinMode (relay4, OUTPUT);
  pinMode (relay5, OUTPUT);
  pinMode (relay6, OUTPUT);
  pinMode (relay7, OUTPUT);
  pinMode (buttonPin, INPUT); // source selector button

  digitalWrite(relay1, LOW);
  digitalWrite(relay2, LOW);
  digitalWrite(relay3, LOW);
  digitalWrite(relay4, LOW);
  digitalWrite(relay5, LOW);
  digitalWrite(relay6, LOW);
}

void loop() {
  // put your main code here, to run repeatedly:

  buttonState = digitalRead(sourcePin);

  if (buttonState != prevbuttonState) {
    if (buttonState == LOW) {
      sourcecounter = sourcecounter + 1;
    }
    prevbuttonState = buttonState;
  }

  switch (sourcecounter) {
    case 1:  // Streamer relay settings

      digitalWrite(relay1, HIGH);
      digitalWrite(relay3, HIGH);
      digitalWrite(relay6, HIGH);
      digitalWrite(ledStreamer, HIGH);
      break;
    case 2: // Turntable relay settings
      digitalWrite(relay1, HIGH);
      digitalWrite(relay4, HIGH);
      digitalWrite(relay7, HIGH);
      digitalWrite(ledTurntable, HIGH);
      break;
    case 3: // Optical relay settings
      digitalWrite(relay2, HIGH);
      digitalWrite(relay3, HIGH);
      digitalWrite(relay7, HIGH);
      digitalWrite(ledOptical, HIGH);
      break;
    case 4: // SPDIF relay settings
      digitalWrite(relay1, HIGH);
      digitalWrite(relay3, HIGH);
      digitalWrite(relay4, HIGH);
      digitalWrite(ledSPDIF, HIGH);
      break;
    case 5: // External USB relay settings
      digitalWrite(relay1, HIGH);
      digitalWrite(relay5, HIGH);
      digitalWrite(relay6, HIGH);
      digitalWrite(ledUSB, HIGH);
      break;
    case 6:  // Aux Balanced relay settings
      digitalWrite(relay2, HIGH);
      digitalWrite(relay5, HIGH);
      digitalWrite(relay6, HIGH);
      digitalWrite(ledAUX, HIGH);
      break;
    default:
      sourcecounter = 1// Set the count back to 1 if it goes above 6
  }