use midi-types and reorg #10

x37v · 2022-06-09T15:11:39Z

I'm using both usb and serial midi in a project so I wanted to share types between the two, embedded-midi uses a platform agnostic midi-types representation of midi messages and we've extracted parsing/rendering into midi-convert. I have a PR open to bring that into embedded-midi, just waiting on the author to get back from a trip.

Anyways.. here is a Draft Request here as I've done a massive reorg of usbd-midi both to use midi-convert (and in turn, midi-types) but also to simplify using usbd-midi as a library, I took some liberties with some things that I thought were a bit more complicated than needed to be..

There are a few things we could bring back into midi-types that you all had in usbd-midi for instance, constants for Note::C2, Channel1, default Controller number naming etc etc but I figured that is all doable later.

Figured I'd see if you all were interested in this and also if so, maybe you'd want to join the rust-midi org at the same time..

also hide members

should require less bounds checking

btrepp · 2022-06-09T23:24:20Z

Very happy to integrate with a wider midi platform. One of my dreams of rust is that you can share the code everywhere, so happy to facilitate.

Definitely okay with joining the org or handing key parts over to the org to maintain.

Haven't read through the pr yet as it's a decent size, so will need to do this after work.

x37v · 2022-06-22T16:28:50Z

@btrepp wondering if you've had a chance to look this over?

x37v · 2022-10-03T22:59:54Z

Been a while, figured I'd ping here again.. @btrepp any thoughts?

btrepp · 2022-10-04T09:14:38Z

Hi Mate.

I've had a look. Overall it's okay, it's a bit hard to grok with the re-org plus type changes.
I think there is also the switch from some compile time checks into runtime asserts?. My personal style is to usually eliminate esoteric knowledge, eg cable numbers only go up to 16, hence the proper type.

I'm also not sure why notes got deleted. I loved the ability to note have to think which number is what note. I think if you prefer to use u8's over the note enum, we should make the API be generic over kinds that have N: Into<Note> and implement this for u8 if the space maps correctly.

I get the intent for the simplification, but I think it's gone to far and remove some of the safety nets I had put in.

x37v · 2022-10-05T16:31:41Z

Hi Mate.

Hey, thanks for taking a look!

I've had a look. Overall it's okay, it's a bit hard to grok with the re-org plus type changes. I think there is also the switch from some compile time checks into runtime asserts?. My personal style is to usually eliminate esoteric knowledge, eg cable numbers only go up to 16, hence the proper type.

I hear that RE CableNumber, I'll bring that back in. I find it a bit awkward to use an enum for a numeric representation especially when it is nearly immediately needed to be converted back into a number.. so i tend to prefer a wrapper type with private internal or an assertion and documentation, but that is just a style thing so I'm fine to bring it back in.

I'm also not sure why notes got deleted. I loved the ability to note have to think which number is what note. I think if you prefer to use u8's over the note enum, we should make the API be generic over kinds that have N: Into<Note> and implement this for u8 if the space maps correctly.

The notes got removed because the types that they represent now exist in the shared type crate.. as I suggested in the PR description, we could add those into the midi-types crate and they could be shared among all the crates that use those shared types, that part just hasn't happened yet but its quick/easy so I can do it. I've kind of resisted baking in 12-tet into things because I'm trying to resist the 12-tet "westernization" tendencies, but I do understand that its useful for a lot of people.

I get the intent for the simplification, but I think it's gone to far and remove some of the safety nets I had put in.

I get that, I find that doing a lot of try_from and try_into in an embedded context to be a bit verbose as I almost always do .unwrap() with them, but I'm fine to bring that back in, was there anything other than CableNumber that needs that?

btrepp · 2022-10-05T23:20:50Z

Thanks for being receptive. In an embedded context I actually move much more heavily to tryInto and types that are compiler verified. Its a pain to figure out via a remote debugger that you put cable number 20 and that's why everything falls over :). Happy to have some 'unsafe' variants that auto unwrap though. Which gives enough here be dragons.

There could also be something with Trait bounds and const generics, e.g being able to use a const generic on the first 16 in cable number. I think we should explore use-ability as a seperate PR though.

No worries on the shared types, I think that's why having a re-org and a change in lib made it a bit hard to read. Using types in a much more common crate I am infinitely supportive of, I only built my own due to there being none at the time.

