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.

Sign up for GitHub

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

PacketBuffer rewrite #9

Merged
merged 9 commits into from
Sep 13, 2017
Merged

PacketBuffer rewrite #9

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
afc9f20
Use upgraded version of coremidi-sys
Boddlnagg Jul 18, 2017
fbfdae7
Depend on now-published version of sys crate
Boddlnagg Jul 19, 2017
45d0044
Add more packet tests
Boddlnagg Jul 19, 2017
30f4054
Change semantics of PacketList type
Boddlnagg Jul 19, 2017
effc202
Change semantics of Packet in the same way as PacketList
Boddlnagg Jul 19, 2017
f554dd3
Simplify access to PacketList fields and validate layout
Boddlnagg Jul 19, 2017
07983d5
PacketBuffer rewrite
Boddlnagg Jul 19, 2017
f72086a
Rename PacketList::length -> len
Boddlnagg Jul 21, 2017
2e6b442
Addressed review comments & cleanup
Boddlnagg Sep 4, 2017
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
4 changes: 1 addition & 3 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,5 @@ keywords = ["CoreMIDI", "MIDI", "OSX", "music"]
[dependencies]
core-foundation-sys = "0.2"
core-foundation = "0.2"
coremidi-sys = "1.0.0"
# coremidi-sys = { git = "https://github.com/chris-zen/coremidi-sys", branch="fix-packed-structs" }
coremidi-sys = "2.0"
time = "0.1"
libc = "0.2"
8 changes: 4 additions & 4 deletions examples/send.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,17 +68,17 @@ fn print_destinations() {
}

