Is there a way to improve this?

Projects Programming
1

Hi there!

I'm currently working on a project where I want to add some brief jingles to notify some events to the user. These are:

  • Joystick button pressed.
  • A task has started.
  • A task has finished.

For that I'm using a passive buzzer. Since I don't want to block the code while playing the melodys, I've merged the examples from pitcher.h with the allmighty BlinkWithoutDelay. I actually achieved that, here's the code:

#include <pitches.h>
#include <Button.h>

#define Buzzer 11

bool PlayingButtonJingle;
unsigned long currentMillis;

Button JoystickButton(2);

void setup() {
  JoystickButton.begin();
}

void loop() {
   currentMillis = millis();
   if(JoystickButton.pressed()){ PlayingButtonJingle = HIGH;}
   if(PlayingButtonJingle){ ButtonJingle();}
}

int ButtonJingleNotes[] = {
  NOTE_A7, NOTE_C8, NOTE_D8
};

int ButtonJingleNotesDuration[] = {
  32, 16, 16
};

bool PlayingToneBJ = LOW;
int currentNoteBJ = 0;
unsigned long previousMillisBJ = 0, pauseBetweenNotesBJ = 0, noteDurationBJ = 0;

void ButtonJingle(){
  
    if(!PlayingToneBJ){
      noteDurationBJ = 1000 / ButtonJingleNotesDuration[currentNoteBJ];
      pauseBetweenNotesBJ = noteDurationBJ * 1.30;
      previousMillisBJ = currentMillis;
      tone(Buzzer, ButtonJingleNotes[currentNoteBJ], noteDurationBJ);
      PlayingToneBJ = HIGH;      
    }
    else{
      if(currentMillis - previousMillisBJ >= pauseBetweenNotesBJ){
        noTone(Buzzer);
        currentNoteBJ++;
        PlayingToneBJ = LOW;
      }
    }
    if(currentNoteBJ > (sizeof(ButtonJingleNotes) / sizeof(ButtonJingleNotes[0]))){
      currentNoteBJ = 0;
      PlayingButtonJingle = LOW;
    }
}

The issue is that I really think this code could be much cleaner, but I'm not capable of doing it. At the end, I will only have three different jingles so I would repeat the code below the loop() just thrice as well as the two lines inside the loop() itself. But what if I have 15 different jingles, for example?

Furthermore, I'm pretty sure that this code could be improved in some many ways, I'm whiling to hear you about it. Memory/Lines saving, more readable code, programmer good practices... Whatever.

The most upsetting thing from my point of view, are the declarations of the variables and the arrays just above the ButtonJingle(), but I think that if they were at the very begining of the code, it would be even worse. However, I suspect that there is a ton of things that I can't actually see in there, so I need your proffessional eyes!

Thanks in advance four y'all!
Wyzed.

2

The most upsetting thing from my point of view, are the declarations of the variables and the arrays just above the ButtonJingle(), but I think that if they were at the very begining of the code, it would be even worse.

Those variables are declared in the Global space(outside of any function) as long as they are declared before they are used it doesn't matter where they are.

I will only have three different jingles so I would repeat the code below the loop() just thrice as well as the two lines inside the loop()

Any time you think you need to repeat code you know your doing it wrong.
What you need is a way to make those functions work for more than one jingle.

One way to do that is to use input parameters for your functions. But since you need several parameters for each jingle using a struct is a good way to go.

Here is a working example of how to implement a struct and use it as a parameter for a function.

// Basic Example showing use of a struct as input to a function


// Struck that declares variables needed
struct MyStruct
{
  int a;
  int b;
  // Put as many variables, arrays etc. as needed
};

// Create an Instance for each jangle
MyStruct jangle1;
MyStruct jangle2;

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

  // pause until serial monitor is opened
  while (!Serial) ;


  // Initialize variables for first jangle
  jangle1.a = 10;
  jangle1.b = 5;

  // Initialize variables for second jangle
  jangle2.a = 8;
  jangle2.b = 3;

  // Call function using struct as parameter
  printJangle(jangle1);
  printJangle(jangle2);

}

void loop() {

}

// function that takes a struct as a parameter
void printJangle(MyStruct &thisJangle) {
  Serial.print(thisJangle.a);
  Serial.print(", ");

  // you can manipulate the variables
  thisJangle.b += 100;
  Serial.println(thisJangle.b);
}

EDIT: Just a note that in this line you multiply an integer and a floating point number then store the result in an integer. Probably not much of a problem here but it truncates the result which will cause errors in your calculations. Probably no big deal here, but could be in a more sensitive operation. Besides it's just poor programming.

      pauseBetweenNotesBJ = noteDurationBJ * 1.30;
3 
int ButtonJingleNotesDuration[] = {
  32, 16, 16
};

it seems, that a byte is enough.
it never changes, so make it const.

bool PlayingToneBJ = LOW;
int currentNoteBJ = 0;
unsigned long previousMillisBJ = 0, pauseBetweenNotesBJ = 0, noteDurationBJ = 0;