With the notes. The advantage was to make it a bit easier to transpose simple songs from sheet music. I do agree that it's a bit westernized, though I am personally not to familiar with other music systems running over midi. I think this could be another one for traits bounds on the functions. E.g changing to Into, which means use whatever system you want, it just needs to be within the bounds of midi.

So to simplify,

Happy to take any types from the shared crate
Lets take simplifications in their own PRs, so we can have the safety for those who want it, but provide faster paths for this who need it, but seperate PRs topics means we can go on the merits of each one.

That way it's also reasonably backwards compatible for anyone who is using this crate.

mendelt · 2022-10-16T18:54:29Z

Hi, I'm the other maintainer of the rust-midi crates. Hope its ok to chime in here.

In an embedded context I actually move much more heavily to tryInto and types that are compiler verified.

I normally wholeheartedly agree with this. Especially in embedded contexts you dont want your code to panic because some value is out of bounds. I think the decision to be a bit looser with this here was originally mine. The conversions from and into u8 for channels etc. are primarily used parsing and serializing messages. I was repeatedly running into situations where the surrounding already provided all the guarantees needed that the values were inside the bounds that we expected for example channel numbers were parsed from a u8 by masking out the other 4 bits.
So I decided I didn't want all the duplicate checks.
I do agree that the runtime checks should maybe be debug_assert!s instead of asserts. These nicely support any unit-tests by adding the bounds checks. But will not cause extra runtime checks for production code.

With the notes. The advantage was to make it a bit easier to transpose simple songs from sheet music. I do agree that it's a bit westernized, though I am personally not to familiar with other music systems running over midi. I think this could be another one for traits bounds on the functions. E.g changing to Into, which means use whatever system you want, it just needs to be within the bounds of midi.

Maybe a good alternative would be to keep the note numbers as a u8 newtype but implement conversions into other representations. That would leave some more room for flexibility. If you want to convert to normal western notes we could convert to/from a 12 note enum type + an octave number.

x37v · 2022-10-17T14:02:20Z

So to simplify,

1. Happy to take any types from the shared crate

2. Lets take simplifications in their own PRs, so we can have the safety for those who want it, but provide faster paths for this who need it, but seperate PRs topics means we can go on the merits of each one.

That way it's also reasonably backwards compatible for anyone who is using this crate.

I'm in kind of a crunch with my day job but I hope to get to this soon.

132ikl · 2022-10-30T06:35:09Z

Hi, I can't seem to get message sending to work on this PR. I took the receiving messages example and modified it to echo the note messages back. When trying to play a note with the PR, the LED lights up fine but no MIDI message is sent (at least according to my DAW). I'm using the RP2040 Feather BSP from rp-hal. Here's the code I tested for both this repo and the PR (I can send full files if needed):

Original working code

fn main() -> ! {
    let mut pac = pac::Peripherals::take().unwrap();
    let mut watchdog = Watchdog::new(pac.WATCHDOG);

    let clocks = init_clocks_and_plls(
        XOSC_CRYSTAL_FREQ,
        pac.XOSC,
        pac.CLOCKS,
        pac.PLL_SYS,
        pac.PLL_USB,
        &mut pac.RESETS,
        &mut watchdog,
    )
    .ok()
    .unwrap();

    let sio = Sio::new(pac.SIO);
    let pins = Pins::new(
        pac.IO_BANK0,
        pac.PADS_BANK0,
        sio.gpio_bank0,
        &mut pac.RESETS,
    );

    let usb_bus = UsbBusAllocator::new(UsbBus::new(
        pac.USBCTRL_REGS,
        pac.USBCTRL_DPRAM,
        clocks.usb_clock,
        true,
        &mut pac.RESETS,
    ));

    let mut midi = MidiClass::new(&usb_bus, 1, 1).unwrap();

    let mut usb_dev = UsbDeviceBuilder::new(&usb_bus, UsbVidPid(0x16c0, 0x5e4))
        .manufacturer("Rose")
        .product("Microphone Buttons")
        .serial_number("MBTN")
        .device_class(1) // from: https://www.usb.org/defined-class-codes
        .device_sub_class(3)
        .build();

    let button_1 = pins.d12.into_pull_down_input();
    let led_1 = pins.d13.into_readable_output();
    let mut pins = (button_1, led_1);

    loop {
        if !usb_dev.poll(&mut [&mut midi]) {
            continue;
        }

        let mut buffer = [0; 64];
        if let Ok(size) = midi.read(&mut buffer) {
            let buffer_reader = MidiPacketBufferReader::new(&buffer, size);
            for packet in buffer_reader.into_iter() {
                if let Ok(packet) = packet {
                    match packet.message {
                        Message::NoteOn(channel, note, vel) => {
                            midi.send_message(UsbMidiEventPacket::from_midi(
                                CableNumber::Cable0,
                                Message::NoteOn(channel, note, vel),
                            ))
                            .unwrap();
                            pins.1.set_high().unwrap();
                        }
                        Message::NoteOff(channel, note, vel) => {
                            midi.send_message(UsbMidiEventPacket::from_midi(
                                CableNumber::Cable0,
                                Message::NoteOff(channel, note, vel),
                            ))
                            .unwrap();
                            pins.1.set_low().unwrap();
                        }
                        _ => {}
                    }
                }
            }
        }
    }
}

