Serial communication and interrupt advice

Good evening,

I have been reading this forum for a while and I have learned a lot. Thanks for that. I am fiddling around with Arduino for a while now and I decided to create a midi sequencer. I have created a proof of concept, still early stages and I already have a lot of stuff working. I am using an Arduino Mega clone for my project. The heart of the timing code for my sequencer is a function that gets called by a timer interrupt. The timer is configured in the init mode, called in the setup function in the Arduino project. Setting the mode to 1 starts the timer, setting the mode to something else disables the timer. I did some function pointer trickery to call the instance method callback when the timer triggers an interrupt. This all works and yields the results I want. I am a bit unsure about the Serial code.

The writeSerialMidi method should sent out bytes over Serial1. I have tested this in a normal piece of code without any interrupts involved and I receive the MIDI signals. I have read about interrupts and I see a lot of recommendations that interrupt routines should be as short as possible. I was thinking about introducing a method loop which gets called in the main loop of the program. In the interrupt method callBack I set a variable _checkMidi to true. When this variable is true I will sent out midi signals and then set the variable to false again. I declared this variable volatile because I have also read that variables that get updated in an interrupt routine should be set as volatile.

Can anybody give me some general guidance if my approach is correct or should I do something else ? I do want to mention that timing is critical for these MIDI events.

Below is the code for my sequencer class, it is still work in progress so some parts might be a bit messy. The DEBUG_PRINTLN and DEBUG_PRINT macros are defined in my debug header.

Thank you very much,

Ron

Header file

#ifndef Sequencer_h
#define Sequencer_h

#include "Arduino.h"

class Sequencer
{
  public:
    Sequencer(unsigned int bpm, bool *updateUI);
    init();
    void setTempo(unsigned int bpm);
    void setMode(uint8_t mode);
    bool setMidiNote(uint8_t note);
    uint8_t getTempo();
    uint8_t getMode();
    uint8_t getNote();
    uint8_t getStep();
    void nextStep();
    void writeSerialMidi(uint8_t cmd, uint8_t note, uint8_t velocity);
    void loop();
    static void isrCallback();
    static Sequencer* instance;

  private:
    uint8_t bank1[16];
    bool *_updateUI;
    volatile bool _checkMidi;

    uint8_t _midiData[3] = {0, 0, 0};
    uint8_t _previousMidiData[3] = {0, 0, 0};

    uint8_t _step = 0;
    uint8_t _mode = 0;
    unsigned int _ppqn = 16;
    unsigned int _notePulse = _ppqn / 4;
    uint8_t _tick = 0;
    unsigned int _frequency;
    unsigned int _bpm;
    unsigned long _prescalerCompare;
    void callBack();
    void calculateInterval();
};

#endif

C++ class

#include "Debug.h"
#include "Arduino.h"
#include "Sequencer.h"

ISR(TIMER3_COMPA_vect)          // timer compare interrupt service routine
{
  Sequencer::isrCallback();
}

// Quarter note one beat long.
// Half note two beats long.
// Whole note is four beats.
// Eighth note is half a quarter note.
// Sixteenth note is half an eight note.

Sequencer::Sequencer(unsigned int bpm, bool *updateUI)
{
  _bpm = bpm;
  _tick = 1;
  _updateUI = updateUI;
  _checkMidi = false;
}

Sequencer * Sequencer::instance;

void Sequencer::isrCallback()
{
  instance->callBack();
}

void Sequencer::loop() {
  if (_checkMidi) {
    // Do serial midi
    _checkMidi = false;
  }
}

void Sequencer::callBack() {
  // With a _ppqn set to 16 every tick is a note spot
  _tick--;
  if (_tick == 0) {
    nextStep();
    DEBUG_PRINTLN(_step);
    DEBUG_PRINT(millis());
    DEBUG_PRINTLN(" _notePulse");
    // Handle note every 250 milliseconds
    _tick = _notePulse;
    _midiData[0] = bank1[_step];
    _checkMidi = true;

  }
}