are currently in global scope but only used in your function. Put them in your function and make them static if needed.

#define Buzzer 11
nowadays i prefer to avoid define for pins, and I use:
const byte buzzer=11;

int ButtonJingleNotes[] = {
NOTE_A7, NOTE_C8, NOTE_D8
};
check what's NOTE_xx is in reallity and if you really need an int. if the array doesn't change, make it const

unsigned long currentMillis;
not used in globald scope. Put it in the loop and make it static.

For your eyes only: variable names and function names should start with small letters

4

Don't write integer * 1.30 as that will generate floating point code.

Do write integer * 13 / 10 as long as integer * 13 is not > integer capacity.

Make the jingle player a state machine.

PS also tone( with duration ) blocks execution until the note is finished.

5

Hutkikz:
Those variables are declared in the Global space(outside of any function) as long as they are declared before they are used it doesn't matter where they are.

For this I think that here's the key:

noiasca:
are currently in global scope but only used in your function. Put them in your function and make them static if needed.

In my own first attempt, the problem was that the variable currentNoteBJ was not saved between calls, so I got played only the first note forever, that's why I declared it on the global scope. Now that I've read about static, this can defintively do the trick. Since that's the only one data I need to store between calls, all the others can be declared locally on the function. This will save memory for another tasks, am I right?

Edit: Just realize that I'm not only need the current note variable, but all of them for watching the time of each note and if I'm currently playing a note or I'm not. So all of them need to be static in the end.

Hutkikz:
Any time you think you need to repeat code you know your doing it wrong.
What you need is a way to make those functions work for more than one jingle.

One way to do that is to use input parameters for your functions. But since you need several parameters for each jingle using a struct is a good way to go.

I was thinking on these since I wrote this line:

if(currentNoteBJ > (sizeof(ButtonJingleNotes) / sizeof(ButtonJingleNotes[0])))

My idea was to make a Jukebox function :grin: where I will only have to pass both arrays, notes and its duration and "he" will do the rest, no matter how long the melody is. But my brain explode when I thought on give an array as a parameter and finally that sort of "chimera" was born from my keyboard :fearful:

I've take a look for the struct, as well as static, that could be the answer. But it seems a little bit more complex. Need a coffee and read it again slowly :cold_sweat:

Hutkikz:
EDIT: Just a note that in this line you multiply an integer and a floating point number then store the result in an integer. Probably not much of a problem here but it truncates the result which will cause errors in your calculations. Probably no big deal here, but could be in a more sensitive operation. Besides it's just poor programming.

GoForSmoke:
Don't write integer * 1.30 as that will generate floating point code.

Do write integer * 13 / 10 as long as integer * 13 is not > integer capacity.

Thanks for that, I'll fix it :wink:

GoForSmoke:
PS also tone( with duration ) blocks execution until the note is finished.

That explains why I couldn't press the button until the last note was played. However I thought that was an issue from Button.h maybe for handle debounces or whatever because I never used it before. I'll work on that, I know that if you play a note as 0 is just silence. I guess that I'll need to add a 0 note between every regular note or something to do the trick. The fact is that if you add a brief pause between notes, you are making them more "audible". At lest that's what I've read.

noiasca:
it seems, that a byte is enough.
it never changes, so make it const.

Sure, the max number I've seen in that sort of data is 32. Working on it!

noiasca:
int ButtonJingleNotes[] = {
NOTE_A7, NOTE_C8, NOTE_D8
};
check what's NOTE_xx is in reallity and if you really need an int. if the array doesn't change, make it const

Actually they are just numbers, representing the Hz of the tone. They go from 31 to 4978. I think that int is the correct type, doesn't it?

Thay array will not change, represents a melody to play. Could be const too, great!

noiasca:
#define Buzzer 11
nowadays i prefer to avoid define for pins, and I use:
const byte buzzer=11;

noiasca:
For your eyes only: variable names and function names should start with small letters

I'll do a research for this. I'll be back if I turn even more confused :stuck_out_tongue_closed_eyes:

Thanks for y'all! That was been helpful. Once I finished the code I will repost it here so you can see the improvements!

Best regards for all,
Wyzed.

6