Non-working code using PR

fn main() -> ! {
    let mut pac = pac::Peripherals::take().unwrap();
    let mut watchdog = Watchdog::new(pac.WATCHDOG);

    let clocks = init_clocks_and_plls(
        XOSC_CRYSTAL_FREQ,
        pac.XOSC,
        pac.CLOCKS,
        pac.PLL_SYS,
        pac.PLL_USB,
        &mut pac.RESETS,
        &mut watchdog,
    )
    .ok()
    .unwrap();

    let sio = Sio::new(pac.SIO);
    let pins = Pins::new(
        pac.IO_BANK0,
        pac.PADS_BANK0,
        sio.gpio_bank0,
        &mut pac.RESETS,
    );

    let usb_bus = UsbBusAllocator::new(UsbBus::new(
        pac.USBCTRL_REGS,
        pac.USBCTRL_DPRAM,
        clocks.usb_clock,
        true,
        &mut pac.RESETS,
    ));

    let mut midi = MidiClass::new(&usb_bus, 1, 1).unwrap();
    let mut usb_dev = UsbDeviceBuilder::new(&usb_bus, UsbVidPid(0x16c0, 0x5e4))
        .manufacturer("Rose")
        .product("Microphone Buttons")
        .serial_number("MBTN")
        .device_class(1) // from: https://www.usb.org/defined-class-codes
        .device_sub_class(3)
        .build();

    let button_1 = pins.d12.into_pull_down_input();
    let led_1 = pins.d13.into_readable_output();
    let mut pins = (button_1, led_1);

    loop {
        if !usb_dev.poll(&mut [&mut midi]) {
            continue;
        }

        let mut buffer = [0; 64];
        if let Ok(size) = midi.read(&mut buffer) {
            let buffer_reader = MidiPacketBufferReader::new(&buffer, size);
            for packet in buffer_reader.into_iter() {
                if let Ok(packet) = packet {
                    match packet.message() {
                        MidiMessage::NoteOn(channel, note, vel) => {
                            midi.send_message(UsbMidiEventPacket::new(
                                0,
                                MidiMessage::NoteOn(*channel, *note, *vel),
                            ))
                            .unwrap();
                            pins.1.set_high().unwrap();
                        }
                        MidiMessage::NoteOff(channel, note, vel) => {
                            midi.send_message(UsbMidiEventPacket::new(
                                0,
                                MidiMessage::NoteOff(*channel, *note, *vel),
                            ))
                            .unwrap();
                            pins.1.set_low().unwrap();
                        }
                        _ => {}
                    }
                }
            }
        }
    }
}

x37v · 2022-10-31T17:57:32Z

@132ikl

Hi, I can't seem to get message sending to work on this PR. I took the receiving messages example and modified it to echo the note messages back. When trying to play a note with the PR, the LED lights up fine but no MIDI message is sent (at least according to my DAW). I'm using the RP2040 Feather BSP from rp-hal. Here's the code I tested for both this repo and the PR (I can send full files if needed):

...