Sequencer::init() {
  instance = this;

  // Calculate the correct settings for the compare match register
  this->calculateInterval();

  // CPU frequency 16Mhz for Arduino
  // maximum timer counter value (256 for 8bit, 65536 for 16bit timer)
  // Divide CPU frequency through the choosen prescaler (16000000 / 256 = 62500)
  // Divide result through the desired frequency (62500 / 2Hz = 31250)
  // Verify the result against the maximum timer counter value (31250 < 65536 success) if fail, choose bigger prescaler.
  // TTCCRx - Timer/Counter Control Register. The prescaler can be configured here.
  // TCNTx - Timer/Counter Register. The actual timer value is stored here.
  // OCRx - Output Compare Register
  // ICRx - Input Capture Register (only for 16bit timer)
  // TIMSKx - Timer/Counter Interrupt Mask Register. To enable/disable timer interrupts.
  // TIFRx - Timer/Counter Interrupt Flag Register. Indicates a pending timer interrupt.
  // https://www.robotshop.com/community/forum/t/arduino-101-timers-and-interrupts/13072

  // Set up interrupt routine
  noInterrupts();
  TCCR3A = 0;
  TCCR3B = 0;
  TCNT3  = 0;
  OCR3A = _prescalerCompare;  // compare match register 16MHz/256/2Hz
  TCCR3B |= (1 << WGM12);     // CTC mode
  TCCR3B |= (1 << CS12);      // 256 prescaler
  //TIMSK3 |= (1 << OCIE3A);  // enable timer compare interrupt in setMode
  interrupts();               // enable all interrupts
}

uint8_t Sequencer::getMode() {
  return _mode;
}

uint8_t Sequencer::getStep() {
  return _step;
}

bool Sequencer::setMidiNote(uint8_t note) {
  if (_mode != 0) {
    return false;
  }
  bank1[_step] = note;
  return true;
}

uint8_t Sequencer::getNote() {
  return bank1[_step];
}

uint8_t Sequencer::getTempo() {
  return _bpm;
}

void Sequencer::nextStep() {
  if (_step == 15) {
    _step = 0;
  } else {
    _step++;
  }
  *_updateUI = true;
}

void Sequencer::setMode(uint8_t mode) {
  _mode = mode;
  if (mode == 1) {
    noInterrupts();
    TIMSK3 |= (1 << OCIE3A);
    interrupts();
  } else {
    noInterrupts();
    TIMSK3 &= ~(1 << OCIE3A);
    interrupts();
  }
}

void Sequencer::calculateInterval() {
  // The frequency of the chosen bpm rate
  // Example frequencies
  // 100 = 250 * 24 / 60
  // 80 = 200 * 24 / 60
  // 24 = 60 * 24 / 60
  // 12 = 30 * 24 / 60
  // 3 = 8 * 24 / 60
  // 2 = 7 * 24 / 60
  // 2 = 6 * 24 / 60
  // 2 = 5 * 24 / 60
  // 1 = 4 * 24 / 60
  // 1 = 3 * 24 / 60
  _frequency = _bpm * _ppqn / 60;

  // Initialize the interval in milliseconds
  // CPU frequency / prescaler = prescaler space
  // 16000000 / 256 = 62500     62500 / 2Hz = 31250     62500 / 100Hz = 625
  // 16000000 / 128 = 125000    125000 / 2Hz = 62500    125000 / 100Hz = 1250
  // 16000000 / 64 = 250000     250000 / 2Hz = 125000   250000 / 100Hz = 2500
  // 16000000 / 32 = 500000     500000 / 2Hz = 250000   500000 / 100Hz = 5000
  _prescalerCompare = 62500 / _frequency;
}

void Sequencer::setTempo(unsigned int bpm) {
  _bpm = bpm;
  calculateInterval();
  noInterrupts();
  OCR3A = _prescalerCompare;
  interrupts();
}

