use shared midi-types and midi-convert

[x37v]

It took me a bit to get back to it but here is a PR that simply replaces the usbd-midi bespoke midi types with the types and conversion functions from the shared midi-types and midi-convert crates. This allows users to share data directly between usbd-midi and embedded_midi or any other crate that might use those same shared types.

The only minor cleanup I did was to remove the unused types and expand CodeIndexNumber

The test data changed a little bit because midi-types uses the more common -2..8 octave range for 12-tone english note names where usbd-midi was using the yamaha convention.

[132ikl]

I'm a bit confused, in the PR description you say:

The test data changed a little bit because midi-types uses the more common -2..8 octave range for 12-tone english note names where usbd-midi was using the yamaha convention.

But as far as I can tell, the Yamaha convention is Middle C = C3 (ie. C3 is note 60), which is what midi-types does, not usbd-midi. Not sure if this is just a small oversight, but this PR just sent me down the MIDI Middle C rabbithole and I don't know what to believe anymore.

This PR does work on my setup however :smile: (unlike #10)

I think it might be worth highlighting the octave change though, since this is arguably a breaking change for applications which already use usbd-midi.

[x37v]

But as far as I can tell, the Yamaha convention is Middle C = C3 (ie. C3 is note 60), which is what midi-types does, not usbd-midi. Not sure if this is just a small oversight, but this PR just sent me down the MIDI Middle C rabbithole and I don't know what to believe anymore.

My understanding is that the Yamaha convention is Middle C (note 60) = C4. This is by no means an official document but reflects that: https://computermusicresource.com/midikeys.html BTW, their chart misidentifies the first octave as -1 along with the 2nd as -1

I haven't found any authoritative source for this info though

[x37v]

I think it might be worth highlighting the octave change though, since this is arguably a breaking change for applications which already use usbd-midi.

yeah.. the whole PR would be a breaking change but I think you're right that it should get highlighted somehow, any ideas how to do that?

[132ikl]

yeah.. the whole PR would be a breaking change

fair enough lol

any ideas how to do that?

I'm not entirely sure since there's no GitHub releases or any other real place for a changelog in this repo. I think I would be pretty confused if I upgraded this crate and every note was an octave higher, even if it's a major version increment. maybe a note in the README?

[btrepp]

I'm happy with this. Though the breaking change is just the bit to communicate.

Could you had a https://keepachangelog.com/en/1.0.0/ with the 'unreleased bit' filled out. That should be good enough to communicate the breaking change part.

For the original types, I just ended up picking one of two. In hindsight, it probably makes sense to have two types on the convention, with a trait of 'into u8' so that users can pick which makes sense, but I'd consider that an improvement in the upstream crate (to support both) so that won't stop this one.

[x37v]

@btrepp good idea on the changelog, I just pushed something, how does that look?

[x37v]

Mostly looks good, only other general thought is that the crate could use a cargo fmt, there's some inconsistency between the code style of this crate and the PR

I've intentionally disabled cargo fmt for now because running it on the code base before this PR created a ton of changes so I thought I'd follow up with that.

[m5p3nc3r]

Hi Guys,

I am interested in using this library in my next project, but wanted to check the status of merging this PR. I am exploring creating libraries that make use common representation of notes, so having a standard language for usbd-midi would be very useful!

[x37v]

Hi Guys,

I am interested in using this library in my next project, but wanted to check the status of merging this PR. I am exploring creating libraries that make use common representation of notes, so having a standard language for usbd-midi would be very useful!

I forked over to https://github.com/rust-midi/usbd-midi but I need to do a little bit of debugging

[laenzlinger]

Hi

I am interested in this PR. Is there anything that needs to be done or I could help? For example do some testing?

[sourcebox]

I would like to close this PR without merging in favour of not having an additional dependency that has to be maintained. As I explained in #5 my preferred solution would be just to do the transport-related tasks in this crate and leave it up to the user which crate he wants to use for converting the raw data to specific types and do fancy calculations on them. Otherwise, this crate would have to be updated everytime when a new version of midi-types' is released to be in sync. If then another transport method like network or Bluetooth MIDI is involved within the same firmware and that also usesmidi-types` at whatever version, it ends up in possible non-resolvable version hell for the end user.

I'll leave this PR open for some comments for a few days. I'm aware that a good amount of work went into it and it's disappointing to have it discarded.

[x37v]

I see a lot of value in having shared types that we can use within an embedded rust ecosystem. It feels to me like that value outweighs some speculative idea of the midi_types authors (I am included in there) not doing a good job with versioning but maybe I'm confusing what you're saying. If you are talking about removing even more of the bespoke types, simply providing bytes in and out, maybe that is fine but IIRC a USB midi packet does need to do some parsing of the data.

[sourcebox]

I think we need to find a nice way to connect this crate with midi-types without having it included as dependency. Disregarding SysEx, the last 3 bytes of a packet always contain the MIDI message itself so that interchanging it with another crate is easy. So the additional parsing required is mainly for SysEx. And there's also the cable number, which (if used at all) is something that makes most sense for routing messages inside the application.

So the main challenge is probably to find a good solution for dealing with SysEx data, or do you see some other aspects that require internal parsing?

[x37v]

I think we need to find a nice way to connect this crate with midi-types without having it included as dependency. Disregarding SysEx, the last 3 bytes of a packet always contain the MIDI message itself so that interchanging it with another crate is easy. So the additional parsing required is mainly for SysEx. And there's also the cable number, which (if used at all) is something that makes most sense for routing messages inside the application.

So the main challenge is probably to find a good solution for dealing with SysEx data, or do you see some other aspects that require internal parsing?

You do have to generate what this library calls the CodeIndexNumber if you're generating MIDI output but, that is pretty simple and maybe not important enough to require an additional dependency.

I can totally see ripping out all of the MIDI specific stuff from this library and simply provide a raw u8 slice/array interface that lets users manage the encode/decoding to/from some other representation (like midi-types) themselves and then we could simply provide an example of how to use that with midi-types, I'm happy to help with that.

with that and Sysex in mind, on the input, I could see an iterator that provides something like:

pub enum Packet<'a> {
  Midi(&'a [u8]),
  SysexStart(&'a [u8]),
  SysexContinue(&'a [u8]),
  SysexEnd(&'a [u8])
}

on the output, maybe just a method that takes a slice and generates the needed packets.. like write_sysex(&mut self, data: &[u8]) I suspect this would need to return something about where in the slice it had to give up so that you can call a write_sysex_continue(&mut self, data &[u8]) where your data is offset by whatever wouldn't fit in the previous USB packet?

[btrepp]

So for my 2c. I did spent a bit of effort in the design of types here to follow Domain Driven Design, to make it so the functions are expecting valid data. A MidiNote doesn't have the same overlapping domain as a u8, so if the user sends you invalid data, how do we then handle it in the crate?. Clamp it?, Mask it?, either way is unexpected, so the idea is if you are parsing from a u8, you need to define what happens in your library if it's not a valid midi note. I personally try to use domain bounded types, especially in embedded contexts, as invalid data = does not compile, and tracing type overflow bugs on a embedded device is nightmarish :).

You can even see MidiNote is fairly easy to go into u8s and u7s, I think the time the TryFrom trait wasn't stable, I believe a TryFrom trait and impl could help without changing the types too much. I suspect you wouldn't even need TryFrom from u7 just From (it probably just wasn't implemented yet), as iirc a midi notes space is similar to a u7

That being said, a core, complete definition of a midi note in a nostd crate is probably better to use. It didn't exist at the time so I created a minimal one, however if the definition in another crate is just the data types, and doesn't bring in std-reps which would make the crate less portable, I see no reason to not share + use other dependencies.

On the Yamaha vs Common one. It probably makes sense for midi-types to have both definitions, with the From conversions to change between them, and maybe some Into's that.

Oh and one whether bits are used or not in practice, the aim of this implementation was to be truthful to the usb-midi specification. lots of annoying time was spent to make sure the domains were covered, to make it useful in contexts I didn't predict, so at the moment we are decently true to the spec (I believe) even with its annoying inconsistencies!. Granted there might be gaps, but the library tries to be truthful to the spec, so that if a consumer needs the cable number it is faithfully there. even if a heap of other software ignores it

[sourcebox]

with that and Sysex in mind, on the input, I could see an iterator that provides something like:

pub enum Packet<'a> {
  Midi(&'a [u8]),
  SysexStart(&'a [u8]),
  SysexContinue(&'a [u8]),
  SysexEnd(&'a [u8])
}

That's quite similar to what I had in mind,maybe even better. I will open a new branch for this to be implemented and tested.

[sourcebox]

@btrepp I can see your points and they are all valid. The dependency issue is a real problem though, it doesn't strike the crate maintainer but the end user when there are conflicting versions in the whole dependency tree.

We can, of course, leave all the checks in that you've implemented, so the user can't just send garbage data. But ideally, when using midi-types on the application level, errors should already be thrown there when constructing the data. It would just lead to double-check everything, some people may claim some performance impact, but that should not really be an issue.

[sourcebox]

Btw. if you really want to have midi-types in here, then my suggestion is: rebase this PR so it merges. But in that case I would really like having @x37v actively here as maintainer to react to any new release of midi-types rapidly, doing fixes and publishing new releases.

[x37v]

Btw. if you really want to have midi-types in here, then my suggestion is: rebase this PR so it merges. But in that case I would really like having @x37v actively here as maintainer to react to any new release of midi-types rapidly, doing fixes and publishing new releases.

I think the idea of removing all the types and focusing on the transport, providing a simple way of getting data in and out and letting the user decide what they use to encode/decode (if they want that at all), makes a lot of sense. I'll close this issue. @btrepp's notes make sense but that could really apply to midi-types or some other encoding/decoding/representation library.