format and simplify oranization #1
Conversation
x37v
commented
Mar 27, 2023
•
x37v
commented
- run cargo fmt
- remove empty files
- reduce the module depth and complexity (it is a simple project but using it as a library wasn't that simple)
- a few misc changes/enhancements
there were error conditions that couldn't happen
we are missing more than just the cable number in this case
|
hey @132ikl care to check this one? |
|
Sure, although it might be a couple days before I'm able to get around to it |
|
I haven't forgotten about this, just been crazy busy. Will review as soon as I can 😅 |
| }; | ||
|
|
||
| let cable_number = CableNumber::try_from(u8::from(raw_cable_number)).expect("(u8 >> 4) < 16"); | ||
| let cable_number = |
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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
src/constants.rs
Outdated
There was a problem hiding this comment.
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;There was a problem hiding this comment.
I reduced the public constants to 6 total, the additional usb module seemed a bit excessive for use and navigating docs..
src/packet_reader.rs
Outdated
| pub struct MidiPacketBufferReader<'a> { | ||
| inner: core::slice::Chunks<'a, u8>, | ||
| } |
There was a problem hiding this comment.
Is there a reason for using a normal struct instead of a tuple struct here?
| const MIDI_IN_SIZE: u8 = 0x06; | ||
| const MIDI_OUT_SIZE: u8 = 0x09; |
There was a problem hiding this comment.
they're not pub, only used in this file, so I don't think so
