Implement SysEx messages and SysEx communication

[mat1jaczyyy]

Because this functionality is needed for my own project which uses this library, I've decided to contribute and implement SysEx communication. Luckily, the library is extremely well written, so this wasn't too hard of a task to achieve on its own. Hopefully lots of users can benefit from this functionality.

Works as expected from manual testing. I am not sure as to how I should implement actual Tests though, as I see for other messages all possible values are tested but this is not so feasible for a SysEx message which can contain an arbitrary amount of data. I'm looking for feedback on how to proceed with those. This is not ready to merge until proper Tests are written.

Oh, and I also encountered a small typo ("TryDecoce" instead of "TryDecode") while observing how NoteOnMessages worked, which I corrected as well.

[micdah]

Hi @mat1jaczyyy - great to see that you wanted to take your time and contribute your changes, that is awesome!

I will have a look at the changes asap and get it merged.

Regarding testing, I know it is quite overkill to test all possible combinations - but for most MIDI messages the value ranges are so low it was easy to just do an exhaustive test suite. But indeed that seems ridiculous.

But might still be nice with a few tests, at least just a positive test to show it can decode a regular sysex message with some arbitrary data?

[mat1jaczyyy]

Alright! SysEx messages are usually manufacturer/device specific, but there are some common ones such as the Device Inquiry message and similar. I'll look up some of the generic ones and write a few tests for those as soon as I catch some free time.

[micdah]

Excellent, would be nice with a test to test it works, at least a smoke test. 😄

A thought on the new SysExMessage, specifically regarding the byte[] Data, as far as I can see the SysExStart and SysExEnd bytes are included - wouldn't it be better to strip these from the data array? I would guess application code is not interested in those parts anyways, as they just constrain the sysex payload?

[mat1jaczyyy]

I had considered this, but ultimately decided against it because that requires recreating the array and copying it every time I want to add the identifier bytes (encoding) and also every time I want to remove them (decoding). Ultimately the way I chose ends up with cleaner code, and removes unnecessary conversions.

It can certainly be done though if you feel it would be better to do so.

[micdah]

Not at all, makes sense - it's pretty easy to iterate over the array skipping the first and last element anyways. 😄 Altough I'm pretty sure the new Span<T> allows one to create a sub-array without any memory copying due to the new low-level memory features added in C# 7.2.

I think it's a good implementation, no need to change anything.

Maybe we should just document it on the SysExMessage so it's clear the status bytes are present if the consumer wants to get rid of them?

[mat1jaczyyy]

Since other kinds of Messages do not keep their status byte (for example, a NoteOnMessage does not keep 0x90 saved anywhere within itself), I will do the same with the SysEx message's data array in order to keep it consistent.

[micdah]

Looks excellent, I'll merge it in and prepare to make a new deploy to NuGet

[mat1jaczyyy]

It wasn't finished! Tests hadn't been written yet and I was still working on a couple extra commits to ensure it all works properly

[mat1jaczyyy]

@micdah should I now open a new pull request to add the extra commits?

[micdah]

@mat1jaczyyy My bad for jumping the gun, yeah I'm unsure if the PR will re-open when you continue to commit to your forked branch, otherwise we might need to create a new PR

I have made a few small changes, so maybe sync with master first?

[mat1jaczyyy]

I've already commited, the PR hasn't reopened. I'm making a new request.