Midi noteOn and millis

Hello,

I cant seem to understand why I cannot make my code work with millis. I am sure it is something simple but I am not seeing where the problem is.

What I want to do is the following:

for (int i=0; i<n_steps; i++){
     
      noteOn(0, preset0_notes[i], 127);
      unsigned long now = millis();
      unsigned long noteOn_start=0;
    
      if (now - noteOn_start >= 1000){
        noteOff(0, preset0_notes[i], 0);
        noteOn_start = now;
      }
    }

The code above does not work and I am not really sure what is wrong with it. It is easy to do with delay, but I really need to have the arduino running without interruptions.

for (int i=0; i<n_steps; i++){
     
      noteOn(0, preset0_notes[i], 127);
      delay(1000);
      noteOff(0, preset0_notes[i], 0);
}

Can anyone help me with this?

The code above does not work

Yes the code above works. It might not do what you want it to do it does what you have written it to do.
What it will do is that the if clause will always return false, and so the note off will never be sent, because just before it you set the now variable to the current time and the start variable to zero. So the current time minus zero will always be zero and will never exceed 1000.

We ask you to post all your code so we can see the wider context.
You are obviously doing other things wrong in the code you have not posted.

I don't understand why the current time minus zero will always be zero. Shouldn't the variable "now" increase each time the system loops until it reaches a value high enough so that the if condition will be triggered?

I will post the full code as soon as i have access to my laptop. Thanks

I have included a zip file with all the required h files. The main sketch is too big to be posted here.
Thanks.

Midi_Stylophone_Synthesizer.zip (8.25 KB)

noteOn_start needs to have global extent, you are creating it afresh each time
into the function, make it a global variable.

The "extent" of a variable is how long it exists for - variables local to a function vanish
when the function returns, called local extent.

I don’t understand why the current time minus zero will always be zero.

Because your sequence of operations are:-

