# Cleaning Up Beginner Code

Hello,

I’m working on a project that uses an arduino, neopixels, and an I2C mp3 trigger.

Basically the user presses one of eight buttons, some neopixels light up, and an audio file is played. The sketch uses a state machine to keep track of what it is doing. The duration of each state is determined by the mp3 trigger. The mp3 trigger can send a TRUE or FALSE result over I2C based on what it is doing (playing audio or idle) and the arduino polls the mp3 trigger for this information. Using this I can control the duration for each state based on the length of the mp3 file it is playing, rather than use millis() (which i’m not good at yet) or a delay().

I feel like the sketch is overly complex and long for the intended function. It works well but I’m a beginner and am looking to improve my coding skills. Was hoping someone could take a look and give me some pointers to improve this or maybe give me some pointers on different ways to think about how this code can run. I feel that the ‘waitForMp3’ function, and the ‘readButtons’ functions could be improved. Is switch case the best way to run these sequences?

(I’m omitting the extra code that is needed to run the mp3 player over i2c).

Here’s a short vid of the system running.

Thanks and much appreciated.

``````//NEOPIXEL STUFF
#define PIN 2
int pixelQty = 112;

//MP3 STUFF
#include <Wire.h>
int mp3vol = 28; // Volume can be 0-31, 0 is mute, 31 is max

//BUTTON STUFF
int button[] = {3, 4, 5, 6, 7, 8, 9, 10, 11};  // "3" is a dummy place holder because arrays are zero indexed.
int buttonQty = 9;
int buttonState[] = {0, 0, 0, 0, 0, 0, 0, 0, 0};

//IDLE MUSIC TIMER
unsigned long previousMillisMusic = 0;
const long MusicInterval = 75000;

//MISC SETUP
int mode = 10; // set initial startup mode to idle
int ledPin = 13; // for status LED
int ledState = LOW;  // ledState used to set the status LED
unsigned long previousMillis = 0; // will store last time LED was updated
const long interval = 250; // interval at which to blink (milliseconds)
int buttonDeBounce = 100; // delay after pressing a button to.. i.e. fake debounce

void setup() {

Serial.begin(9600);

// set all buttons to INPUT_PULLUP
for (int buttonPinMode = 1; buttonPinMode < buttonQty; buttonPinMode++) {
pinMode(button[buttonPinMode], INPUT_PULLUP);
}

Serial.println("MP3 SELF TEST");

delay(1000);

mp3ChangeVolume(mp3vol);
Serial.print("Hello! I see ");
Serial.print(mp3SongCount());
Serial.print(" mp3 files on the SD card... ");
mp3PlayFile(1); // play idle music

delay(500);

//INIT NEOPIXELS
strip.begin();
strip.show(); // Initialize all pixels to 'off'

pinMode(ledPin, OUTPUT);
delay(1500);
Serial.println("Let's GO!");
delay(500);
}

void loop() {

switch (mode) {

case 1: // ROUTE 66
set_route66();
strip.show();
mp3PlayFile(9);
waitForMp3();
mp3PlayFile(1);
mode = 9; // go back to idle
break;

case 2: // GEMENI GIANT
set_gemeniGiant();
strip.show();
mp3PlayFile(2);
waitForMp3();
mp3PlayFile(1);
mode = 9;
break;

case 3: // BLUE WHALE
buildLEDs(16);
set_blueWhale();
strip.show();
mp3PlayFile(3);
waitForMp3();
mp3PlayFile(1);
mode = 9;
break;

case 4: // RAINBOW BRIDGE
buildLEDs(32);
set_rainbowBridge();
strip.show();
mp3PlayFile(4);
waitForMp3();
mp3PlayFile(1);
mode = 9;
break;

buildLEDs(48);
strip.show();
mp3PlayFile(5);
waitForMp3();
mp3PlayFile(1);
mode = 9;
break;

case 6: // BLUE SWALLOW MOTEL
buildLEDs(64);
set_blueSwallow();
strip.show();
mp3PlayFile(6);
waitForMp3();
mp3PlayFile(1);
mode = 9;
break;

case 7: //WIGWAM MOTEL
buildLEDs(80);
set_wigwamMotel();
strip.show();
mp3PlayFile(7);
waitForMp3();
mp3PlayFile(1);
mode = 9;
break;

case 8: // PACIFIC PARK
buildLEDs(96);
set_pacificPark();
strip.show();
mp3PlayFile(8);
waitForMp3();
mp3PlayFile(1);
mode = 9;
break;

case 9: //IDLE
set_idle();
strip.show();
Serial.println(mode);
imAlive();
idleMusic();

break;

case 10:
imAlive();
neoTest(2);
mode = 9;
break;

}

}

