WIP: Convert CV2 <-> CV1

[Rahzael]

A work in progress attempt to implement appendix D of the Midi 2.0 spec that allows for automatic conversion between specific MIDI 2.0 and 1.0 messages.

See #16 for details.

Conversion Algorithms

• MinCenterMax
  • Upscale
  • Downscale
• ZeroExtension
  • Upscale
  • Downscale
• SteppedValue (might not be neccesary?)
  • Upscale
  • Downscale

CV2 -> CV1

• Note On
  • From<CV2, CV1>
  • FromCV2<CV2>
  • TryFromCV2<CV2>
• Note Off
  • From<CV2, CV1>
  • FromCV2<CV2>
  • TryFromCV2<CV2>
• Key Pressure
  • From<CV2, CV1>
  • FromCV2<CV2>
  • TryFromCV2<CV2>
• Control Change
  • From<CV2, CV1>
  • FromCV2<CV2>
  • TryFromCV2<CV2>
• Channel Pressure
  • From<CV2, CV1>
  • FromCV2<CV2>
  • TryFromCV2<CV2>
• Pitch Bend
  • From<CV2, CV1>
  • FromCV2<CV2>
  • TryFromCV2<CV2>

CV2 <- CV1

• Note On
  • TryFrom<CV1, CV2>
  • TryFromCV1<CV1>
• Note Off
  • From<CV1, CV2>
  • FromCV1<CV1>
  • TryFromCV1<CV1>
  • TryFromCV1<CV1::NoteOn>
• Key Pressure
  • From<CV1, CV2>
  • FromCV1<CV1>
  • TryFromCV1<CV1>
• Control Change
  • From<CV1, CV2>
  • FromCV1<CV1>
  • TryFromCV1<CV1>
• Channel Pressure
  • From<CV1, CV2>
  • FromCV1<CV1>
  • TryFromCV1<CV1>
• Pitch Bend
  • From<CV1, CV2>
  • FromCV1<CV1>
  • TryFromCV1<CV1>

[BenLeadbetter]

Clippy has some good suggestions btw 🤓

[Rahzael]

Clippy has some good suggestions btw 🤓

I'll check them out! I need to figure how to get it to automatically run with my setup so I see them myself before uploading. I had assumed that it would automatically run whenever I ran cargo test but apparently that isn't the case.

[BenLeadbetter]

I'll check them out! I need to figure how to get it to automatically run with my setup so I see them myself before uploading. I had assumed that it would automatically run whenever I ran cargo test but apparently that isn't the case.

You should be able to run them locally with

> cargo clippy --all-features

[BenLeadbetter]

How's the work here coming along ? Is there anything I can do to help ?

I'd like to get this feature into the forthcoming 0.12.0 feature release.

[Rahzael]

How's the work here coming along ? Is there anything I can do to help ?

I'd like to get this feature into the forthcoming 0.12.0 feature release.

Apologies for the delay on this. I had some technical difficulties with my setup then was focused on trying to apply for funding for a programming language that I'm working on.

I'm going back through now trying to remember what is done and what isn't. I'm going to add this info to the PRs description so I don't forget later.

[Rahzael]

@BenLeadbetter I've updated the PR's description with what work is done and what work I think I have yet to do.

Let me know if any of that seems like it should be out of scope for this PR (sysex7/8 perhaps?).

[Rahzael]

@BenLeadbetter Also, which MSRV are we targeting? I'm getting an incompatible rustc version with fixed@1.31.0 when attempting to recompile with my new setup.

[BenLeadbetter]

@BenLeadbetter I've updated the PR's description with what work is done and what work I think I have yet to do.

Let me know if any of that seems like it should be out of scope for this PR (sysex7/8 perhaps?).

Sorry for the slow reply. Great to see this work picked up again!

Yes, I think sysex8 and sysex7 shouldn't need to implement these new conversion traits. For one thing The traits being named "CV" explicitly excludes them - only the channel_voice* modules should implement these.

[Rahzael]

