Moederbeer commented 5 years ago

I'm working on updating the code to Python 3 and adding coreXY support. I'm however running into the issue that I'm always getting:

Pause for 0.00 seconds G4 P0.0000

If I just print out the duration values I get values for midi notes but rests are always 0 long. I get the same when running your original script.

Added the output.code as a text file, I just ran ./mid2cnc.py --verbose and got this output on your source. output.txt

michthom commented 5 years ago

Hi Paul I've not touched this code in a looooooong time, but send me a link to your repository and I'll see if I can spot anything?

Moederbeer commented 5 years ago

My repo is here: https://github.com/Moederbeer/MIDI-to-CNC please note I've never coded in Python before and spent most of my time just making the code compatible with Py3 and coreXY.

Your original code exhibits the same behaviour.

michthom commented 5 years ago

So, tracing backwards, if duration == 0.000 when the rest is processed that could mean:

Line 504 of mid2cnc-python-midi.py note[0] (timestamp of the current note) == last_time (the timestamp of the previous note processed.

But then if that were true, the if statement at line 461 would be false, so that whole block should be skipped!

Very odd, indeed. I can't see anything obviously wrong, but I'm no Python ouru either. Looks like I'll have to recreate an environment and try this too. Race you? ;-)

I'd start by enabling all of the debugging output (with the --verbose flag), and put an extra print statement in just before line 504 to print out those two and see if either last_time is being clobbered or the note[0] is picking up a matching timestamp?

Moederbeer commented 5 years ago

Thank you for looking at this with me though, I'm obsessed with this piece of code more than is healthy for me. Mind you that I'm running the plain mid2cnc.py, not mid2cnc-pyton-midi.py.

I've been fighting with this for a while now and have little experience in working with binary data, let alone midi files but I'm trying my best. I'll sure accept the challenge of completing this first but my guess is that I won't be able to beat you!

I also find it odd what you've explained above, I've found that part too and am adding prints everywhere I feel an issue could be.

Edit, got a fourth axis (extruder) working. :)

michthom commented 5 years ago

Hi Paul OK, I have cloned your repo and run the following to get some debug info to play with:

python3 -d ./mid2cnc.py --verbose > ~/Desktop/midi2cnc.txt

Attached as midi2cnc.txt

Then I added some more debug just before the duration gets set for each note in the active_notes:

                if args.verbose:
                    print ("note[0]: %7.3f" % note[0])
                    print ("last_time: %7.3f" % last_time)
                    print ("midi.division: %7.3f" % midi.division)
                    print ("tempo: %7.3f" % tempo)

That proved the duration of the previous note wasn't zero:

Warning: tried to turn off note that wasn't on!
note[0]: 635.000
last_time: 572.000
midi.division:  96.000
tempo: 252100.000
Chord: [220.000,   0.000,   0.000] in Hz for  0.17 seconds at timestamp 635
 Feed: [1121.781,   0.000,   0.000] XYZ mm/min and  1121.78 combined
Moves: [  3.093,   0.000,   0.000] XYZ relative millimetre
G01 X128.0200172199 Y139.6416573795 Z9.9319348834 F1121.7812526557

0
Pause for 0.00 seconds
G4 P0.0000

The warning suggests to me that since there are 3 axes (for the default cupcake) but 5 channels in the default input.mid file, we're not operating on both ends of a chosen note... e.g. if we don't 'play' the start of a note, we shouldn't be processing the 'off' event for that note. Hmm.

michthom commented 5 years ago

When parsing the midi file, the original channel information is not being included in the noteEventList. This does need better handling. If we choose to include a note_on from channel X then we must use the note_off from the same channel as the end of that move... The more I look back at this the more I realised I didn't understand what I did when I cloned it myself!

michthom commented 5 years ago

OK. I can hardly believe this code lasted as long as its has without someone pointing out how truly deeply and madly flawed it is, under the hood. What I propose to do is to rip up the internals and try this approach:

