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

Roadmap for further development #18

Open
4 tasks done
sourcebox opened this issue May 29, 2024 · 33 comments
Open
4 tasks done

Roadmap for further development #18

sourcebox opened this issue May 29, 2024 · 33 comments

Comments

@sourcebox
Copy link
Collaborator

sourcebox commented May 29, 2024

I just want to put some notes here for further discussion, nothing set in stone.

  • - A solution how to deal with SysEx messages has to be evaluated.

  • - An example how to interface with other crates and how to send and receive SysEx will be put up.

  • - We will do a release which is compatible with usb-device 0.3 after the SysEx support is tested.

  • - The internal types need to be completed with the missing variants for System Common and System Realtime messages.

  • There's hopefully a 0.4 release of usb-device at some point that removes the control-buffer-256 feature in favour of passing a dedicated control buffer to UsbDeviceBuilder::new(). That will solve the issues with large config descriptors. We do a new release right after usb-device 0.4 is out.

  • MIDI 2.0 is something that comes up on the horizon. Should we try to integrate it or keep everything at 1.0 and start a new crate for it? If it's decided to be integrated, what about all the types it requires?

  • Since having more than one jack is already supported, it would be nice to have string descriptors to have them labelled on the host.

  • Using a builder pattern could possibly be helpful to manage those things like string descriptors.

@sourcebox
Copy link
Collaborator Author

I updated the list because #11 has been closed.

If we recommend the user to use an external types crate, we maybe should decide what to do with the ones that are now in. Possible options:

  • Keep all of them in forever for backwards compatibility.
  • Keep some of them in forever for backwards compatibility. If so, which ones?
  • Mark them as deprecated, remove them in a later release to give users time to switch.
  • Remove them as soon as interfacing with other libraries is implemented.

@btrepp
Copy link
Collaborator

btrepp commented May 31, 2024

I was thinking about this, it could be a good use-case for features?.
Eg say midi notes, the bare minimum needed is the u7 type.
User facing functions could take the type Into or return something that implements From. IIRC u7 is the full domain of a midi note.

This means the library is cut-down, minimal implementation, but the use can opt into a 'midi-types' feature that implements the traits from midi types. Users that wish to use external crates from note management either a) convert to/from in their own code, or b) add in a Pull request for the implementation of a new feature that allows people to 'seemlessly' use the external crate type.

As far as the change, a breaking change where they are dropped seems okay. I imagine this crate isn't used in a heap of things. So the affected parties are small.

Also I guess on types, everything in data is kinda 'if this type existed' we wouldn't need our own. I think the intent was always to have a more usbd-midi-data crate, with just the domain, and no real procedural logic. It just wasn't worth the effort at the time. If the data types in this crate are made into their own crate, other crates can implement the conversions aswell.

@sourcebox
Copy link
Collaborator Author

Regarding the SysEx processing, maybe the suggestion from @x37v mentioned in the original PR could be further investigated.

My initial idea here is to add an event() method to UsbMidiEventPacket that returns something like:

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

Because the enum variants contain &[u8] slices to the raw bytes, this would require to store them inside the packet as [u8; 4] instead of cable_number and message as it's done now. So it would be a breaking change because both fields are public. They could then be replaced by cable_number() and message() methods that create them on-the-fly.

For writing SysEx to the host, maybe a solution also based on iterators is possible. Not sure about that yet.

@laenzlinger
Copy link

I like the resolution to expose the regular midi raw data and let the application decide on the further midi processing.

@laenzlinger
Copy link

laenzlinger commented Dec 16, 2024

How can I use this crate to parse the raw packets into https://github.com/rust-midi/midi-types. Is there a recommended way?

@sourcebox
Copy link
Collaborator Author

How can I use this crate to parse the raw packets into https://github.com/rust-midi/midi-types. Is there a recommended way?

Unfortunately, there's no easy way to do it right now for two reasons:

  • I had a look into the docs of the midi-types crate and did not find a way to construct a message from a raw &[u8] slice. That would be one requirement.
  • usbd-midi currently doesn't offer a reliable way to access the underlying raw data beside than using &packet[1..]. That only works if you filter out certain packets by matching the code index number on packet[0] so that SysEx messages are discarded.

@x37v
Copy link
Collaborator

x37v commented Dec 17, 2024

midi-types simply represents the midi, midi-convert parses and renders

@sourcebox
Copy link
Collaborator Author

midi-types simply represents the midi, midi-convert parses and renders

Thanks for the info, that solves at least part of the question. On our side, making things nicer does require a breaking change which is planned after a version compatible with still-not-released usb-device 0.4.

@laenzlinger
Copy link

laenzlinger commented Dec 17, 2024

