use shared midi-types and midi-convert #11

x37v · 2022-11-22T19:18:28Z

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 · 2022-11-22T22:21:43Z

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 😄 (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 · 2022-11-22T23:22:55Z

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 · 2022-11-22T23:24:11Z

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 · 2022-11-25T19:10:18Z

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 · 2022-12-01T22:53:05Z

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 · 2022-12-02T17:43:48Z

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

CHANGELOG.md

@132ikl

text from @132ikl

132ikl

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

132ikl · 2022-12-10T19:09:55Z

src/data/byte/mod.rs

@@ -1,3 +1 @@
-pub mod u7;
 pub mod u4;


Maybe u4 should be moved into midi-types as well?

midi-types has Channel so doesn't need u4..

Ah, I think I was mixing up CableNumber and Channel

src/data/usb_midi/usb_midi_event_packet.rs

132ikl · 2022-12-10T19:36:10Z

src/data/usb_midi/usb_midi_event_packet.rs

@@ -66,7 +62,7 @@ impl TryFrom<&[u8]> for UsbMidiEventPacket {
            None => return Err(MidiPacketParsingError::MissingDataPacket)
        };

-        let message = Message::try_from(message_body)?;
+        let message = MidiMessage::try_parse_slice(message_body).map_err(|_| MidiPacketParsingError::MissingDataPacket)?;


This shouldn't always be mapped to MissingDataPacket; the error doesn't make sense if try_parse_slice is given a valid packet with with invalid event. Maybe this should be something like:

let message = match MidiMessage::try_parse_slice(message_body) { Ok(message) => message, Err(ParseError::MessageNotFound) => return Err(MidiPacketParsingError::InvalidEventType), Err(_) => return Err(MidiPacketParsingError::MissingDataPacket), };

Right now InvalidEventType has a field for the invalid event value since Message::try_from included that with the error, but MidiMessage::try_parse_slice doesn't. I see a couple options:

Remove the u8 from InvalidEventType, lose out on potentially useful debugging info

Keep it, and add the value to ParseError::MessageNotFound as well

Re-parse the message here in order to figure out the event type (this one seems a bit silly)

I realized that the cable number parse can never fail and so I added a MissingCableNumber and also simply just forward the parse error... I cleaned up the errors a bit with that.

132ikl · 2022-12-10T19:41:10Z

src/data/usb_midi/usb_midi_event_packet.rs

-    use crate::data::midi::notes::Note;
-    use crate::data::byte::u7::U7;
-    use crate::data::midi::message::Message;
+    use crate::midi_types::{Channel, MidiMessage, Value7, Value14, Note, Program, Control};
    use crate::data::usb_midi::cable_number::CableNumber::{Cable0,Cable1};


Maybe CableNumber could be moved to midi-types? I'm not sure how useful they would be outside of the context of usbd-midi, but it could avoid duplication if a different project implements USB MIDI (maybe a usb feature could be added to midi-types?)

I could see that being a TODO in case anything ever wanted it but if nothing ever does, it doesn't have any value for other non usb midi work so, I figure, no rush?

x37v · 2022-12-10T20:04:51Z

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.

cable_number can never fail, 0xFF >> 4 is always less than 16 propagate parse error

m5p3nc3r · 2023-05-18T22:53:08Z

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 · 2023-05-18T23:05:00Z

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 · 2023-07-17T22:41:30Z

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 · 2024-05-28T12:46:07Z

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 uses midi-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 · 2024-05-28T16:02:01Z

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 · 2024-05-28T16:18:05Z

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 · 2024-05-28T23:41:52Z

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 · 2024-05-29T00:09:37Z

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 · 2024-05-29T07:56:52Z

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 · 2024-05-29T08:18:12Z

@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 · 2024-05-29T08:29:45Z

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 · 2024-05-30T13:36:54Z

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.

x37v added 6 commits November 22, 2022 10:27

add midi-convert

5090ac8

upgrade midi-types

97a6a66

use MidiMessage

27f2ed5

fixup tests with midi_types

b85cebe

remove redundant types/code

cfae75f

add realtime and system common to CodeIndexNumber::find_from_message

89c63c9

x37v mentioned this pull request Nov 22, 2022

use midi-types and reorg #10

Closed

update README with MidiMessage

17f1caf

x37v mentioned this pull request Nov 22, 2022

How does this crate fit into the usb-audio rust ecosystem #5

Open

x37v mentioned this pull request Dec 1, 2022

version bump and release? #9

Closed

add changelog with unreleased details

80f0754

132ikl reviewed Dec 2, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

updated octave change NOTE

1ae87bb

text from @132ikl

x37v requested review from btrepp and 132ikl December 8, 2022 15:18

132ikl reviewed Dec 10, 2022

View reviewed changes

x37v added 3 commits December 10, 2022 12:11

use midi_types types;

0231e4f

derive for InvalidCableNumber

c6c5ce1

cleanup UsbMidiEventPacket try_from

712804f

cable_number can never fail, 0xFF >> 4 is always less than 16 propagate parse error

x37v requested a review from 132ikl December 10, 2022 20:29

132ikl approved these changes Dec 10, 2022

View reviewed changes

laenzlinger mentioned this pull request Jul 17, 2023

Switch over to rust-midi crate pedalboard/pedalboard-midi#5

Merged

x37v closed this May 30, 2024

sourcebox mentioned this pull request May 30, 2024

Roadmap for further development #18

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use shared midi-types and midi-convert #11

use shared midi-types and midi-convert #11

x37v commented Nov 22, 2022

132ikl commented Nov 22, 2022

x37v commented Nov 22, 2022

x37v commented Nov 22, 2022

132ikl commented Nov 25, 2022

btrepp commented Dec 1, 2022

x37v commented Dec 2, 2022

132ikl left a comment

132ikl Dec 10, 2022

x37v Dec 10, 2022

132ikl Dec 10, 2022

132ikl Dec 10, 2022

x37v Dec 10, 2022

132ikl Dec 10, 2022

x37v Dec 10, 2022

x37v commented Dec 10, 2022

m5p3nc3r commented May 18, 2023

x37v commented May 18, 2023

laenzlinger commented Jul 17, 2023

sourcebox commented May 28, 2024

x37v commented May 28, 2024

sourcebox commented May 28, 2024

x37v commented May 28, 2024

btrepp commented May 29, 2024

sourcebox commented May 29, 2024

sourcebox commented May 29, 2024

sourcebox commented May 29, 2024

x37v commented May 30, 2024

use shared midi-types and midi-convert #11

use shared midi-types and midi-convert #11

Conversation

x37v commented Nov 22, 2022

132ikl commented Nov 22, 2022

x37v commented Nov 22, 2022

x37v commented Nov 22, 2022

132ikl commented Nov 25, 2022

btrepp commented Dec 1, 2022

x37v commented Dec 2, 2022

132ikl left a comment

Choose a reason for hiding this comment

132ikl Dec 10, 2022

Choose a reason for hiding this comment

x37v Dec 10, 2022

Choose a reason for hiding this comment

132ikl Dec 10, 2022

Choose a reason for hiding this comment

132ikl Dec 10, 2022

Choose a reason for hiding this comment

x37v Dec 10, 2022

Choose a reason for hiding this comment

132ikl Dec 10, 2022

Choose a reason for hiding this comment

x37v Dec 10, 2022

Choose a reason for hiding this comment

x37v commented Dec 10, 2022

m5p3nc3r commented May 18, 2023

x37v commented May 18, 2023

laenzlinger commented Jul 17, 2023

sourcebox commented May 28, 2024

x37v commented May 28, 2024

sourcebox commented May 28, 2024

x37v commented May 28, 2024

btrepp commented May 29, 2024

sourcebox commented May 29, 2024

sourcebox commented May 29, 2024

sourcebox commented May 29, 2024

x37v commented May 30, 2024