Parse the midi file looking for events (unchanged)
If we see a note on, add it to a temporary list of "active" notes (only used while parsing midi), unless that channel already has a note playing (if so, discard the event)
When we see a note off, discard the event if we have no active note playing on that channel at this time.
Otherwise, find the matching entry "playing" on in that channel, calculate the note duration and add an entry to our list for gcode conversion (removing it from the temporary list). If this note is identical (start, end, pitch, velocity) to one already in the list for gcode conversion, but on another channel, then discard it.
Once we get to the end of the midi file, we sort all the notes in our filtered list by the start time, and then by frequency (descending) because the higher notes are easier to hear so we discard bass notes when we run out of channels to play them on.
THEN we start creating the GCode... Loop while there are notes left to play...
While we have any axis not busy playing a note, we take the next note off the list, figure out the frequency and distance for the GCode entry, and the timestamp when this note will end (so we're tracking end time per axis now)
Discard all notes off the queue that start before the lowest of the axis end times, since we're busy and can't play those.
loop back around and keep filling the axes up as their notes end.

What do you think?

michthom commented 5 years ago

Having sprinkled some more print statements into the code, and tried some other pieces that have actual gaps in their melody I think that we're seeing ghosts (and my logic above was naive, since we need to "play" whole notes as a concatenation of GCode moves, to account for longer notes playing over shorter ones. Otherwise I'd be throwing away valid tones from the melody.)

If there are no rests in the midi file, the current code generates zero-length pauses as groups of notes end together and others start immediately afterward. We can fix that by only generating a pause command if the duration is non-zero.

I'll try that and see if it fixes the problem!

Moederbeer commented 5 years ago

Wow, I've been away for the past two days but it seems like you've done a ton of work on this! Great! I feel like I won't be able to help much with this as I have a lot less understanding of the midiparser and midi in general than you do. I'll make sure to clean up the code later and add some proper coreXY args so the code is multifunctional for cartesian and coreXY with the added E axis.

michthom commented 5 years ago

I've committed two tiny changes to just skip the generation of unnecessary (zero length) pauses - can you merge into your Python3 code and see if that fixes things for you?

Moederbeer commented 5 years ago

I only see one change in your commit. Adding this just omits all pauses from being added since duration==0 when a pause has to be added in, so duration > 0 doesn't work in this case.

michthom commented 5 years ago

OK, I've clearly messed up again. I committed one change to mid2cnc-python-midi.py and another to mid2cnc.py . Do you have an example .mid file that contains a silent period I can test with please? Please upload to this ticket when you can?

Moederbeer commented 5 years ago

I'm sorry for being absent for a couple of days, had some stuff I needed to do. I made a very simple test midi file which has one 1/4 note, then 3/4 rest followed by another 1/4 note.

The file is here: https://github.com/Moederbeer/MIDI-to-CNC/blob/master/midi_files/test.mid

When running this with your original rest code I get this:

Chord: [130.813,   0.000,   0.000,   0.000] in Hz for  0.50 seconds at timestamp 192
 Feed: [ 98.110,   0.000,   0.000,   0.000] XYZE mm/min and    98.11 combined
Moves: [  0.818,   0.000,   0.000,   0.000] XYZE relative millimetre
G01 X147.5780289833 Y147.5780289833 Z1.0000000000 E0.0000000000 F98.1095869877

Pause for 0.00 seconds
G4 P0.0000

Chord: [130.813,   0.000,   0.000,   0.000] in Hz for  0.50 seconds at timestamp 960
 Feed: [ 98.110,   0.000,   0.000,   0.000] XYZE mm/min and    98.11 combined
Moves: [  0.818,   0.000,   0.000,   0.000] XYZE relative millimetre
G01 X148.1560579667 Y148.1560579667 Z1.0000000000 E0.0000000000 F98.1095869877

When running with your updated code I get this:

Chord: [130.813,   0.000,   0.000,   0.000] in Hz for  0.50 seconds at timestamp 192
 Feed: [ 98.110,   0.000,   0.000,   0.000] XYZE mm/min and    98.11 combined
Moves: [  0.818,   0.000,   0.000,   0.000] XYZE relative millimetre
G01 X147.5780289833 Y147.5780289833 Z1.0000000000 E0.0000000000 F98.1095869877

