Go Down

Topic: Library for playing MIDI files from the Arduino (Read 6723 times) previous topic - next topic

marco_c

Dec 30, 2012, 03:17 am Last Edit: Jan 05, 2013, 12:55 am by marco_c Reason: 1
Update - See last posting.

I have been working on a class to read MIDI files (Standard MIDI Format) from an SD card and play them through a MIDI serial interface on the Arduino. My aim is to be able to play any MIDI file as long as it is properly constructed.

This code is now very stable but at a point where (a) I have been looking at it too long, and (b) I need input from people who know more about MIDI than I do.

The class generally works ok, but there are a few files that do not play well (or at all) and I am stumped as to what makes these files 'special'. All the MIDI files are playable on my Windows PC and I can't see why is in the data that makes them not play when I print out what is in the file (using the dump() method in the class).

All the library code, some example program files that call the library, and a collection of MIDI files are attached. There is a dependency on other libraries - MIDI, SdFat and Flash. Locations to get these is documented in MIDIFile.h. You should also have a working MIDI serial interface - OUT is the only channel used form the Arduino for this software.

The problems I am experiencing are probably best explained in the MIDIFile_Play example - some of the files play really well and other sound strange or don't even start. My feeling is that I am not getting some of the time signals correct as the Time Signature META event is only partially processed, but I can't see how what I have ignore affects anything (ignorance here, probabaly, as I am not a musician).

Happy to get feedback and or help on sorting these issues out from anyone with some free time and interest to test or review the code.

Thanks in advance.
Arduino libraries http://arduinocode.codeplex.com
Parola for Arduino http://parola.codeplex.com

robtillaart

Quote
but there are a few files that do not play well (or at all) and I am stumped as to what makes these files 'special'.

Make a midi2text sketch and you might see unsupported codes...

I have no midi board (yet) but will have a look at the code.
Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

robtillaart


starnges that an destructor calls initialize
Code: [Select]
MFTrack::~MFTrack()
{
close();
}

void MFTrack::close(void)
{
initialise();
}



bool MFTrack::getNextEvent(MIDIFile *mf)
is not handling all values between 0xf1 ... 0xff:

midi2txt - saw you have a dump mode (but it is a cooked dump not a raw dump)
Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

robtillaart

WARNING:
Code: [Select]
void MIDIFile::setMicrosecondPerQuarterNote(uint32_t m)
// This is the value given in the META message setting tempo
{
uint32_t x;

_microsecondDelta = m / _ticksPerQuarterNote;

// work out the tempo from the delta by reversing the calcs in calcMicrosecondsDelta
// m is alreay per quarter note
_tempo = (60 * 1000000L) / m;
}

x not used

BUG:
Code: [Select]

bool MIDIFile::isEOF(void)
{
bool eof = true;

for (int i=0; i<_trackCount; i++)
eof &= _track[i].getEndOfTrack(); // all must be true for eof to be true

return(eof);
}

is syntactically wrong as you should use the boolean && not the bitwise & (it just works because all trues seem to be 1)

and it can be faster
Code: [Select]

bool MIDIFile::isEOF(void)
{
for (int i=0; i<_trackCount; i++)
{
if (! _track[i].getEndOfTrack() ) return false;  // breaks at first false
}
return true;
}


Generic:
many loops use int i;   you could check if uint8_t is enough ?


Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

robtillaart


STYLE:

Code: [Select]


#if EVENT_TRACK_PRIORITY
// process all viable events from each track first - TRACK PRIORITY
#if DUMP_DATA
Serial.print(F("\n-- TRK "));
#endif // DUMP_DATA
for (uint8_t i=0; i<_trackCount; i++)
{
#if DUMP_DATA
int n = 0;

Serial.print(i, HEX);
#endif // DUMP_DATA
while (_track[i].getNextEvent(this))
{
#if DUMP_DATA
n++; // do all events from this track
#endif
}
#if DUMP_DATA
if (n>0)
Serial.print(F("\n-- TRK "));
#endif
}
#else
bool doneEvents;

#if DUMP_DATA
Serial.print(F("\n-- TRK "));
#endif // DUMP_DATA
do
{
doneEvents = false;

for (int i = 0; i<_trackCount; i++)
{
bool b;
#if DUMP_DATA
Serial.print(i, HEX);
#endif // DUMP_DATA
b = _track[i].getNextEvent(this);
#if DUMP_DATA
if (b)
Serial.print(F("\n-- TRK "));
#endif // DUMP_DATA
doneEvents |= b;
}
} while (doneEvents);
#endif // else EVENT_TRACK_PRIORITY

// save for next round of checks
_lastEventCheckTime = micros();

return;
}


