-
Notifications
You must be signed in to change notification settings - Fork 270
Parse function reworked to better deal with SysEx #368
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
base: master
Are you sure you want to change the base?
Conversation
A reworking of parse() to properly deal with SysEx start & stop bytes as per Midi guidelines.
Thanks, and congrats for your first PR (welcome to open source)! I see the build has failed in CI, I'll have to take a closer look. One little nitpick is the formatting, the project usually uses 4 spaces per indentation, and switching to two causes the diff to be overly large, would you mind fixing that please? (Spending 2 minutes outside of the web ecosystem makes me miss its tooling, we need to setup an automatic code formatter like clang-format to let machines do this job). |
Fixed formatting
Thanks @franky47 "2 workflows awaiting approval Do you know what this means? Is it because its my first PR? Thanks again! |
Thanks, yes it's a security feature, there have been a few attacks on the web ecosystem this way in the past weeks. I wish GitHub had a way to "bless" a PR to auto-approve workflow runs.. |
It looks like the tests fail due to some internals of Google Test, I'll have to take a look at that when I have some time. |
No worries buddy... Yeah I did see something about 'actions/cache: v2' being out of date.... I did have a quick scan through the unit tests though, and spotted at least two tests that will fail due to the different behaviour of my reworked parse. Anyway take your time, I'm in no rush. |
Tidied up Parse a little to reduce duplicate code. Also now deals with Undefined Midi Status bytes correctly. Undefined System Common F4 & F5 received will now reset Parse, and Undefined Realtime FD & F9 will be ignored (although F9 is currently used for Tick which is not really Midi spec). TuneRequest is also now treated as System Common (not Realtime), and will also reset Parse.
Removed the need the RunningStatus_RX variable, and also resetInput() is not really needed anymore
I've tidied up the parse function a little more. I've decided to remove the RunningStatus_RX varaible from the parser as it's not really needed (mPendingMessageIndex can handle the running status side of things), also resetInput() is no longer really needed anymore, but I've left it in for now. I may close this PR and start a new one at some point. I really shouldn't of made this PR from my master branch.... n00b error |
A reworking of the parse function to deal with SysEx start & stop bytes as per the Midi guidelines:
http://midi.teragonaudio.com/tech/midispec/sysex.htm#
In summary....
Non realtime Status bytes received will now abort any current SysEx message..... and SysEx stop bytes will now be ignored unless there has previously been a SysEx start byte received for the current pending message.... Also reworked how SysEx bytes are dealt with in general (Start and Stop bytes are no longer treated as the same).
This was the root cause of the Midi input becoming locked in a SysEx loop, with no route to recovery when an erroneous SysEx start or stop byte was received potentially caused by plugging in a TRS Midi connector (which can cause random garbage data), or the unlikely event of connecting Midi cables in-between a SysEx transmission. #367
Also SysEx messages which are larger than the SysEx buffer (which will result in them being split into chunks) will no longer be considered valid Midi messages, and will have to be dealt with via callbacks only. This is to avoid malformed and incomplete SysEx messages (at the end of a split), which would be automatically transmitted if Thru mode was On. Also reintroduced the WarningSplitSysEx error which will notify when a SysEx message has overrun its buffer, and will be cleared when a fresh SysEx message has begun.
I have tested this as much as I can with my Midi setup (Teensy 4) and all looks good, but I have not run the unit tests because I (embarrassingly) am unsure how to set them up. I've been a bit generous with the comments so hopefully the code is easy to follow.
Thanks to everyone who has previously contributed to this. For the most part this library has been pretty solid for me, and I'm happy to potentially contribute..... I'm not a professional when it comes to coding, and this is my first ever PR, (I'll be quietly chuffed if it gets taken in), but it's a fairly big change, so it's probably a good idea for others to give this one a proper check over first.