What can be used to get packages? Would I use the MidiPacketBufferReader to get the &packet[1..] ? However that does not seem to expose the raw data. Or do I need to use the master branch for that until 0.4 is released?

@sourcebox
Copy link
Collaborator Author

What can be used to get packages? Would I use the MidiPacketBufferReader to get the &packet[1..] ? However that does not seem to expose the raw data. Or do I need to use the master branch for that until 0.4 is released?

It's something like this:

let mut midi_class = MidiClass::new(&usb_bus, 1, 1).unwrap();
let mut buffer = [0; 64];

if let Ok(size) = midi_class.read(&mut buffer) {
    for chunk in buffer[..size].chunks(4) {
        // Convert the chunk to message
    }
}

The chunk is &[u8] with a size of 4, so you can slice it as &chunk[1..]. But there is a problem with it: I doubt that midi-convert will accept all messages as 3 byte slices because their length is dependent on the type.

@sourcebox
Copy link
Collaborator Author

To improve the whole situation, I started some rework on the next branch. The packet itself now stores the raw message so that it can be accessed later. Please have a look at the ESP32-S3 example. It now converts the raw packets to midi-types messages and prints them on the console. Please note that SysEx is still not supported and requires more work. There's also a good amount of optimization to be done.

@laenzlinger
Copy link

Thank you very much @sourcebox . I applied your example to my application and it works well so far!

Now I need to figure out how to use it in the other direction. I want to render rust-midi/midi-types messages and send them to a usbd_midi cable.

@btrepp
Copy link
Collaborator

btrepp commented Dec 17, 2024

How can I use this crate to parse the raw packets into https://github.com/rust-midi/midi-types. Is there a recommended way?

Unfortunately, there's no easy way to do it right now for two reasons:

  • I had a look into the docs of the midi-types crate and did not find a way to construct a message from a raw &[u8] slice. That would be one requirement.
  • usbd-midi currently doesn't offer a reliable way to access the underlying raw data beside than using &packet[1..]. That only works if you filter out certain packets by matching the code index number on packet[0] so that SysEx messages are discarded.

So what parts are people interested in here? And why. A quick glance and we have from note to u8 and u7.

The data structures in data should be constructable from raw midi data as well. The io layers just use data as if is a public library.

There might not be examples, but I think most of the requirements here are doable, or perhaps just need slight changes to the code if a from/to definition was missing

Edit: Ah. Is this the external crate interacting with this one?. So more an issue there?

All the types in this crate should have from and try from, so should be creatable in code, from their primitive bytes, you would just have to handle errors if you gave it invalid data.

@sourcebox
Copy link
Collaborator Author

Now I need to figure out how to use it in the other direction. I want to render rust-midi/midi-types messages and send them to a usbd_midi cable.

I will have a look into that, but please give me a bit of time.

@sourcebox
Copy link
Collaborator Author

@laenzlinger I updated the example to send midi-types generated messages.

@laenzlinger
Copy link

Thank you very much for this effort @sourcebox and implementing these hooks. It works as expected for my use case.

@sourcebox
Copy link
Collaborator Author

I did a good amount of refactoring in the next branch today, mainly motivated to isolate and feature-gate all types related to the messages. The crate can now be compiled without these types, so we can decide if we keep them. In my opinion it does not harm to have them, but there's surely some work to do on them, e.g. implement the missing realtime variants.

I also did some heavy reorganization on the module structure to simplify it. The changelog contains the details about all changes. The ESP32-S3 example is kept in sync with the modifications and can already interface with third-party crates like midi-types. The next step will be some work on the SysEx support.

I would also like to talk about the initial plan on waiting for usb-device version 0.4 to be released. While this is certainly a good idea to keep transition smooth, there's no time scheduling on that, meaning it can take forever. So maybe we should do the next release still based on usb-device version 0.3.2. That would also allow anyone to use the new features because their HALs are still compatible with the current usb-device crate.

@laenzlinger
Copy link

laenzlinger commented Dec 21, 2024

I have tested the latest next branch. It works for me as expected.

+1 for a release based on usd-device version 0.3.2.

I have a question to the example. I have seen that you are setting device_class and device_sub_class to 0.

   .device_class(0)
   .device_sub_class(0)

Previously I have used the constants of this crate (USB_AUDIO_CLASS, USB_MIDISTREAMING_SUBCLASS) to set these values. But now these constants are private. Does it make a difference if they are set to 0 as in the example?

@sourcebox
Copy link
Collaborator Author

Previously I have used the constants of this crate (USB_AUDIO_CLASS, USB_MIDISTREAMING_SUBCLASS) to set these values. But now these constants are private. Does it make a difference if they are set to 0 as in the example?