So many #if 's makes me wonder if you could not better do something like
Code: [Select]

#if DUMP_DATA
#define dump.print(x) Serial.print(x)
#define dump.println(x) Serial.print(x)
#else
#define dump.print(x)
#define dump.println(x)
#endif

in the header and just call dump.print() everywhere => would give less conditionals in the code

Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

marco_c

#5
Dec 30, 2012, 08:50 pm Last Edit: Dec 30, 2012, 08:52 pm by marco_c Reason: 1
Thanks robtillart for taking the time to look and comment.

Quote
starnges that an destructor calls initialize

Initially I had some memory allocation going on and I had to free memory (that was being done in close()), so the destructor needed to call close(). I have renamed initialise() to resetTrack() which may give us all less of an uncomfortable feeling.

Quote
saw you have a dump mode (but it is a cooked dump not a raw dump)

There is a sketch that does a raw dump of the data in the chunks after the header, without using the library. I also use a utility called "MIDI File Analyzer" (picked up somewhere off a Google link) that basically agrees with my own dump(s) of the files, so I am have a high confidence the dumps are correct.

Quote
not handling all values between 0xf1 ... 0xff:

Yes, correct. These values are real time valuies that appear in a MIDI live stream as control messages but are not valid in a file - 0xf1 is undefined, 0xf2 is Start a sequence, 0xf3 is Continue sequence. 0xf4 is Stop sequence, 0xf5 is undefined, 0xf6 is Active Sense (like a watchdog timer), 0xff is Reset System. All others in this range should be processed.

Quote
x not used

A leftover from when I had an intermediate result being calculated.

Quote
syntactically wrong as you should use the boolean && not the bitwise &
and it can be faster

Agree. Suggested version also saves 1 byte of stack space :-)

Quote
many loops use int i;   you could check if uint8_t is enough ?

Good point, will check.

Quote
So many #if 's makes me wonder if you could not better

Great point here. I have done what you suggested before and I don't know why I didn't do it here as well. This will also make the code more maintanable, as an alternative output stream is easier to implement if needed.

I'll post an updated version of the library once implemented and tested.
Arduino libraries http://arduinocode.codeplex.com
Parola for Arduino http://parola.codeplex.com

robtillaart

Quote
not handling all values between 0xf1 ... 0xff:
Yes, correct. These values are real time valuies that appear in a MIDI live stream as control messages but are not valid in a file - 0xf1 is undefined, 0xf2 is Start a sequence, 0xf3 is Continue sequence. 0xf4 is Stop sequence, 0xf5 is undefined, 0xf6 is Active Sense (like a watchdog timer), 0xff is Reset System. All others in this range should be processed.

but that still means that when you encounter them in a stream you should handle them appropriately (skip/resync stream etc).
Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

marco_c

#7
Dec 30, 2012, 10:27 pm Last Edit: Dec 30, 2012, 11:12 pm by marco_c Reason: 1
I take your point, but they are actually not allowed to exist in the stream according to the Standard MIDI File documentation. If they are in there then either the file should be considered invalid (ie, abandon it) or the sync has to be to the next byte with a top bit not set, which should be the next deltaT.

The problem with MIDI files is that they are sequential - each note or event is defined in a time offset (deltaT) from the previous one and if you sync forward you get out of time sequence with respect to the other tracks. I actually had this happening early in the piece and it sounds awful.

I'll need to think about this one.

Edit - on reflection, the most straightforward way is to make the track invalid (ie, mark it as end of track) and stop playing it. This allows the other tracks to play on and the file is not totally lost.
Arduino libraries http://arduinocode.codeplex.com
Parola for Arduino http://parola.codeplex.com

marco_c

#8
Dec 30, 2012, 11:17 pm Last Edit: Jan 05, 2013, 12:55 am by marco_c Reason: 1
Updated version of the library is attached. This implements points discussed above plus some additional tidy up of the debug messages.
Arduino libraries http://arduinocode.codeplex.com
Parola for Arduino http://parola.codeplex.com

robtillaart

Quote
I take your point, but they are actually not allowed to exist in the stream according to the Standard MIDI File documentation.

There are many things in the world not allowed that still happen ...

if you encounter forbidden codes you should close the file immediately. In the big switch this should be the default: option to catch all non supported







Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

marco_c

#10
Jan 01, 2013, 08:13 am Last Edit: Jan 05, 2013, 12:56 am by marco_c Reason: 1
This version (0.4) now works with everything I have been able to throw at it. :)

I have reworked the timing section so that it is tighter and more accurate in keeping the music to time. There were also serious logical errors in the processing of MIDI run on messages that caused the some files to play badly or not at all (see my first post).

I think there is a bit of tidy up to do and then it can be released as v1.


