Puzzled about bit of code

So I'm working on a simple MIDI synth, and this one part of the program has got me puzzled as to why it doesn't work.
I took out a lot of the program, to isolate the problem. So now, when any MIDI note is received, it should play a 440 hertz tone. (or some octave of it).
I'm using 2 timers to create the tone. One to create a PWM signal that is filtered to get an analog output. The other runs a interrupt function that updates the output.

Here is the code:

#include <WProgram.h>
#include <MIDI.h>
#include "osc.h"


MIDI_Class input;
osc osc1;

volatile int currentIndex;
int currentWave = 0;

volatile unsigned long clock;

void setup()  {
  cli();
  pinMode(11, OUTPUT);  //set OC2A pin to output

  TCCR2A |= 1 << WGM20;  //     \
  TCCR2A |= 1 << WGM21;  //   }- Set timer to phase fast PWM mode
  TCCR2B &= ~(1 << WGM22);  //  /  

  TCCR2A |= 1 << COM2A1;  //      }- Set to non-inverting PWM
  TCCR2A &= ~(1 << COM2A0);  //  /

  TCCR2B |= 1<< CS20;  //      \
  TCCR2B &= ~(1 << CS21);  //   }- Set to prescalar of 1 (no prescalar).
  TCCR2B &= ~(1 << CS22);  //  /           62500 Hz PWM

  //============================================================================

  //Setting up timer1 for CTC mode
  TCCR1A &= ~(1 << WGM10);  
  TCCR1A &= ~(1 << WGM11);
  TCCR1B |= 1 << WGM12;
  TCCR1B &= ~(1 << WGM13);

  //Prescalar 1
  TCCR1B |= 1<< CS10;
  TCCR1B &= ~(1 << CS11);
  TCCR1B &= ~(1 << CS12);

  unsigned char sreg;
  sreg = SREG; 
  OCR1A = 199; 
  SREG = sreg;

  TIMSK1 |= (1 << OCIE1A); // Enable CTC interrupt 
  sei();

  pinMode(6, OUTPUT);
  digitalWrite(6, HIGH);

  input.begin(MIDI_CHANNEL_OMNI);
  input.turnThruOff();

  //osc1.setFreq(clock, 440);
}


void loop()  {
  //Waveform select code  \/  \/  \/

  int reading = analogRead(5);
  if(reading < 300)  {
    osc1.setWaveform(0);
  }
  else if(reading > 700)  {
    osc1.setWaveform(2);
  }
  else {
    osc1.setWaveform(1);
  }
  //End waveform select




  //osc1.setFreq(clock, 440);




  //MIDI handling code \/ \/ \/

  if(input.read() == true)  {
    osc1.setFreq(clock, 440);

  }

  //End MIDI handling code

}


ISR(TIMER1_COMPA_vect)  {
  clock += 10;
  byte output;
  output += osc1.refresh(clock);

  OCR2A = output;
}

Here is osc.h:

class osc  {
public:
  osc();
  byte refresh(unsigned long clock);
  void setWaveform(int waveform);
  void setFreq(unsigned long clock, int hertz);

  static const byte _waves[3][16];



private:

  int _currentStep;
  unsigned long _ticks;
  unsigned long _lastStep;
  int _waveform;
  byte _on;

};


const byte osc::_waves[3][16]  = {
  {
    0x0, 0x2, 0x4, 0x6, 
    0x8, 0xa, 0xc, 0xe, 
    0xf, 0xe, 0xc, 0xa, 
    0x8, 0x6, 0x4, 0x2
  }
  ,
  {
    0xf, 0xf, 0xf, 0xf, 
    0xf, 0xf, 0xf, 0xf, 
    0x0, 0x0, 0x0, 0x0, 
    0x0, 0x0, 0x0, 0x0
  }
  ,
  {
    0xf, 0xe, 0xd, 0xc, 
    0xb, 0xa, 0x9, 0x8, 
    0x7, 0x6, 0x5, 0x4, 
    0x3, 0x2, 0x1, 0x0
  }

};

osc::osc()  {
}

