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

Add trait for linear allocation to enforce use of LinearAllocator at compile-time #188

Merged
merged 3 commits into from
May 19, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
6 changes: 3 additions & 3 deletions ctru-rs/examples/audio-filters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,10 @@ fn main() {

// We create a buffer on the LINEAR memory that will hold our audio data.
// It's necessary for the buffer to live on the LINEAR memory sector since it needs to be accessed by the DSP processor.
let mut audio_data1 = Box::new_in([0u8; AUDIO_WAVE_LENGTH], LinearAllocator);
let mut audio_data1: Box<[_], _> = Box::new_in([0u8; AUDIO_WAVE_LENGTH], LinearAllocator);
Meziu marked this conversation as resolved.
Show resolved Hide resolved

// Fill the buffer with the first set of data. This simply writes a sine wave into the buffer.
fill_buffer(audio_data1.as_mut_slice(), NOTEFREQ[4]);
fill_buffer(&mut audio_data1, NOTEFREQ[4]);

// Clone the original buffer to obtain an equal buffer on the LINEAR memory used for double buffering.
let audio_data2 = audio_data1.clone();
Expand Down Expand Up @@ -154,7 +154,7 @@ fn main() {
}

// Double buffer alternation depending on the one used.
let current: &mut Wave = if altern {
let current: &mut Wave<_> = if altern {
&mut wave_info1
} else {
&mut wave_info2
Expand Down
1 change: 1 addition & 0 deletions ctru-rs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#![feature(try_trait_v2)]
#![feature(allocator_api)]
#![feature(new_uninit)]
#![feature(diagnostic_namespace)]
Copy link
Member

Choose a reason for hiding this comment

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

This feature is stable, there is no need to add it to the list.

Copy link
Member Author

@ian-h-chamberlain ian-h-chamberlain May 18, 2024

Choose a reason for hiding this comment

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

Unfortunately it's not stable yet in the pinned compiler version we have... I could raise the rust-version in our crate to 1.78 and bump the pinned nightly, if you think that's preferable?

Edit: actually, tried this but it won't work unless we update to a very recent nightly that contains rust-lang/rust#124649
My preference would probably be to leave it with #![feature] for now, but could still go either way.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right. I'm for the idea of "if we use an unstable feature, we should update the MSRV as soon as it is stabilised" since stability is the biggest reason I see why we should bump the MSRV.

Still, for such a small thing it isn't necessary. We can bump it later down the line whenever we see fit.

#![test_runner(test_runner::run_gdb)] // TODO: does this make sense to have configurable?
#![doc(
html_favicon_url = "https://user-images.githubusercontent.com/11131775/225929072-2fa1741c-93ae-4b47-9bdf-af70f3d59910.png"
Expand Down
26 changes: 26 additions & 0 deletions ctru-rs/src/linear.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@

use std::alloc::{AllocError, Allocator, Layout};
use std::ptr::NonNull;
use std::rc::{self, Rc};
use std::sync::{self, Arc};

// Implementing an `std::alloc::Allocator` type is the best way to handle this case, since it gives
// us full control over the normal `std` implementations (like `Box`). The only issue is that this is another unstable feature to add.
Expand Down Expand Up @@ -47,3 +49,27 @@ unsafe impl Allocator for LinearAllocator {
}
}
}

/// Trait indicating a type has been allocated using [`LinearAllocator`].
/// This can be used to enforce that a given slice was allocated in LINEAR memory.
///
/// # Safety
///
/// Implementing this trait is a promise that the backing storage was allocated with
/// [`LinearAllocator`]. If this is not the case, attempting to use the
/// data with a `LinearAllocation` bound may result in undefined behavior.
#[diagnostic::on_unimplemented(
Meziu marked this conversation as resolved.
Show resolved Hide resolved
message = "{Self} is not allocated with `ctru::linear::LinearAllocator`"
)]
pub unsafe trait LinearAllocation {}

