# I'm looking for advice or criticism on how to improve

**URL:** <https://forum.arduino.cc/t/im-looking-for-advice-or-criticism-on-how-to-improve/1025629>\
**Category:** Programming\
**Created:** [August 25, 2022, 2:14pm UTC](https://forum.arduino.cc/t/im-looking-for-advice-or-criticism-on-how-to-improve/1025629 "2022-08-25T14:14:15Z")\
**Posts on this page:** 20\
**Page:** 1

<div class="post-metadata">

**Author:** ![red\_c\_c\_c\_p2002](https://avatars.discourse-cdn.com/v4/letter/r/eada6e/32.png) [@red\_c\_c\_c\_p2002](https://forum.arduino.cc/u/red_c_c_c_p2002)\
**Post date:** [August 25, 2022, 2:14pm UTC](https://forum.arduino.cc/t/im-looking-for-advice-or-criticism-on-how-to-improve/1025629/1 "2022-08-25T14:14:15Z")

</div>

Hello there

I'm sure some may have seen me on here asking for help and thanks to those people I've done my most advanced project. I feel I've somewhat got a basic understanding now on how code works but there a lot I'm not sure on or don't even know exists.

Any advice is welcome just don't scold me too hard I'm only like 40hrs or less in 🫠

After all the comments, bar the butchery of my interupt routine anything elsere commended likestructures or?

```cpp
#include <LiquidCrystal.h>

// initialize the library with the numbers of the interface pins
LiquidCrystal lcd(7, 8, 9, 10, 11, 12);

#include "pitches.h"

/*empire////////////////////////////////*/

#define NOTE_B0 31
#define NOTE_C1 33
#define NOTE_CS1 35
#define NOTE_D1 37
#define NOTE_DS1 39
#define NOTE_E1 41
#define NOTE_F1 44
#define NOTE_FS1 46
#define NOTE_G1 49
#define NOTE_GS1 52
#define NOTE_A1 55
#define NOTE_AS1 58
#define NOTE_B1 62
#define NOTE_C2 65
#define NOTE_CS2 69
#define NOTE_D2 73
#define NOTE_DS2 78
#define NOTE_E2 82
#define NOTE_F2 87
#define NOTE_FS2 93
#define NOTE_G2 98
#define NOTE_GS2 104
#define NOTE_A2 110
#define NOTE_AS2 117
#define NOTE_B2 123
#define NOTE_C3 131
#define NOTE_CS3 139
#define NOTE_D3 147
#define NOTE_DS3 156
#define NOTE_E3 165
#define NOTE_F3 175
#define NOTE_FS3 185
#define NOTE_G3 196
#define NOTE_GS3 208
#define NOTE_A3 220
#define NOTE_AS3 233
#define NOTE_B3 247
#define NOTE_C4 262
#define NOTE_CS4 277
#define NOTE_D4 294
#define NOTE_DS4 311
#define NOTE_E4 330
#define NOTE_F4 349
#define NOTE_FS4 370
#define NOTE_G4 392
#define NOTE_GS4 415
#define NOTE_A4 440
#define NOTE_AS4 466
#define NOTE_B4 494
#define NOTE_C5 523
#define NOTE_CS5 554
#define NOTE_D5 587
#define NOTE_DS5 622
#define NOTE_E5 659
#define NOTE_F5 698
#define NOTE_FS5 740
#define NOTE_G5 784
#define NOTE_GS5 831
#define NOTE_A5 880
#define NOTE_AS5 932
#define NOTE_B5 988
#define NOTE_C6 1047
#define NOTE_CS6 1109
#define NOTE_D6 1175
#define NOTE_DS6 1245
#define NOTE_E6 1319
#define NOTE_F6 1397
#define NOTE_FS6 1480
#define NOTE_G6 1568
#define NOTE_GS6 1661
#define NOTE_A6 1760
#define NOTE_AS6 1865
#define NOTE_B6 1976
#define NOTE_C7 2093
#define NOTE_CS7 2217
#define NOTE_D7 2349
#define NOTE_DS7 2489
#define NOTE_E7 2637
#define NOTE_F7 2794
#define NOTE_FS7 2960
#define NOTE_G7 3136
#define NOTE_GS7 3322
#define NOTE_A7 3520
#define NOTE_AS7 3729
#define NOTE_B7 3951
#define NOTE_C8 4186
#define NOTE_CS8 4435
#define NOTE_D8 4699
#define NOTE_DS8 4978
#define REST 0

// change this to make the song slower or faster
int tempo = 120;

// Ns of the moledy followed by the duration.
// a 4 means a quarter note, 8 an eighteenth , 16 sixteenth, so on
// !!negative numbers are used to represent dotted Ns,
// so -4 means a dotted quarter note, that is, a quarter plus an eighteenth!!
int M[] = {
  
  // Dart Vader theme (Imperial March) - Star wars 
  // Score available at https://musescore.com/user/202909/scores/1141521
  // The tenor saxophone part was used
  
  NOTE_A4,-4, NOTE_A4,-4, NOTE_A4,16, NOTE_A4,16, NOTE_A4,16, NOTE_A4,16, NOTE_F4,8, REST,8,
  NOTE_A4,-4, NOTE_A4,-4, NOTE_A4,16, NOTE_A4,16, NOTE_A4,16, NOTE_A4,16, NOTE_F4,8, REST,8,
  NOTE_A4,4, NOTE_A4,4, NOTE_A4,4, NOTE_F4,-8, NOTE_C5,16,

  NOTE_A4,4, NOTE_F4,-8, NOTE_C5,16, NOTE_A4,2,//4
  NOTE_E5,4, NOTE_E5,4, NOTE_E5,4, NOTE_F5,-8, NOTE_C5,16,
  NOTE_A4,4, NOTE_F4,-8, NOTE_C5,16, NOTE_A4,2,
  
  NOTE_A5,4, NOTE_A4,-8, NOTE_A4,16, NOTE_A5,4, NOTE_GS5,-8, NOTE_G5,16, //7 
  NOTE_DS5,16, NOTE_D5,16, NOTE_DS5,8, REST,8, NOTE_A4,8, NOTE_DS5,4, NOTE_D5,-8, NOTE_CS5,16,

  NOTE_C5,16, NOTE_B4,16, NOTE_C5,16, REST,8, NOTE_F4,8, NOTE_GS4,4, NOTE_F4,-8, NOTE_A4,-16,//9
  NOTE_C5,4, NOTE_A4,-8, NOTE_C5,16, NOTE_E5,2,

  NOTE_A5,4, NOTE_A4,-8, NOTE_A4,16, NOTE_A5,4, NOTE_GS5,-8, NOTE_G5,16, //7 
  NOTE_DS5,16, NOTE_D5,16, NOTE_DS5,8, REST,8, NOTE_A4,8, NOTE_DS5,4, NOTE_D5,-8, NOTE_CS5,16,

  NOTE_C5,16, NOTE_B4,16, NOTE_C5,16, REST,8, NOTE_F4,8, NOTE_GS4,4, NOTE_F4,-8, NOTE_A4,-16,//9
  NOTE_A4,4, NOTE_F4,-8, NOTE_C5,16, NOTE_A4,2,
  
};

// sizeof gives the number of bytes, each int value is composed of two bytes (16 bits)
// there are two values per note (pitch and duration), so for each note there are four bytes
int Ns = sizeof(M) / sizeof(M[0]) / 2;

// this calculates the duration of a whole note in ms
int wholenote = (60000 * 4) / tempo;

int divider = 0, ND = 0;

/*empire////////////////////////////////*/

// Defining frequency of each music note
#define NOTE_C4 262
#define NOTE_D4 294
#define NOTE_E4 330
#define NOTE_F4 349
#define NOTE_G4 392
#define NOTE_A4 440
#define NOTE_B4 494
#define NOTE_C5 523
#define NOTE_D5 587
#define NOTE_E5 659
#define NOTE_F5 698
#define NOTE_G5 784
#define NOTE_A5 880
#define NOTE_B5 988

// Music notes of the song, 0 is a rest/pulse
int notes[] = {
    NOTE_E4, NOTE_G4, NOTE_A4, NOTE_A4, 0,
    NOTE_A4, NOTE_B4, NOTE_C5, NOTE_C5, 0,
    NOTE_C5, NOTE_D5, NOTE_B4, NOTE_B4, 0,
    NOTE_A4, NOTE_G4, NOTE_A4, 0,

    NOTE_E4, NOTE_G4, NOTE_A4, NOTE_A4, 0,
    NOTE_A4, NOTE_B4, NOTE_C5, NOTE_C5, 0,
    NOTE_C5, NOTE_D5, NOTE_B4, NOTE_B4, 0,
    NOTE_A4, NOTE_G4, NOTE_A4, 0,

    NOTE_E4, NOTE_G4, NOTE_A4, NOTE_A4, 0,
    NOTE_A4, NOTE_C5, NOTE_D5, NOTE_D5, 0,
    NOTE_D5, NOTE_E5, NOTE_F5, NOTE_F5, 0,
    NOTE_E5, NOTE_D5, NOTE_E5, NOTE_A4, 0,

    NOTE_A4, NOTE_B4, NOTE_C5, NOTE_C5, 0,
    NOTE_D5, NOTE_E5, NOTE_A4, 0,
    NOTE_A4, NOTE_C5, NOTE_B4, NOTE_B4, 0,
    NOTE_C5, NOTE_A4, NOTE_B4, 0,

    NOTE_A4, NOTE_A4,
    //Repeat of first part
    NOTE_A4, NOTE_B4, NOTE_C5, NOTE_C5, 0,
    NOTE_C5, NOTE_D5, NOTE_B4, NOTE_B4, 0,
    NOTE_A4, NOTE_G4, NOTE_A4, 0,

    NOTE_E4, NOTE_G4, NOTE_A4, NOTE_A4, 0,
    NOTE_A4, NOTE_B4, NOTE_C5, NOTE_C5, 0,
    NOTE_C5, NOTE_D5, NOTE_B4, NOTE_B4, 0,
    NOTE_A4, NOTE_G4, NOTE_A4, 0,

    NOTE_E4, NOTE_G4, NOTE_A4, NOTE_A4, 0,
    NOTE_A4, NOTE_C5, NOTE_D5, NOTE_D5, 0,
    NOTE_D5, NOTE_E5, NOTE_F5, NOTE_F5, 0,
    NOTE_E5, NOTE_D5, NOTE_E5, NOTE_A4, 0,

    NOTE_A4, NOTE_B4, NOTE_C5, NOTE_C5, 0,
    NOTE_D5, NOTE_E5, NOTE_A4, 0,
    NOTE_A4, NOTE_C5, NOTE_B4, NOTE_B4, 0,
    NOTE_C5, NOTE_A4, NOTE_B4, 0,
    //End of Repeat

    NOTE_E5, 0, 0, NOTE_F5, 0, 0,
    NOTE_E5, NOTE_E5, 0, NOTE_G5, 0, NOTE_E5, NOTE_D5, 0, 0,
    NOTE_D5, 0, 0, NOTE_C5, 0, 0,
    NOTE_B4, NOTE_C5, 0, NOTE_B4, 0, NOTE_A4,

    NOTE_E5, 0, 0, NOTE_F5, 0, 0,
    NOTE_E5, NOTE_E5, 0, NOTE_G5, 0, NOTE_E5, NOTE_D5, 0, 0,
    NOTE_D5, 0, 0, NOTE_C5, 0, 0,
    NOTE_B4, NOTE_C5, 0, NOTE_B4, 0, NOTE_A4};

// Durations (in ms) of each music note of the song
// Quarter Note is 250 ms when songSpeed = 1.0
int durations[] = {
    125, 125, 250, 125, 125,
    125, 125, 250, 125, 125,
    125, 125, 250, 125, 125,
    125, 125, 375, 125,

    125, 125, 250, 125, 125,
    125, 125, 250, 125, 125,
    125, 125, 250, 125, 125,
    125, 125, 375, 125,

    125, 125, 250, 125, 125,
    125, 125, 250, 125, 125,
    125, 125, 250, 125, 125,
    125, 125, 125, 250, 125,

    125, 125, 250, 125, 125,
    250, 125, 250, 125,
    125, 125, 250, 125, 125,
    125, 125, 375, 375,

    250, 125,
    //Rpeat of First Part
    125, 125, 250, 125, 125,
    125, 125, 250, 125, 125,
    125, 125, 375, 125,

    125, 125, 250, 125, 125,
    125, 125, 250, 125, 125,
    125, 125, 250, 125, 125,
    125, 125, 375, 125,

    125, 125, 250, 125, 125,
    125, 125, 250, 125, 125,
    125, 125, 250, 125, 125,
    125, 125, 125, 250, 125,

    125, 125, 250, 125, 125,
    250, 125, 250, 125,
    125, 125, 250, 125, 125,
    125, 125, 375, 375,
    //End of Repeat

    250, 125, 375, 250, 125, 375,
    125, 125, 125, 125, 125, 125, 125, 125, 375,
    250, 125, 375, 250, 125, 375,
    125, 125, 125, 125, 125, 500,

    250, 125, 375, 250, 125, 375,
    125, 125, 125, 125, 125, 125, 125, 125, 375,
    250, 125, 375, 250, 125, 375,
    125, 125, 125, 125, 125, 500};

int melody[] = {
  NOTE_G4, NOTE_C5, NOTE_G4, NOTE_A4, NOTE_B4, NOTE_E4, NOTE_E4, 
  NOTE_A4, NOTE_G4, NOTE_F4, NOTE_G4, NOTE_C4, NOTE_C4, 
  NOTE_D4, NOTE_D4, NOTE_E4, NOTE_F4, NOTE_F4, NOTE_G4, NOTE_A4, NOTE_B4, NOTE_C5, NOTE_D5, 
  NOTE_E5, NOTE_D5, NOTE_C5, NOTE_D5, NOTE_B4, NOTE_G4, 
  NOTE_C5, NOTE_B4, NOTE_A4, NOTE_B4, NOTE_E4, NOTE_E4, 
  NOTE_A4, NOTE_G4, NOTE_F4, NOTE_G4, NOTE_C4, NOTE_C4, 
  NOTE_C5, NOTE_B4, NOTE_A4, NOTE_G4, NOTE_B4, NOTE_C5, NOTE_D5, 
  NOTE_E5, NOTE_D5, NOTE_C5, NOTE_B4, NOTE_C5, NOTE_D5, NOTE_G4, NOTE_G4, NOTE_B4, NOTE_C5, NOTE_D5,
  NOTE_C5, NOTE_B4, NOTE_A4, NOTE_G4, NOTE_A4, NOTE_B4, NOTE_E4, NOTE_E4, NOTE_G4, NOTE_A4, NOTE_B4,
  NOTE_C5, NOTE_A4, NOTE_B4, NOTE_C5, NOTE_A4, NOTE_B4, NOTE_C5, NOTE_A4, NOTE_C5, NOTE_F5,
  NOTE_F5, NOTE_E5, NOTE_D5, NOTE_C5, NOTE_D5, NOTE_E5, NOTE_C5, NOTE_C5,
  NOTE_D5, NOTE_C5, NOTE_B4, NOTE_A4, NOTE_B4, NOTE_C5, NOTE_A4, NOTE_A4,
  NOTE_C5, NOTE_B4, NOTE_A4, NOTE_G4, NOTE_C4, NOTE_G4, NOTE_A4, NOTE_B4, NOTE_C5
};

int noteDurations[] = {
  8, 4, 6, 16, 4, 8, 8, 
  4, 6, 16, 4, 8, 8, 
  4, 8, 8, 4, 8, 8, 4, 8, 8, 2,
  4, 6, 16, 4, 8, 8, 
  4, 6, 16, 4, 8, 8, 
  4, 6, 16, 4, 6, 16, 
  4, 6, 16, 8, 8, 8, 8, 
  2, 8, 8, 8, 8, 3, 8, 8, 8, 8, 8,
  2, 8, 8, 8, 8, 3, 8, 8, 8, 8, 8,
  4, 6, 16, 4, 6, 16, 4, 8, 8, 2,
  2, 8, 8, 8, 8, 3, 8, 2,
  2, 8, 8, 8, 8, 3, 8, 2,
  4, 6, 16, 4, 4, 2, 4, 4, 1
};

const float songSpeed = 1.0;

#define redLED 6
#define greenLED 5
#define blueLED 13

int red = 6; //select the pin for the red LED
int green =5;// select the pin for the green LED
int blue =13; // select the pin for the blue LED

int CLK = 2;//CLK->D2
int DT = 3;//DT->D3
int SW = 4;//SW->D4
const int interrupt0 = 0;// Interrupt 0 在 pin 2 上
     
      volatile int count = 0;//Define the count
      //This fixed the song issue volatile is crucial for interupts

int lastCLK = 0;//CLK initial value

int btn = 1;
int btncount;

int buzzerPin = A3;

void setup() {
  Serial.begin(9600);

   pinMode(btn,INPUT);
 //RGB LED set as output
 pinMode(redLED,OUTPUT);
 pinMode(greenLED,OUTPUT);
 pinMode(blueLED,OUTPUT);

 digitalWrite(redLED,LOW);
 digitalWrite(greenLED,LOW);
 digitalWrite(blueLED,LOW);

   pinMode(btn, INPUT);
 pinMode(buzzerPin, OUTPUT);
  noTone(buzzerPin);

  
   pinMode(SW, INPUT);
  analogWrite(SW, HIGH);
  pinMode(CLK, INPUT);
  pinMode(DT, INPUT);
  attachInterrupt(interrupt0, ClockChanged, CHANGE);//Set the interrupt 0 handler, trigger level change
  pinMode(btn, INPUT);

  
  // set up the LCD's number of columns and rows:
  lcd.begin(16, 2);
  // Print a message to the LCD.
  
  lcd.setCursor(5, 1);
  lcd.print("start");
  delay(5000); 
  lcd.clear();
    lcd.setCursor(4, 1);
    lcd.print("welcome");
    delay(2000);
   count = 20;
}

void loop() {

 / ***********************LEDS MENU************************ /

if(count <= 30 && count >= 15)
{
 
digitalWrite(redLED,LOW);
digitalWrite(greenLED,LOW);
digitalWrite(blueLED,LOW);
    lcd.setCursor(6, 0);
    lcd.print("Menu");
    lcd.setCursor(0, 1);
    lcd.print("30= LED 13=Song");
    delay(500);
    lcd.clear();

 / ***********************LEDS************************ /
 
  while(count >=30)
  {
 digitalWrite(redLED,LOW);
 digitalWrite(greenLED,LOW);
 digitalWrite(blueLED,LOW);

  lcd.setCursor(3, 0);
  
  lcd.print("LED menu");
  delay(2000); 
  lcd.clear();

  //MENUABOVE
  lcd.setCursor(0, 1);
  lcd.print("39 for red");
   delay(3000); 
     lcd.clear();  
     while (count <= 39 && count >=35 )
     {
    delay(500); 
  lcd.clear();
    lcd.print("red");
    digitalWrite(redLED,HIGH);
 digitalWrite(greenLED,LOW);
 digitalWrite(blueLED,LOW);    

}     
   lcd.setCursor(0, 0);
    lcd.print("49 for green");
    delay(2000);
      lcd.clear();
    while (count <= 49 && count >=45 )
    {
     delay(500); 
  lcd.clear();
    lcd.print("Green");
     digitalWrite(redLED,LOW);
 digitalWrite(greenLED,HIGH);
 digitalWrite(blueLED,LOW);
 
}
  
  lcd.setCursor(1, 1);
  lcd.print("59 for blue");
   delay(2000);
      lcd.clear();
    while (count <= 59 && count >=55 )
    {
     delay(500); 
  lcd.clear();
    lcd.print("Blue");
     digitalWrite(redLED,LOW);
 digitalWrite(greenLED,LOW);
 digitalWrite(blueLED,HIGH);

}      
   
   lcd.setCursor(0, 0);
    lcd.print("69 for purple");
     delay(2000);
      lcd.clear();  
    while (count <= 69 && count >=65)
    {
     delay(500); 
  lcd.clear();
    lcd.print("Purple");
     digitalWrite(redLED,HIGH);
 digitalWrite(greenLED,LOW);
 digitalWrite(blueLED,HIGH);
 
}   

    lcd.setCursor(1, 1);
  lcd.print("79 for white");
   delay(2000);
      lcd.clear();
    while (count <= 79 && count >=75)
    {
     delay(500); 
  lcd.clear();
    lcd.print("White");
     digitalWrite(redLED,HIGH);
 digitalWrite(greenLED,HIGH);
 digitalWrite(blueLED,HIGH);
 
}  

   lcd.setCursor(0, 0);
    lcd.print("90 to menu");
       delay(3000);
      lcd.clear();
      while (count >= 85){
    lcd.print("going back");
    delay(2000);
   count = 20;
    lcd.clear();
    return;
     } 
    }   
}     

/ *************SONG MENU + CODE************* /

 if(count <= 14 )
 {
lcd.setCursor(6, 0);
  lcd.print("Menu");
    lcd.setCursor(0, 1);
  lcd.print("9=cccp 5=pirate 1=em");
 
          digitalWrite(redLED,LOW);
          digitalWrite(greenLED,LOW);
          digitalWrite(blueLED,LOW);
  
  delay(1000);
  lcd.clear();
  
 }
 
/ ***************CCCP********************* /   
    
  if(count == 9 )
  {  
  // Execute once button has been released
  for ( int thisNote = 0; thisNote < sizeof( melody ) / 2; thisNote++ ) 
  {
    
    if (count != 9) 
    {
      // Break from loop once button is released    
       lcd.print("reeeeeed");
       delay(1000);
      count = 14;
      break;        
    }
  
    int noteDuration = 2000 / noteDurations[thisNote];
    tone( buzzerPin, melody[thisNote], noteDuration );      
    lcd.clear();
    int pauseBetweenNotes = noteDuration * 1.30;
    delay( pauseBetweenNotes );

      digitalWrite(redLED,HIGH);
digitalWrite(greenLED,LOW);
 digitalWrite(blueLED,LOW);   
    }
  }
 
/ ***************CCCP********************* /   

/ **************PIRATES***************** /

   if(count == 5 ){

  const int totalNotes = sizeof(notes) / sizeof(int);
  // Loop through each note
  for (int i = 0; i < totalNotes; i++)
{

          if(count != 5)
          {
      delay(1000); 
  lcd.clear();
    lcd.print("yarrrr");
    count = 14;
    
    break;
    }
    
    const int currentNote = notes[i];
    float wait = durations[i] / songSpeed;
    // Play tone if currentNote is not 0 frequency, otherwise pause (noTone) 
  digitalWrite(redLED,HIGH);
 digitalWrite(greenLED,LOW);
 digitalWrite(blueLED,HIGH);
    if (currentNote != 0)
    {
      tone(buzzerPin, notes[i], wait); // tone(pin, frequency, duration) 
    }
    else
    {
      noTone(buzzerPin);
    }
    // delay is used to wait for tone to finish playing before moving to next loop
    delay(wait); 
  }
}

/ ***************PIRATES******************* /

/ ***************IMPERIAL***************** /

/*
if(count == 5 ){
   
   
   // iterate over the Ns of the M. 
  // Remember, the array is twice the number of Ns (Ns + durations)
  for (int TN = 0; TN < Ns * 2; TN = TN + 2) {

          if(count != 5)
          {
      delay(1000); 
  lcd.clear();
    lcd.print("yarrrr");
    count = 14;
    
    break;
    }

    // calculates the duration of each note
    divider = M[TN + 1];
    if (divider > 0) {
      // regular note, just proceed
      ND = (wholenote) / divider;
    } else if (divider < 0) {
      // dotted Ns are represented with negative durations!!
      ND = (wholenote) / abs(divider);
      ND *= 1.5; // increases the duration in half for dotted Ns
    }

    // we only play the note for 90% of the duration, leaving 10% as a pause
    tone(buzzerPin, M[TN], ND*0.9);

    // Wait for the specief duration before playing the next note.
    delay(ND);
    
    // stop the waveform generation before the next note.
    noTone(buzzerPin);
  }
}

*/
// this code works there just not enough memeory rn
/ ***************IMPERIAL***************** /

//VoidLoop end
}
//

//The interrupt handlers
void ClockChanged()
{
  int clkValue = digitalRead(CLK);//Read the CLK pin level
  int dtValue = digitalRead(DT);//Read the DT pin level
  if (lastCLK != clkValue)
  {
    lastCLK = clkValue;
    count += (clkValue != dtValue ? 1 : -1);//CLK and inconsistent DT + 1, otherwise - 1

   delay(500); 
  lcd.clear();
    lcd.print(count);
   
   
  }
}
```

---

<div class="post-metadata">

**Author:** ![killzone\_kid](https://dub1.discourse-cdn.com/arduino/user_avatar/forum.arduino.cc/killzone_kid/32/1098074_2.png) [@killzone\_kid](https://forum.arduino.cc/u/killzone_kid)\
**Post date:** [August 25, 2022, 2:43pm UTC](https://forum.arduino.cc/t/im-looking-for-advice-or-criticism-on-how-to-improve/1025629/2 "2022-08-25T14:43:36Z")

</div>

> [@red\_c\_c\_c\_p2002](#):
>
> ```arduino
> delay(500); 
> lcd.clear();
> lcd.print(count);
> 
> ```

![image](https://europe1.discourse-cdn.com/arduino/original/4X/8/9/b/89bd79282fb7e981b9ab8f165ebae2e9cd273802.gif)

---

<div class="post-metadata">

**Author:** ![UKHeliBob](https://dub1.discourse-cdn.com/arduino/user_avatar/forum.arduino.cc/ukhelibob/32/215663_2.png) [@UKHeliBob](https://forum.arduino.cc/u/UKHeliBob)\
**Post date:** [August 25, 2022, 2:49pm UTC](https://forum.arduino.cc/t/im-looking-for-advice-or-criticism-on-how-to-improve/1025629/4 "2022-08-25T14:49:06Z")

</div>

> [@red\_c\_c\_c\_p2002](#):
>
> Any advice is welcome

Start by describing what your sketch should do

---

<div class="post-metadata">

**Author:** ![red\_c\_c\_c\_p2002](https://avatars.discourse-cdn.com/v4/letter/r/eada6e/32.png) [@red\_c\_c\_c\_p2002](https://forum.arduino.cc/u/red_c_c_c_p2002)\
**Post date:** [August 25, 2022, 2:51pm UTC](https://forum.arduino.cc/t/im-looking-for-advice-or-criticism-on-how-to-improve/1025629/5 "2022-08-25T14:51:37Z")

</div>

Oh right sorry so its a 2 part menu. all parts are divided and labelled top and bottom. Primarily count = a rotary encoder used to select 1 of the 5 LEDS or go to different menus to the select a song which can be stopped or reset if moved.

---

<div class="post-metadata">

**Author:** ![red\_c\_c\_c\_p2002](https://avatars.discourse-cdn.com/v4/letter/r/eada6e/32.png) [@red\_c\_c\_c\_p2002](https://forum.arduino.cc/u/red_c_c_c_p2002)\
**Post date:** [August 25, 2022, 2:54pm UTC](https://forum.arduino.cc/t/im-looking-for-advice-or-criticism-on-how-to-improve/1025629/6 "2022-08-25T14:54:01Z")

</div>

I don't know how to make text stay on LCD without delay + clear or it goes everywhere.

---

<div class="post-metadata">

**Author:** ![ToddL1962](https://dub1.discourse-cdn.com/arduino/user_avatar/forum.arduino.cc/toddl1962/32/225060_2.png) [@ToddL1962](https://forum.arduino.cc/u/ToddL1962)\
**Post date:** [August 25, 2022, 2:54pm UTC](https://forum.arduino.cc/t/im-looking-for-advice-or-criticism-on-how-to-improve/1025629/7 "2022-08-25T14:54:42Z")

</div>

> [@red\_c\_c\_c\_p2002](#):
>
> Oh right sorry so its a 2 part menu. all parts are divided and labelled top and bottom. Primarily count = a rotary encoder used to select 1 of the 5 LEDS or go to different menus to the select a song which can be stopped or reset if moved.

1. Does it work? (i.e. do what you want it to do?)
2. Why are you delaying and updating LCD in an interrupt? DON'T!!!

```arduino
//The interrupt handlers
void ClockChanged()
{
  int clkValue = digitalRead(CLK);//Read the CLK pin level
  int dtValue = digitalRead(DT);//Read the DT pin level
  if (lastCLK != clkValue)
  {
    lastCLK = clkValue;
    count += (clkValue != dtValue ? 1 : -1);//CLK and inconsistent DT + 1, otherwise - 1

    delay(500);
    lcd.clear();
    lcd.print(count);
  }
}

```

1. What Arduino are you using for this?

---

<div class="post-metadata">

**Author:** ![red\_c\_c\_c\_p2002](https://avatars.discourse-cdn.com/v4/letter/r/eada6e/32.png) [@red\_c\_c\_c\_p2002](https://forum.arduino.cc/u/red_c_c_c_p2002)\
**Post date:** [August 25, 2022, 2:57pm UTC](https://forum.arduino.cc/t/im-looking-for-advice-or-criticism-on-how-to-improve/1025629/8 "2022-08-25T14:57:12Z")

</div>

It works fully as i intended, its to stop the text going all over the LCD.

im using UNO.

---

<div class="post-metadata">

**Author:** ![ToddL1962](https://dub1.discourse-cdn.com/arduino/user_avatar/forum.arduino.cc/toddl1962/32/225060_2.png) [@ToddL1962](https://forum.arduino.cc/u/ToddL1962)\
**Post date:** [August 25, 2022, 3:00pm UTC](https://forum.arduino.cc/t/im-looking-for-advice-or-criticism-on-how-to-improve/1025629/9 "2022-08-25T15:00:49Z")

</div>

> [@red\_c\_c\_c\_p2002](#):
>
> It works fully as i intended, its to stop the text going all over the LCD.

You should not perform any delays or I/O which requires interrupts inside of an interrupt. Also, any multibyte variables shared between interrupt and non-interrupt code should only be accessed at non-interrupt level with interrupts disabled or you risk corruption.

These are common mistakes of people who don't understand interrupts and how to properly use them.

---

<div class="post-metadata">

**Author:** ![red\_c\_c\_c\_p2002](https://avatars.discourse-cdn.com/v4/letter/r/eada6e/32.png) [@red\_c\_c\_c\_p2002](https://forum.arduino.cc/u/red_c_c_c_p2002)\
**Post date:** [August 25, 2022, 3:08pm UTC](https://forum.arduino.cc/t/im-looking-for-advice-or-criticism-on-how-to-improve/1025629/10 "2022-08-25T15:08:49Z")

</div>

> [@ToddL1962](#):
>
> Also, any multibyte variables shared between interrupt and non-interrupt code should only be accessed at non-interrupt level with interrupts disabled or you risk corruption.

I've read about this i just don't see how it would work for me in my code, its also language like that is hard for me to understand as im new

> [@ToddL1962](#):
>
> non-interrupt code should only be accessed at non-interrupt level with interrupts

like this

---

<div class="post-metadata">

**Author:** ![red\_c\_c\_c\_p2002](https://avatars.discourse-cdn.com/v4/letter/r/eada6e/32.png) [@red\_c\_c\_c\_p2002](https://forum.arduino.cc/u/red_c_c_c_p2002)\
**Post date:** [August 25, 2022, 3:09pm UTC](https://forum.arduino.cc/t/im-looking-for-advice-or-criticism-on-how-to-improve/1025629/11 "2022-08-25T15:09:28Z")

</div>

forgot to say thanks for the input btw greatly appreciated

---

<div class="post-metadata">

**Author:** ![red\_c\_c\_c\_p2002](https://avatars.discourse-cdn.com/v4/letter/r/eada6e/32.png) [@red\_c\_c\_c\_p2002](https://forum.arduino.cc/u/red_c_c_c_p2002)\
**Post date:** [August 25, 2022, 3:10pm UTC](https://forum.arduino.cc/t/im-looking-for-advice-or-criticism-on-how-to-improve/1025629/12 "2022-08-25T15:10:43Z")

</div>

Lastly the delay is solely for the LCD text to not spam all over the LCD

---

<div class="post-metadata">

**Author:** ![ToddL1962](https://dub1.discourse-cdn.com/arduino/user_avatar/forum.arduino.cc/toddl1962/32/225060_2.png) [@ToddL1962](https://forum.arduino.cc/u/ToddL1962)\
**Post date:** [August 25, 2022, 3:35pm UTC](https://forum.arduino.cc/t/im-looking-for-advice-or-criticism-on-how-to-improve/1025629/13 "2022-08-25T15:35:24Z")

</div>

> [@red\_c\_c\_c\_p2002](#):
>
> I've read about this i just don't see how it would work for me in my code, its also language like that is hard for me to understand as im new

I get it. But interrupts are an advanced concept. They are appropriate for applications such as rotary encoders but you just can't add any code you want into an interrupt. Interrupts are used to process time critical events and should be as short and efficient as possible.

---

<div class="post-metadata">

**Author:** ![groundFungus](https://avatars.discourse-cdn.com/v4/letter/g/73ab20/32.png) [@groundFungus](https://forum.arduino.cc/u/groundFungus)\
**Post date:** [August 25, 2022, 3:38pm UTC](https://forum.arduino.cc/t/im-looking-for-advice-or-criticism-on-how-to-improve/1025629/14 "2022-08-25T15:38:36Z")

</div>

All other interrupts are turned off during an ISR execution so any functions that depend on interrupts, like delay() or print(), will not work right and will introduce "bugs" that can be hard to resolve.

---

<div class="post-metadata">

**Author:** ![red\_c\_c\_c\_p2002](https://avatars.discourse-cdn.com/v4/letter/r/eada6e/32.png) [@red\_c\_c\_c\_p2002](https://forum.arduino.cc/u/red_c_c_c_p2002)\
**Post date:** [August 25, 2022, 3:47pm UTC](https://forum.arduino.cc/t/im-looking-for-advice-or-criticism-on-how-to-improve/1025629/15 "2022-08-25T15:47:58Z")

</div>

noted thanks, i get things given to me at work to roughly design to increase my knowledge of components and code so i have to work around things i don't get which is why im on here quite alot 😅 🫠

---

<div class="post-metadata">

**Author:** ![red\_c\_c\_c\_p2002](https://avatars.discourse-cdn.com/v4/letter/r/eada6e/32.png) [@red\_c\_c\_c\_p2002](https://forum.arduino.cc/u/red_c_c_c_p2002)\
**Post date:** [August 25, 2022, 3:49pm UTC](https://forum.arduino.cc/t/im-looking-for-advice-or-criticism-on-how-to-improve/1025629/16 "2022-08-25T15:49:01Z")

</div>

yeah ive had a few of these although many are ironed out using the delay clear which i know is equivalent of putting a plaster on a shotgun wound.

---

<div class="post-metadata">

**Author:** ![alto777](https://dub1.discourse-cdn.com/arduino/user_avatar/forum.arduino.cc/alto777/32/317385_2.png) [@alto777](https://forum.arduino.cc/u/alto777)\
**Post date:** [August 25, 2022, 4:27pm UTC](https://forum.arduino.cc/t/im-looking-for-advice-or-criticism-on-how-to-improve/1025629/17 "2022-08-25T16:27:50Z")

</div>

> [@red\_c\_c\_c\_p2002](#):
>
> Any advice is welcome just don't scold me too hard I'm only like 40hrs or less in

No scolding, we all started nowhere. And 40 hours is nothing. 40 weeks of diligent learning might really put you somewhere.

There's ppl here 40 years in who still find they can learn something everyday.

Hard until it is easy.

But I must ask, what parts of the sketch you are showing us here did you have anything to do with?

It really is something like I've never seen before. I've spent time looking at it or a similar one that was on another thread, still don't know how it works or why it didn't work when it wasn't. Working.

If you found it somewhere and beat on it awhile, I would say throw it in the trash.

If you wrote it mostly yourself, you're way over your skis trying to be creative. You should slow your roll, reduce your expectations and get a firm(er) grip on very basic coding, probably most effectively by reading reading reading and experimenting with baby steps… getting creative is some good number of hours from where you are.

IMO & YMMV always.

a7

---

<div class="post-metadata">

**Author:** ![noiasca](https://dub1.discourse-cdn.com/arduino/user_avatar/forum.arduino.cc/noiasca/32/1126910_2.png) [@noiasca](https://forum.arduino.cc/u/noiasca)\
**Post date:** [August 25, 2022, 5:43pm UTC](https://forum.arduino.cc/t/im-looking-for-advice-or-criticism-on-how-to-improve/1025629/18 "2022-08-25T17:43:37Z")

</div>

> [@red\_c\_c\_c\_p2002](#):
>
> Any advice is welcome just don't scold me too hard I'm only like 40hrs or less in

let me give you 4 advices:

- make your life easier and press **CTRL-T** in your IDE. Does it look nice now?
- your program is worthless if you don't have a clear **specification** what it does. If you want me to read your code ... let me read your specification so I know up front what your program should do.
- you are using arrays - fine ... next step learn about **c++ structure** s
- put your music sketch beside. Open the IDE example **Blink without delay** and expand it to blink three LEDs in different intervals like 1100ms, 2200ms and 3300ms. If this works - see again your music code and improve it. I guess you will start from scratch.

Take your time - hope to hear from you in 40 hours again 😉

---

<div class="post-metadata">

**Author:** ![jfjlaros](https://dub1.discourse-cdn.com/arduino/user_avatar/forum.arduino.cc/jfjlaros/32/716915_2.png) [@jfjlaros](https://forum.arduino.cc/u/jfjlaros)\
**Post date:** [August 26, 2022, 8:06am UTC](https://forum.arduino.cc/t/im-looking-for-advice-or-criticism-on-how-to-improve/1025629/19 "2022-08-26T08:06:40Z")

</div>

> [@red\_c\_c\_c\_p2002](#):
>
> Any advice is welcome

An efficient way of storing a melody would be to specify the frequency and the duration of a tone directly and to store these values in [program memory](https://www.arduino.cc/reference/en/language/variables/utilities/progmem/).

```cpp
struct Tone {
  uint16_t const frequency;
  uint16_t const duration;
};

Tone const melody[] PROGMEM = {{58, 4}, {73, 4}, {277, 16}};

```

We can retrieve the melody from program memory as follows.

```cpp
for (Tone const& toneP: melody) {
  Tone toneR{};
  memcpy_P(&toneR, &toneP, sizeof(Tone));

  // Do something with `toneR.frequency` and `toneR.duration`.
}

```

* * *

This may not be the most user friendly method to write a melody, this is probably why you introduced the predefined notes.

> [@red\_c\_c\_c\_p2002](#):
>
> ```arduino
> #define NOTE_B0 31
> // ...
> #define NOTE_DS8 4978
> 
> ```

We could write a program that generates the `melody` array on a computer, which we then can copy to the sketch. Alternatively, we could let the program generate a file that can be included from the sketch.

The predefined values seem to be based on a generic formula, described [here](https://www.intmath.com/trigonometric-graphs/music.php). In C++, this formula looks as follows:

```cpp
unsigned int frequency(int const note) {
  return 440 * pow(2, note / 12.0) + 0.5;
}

```

We can introduce user friendly notations (e.g., `A#1`, `D2`, `C#4`, etc.) with some additional helper functions.

```cpp
size_t lcp(char const a[], char const b[]) {
  size_t i;
  for (i = 0; a[i] and b[i] and a[i] == b[i]; i++);
  return i;
}

int notationToNote(char const str[]) {
  static char const* const notation[] = {
    "C", "C#", "D", "D#", "E", "F", "F#", "G", "G#", "A", "A#", "B"};
  size_t len = strlen(str) - 1;
  for (size_t i = 0; i < 12; i++) {
    if (lcp(notation[i], str) == len) {
      return 12 * (str[len] - '0') + i - 57;
    }
  }
  return 0;
}

```

The `notationToNote` function can now be used to write a melody.

```cpp
struct Tone {
  char const* const notation;
  unsigned long const duration;
};

int main() {
  Tone const melody[] = {{"A#1", 4}, {"D2", 4}, {"C#4", 16}};
  for (Tone const& tone: melody) {
    // Do something with `frequency(notationToNote(tone.notation))` and `tone.duration`.
  }
}

```

---

<div class="post-metadata">

**Author:** ![red\_c\_c\_c\_p2002](https://avatars.discourse-cdn.com/v4/letter/r/eada6e/32.png) [@red\_c\_c\_c\_p2002](https://forum.arduino.cc/u/red_c_c_c_p2002)\
**Post date:** [August 26, 2022, 3:57pm UTC](https://forum.arduino.cc/t/im-looking-for-advice-or-criticism-on-how-to-improve/1025629/20 "2022-08-26T15:57:22Z")

</div>

thanks for the amazing input, i out sourced most of the notes etc as i cant read them 🫠

definitely adding this to my study folder and will try to implement it.

hopefully.. 😅

---

<div class="post-metadata">

**Author:** ![red\_c\_c\_c\_p2002](https://avatars.discourse-cdn.com/v4/letter/r/eada6e/32.png) [@red\_c\_c\_c\_p2002](https://forum.arduino.cc/u/red_c_c_c_p2002)\
**Post date:** [August 26, 2022, 3:58pm UTC](https://forum.arduino.cc/t/im-looking-for-advice-or-criticism-on-how-to-improve/1025629/21 "2022-08-26T15:58:46Z")

</div>

will do ive been trying to add this but ive not had much spare time at work, hopefully when i get some free time ill post an updated version 🫠

many thanks

red

[Next page](https://forum.arduino.cc/t/im-looking-for-advice-or-criticism-on-how-to-improve/1025629.md?page=2)