I'm not 100% why but I had to set device_class to USB_CLASS_NONE and not set device_sub_class at all to have reliable usb-midi pre/post this PR with usbd-midi.. I see that that isn't what you're doing in your non working code example...

btrepp · 2022-10-31T23:04:12Z

For the weird stuff like device classes. Usb as a spec is special. You really have to only say the specific things. I have found different os seem to respond to edge cases differently. I think if list yourself as xyz, you must respond to certain messages, or the Os drops it. So might be why you need class none

…

On Tue, 1 Nov 2022, 01:57 Alex Norman, ***@***.***> wrote: Hi, I can't seem to get message sending to work on this PR. I took the receiving messages example and modified it to echo the note messages back. When trying to play a note with the PR, the LED lights up fine but no MIDI message is sent (at least according to my DAW). I'm using the RP2040 Feather BSP from rp-hal. Here's the code I tested for both this repo and the PR (I can send full files if needed): ... I'm not 100% why but I had to set device_class to USB_CLASS_NONE and not set device_sub_class at all to have reliable usb-midi pre/post this PR with usbd-midi.. I see that that isn't what you're doing in your non working code example... — Reply to this email directly, view it on GitHub <#10 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACAGFOP3QH5BJV4S2TOXADWGACBPANCNFSM5YKODH2A> . You are receiving this because you were mentioned.Message ID: ***@***.***>

132ikl · 2022-11-02T19:47:19Z

I had to set device_class to USB_CLASS_NONE and not set device_sub_class at all

Hi, just tried that and it didn't seem to affect anything. Sorry for taking a couple days, I'll turn on email notifications for this thread so if there's anything else you want me to try out let me know!

x37v · 2022-11-22T19:19:44Z

simplified for #11
will follow that up with some cleanup-reorg if that is accepted

x37v added 16 commits May 25, 2022 18:48

ditch unused

da2a14c

use the idomatic 'new' and provide getters for members

f9bffb6

also hide members

cargo format

1e16171

use slice chuck struct to read packets

f5f4c21

should require less bounds checking

more cleanup/simplification

4ff8d53

reduce file complexity

7b71305

simplify use

3d316d2

ditch unused error

e49fee3

derive for Error

686e75c

impl ExactSizeIterator and add very basic tests

3a87c2b

move constants around, rename error type, flatten library

5a1cb19

use midi-parse-render crate

9d7fe38

use midi-convert

760cba4

Merge branch 'develop' of github.com:x37v/usbd-midi into develop

1fa9537

use released midi-convert

622d1d3

update README example to use USB_CLASS_NONE and MidiMessage

d3fc63f

x37v closed this Nov 22, 2022

132ikl mentioned this pull request Nov 22, 2022

use shared midi-types and midi-convert #11

Closed

x37v mentioned this pull request Dec 1, 2022

version bump and release? #9

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use midi-types and reorg #10

use midi-types and reorg #10

x37v commented Jun 9, 2022

btrepp commented Jun 9, 2022

x37v commented Jun 22, 2022

x37v commented Oct 3, 2022

btrepp commented Oct 4, 2022

x37v commented Oct 5, 2022

btrepp commented Oct 5, 2022

mendelt commented Oct 16, 2022 •

edited

Loading

x37v commented Oct 17, 2022

132ikl commented Oct 30, 2022

x37v commented Oct 31, 2022 •

edited

Loading

btrepp commented Oct 31, 2022 via email

132ikl commented Nov 2, 2022

x37v commented Nov 22, 2022

use midi-types and reorg #10

use midi-types and reorg #10

Conversation

x37v commented Jun 9, 2022

btrepp commented Jun 9, 2022

x37v commented Jun 22, 2022

x37v commented Oct 3, 2022

btrepp commented Oct 4, 2022

x37v commented Oct 5, 2022

btrepp commented Oct 5, 2022

mendelt commented Oct 16, 2022 • edited Loading

x37v commented Oct 17, 2022

132ikl commented Oct 30, 2022

x37v commented Oct 31, 2022 • edited Loading

btrepp commented Oct 31, 2022 via email

132ikl commented Nov 2, 2022

x37v commented Nov 22, 2022

mendelt commented Oct 16, 2022 •

edited

Loading

x37v commented Oct 31, 2022 •

edited

Loading