Arduino libraries http://arduinocode.codeplex.com
Parola for Arduino http://parola.codeplex.com

robtillaart

Good to hear it is working now!
I wait until after the cleanup is done before reviewing again ;)
Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

marco_c

This work is now completed and can be found at  http://code.google.com/p/arduino-code-repository/

Thanks to robtillaart for his great comments and feedback during development.

The library is used to process MIDI files from the SD card. MIDI and SYSEX events are passed to the calling program through callback functions for processing. This allows programmers the flexibility to implement a message to a MIDI synthesiser through serial interface or other output devices,
like a MIDI shield or custom hardware.

Processing of files may may be paused and restarted using methods in the library.

Download the library and see MIDIFile.h for more information if you are interested.
Arduino libraries http://arduinocode.codeplex.com
Parola for Arduino http://parola.codeplex.com

robtillaart


Hi,
minor issue, the midihelper.h has no construct like

#ifndef _MIDIFILE_H
#define _MIDIFILE_H

in midifile.cpp ~line 270
Code: [Select]

for (n = 0; (n < 100) && (!doneEvents); n++)  //<<<<<<<<<<<<<<<<<<<<
{
doneEvents = false;

for (uint8_t i = 0; i < _trackCount; i++) // cycle through all
{
bool b;

if (_format != 0) DUMPX(i);
// Other than the first event, the other have an elapsed time of 0 (ie occur simultaneously)
b = _track[i].getNextEvent(this, (n==0 ? elapsedTime : 0));
if (b && (_format != 0))
DUMPS("\n-- TRK ");
doneEvents |= b;
}
n++;  //<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< extra n++ in the loop? why ?

// When there are no more events, just break out
if (!doneEvents)
break;
}


in int MIDIFile::load()
if there is an error should you not close the file ?
+ some var names are still cryptic e.g. c  (which is a string containing part of header)



shorter?
Code: [Select]
uint32_t readVarLen(SdFile *f)
// read a variable length parameter from the input stream
{
  uint32_t  value = 0;
  char      c;
 
  do
  {
    c = f->read();
    value = (value << 7) + (c & 0x7f);
  }  while (c & 0x80);
 
  return(value);
}


buffer with "position info"
Code: [Select]
void dumpBuffer(uint8_t *p, int len)
{
  for (int i=0; i<len; i++, p++)
  {
    if (i% 8 ==0)
{
   DUMP('\n');    // << #DEFINE DUMPLN(T) Serial.println(T) ??
   DUMP(i);
   DUMP('\t');
}
    DUMP(' ');
    if (*p<=0xf)
DUMP('0');
    DUMPX(*p);
  }
}


finally there is a style diff between the #ifdef used for void MFTrack::dump(void) and  void MIDIFile::dump(void)

(end of nitpicking ;)
Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

marco_c

Thanks again for the review (nitpick?) :)

Quote
midihelper.h has no construct like

OK. It has now, but it is only ever included in one file which is protected from re-inclusion.

Quote
n++;  //<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< extra n++ in the loop? why ?

Left over from when it was a while loop. Should not affect the functionality though, as the for loop is just a way to limit too many events. Effectively halves the (arbitrary) limit.

Quote
if there is an error should you not close the file ?

Yes absolutely. Interesting is that SDFat did not fail over eepeated teationg with a fle that was not correct. It must handle the case of the open file (or maybe it just doesn't care?).

Quote
some var names are still cryptic e.g. c

This is picky. The char array is local to a block of 5 lines of code! To keep you happy I have renamed it 'h' :P

Quote
shorter?

Clearly so. I'll do some testing as the original code was proposed as the right code to process the varlen parameters by the MIDI organisation.

Quote
#DEFINE DUMPLN(T) Serial.println(T) ??

The philosophy adopted in the DUMP output is that the \n is at the start of the new line, not at the end of the previous one. This makes it cleaner as I can't always tell where the end may be but am sure when I start a new line. So I need to explicitely use '\n' when I want it. It also makes it easier if I want to add debug as I don't need to worry about the line endings sprinkled through the code.

Quote
style diff between the #ifdef used for void MFTrack::dump(void) and  void MIDIFile::dump(void)

I assume you mean in the header file for the class definition?
I wanted to keep the dump function prototype available at all times so that I don't need #if in the sketch code around a call to the dump() method, even if it is an empty function. However, in the classes, the #if switch will cut out all the code for the MFTRack class, as this is only called by MIDIFile.dump().
I suppose an alternative is an empty inline definition in the class dependent on the #if, but the result isthe same.
Arduino libraries http://arduinocode.codeplex.com
Parola for Arduino http://parola.codeplex.com

Go Up