byte osc::refresh(unsigned long clock)  {
  if(clock - _lastStep >= _ticks)  {
    _currentStep = (++_currentStep)%16;
    _lastStep += _ticks;
  }
  return _waves[_waveform][_currentStep] *_on;
}

void osc::setWaveform(int waveform)  {
  _waveform = waveform;
}

void osc::setFreq(unsigned long clock, int hertz)  {
  if(hertz > 0)  {
    _ticks = 400000UL /(hertz * 4UL);
    _on = 1;
      _currentStep = 0;
    _lastStep = clock;
  }
  else  {
    _on = 0;
  }
}

Inside the if(input.read() == true) statement, the call to osc1.setFreq does not work.
MIDI notes are received correctly, because if I put a tone(11, 440) in the if statement, it makes a tone when I send it a midi note.
And the osc class does work correctly. If I call the setFreq function in either of the 2 places it is commented out (at the end of setup, or in the middle of loop), it plays the tone as expected. (although in loop, it sounds strange because it is called repeatedly)

So what would cause the function to not function inside the if statement?

If I understand your description correctly, the implication is that it is the input.read() that is causing the problem. Does it use/change the timer/interrupt that you are using to generate the audio? I had a quick look at the source for the MIDI library and couldn't see anything obvious - but I'm not an expert.

You could try commenting out chunks of the read() function in the MIDI library to isolate the cause?

I looked thru the midi source, and I also couldn't find anything.

The midi.read function couldn't be interfering with interrupts, because a call to the setFreq function outside the if statement works.

I looked through the source, and I think your issue is that the return of input.read() is only true momentarily. Thus, as soon as the code loops back through again, setFreq() is not called, and no tone is played. Try replacing the if (input.read() == true) with if (1) and I think it should work. The way around this is to set a flag based on input.read() and service it each loop.

Hope this works

I'll try that, but I don't believe that is the problem.

Once setFreq is called,the tone plays continuously. The only time the tone will not play is if you set the frequency to 0.
Calling it within setup() will yield a continuous tone.
Also calling tone() within the if statement brackets causes a continuous (assuming it was called with unspecified duration) square wave tone.

Oh I wasn't aware that setFreq plays a tone indefinitely. The only thing I can think of is that the MIDI library's timer interrupts are interfering with yours.

This may not have anything to do with the problem you're trying to fix but a question occurred to me:

Is there a potential problem with the ISR calling refresh() while the main loop() is in the middle of a call to setFreq(). I mean, could setFreq be in the middle of updating the control variables when it is interrupted by the timer/ISR and an inconsistent set of variables is used in refresh()? It's a classic multi-threading problem but I don't really know whether it applies here.

You're right, those variables should be declared volatile.

Does the whole class need to be declared volatile?

I'll change that, whether or not it's the cause of my problem.

I've never used volatile so I can't answer your question. I've only ever solved this using mutexes, which isn't an option here. I suppose you could use a boolean variable as a mutex but I'm not sure that would be 100% safe.

ISR(TIMER1_COMPA_vect) {
clock += 10;

byte output;
// At this point, output contains garbage. It needs to be static or initialized or the following needs to be a simple assignment...
output += osc1.refresh(clock);

OCR2A = output;
}

Does the whole class need to be declared volatile?

The situation is much more complicated than attaching volatile to the class. As far as I can tell, each element has to be individually declared volatile. In addition, sharing osc1 with an interrupt service routine creates several race conditions. I strongly suggest trying to get your application working with no osc1 access in the interrupt service routine.

The alternative is to end up like this guy...

hair-loss-due-to-stress.jpg

I thought that "byte output" would automatically initiate output as 0. I'll try explicitly initializing it to 0.

As for the osc problem, I'll change it to not use the class. I just thought it would make it simpler to change the number of oscillators, just initialize a new one.

Locals / automatics are not initialized. Statics / globals are.

You are correct about using a class / object. But, combining a shared object instance with an interrupt service routine is difficult path.

So does the local variable without initialization just contain whatever happened to be previously allocated to that slot in the memory?

...or register ... yes.