void Sequencer::writeSerialMidi(uint8_t cmd, uint8_t note, uint8_t velocity) {
  Serial1.write(cmd);
  Serial1.write(note);
  Serial1.write(velocity);
  DEBUG_PRINT(cmd == noteOffCommand ? "OFF " : "ON ");
  DEBUG_PRINTLN(cmd);
  DEBUG_PRINTLN(note);
  DEBUG_PRINTLN(velocity);
  DEBUG_PRINTLN("-------");
}

Did you encounter any problems?
Did you do tests that elaborates the limits of your code?

I'm coding for a long time but do still not know much about classes and how to write them.

After looking over your code I saw that there is some kind of callback-mechanism
and I saw that you have serial prints.
I did not dive deep enough into your code to analyse if these serial printing is called from inside your Interupt-Service-Routine (in short ISR)

If you do - even for debugging purposes - this is a bad idea and against the rule keep execution-time of the ISR as short as possible.

If you can't avoid using serial debug inside the ISR rise the baudrate to the highest possible value
and print only single bytes to keep the printing-time short.

Alternatives would be to switch IO-pins high-low and to record this with a storage-oscilloscope or a logic analyser. A Logic analysers 24 MHz 8 channel for $10 will do

best regards Stefan

The general advice is never to use Serial.print() in an ISR because it relies on interrupts and they are automatically disabled when in an ISR

Thank you for your answers.

For debugging I definitely did not follow the one byte rule. I was happily outputting all kinds of data, even in the ISR routines. I will remove that from the code and make sure that I will handle debug writes out of the ISR. I also did not set the baud rate properly, in my setup function I was setting it to 9600 baud.

Serial.begin(9600);

I see I can set it to 2 million baud, I will try setting the baud rate for the serial monitor to 2 million in the setup function and check if that works.

I have experimented a bit and setting a variable in the ISR and handle the actual serial in the main loop method works without delays. I now have a callback method which only manipulates some variables.

Method which handles the ISR

void Sequencer::callBack() {
  // With a _ppqn set to 16 every tick is a note spot
  _tick--;
  if (_tick == 0) {
    nextStep();
    // Handle note every 250 milliseconds
    _tick = _notePulse;
    _checkMidi = true;
  }
}

Method called in the main loop
The sequencer loop method is called in the main loop of the program. When _checkMidi is true the sequencer is working out what / if to send. When the variable is false it does nothing.

void Sequencer::loop() {
  if (_checkMidi) {
    _previousMidiData[0] = _midiData[0];
    _previousMidiData[1] = _midiData[1];
    _previousMidiData[2] = _midiData[2];
    _midiData[0] = bank1[_step];

    uint8_t midiNote = _midiData[0];
    uint8_t previousMidiNote = _previousMidiData[0];
    
    DEBUG_PRINT(millis());
    DEBUG_PRINTLN(" send midi");
    
    if(midiNote == previousMidiNote) {
      // If the data is the same, don't send anything
    } else if(midiNote == 0 && previousMidiNote > 0) {
      // Send a note off for the previous note
      writeSerialMidi(NOTE_OFF, previousMidiNote, VELOCITY_OFF);             
    } else if(midiNote > 0 && previousMidiNote > 0) {
      // Send a note off for the previous note
      writeSerialMidi(NOTE_OFF, previousMidiNote, VELOCITY_OFF);   
      // Send a note on for the current note
      writeSerialMidi(NOTE_ON, midiNote, VELOCITY_ON);     
    } else if(midiNote > 0 && previousMidiNote == 0) {
      // Send a note on for the current note
      writeSerialMidi(NOTE_ON, midiNote, VELOCITY_ON);     
    }
    DEBUG_PRINT(millis());
    DEBUG_PRINTLN(" after send midi");
    // Do serial midi
    _checkMidi = false;
  }
}

The logic analyzer also sounds good. I never worked with a logic analyzer but I will purchase one and experiment.

Thank you for your answer.

I will definitely stop using serial writes in the ISR. I will make sure that the debug writes get handled in the loop method. I will need to think about the most efficient way to solve it but I will probably make some macros that can handle this.