unsafe impl<T> LinearAllocation for Vec<T, LinearAllocator> {}
unsafe impl<T: ?Sized> LinearAllocation for Rc<T, LinearAllocator> {}
unsafe impl<T: ?Sized> LinearAllocation for rc::Weak<T, LinearAllocator> {}
unsafe impl<T: ?Sized> LinearAllocation for Arc<T, LinearAllocator> {}
unsafe impl<T: ?Sized> LinearAllocation for sync::Weak<T, LinearAllocator> {}
unsafe impl<T: ?Sized> LinearAllocation for Box<T, LinearAllocator> {}

// We could also impl for various std::collections types, but it seems unlikely
// those would ever be used for this purpose in practice, since most of the type
// we're dereferencing to a &[T]. The workaround would just be to convert to a Vec/Box.
8 changes: 6 additions & 2 deletions ctru-rs/src/services/ndsp/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ pub mod wave;
use wave::{Status, Wave};

use crate::error::ResultCode;
use crate::linear::LinearAllocation;
use crate::services::ServiceReference;

use std::cell::{RefCell, RefMut};
Expand Down Expand Up @@ -523,7 +524,7 @@ impl Channel<'_> {
/// let ndsp = Ndsp::new()?;
/// let mut channel_0 = ndsp.channel(0)?;
///
/// # let audio_data = Box::new_in([0u8; 96], LinearAllocator);
/// # let audio_data: Box<[_], _> = Box::new_in([0u8; 96], LinearAllocator);
///
/// // Provide your own audio data.
/// let mut wave = Wave::new(audio_data, AudioFormat::PCM16Stereo, false);
Expand All @@ -537,7 +538,10 @@ impl Channel<'_> {
// TODO: Find a better way to handle the wave lifetime problem.
// These "alive wave" shenanigans are the most substantial reason why I'd like to fully re-write this service in Rust.
#[doc(alias = "ndspChnWaveBufAdd")]
pub fn queue_wave(&mut self, wave: &mut Wave) -> std::result::Result<(), Error> {
pub fn queue_wave<Buffer: LinearAllocation + AsRef<[u8]>>(
&mut self,
wave: &mut Wave<Buffer>,
) -> std::result::Result<(), Error> {
match wave.status() {
Status::Playing | Status::Queued => return Err(Error::WaveBusy(self.id)),
_ => (),
Expand Down
48 changes: 27 additions & 21 deletions ctru-rs/src/services/ndsp/wave.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@
//! This modules has all methods and structs required to work with audio waves meant to be played via the [`ndsp`](crate::services::ndsp) service.

use super::{AudioFormat, Error};
use crate::linear::LinearAllocator;
use crate::linear::LinearAllocation;

/// Informational struct holding the raw audio data and playback info.
///
/// You can play audio [`Wave`]s by using [`Channel::queue_wave()`](super::Channel::queue_wave).
pub struct Wave {
pub struct Wave<Buffer: LinearAllocation + AsRef<[u8]>> {
/// Data block of the audio wave (and its format information).
buffer: Box<[u8], LinearAllocator>,
buffer: Buffer,
audio_format: AudioFormat,
// Holding the data with the raw format is necessary since `libctru` will access it.
pub(crate) raw_data: ctru_sys::ndspWaveBuf,
Expand All @@ -31,7 +31,10 @@ pub enum Status {
Done = ctru_sys::NDSP_WBUF_DONE,
}

impl Wave {
impl<Buffer> Wave<Buffer>
where
Buffer: LinearAllocation + AsRef<[u8]>,
{
/// Build a new playable wave object from a raw buffer on [LINEAR memory](`crate::linear`) and some info.
///
/// # Example
Expand All @@ -45,26 +48,23 @@ impl Wave {
/// use ctru::services::ndsp::{AudioFormat, wave::Wave};
///
/// // Zeroed box allocated in the LINEAR memory.
/// let audio_data = Box::new_in([0u8; 96], LinearAllocator);
/// let audio_data: Box<[_], _> = Box::new_in([0u8; 96], LinearAllocator);
///
/// let wave = Wave::new(audio_data, AudioFormat::PCM16Stereo, false);
/// # }
/// ```
pub fn new(
buffer: Box<[u8], LinearAllocator>,
audio_format: AudioFormat,
looping: bool,
) -> Self {
let sample_count = buffer.len() / audio_format.size();
pub fn new(buffer: Buffer, audio_format: AudioFormat, looping: bool) -> Self {
let buf = buffer.as_ref();
let sample_count = buf.len() / audio_format.size();

// Signal to the DSP processor the buffer's RAM sector.
// This step may seem delicate, but testing reports failure most of the time, while still having no repercussions on the resulting audio.
unsafe {
let _r = ctru_sys::DSP_FlushDataCache(buffer.as_ptr().cast(), buffer.len() as u32);
let _r = ctru_sys::DSP_FlushDataCache(buf.as_ptr().cast(), buf.len() as u32);
}

let address = ctru_sys::tag_ndspWaveBuf__bindgen_ty_1 {
data_vaddr: buffer.as_ptr().cast(),
data_vaddr: buf.as_ptr().cast(),
};

let raw_data = ctru_sys::ndspWaveBuf {
Expand All @@ -89,7 +89,7 @@ impl Wave {

/// Returns a slice to the audio data (on the LINEAR memory).
pub fn get_buffer(&self) -> &[u8] {
&self.buffer
self.buffer.as_ref()
}

/// Returns a mutable slice to the audio data (on the LINEAR memory).
Expand All @@ -98,12 +98,15 @@ impl Wave {
///
/// This function will return an error if the [`Wave`] is currently busy,
/// with the id to the channel in which it's queued.
pub fn get_buffer_mut(&mut self) -> Result<&mut [u8], Error> {
pub fn get_buffer_mut(&mut self) -> Result<&mut [u8], Error>
where
Buffer: AsMut<[u8]>,
{
match self.status() {
Status::Playing | Status::Queued => {
Err(Error::WaveBusy(self.played_on_channel.unwrap()))
}
_ => Ok(&mut self.buffer),
_ => Ok(self.buffer.as_mut()),
}
}

Expand All @@ -117,7 +120,7 @@ impl Wave {
/// # let _runner = test_runner::GdbRunner::default();
/// #
/// # use ctru::linear::LinearAllocator;
/// # let _audio_data = Box::new_in([0u8; 96], LinearAllocator);
/// # let _audio_data: Box<[_], _> = Box::new_in([0u8; 96], LinearAllocator);
/// #
/// use ctru::services::ndsp::{AudioFormat, wave::{Wave, Status}};
///
Expand Down Expand Up @@ -173,7 +176,7 @@ impl Wave {
_ => (),
}

let max_count = self.buffer.len() / self.audio_format.size();
let max_count = self.buffer.as_ref().len() / self.audio_format.size();

if sample_count > max_count {
return Err(Error::SampleCountOutOfBounds(sample_count, max_count));
Expand All @@ -199,7 +202,10 @@ impl TryFrom<u8> for Status {
}
}

impl Drop for Wave {
impl<Buffer> Drop for Wave<Buffer>
where
Buffer: LinearAllocation + AsRef<[u8]>,
{
fn drop(&mut self) {
// This was the only way I found I could check for improper drops of `Wave`.
// A panic was considered, but it would cause issues with drop order against `Ndsp`.
Expand All @@ -216,8 +222,8 @@ impl Drop for Wave {
// Flag the buffer's RAM sector as unused
// This step has no real effect in normal applications and is skipped even by devkitPRO's own examples.
let _r = ctru_sys::DSP_InvalidateDataCache(
self.buffer.as_ptr().cast(),
self.buffer.len().try_into().unwrap(),
self.get_buffer().as_ptr().cast(),
self.get_buffer().len().try_into().unwrap(),
);
}
}
Expand Down
Loading