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

Implemented Sysex Coalescing #200

Merged
merged 17 commits into from
Aug 22, 2017
Merged

Implemented Sysex Coalescing #200

merged 17 commits into from
Aug 22, 2017

Conversation

nightbirdsevolve
Copy link
Contributor

@nightbirdsevolve nightbirdsevolve commented Jun 12, 2017

References Issues #198, #122
Alternative Implementation to PR #130

Safeguards:

  • Handle invalid start byte in each packet's data (NOT IN [0 - 0x7F])
  • Handle EOT regardless of byte position in stream. Ignore remaining data in packet.
  • Handle absence of EOT, or timed-out transmissions: End Sysex when a timer expires.

EOT = End of transmission marker 0xF7

Took some hints from SnoizeMIDI Source

@armadsen
Copy link
Member

This looks like it's shaping up to be a great PR. I've noticed you continuing to work on it (which is great). Please ping me when you think it's ready to merge.

Also, if at all possible, some kind of test harness for it would be great. If you can write a program that exercises it without any extra hardware, that would be ideal. Even if you can just describe how you're testing it and which hardware you're using, that would be great too.

@nightbirdsevolve
Copy link
Contributor Author

nightbirdsevolve commented Jun 15, 2017

@armadsen Thanks! Yeah it's finished.
I'll write some tests, that's definitely needed.

I'm already using 0af5172 in production for Juno Editor v2.2 because one client had chunking issues on his “usb to midi cables” interface.
I have never had the issue on my MOTU 828, but those cheap interfaces seem to always chunk data, so I think that feature is much needed!

@nightbirdsevolve nightbirdsevolve changed the title Implemented basic Sysex Coalescing Implemented Sysex Coalescing Jun 15, 2017
@nightbirdsevolve
Copy link
Contributor Author

nightbirdsevolve commented Jun 16, 2017

Simplified my approach to checking for invalid data, as I was being overly cautious about it.

Also, when EOT is found before the end of packet data, I am not parsing remaining data of packet anymore: One packet = one logical unit of data. Assuming we will find sysex + other commands inside a single packet is kinda going overboard.

If a packet contains invalid sysex data (after its start byte), i believe that's not the framework's job to do extensive sanitizing.

The current code is built around the following assumptions:

  • Sysex data length is undefined, and may span across multiple packets (chunking) and, on some devices, multiple packet Lists (multiple callbacks).
  • Sysex data might not have and end marker.

hence:

  • The timer handles worst case scenarios (disconnections, non-terminated)
  • While parsing sysex, we check the start byte of each following packet to see if it still is within the bounds of a valid sysex command. If not, purge sysex data gathered until now by sending it (could optionally discard it) and parse the following packets normally.

@nightbirdsevolve
Copy link
Contributor Author

Note: re-wrote packetList handling to be block-based so I could test time-out properly. This accidentally lead to even better code… tests are awesome!

@armadsen Finally ready to review :)

@armadsen armadsen merged commit ea14c1c into mixedinkey-opensource:master Aug 22, 2017
@armadsen
Copy link
Member

Finally got this reviewed and merge. Looks great, thanks so much!

@nightbirdsevolve nightbirdsevolve deleted the Sysex-Coalescing branch September 25, 2017 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants