Arduino time gets out of sync

Hello. I’ve made a sequencer that sends MIDI notes, it’s a step sequencer, each step can be changed individually. I’m having a little problem though. When i run it with all the steps at 500ms, i start a metronome at 500ms in Puredata, but the sequence by the arduino gets out of sync. I then tried with 502ms metronome and it was a bit better. I’m not sure if it’s constant, so i put a timer object in PD, to measure the time between each step… sometimes it’s 502ms, others 497ms, 500.5ms, and so on.
I suppose it’s my crappy code, i started to comment parts of the code to make it simple.
I’m using interrupts, on Arduino Uno… I just started with this timer things so i suppose there’s the problem…
These are the important parts of the code:

void setup(){
 //interrupt for note on
  attachInterrupt(0, noteon, RISING);
  //interrupt for note off
  attachInterrupt(1, noteoff, FALLING);
}
void loop(){

unsigned long currentMillis = millis();
//counter, 500 is for how long arduino "holds" the note on
//0 is the time for note off
if(currentMillis - previousMillis > (ledState ? 500 : 0)) {
    
    previousMillis = currentMillis;   
    ledState = !ledState;
    
//ledState triggers the interrupts
    digitalWrite(4, ledState);
    ;
    
  }
//this are the variables for reading the values from 3x8 potentiometers, and for leds
//i is for led number, j are the pots for note duration, k for pitch, and m for velocity

  if(leds>7 && j>7 && pitch>7 && vel>7){
    leds=0;
    j=0;
    pitch=0;
    vel=0;
  }


}
void parpadeo()
{

   nota = readMux(pitch);
   
   nota = map(nota, 0, 1023, 36, 78);
 
   velo = readMux3(vel)/8; // escala a 128
  
 
  MIDI.sendNoteOn(nota, velo, 1);
  //lights led
  shifter.setPin(leds, HIGH);
  shifter.write();

}


void parpadeo2()
{
  //note off
  
  MIDI.sendNoteOff(nota, 0, 1);
  //shut down led
  shifter.setPin(leds, LOW);
  shifter.write();
  //it increments by one for each step
  leds++;
  duration++;
  pitch++;
  vel++;
  
}

Function parpadeo sends note on and puts the led on
parpadeo2 shuts down the leds and sends note off

the rest of the code is a bit messy, variables and multiplexers. If needed i post it all.
Anyways, if i want to play this sequencer with other instruments at the same time, i would use midi clock. I’ll add midi clock in, but i would like to know the sync problem will get fixed with midi clock, or my code at the interrupt part maybe is too long? and it’ll get out of sync anyways?.

thanks

if(currentMillis - previousMillis > (ledState ? 500 : 0)) {

Two statements are sometimes better than one. This is one such case.

Comments are a good thing, too. What the heck is this doing? I can figure it out, but comments would have saved a lot of head scratching.

    ;

Useful. I wonder why you don't have more statements like this.

  if(i>7 && j>7 && k>7 && m>7){
    i=0;
    j=0;
    k=0;
    m=0;
  }

One letter global variable names suck. Use meaningful names.

Function parpadeo sends note on and puts the led on
parpadeo2 shuts down the leds and sends note off

Then why don't the names reflect that? What the hell does 2 imply?

or my code at the interrupt part maybe is too long?

An interrupt handler should do not much more than set a flag. Yours are doing a lot. That might be a problem.

PaulS:

if(currentMillis - previousMillis > (ledState ? 500 : 0)) {

Two statements are sometimes better than one. This is one such case.

Comments are a good thing, too. What the heck is this doing? I can figure it out, but comments would have saved a lot of head scratching.

    ;

Useful. I wonder why you don't have more statements like this.

  if(i>7 && j>7 && k>7 && m>7){

i=0;
    j=0;
    k=0;
    m=0;
  }



One letter global variable names suck. Use meaningful names.



Function parpadeo sends note on and puts the led on
parpadeo2 shuts down the leds and sends note off



Then why don't the names reflect that? What the hell does 2 imply?



> or my code at the interrupt part maybe is too long?


An interrupt handler should do not much more than set a flag. Yours are doing a lot. That might be a problem.

Whoops, sorry for the typos and uncommented lines. Actually i had it commented but in spanish, so i edited it when posted and screwed it up...
I also think the problem is on the interrupts... What exactly is "set a flag"?
Would be better to create a function and when the interrupt is activated, it runs the code inside the function instead of running everything inside the interrupt? That's what it comes to my head with the term "set a flag". I'll try.
thanks, i'll edit the original post to make it more readable

What exactly is "set a flag"?

Change the value of a variable (a flag) to indicate that the ISR has been triggered, then, in the main code respond to the changed variable. This ensures that ISRs are kept short and do not take long to run.

Two points

  1. millis is not incremented in while an interrupt is being handled ,

  2. You are using a processor with a 16 MHz clock. That only gives you 16,000 clocks per mill.
    a byte instruction takes 1 clock, an int takes 2 and long takes. Don't even try to think about floats.

None of the CPUs have multiply/ divide instructions so we have to use loops of adds/subtracts to do it

This makes map a very expensive function. Here's the code for map

long map(long x, long in_min, long in_max, long out_min, long out_max)
{
  return (x - in_min) * (out_max - out_min) / (in_max - in_min) + out_min;
}

This will take a significant fraction of a mill to do. Then add the time it take to execute the rest of your code!

Mark

I haven't figured out the details of how your sketch works, but as far as I can guess you're using loop() to toggle a digital output at regular intervals and then everything else is being done in interrupts - although exactly what and how, I don't see.

Using millis() to perform real-time timing and interrupts to perform sequencing/control strikes me as the exact reverse of a sensible interrupt-based real-time system.

I don't understand what the interrupts are used for at all in this case - I suggest that interrupts should be avoided unless/until it is necessary to use them, and the behaviour I can follow so far does not require them. Unless you have some compelling reason I've missed to use interrupts, I suggest you put as much of the functionality as you can in the main code i.e. setup and loop, and code called from them, and ideally put all the functionality there.

The method you're using for incrementing time is slightly flawed because you may not evaluate millis() at the precise moment that your condition becomes true. If there is any latency in recognising the event, all your subsequent timed activities would be delayed by the same latency and this would result in timing slip. This would be a better way to do it:

if(millis() - lastTime > interval)
{
   lastTime += interval;
   // other stuff you need to do at regular intervals goes here ...
}

The key point is that the next timing event is based on when the previous event became due, not when you noticed that it was due.

I suggest you also try to get rid of the 'zero delay' branch of your logic. If you want to toggle the 'LED' off some interval after turning it in, toggle it off after the defined interval. If you want to turn it off next time loop() runs (without keeping control over when that is) then turn it off next time loop() runs. But don't (I suggest) muck up your main timing logic with this bodged-on additional behaviour.

This would be an even better way to do it:

if(millis() - lastTime >= interval)

Very true!

Thanks guys. I modified it yesterday. I got rid of interrupts, and used a the millis with one time instead of two as suggested, the noteoff is in the else event. I used interrupts because i needed to be just a pulse. But i managed to do it now with just an if and a counter to 1. If the counter is 1 it's noteoff and it resets to 0. If the counter is 0 it sends noteon and does counter++. So it happens just once. Else, i would be sending midi noteons for 500ms for example.
I also made void functions for the code that was inside the interrupts.
I'll try what peterH and AWOL says about the millis. Although i don't understand what is the difference. Im not on my computer, maybe when i try it i'll understand it better.
thanks