fn create_note_on(channel: u8, note: u8, velocity: u8) -> coremidi::PacketBuffer {
let data = vec![
let data = &[
0x90 | (channel & 0x0f),
note & 0x7f,
velocity & 0x7f];
coremidi::PacketBuffer::from_data(0, data)
coremidi::PacketBuffer::new(0, data)
}

fn create_note_off(channel: u8, note: u8, velocity: u8) -> coremidi::PacketBuffer {
let data = vec![
let data = &[
0x80 | (channel & 0x0f),
note & 0x7f,
velocity & 0x7f];
coremidi::PacketBuffer::from_data(0, data)
coremidi::PacketBuffer::new(0, data)
}
8 changes: 4 additions & 4 deletions examples/virtual-source.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,17 +22,17 @@ fn main() {
}

fn create_note_on(channel: u8, note: u8, velocity: u8) -> coremidi::PacketBuffer {
let data = vec![
let data = &[
0x90 | (channel & 0x0f),
note & 0x7f,
velocity & 0x7f];
coremidi::PacketBuffer::from_data(0, data)
coremidi::PacketBuffer::new(0, data)
}

fn create_note_off(channel: u8, note: u8, velocity: u8) -> coremidi::PacketBuffer {
let data = vec![
let data = &[
0x80 | (channel & 0x0f),
note & 0x7f,
velocity & 0x7f];
coremidi::PacketBuffer::from_data(0, data)
coremidi::PacketBuffer::new(0, data)
}
12 changes: 6 additions & 6 deletions src/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use coremidi_sys::{
MIDIPortRef, MIDIOutputPortCreate, MIDIEndpointRef, MIDISourceCreate
};

use coremidi_sys_ext::{
use coremidi_sys::{
MIDIPacketList, MIDIInputPortCreate, MIDIDestinationCreate
};

Expand Down Expand Up @@ -151,7 +151,7 @@ impl Client {

extern "C" fn notify_proc(
notification_ptr: *const MIDINotification,
ref_con: *mut ::libc::c_void) {
ref_con: *mut ::std::os::raw::c_void) {

let _ = ::std::panic::catch_unwind(|| unsafe {
match Notification::from(&*notification_ptr) {
Expand All @@ -165,12 +165,12 @@ impl Client {

extern "C" fn read_proc(
pktlist: *const MIDIPacketList,
read_proc_ref_con: *mut ::libc::c_void,
_: *mut ::libc::c_void) { //srcConnRefCon
read_proc_ref_con: *mut ::std::os::raw::c_void,
_: *mut ::std::os::raw::c_void) { //srcConnRefCon

let _ = ::std::panic::catch_unwind(|| unsafe {
let packet_list = PacketList(pktlist);
BoxedCallback::call_from_raw_ptr(read_proc_ref_con, &packet_list);
let packet_list = &*(pktlist as *const PacketList);
BoxedCallback::call_from_raw_ptr(read_proc_ref_con, packet_list);
});
}
}
Expand Down
79 changes: 0 additions & 79 deletions src/coremidi_sys_ext.rs
View file
Open in desktop

This file was deleted.

8 changes: 2 additions & 6 deletions src/endpoints/sources.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,7 @@
use core_foundation_sys::base::OSStatus;

use coremidi_sys::{
MIDIGetNumberOfSources, MIDIGetSource, MIDIEndpointDispose, ItemCount
};

use coremidi_sys_ext::{
MIDIReceived
MIDIGetNumberOfSources, MIDIGetSource, MIDIReceived, MIDIEndpointDispose, ItemCount
};

use std::ops::Deref;
Expand Down Expand Up @@ -100,7 +96,7 @@ impl VirtualSource {
pub fn received(&self, packet_list: &PacketList) -> Result<(), OSStatus> {
let status = unsafe { MIDIReceived(
self.endpoint.object.0,
packet_list.0)
packet_list.as_ptr())
};
if status == 0 { Ok(()) } else { Err(status) }
}
Expand Down
50 changes: 35 additions & 15 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ use std::thread;
let client = coremidi::Client::new("example-client").unwrap();
let output_port = client.output_port("example-port").unwrap();
let destination = coremidi::Destination::from_index(0).unwrap();
let note_on = coremidi::PacketBuffer::from_data(0, vec![0x90, 0x40, 0x7f]);
let note_off = coremidi::PacketBuffer::from_data(0, vec![0x80, 0x40, 0x7f]);
let note_on = coremidi::PacketBuffer::new(0, &[0x90, 0x40, 0x7f]);
let note_off = coremidi::PacketBuffer::new(0, &[0x80, 0x40, 0x7f]);
output_port.send(&destination, &note_on).unwrap();
thread::sleep(Duration::from_millis(1000));
output_port.send(&destination, &note_off).unwrap();
Expand All @@ -41,16 +41,11 @@ For handling low level MIDI data you may look into:
extern crate core_foundation_sys;
extern crate core_foundation;
extern crate coremidi_sys;
extern crate libc;

use core_foundation_sys::base::OSStatus;

use coremidi_sys::{
MIDIObjectRef, MIDIFlushOutput, MIDIRestart
};

use coremidi_sys_ext::{
MIDIPacketList
MIDIObjectRef, MIDIFlushOutput, MIDIRestart, MIDIPacket, MIDIPacketList
};

/// A [MIDI Object](https://developer.apple.com/reference/coremidi/midiobjectref).
Expand Down Expand Up @@ -89,12 +84,12 @@ impl<T> BoxedCallback<T> {
BoxedCallback(::std::ptr::null_mut())
}

fn raw_ptr(&mut self) -> *mut ::libc::c_void {
self.0 as *mut ::libc::c_void
fn raw_ptr(&mut self) -> *mut ::std::os::raw::c_void {
self.0 as *mut ::std::os::raw::c_void
}

// must not be null
unsafe fn call_from_raw_ptr(raw_ptr: *mut ::libc::c_void, arg: &T) {
unsafe fn call_from_raw_ptr(raw_ptr: *mut ::std::os::raw::c_void, arg: &T) {
let callback = &mut *(raw_ptr as *mut Box<FnMut(&T)>);
callback(arg);
}
Expand Down Expand Up @@ -126,7 +121,7 @@ pub struct Port { object: Object }
/// let client = coremidi::Client::new("example-client").unwrap();
/// let output_port = client.output_port("example-port").unwrap();
/// let destination = coremidi::Destination::from_index(0).unwrap();
/// let packets = coremidi::PacketBuffer::from_data(0, vec![0x90, 0x40, 0x7f]);
/// let packets = coremidi::PacketBuffer::new(0, &[0x90, 0x40, 0x7f]);
/// output_port.send(&destination, &packets).unwrap();
/// ```
#[derive(Debug)]
Expand Down Expand Up @@ -217,12 +212,37 @@ pub struct VirtualDestination {
#[derive(PartialEq)]
pub struct Device { object: Object }

#[cfg(any(target_arch = "arm", target_arch = "aarch64"))]
type AlignmentMarker = [u32; 0]; // ensures 4-byte alignment (on ARM)


#[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
type AlignmentMarker = [u8; 0]; // unaligned
Copy link
Owner

Choose a reason for hiding this comment

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

What about other architectures than arm or x86 ? would it be possible to have the unaligned case for all the non arm-aarch64 architectures ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that would be possible. I'm just not sure wheter "unaligned" is the right default, so I did it in such a way that it would error on other architectures. Is macOS/iOS even available for other architectures?

Copy link
Owner

@chris-zen chris-zen Sep 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Is macOS/iOS even available for other architectures?

Good point. I guess that failing in case of unknown architecture is an acceptable solution here.


/// A [list of MIDI events](https://developer.apple.com/reference/coremidi/midipacketlist) being received from, or being sent to, one endpoint.
///
#[derive(PartialEq)]
pub struct PacketList(*const MIDIPacketList);
#[repr(C)]
pub struct PacketList {
// NOTE: This type must only exist in the form of immutable references
// pointing to valid instances of MIDIPacketList.
// This type must NOT implement `Copy`!
inner: PacketListInner,
_do_not_construct: AlignmentMarker
Copy link
Owner

Choose a reason for hiding this comment

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

is _do_not_construct a convention name for such cases ? If not, could you rename to something like _alignment_marker ?

Copy link
Contributor Author

@Boddlnagg Boddlnagg Sep 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It's not a convention. I just did this to solve two problems at once, namely (1) to have the correct alignment and (2) to give a strong hint that any code that would try to construct an instance of this type would be wrong (but this is only relevant within the library, not for users of the library). I can rename if you prefer.

Copy link
Owner

Choose a reason for hiding this comment

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

Not feeling strong about this renaming, but given that it is not exposed to the public through the API, (1) is the main point to hint, so the proposed name looks more intuitive to me. You can focus on other comments first, and if having enough time do this ;-)

}

#[repr(packed)]
struct PacketListInner {
num_packets: u32,
data: [MIDIPacket; 0]
Copy link
Owner

Choose a reason for hiding this comment

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

I am wondering if the differentiation between PacketList and PacketListInner is really necessary. Couldn't we just put the alignment marker after the data field and rename PacketListInner to PacketList ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, PacketListInner is necessary, because of a lack of the #[repr(packed = "N")] feature in Rust (see rust-lang/rust#33158). We want an alignment of 4 on ARM and 1 on other architectures, but #[repr(packed)] always gives an alignment of 1. So I'm defining PacketListInner with an alignment of 1, and using that within PacketList together with AlignmentMarker, so PacketList will choose the alignment as max(alignof(PacketListInner), alignof(AlignmentMarker)), which results in the correct alignment.

}

mod coremidi_sys_ext;
impl PacketList {
/// For internal usage only.
/// Requires this instance to actually point to a valid MIDIPacketList
unsafe fn as_ptr(&self) -> *mut MIDIPacketList {
self as *const PacketList as *mut PacketList as *mut MIDIPacketList
}
}

mod object;
mod devices;
Expand Down
4 changes: 2 additions & 2 deletions src/notifications.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ pub enum Notification {

impl Notification {
pub fn from(notification: &MIDINotification) -> Result<Notification, i32> {
match notification.messageID as ::libc::c_uint {
match notification.messageID as ::std::os::raw::c_uint {
kMIDIMsgSetupChanged => Ok(Notification::SetupChanged),
kMIDIMsgObjectAdded | kMIDIMsgObjectRemoved => Self::from_object_added_removed(notification),
kMIDIMsgPropertyChanged => Self::from_property_changed(notification),
Expand All @@ -84,7 +84,7 @@ impl Notification {
child: Object(add_remove_notification.child),
child_type: child_type.unwrap()
};
match notification.messageID as ::libc::c_uint {
match notification.messageID as ::std::os::raw::c_uint {
kMIDIMsgObjectAdded => Ok(Notification::ObjectAdded(add_remove_info)),
kMIDIMsgObjectRemoved => Ok(Notification::ObjectRemoved(add_remove_info)),
_ => Err(0) // Never reached
Expand Down
Loading