Chord: [130.813,   0.000,   0.000,   0.000] in Hz for  0.50 seconds at timestamp 960
 Feed: [ 98.110,   0.000,   0.000,   0.000] XYZE mm/min and    98.11 combined
Moves: [  0.818,   0.000,   0.000,   0.000] XYZE relative millimetre
G01 X148.1560579667 Y148.1560579667 Z1.0000000000 E0.0000000000 F98.1095869877

The pause should be 0.75s in this case. duration is always 0 when no notes are playing. It looks like your code isn't calculating duration for inactive notes. Does this have to do with the for loop around duration maybe? for i in range(0, min(len(active_notes.values()), active_axes)): Doesn't this only calculate for active_notes?

michthom commented 5 years ago

Hi Paul Thanks for this, I'll take a look in the coming days, and see if I can find a better solution. Might have to rethink the logic more thoroughly after all! Mike

michthom commented 5 years ago

Hah. I was struggling to create a simple MIDI file in Garageband (fail) but then Google rescued me with the Online Sequencer so I can create test files easily now. Will try to look at this over the weekend, but I'm hoping this one exercises the right parts of the code - a rising arpeggio should play the lower three tones as they are added, and there won't be a spare axis for the octave. Then a rest to test the pause generation, then a chord that should select the top three tones to play. Untitled _1547748324901.mid.zip

Moederbeer commented 5 years ago

I took a look at your midi file, looks good and should test everything. I wonder if it would be trivial enough to add an option to only play the highest note on every track instead of the highest notes in general. I've noticed that some midi files which have many chords don't sound so good. If you would have only the higest notes of those chords playing you would maybe get more different melodies to represent the entire song.

I really appreciate you taking a look at this again, I wish I could do more to help. Maybe I'll try digging into my python skills as well this weekend and see if I can come up with something.

Moederbeer commented 4 years ago

So with being quarantined and all I took another look at the midi2cnc script. I've updated it to use the mido library and found the error causing rests to be 0s.

The else statement handling rests does not calculate the duration of this rest as this duration is calculated only on active notes. Thus it reverts to it's init of 0 ever loop. I've fixed this by adding the duration calculation to the 'rest' section only changing tempo / 1000000.0 to tempo / 1000.0 to account for ms instead of seconds.

Thanks again for this program, I'm having a lot of fun with it. I will upload a fork with python 3 support, coreXY support and mido when it's done.

michthom commented 4 years ago

Hi Paul

I’m impressed by your continued interest and contributions, but as this was a project that I forked from upstream to fiddle with, as Douglas Adams might be misquoted “It’s thanks all the way up!”

When you’re done, I think the best move for me might be to refer anyone that comes across my version towards yours, as I’ve not done any active work on my fork for a VERY long time, quarantine or no…

Ain’t open source grand?!

All the best Mike

On 21 Apr 2020, at 20:47, Paul de Groot notifications@github.com wrote:

So with being quarantined and all I took another look at the midi2cnc script. I've updated it to use the mido library and found the error causing rests to be 0s.

The else statement handling rests does not calculate the duration of this rest as this duration is calculated only on active notes. Thus it reverts to it's init of 0 ever loop. I've fixed this by adding the duration calculation to the 'rest' section only changing tempo / 1000000.0 to tempo / 1000.0 to account for ms instead of seconds.

Thanks again for this program, I'm having a lot of fun with it. I will upload a fork with python 3 support, coreXY support and mido when it's done.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/michthom/MIDI-to-CNC/issues/9#issuecomment-617376643, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEFLA7C5NTTV7WYXDJNG5TRNXZ5ZANCNFSM4GPHVODQ.

Spheroman commented 3 years ago

Hey, I found this code and I have the same issue with the 0 second long pauses. @Moederbeer have you uploaded your updated code yet? I looked at your profile and I can see that it was updated in 2019, before you finished it. I'm new to github so correct me if its posted somewhere else.

Thanks! Jack

edit: Never mind, I got it to work. Thanks for the program!

michthom commented 1 year ago

Long overdue housekeeping - closing as it's been unloved too long.

michthom / MIDI-to-CNC

Pauses are always 0 seconds #9

python3 -d ./mid2cnc.py --verbose > ~/Desktop/midi2cnc.txt