if (buttonState[1] == LOW) {
delay(buttonDeBounce);
mode = 1;
}
else if
(buttonState[2] == LOW) {
delay(buttonDeBounce);
mode = 2;
}
else if
(buttonState[3] == LOW) {
delay(buttonDeBounce);
mode = 3;
}
else if
(buttonState[4] == LOW) {
delay(buttonDeBounce);
mode = 4;
}
else if
(buttonState[5] == LOW) {
delay(buttonDeBounce);
mode = 5;
}
else if
(buttonState[6] == LOW) {
delay(buttonDeBounce);
mode = 6;
}
else if
(buttonState[7] == LOW) {
delay(buttonDeBounce);
mode = 7;
}
else if
(buttonState[8] == LOW) {
delay(buttonDeBounce);
mode = 8;
}
} // end function

// NEOPIXEL GEMENI GIANT
void set_gemeniGiant() {
for (int a = 0; a < 16; a++) {
strip.setPixelColor(a, 100, 0, 0, 0);
}
}

// NEOPIXEL BLUE WHALE
void set_blueWhale() {
for (int b = 16; b < 32; b++) {
strip.setPixelColor(b, 100, 0, 0, 0);
}
}

// NEOPIXEL RAINBOW BRIDGE
void set_rainbowBridge() {
for (int c = 32; c < 48; c++) {
strip.setPixelColor(c, 100, 0, 0, 0);
}
}

for (int d = 48; d < 64; d++) {
strip.setPixelColor(d, 100, 0, 0, 0);
}
}

// NEOPIXEL BLUE SWALLOW
void set_blueSwallow() {
for (int e = 64; e < 80; e++) {
strip.setPixelColor(e, 100, 0, 0, 0);
}
}

// NEOPIXEL WIGWAM MOTEL
void set_wigwamMotel() {
for (int f = 80; f < 96; f++) {
strip.setPixelColor(f, 100, 0, 0, 0);
}
}

// NEOPIXEL PACIFIC PARK
void set_pacificPark() {
for (int g = 96; g < 112; g++) {
strip.setPixelColor(g, 100, 0, 0, 0);
}
}

// NEOPIXEL ROUTE 66
void set_route66() {
for (int j = 0; j < 112; j++) {
strip.setPixelColor(j, 100, 0, 0, 0);
}
}

// NEOPIXEL IDLE
void set_idle() {
for (int h = 0; h < 112; h++) {
strip.setPixelColor(h, 3, 25, 0, 0);
}
}

// NEOPIXEL ALL OFF
void set_allOff() {
for (int i = 0; i < 112; i++) {
strip.setPixelColor(i, 0, 0, 0, 0);
}
}

// MP3 STATUS
void waitForMp3() {
while (mp3IsPlaying() == true) {
delay(1);
}
}

// BLINK LED 13 WHEN IDLE
void imAlive() {
unsigned long currentMillis = millis();
if (currentMillis - previousMillis >= interval) {
previousMillis = currentMillis;
if (ledState == LOW) {
ledState = HIGH;
} else {
ledState = LOW;
}
digitalWrite(ledPin, ledState);
}
}

//PERIODICALLY PLAY MUSIC WHEN IDLE
void idleMusic() {
unsigned long currentMillisMusic = millis();

if (currentMillisMusic - previousMillisMusic >= MusicInterval) {
previousMillisMusic = currentMillisMusic;
mp3PlayFile(1); //play music
}
}