@BenLeadbetter How do you want to handle the case where a CV1 NoteOn message with 0 velocity needs to be converted to a CV2 NoteOff, not a CV2 NoteOn?

Do you want me to only implement a TryFrom<CV1, CV2> with the zero velocity case returning an new type of error, or should I investigate some sort of enum implementation that returns the correct CV2 message based on the velocity of the of the CV1 NoteOn?

[BenLeadbetter]

@BenLeadbetter How do you want to handle the case where a CV1 NoteOn message with 0 velocity needs to be converted to a CV2 NoteOff, not a CV2 NoteOn?

Do you want me to only implement a TryFrom<CV1, CV2> with the zero velocity case returning an new type of error, or should I investigate some sort of enum implementation that returns the correct CV2 message based on the velocity of the of the CV1 NoteOn?

I guess we could return a channel_voice2::Message which is already an aggregated enum of all the different message possibilities .. It's not ideal that it is more general than it needs to be, though.

Returning an error could be an option, I guess - technically converting a zero-velocity cv1 into a cv2::NoteOn could be regarded as invalid data. This would also save us a rather ugly, asymmetrical API too. Only thing is that it pushed the responsibility to handle this special case back onto the client..

On balance I think returning an Error::InvalidData(&str) with a detailed message will suffice. In future perhaps we can offer a utility function for converting note-on events and handling this case, but we can regard that as a refinement. Let's implement this first iteration with a consistent and foundational API which we can build upon later.

[Rahzael]

@BenLeadbetter Does this look about right? I return InvalidData if the velocity is 0, or BufferOverflow if it fails to allocate, and it will only attempt to allocate if the velocity isn't 0 (to avoid wasting memory in the target buffer).

/// Tries to convert a CV1 Note On message to CV2 Note On message,
/// storing the result in a pre-allocated CV2 Note On.
///
/// Will fail if the CV1 Note On has 0 Velocity, as it must be converted
/// to a CV2 Note Off message instead.
#[cfg(feature = "channel-voice1")]
impl<
        A: crate::buffer::Buffer<Unit = u32>,
        B: crate::buffer::Buffer<Unit = u32> + crate::buffer::BufferMut,
    > TryFrom<(crate::channel_voice1::NoteOn<A>, NoteOn<B>)> for NoteOn<B>
{
    type Error = crate::error::InvalidData;

    fn try_from(val: (crate::channel_voice1::NoteOn<A>, NoteOn<B>)) -> Result<Self, Self::Error> {
        use crate::conversion::MinCenterMax;
        use crate::error::InvalidData;
        use crate::traits::{Channeled, Grouped};

        let (src, mut dest) = val;
        if src.velocity() == ux::u7::new(0) {
            Err(InvalidData("CV1 Note On messages with 0 veolicty should be converted to CV2 Note Off messages."))
        } else {
            dest.set_group(src.group());
            dest.set_channel(src.channel());
            dest.set_note_number(src.note_number());
            dest.set_velocity(src.velocity().mcm_upscale::<u16>());
            Ok(dest)
        }
    }
}

/// Tries to convert a CV1 Note On message to CV2 Note On message.
///
/// Will fail if the CV1 Note On has 0 Velocity, as it must be converted
/// to a CV2 Note Off message instead.
///
/// Will also fail if there is not enough room in the destination buffer to
/// allocate a new CV2 NoteOn.
///
/// Will only attempt to allocate a new CV2 Note On if the given
/// CV1 NoteOn has a non-zero velocity.
#[cfg(feature = "channel-voice1")]
impl<
        A: crate::buffer::Buffer<Unit = u32>,
        B: crate::buffer::Buffer<Unit = u32>
            + crate::buffer::BufferMut
            + crate::buffer::BufferDefault
            + crate::buffer::BufferTryResize,
    > crate::TryFromCv1<crate::channel_voice1::NoteOn<A>> for NoteOn<B>
{
    type Error = crate::error::Error;

    fn try_from_cv1(val: crate::channel_voice1::NoteOn<A>) -> Result<Self, Self::Error> {
        use crate::error::InvalidData;

        if val.velocity() == ux::u7::new(0) {
            Err(Self::Error::InvalidData(InvalidData("CV1 Note On messages with 0 veolicty should be converted to CV2 Note Off messages.")))
        } else {
            let dest = NoteOn::<B>::try_new()?;
            Ok((val, dest)
                .try_into()
                .expect("Conversion should not fail. We already checked for 0 velocity."))
        }
    }
}

[BenLeadbetter]

@BenLeadbetter Does this look about right? I return InvalidData if the velocity is 0, or BufferOverflow if it fails to allocate, and it will only attempt to allocate if the velocity isn't 0 (to avoid wasting memory in the target buffer).

LGTM 👍

Perhaps the implementation of NoteOn::try_from_cv1 could be even more terse ?

#[cfg(feature = "channel-voice1")]
impl<
        A: crate::buffer::Buffer<Unit = u32>,
        B: crate::buffer::Buffer<Unit = u32>
            + crate::buffer::BufferMut
            + crate::buffer::BufferDefault
            + crate::buffer::BufferTryResize,
    > crate::TryFromCv1<crate::channel_voice1::NoteOn<A>> for NoteOn<B>
{
    fn try_from_cv1(val: crate::channel_voice1::NoteOn<A>) -> Result<Self, Self::Error> {
        let dest = NoteOn::<B>::try_new()?;
        Ok((val, dest).try_into()?)
    }
}

[Rahzael]

@BenLeadbetter Does this look about right? I return InvalidData if the velocity is 0, or BufferOverflow if it fails to allocate, and it will only attempt to allocate if the velocity isn't 0 (to avoid wasting memory in the target buffer).

LGTM 👍

Perhaps the implementation of NoteOn::try_from_cv1 could be even more terse ?

#[cfg(feature = "channel-voice1")]
impl<
        A: crate::buffer::Buffer<Unit = u32>,
        B: crate::buffer::Buffer<Unit = u32>
            + crate::buffer::BufferMut
            + crate::buffer::BufferDefault
            + crate::buffer::BufferTryResize,
    > crate::TryFromCv1<crate::channel_voice1::NoteOn<A>> for NoteOn<B>
{
    fn try_from_cv1(val: crate::channel_voice1::NoteOn<A>) -> Result<Self, Self::Error> {
        let dest = NoteOn::<B>::try_new()?;
        Ok((val, dest).try_into()?)
    }
}

I think there is a subtle distinction here. I want to be able to guarantee that the destination buffer isn't modified if the conversion fails.

In general, I'm trying to consider the case where we attempt to convert a CV1 NoteOn with 0 velocity to a CV2 NoteOn with a destination buffer that can contain multiple messages. We shouldn't allocate a CV2 NoteOn if the CV1 NoteOn has 0 velocity, as we instead need to allocate a CV2 NoteOff.

Since our destination buffer can be stateful, I think we should only allocate in the case where we know we are allocating the correct type of message. I think this is especially true in the case where we are trying to convert a buffer of multiple CV1 messages into a buffer of CV2 messages. Our destination buffer might not support deallocation, so any erronous allocations would require rebuffering every previous successful allocation before the erroneous one into a new buffer (which in the embedded case we might not actually have the memory for).

Let me know if you don't think this is worth the extra check and using expect(), which might (but should never) panic. I.e. the most valuable assertions are the ones that should never fail.

[BenLeadbetter]

Since our destination buffer can be stateful, I think we should only allocate in the case where we know we are allocating the correct type of message. I think this is especially true in the case where we are trying to convert a buffer of multiple CV1 messages into a buffer of CV2 messages. Our destination buffer might not support deallocation, so any erronous allocations would require rebuffering every previous successful allocation before the erroneous one into a new buffer (which in the embedded case we might not actually have the memory for).

Fair play - I hadn't considered the optimization of saving the buffer allocation. Let's do it the way you suggest 👍