for (int i=0; i<n_steps; i++){
      noteOn(0, preset0_notes[i], 127);
      unsigned long now = millis();
      unsigned long noteOn_start=0;
   
      if (now - noteOn_start >= 1000){

So you reset the variables “now” and “noteOn” IMMEDIATELY every time before the “if” statement so there is no time for millis() to advance. Every time you do the “if” the variables have just been reset.

Also your variables “now” and “noteOn” need to be separate variables for each note that is playing, this means that these variables have to be arrays. They should be set ONLY when the note on message is sent for the first time, so you need another array that holds if a note on message has been sent or not so you will know not to sent the note on message every time round that loop. You reset this variable when you send the note off message at the appropriate time.

One quick and dirty way round this is to always send a note off message before you send the note on message, this works for sequencers but not for keyboards.

MarkT : Ok, thanks. I did not know that. I am still very confused in general about how to actually define and use the variables, but I learn as I go.

Grumpy_Mike: After reading your answer I realized that I might be complicating things going on this direction. After researching the forum for alternatives I came across this topic which I can relate to:

After trying to understand the code, I kind of adapted it to my code, with noteOn and noteOff instead of using LEDs. I think right now this approach is making more sense to me and was finally able to get the code to do what I intended. I believe there are still alot more adjustments, but I want to dig more into using STATES and CASE.

This is the code I got working and does what I wanted:

void handleNotes(){

  static uint32_t previousMillis = 0;
  static byte state = 0;
  if (millis() - previousMillis >= preset0_durations[state])
  {
    state++;            // its time to move to next state
    state = state % 8;  // setting state limit value 8 (state = 0 until state = 7)

    switch (state)      // act according to the state
    {
      case 0:
        noteOff(0, preset0_notes[7], 0);
        noteOn(0, preset0_notes[0], 127);
        break;

      case 1:
        noteOff(0, preset0_notes[0], 0);
        noteOn(0, preset0_notes[1], 127);
        break;

      case 2:
        noteOff(0, preset0_notes[1], 0);
        noteOn(0, preset0_notes[2], 127);
        break;

      case 3:
        noteOff(0, preset0_notes[2], 0);
        noteOn(0, preset0_notes[3], 127);
        break;

      case 4:
        noteOff(0, preset0_notes[3], 0);
        noteOn(0, preset0_notes[4], 127);
        break;

      case 5:
        noteOff(0, preset0_notes[4], 0);
        noteOn(0, preset0_notes[5], 127);
        break;
        
      case 6:
        noteOff(0, preset0_notes[5], 0);
        noteOn(0, preset0_notes[6], 127);
        break;

      case 7:
        noteOff(0, preset0_notes[6], 0);
        noteOn(0, preset0_notes[7], 127);
        break;
    }
    previousMillis = millis();
  }
}

Thanks everyone for the help. I think I can proceed with my project now :smiley:

This is the code I got working and does what I wanted:

Have you noticed that you are doing the same thing in each case?
That means you can simply replace the whole of that case structure with what is in just one case statement. By choosing the index to those arrays to be variables not constants you can remove the whole case structure.

Anyway the effect of this is that you only hold a note until you get to play the next note, which is not what you set out to do.

Are you just doing this because you can't understand reply #5?

Grumpy_Mike:
Have you noticed that you are doing the same thing in each case?

Yes.

Grumpy_Mike:
That means you can simply replace the whole of that case structure with what is in just one case statement. By choosing the index to those arrays to be variables not constants you can remove the whole case structure.

I realized that and tried the following approach:

    int tempo = ((analogRead(A3))/128);

    static uint32_t previousMillis = 0;
    static byte state = 0;
    static byte i = 0;
    
    if (millis() - previousMillis >= preset_durations[i] * tempo){
      
      state++;            // its time to move to next state
      state = state % 2;  // setting state limit value 2 (state = 0 until state = 1)  
      
      switch (state)      // act according to the state
      {       
        case 0:                               // This state the note starts
          noteOn(0, preset_notes[i], 127);
          lcd.clear();        
          lcd.setCursor(0,0);
          lcd.print(preset_notes[i]);
          lcd.setCursor(i,1);
          lcd.write(0);
          break;
  
        case 1:                              // This state the note finishes
          noteOff(0, preset_notes[i], 127);
          lcd.clear();        
          lcd.setCursor(0,0);
          lcd.print(preset_notes[i]);
          lcd.setCursor(i,1);
          lcd.write(0);
          i++;
          i = i % n_preset_notes;
          break;          
      }
      previousMillis = millis();
    }

Its is not great but it is almost doing what I wanted.

Grumpy_Mike:
Are you just doing this because you can't understand reply #5?

Yes. I cannot fully understand what you wrote. I understood that the noteOn_start=0 is being reset in each loop, but the now = millis() I cannot understand why you say it resets. Shouldn't the variable "now" update its value each loop? (increasing the value as time passes)?

but the now = millis() I cannot understand why you say it resets. Shouldn't the variable "now" update its value each loop?

Yes it will sorry for that confusion. But why use a variable anyway why not just replace now with millis()?

With this:-

switch (state)      // act according to the state
    {
      case 0:
        noteOff(0, preset0_notes[7], 0);
        noteOn(0, preset0_notes[0], 127);
        break;

I was thinking more of this sort of thing:-

// declare these as global or static variables
nextNoteOn = 0;
nextNoteOff = 7;
// then in place of the whole case statement use
        noteOff(0, preset0_notes[nextNoteOff], 0);
        noteOn(0, preset0_notes[nextNoteOn], 127);
// then add these two lines:-
nextNoteOn = (nextNoteOn +1) & 0x7; // increment variable but keep it in the range 0 to 7
nextNoteOff = (nextNoteOff +1) & 0x7; // increment variable but keep it in the range 0 to 7

Thanks Mike. Your sugestion makes sense. However I am now trying to not be limited to 8 notes sequence. I am currently goig with my last suggestion and get it to run better since I can have a variable number of notes in a sequence.

I have come to another issue again but its for another topic later. Thanks again