See how this compiles (I didn't) and close to running it is.. should be close.

#include <pitches.h>
#include <Button.h>

const byte Buzzer = 11; // #define makes int by default

byte playState;

Button JoystickButton(2);

byte ButtonJingleNotes[] = {
  NOTE_A7, NOTE_C8, NOTE_D8
};

byte ButtonJingleNotesDuration[] = {
  32, 16, 16
};

byte currentNoteBJ = 0;

void ButtonJingle( byte playNote )  // you pass it what note to play
{
  static unsigned long startWait = 0;
  static unsigned long wait = 0;

  if ( wait > 0 )  // this is a 1-shot, it turns off when done
  {
    if ( millis() - startWait >= wait )     wait = 0;
  }
  else
  {
    switch ( playState )
    { 
      case 0 : // start playing  --- uses the 1-shot timer above      
      tone(Buzzer, ButtonJingleNotes[ playNote ]);
      wait = 1000 / ButtonJingleNotesDuration[ playNote ];  // timer set
      startWait = millis();  // start time set
      playState = 1;
      break;

      case 1 : // stop playing  --- uses the 1-shot timer above
      noTone(Buzzer);
      wait = noteDurationBJ * 13 / 10;  // timer set
      startWait = millis();  // start time set
      playState = 2;
      PlayingToneBJ = LOW;
      break;

      case 2 : // finished pause
      playState = 0;
      playJingle= 2;
      break;
  }
}


void setup() 
{
  JoystickButton.begin();
}

void loop() 
{
   if (JoystickButton.pressed()) 
   { 
      playJingle = 1;
   }

   if ( playJingle == 1 )
   { 
      ButtonJingle( currentNoteBJ );
   }

   if ( playJingle == 2 )
   {
      payJingle = 0;
      if ( ++currentNoteBJ >= notes )  currentNoteBJ = 0;
   }
}
7

Hi there!

I worked on my code, but I can't go any further.

#include <pitches.h>
#include <Button.h>

const byte buzzer = 11; 

Button joystickButton(2);

unsigned long currentMillis; // I'm planning to use it in some other functions, so I need to declare it in Global Scope.
bool playingButtonJingle = LOW; // What about this? It's the "flag" I use to call the jukebox() with the correct parameters. Could this be cleaner?

const int buttonJingleNotes[] = {
  NOTE_A7, 0, NOTE_C8, 0, NOTE_D8
};

const byte buttonJingleNotesDuration[] = {
  32, 42, 16, 21, 16
};

void setup() {
  joystickButton.begin();  
}

void loop() {
   currentMillis = millis();
   if(joystickButton.pressed()){playingButtonJingle = HIGH;}
   if(playingButtonJingle){ jukebox(buttonJingleNotes, buttonJingleNotesDuration
                            , (sizeof(buttonJingleNotes) / sizeof(buttonJingleNotes[0])));}
}


void jukebox(int notes[], byte duration[], int lenght){
    static bool playingTone = LOW;
    static byte currentNote = 0;
    static unsigned long previousMillis = 0, noteDuration = 0;

    if(!playingTone){
      noteDuration = 1000 / duration[currentNote];
      previousMillis = currentMillis;
      tone(buzzer, notes[currentNote]);
      playingTone = HIGH;     
    }
    else{
      if(currentMillis - previousMillis >= noteDuration){
        noTone(buzzer);
        currentNote++;
        playingTone = LOW;
        if(currentNote == lenght){ // I realize that I cannot get the size of a passed array, since I'm not passing an array but a pointer. So I'm passing the lenght instead.
          playingButtonJingle = LOW;
          currentNote = 0;
        }
      }
    }
}

I've tryed to implement the Sctruct with no success. I've got problems with dynamic arrays (Each melody has different lenghts) and Arduino said to me that they don't get along very well. This is what I've tryed so far:

struct Melodies{
int notes[];
byte durations[];
};

Melodies buttonJangle;

void setup(){
buttonJangle.notes[]= {NOTE_A7, 0, NOTE_C8, 0, NOTE_D8};
buttonJangle.durations[]= {32, 42, 16, 21, 16};
}

Just that returns me tons of errors that I were handling with no clue of what I was doing and after closing 20 tabs of StackOverflow, I go for the easy and rude solution. I didn't even try to call the function with the struct parameter.

By the moment the closest I'm capable of get is the code above, the obvious disadvantage is that I need to declare both arrays, notes and durations separately for each melody. And the function call looks a bit awful. I added two melodies more, so with five in total, that give us 10 differents unidimensional arrays (I've also tryed multidimensional with the same result) but right now, after several hours, I'm stuck. I'm not saying that struct or even multidimensional arrays won't work here, but I lack of knowledge about them.

I solved the thing about integers, since I added "virtual" pauses as silent notes, there is no need of calculating that pause. With this I've also avoided the duration parameter in tone() function. As well as the thing about static variables, now the Jukebox function looks clean for me. The variables were also dimensioned for its values... And I think that's all.

I leave here another jangle, maybe you will find it familiar. Just for example of how different a melody can be from another:

const int anotherJangleNotes[] = {
  NOTE_E6, 0, NOTE_G6, 0, NOTE_E7, 0, NOTE_C7, 0, NOTE_D7, 0, NOTE_G7
};

const byte anotherJangleNotesDuration[] = {
  12, 16, 12, 16, 12, 16, 12, 16, 12, 16, 12
};

Thanks y'all for your help, I think I'm going to sleep smarter today! But improvements are always available :smiley: I'm not giving up.

Wyzed.

8

as mentioned, you should check what how the notes are defined.
I found that link pitches.h · GitHub
therefore it's obvious that you need uint16_t or unsigned integer if you want play NOTE_C4 and higher.