// START UP SELF TEST RAINBOW PATTERN
void neoTest(uint8_t wait) {
uint16_t i, j;

for (j = 0; j < 256; j++) {
for (i = 0; i < strip.numPixels(); i++) {
strip.setPixelColor(i, Wheel((i + j) & 255));
}
strip.show();
delay(wait);
}
}

// COLOR WHEEL FOR SELF TEST
uint32_t Wheel(byte WheelPos) {
WheelPos = 255 - WheelPos;
if (WheelPos < 85) {
return strip.Color(255 - WheelPos * 3, 0, WheelPos * 3);
}
if (WheelPos < 170) {
WheelPos -= 85;
return strip.Color(0, WheelPos * 3, 255 - WheelPos * 3);
}
WheelPos -= 170;
return strip.Color(WheelPos * 3, 255 - WheelPos * 3, 0);
}

int buildLEDs(int t){
int result;
result = t;
for(int u = 0; u<t; u++){
strip.setPixelColor(u, 20,115,0,0);
strip.show();
delay(5);
}
}
``````

What your code looks like in an editor/IDE is irrelevant save for it's ongoing maintenance. Many equate compact source code with compact/fast executables while the truth is that there is no relationship at all. Compact code is ofttimes unnecessarily confusing. A lost art is what we used to call simple code, which seems to have been replaced by a drive to demonstrate an intricate knowledge of the obscurities of the language, useful or not. Compilers are amazingly efficient these days and will probably do a far better job of optimizing your code than you (or I) could.

Your code looks fine. It is very readable. If you wanted to do something with it, you could investigate using test-case arrays to loop and choose, thereby eliminating some of your redundant code. A carefully defined structure array could provide an extensible platform more easily handled then a limited-scope switch function. On the other hand, if it works, why fix it?

In your loop() function, look at the last several statements in each case:

``````// within each case...
strip.show();
mp3PlayFile(7);
waitForMp3();
mp3PlayFile(1);
mode = 9;
break;
``````

What if you wrote a function and just passed in the play file number and added the case 9 calls to it? Also, what about:

``````  buttonState[1] = digitalRead(button[1]);
// ...followed by the cascading if-else blocks
``````

Could you replace it with something like:

``````for (int i = 1; i < 9; i++) {
if (buttonState[i] == LOW) {
miode = i;                                       // Assumes only 1 can be low
break;
}
}
``````

``````  // set all buttons to INPUT_PULLUP
for (int buttonPinMode = 1; buttonPinMode < buttonQty; buttonPinMode++) {
pinMode(button[buttonPinMode], INPUT_PULLUP);
}
``````

The index variable name is unnecessarily long. Loop indices are typically one letter names.

The index variable will contain values from 0 (when corrected) to 9. Does it REALLY need to be an int?

The code before the for loop is wrong, because array indices start at 0.

That code should be:

``````  // set all buttons to INPUT_PULLUP
for (byte b = 0; b < buttonQty; b++)
{
pinMode(button[b], INPUT_PULLUP);
}
``````
``````delay(1000);

mp3ChangeVolume(mp3vol);
``````

Unnecessary

amounts

of

white

space.

Poor
indenting
making
the
code
unnecessarily
hard
to

Tools + Auto Format is you friend. Use it often.

``````  else if
(buttonState[2] == LOW) {
``````

This ALL belongs on ONE line.

And, as has been pointed out, should be flattened into a for loop.

All of your set_Xxx functions do the same thing, except for the range of LEDs that are colored.

``````void waitForMp3() {
while (mp3IsPlaying() == true) {
delay(1);
}
}
``````

Why do you care how many times the while loop iterates? There is NO excuse for the delay() in the body.

``````    if (ledState == LOW) {
ledState = HIGH;
} else {
ledState = LOW;
}
``````

or

``````   ledState = !ledState;
``````

I’m a BIG fan of less code. Less place for bugs to hide.