Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add dedicated message type for MIDI Show Control messages #67

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

spark404
Copy link

Heya,

I've been using show control for some shows recently and found that MidiMonitor doesn't have a dedicated parser for it. As i'm using it fairly frequently to test and diagnose it proved handy for me to add it. If you are interested in having this in the main distribution i'll happily work with you to get this code into shape for inclusion.

Based on MIDI Show Control 1.1 specification
image

Tests included in a new scheme SnoizeMIDITests

@krevis
Copy link
Owner

krevis commented Dec 23, 2018 via email

@krevis
Copy link
Owner

krevis commented Jan 3, 2019

Sorry for the delay... holidays, and then I got the usual post-holiday cold.

This generally looks really good. I'd like to tweak a few things:

  • Adding a new message class means that newly created files won't be compatible with older versions of the app, and older files won't show show control messages. I'll see what we can do about that. Perhaps we can just change SMSystemExclusiveMessage to detect when it's a show-control message, and change how it formats itself. May need to cache some things internally.

  • Shouldn't assume anything about the length of the sysex data.There are a few places where you do things like (((Byte *)newData.bytes)[4]), which will crash if newData is too small.

But otherwise this fits in nicely. Thanks for doing all the hard work, especially in such an antique codebase.

I'll take a closer pass over this when I can, and tweak the PR.

@spark404
Copy link
Author

spark404 commented Jan 7, 2019

Hey, happy new year :) Thanks for the feedback. I'll see if i can fix the issue you mentioned on the assumption of data length. I can make some more asserts to check packet length before and while parsing.

@spark404
Copy link
Author

spark404 commented Jan 7, 2019

Good point about reading old files. I started off doing what you mentioned but later made it into a separate message. If they can be stored as SysEx messages but parsed on the fly as show control it would be compatible right?

@krevis
Copy link
Owner

krevis commented Jan 8, 2019

If they can be stored as SysEx messages but parsed on the fly as show control it would be compatible right?

Yes, that should work.

@krevis
Copy link
Owner

krevis commented Jan 21, 2019

Finally got a little time to work on this. Pushed a change that does the following:

  • Perhaps we can just change SMSystemExclusiveMessage to detect when it's a show-control message, and change how it formats itself. May need to cache some things internally.

Done.

  • Shouldn't assume anything about the length of the sysex data.There are a few places where you do things like (((Byte *)newData.bytes)[4]), which will crash if newData is too small.

I fixed this in a couple of places, but need to do more.

I haven't really tested it, though -- I don't typically use show control and I don't even know where to start. If you have time, could you send me a MIDI Monitor document with some show control messages in it? What software do you use to generate show control messages?

@sammysmallman
Copy link

sammysmallman commented Jan 21, 2019 via email

@spark404
Copy link
Author

spark404 commented May 5, 2020

Works like a charm :-)

I've attached a mmon file with a few show control commands in it. And a demo movie of how i generated the commands from QLab.

MSC_examples.zip

spark404 and others added 5 commits May 5, 2020 15:17
- Don't use a separate show control message type, just put it all in sysex message
- Cache dataForDisplay in sysex messages
- rename some stuff
- fix leak in parseCueItemsData

TODO:
- In tests target, use config files instead of ad-hoc settings
- test failing in parseCueItemsData, clean that up probably
- check this with MIDI Show Control spec, I suspect there is a lot more to show
- new TODOs in sysex message, e.g. checking for message lengths
@spark404
Copy link
Author

spark404 commented May 5, 2020

Rebased on latest master / no rebase issues

Base automatically changed from master to main February 24, 2021 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants