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

format and simplify oranization #1

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,6 @@ license = "MIT"

[dependencies]
embedded-hal = "0.2.2"
nb = "1.0.0"
usb-device = "0.2.3"
midi-convert = "0.1.1"
midi-types = "0.1.4"

[dependencies.num_enum]
version = "0.5.1"
default-features = false
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ fn main() -> ! {
let mut buffer = [0; 64];

if let Ok(size) = midi.read(&mut buffer) {
let buffer_reader = MidiPacketBufferReader::new(&buffer, size);
let buffer_reader = MidiPacketBufferReader::new(&buffer[0..size]);
for packet in buffer_reader.into_iter() {
if let Ok(packet) = packet {
match packet.message {
Expand Down
9 changes: 1 addition & 8 deletions src/data/usb_midi/cable_number.rs → src/cable_number.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use crate::data::byte::u4::U4;
use core::convert::TryFrom;

/// The Cable Number (CN) is a value ranging from 0x0 to 0xF
Expand All @@ -25,7 +24,7 @@ pub enum CableNumber {
Cable15 = 0xF,
}

#[derive(Debug,Clone,Copy,Eq, PartialEq)]
#[derive(Debug, Clone, Copy, Eq, PartialEq)]
pub struct InvalidCableNumber(u8);

impl TryFrom<u8> for CableNumber {
Expand Down Expand Up @@ -59,12 +58,6 @@ impl From<CableNumber> for u8 {
}
}

impl From<CableNumber> for U4 {
fn from(value: CableNumber) -> U4 {
U4::from_overflowing_u8(u8::from(value))
}
}

#[cfg(test)]
mod tests {

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use crate::data::byte::u4::U4;
use crate::midi_types::MidiMessage;
use core::convert::TryFrom;

Expand All @@ -18,9 +17,9 @@ impl TryFrom<u8> for CodeIndexNumber {
}
}

impl From<CodeIndexNumber> for U4 {
fn from(value: CodeIndexNumber) -> U4 {
U4::from_overflowing_u8(value.0)
impl From<CodeIndexNumber> for u8 {
fn from(value: CodeIndexNumber) -> u8 {
value.0
}
}

Expand Down
4 changes: 4 additions & 0 deletions src/data/usb/constants.rs → src/constants.rs
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe constants could be grouped into namespaces, eg. constants::usb, and misc. constants can just live in the top-level module, eg.

pub mod usb {
    pub const USB_CLASS_NONE: u8 = 0x00;
    pub const USB_AUDIO_CLASS: u8 = 0x01;
    // ...
}

pub const MIDI_PACKET_SIZE: usize = 4;
pub const MAX_PACKET_SIZE: usize = 64;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reduced the public constants to 6 total, the additional usb module seemed a bit excessive for use and navigating docs..

Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
//! Shared constants
//This should be better organized in future

pub const USB_CLASS_NONE: u8 = 0x00;
Expand All @@ -13,3 +14,6 @@ pub const CS_ENDPOINT: u8 = 0x25;
pub const HEADER_SUBTYPE: u8 = 0x01;
pub const MS_HEADER_SUBTYPE: u8 = 0x01;
pub const MS_GENERAL: u8 = 0x01;

pub const MIDI_PACKET_SIZE: usize = 4;
pub const MAX_PACKET_SIZE: usize = 64;
1 change: 0 additions & 1 deletion src/data/byte/mod.rs

This file was deleted.

46 changes: 0 additions & 46 deletions src/data/byte/u4.rs

This file was deleted.

3 changes: 0 additions & 3 deletions src/data/mod.rs

This file was deleted.

1 change: 0 additions & 1 deletion src/data/usb/mod.rs

This file was deleted.

39 changes: 0 additions & 39 deletions src/data/usb_midi/midi_packet_reader.rs

This file was deleted.

4 changes: 0 additions & 4 deletions src/data/usb_midi/mod.rs

This file was deleted.

66 changes: 32 additions & 34 deletions src/data/usb_midi/usb_midi_event_packet.rs → src/event_packet.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,10 @@
use crate::data::byte::u4::U4;
use crate::data::usb_midi::cable_number::CableNumber;
use crate::data::usb_midi::code_index_number::CodeIndexNumber;
use crate:: midi_types::MidiMessage;
use core::convert::TryFrom;
use midi_convert::{
MidiParseError,
MidiRenderSlice,
MidiTryParseSlice,
use crate::{
cable_number::CableNumber,
code_index_number::CodeIndexNumber,
midi_convert::{MidiParseError, MidiRenderSlice, MidiTryParseSlice},
midi_types::MidiMessage,
};

use core::convert::TryFrom;

/// A packet that communicates with the host
/// Currently supported is sending the specified normal midi
Expand All @@ -22,22 +18,29 @@ pub struct UsbMidiEventPacket {
impl From<UsbMidiEventPacket> for [u8; 4] {
fn from(value: UsbMidiEventPacket) -> [u8; 4] {
let message = value.message;
let cable_number = U4::from(value.cable_number);
let index_number = {
let code_index = CodeIndexNumber::find_from_message(&message);
U4::from(code_index)
};
let header = U4::combine(cable_number, index_number);
let cable_number: u8 = value.cable_number.into();
let index_number: u8 = CodeIndexNumber::find_from_message(&message).into();
let header = cable_number << 4 | index_number;
let mut data: [u8; 4] = [header, 0, 0, 0];
message.render_slice(&mut data[1..]);

data
}
}

#[derive(Debug)]
impl From<(CableNumber, MidiMessage)> for UsbMidiEventPacket {
fn from(value: (CableNumber, MidiMessage)) -> Self {
let (cable_number, message) = value;
Self {
cable_number,
message,
}
}
}

#[derive(Debug, PartialEq, Clone)]
pub enum MidiPacketParsingError {
MissingCableNumber,
MissingHeader,
MissingDataPacket,
ParseError(MidiParseError),
}
Expand All @@ -48,16 +51,18 @@ impl TryFrom<&[u8]> for UsbMidiEventPacket {
fn try_from(value: &[u8]) -> Result<Self, Self::Error> {
let raw_cable_number = match value.get(0) {
Some(byte) => *byte >> 4,
None => return Err(MidiPacketParsingError::MissingCableNumber)
None => return Err(MidiPacketParsingError::MissingHeader),
};

let cable_number = CableNumber::try_from(u8::from(raw_cable_number)).expect("(u8 >> 4) < 16");
let cable_number =
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should be handled as a ParseError/similar instead of just expecting? If the MIDI packet is malformed for some reason this could reasonably happen, and straight up crashing in a try_from is probably not ideal. Maybe there should also be a sanity check here for the MIDI magic string (I assume that's what the first 4 bytes are?)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0xFF >> 4 == 0xF which will never fail here, so maybe it should just be .unwrap() but I don't think we need a parse error

CableNumber::try_from(u8::from(raw_cable_number)).expect("(u8 >> 4) < 16");
let message_body = match value.get(1..) {
Some(bytes) => bytes,
None => return Err(MidiPacketParsingError::MissingDataPacket),
};

let message = MidiMessage::try_parse_slice(message_body).map_err(|e| MidiPacketParsingError::ParseError(e))?;
let message = MidiMessage::try_parse_slice(message_body)
.map_err(|e| MidiPacketParsingError::ParseError(e))?;

Ok(UsbMidiEventPacket {
cable_number,
Expand All @@ -66,28 +71,21 @@ impl TryFrom<&[u8]> for UsbMidiEventPacket {
}
}

impl UsbMidiEventPacket {
pub fn from_midi(cable: CableNumber, midi: MidiMessage) -> UsbMidiEventPacket {
UsbMidiEventPacket {
cable_number: cable,
message: midi,
}
}
}

#[cfg(test)]
mod tests {
use crate::data::usb_midi::cable_number::CableNumber::{Cable0, Cable1};
use crate::data::usb_midi::usb_midi_event_packet::UsbMidiEventPacket;
use crate::midi_types::{Channel, Control, MidiMessage, Note, Program, Value14, Value7};
use crate::{
cable_number::CableNumber::{Cable0, Cable1},
event_packet::UsbMidiEventPacket,
midi_types::{Channel, Control, MidiMessage, Note, Program, Value14, Value7},
};
use core::convert::TryFrom;

macro_rules! decode_message_test {
($($id:ident:$value:expr,)*) => {
$(
#[test]
fn $id() {
let (usb_midi_data_packet,expected) = $value;
let (usb_midi_data_packet, expected) = $value;
let message = UsbMidiEventPacket::try_from(&usb_midi_data_packet[..]).unwrap();
assert_eq!(expected, message);
}
Expand Down
21 changes: 17 additions & 4 deletions src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,20 @@
#![no_std]

pub mod data;
pub mod midi_device;
mod util;

/// re-exports
pub use midi_convert;
pub use midi_types;

pub mod constants;

pub use {
cable_number::{CableNumber, InvalidCableNumber},
event_packet::{MidiPacketParsingError, UsbMidiEventPacket},
midi_device::{MidiClass, MidiClassInvalidArgs},
packet_reader::MidiPacketBufferReader,
};

mod cable_number;
mod code_index_number;
mod event_packet;
mod midi_device;
mod packet_reader;
33 changes: 11 additions & 22 deletions src/midi_device.rs
Original file line number Diff line number Diff line change
@@ -1,17 +1,10 @@
use crate::data::usb::constants::*;
use crate::data::usb_midi::usb_midi_event_packet::{MidiPacketParsingError, UsbMidiEventPacket};
use usb_device::class_prelude::*;
use usb_device::Result;
use crate::{constants::*, event_packet::UsbMidiEventPacket};
use usb_device::{class_prelude::*, Result};

const MIDI_IN_SIZE: u8 = 0x06;
const MIDI_OUT_SIZE: u8 = 0x09;
Comment on lines 4 to 5
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be moved to constants?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they're not pub, only used in this file, so I don't think so


pub const MIDI_PACKET_SIZE: usize = 4;
pub const MAX_PACKET_SIZE: usize = 64;

///Note we are using MidiIn here to refer to the fact that
///The Host sees it as a midi in device
///This class allows you to send data in
/// USB Midi Device
pub struct MidiClass<'a, B: UsbBus> {
standard_ac: InterfaceNumber,
standard_mc: InterfaceNumber,
Expand All @@ -21,13 +14,9 @@ pub struct MidiClass<'a, B: UsbBus> {
n_out_jacks: u8,
}

pub enum MidiReadError {
ParsingFailed(MidiPacketParsingError),
UsbError(UsbError),
}

/// Invalid construction error struct
#[derive(Debug)]
pub struct InvalidArguments;
pub struct MidiClassInvalidArgs;

impl<B: UsbBus> MidiClass<'_, B> {
/// Creates a new MidiClass with the provided UsbBus and `n_in/out_jacks` embedded input/output jacks (or "cables",
Expand All @@ -37,9 +26,9 @@ impl<B: UsbBus> MidiClass<'_, B> {
alloc: &UsbBusAllocator<B>,
n_in_jacks: u8,
n_out_jacks: u8,
) -> core::result::Result<MidiClass<'_, B>, InvalidArguments> {
) -> core::result::Result<MidiClass<'_, B>, MidiClassInvalidArgs> {
if n_in_jacks > 16 || n_out_jacks > 16 {
return Err(InvalidArguments);
return Err(MidiClassInvalidArgs);
}
Ok(MidiClass {
standard_ac: alloc.interface(),
Expand All @@ -66,23 +55,23 @@ impl<B: UsbBus> MidiClass<'_, B> {
/// calculates the index'th external midi in jack id
fn in_jack_id_ext(&self, index: u8) -> u8 {
debug_assert!(index < self.n_in_jacks);
return 2 * index + 1;
2 * index + 1
}
/// calculates the index'th embedded midi out jack id
fn out_jack_id_emb(&self, index: u8) -> u8 {
debug_assert!(index < self.n_in_jacks);
return 2 * index + 2;
2 * index + 2
}

/// calculates the index'th external midi out jack id
fn out_jack_id_ext(&self, index: u8) -> u8 {
debug_assert!(index < self.n_out_jacks);
return 2 * self.n_in_jacks + 2 * index + 1;
2 * self.n_in_jacks + 2 * index + 1
}
/// calculates the index'th embedded midi in jack id
fn in_jack_id_emb(&self, index: u8) -> u8 {
debug_assert!(index < self.n_out_jacks);
return 2 * self.n_in_jacks + 2 * index + 2;
2 * self.n_in_jacks + 2 * index + 2
}
}

Expand Down
Loading