The problem with these constants is that they don't work with macOS, so they got removed from the example already a while ago:

See: #4

Commit: 726f677

@sourcebox
Copy link
Collaborator Author

I updated the initial post to reflect the latest developments. The next branch now has support for sending and receiving SysEx messages, the ESP32-S3 example shows how to do this. As next steps, I intend to add some unit tests to the packet module to do checks on the encoding and decoding of the packets in general but also more specific with SysEx. If these all pass, I will have a full test with a real-world project which heavily relies on SysEx communication.

@laenzlinger
Copy link

I had a quick look at the ESP32-S3 example and was asking myself: Why is a separate buffer used for Sysex messages? Couldn't the same buffer be reused as for "normal" MIDI messages?

@sourcebox
Copy link
Collaborator Author

I had a quick look at the ESP32-S3 example and was asking myself: Why is a separate buffer used for Sysex messages? Couldn't the same buffer be reused as for "normal" MIDI messages?

In theory, you can do this but there's a problem with that: while receiving a SysEx message consisting of several packets, the host can also insert some regular packets inbetween. In that case, you have to process these messages directly and continue to save your SysEx data thereafter. The use case for this scenario is with realtime messages like clock, so e.g. a sequencer can be still in sync while receiving large amounts of SysEx.

Since SysEx messages can be of arbitrary size, it's sometimes not even an option to buffer them while receiving but process them on the fly. The same can be done for generating them.

@sourcebox
Copy link
Collaborator Author

So far all the unit tests pass and my project test case also works quite well. I did a bunch of struct renamings where struct names were sometimes prefixed with UsbMidi, sometimes just with Midi. They all use UsbMidi now as prefix for consistency. Maybe we can have a look together about the whole naming and error handling topics in a future release.

I think, a new release can be done soon. The changelog is quite detailed about the modifications. Feel free to have a closer look and comment.

@laenzlinger
Copy link

I am currently trying to implement the OpenDeck Sysex Configuration Protocol. So far I did not find any problem with this usbd_midi.

@sourcebox
Copy link
Collaborator Author

Version 0.4.0 is now released on crates.io.

@sourcebox
Copy link
Collaborator Author

@btrepp I think you were the one who did most of the work on the message types. Would you like to add the missing System Common and System Realtime variants to the Message enum?

There's also one thing with Pitch Wheel messages that isn't that nice: Instead of being PitchWheelChange(Channel, U7, U7), changing this to PitchWheelChange(Channel, U14) would make more sense because it's a 14-bit value that's relevant to the user and forcing him to do the conversion manually is error-prone. midi-types e.g. does this by offering a Value14 struct.

@laenzlinger
Copy link

Just stumbled upon https://crates.io/crates/midi2

What do you think about this representation of MIDI messages? It looks to me to be already very complete.

@sourcebox
Copy link
Collaborator Author

Just stumbled upon https://crates.io/crates/midi2

What do you think about this representation of MIDI messages? It looks to me to be already very complete.

These are MIDI 2.0 message types, so they can't be used with usbd-midi right now since we don't support that standard. Doing so is maybe planned for the future, but requires a good amount of work.

@laenzlinger
Copy link

Thanks @sourcebox . Is it correct that MIDI2.0 would be a different USB class https://www.usb.org/sites/default/files/USB%20MIDI%20v2_0.pdf ?

I thought it might be worth mentioning this crate because you have also listed the MIDI 2.0 topic on the roadmap in the beginning of this thread. Instead of trying to introduce a representation of MIDI messages within this crate, would it make sense to base it on an existing crate?

@sourcebox
Copy link
Collaborator Author

Thanks @sourcebox . Is it correct that MIDI2.0 would be a different USB class https://www.usb.org/sites/default/files/USB%20MIDI%20v2_0.pdf ?

This crate could potentially be extended to offer MIDI 2.0 support. It would have to define an additional alternate setting for the 2.0 protocol. It's then up to the host to decide whether to use it or not. In any case, both protocol versions have to be supported by the crate and the user also has to separate the processing depending on the protocol.

Regarding the MIDI 2.0 message types, I would go the same route as we now do with midi-types: offer a simple way to interface with a third-party crate. Especially because there's still a lot of development going on with these types.

@sourcebox
Copy link
Collaborator Author

sourcebox commented Jan 12, 2025

I've added the missing variants for System Common and System Realtime to the Message enum. What's still to be done is implementing a new U14 type for the Pitch Wheel and Song Position Pointer values.

@sourcebox
Copy link
Collaborator Author

The U14 type is now implemented and used for Pitch Wheel and Song Position Pointer, but I've not done any real-world testing with them yet.

@sourcebox
Copy link
Collaborator Author

I published version 0.5.0 with the new message variants added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants